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();
Thanks