-----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?
-DaveE
+EXPORT_SYMBOL_GPL(__ancillary_device_add);
+static int ancillary_probe_driver(struct device *dev) +{
- struct ancillary_driver *ancildrv = to_ancillary_drv(dev->driver);
- struct ancillary_device *ancildev = to_ancillary_dev(dev);
- int ret;
- ret = dev_pm_domain_attach(dev, true);
- if (ret) {
dev_warn(dev, "Failed to attach to PM Domain : %d\n", ret);return ret;- }
- ret = ancildrv->probe(ancildev, ancillary_match_id(ancildrv-
id_table, ancildev));
I don't think that you need to call ->probe() if ancillary_match_id() returned NULL and probably that check should be done before dev_pm_domain_attach().
we'll look into this.
- if (ret)
dev_pm_domain_detach(dev, true);- return ret;
+}
+static int ancillary_remove_driver(struct device *dev) +{
- struct ancillary_driver *ancildrv = to_ancillary_drv(dev->driver);
- struct ancillary_device *ancildev = to_ancillary_dev(dev);
- int ret;
- ret = ancildrv->remove(ancildev);
- dev_pm_domain_detach(dev, true);
- return ret;
You returned an error to user and detached from PM, what will user do with this information? Should user ignore it? retry?
That comment was also provided in earlier reviews. In practice the error is typically ignored so there was a suggestion to move the return type to void, that could be done if this was desired by the majority.
+1 from me.
[...]
diff --git a/include/linux/mod_devicetable.h
b/include/linux/mod_devicetable.h
index 5b08a473cdba..7d596dc30833 100644 --- a/include/linux/mod_devicetable.h +++ b/include/linux/mod_devicetable.h @@ -838,4 +838,12 @@ struct mhi_device_id { kernel_ulong_t driver_data; };
+#define ANCILLARY_NAME_SIZE 32 +#define ANCILLARY_MODULE_PREFIX "ancillary:"
+struct ancillary_device_id {
- char name[ANCILLARY_NAME_SIZE];
I hope that this be enough.
Are you suggesting a different value to allow for a longer string?
I have no idea, but worried that there were no checks at all if name is more than 32. Maybe compiler warn about it?
Thanks