-----Original Message----- From: Alsa-devel alsa-devel-bounces@alsa-project.org On Behalf Of Leon Romanovsky Sent: Wednesday, October 7, 2020 12:26 PM To: Ertman, David M david.m.ertman@intel.com Cc: alsa-devel@alsa-project.org; parav@mellanox.com; tiwai@suse.de; netdev@vger.kernel.org; ranjani.sridharan@linux.intel.com; Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com; fred.oh@linux.intel.com; linux-rdma@vger.kernel.org; dledford@redhat.com; broonie@kernel.org; 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
On Wed, Oct 07, 2020 at 06:06:30PM +0000, Ertman, David M wrote:
-----Original Message----- From: Leon Romanovsky leon@kernel.org Sent: Tuesday, October 6, 2020 10:03 AM To: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Cc: Ertman, David M david.m.ertman@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; 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
On Tue, Oct 06, 2020 at 10:18:07AM -0500, Pierre-Louis Bossart wrote:
Thanks for the review Leon.
Add support for the Ancillary Bus, ancillary_device and
ancillary_driver.
It enables drivers to create an ancillary_device and bind an ancillary_driver to it.
I was under impression that this name is going to be changed.
It's part of the opens stated in the cover letter.
ok, so what are the variants? system bus (sysbus), sbsystem bus (subbus), crossbus ?
[...]
- const struct my_driver my_drv = {
.ancillary_drv = {.driver = {.name = "myancillarydrv",Why do we need to give control over driver name to the driver
authors?
It can be problematic if author puts name that already exists.
Good point. When I used the ancillary_devices for my own SoundWire
test,
the
driver name didn't seem specifically meaningful but needed to be set to something, what mattered was the id_table. Just thinking aloud, maybe
we
can
add prefixing with KMOD_BUILD, as we've done already to avoid
collisions
between device names?
IMHO, it shouldn't be controlled by the drivers at all and need to have kernel module name hardwired. Users will use it later for various bind/unbind/autoprobe tricks and it will give predictability for them.
[...]
+int __ancillary_device_add(struct ancillary_device *ancildev, const
char *modname)
+{
- struct device *dev = &ancildev->dev;
- int ret;
- if (!modname) {
pr_err("ancillary device modname is NULL\n");return -EINVAL;- }
- ret = dev_set_name(dev, "%s.%s.%d", modname, ancildev-
name,
ancildev->id);
- if (ret) {
pr_err("ancillary device dev_set_name failed: %d\n",ret);
return ret;- }
- ret = device_add(dev);
- if (ret)
dev_err(dev, "adding ancillary device failed!: %d\n",ret);
- return ret;
+}
Sorry, but this is very strange API that requires users to put internal call to "dev" that is buried inside "struct ancillary_device".
For example in your next patch, you write this "put_device(&cdev-
ancildev.dev);"
I'm pretty sure that the amount of bugs in error unwind will be astonishing, so if you are doing wrappers over core code, better do
not
pass complexity to the users.
In initial reviews, there was pushback on adding wrappers that don't do anything except for a pointer indirection.
Others had concerns that the API wasn't balanced and blurring layers.
Are you talking about internal review or public? If it is public, can I get a link to it?
Both points have merits IMHO. Do we want wrappers for everything
and
completely hide the low-level device?
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() return err; }
And prefer this flow: ancillary_device_initialize() if (ancillary_device_add()) { ancillary_device_unregister() return err; }
In this way, the ancillary users won't need to do non-intuitive put_device();
Isn't there a problem calling device_unregister() if device_add() fails? device_unregister() does a device_del() and if the device_add() failed there is nothing to delete?
-DaveE
Thanks