[U-Boot] [PATCH 0/5] RFC: usb: Drop requirement for USB unbinding
There was quite a bit of discussion about the change that required the unbinding of USB devices for the subsystem to function correctly. E.g.
https://patchwork.ozlabs.org/patch/485637/
The key issue is the usb_get_dev_index() function which is not a good API for driver model. We can drop use of this function once everything is converted to driver model. Then I believe the problems raised by Hans go away. For now we can add a deprecation warning on the function.
It is easy to convert USB keyboards to driver model. This series includes a patch for that.
This series also includes reverts for the three commits which as discussed I would like to drop. U-Boot should be able to run normally and exit without unbinding anything.
I cannot actually repeat the problem that Hans mentions in the above thread so I may be missing a step. Hans, any ideas on this?
Simon Glass (5): Revert "dm: usb: Rename usb_find_child to usb_find_emul_child" Revert "dm: usb: Use device_unbind_children to clean up usb devs on stop" Revert "dm: Export device_remove_children / device_unbind_children" dm: usb: Add support for USB keyboards with driver model dm: usb: Deprecate usb_get_dev_index()
common/cmd_usb.c | 12 +++++----- common/usb_kbd.c | 52 +++++++++++++++++++++++++++++++++++++++++-- drivers/core/device-remove.c | 22 ++++++++++++++---- drivers/usb/host/usb-uclass.c | 31 ++++++++++++++++---------- include/dm/device-internal.h | 26 ---------------------- include/dm/uclass-id.h | 1 + 6 files changed, 94 insertions(+), 50 deletions(-)
This reverts commit 9b510df703d282effba4f56ac567aa8011d56e6b.
We want to avoid having the USB stack rely on unbind.
Signed-off-by: Simon Glass sjg@chromium.org ---
drivers/usb/host/usb-uclass.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c index b17a7d7..3c45181 100644 --- a/drivers/usb/host/usb-uclass.c +++ b/drivers/usb/host/usb-uclass.c @@ -493,14 +493,15 @@ error: }
/** - * usb_find_emul_child() - Find an existing device for emulated devices + * usb_find_child() - Find an existing device which matches our needs + * + * */ -static int usb_find_emul_child(struct udevice *parent, - struct usb_device_descriptor *desc, - struct usb_interface_descriptor *iface, - struct udevice **devp) +static int usb_find_child(struct udevice *parent, + struct usb_device_descriptor *desc, + struct usb_interface_descriptor *iface, + struct udevice **devp) { -#ifdef CONFIG_SANDBOX struct udevice *dev;
*devp = NULL; @@ -519,7 +520,7 @@ static int usb_find_emul_child(struct udevice *parent, return 0; } } -#endif + return -ENOENT; }
@@ -579,8 +580,8 @@ int usb_scan_device(struct udevice *parent, int port, debug("read_descriptor for '%s': ret=%d\n", parent->name, ret); if (ret) return ret; - ret = usb_find_emul_child(parent, &udev->descriptor, iface, &dev); - debug("** usb_find_emul_child returns %d\n", ret); + ret = usb_find_child(parent, &udev->descriptor, iface, &dev); + debug("** usb_find_child returns %d\n", ret); if (ret) { if (ret != -ENOENT) return ret;
This reverts commit 6cda369509e0d3fa5f9e33c9d71589c4523799fa.
We want to avoid having the USB stack rely on unbind.
Signed-off-by: Simon Glass sjg@chromium.org ---
drivers/usb/host/usb-uclass.c | 3 --- 1 file changed, 3 deletions(-)
diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c index 3c45181..ebe22d6 100644 --- a/drivers/usb/host/usb-uclass.c +++ b/drivers/usb/host/usb-uclass.c @@ -157,9 +157,6 @@ int usb_stop(void) ret = device_remove(bus); if (ret && !err) err = ret; - ret = device_unbind_children(bus); - if (ret && !err) - err = ret; }
#ifdef CONFIG_SANDBOX
This reverts commit bb52b367f6ca4a3a918e77737f4ff6a1089912d9.
Signed-off-by: Simon Glass sjg@chromium.org ---
drivers/core/device-remove.c | 22 ++++++++++++++++++---- include/dm/device-internal.h | 26 -------------------------- 2 files changed, 18 insertions(+), 30 deletions(-)
diff --git a/drivers/core/device-remove.c b/drivers/core/device-remove.c index bd6d406..e1714b2 100644 --- a/drivers/core/device-remove.c +++ b/drivers/core/device-remove.c @@ -18,7 +18,16 @@ #include <dm/uclass-internal.h> #include <dm/util.h>
-int device_unbind_children(struct udevice *dev) +/** + * device_chld_unbind() - Unbind all device's children from the device + * + * On error, the function continues to unbind all children, and reports the + * first error. + * + * @dev: The device that is to be stripped of its children + * @return 0 on success, -ve on error + */ +static int device_chld_unbind(struct udevice *dev) { struct udevice *pos, *n; int ret, saved_ret = 0; @@ -34,7 +43,12 @@ int device_unbind_children(struct udevice *dev) return saved_ret; }
-int device_remove_children(struct udevice *dev) +/** + * device_chld_remove() - Stop all device's children + * @dev: The device whose children are to be removed + * @return 0 on success, -ve on error + */ +static int device_chld_remove(struct udevice *dev) { struct udevice *pos, *n; int ret; @@ -73,7 +87,7 @@ int device_unbind(struct udevice *dev) return ret; }
- ret = device_unbind_children(dev); + ret = device_chld_unbind(dev); if (ret) return ret;
@@ -153,7 +167,7 @@ int device_remove(struct udevice *dev) if (ret) return ret;
- ret = device_remove_children(dev); + ret = device_chld_remove(dev); if (ret) goto err;
diff --git a/include/dm/device-internal.h b/include/dm/device-internal.h index 322d35a..9388870 100644 --- a/include/dm/device-internal.h +++ b/include/dm/device-internal.h @@ -107,32 +107,6 @@ int device_unbind(struct udevice *dev); static inline int device_unbind(struct udevice *dev) { return 0; } #endif
-/** - * device_remove_children() - Stop all device's children - * @dev: The device whose children are to be removed - * @return 0 on success, -ve on error - */ -#if CONFIG_IS_ENABLED(DM_DEVICE_REMOVE) -int device_remove_children(struct udevice *dev); -#else -static inline int device_remove_children(struct udevice *dev) { return 0; } -#endif - -/** - * device_unbind_children() - Unbind all device's children from the device - * - * On error, the function continues to unbind all children, and reports the - * first error. - * - * @dev: The device that is to be stripped of its children - * @return 0 on success, -ve on error - */ -#if CONFIG_IS_ENABLED(DM_DEVICE_REMOVE) -int device_unbind_children(struct udevice *dev); -#else -static inline int device_unbind_children(struct udevice *dev) { return 0; } -#endif - #if CONFIG_IS_ENABLED(DM_DEVICE_REMOVE) void device_free(struct udevice *dev); #else
Switch USB keyboards over to use driver model instead of scanning with the horrible usb_get_dev_index() function. This involves creating a new uclass for keyboards, although so far there is no API.
Signed-off-by: Simon Glass sjg@chromium.org ---
common/cmd_usb.c | 12 ++++++------ common/usb_kbd.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++-- include/dm/uclass-id.h | 1 + 3 files changed, 57 insertions(+), 8 deletions(-)
diff --git a/common/cmd_usb.c b/common/cmd_usb.c index 6874af7..665f8ec 100644 --- a/common/cmd_usb.c +++ b/common/cmd_usb.c @@ -526,11 +526,14 @@ static void do_usb_start(void)
/* Driver model will probe the devices as they are found */ #ifndef CONFIG_DM_USB -#ifdef CONFIG_USB_STORAGE +# ifdef CONFIG_USB_STORAGE /* try to recognize storage devices immediately */ usb_stor_curr_dev = usb_stor_scan(1); -#endif -#endif +# endif +# ifdef CONFIG_USB_KEYBOARD + drv_usb_kbd_init(); +# endif +#endif /* !CONFIG_DM_USB */ #ifdef CONFIG_USB_HOST_ETHER # ifdef CONFIG_DM_ETH # ifndef CONFIG_DM_USB @@ -541,9 +544,6 @@ static void do_usb_start(void) usb_ether_curr_dev = usb_host_eth_scan(1); # endif #endif -#ifdef CONFIG_USB_KEYBOARD - drv_usb_kbd_init(); -#endif }
#ifdef CONFIG_DM_USB diff --git a/common/usb_kbd.c b/common/usb_kbd.c index 0227024..8037ebf 100644 --- a/common/usb_kbd.c +++ b/common/usb_kbd.c @@ -398,7 +398,7 @@ static int usb_kbd_getc(struct stdio_dev *sdev) }
/* probes the USB device dev for keyboard type. */ -static int usb_kbd_probe(struct usb_device *dev, unsigned int ifnum) +static int usb_kbd_probe_dev(struct usb_device *dev, unsigned int ifnum) { struct usb_interface *iface; struct usb_endpoint_descriptor *ep; @@ -495,7 +495,7 @@ static int probe_usb_keyboard(struct usb_device *dev) int error;
/* Try probing the keyboard */ - if (usb_kbd_probe(dev, 0) != 1) + if (usb_kbd_probe_dev(dev, 0) != 1) return -ENOENT;
/* Register the keyboard */ @@ -532,6 +532,7 @@ static int probe_usb_keyboard(struct usb_device *dev) return 0; }
+#ifndef CONFIG_DM_USB /* Search for keyboard and register it if found. */ int drv_usb_kbd_init(void) { @@ -590,6 +591,7 @@ int drv_usb_kbd_init(void) /* No USB Keyboard found */ return -1; } +#endif
/* Deregister the keyboard. */ int usb_kbd_deregister(int force) @@ -621,3 +623,49 @@ int usb_kbd_deregister(int force) return 1; #endif } + +#ifdef CONFIG_DM_USB + +static int usb_kbd_probe(struct udevice *dev) +{ + struct usb_device *udev = dev_get_parentdata(dev); + int ret; + + ret = probe_usb_keyboard(udev); + + return ret; +} + +static const struct udevice_id usb_kbd_ids[] = { + { .compatible = "usb-keyboard" }, + { } +}; + +U_BOOT_DRIVER(usb_kbd) = { + .name = "usb_kbd", + .id = UCLASS_KEYBOARD, + .of_match = usb_kbd_ids, + .probe = usb_kbd_probe, +}; + +/* TODO(sjg@chromium.org): Move this into a common location */ +UCLASS_DRIVER(keyboard) = { + .id = UCLASS_KEYBOARD, + .name = "keyboard", +}; + +static const struct usb_device_id kbd_id_table[] = { + { + .match_flags = USB_DEVICE_ID_MATCH_INT_CLASS | + USB_DEVICE_ID_MATCH_INT_SUBCLASS | + USB_DEVICE_ID_MATCH_INT_PROTOCOL, + .bInterfaceClass = USB_CLASS_HID, + .bInterfaceSubClass = 1, + .bInterfaceProtocol = 1, + }, + { } /* Terminating entry */ +}; + +U_BOOT_USB_DEVICE(usb_kbd, kbd_id_table); + +#endif diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h index 1eeec74..bab0025 100644 --- a/include/dm/uclass-id.h +++ b/include/dm/uclass-id.h @@ -36,6 +36,7 @@ enum uclass_id { UCLASS_I2C_EEPROM, /* I2C EEPROM device */ UCLASS_I2C_GENERIC, /* Generic I2C device */ UCLASS_I2C_MUX, /* I2C multiplexer */ + UCLASS_KEYBOARD, /* Keyboard input device */ UCLASS_LED, /* Light-emitting diode (LED) */ UCLASS_LPC, /* x86 'low pin count' interface */ UCLASS_MASS_STORAGE, /* Mass storage device */
On Tuesday, September 08, 2015 at 07:15:11 PM, Simon Glass wrote:
Switch USB keyboards over to use driver model instead of scanning with the horrible usb_get_dev_index() function. This involves creating a new uclass for keyboards, although so far there is no API.
Hi,
Why don't you create an UCLASS for generic input device instead ?
But in general, looks pretty standard/OK.
Best regards, Marek Vasut
Hi Marek,
On 8 September 2015 at 12:33, Marek Vasut marex@denx.de wrote:
On Tuesday, September 08, 2015 at 07:15:11 PM, Simon Glass wrote:
Switch USB keyboards over to use driver model instead of scanning with the horrible usb_get_dev_index() function. This involves creating a new uclass for keyboards, although so far there is no API.
Hi,
Why don't you create an UCLASS for generic input device instead ?
I sent a series that does that later. My intent with this series is to get something applied for this release.
But in general, looks pretty standard/OK.
Regards, Simon
On Thursday, September 10, 2015 at 04:45:53 AM, Simon Glass wrote:
Hi Marek,
On 8 September 2015 at 12:33, Marek Vasut marex@denx.de wrote:
On Tuesday, September 08, 2015 at 07:15:11 PM, Simon Glass wrote:
Switch USB keyboards over to use driver model instead of scanning with the horrible usb_get_dev_index() function. This involves creating a new uclass for keyboards, although so far there is no API.
Hi,
Why don't you create an UCLASS for generic input device instead ?
I sent a series that does that later. My intent with this series is to get something applied for this release.
Hi!
Aren't we pretty much post-RC3 now ?
Best regards, Marek Vasut
Hi Marek,
On 10 September 2015 at 04:40, Marek Vasut marex@denx.de wrote:
On Thursday, September 10, 2015 at 04:45:53 AM, Simon Glass wrote:
Hi Marek,
On 8 September 2015 at 12:33, Marek Vasut marex@denx.de wrote:
On Tuesday, September 08, 2015 at 07:15:11 PM, Simon Glass wrote:
Switch USB keyboards over to use driver model instead of scanning with the horrible usb_get_dev_index() function. This involves creating a new uclass for keyboards, although so far there is no API.
Hi,
Why don't you create an UCLASS for generic input device instead ?
I sent a series that does that later. My intent with this series is to get something applied for this release.
Hi!
Aren't we pretty much post-RC3 now ?
Yes. It's not critical and I am late - let's see what Hans says.
Regards, Simon
Hi,
On 09/11/2015 02:43 AM, Simon Glass wrote:
Hi Marek,
On 10 September 2015 at 04:40, Marek Vasut marex@denx.de wrote:
On Thursday, September 10, 2015 at 04:45:53 AM, Simon Glass wrote:
Hi Marek,
On 8 September 2015 at 12:33, Marek Vasut marex@denx.de wrote:
On Tuesday, September 08, 2015 at 07:15:11 PM, Simon Glass wrote:
Switch USB keyboards over to use driver model instead of scanning with the horrible usb_get_dev_index() function. This involves creating a new uclass for keyboards, although so far there is no API.
Hi,
Why don't you create an UCLASS for generic input device instead ?
I sent a series that does that later. My intent with this series is to get something applied for this release.
Hi!
Aren't we pretty much post-RC3 now ?
Yes. It's not critical and I am late - let's see what Hans says.
I have looking into the RFC patchset on my todo, not sure if I will get around to it this weekend though, and after that I'm travelling for a week. Even if I get around to testing this I would prefer for this to be delayed to post v2015.10. I'm fine with the concept of the set but this needs some careful testing.
Regards,
Hans
Hi,
On 08-09-15 19:15, Simon Glass wrote:
Switch USB keyboards over to use driver model instead of scanning with the horrible usb_get_dev_index() function. This involves creating a new uclass for keyboards, although so far there is no API.
Signed-off-by: Simon Glass sjg@chromium.org
In general I like this patch, so ack for the principle, but the implementation has issues.
You're now allowing the registration of multiple usb keyb stdio input devices, but you are still relying on usb_kbd_deregister() to remove them from the stdio devices list, and when multiple or used that one will only remove the first one.
This can be fixed by switching to stdio_register_dev, and store the returned struct stdio_dev pointer for the new dev, and add a dm remove callback which deregisters that specific stdio_dev that will also remove the ugliness of looking up the device by its name to unregister it.
The name which in itself is another, harder to fix issue, when using iomux, and the stdin string contains usbkbd we really want to get all usbkbd-s to work, but iomux will only take the first one.
This can be fixed in 2 ways:
1) in the usbkbd driver by registering a shared stdio_dev for all usbkbd's and deregistering that only when the last usbkbd is removed (in the case of dm), this will require a global list of usbkbd devices, and stdio callbacks to walk over this list, I believe that this is likely the best approach
2) Fix this in iomux, and make it look for multiple devs with the same name and mux them all.
This seems cleaner at a conceptual level, but likely somewhat hard to implement.
Regards,
Hans
common/cmd_usb.c | 12 ++++++------ common/usb_kbd.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++-- include/dm/uclass-id.h | 1 + 3 files changed, 57 insertions(+), 8 deletions(-)
diff --git a/common/cmd_usb.c b/common/cmd_usb.c index 6874af7..665f8ec 100644 --- a/common/cmd_usb.c +++ b/common/cmd_usb.c @@ -526,11 +526,14 @@ static void do_usb_start(void)
/* Driver model will probe the devices as they are found */ #ifndef CONFIG_DM_USB -#ifdef CONFIG_USB_STORAGE +# ifdef CONFIG_USB_STORAGE /* try to recognize storage devices immediately */ usb_stor_curr_dev = usb_stor_scan(1); -#endif -#endif +# endif +# ifdef CONFIG_USB_KEYBOARD
- drv_usb_kbd_init();
+# endif +#endif /* !CONFIG_DM_USB */ #ifdef CONFIG_USB_HOST_ETHER # ifdef CONFIG_DM_ETH # ifndef CONFIG_DM_USB @@ -541,9 +544,6 @@ static void do_usb_start(void) usb_ether_curr_dev = usb_host_eth_scan(1); # endif #endif -#ifdef CONFIG_USB_KEYBOARD
- drv_usb_kbd_init();
-#endif }
#ifdef CONFIG_DM_USB diff --git a/common/usb_kbd.c b/common/usb_kbd.c index 0227024..8037ebf 100644 --- a/common/usb_kbd.c +++ b/common/usb_kbd.c @@ -398,7 +398,7 @@ static int usb_kbd_getc(struct stdio_dev *sdev) }
/* probes the USB device dev for keyboard type. */ -static int usb_kbd_probe(struct usb_device *dev, unsigned int ifnum) +static int usb_kbd_probe_dev(struct usb_device *dev, unsigned int ifnum) { struct usb_interface *iface; struct usb_endpoint_descriptor *ep; @@ -495,7 +495,7 @@ static int probe_usb_keyboard(struct usb_device *dev) int error;
/* Try probing the keyboard */
- if (usb_kbd_probe(dev, 0) != 1)
if (usb_kbd_probe_dev(dev, 0) != 1) return -ENOENT;
/* Register the keyboard */
@@ -532,6 +532,7 @@ static int probe_usb_keyboard(struct usb_device *dev) return 0; }
+#ifndef CONFIG_DM_USB /* Search for keyboard and register it if found. */ int drv_usb_kbd_init(void) { @@ -590,6 +591,7 @@ int drv_usb_kbd_init(void) /* No USB Keyboard found */ return -1; } +#endif
/* Deregister the keyboard. */ int usb_kbd_deregister(int force) @@ -621,3 +623,49 @@ int usb_kbd_deregister(int force) return 1; #endif }
+#ifdef CONFIG_DM_USB
+static int usb_kbd_probe(struct udevice *dev) +{
- struct usb_device *udev = dev_get_parentdata(dev);
- int ret;
- ret = probe_usb_keyboard(udev);
- return ret;
+}
+static const struct udevice_id usb_kbd_ids[] = {
- { .compatible = "usb-keyboard" },
- { }
+};
+U_BOOT_DRIVER(usb_kbd) = {
- .name = "usb_kbd",
- .id = UCLASS_KEYBOARD,
- .of_match = usb_kbd_ids,
- .probe = usb_kbd_probe,
+};
+/* TODO(sjg@chromium.org): Move this into a common location */ +UCLASS_DRIVER(keyboard) = {
- .id = UCLASS_KEYBOARD,
- .name = "keyboard",
+};
+static const struct usb_device_id kbd_id_table[] = {
- {
.match_flags = USB_DEVICE_ID_MATCH_INT_CLASS |USB_DEVICE_ID_MATCH_INT_SUBCLASS |USB_DEVICE_ID_MATCH_INT_PROTOCOL,.bInterfaceClass = USB_CLASS_HID,.bInterfaceSubClass = 1,.bInterfaceProtocol = 1,- },
- { } /* Terminating entry */
+};
+U_BOOT_USB_DEVICE(usb_kbd, kbd_id_table);
+#endif diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h index 1eeec74..bab0025 100644 --- a/include/dm/uclass-id.h +++ b/include/dm/uclass-id.h @@ -36,6 +36,7 @@ enum uclass_id { UCLASS_I2C_EEPROM, /* I2C EEPROM device */ UCLASS_I2C_GENERIC, /* Generic I2C device */ UCLASS_I2C_MUX, /* I2C multiplexer */
- UCLASS_KEYBOARD, /* Keyboard input device */ UCLASS_LED, /* Light-emitting diode (LED) */ UCLASS_LPC, /* x86 'low pin count' interface */ UCLASS_MASS_STORAGE, /* Mass storage device */
Hi Hans,
On 12 September 2015 at 09:15, Hans de Goede hdegoede@redhat.com wrote:
Hi,
On 08-09-15 19:15, Simon Glass wrote:
Switch USB keyboards over to use driver model instead of scanning with the horrible usb_get_dev_index() function. This involves creating a new uclass for keyboards, although so far there is no API.
Signed-off-by: Simon Glass sjg@chromium.org
In general I like this patch, so ack for the principle, but the implementation has issues.
You're now allowing the registration of multiple usb keyb stdio input devices, but you are still relying on usb_kbd_deregister() to remove them from the stdio devices list, and when multiple or used that one will only remove the first one.
This can be fixed by switching to stdio_register_dev, and store the returned struct stdio_dev pointer for the new dev, and add a dm remove callback which deregisters that specific stdio_dev that will also remove the ugliness of looking up the device by its name to unregister it.
The name which in itself is another, harder to fix issue, when using iomux, and the stdin string contains usbkbd we really want to get all usbkbd-s to work, but iomux will only take the first one.
This can be fixed in 2 ways:
- in the usbkbd driver by registering a shared stdio_dev
for all usbkbd's and deregistering that only when the last usbkbd is removed (in the case of dm), this will require a global list of usbkbd devices, and stdio callbacks to walk over this list, I believe that this is likely the best approach
- Fix this in iomux, and make it look for multiple
devs with the same name and mux them all.
This seems cleaner at a conceptual level, but likely somewhat hard to implement.
I've had another look at this and here are my comments so far:
1. The existing driver does not support multiple keyboards. It implements this limitation in multiple ways which would be a real pain to fix while keeping the old code. I think it makes much more sense to remove this limitation when we have either a) moved all uses of USB keyboard to driver model, or perhaps b) moved stdio to driver model. For now the driver model approach provides the same functionality as before so I think it is fine.
2. The point about out-of-order devices in the 'usb tree' display....well if you disable unbinding that is what you get. I'm sure we could fix it by sorting the devices before displaying them, but it does not seem that important to me. It is more likely that the unbind support will be enabled in U-Boot proper, and perhaps disabled in SPL, which doesn't have commands anyway.
[snip]
Regards, Simon
Hi,
On 19-10-15 01:17, Simon Glass wrote:
Hi Hans,
On 12 September 2015 at 09:15, Hans de Goede hdegoede@redhat.com wrote:
Hi,
On 08-09-15 19:15, Simon Glass wrote:
Switch USB keyboards over to use driver model instead of scanning with the horrible usb_get_dev_index() function. This involves creating a new uclass for keyboards, although so far there is no API.
Signed-off-by: Simon Glass sjg@chromium.org
In general I like this patch, so ack for the principle, but the implementation has issues.
You're now allowing the registration of multiple usb keyb stdio input devices, but you are still relying on usb_kbd_deregister() to remove them from the stdio devices list, and when multiple or used that one will only remove the first one.
This can be fixed by switching to stdio_register_dev, and store the returned struct stdio_dev pointer for the new dev, and add a dm remove callback which deregisters that specific stdio_dev that will also remove the ugliness of looking up the device by its name to unregister it.
The name which in itself is another, harder to fix issue, when using iomux, and the stdin string contains usbkbd we really want to get all usbkbd-s to work, but iomux will only take the first one.
This can be fixed in 2 ways:
- in the usbkbd driver by registering a shared stdio_dev
for all usbkbd's and deregistering that only when the last usbkbd is removed (in the case of dm), this will require a global list of usbkbd devices, and stdio callbacks to walk over this list, I believe that this is likely the best approach
- Fix this in iomux, and make it look for multiple
devs with the same name and mux them all.
This seems cleaner at a conceptual level, but likely somewhat hard to implement.
I've had another look at this and here are my comments so far:
- The existing driver does not support multiple keyboards. It
implements this limitation in multiple ways which would be a real pain to fix while keeping the old code. I think it makes much more sense to remove this limitation when we have either a) moved all uses of USB keyboard to driver model, or perhaps b) moved stdio to driver model. For now the driver model approach provides the same functionality as before so I think it is fine.
I think that supporting multiple keyboards the way I've outlined above as "1)" should not be that hard. But I do not plan to make time for this anytime soon, and as such I can hardly ask you to do this.
So I reluctantly agree to keep this as is (I was hoping the move to dm would fix this).
- The point about out-of-order devices in the 'usb tree'
display....well if you disable unbinding that is what you get. I'm sure we could fix it by sorting the devices before displaying them, but it does not seem that important to me. It is more likely that the unbind support will be enabled in U-Boot proper, and perhaps disabled in SPL, which doesn't have commands anyway.
I'm fine with "usb tree" showing things the wrong way on builds where unbinding is disabled. But if I remember the patch-set this thread is about correctly, it completely removed unbinding from the usb code.
If you do a new version where unbinding is only skipped when compiled out then that is fine with me.
Regards,
Hans
Hi Hans,
On 19 October 2015 at 02:34, Hans de Goede hdegoede@redhat.com wrote:
Hi,
On 19-10-15 01:17, Simon Glass wrote:
Hi Hans,
On 12 September 2015 at 09:15, Hans de Goede hdegoede@redhat.com wrote:
Hi,
On 08-09-15 19:15, Simon Glass wrote:
Switch USB keyboards over to use driver model instead of scanning with the horrible usb_get_dev_index() function. This involves creating a new uclass for keyboards, although so far there is no API.
Signed-off-by: Simon Glass sjg@chromium.org
In general I like this patch, so ack for the principle, but the implementation has issues.
You're now allowing the registration of multiple usb keyb stdio input devices, but you are still relying on usb_kbd_deregister() to remove them from the stdio devices list, and when multiple or used that one will only remove the first one.
This can be fixed by switching to stdio_register_dev, and store the returned struct stdio_dev pointer for the new dev, and add a dm remove callback which deregisters that specific stdio_dev that will also remove the ugliness of looking up the device by its name to unregister it.
The name which in itself is another, harder to fix issue, when using iomux, and the stdin string contains usbkbd we really want to get all usbkbd-s to work, but iomux will only take the first one.
This can be fixed in 2 ways:
- in the usbkbd driver by registering a shared stdio_dev
for all usbkbd's and deregistering that only when the last usbkbd is removed (in the case of dm), this will require a global list of usbkbd devices, and stdio callbacks to walk over this list, I believe that this is likely the best approach
- Fix this in iomux, and make it look for multiple
devs with the same name and mux them all.
This seems cleaner at a conceptual level, but likely somewhat hard to implement.
I've had another look at this and here are my comments so far:
- The existing driver does not support multiple keyboards. It
implements this limitation in multiple ways which would be a real pain to fix while keeping the old code. I think it makes much more sense to remove this limitation when we have either a) moved all uses of USB keyboard to driver model, or perhaps b) moved stdio to driver model. For now the driver model approach provides the same functionality as before so I think it is fine.
I think that supporting multiple keyboards the way I've outlined above as "1)" should not be that hard. But I do not plan to make time for this anytime soon, and as such I can hardly ask you to do this.
So I reluctantly agree to keep this as is (I was hoping the move to dm would fix this).
- The point about out-of-order devices in the 'usb tree'
display....well if you disable unbinding that is what you get. I'm sure we could fix it by sorting the devices before displaying them, but it does not seem that important to me. It is more likely that the unbind support will be enabled in U-Boot proper, and perhaps disabled in SPL, which doesn't have commands anyway.
I'm fine with "usb tree" showing things the wrong way on builds where unbinding is disabled. But if I remember the patch-set this thread is about correctly, it completely removed unbinding from the usb code.
If you do a new version where unbinding is only skipped when compiled out then that is fine with me.
OK, for now I'm going to apply just this patch from the series, to unblock the input uclass.
I'll then redo this series to allow the unbinding as, indeed, that is what I want to happen too.
Regards, Simon
On 29 October 2015 at 13:09, Simon Glass sjg@chromium.org wrote:
Hi Hans,
On 19 October 2015 at 02:34, Hans de Goede hdegoede@redhat.com wrote:
Hi,
On 19-10-15 01:17, Simon Glass wrote:
Hi Hans,
On 12 September 2015 at 09:15, Hans de Goede hdegoede@redhat.com wrote:
Hi,
On 08-09-15 19:15, Simon Glass wrote:
Switch USB keyboards over to use driver model instead of scanning with the horrible usb_get_dev_index() function. This involves creating a new uclass for keyboards, although so far there is no API.
Signed-off-by: Simon Glass sjg@chromium.org
In general I like this patch, so ack for the principle, but the implementation has issues.
You're now allowing the registration of multiple usb keyb stdio input devices, but you are still relying on usb_kbd_deregister() to remove them from the stdio devices list, and when multiple or used that one will only remove the first one.
This can be fixed by switching to stdio_register_dev, and store the returned struct stdio_dev pointer for the new dev, and add a dm remove callback which deregisters that specific stdio_dev that will also remove the ugliness of looking up the device by its name to unregister it.
The name which in itself is another, harder to fix issue, when using iomux, and the stdin string contains usbkbd we really want to get all usbkbd-s to work, but iomux will only take the first one.
This can be fixed in 2 ways:
- in the usbkbd driver by registering a shared stdio_dev
for all usbkbd's and deregistering that only when the last usbkbd is removed (in the case of dm), this will require a global list of usbkbd devices, and stdio callbacks to walk over this list, I believe that this is likely the best approach
- Fix this in iomux, and make it look for multiple
devs with the same name and mux them all.
This seems cleaner at a conceptual level, but likely somewhat hard to implement.
I've had another look at this and here are my comments so far:
- The existing driver does not support multiple keyboards. It
implements this limitation in multiple ways which would be a real pain to fix while keeping the old code. I think it makes much more sense to remove this limitation when we have either a) moved all uses of USB keyboard to driver model, or perhaps b) moved stdio to driver model. For now the driver model approach provides the same functionality as before so I think it is fine.
I think that supporting multiple keyboards the way I've outlined above as "1)" should not be that hard. But I do not plan to make time for this anytime soon, and as such I can hardly ask you to do this.
So I reluctantly agree to keep this as is (I was hoping the move to dm would fix this).
- The point about out-of-order devices in the 'usb tree'
display....well if you disable unbinding that is what you get. I'm sure we could fix it by sorting the devices before displaying them, but it does not seem that important to me. It is more likely that the unbind support will be enabled in U-Boot proper, and perhaps disabled in SPL, which doesn't have commands anyway.
I'm fine with "usb tree" showing things the wrong way on builds where unbinding is disabled. But if I remember the patch-set this thread is about correctly, it completely removed unbinding from the usb code.
If you do a new version where unbinding is only skipped when compiled out then that is fine with me.
OK, for now I'm going to apply just this patch from the series, to unblock the input uclass.
I'll then redo this series to allow the unbinding as, indeed, that is what I want to happen too.
Applied to u-boot-dm.
This function should not be used with driver model. While there are users of USB Ethernet that use driver model for USB but not Ethernet, we have to keep it around. Add a comment to that effect.
Signed-off-by: Simon Glass sjg@chromium.org ---
drivers/usb/host/usb-uclass.c | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c index ebe22d6..243e699 100644 --- a/drivers/usb/host/usb-uclass.c +++ b/drivers/usb/host/usb-uclass.c @@ -278,6 +278,14 @@ int usb_init(void) return usb_started ? 0 : -1; }
+/* + * TODO(sjg@chromium.org): Remove this legacy function. At present it is needed + * to support boards which use driver model for USB but not Ethernet, and want + * to use USB Ethernet. + * + * The #if clause is here to ensure that remains the only case. + */ +#if !defined(CONFIG_DM_ETH) && defined(CONFIG_USB_HOST_ETHER) static struct usb_device *find_child_devnum(struct udevice *parent, int devnum) { struct usb_device *udev; @@ -311,6 +319,7 @@ struct usb_device *usb_get_dev_index(struct udevice *bus, int index)
return find_child_devnum(dev, devnum); } +#endif
int usb_post_bind(struct udevice *dev) {
Hi,
On 08-09-15 19:15, Simon Glass wrote:
There was quite a bit of discussion about the change that required the unbinding of USB devices for the subsystem to function correctly. E.g.
https://patchwork.ozlabs.org/patch/485637/
The key issue is the usb_get_dev_index() function which is not a good API for driver model. We can drop use of this function once everything is converted to driver model. Then I believe the problems raised by Hans go away. For now we can add a deprecation warning on the function.
It is easy to convert USB keyboards to driver model. This series includes a patch for that.
This series also includes reverts for the three commits which as discussed I would like to drop. U-Boot should be able to run normally and exit without unbinding anything.
I cannot actually repeat the problem that Hans mentions in the above thread so I may be missing a step. Hans, any ideas on this?
So I've just tried this series on an allwinner tablet which uses a musb controller in host mode. Note that this has no root hub, so the first child of the controller is an actual device not a root hub.
When I first plug in a usb keyboard directly into the otg port and then do usb tree + dm tree (output shortened) I get:
USB device tree: 1 Human Interface (1.5 Mb/s, 100mA) SINO WEALTH USB Composite Device
=> dm tree Class Probed Name ---------------------------------------- usb [ + ] `-- sunxi-musb keyboard [ + ] `-- usb_kbd
If I then unplug the keyboard, plug in a hub and plug the keyboard into the hub and do a usb reset I get:
=> usb tree USB device tree: => dm tree Class Probed Name ---------------------------------------- usb [ + ] `-- sunxi-musb keyboard [ ] |-- usb_kbd usb_hub [ + ] `-- usb_hub keyboard [ + ] `-- usb_kbd
Notice how the "usb tree" command output is empty, that is because the usb tree code stops at the first removed usb-device. As discussed before if we go the route this patch-set is going we need to teach *all* code iterating over devices to skip removed devices, rather then to see a removed device as the end of the list.
At least thanks to the conversion of the usb-kbd driver to the dm the keyboard still works (which it did not in the past). That conversion has issues of its own btw, I will reply to that patch with my comments.
###
Related to the above (failed) test I believe that the first set of this patch:
Revert "dm: usb: Rename usb_find_child to usb_find_emul_child"
Is wrong and is the beginning of various problems, last time we discussed not making the usb code depend on the dm unbind code you suggested to simply remove the devices, and not re-use them ever (which means not using usb_find_child in non sandbox builds).
I believe that this is the right approach, re-using them will result in all kind of weirdness, let me give an example:
Plug in a hub, plug a keyb into port 1 and a storage device into port 2, do "usb tree" :
=> usb tree USB device tree: 1 Hub (480 Mb/s, 100mA) | USB2.0 Hub | +-2 Human Interface (1.5 Mb/s, 100mA) | SINO WEALTH USB Composite Device | +-3 Mass Storage (480 Mb/s, 100mA) USB Flash Disk 4C0E960F
No swap the 2 devices, so usb storage into port 1, keyb into port 2:
USB device tree: 1 Hub (480 Mb/s, 100mA) | USB2.0 Hub | +-3 Human Interface (1.5 Mb/s, 100mA) | SINO WEALTH USB Composite Device | +-2 Mass Storage (480 Mb/s, 100mA) USB Flash Disk 4C0E960F
And here we see that the usb stack now scans the storage-dev first and assigs it usb addr 2, but usb tree shows it after the keyb because the existing dm-device structs were re-used, and they appear out of order in the list now making the tree output no longer an accurate representation of the actual physical topology.
And if we add more levels of hubs etc, things will likely only get worse, not better, possibly leading to non cosmetic problems.
I believe that the way to make the dm usb code work without dm-unbind is to simply keep the removed devices, and make sure that all code going over the device tree ignores these removed usb devices (with the exception of core dm functions), and to NEVER re-use them.
That and in the case of not building without dm-unbind actually call device_unbind_children(bus), iow instead of reverting "dm: usb: Use device_unbind_children to clean up usb devs on stop" wrap the device_unbind_children(bus) call it adds in #ifdef CONFIG_DM_DEVICE_REMOVE
###
How ever we end up fixing this, I believe that this set certainly is not ready for merging into v2015.10.
Regards,
Hans
Hi Hans,
On 12 September 2015 at 08:00, Hans de Goede hdegoede@redhat.com wrote:
Hi,
On 08-09-15 19:15, Simon Glass wrote:
There was quite a bit of discussion about the change that required the unbinding of USB devices for the subsystem to function correctly. E.g.
https://patchwork.ozlabs.org/patch/485637/
The key issue is the usb_get_dev_index() function which is not a good API for driver model. We can drop use of this function once everything is converted to driver model. Then I believe the problems raised by Hans go away. For now we can add a deprecation warning on the function.
It is easy to convert USB keyboards to driver model. This series includes a patch for that.
This series also includes reverts for the three commits which as discussed I would like to drop. U-Boot should be able to run normally and exit without unbinding anything.
I cannot actually repeat the problem that Hans mentions in the above thread so I may be missing a step. Hans, any ideas on this?
So I've just tried this series on an allwinner tablet which uses a musb controller in host mode. Note that this has no root hub, so the first child of the controller is an actual device not a root hub.
When I first plug in a usb keyboard directly into the otg port and then do usb tree + dm tree (output shortened) I get:
USB device tree: 1 Human Interface (1.5 Mb/s, 100mA) SINO WEALTH USB Composite Device
=> dm tree Class Probed Name
usb [ + ] `-- sunxi-musb keyboard [ + ] `-- usb_kbd
If I then unplug the keyboard, plug in a hub and plug the keyboard into the hub and do a usb reset I get:
=> usb tree USB device tree: => dm tree Class Probed Name
usb [ + ] `-- sunxi-musb keyboard [ ] |-- usb_kbd usb_hub [ + ] `-- usb_hub keyboard [ + ] `-- usb_kbd
Notice how the "usb tree" command output is empty, that is because the usb tree code stops at the first removed usb-device. As discussed before if we go the route this patch-set is going we need to teach *all* code iterating over devices to skip removed devices, rather then to see a removed device as the end of the list.
At least thanks to the conversion of the usb-kbd driver to the dm the keyboard still works (which it did not in the past). That conversion has issues of its own btw, I will reply to that patch with my comments.
###
Related to the above (failed) test I believe that the first set of this patch:
Revert "dm: usb: Rename usb_find_child to usb_find_emul_child"
Is wrong and is the beginning of various problems, last time we discussed not making the usb code depend on the dm unbind code you suggested to simply remove the devices, and not re-use them ever (which means not using usb_find_child in non sandbox builds).
I believe that this is the right approach, re-using them will result in all kind of weirdness, let me give an example:
Plug in a hub, plug a keyb into port 1 and a storage device into port 2, do "usb tree" :
=> usb tree USB device tree: 1 Hub (480 Mb/s, 100mA) | USB2.0 Hub | +-2 Human Interface (1.5 Mb/s, 100mA) | SINO WEALTH USB Composite Device | +-3 Mass Storage (480 Mb/s, 100mA) USB Flash Disk 4C0E960F
No swap the 2 devices, so usb storage into port 1, keyb into port 2:
USB device tree: 1 Hub (480 Mb/s, 100mA) | USB2.0 Hub | +-3 Human Interface (1.5 Mb/s, 100mA) | SINO WEALTH USB Composite Device | +-2 Mass Storage (480 Mb/s, 100mA) USB Flash Disk 4C0E960F
And here we see that the usb stack now scans the storage-dev first and assigs it usb addr 2, but usb tree shows it after the keyb because the existing dm-device structs were re-used, and they appear out of order in the list now making the tree output no longer an accurate representation of the actual physical topology.
And if we add more levels of hubs etc, things will likely only get worse, not better, possibly leading to non cosmetic problems.
I believe that the way to make the dm usb code work without dm-unbind is to simply keep the removed devices, and make sure that all code going over the device tree ignores these removed usb devices (with the exception of core dm functions), and to NEVER re-use them.
That and in the case of not building without dm-unbind actually call device_unbind_children(bus), iow instead of reverting "dm: usb: Use device_unbind_children to clean up usb devs on stop" wrap the device_unbind_children(bus) call it adds in #ifdef CONFIG_DM_DEVICE_REMOVE
###
How ever we end up fixing this, I believe that this set certainly is not ready for merging into v2015.10.
Thanks for looking at this. I'm sorry I didn't get to it before. But your original patches fixed a bug so I was keen to to avoid holding things up. Let's target the next release.
My main objective is to make the unbinding optional, rather than required for USB to function.
I'll see if I can repeat your test case. It is the other way around from what I was trying, and from how I understood your original bug report: here you are plugging in a device, then removing it and plugging it in via a hub. I was doing it the other way around.
I think the best approach may be to create a sandbox test which checks this behaviour as well as the output of 'usb tree'.
Were you able to test the driver model USB keyboard support?
Regards, Simon
Hi,
On 12-09-15 17:11, Simon Glass wrote:
<snip>
Were you able to test the driver model USB keyboard support?
Yep as said that at least keeps the keyboard working with my test-case, where as before my test case not only made "usb tree" misbehave but also made the keyb no longer work.
I do believe we need to think a bit about how to deal with multiple keyboards though (no tested). I realize this is something the old code did not handle either, but that does not mean that it is not something which we should fix while switching to the dm.
Regards,
Hans
participants (3)
-
Hans de Goede -
Marek Vasut -
Simon Glass