On 10/7/20 4:22 PM, Ertman, David M wrote:
-----Original Message-----
From: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>
Sent: Wednesday, October 7, 2020 1:59 PM
To: Ertman, David M <david.m.ertman@xxxxxxxxx>; Parav Pandit
<parav@xxxxxxxxxx>; Leon Romanovsky <leon@xxxxxxxxxx>
Cc: alsa-devel@xxxxxxxxxxxxxxxx; parav@xxxxxxxxxxxx; tiwai@xxxxxxx;
netdev@xxxxxxxxxxxxxxx; ranjani.sridharan@xxxxxxxxxxxxxxx;
fred.oh@xxxxxxxxxxxxxxx; linux-rdma@xxxxxxxxxxxxxxx;
dledford@xxxxxxxxxx; broonie@xxxxxxxxxx; Jason Gunthorpe
<jgg@xxxxxxxxxx>; gregkh@xxxxxxxxxxxxxxxxxxx; kuba@xxxxxxxxxx; Williams,
Dan J <dan.j.williams@xxxxxxxxx>; Saleem, Shiraz
<shiraz.saleem@xxxxxxxxx>; davem@xxxxxxxxxxxxx; Patil, Kiran
<kiran.patil@xxxxxxxxx>
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.
