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

8 Oct 2020


      On Wed, Oct 07, 2020 at 08:46:45PM +0000, Ertman, David M wrote:
...
...
-----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?
Let me summarize.
1. You are not providing driver/core API but simplification and obfuscation
of basic primitives and structures. This is new layer. There is no room for
a claim that we must to follow internal API.
2. API should be symmetric. If you call to _register()/_add(), you will need
to call to _unregister()/_del(). Please don't add obscure _put().
3. You can't "ask" from users to call internal calls (put_device) over internal
fields in ancillary_device.
4. This API should be clear to drivers authors, "device_add()" call (and
semantic) is not used by the drivers (git grep " device_add(" drivers/).
Thanks
...
-DaveE