[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <357d9576-f79a-0ea6-eee4-292e27597565@linux.intel.com>
Date: Wed, 24 Mar 2021 09:55:01 -0500
From: Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
To: Greg KH <gregkh@...uxfoundation.org>
Cc: alsa-devel@...a-project.org, vinod.koul@...aro.org,
linux-kernel@...r.kernel.org, hui.wang@...onical.com,
vkoul@...nel.org, srinivas.kandagatla@...aro.org,
sanyog.r.kale@...el.com,
Bard Liao <yung-chuan.liao@...ux.intel.com>,
rander.wang@...ux.intel.com, bard.liao@...el.com
Subject: Re: [PATCH] soundwire: intel: move to auxiliary bus
>>>>>> Note that the auxiliary bus API has separate init and add steps, which
>>>>>> requires more attention in the error unwinding paths. The main loop
>>>>>> needs to deal with kfree() and auxiliary_device_uninit() for the
>>>>>> current iteration before jumping to the common label which releases
>>>>>> everything allocated in prior iterations.
>>>>>
>>>>> The init/add steps can be moved together in the aux bus code if that
>>>>> makes this usage simpler. Please do that instead.
>>>>
>>>> IIRC the two steps were separated during the auxbus reviews to allow the
>>>> parent to call kfree() on an init failure, and auxiliary_device_uninit()
>>>> afterwards.
>>>>
>>>> https://www.kernel.org/doc/html/latest/driver-api/auxiliary_bus.html#auxiliary-device
>>>>
>>>> With a single auxbus_register(), the parent wouldn't know whether to use
>>>> kfree() or auxiliary_device_uinit() when an error is returned, would it?
>>>>
>>>
>>> It should, you know the difference when you call device_register() vs.
>>> device_initialize()/device_add(), for what to do, right?
>>>
>>> Should be no difference here either :)
>>
>> sorry, not following.
>>
>> with the regular devices, the errors can only happen on the second "add"
>> stage.
>>
>> int device_register(struct device *dev)
>> {
>> device_initialize(dev);
>> return device_add(dev);
>> }
>>
>> that's not what is currently implemented for the auxiliary bus
>>
>> the current flow is
>>
>> ldev = kzalloc(..)
>> some inits
>> ret = auxiliary_device_init(&ldev->auxdev)
>> if (ret < 0) {
>> kfree(ldev);
>> goto err1;
>> }
>>
>> ret = auxiliary_device_add(&ldev->auxdev)
>> if (ret < 0)
>> auxiliary_device_uninit(&ldev->auxdev)
>> goto err2;
>> }
>> ...
>> err2:
>> err1:
>>
>> How would I convert this to
>>
>> ldev = kzalloc(..)
>> some inits
>> ret = auxiliary_device_register()
>> if (ret) {
>> kfree(ldev) or not?
>> unit or not?
>> }
>>
>> IIRC during reviews there was an ask that the parent and name be checked,
>> and that's why the code added the two checks below:
>>
>> int auxiliary_device_init(struct auxiliary_device *auxdev)
>> {
>> struct device *dev = &auxdev->dev;
>>
>> if (!dev->parent) {
>> pr_err("auxiliary_device has a NULL dev->parent\n");
>> return -EINVAL;
>> }
>>
>> if (!auxdev->name) {
>> pr_err("auxiliary_device has a NULL name\n");
>> return -EINVAL;
>> }
>>
>> dev->bus = &auxiliary_bus_type;
>> device_initialize(&auxdev->dev);
>> return 0;
>> }
>>
>> does this clarify the sequence?
>
> Yes, thanks, but I don't know the answer to your question, sorry. This
> feels more complex than it should be, but I do not have the time at the
> moment to look into it, sorry.
>
> Try getting the authors of this code to fix it up :)
We can try to check why those two tests were added before initialize(),
I don't fully recall these details
If we could move these tests after device_initialize() then we could add
a _register function.
Note at this point it would mean an API change and impact the existing
Nvidia/Mellanox code, we are using the same sequence as them
https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/mellanox/mlx5/core/dev.c#L262
Powered by blists - more mailing lists