| Message ID | YmSpdxaDNeC2BBOf@rowland.harvard.edu |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
Hi Alan, On Sat, 23 Apr 2022, Alan Stern wrote: > This patch adds a "gadget" bus and uses it for registering gadgets and > their drivers. From now on, bindings will be managed by the driver > core rather than through ad-hoc manipulations in the UDC core. > > As part of this change, the driver_pending_list is removed. The UDC > core won't need to keep track of unbound drivers for later binding, > because the driver core handles all of that for us. > > However, we do need one new feature: a way to prevent gadget drivers > from being bound to more than one gadget at a time. The existing code > does this automatically, but the driver core doesn't -- it's perfectly > happy to bind a single driver to all the matching devices on the bus. > The patch adds a new bitflag to the usb_gadget_driver structure for > this purpose. > > A nice side effect of this change is a reduction in the total lines of > code, since now the driver core will do part of the work that the UDC > used to do. > > A possible future patch could add udc devices to the gadget bus, say > as a separate device type. > > Signed-off-by: Alan Stern <stern@rowland.harvard.edu> Thanks for your patch, which is now commit fc274c1e997314bf ("USB: gadget: Add a new bus for gadgets") in usb-next. This patch cause a regression on the Renesas Salvator-XS development board, as R-Car H3 has multiple USB gadget devices: sysfs: cannot create duplicate filename '/bus/gadget/devices/gadget' CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.18.0-rc1-arm64-renesas-00074-gfc274c1e9973 #1587 Hardware name: Renesas Salvator-X 2nd version board based on r8a77951 (DT) Call trace: dump_backtrace+0xcc/0xd8 show_stack+0x14/0x30 dump_stack_lvl+0x88/0xb0 dump_stack+0x14/0x2c sysfs_warn_dup+0x60/0x78 sysfs_do_create_link_sd.isra.0+0xe4/0xf0 sysfs_create_link+0x20/0x40 bus_add_device+0x64/0x110 device_add+0x31c/0x850 usb_add_gadget+0x124/0x1a0 usb_add_gadget_udc_release+0x1c/0x50 usb_add_gadget_udc+0x10/0x18 renesas_usb3_probe+0x450/0x728 platform_probe+0x64/0xd0 really_probe+0x100/0x2a0 __driver_probe_device+0x74/0xd8 driver_probe_device+0x3c/0xe0 __driver_attach+0x80/0x110 bus_for_each_dev+0x6c/0xc0 driver_attach+0x20/0x28 bus_add_driver+0x138/0x1e0 driver_register+0x60/0x110 __platform_driver_register+0x24/0x30 renesas_usb3_driver_init+0x18/0x20 do_one_initcall+0x15c/0x31c kernel_init_freeable+0x2f0/0x354 kernel_init+0x20/0x120 ret_from_fork+0x10/0x20 renesas_usb3: probe of ee020000.usb failed with error -17 ... renesas_usbhs: probe of e6590000.usb failed with error -17 After boot-up, only one gadget device is visible: root@h3-salvator-xs:~# ls -l /sys/bus/gadget/devices/ total 0 lrwxrwxrwx 1 root root 0 Feb 14 2019 gadget -> ../../../devices/platform/soc/e659c000.usb/gadget root@h3-salvator-xs:~# Reverting this patch fixes the issue. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Tue, May 03, 2022 at 11:48:33AM -0400, Alan Stern wrote: > On Tue, May 03, 2022 at 05:27:08PM +0200, Geert Uytterhoeven wrote: > > Hi Alan, > > > > On Tue, May 3, 2022 at 5:14 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > > On Tue, May 03, 2022 at 12:14:30PM +0200, Geert Uytterhoeven wrote: > > > > On Sat, 23 Apr 2022, Alan Stern wrote: > > > > > This patch adds a "gadget" bus and uses it for registering gadgets and > > > > > their drivers. From now on, bindings will be managed by the driver > > > > > core rather than through ad-hoc manipulations in the UDC core. > > > > > > > > > > As part of this change, the driver_pending_list is removed. The UDC > > > > > core won't need to keep track of unbound drivers for later binding, > > > > > because the driver core handles all of that for us. > > > > > > > > > > However, we do need one new feature: a way to prevent gadget drivers > > > > > from being bound to more than one gadget at a time. The existing code > > > > > does this automatically, but the driver core doesn't -- it's perfectly > > > > > happy to bind a single driver to all the matching devices on the bus. > > > > > The patch adds a new bitflag to the usb_gadget_driver structure for > > > > > this purpose. > > > > > > > > > > A nice side effect of this change is a reduction in the total lines of > > > > > code, since now the driver core will do part of the work that the UDC > > > > > used to do. > > > > > > > > > > A possible future patch could add udc devices to the gadget bus, say > > > > > as a separate device type. > > > > > > > > > > Signed-off-by: Alan Stern <stern@rowland.harvard.edu> > > > > > > > > Thanks for your patch, which is now commit fc274c1e997314bf ("USB: > > > > gadget: Add a new bus for gadgets") in usb-next. > > > > > > > > This patch cause a regression on the Renesas Salvator-XS development > > > > board, as R-Car H3 has multiple USB gadget devices: > > > > > > Then these gadgets ought to have distinct names in order to avoid the > > > conflict below: > > > > > > > sysfs: cannot create duplicate filename '/bus/gadget/devices/gadget' > > > > CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.18.0-rc1-arm64-renesas-00074-gfc274c1e9973 #1587 > > > > Hardware name: Renesas Salvator-X 2nd version board based on r8a77951 (DT) > > > > Call trace: > > > > dump_backtrace+0xcc/0xd8 > > > > show_stack+0x14/0x30 > > > > dump_stack_lvl+0x88/0xb0 > > > > dump_stack+0x14/0x2c > > > > sysfs_warn_dup+0x60/0x78 > > > > sysfs_do_create_link_sd.isra.0+0xe4/0xf0 > > > > sysfs_create_link+0x20/0x40 > > > > bus_add_device+0x64/0x110 > > > > device_add+0x31c/0x850 > > > > usb_add_gadget+0x124/0x1a0 > > > > usb_add_gadget_udc_release+0x1c/0x50 > > > > usb_add_gadget_udc+0x10/0x18 > > > > renesas_usb3_probe+0x450/0x728 > > > ... > > > > > > Having three gadget devices, all named "gadget", doesn't seem like a > > > good idea. > > > > I'm not so sure where these names are coming from. > > `git grep '"gadget"'` points to the following likely targets: > > > > drivers/usb/gadget/udc/core.c: dev_set_name(&gadget->dev, "gadget"); > > drivers/usb/renesas_usbhs/mod_gadget.c: gpriv->mod.name = "gadget"; > > > > Changing both names reveals the problem is actually caused by > > the former ;-) > > Ah, good. > > One way to attack this would be to keep a static counter and dynamically > set the name to "gadget.%d" using the counter's value. Or keep a bitmap > of allocated gadget numbers and use the first available number. > > Felipe, Greg, any opinions? Just use an idr structure for the number, that's the simplest way to track that. thanks, greg k-h
On Tue, May 03, 2022 at 11:48:33AM -0400, Alan Stern wrote: > On Tue, May 03, 2022 at 05:27:08PM +0200, Geert Uytterhoeven wrote: > > Hi Alan, > > > > On Tue, May 3, 2022 at 5:14 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > > On Tue, May 03, 2022 at 12:14:30PM +0200, Geert Uytterhoeven wrote: > > > > On Sat, 23 Apr 2022, Alan Stern wrote: > > > > > This patch adds a "gadget" bus and uses it for registering gadgets and > > > > > their drivers. From now on, bindings will be managed by the driver > > > > > core rather than through ad-hoc manipulations in the UDC core. > > > > > > > > > > As part of this change, the driver_pending_list is removed. The UDC > > > > > core won't need to keep track of unbound drivers for later binding, > > > > > because the driver core handles all of that for us. > > > > > > > > > > However, we do need one new feature: a way to prevent gadget drivers > > > > > from being bound to more than one gadget at a time. The existing code > > > > > does this automatically, but the driver core doesn't -- it's perfectly > > > > > happy to bind a single driver to all the matching devices on the bus. > > > > > The patch adds a new bitflag to the usb_gadget_driver structure for > > > > > this purpose. > > > > > > > > > > A nice side effect of this change is a reduction in the total lines of > > > > > code, since now the driver core will do part of the work that the UDC > > > > > used to do. > > > > > > > > > > A possible future patch could add udc devices to the gadget bus, say > > > > > as a separate device type. > > > > > > > > > > Signed-off-by: Alan Stern <stern@rowland.harvard.edu> > > > > > > > > Thanks for your patch, which is now commit fc274c1e997314bf ("USB: > > > > gadget: Add a new bus for gadgets") in usb-next. > > > > > > > > This patch cause a regression on the Renesas Salvator-XS development > > > > board, as R-Car H3 has multiple USB gadget devices: > > > > > > Then these gadgets ought to have distinct names in order to avoid the > > > conflict below: Geert: Can you test the patch below? It ought to fix the problem (although it might end up causing other problems down the line...) Alan Stern Index: usb-devel/drivers/usb/gadget/udc/core.c =================================================================== --- usb-devel.orig/drivers/usb/gadget/udc/core.c +++ usb-devel/drivers/usb/gadget/udc/core.c @@ -12,6 +12,7 @@ #include <linux/module.h> #include <linux/device.h> #include <linux/list.h> +#include <linux/idr.h> #include <linux/err.h> #include <linux/dma-mapping.h> #include <linux/sched/task_stack.h> @@ -23,6 +24,8 @@ #include "trace.h" +static DEFINE_IDA(gadget_id_numbers); + static struct bus_type gadget_bus_type; /** @@ -1248,7 +1251,6 @@ static void usb_udc_nop_release(struct d void usb_initialize_gadget(struct device *parent, struct usb_gadget *gadget, void (*release)(struct device *dev)) { - dev_set_name(&gadget->dev, "gadget"); INIT_WORK(&gadget->work, usb_gadget_state_work); gadget->dev.parent = parent; @@ -1304,12 +1306,21 @@ int usb_add_gadget(struct usb_gadget *ga usb_gadget_set_state(gadget, USB_STATE_NOTATTACHED); udc->vbus = true; + ret = ida_alloc(&gadget_id_numbers, GFP_KERNEL); + if (ret < 0) + goto err_del_udc; + gadget->id_number = ret; + dev_set_name(&gadget->dev, "gadget.%d", ret); + ret = device_add(&gadget->dev); if (ret) - goto err_del_udc; + goto err_free_id; return 0; + err_free_id: + ida_free(&gadget_id_numbers, gadget->id_number); + err_del_udc: flush_work(&gadget->work); device_del(&udc->dev); @@ -1417,6 +1428,7 @@ void usb_del_gadget(struct usb_gadget *g kobject_uevent(&udc->dev.kobj, KOBJ_REMOVE); flush_work(&gadget->work); device_del(&gadget->dev); + ida_free(&gadget_id_numbers, gadget->id_number); device_unregister(&udc->dev); } EXPORT_SYMBOL_GPL(usb_del_gadget); Index: usb-devel/include/linux/usb/gadget.h =================================================================== --- usb-devel.orig/include/linux/usb/gadget.h +++ usb-devel/include/linux/usb/gadget.h @@ -386,6 +386,7 @@ struct usb_gadget_ops { * @lpm_capable: If the gadget max_speed is FULL or HIGH, this flag * indicates that it supports LPM as per the LPM ECN & errata. * @irq: the interrupt number for device controller. + * @id_number: a unique ID number for ensuring that gadget names are distinct * * Gadgets have a mostly-portable "gadget driver" implementing device * functions, handling all usb configurations and interfaces. Gadget @@ -446,6 +447,7 @@ struct usb_gadget { unsigned connected:1; unsigned lpm_capable:1; int irq; + int id_number; }; #define work_to_gadget(w) (container_of((w), struct usb_gadget, work))
Hi Alan, On Sat, May 7, 2022 at 5:36 PM Alan Stern <stern@rowland.harvard.edu> wrote: > On Tue, May 03, 2022 at 11:48:33AM -0400, Alan Stern wrote: > > On Tue, May 03, 2022 at 05:27:08PM +0200, Geert Uytterhoeven wrote: > > > On Tue, May 3, 2022 at 5:14 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > > > On Tue, May 03, 2022 at 12:14:30PM +0200, Geert Uytterhoeven wrote: > > > > > On Sat, 23 Apr 2022, Alan Stern wrote: > > > > > > This patch adds a "gadget" bus and uses it for registering gadgets and > > > > > > their drivers. From now on, bindings will be managed by the driver > > > > > > core rather than through ad-hoc manipulations in the UDC core. > > > > > > > > > > > > As part of this change, the driver_pending_list is removed. The UDC > > > > > > core won't need to keep track of unbound drivers for later binding, > > > > > > because the driver core handles all of that for us. > > > > > > > > > > > > However, we do need one new feature: a way to prevent gadget drivers > > > > > > from being bound to more than one gadget at a time. The existing code > > > > > > does this automatically, but the driver core doesn't -- it's perfectly > > > > > > happy to bind a single driver to all the matching devices on the bus. > > > > > > The patch adds a new bitflag to the usb_gadget_driver structure for > > > > > > this purpose. > > > > > > > > > > > > A nice side effect of this change is a reduction in the total lines of > > > > > > code, since now the driver core will do part of the work that the UDC > > > > > > used to do. > > > > > > > > > > > > A possible future patch could add udc devices to the gadget bus, say > > > > > > as a separate device type. > > > > > > > > > > > > Signed-off-by: Alan Stern <stern@rowland.harvard.edu> > > > > > > > > > > Thanks for your patch, which is now commit fc274c1e997314bf ("USB: > > > > > gadget: Add a new bus for gadgets") in usb-next. > > > > > > > > > > This patch cause a regression on the Renesas Salvator-XS development > > > > > board, as R-Car H3 has multiple USB gadget devices: > > > > > > > > Then these gadgets ought to have distinct names in order to avoid the > > > > conflict below: > > Geert: > > Can you test the patch below? It ought to fix the problem (although it Thanks! root@h3-salvator-xs:~# ls -l /sys/bus/gadget/devices/ total 0 lrwxrwxrwx 1 root root 0 Feb 14 2019 gadget.0 -> ../../../devices/platform/soc/e659c000.usb/gadget.0 lrwxrwxrwx 1 root root 0 Feb 14 2019 gadget.1 -> ../../../devices/platform/soc/ee020000.usb/gadget.1 lrwxrwxrwx 1 root root 0 Feb 14 2019 gadget.2 -> ../../../devices/platform/soc/e6590000.usb/gadget.2 Tested-by: Geert Uytterhoeven <geert+renesas@glider.be> LGTM, so Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > might end up causing other problems down the line...) Can you please elaborate? I'm not too familiar with UBS gadgets. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Mon, May 09, 2022 at 09:46:25AM +0200, Geert Uytterhoeven wrote: > > Geert: > > > > Can you test the patch below? It ought to fix the problem (although it > > Thanks! > > root@h3-salvator-xs:~# ls -l /sys/bus/gadget/devices/ > total 0 > lrwxrwxrwx 1 root root 0 Feb 14 2019 gadget.0 -> > ../../../devices/platform/soc/e659c000.usb/gadget.0 > lrwxrwxrwx 1 root root 0 Feb 14 2019 gadget.1 -> > ../../../devices/platform/soc/ee020000.usb/gadget.1 > lrwxrwxrwx 1 root root 0 Feb 14 2019 gadget.2 -> > ../../../devices/platform/soc/e6590000.usb/gadget.2 > > Tested-by: Geert Uytterhoeven <geert+renesas@glider.be> > > LGTM, so > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Thanks! > > might end up causing other problems down the line...) > > Can you please elaborate? I'm not too familiar with UBS gadgets. I was concerned about the fact that changing the name of a file, directory, or symbolic link in sysfs means changing a user API, and so it might cause some existing programs to fail. That would be a regression. Perhaps the best way to work around the problem is to leave the name set to "gadget" if the ID number is 0, while adding the ID number on to the name if the value is > 0. What do you think? Alan Stern
Hi Alan, On Mon, May 9, 2022 at 4:15 PM Alan Stern <stern@rowland.harvard.edu> wrote: > On Mon, May 09, 2022 at 09:46:25AM +0200, Geert Uytterhoeven wrote: > > > Geert: > > > > > > Can you test the patch below? It ought to fix the problem (although it > > > > Thanks! > > > > root@h3-salvator-xs:~# ls -l /sys/bus/gadget/devices/ > > total 0 > > lrwxrwxrwx 1 root root 0 Feb 14 2019 gadget.0 -> > > ../../../devices/platform/soc/e659c000.usb/gadget.0 > > lrwxrwxrwx 1 root root 0 Feb 14 2019 gadget.1 -> > > ../../../devices/platform/soc/ee020000.usb/gadget.1 > > lrwxrwxrwx 1 root root 0 Feb 14 2019 gadget.2 -> > > ../../../devices/platform/soc/e6590000.usb/gadget.2 > > > > Tested-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > > LGTM, so > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > Thanks! > > > > might end up causing other problems down the line...) > > > > Can you please elaborate? I'm not too familiar with UBS gadgets. > > I was concerned about the fact that changing the name of a file, > directory, or symbolic link in sysfs means changing a user API, and so > it might cause some existing programs to fail. That would be a > regression. > > Perhaps the best way to work around the problem is to leave the name set > to "gadget" if the ID number is 0, while adding the ID number on to the > name if the value is > 0. What do you think? Oh, you mean the "gadget.N" subdirs, which are the targets of the symlinks above? These were indeed named "gadget" before. Would it be possible to append the ".N" suffixes only to the actual symlinks, while keeping the target directory names unchanged? E.g. /sys/bus/gadget/devices/gadget.0 -> ../../../devices/platform/soc/e659c000.usb/gadget Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Mon, May 09, 2022 at 04:42:01PM +0200, Geert Uytterhoeven wrote: > Hi Alan, > > On Mon, May 9, 2022 at 4:15 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > On Mon, May 09, 2022 at 09:46:25AM +0200, Geert Uytterhoeven wrote: > > > > Geert: > > > > > > > > Can you test the patch below? It ought to fix the problem (although it > > > > > > Thanks! > > > > > > root@h3-salvator-xs:~# ls -l /sys/bus/gadget/devices/ > > > total 0 > > > lrwxrwxrwx 1 root root 0 Feb 14 2019 gadget.0 -> > > > ../../../devices/platform/soc/e659c000.usb/gadget.0 > > > lrwxrwxrwx 1 root root 0 Feb 14 2019 gadget.1 -> > > > ../../../devices/platform/soc/ee020000.usb/gadget.1 > > > lrwxrwxrwx 1 root root 0 Feb 14 2019 gadget.2 -> > > > ../../../devices/platform/soc/e6590000.usb/gadget.2 > > > > > > Tested-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > > > > LGTM, so > > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > > Thanks! > > > > > > might end up causing other problems down the line...) > > > > > > Can you please elaborate? I'm not too familiar with UBS gadgets. > > > > I was concerned about the fact that changing the name of a file, > > directory, or symbolic link in sysfs means changing a user API, and so > > it might cause some existing programs to fail. That would be a > > regression. > > > > Perhaps the best way to work around the problem is to leave the name set > > to "gadget" if the ID number is 0, while adding the ID number on to the > > name if the value is > 0. What do you think? > > Oh, you mean the "gadget.N" subdirs, which are the targets of the > symlinks above? These were indeed named "gadget" before. > > Would it be possible to append the ".N" suffixes only to the actual > symlinks, while keeping the target directory names unchanged? > E.g. /sys/bus/gadget/devices/gadget.0 -> > ../../../devices/platform/soc/e659c000.usb/gadget No, it's not possible. Or at least, not without significant changes to the driver core. Besides, people expect these names to be the same. Alan Stern
On Mon, May 09, 2022 at 11:05:06AM -0400, Alan Stern wrote: > On Mon, May 09, 2022 at 04:42:01PM +0200, Geert Uytterhoeven wrote: > > Hi Alan, > > > > On Mon, May 9, 2022 at 4:15 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > > On Mon, May 09, 2022 at 09:46:25AM +0200, Geert Uytterhoeven wrote: > > > > > Geert: > > > > > > > > > > Can you test the patch below? It ought to fix the problem (although it > > > > > > > > Thanks! > > > > > > > > root@h3-salvator-xs:~# ls -l /sys/bus/gadget/devices/ > > > > total 0 > > > > lrwxrwxrwx 1 root root 0 Feb 14 2019 gadget.0 -> > > > > ../../../devices/platform/soc/e659c000.usb/gadget.0 > > > > lrwxrwxrwx 1 root root 0 Feb 14 2019 gadget.1 -> > > > > ../../../devices/platform/soc/ee020000.usb/gadget.1 > > > > lrwxrwxrwx 1 root root 0 Feb 14 2019 gadget.2 -> > > > > ../../../devices/platform/soc/e6590000.usb/gadget.2 > > > > > > > > Tested-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > > > > > > LGTM, so > > > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > > > > Thanks! > > > > > > > > might end up causing other problems down the line...) > > > > > > > > Can you please elaborate? I'm not too familiar with UBS gadgets. > > > > > > I was concerned about the fact that changing the name of a file, > > > directory, or symbolic link in sysfs means changing a user API, and so > > > it might cause some existing programs to fail. That would be a > > > regression. > > > > > > Perhaps the best way to work around the problem is to leave the name set > > > to "gadget" if the ID number is 0, while adding the ID number on to the > > > name if the value is > 0. What do you think? > > > > Oh, you mean the "gadget.N" subdirs, which are the targets of the > > symlinks above? These were indeed named "gadget" before. > > > > Would it be possible to append the ".N" suffixes only to the actual > > symlinks, while keeping the target directory names unchanged? > > E.g. /sys/bus/gadget/devices/gadget.0 -> > > ../../../devices/platform/soc/e659c000.usb/gadget > > No, it's not possible. Or at least, not without significant changes to > the driver core. Besides, people expect these names to be the same. It should always be ok to change the names of devices as those are not going to be persistent / determinisitic. It's the attributes of devices that are supposed to be used to determine those types of things. So I think let's start out with the .N suffix for everything for now. I'll be glad to submit the fixed patch to the Android kernel build system to see what their testing comes back with to see if they happened to make any name assumptions as that's the largest user of this codebase. thanks, greg k-h
On Mon, May 09, 2022 at 06:23:31PM +0200, Greg KH wrote: > On Mon, May 09, 2022 at 11:05:06AM -0400, Alan Stern wrote: > > On Mon, May 09, 2022 at 04:42:01PM +0200, Geert Uytterhoeven wrote: > > > Hi Alan, > > > > > > On Mon, May 9, 2022 at 4:15 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > > > On Mon, May 09, 2022 at 09:46:25AM +0200, Geert Uytterhoeven wrote: > > > > > > Geert: > > > > > > > > > > > > Can you test the patch below? It ought to fix the problem (although it > > > > > > > > > > Thanks! > > > > > > > > > > root@h3-salvator-xs:~# ls -l /sys/bus/gadget/devices/ > > > > > total 0 > > > > > lrwxrwxrwx 1 root root 0 Feb 14 2019 gadget.0 -> > > > > > ../../../devices/platform/soc/e659c000.usb/gadget.0 > > > > > lrwxrwxrwx 1 root root 0 Feb 14 2019 gadget.1 -> > > > > > ../../../devices/platform/soc/ee020000.usb/gadget.1 > > > > > lrwxrwxrwx 1 root root 0 Feb 14 2019 gadget.2 -> > > > > > ../../../devices/platform/soc/e6590000.usb/gadget.2 > > > > > > > > > > Tested-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > > > > > > > > LGTM, so > > > > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > > > > > > Thanks! > > > > > > > > > > might end up causing other problems down the line...) > > > > > > > > > > Can you please elaborate? I'm not too familiar with UBS gadgets. > > > > > > > > I was concerned about the fact that changing the name of a file, > > > > directory, or symbolic link in sysfs means changing a user API, and so > > > > it might cause some existing programs to fail. That would be a > > > > regression. > > > > > > > > Perhaps the best way to work around the problem is to leave the name set > > > > to "gadget" if the ID number is 0, while adding the ID number on to the > > > > name if the value is > 0. What do you think? > > > > > > Oh, you mean the "gadget.N" subdirs, which are the targets of the > > > symlinks above? These were indeed named "gadget" before. > > > > > > Would it be possible to append the ".N" suffixes only to the actual > > > symlinks, while keeping the target directory names unchanged? > > > E.g. /sys/bus/gadget/devices/gadget.0 -> > > > ../../../devices/platform/soc/e659c000.usb/gadget > > > > No, it's not possible. Or at least, not without significant changes to > > the driver core. Besides, people expect these names to be the same. > > It should always be ok to change the names of devices as those are not > going to be persistent / determinisitic. It's the attributes of devices > that are supposed to be used to determine those types of things. > > So I think let's start out with the .N suffix for everything for now. > I'll be glad to submit the fixed patch to the Android kernel build > system to see what their testing comes back with to see if they happened > to make any name assumptions as that's the largest user of this > codebase. Okay. Do you need me to send it as a proper patch before you try it out? Alan Stern
On Mon, May 09, 2022 at 12:47:19PM -0400, Alan Stern wrote: > On Mon, May 09, 2022 at 06:23:31PM +0200, Greg KH wrote: > > On Mon, May 09, 2022 at 11:05:06AM -0400, Alan Stern wrote: > > > On Mon, May 09, 2022 at 04:42:01PM +0200, Geert Uytterhoeven wrote: > > > > Hi Alan, > > > > > > > > On Mon, May 9, 2022 at 4:15 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > > > > On Mon, May 09, 2022 at 09:46:25AM +0200, Geert Uytterhoeven wrote: > > > > > > > Geert: > > > > > > > > > > > > > > Can you test the patch below? It ought to fix the problem (although it > > > > > > > > > > > > Thanks! > > > > > > > > > > > > root@h3-salvator-xs:~# ls -l /sys/bus/gadget/devices/ > > > > > > total 0 > > > > > > lrwxrwxrwx 1 root root 0 Feb 14 2019 gadget.0 -> > > > > > > ../../../devices/platform/soc/e659c000.usb/gadget.0 > > > > > > lrwxrwxrwx 1 root root 0 Feb 14 2019 gadget.1 -> > > > > > > ../../../devices/platform/soc/ee020000.usb/gadget.1 > > > > > > lrwxrwxrwx 1 root root 0 Feb 14 2019 gadget.2 -> > > > > > > ../../../devices/platform/soc/e6590000.usb/gadget.2 > > > > > > > > > > > > Tested-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > > > > > > > > > > LGTM, so > > > > > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > > > > > > > > Thanks! > > > > > > > > > > > > might end up causing other problems down the line...) > > > > > > > > > > > > Can you please elaborate? I'm not too familiar with UBS gadgets. > > > > > > > > > > I was concerned about the fact that changing the name of a file, > > > > > directory, or symbolic link in sysfs means changing a user API, and so > > > > > it might cause some existing programs to fail. That would be a > > > > > regression. > > > > > > > > > > Perhaps the best way to work around the problem is to leave the name set > > > > > to "gadget" if the ID number is 0, while adding the ID number on to the > > > > > name if the value is > 0. What do you think? > > > > > > > > Oh, you mean the "gadget.N" subdirs, which are the targets of the > > > > symlinks above? These were indeed named "gadget" before. > > > > > > > > Would it be possible to append the ".N" suffixes only to the actual > > > > symlinks, while keeping the target directory names unchanged? > > > > E.g. /sys/bus/gadget/devices/gadget.0 -> > > > > ../../../devices/platform/soc/e659c000.usb/gadget > > > > > > No, it's not possible. Or at least, not without significant changes to > > > the driver core. Besides, people expect these names to be the same. > > > > It should always be ok to change the names of devices as those are not > > going to be persistent / determinisitic. It's the attributes of devices > > that are supposed to be used to determine those types of things. > > > > So I think let's start out with the .N suffix for everything for now. > > I'll be glad to submit the fixed patch to the Android kernel build > > system to see what their testing comes back with to see if they happened > > to make any name assumptions as that's the largest user of this > > codebase. > > Okay. Do you need me to send it as a proper patch before you try it > out? Yes please. thanks, greg k-h
Index: usb-devel/drivers/usb/gadget/udc/core.c =================================================================== --- usb-devel.orig/drivers/usb/gadget/udc/core.c +++ usb-devel/drivers/usb/gadget/udc/core.c @@ -23,6 +23,8 @@ #include "trace.h" +static struct bus_type gadget_bus_type; + /** * struct usb_udc - describes one usb device controller * @driver: the gadget driver pointer. For use by the class code @@ -47,11 +49,9 @@ struct usb_udc { static struct class *udc_class; static LIST_HEAD(udc_list); -static LIST_HEAD(gadget_driver_pending_list); -static DEFINE_MUTEX(udc_lock); -static int udc_bind_to_driver(struct usb_udc *udc, - struct usb_gadget_driver *driver); +/* Protects udc_list, udc->driver, driver->is_bound, and related calls */ +static DEFINE_MUTEX(udc_lock); /* ------------------------------------------------------------------------- */ @@ -1238,24 +1238,6 @@ static void usb_udc_nop_release(struct d dev_vdbg(dev, "%s\n", __func__); } -/* should be called with udc_lock held */ -static int check_pending_gadget_drivers(struct usb_udc *udc) -{ - struct usb_gadget_driver *driver; - int ret = 0; - - list_for_each_entry(driver, &gadget_driver_pending_list, pending) - if (!driver->udc_name || strcmp(driver->udc_name, - dev_name(&udc->dev)) == 0) { - ret = udc_bind_to_driver(udc, driver); - if (ret != -EPROBE_DEFER) - list_del_init(&driver->pending); - break; - } - - return ret; -} - /** * usb_initialize_gadget - initialize a gadget and its embedded struct device * @parent: the parent device to this udc. Usually the controller driver's @@ -1276,6 +1258,7 @@ void usb_initialize_gadget(struct device gadget->dev.release = usb_udc_nop_release; device_initialize(&gadget->dev); + gadget->dev.bus = &gadget_bus_type; } EXPORT_SYMBOL_GPL(usb_initialize_gadget); @@ -1312,6 +1295,7 @@ int usb_add_gadget(struct usb_gadget *ga mutex_lock(&udc_lock); list_add_tail(&udc->list, &udc_list); + mutex_unlock(&udc_lock); ret = device_add(&udc->dev); if (ret) @@ -1324,23 +1308,14 @@ int usb_add_gadget(struct usb_gadget *ga if (ret) goto err_del_udc; - /* pick up one of pending gadget drivers */ - ret = check_pending_gadget_drivers(udc); - if (ret) - goto err_del_gadget; - - mutex_unlock(&udc_lock); - return 0; - err_del_gadget: - device_del(&gadget->dev); - err_del_udc: flush_work(&gadget->work); device_del(&udc->dev); err_unlist_udc: + mutex_lock(&udc_lock); list_del(&udc->list); mutex_unlock(&udc_lock); @@ -1419,24 +1394,6 @@ int usb_add_gadget_udc(struct device *pa } EXPORT_SYMBOL_GPL(usb_add_gadget_udc); -static void usb_gadget_remove_driver(struct usb_udc *udc) -{ - dev_dbg(&udc->dev, "unregistering UDC driver [%s]\n", - udc->driver->function); - - kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE); - - usb_gadget_disconnect(udc->gadget); - usb_gadget_disable_async_callbacks(udc); - if (udc->gadget->irq) - synchronize_irq(udc->gadget->irq); - udc->driver->unbind(udc->gadget); - usb_gadget_udc_stop(udc); - - udc->driver = NULL; - udc->gadget->dev.driver = NULL; -} - /** * usb_del_gadget - deletes a gadget and unregisters its udc * @gadget: the gadget to be deleted. @@ -1455,13 +1412,6 @@ void usb_del_gadget(struct usb_gadget *g mutex_lock(&udc_lock); list_del(&udc->list); - - if (udc->driver) { - struct usb_gadget_driver *driver = udc->driver; - - usb_gadget_remove_driver(udc); - list_add(&driver->pending, &gadget_driver_pending_list); - } mutex_unlock(&udc_lock); kobject_uevent(&udc->dev.kobj, KOBJ_REMOVE); @@ -1486,123 +1436,147 @@ EXPORT_SYMBOL_GPL(usb_del_gadget_udc); /* ------------------------------------------------------------------------- */ -static int udc_bind_to_driver(struct usb_udc *udc, struct usb_gadget_driver *driver) +static int gadget_match_driver(struct device *dev, struct device_driver *drv) { - int ret; + struct usb_gadget *gadget = dev_to_usb_gadget(dev); + struct usb_udc *udc = gadget->udc; + struct usb_gadget_driver *driver = container_of(drv, + struct usb_gadget_driver, driver); + + /* If the driver specifies a udc_name, it must match the UDC's name */ + if (driver->udc_name && + strcmp(driver->udc_name, dev_name(&udc->dev)) != 0) + return 0; + + /* If the driver is already bound to a gadget, it doesn't match */ + if (driver->is_bound) + return 0; + + /* Otherwise any gadget driver matches any UDC */ + return 1; +} - dev_dbg(&udc->dev, "registering UDC driver [%s]\n", - driver->function); +static int gadget_bind_driver(struct device *dev) +{ + struct usb_gadget *gadget = dev_to_usb_gadget(dev); + struct usb_udc *udc = gadget->udc; + struct usb_gadget_driver *driver = container_of(dev->driver, + struct usb_gadget_driver, driver); + int ret = 0; + mutex_lock(&udc_lock); + if (driver->is_bound) { + mutex_unlock(&udc_lock); + return -ENXIO; /* Driver binds to only one gadget */ + } + driver->is_bound = true; udc->driver = driver; - udc->gadget->dev.driver = &driver->driver; + mutex_unlock(&udc_lock); + + dev_dbg(&udc->dev, "binding gadget driver [%s]\n", driver->function); usb_gadget_udc_set_speed(udc, driver->max_speed); + mutex_lock(&udc_lock); ret = driver->bind(udc->gadget, driver); if (ret) - goto err1; + goto err_bind; + ret = usb_gadget_udc_start(udc); - if (ret) { - driver->unbind(udc->gadget); - goto err1; - } + if (ret) + goto err_start; usb_gadget_enable_async_callbacks(udc); usb_udc_connect_control(udc); + mutex_unlock(&udc_lock); kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE); return 0; -err1: + + err_start: + driver->unbind(udc->gadget); + + err_bind: if (ret != -EISNAM) dev_err(&udc->dev, "failed to start %s: %d\n", - udc->driver->function, ret); + driver->function, ret); + udc->driver = NULL; - udc->gadget->dev.driver = NULL; + driver->is_bound = false; + mutex_unlock(&udc_lock); + return ret; } -int usb_gadget_register_driver(struct usb_gadget_driver *driver) +static void gadget_unbind_driver(struct device *dev) +{ + struct usb_gadget *gadget = dev_to_usb_gadget(dev); + struct usb_udc *udc = gadget->udc; + struct usb_gadget_driver *driver = udc->driver; + + dev_dbg(&udc->dev, "unbinding gadget driver [%s]\n", driver->function); + + kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE); + + mutex_lock(&udc_lock); + usb_gadget_disconnect(gadget); + usb_gadget_disable_async_callbacks(udc); + if (gadget->irq) + synchronize_irq(gadget->irq); + udc->driver->unbind(gadget); + usb_gadget_udc_stop(udc); + + driver->is_bound = false; + udc->driver = NULL; + mutex_unlock(&udc_lock); +} + +/* ------------------------------------------------------------------------- */ + +int usb_gadget_register_driver_owner(struct usb_gadget_driver *driver, + struct module *owner, const char *mod_name) { - struct usb_udc *udc = NULL, *iter; - int ret = -ENODEV; + int ret; if (!driver || !driver->bind || !driver->setup) return -EINVAL; + driver->driver.bus = &gadget_bus_type; + driver->driver.owner = owner; + driver->driver.mod_name = mod_name; + ret = driver_register(&driver->driver); + if (ret) { + pr_warn("%s: driver registration failed: %d\n", + driver->function, ret); + return ret; + } + mutex_lock(&udc_lock); - if (driver->udc_name) { - list_for_each_entry(iter, &udc_list, list) { - ret = strcmp(driver->udc_name, dev_name(&iter->dev)); - if (ret) - continue; - udc = iter; - break; - } - if (ret) - ret = -ENODEV; - else if (udc->driver) + if (!driver->is_bound) { + if (driver->match_existing_only) { + pr_warn("%s: couldn't find an available UDC or it's busy\n", + driver->function); ret = -EBUSY; - else - goto found; - } else { - list_for_each_entry(iter, &udc_list, list) { - /* For now we take the first one */ - if (iter->driver) - continue; - udc = iter; - goto found; + } else { + pr_info("%s: couldn't find an available UDC\n", + driver->function); } - } - - if (!driver->match_existing_only) { - list_add_tail(&driver->pending, &gadget_driver_pending_list); - pr_info("couldn't find an available UDC - added [%s] to list of pending drivers\n", - driver->function); ret = 0; } - mutex_unlock(&udc_lock); + if (ret) - pr_warn("couldn't find an available UDC or it's busy: %d\n", ret); - return ret; -found: - ret = udc_bind_to_driver(udc, driver); - mutex_unlock(&udc_lock); + driver_unregister(&driver->driver); return ret; } -EXPORT_SYMBOL_GPL(usb_gadget_register_driver); +EXPORT_SYMBOL_GPL(usb_gadget_register_driver_owner); int usb_gadget_unregister_driver(struct usb_gadget_driver *driver) { - struct usb_udc *udc = NULL; - int ret = -ENODEV; - if (!driver || !driver->unbind) return -EINVAL; - mutex_lock(&udc_lock); - list_for_each_entry(udc, &udc_list, list) { - if (udc->driver == driver) { - usb_gadget_remove_driver(udc); - usb_gadget_set_state(udc->gadget, - USB_STATE_NOTATTACHED); - - /* Maybe there is someone waiting for this UDC? */ - check_pending_gadget_drivers(udc); - /* - * For now we ignore bind errors as probably it's - * not a valid reason to fail other's gadget unbind - */ - ret = 0; - break; - } - } - - if (ret) { - list_del(&driver->pending); - ret = 0; - } - mutex_unlock(&udc_lock); - return ret; + driver_unregister(&driver->driver); + return 0; } EXPORT_SYMBOL_GPL(usb_gadget_unregister_driver); @@ -1754,8 +1728,17 @@ static int usb_udc_uevent(struct device return 0; } +static struct bus_type gadget_bus_type = { + .name = "gadget", + .probe = gadget_bind_driver, + .remove = gadget_unbind_driver, + .match = gadget_match_driver, +}; + static int __init usb_udc_init(void) { + int rc; + udc_class = class_create(THIS_MODULE, "udc"); if (IS_ERR(udc_class)) { pr_err("failed to create udc class --> %ld\n", @@ -1764,12 +1747,17 @@ static int __init usb_udc_init(void) } udc_class->dev_uevent = usb_udc_uevent; - return 0; + + rc = bus_register(&gadget_bus_type); + if (rc) + class_destroy(udc_class); + return rc; } subsys_initcall(usb_udc_init); static void __exit usb_udc_exit(void) { + bus_unregister(&gadget_bus_type); class_destroy(udc_class); } module_exit(usb_udc_exit); Index: usb-devel/include/linux/usb/gadget.h =================================================================== --- usb-devel.orig/include/linux/usb/gadget.h +++ usb-devel/include/linux/usb/gadget.h @@ -664,9 +664,9 @@ static inline int usb_gadget_check_confi * @driver: Driver model state for this driver. * @udc_name: A name of UDC this driver should be bound to. If udc_name is NULL, * this driver will be bound to any available UDC. - * @pending: UDC core private data used for deferred probe of this driver. - * @match_existing_only: If udc is not found, return an error and don't add this - * gadget driver to list of pending driver + * @match_existing_only: If udc is not found, return an error and fail + * the driver registration + * @is_bound: Allow a driver to be bound to only one gadget * * Devices are disabled till a gadget driver successfully bind()s, which * means the driver will handle setup() requests needed to enumerate (and @@ -729,8 +729,8 @@ struct usb_gadget_driver { struct device_driver driver; char *udc_name; - struct list_head pending; unsigned match_existing_only:1; + bool is_bound:1; }; @@ -740,22 +740,30 @@ struct usb_gadget_driver { /* driver modules register and unregister, as usual. * these calls must be made in a context that can sleep. * - * these will usually be implemented directly by the hardware-dependent - * usb bus interface driver, which will only support a single driver. + * A gadget driver can be bound to only one gadget at a time. */ /** - * usb_gadget_register_driver - register a gadget driver + * usb_gadget_register_driver_owner - register a gadget driver * @driver: the driver being registered + * @owner: the driver module + * @mod_name: the driver module's build name * Context: can sleep * * Call this in your gadget driver's module initialization function, - * to tell the underlying usb controller driver about your driver. + * to tell the underlying UDC controller driver about your driver. * The @bind() function will be called to bind it to a gadget before this * registration call returns. It's expected that the @bind() function will * be in init sections. + * + * Use the macro defined below instead of calling this directly. */ -int usb_gadget_register_driver(struct usb_gadget_driver *driver); +int usb_gadget_register_driver_owner(struct usb_gadget_driver *driver, + struct module *owner, const char *mod_name); + +/* use a define to avoid include chaining to get THIS_MODULE & friends */ +#define usb_gadget_register_driver(driver) \ + usb_gadget_register_driver_owner(driver, THIS_MODULE, KBUILD_MODNAME) /** * usb_gadget_unregister_driver - unregister a gadget driver
This patch adds a "gadget" bus and uses it for registering gadgets and their drivers. From now on, bindings will be managed by the driver core rather than through ad-hoc manipulations in the UDC core. As part of this change, the driver_pending_list is removed. The UDC core won't need to keep track of unbound drivers for later binding, because the driver core handles all of that for us. However, we do need one new feature: a way to prevent gadget drivers from being bound to more than one gadget at a time. The existing code does this automatically, but the driver core doesn't -- it's perfectly happy to bind a single driver to all the matching devices on the bus. The patch adds a new bitflag to the usb_gadget_driver structure for this purpose. A nice side effect of this change is a reduction in the total lines of code, since now the driver core will do part of the work that the UDC used to do. A possible future patch could add udc devices to the gadget bus, say as a separate device type. Signed-off-by: Alan Stern <stern@rowland.harvard.edu> --- [as1979] drivers/usb/gadget/udc/core.c | 256 ++++++++++++++++++++---------------------- include/linux/usb/gadget.h | 26 ++-- 2 files changed, 139 insertions(+), 143 deletions(-)