RE: [PATCH v2 1/6] Add ancillary bus support

7 Oct 2020


      ...
-----Original Message-----
From: Parav Pandit parav@nvidia.com
Sent: Wednesday, October 7, 2020 1:17 PM
To: Leon Romanovsky leon@kernel.org; Ertman, David M
david.m.ertman@intel.com
Cc: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com; alsa-
devel@alsa-project.org; parav@mellanox.com; tiwai@suse.de;
netdev@vger.kernel.org; ranjani.sridharan@linux.intel.com;
fred.oh@linux.intel.com; linux-rdma@vger.kernel.org;
dledford@redhat.com; broonie@kernel.org; Jason Gunthorpe
jgg@nvidia.com; gregkh@linuxfoundation.org; kuba@kernel.org; Williams,
Dan J dan.j.williams@intel.com; Saleem, Shiraz
shiraz.saleem@intel.com; davem@davemloft.net; Patil, Kiran
kiran.patil@intel.com
Subject: RE: [PATCH v2 1/6] Add ancillary bus support
...
From: Leon Romanovsky leon@kernel.org
Sent: Thursday, October 8, 2020 12:56 AM
...
...
This API is partially obscures low level driver-core code and needs
to provide clear and proper abstractions without need to remember
about put_device. There is already _add() interface why don't you do
put_device() in it?
The pushback Pierre is referring to was during our mid-tier internal
review.  It was primarily a concern of Parav as I recall, so he can speak to
his
...
reasoning.
...
What we originally had was a single API call
(ancillary_device_register) that started with a call to
device_initialize(), and every error path out of the function performed a
put_device().
...
Is this the model you have in mind?
I don't like this flow:
ancillary_device_initialize()
if (ancillary_ancillary_device_add()) {
  put_device(....)
  ancillary_device_unregister()
Calling device_unregister() is incorrect, because add() wasn't successful.
Only put_device() or a wrapper ancillary_device_put() is necessary.
...
return err;
}
And prefer this flow:
ancillary_device_initialize()
if (ancillary_device_add()) {
  ancillary_device_unregister()
This is incorrect and a clear deviation from the current core APIs that adds the
confusion.
...
return err;
}
In this way, the ancillary users won't need to do non-intuitive put_device();
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.
At that point, do we need wrappers on the primitives init, add, del,
and put?
-DaveE