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

[Greg KH <gregkh@linuxfoundation.org>]

On Tue, Mar 23, 2021 at 02:14:18PM -0500, Pierre-Louis Bossart wrote:
> 
> 
> 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?

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 :)

thanks,

greg k-h