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

8 Oct 2020


      ...
-----Original Message-----
From: Dan Williams dan.j.williams@intel.com
Sent: Wednesday, October 7, 2020 11:32 PM
To: Leon Romanovsky leon@kernel.org
Cc: Ertman, David M david.m.ertman@intel.com; Parav Pandit
parav@nvidia.com; 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; 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
On Wed, Oct 7, 2020 at 10:21 PM Leon Romanovsky leon@kernel.org
wrote:
...
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.

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.
Yes, this a driver core api, Greg even questioned why it was in
drivers/bus instead of drivers/base which I think makes sense.
Will move to drivers/base with next patch set.
-DaveE