Re: [PATCH] soundwire: intel: move to auxiliary bus

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

On 3/23/21 1:32 PM, Greg KH wrote:
> On Tue, Mar 23, 2021 at 01:04:49PM -0500, Pierre-Louis Bossart wrote:
>>
>>>> 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?