[PATCH v2 1/6] Add ancillary bus support

[Ertman, David M]

> -----Original Message-----
> From: Parav Pandit <parav at nvidia.com>
> Sent: Wednesday, October 7, 2020 9:56 PM
> To: Pierre-Louis Bossart <pierre-louis.bossart at linux.intel.com>; Ertman,
> David M <david.m.ertman at intel.com>; Leon Romanovsky
> <leon at kernel.org>
> Cc: alsa-devel at alsa-project.org; parav at mellanox.com; tiwai at suse.de;
> netdev at vger.kernel.org; ranjani.sridharan at linux.intel.com;
> fred.oh at linux.intel.com; linux-rdma at vger.kernel.org;
> dledford at redhat.com; broonie at kernel.org; Jason Gunthorpe
> <jgg at nvidia.com>; gregkh at linuxfoundation.org; kuba at kernel.org; Williams,
> Dan J <dan.j.williams at intel.com>; Saleem, Shiraz
> <shiraz.saleem at intel.com>; davem at davemloft.net; Patil, Kiran
> <kiran.patil at intel.com>
> Subject: RE: [PATCH v2 1/6] Add ancillary bus support
> 
> 
> 
> > From: Pierre-Louis Bossart <pierre-louis.bossart at linux.intel.com>
> > Sent: Thursday, October 8, 2020 3:20 AM
> >
> >
> > On 10/7/20 4:22 PM, Ertman, David M wrote:
> > >> -----Original Message-----
> > >> From: Pierre-Louis Bossart <pierre-louis.bossart at linux.intel.com>
> > >> Sent: Wednesday, October 7, 2020 1:59 PM
> > >> To: Ertman, David M <david.m.ertman at intel.com>; Parav Pandit
> > >> <parav at nvidia.com>; Leon Romanovsky <leon at kernel.org>
> > >> Cc: alsa-devel at alsa-project.org; parav at mellanox.com; tiwai at suse.de;
> > >> netdev at vger.kernel.org; ranjani.sridharan at linux.intel.com;
> > >> fred.oh at linux.intel.com; linux-rdma at vger.kernel.org;
> > >> dledford at redhat.com; broonie at kernel.org; Jason Gunthorpe
> > >> <jgg at nvidia.com>; gregkh at linuxfoundation.org; kuba at kernel.org;
> > >> Williams, Dan J <dan.j.williams at intel.com>; Saleem, Shiraz
> > >> <shiraz.saleem at intel.com>; davem at davemloft.net; Patil, Kiran
> > >> <kiran.patil at intel.com>
> > >> Subject: Re: [PATCH v2 1/6] Add ancillary bus support
> > >>
> > >>
> > >>
> > >>>> Below is most simple, intuitive and matching with core APIs for
> > >>>> name and design pattern wise.
> > >>>> init()
> > >>>> {
> > >>>> 	err = ancillary_device_initialize();
> > >>>> 	if (err)
> > >>>> 		return ret;
> > >>>>
> > >>>> 	err = ancillary_device_add();
> > >>>> 	if (ret)
> > >>>> 		goto err_unwind;
> > >>>>
> > >>>> 	err = some_foo();
> > >>>> 	if (err)
> > >>>> 		goto err_foo;
> > >>>> 	return 0;
> > >>>>
> > >>>> err_foo:
> > >>>> 	ancillary_device_del(adev);
> > >>>> err_unwind:
> > >>>> 	ancillary_device_put(adev->dev);
> > >>>> 	return err;
> > >>>> }
> > >>>>
> > >>>> cleanup()
> > >>>> {
> > >>>> 	ancillary_device_de(adev);
> > >>>> 	ancillary_device_put(adev);
> > >>>> 	/* It is common to have a one wrapper for this as
> > >>>> ancillary_device_unregister().
> > >>>> 	 * This will match with core device_unregister() that has precise
> > >>>> documentation.
> > >>>> 	 * but given fact that init() code need proper error unwinding,
> > >>>> like above,
> > >>>> 	 * it make sense to have two APIs, and no need to export another
> > >>>> symbol for unregister().
> > >>>> 	 * This pattern is very easy to audit and code.
> > >>>> 	 */
> > >>>> }
> > >>>
> > >>> I like this flow +1
> > >>>
> > >>> But ... since the init() function is performing both device_init and
> > >>> device_add - it should probably be called ancillary_device_register,
> > >>> and we are back to a single exported API for both register and
> > >>> unregister.
> > >>
> > >> Kind reminder that we introduced the two functions to allow the
> > >> caller to know if it needed to free memory when initialize() fails,
> > >> and it didn't need to free memory when add() failed since
> > >> put_device() takes care of it. If you have a single init() function
> > >> it's impossible to know which behavior to select on error.
> > >>
> > >> I also have a case with SoundWire where it's nice to first
> > >> initialize, then set some data and then add.
> > >>
> > >
> > > The flow as outlined by Parav above does an initialize as the first
> > > step, so every error path out of the function has to do a
> > > put_device(), so you would never need to manually free the memory in
> > the setup function.
> > > It would be freed in the release call.
> >
> > err = ancillary_device_initialize();
> > if (err)
> > 	return ret;
> >
> > where is the put_device() here? if the release function does any sort of
> > kfree, then you'd need to do it manually in this case.
> Since device_initialize() failed, put_device() cannot be done here.
> So yes, pseudo code should have shown,
> if (err) {
> 	kfree(adev);
> 	return err;
> }
> 
> If we just want to follow register(), unregister() pattern,
> 
> Than,
> 
> ancillar_device_register() should be,
> 
> /**
>  * ancillar_device_register() - register an ancillary device
>  * NOTE: __never directly free @adev after calling this function, even if it
> returned
>  * an error. Always use ancillary_device_put() to give up the reference
> initialized by this function.
>  * This note matches with the core and caller knows exactly what to be done.
>  */
> ancillary_device_register()
> {
> 	device_initialize(&adev->dev);
> 	if (!dev->parent || !adev->name)
> 		return -EINVAL;
> 	if (!dev->release && !(dev->type && dev->type->release)) {
> 		/* core is already capable and throws the warning when
> release callback is not set.
> 		 * It is done at drivers/base/core.c:1798.
> 		 * For NULL release it says, "does not have a release()
> function, it is broken and must be fixed"
> 		 */
> 		return -EINVAL;
> 	}
That code is in device_release().  Because of this check we will never hit that code.

We either need to leave the error message here, or if we are going to rely on the core
to find this condition at the end of the process, then we need to completely remove
this check from the registration flow.

-DaveE