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

[Pierre-Louis Bossart]

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