On Sat 15 Feb 2025 at 07:53, Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
[...]
>>
>> >
>> >> + int id)
>> >> +{
>> >> + struct auxiliary_device *auxdev;
>> >> + int ret;
>> >> +
>> >> + auxdev = kzalloc(sizeof(*auxdev), GFP_KERNEL);
>> >> + if (!auxdev)
>> >> + return ERR_PTR(-ENOMEM);
>> >
>> > Ick, who cares what the error value really is? Why not just do NULL or
>> > a valid pointer? That makes the caller much simpler to handle, right?
>> >
>>
>> Sure why not
I have tried the 'NULL or valid' approach. In the consumers,
which mostly return an integer from their various init function, I got
this weird to come up with one from NULL. EINVAL, ENOMEM, etc ... can't
really pick one.
It is actually easier to pass something along.
>>
>> >> +
>> >> + auxdev->id = id;
>> >> + auxdev->name = devname;
>> >> + auxdev->dev.parent = dev;
>> >> + auxdev->dev.platform_data = platform_data;
>> >> + auxdev->dev.release = auxiliary_device_release;
>> >> + device_set_of_node_from_dev(&auxdev->dev, dev);
>> >> +
>> >> + ret = auxiliary_device_init(auxdev);
>> >
>> > Only way this will fail is if you forgot to set parent or a valid name.
>> > So why not check for devname being non-NULL above this?
>>
>> If auxiliary_device_init() ever changes it would be easy to forget to
>> update that and lead to something nasty to debug, don't you think ?
>
> Yes, this is being more defensive, I take back my objection, thanks.
>
>> >> + if (ret) {
>> >> + kfree(auxdev);
>> >> + return ERR_PTR(ret);
>> >> + }
>> >> +
>> >> + ret = __auxiliary_device_add(auxdev, modname);
>> >> + if (ret) {
>> >> + /*
>> >> + * NOTE: It may look odd but auxdev should not be freed
>> >> + * here. auxiliary_device_uninit() calls device_put()
>> >> + * which call the device release function, freeing auxdev.
>> >> + */
>> >> + auxiliary_device_uninit(auxdev);
>> >
>> > Yes it is odd, are you SURE you should be calling device_del() on the
>> > device if this fails? auxiliary_device_uninit(), makes sense so why not
>> > just call that here?
>>
>> I'm confused ... I am call auxiliary_device_uninit() here. What do you
>> mean ?
>
> Oh wow, I got this wrong, sorry, I was thinking you were calling
> auxiliary_device_destroy(). Nevermind, ugh, it was a long day...
>
No worries
> thanks,
>
> greg k-h
--
Jerome
