| Message ID | 20200907155052.2450-2-yazzep@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] USB: hub.c: decrease the number of attempts of enumeration scheme | expand |
Dear Alan, Dear Greg, On Fri, Sep 11, 2020 at 11:12:28AM -0400, Alan Stern wrote: > The thing is, I'm afraid that without these retry loops, some devices > will stop working. If that happens, we will not be able to keep this > patch in place; we will just have to accept that we fail the PET test. > > Alan Stern Does this mean that Linux community leaves no choice but to ship a forked kernel (with no chance of alignment to upstream) for organizations which design embedded devices where USB plays a key role, hence requires passing the USB-IF Compliance Program [*]? Is there hope to give users a knob (build-time or run-time) which would enable the behavior expected and thoroughly described in compliance docs, particularly chapter "6.7.22 A-UUT Device No Response for connection timeout" of "USB On-The-Go and Embedded Host Automated Compliance Plan" [**]? [*] https://www.usb.org/compliance [**] https://www.usb.org/sites/default/files/otgeh_compliance_plan_1_2.pdf -- Best regards, Eugeniu Rosca
On Tue, Sep 15, 2020 at 11:45:31AM +0200, Eugeniu Rosca wrote: > Dear Alan, > Dear Greg, > > On Fri, Sep 11, 2020 at 11:12:28AM -0400, Alan Stern wrote: > > > The thing is, I'm afraid that without these retry loops, some devices > > will stop working. If that happens, we will not be able to keep this > > patch in place; we will just have to accept that we fail the PET test. > > > > Alan Stern > > Does this mean that Linux community leaves no choice but to ship a > forked kernel (with no chance of alignment to upstream) for > organizations which design embedded devices where USB plays a key > role, hence requires passing the USB-IF Compliance Program [*]? We are saying that if you ship such a kernel, we _KNOW_ that it will fail to work in a number of known systems. I doubt you want that to happen if you care about shipping a device, right? > Is there hope to give users a knob (build-time or run-time) which would > enable the behavior expected and thoroughly described in compliance > docs, particularly chapter "6.7.22 A-UUT Device No Response for > connection timeout" of "USB On-The-Go and Embedded Host Automated > Compliance Plan" [**]? Given that the USB-IF has explicitly kicked-out the Linux community from its specification work and orginization, I personally don't really care what they say here. If you are a member of the USB-IF, please work with them to fix the test to reflect real-world systems and not an idealized system. Last I heard, they wanted nothing to do with Linux systems at all :( thanks, greg k-h
On Tue, Sep 15, 2020 at 11:45:31AM +0200, Eugeniu Rosca wrote: > Dear Alan, > Dear Greg, > > On Fri, Sep 11, 2020 at 11:12:28AM -0400, Alan Stern wrote: > > > The thing is, I'm afraid that without these retry loops, some devices > > will stop working. If that happens, we will not be able to keep this > > patch in place; we will just have to accept that we fail the PET test. > > > > Alan Stern > > Does this mean that Linux community leaves no choice but to ship a > forked kernel (with no chance of alignment to upstream) for > organizations which design embedded devices where USB plays a key > role, hence requires passing the USB-IF Compliance Program [*]? > > Is there hope to give users a knob (build-time or run-time) which would > enable the behavior expected and thoroughly described in compliance > docs, particularly chapter "6.7.22 A-UUT Device No Response for > connection timeout" of "USB On-The-Go and Embedded Host Automated > Compliance Plan" [**]? It is possible to add a build-time Kconfig option that would control the maximum number of port initialization attempts. But let's start with testing the patch I sent you. After all, the first step is to get something that does what you want correctly. Alan Stern
On Tue, Sep 15, 2020 at 01:01:11PM +0200, Greg KH wrote: > On Tue, Sep 15, 2020 at 11:45:31AM +0200, Eugeniu Rosca wrote: > > Dear Alan, > > Dear Greg, > > > > On Fri, Sep 11, 2020 at 11:12:28AM -0400, Alan Stern wrote: > > > > > The thing is, I'm afraid that without these retry loops, some devices > > > will stop working. If that happens, we will not be able to keep this > > > patch in place; we will just have to accept that we fail the PET test. > > > > > > Alan Stern > > > > Does this mean that Linux community leaves no choice but to ship a > > forked kernel (with no chance of alignment to upstream) for > > organizations which design embedded devices where USB plays a key > > role, hence requires passing the USB-IF Compliance Program [*]? > > We are saying that if you ship such a kernel, we _KNOW_ that it will > fail to work in a number of known systems. I doubt you want that to > happen if you care about shipping a device, right? > > > Is there hope to give users a knob (build-time or run-time) which would > > enable the behavior expected and thoroughly described in compliance > > docs, particularly chapter "6.7.22 A-UUT Device No Response for > > connection timeout" of "USB On-The-Go and Embedded Host Automated > > Compliance Plan" [**]? > > Given that the USB-IF has explicitly kicked-out the Linux community from > its specification work and orginization, I personally don't really care > what they say here. If you are a member of the USB-IF, please work with > them to fix the test to reflect real-world systems and not an idealized > system. Last I heard, they wanted nothing to do with Linux systems at > all :( <irony>If the USB-IF were the final authority regarding USB devices, we wouldn't have this problem in the first place.</irony> Every device would respond properly to the very first port initialization attempt and no retries would be needed. But real hardware doesn't always follow the official spec. And then when it fails to work with Linux, _we_ are the people who get blamed. Alan Stern
Dear Alan Dear Greg Thank you very much for your feedback. I understood that there is a gap between real system and ideal specification, and the Linux community stands on the real system side.(of course). I really appreciate your proposal(Kconfig option). > But let's start with > testing the patch I sent you. After all, the first step is to get > something that does what you want correctly. The patch you provided worked fine. https://marc.info/?l=linux-usb&m=159984230312068&w=4 An excerpt from dmesg is as follows. It detected the enumeration failure after 27.7seconds since the test started. so,the PET test passed. [2]-[1] =27.7seconds [ 111.482541] *** Setting Test Suite *** [ 130.226648] *** Ready OK Test Start*** [ 134.808206] usb 1-1.1: new full-speed USB device number 7 using ehci-platform ... [1] [ 140.034944] usb 1-1.1: device descriptor read/64, error -110 [ 140.118069] usb 1-1.1: new full-speed USB device number 8 using ehci-platform [ 145.155142] usb 1-1.1: device descriptor read/64, error -110 [ 145.163074] usb 1-1-port1: attempt power cycle [ 147.037094] usb 1-1.1: new full-speed USB device number 9 using ehci-platform [ 152.323168] usb 1-1.1: device descriptor read/64, error -110 [ 152.407069] usb 1-1.1: new full-speed USB device number 10 using ehci-platform [ 157.442518] usb 1-1.1: device not accepting address 10, error -110 [ 157.527067] usb 1-1.1: new full-speed USB device number 11 using ehci-platform [ 162.563123] usb 1-1.1: device not accepting address 11, error -110 [ 162.571302] usb 1-1-port1: unable to enumerate USB device ... [2] [ 176.523060] *** End of Test *** 2020年9月15日(火) 23:52 Alan Stern <stern@rowland.harvard.edu>: > > On Tue, Sep 15, 2020 at 01:01:11PM +0200, Greg KH wrote: > > On Tue, Sep 15, 2020 at 11:45:31AM +0200, Eugeniu Rosca wrote: > > > Dear Alan, > > > Dear Greg, > > > > > > On Fri, Sep 11, 2020 at 11:12:28AM -0400, Alan Stern wrote: > > > > > > > The thing is, I'm afraid that without these retry loops, some devices > > > > will stop working. If that happens, we will not be able to keep this > > > > patch in place; we will just have to accept that we fail the PET test. > > > > > > > > Alan Stern > > > > > > Does this mean that Linux community leaves no choice but to ship a > > > forked kernel (with no chance of alignment to upstream) for > > > organizations which design embedded devices where USB plays a key > > > role, hence requires passing the USB-IF Compliance Program [*]? > > > > We are saying that if you ship such a kernel, we _KNOW_ that it will > > fail to work in a number of known systems. I doubt you want that to > > happen if you care about shipping a device, right? > > > > > Is there hope to give users a knob (build-time or run-time) which would > > > enable the behavior expected and thoroughly described in compliance > > > docs, particularly chapter "6.7.22 A-UUT Device No Response for > > > connection timeout" of "USB On-The-Go and Embedded Host Automated > > > Compliance Plan" [**]? > > > > Given that the USB-IF has explicitly kicked-out the Linux community from > > its specification work and orginization, I personally don't really care > > what they say here. If you are a member of the USB-IF, please work with > > them to fix the test to reflect real-world systems and not an idealized > > system. Last I heard, they wanted nothing to do with Linux systems at > > all :( > > <irony>If the USB-IF were the final authority regarding USB devices, we > wouldn't have this problem in the first place.</irony> Every device > would respond properly to the very first port initialization attempt and > no retries would be needed. > > But real hardware doesn't always follow the official spec. And then > when it fails to work with Linux, _we_ are the people who get blamed. > > Alan Stern
Dear Alan, Could you please proceed with your proposal(Kconfig option)? After all, Customer products need to get USB certification. Thank you for considering my request. 2020年9月16日(水) 19:16 yasushi asano <yazzep@gmail.com>: > > Dear Alan > Dear Greg > > Thank you very much for your feedback. > I understood that there is a gap between real system and ideal specification, > and the Linux community stands on the real system side.(of course). > > I really appreciate your proposal(Kconfig option). > > > But let's start with > > testing the patch I sent you. After all, the first step is to get > > something that does what you want correctly. > > The patch you provided worked fine. > https://marc.info/?l=linux-usb&m=159984230312068&w=4 > An excerpt from dmesg is as follows. > It detected the enumeration failure after 27.7seconds since the test started. > so,the PET test passed. [2]-[1] =27.7seconds > > [ 111.482541] *** Setting Test Suite *** > [ 130.226648] *** Ready OK Test Start*** > [ 134.808206] usb 1-1.1: new full-speed USB device number 7 using > ehci-platform ... [1] > [ 140.034944] usb 1-1.1: device descriptor read/64, error -110 > [ 140.118069] usb 1-1.1: new full-speed USB device number 8 using ehci-platform > [ 145.155142] usb 1-1.1: device descriptor read/64, error -110 > [ 145.163074] usb 1-1-port1: attempt power cycle > [ 147.037094] usb 1-1.1: new full-speed USB device number 9 using ehci-platform > [ 152.323168] usb 1-1.1: device descriptor read/64, error -110 > [ 152.407069] usb 1-1.1: new full-speed USB device number 10 using > ehci-platform > [ 157.442518] usb 1-1.1: device not accepting address 10, error -110 > [ 157.527067] usb 1-1.1: new full-speed USB device number 11 using > ehci-platform > [ 162.563123] usb 1-1.1: device not accepting address 11, error -110 > [ 162.571302] usb 1-1-port1: unable to enumerate USB device ... [2] > [ 176.523060] *** End of Test *** > > 2020年9月15日(火) 23:52 Alan Stern <stern@rowland.harvard.edu>: > > > > On Tue, Sep 15, 2020 at 01:01:11PM +0200, Greg KH wrote: > > > On Tue, Sep 15, 2020 at 11:45:31AM +0200, Eugeniu Rosca wrote: > > > > Dear Alan, > > > > Dear Greg, > > > > > > > > On Fri, Sep 11, 2020 at 11:12:28AM -0400, Alan Stern wrote: > > > > > > > > > The thing is, I'm afraid that without these retry loops, some devices > > > > > will stop working. If that happens, we will not be able to keep this > > > > > patch in place; we will just have to accept that we fail the PET test. > > > > > > > > > > Alan Stern > > > > > > > > Does this mean that Linux community leaves no choice but to ship a > > > > forked kernel (with no chance of alignment to upstream) for > > > > organizations which design embedded devices where USB plays a key > > > > role, hence requires passing the USB-IF Compliance Program [*]? > > > > > > We are saying that if you ship such a kernel, we _KNOW_ that it will > > > fail to work in a number of known systems. I doubt you want that to > > > happen if you care about shipping a device, right? > > > > > > > Is there hope to give users a knob (build-time or run-time) which would > > > > enable the behavior expected and thoroughly described in compliance > > > > docs, particularly chapter "6.7.22 A-UUT Device No Response for > > > > connection timeout" of "USB On-The-Go and Embedded Host Automated > > > > Compliance Plan" [**]? > > > > > > Given that the USB-IF has explicitly kicked-out the Linux community from > > > its specification work and orginization, I personally don't really care > > > what they say here. If you are a member of the USB-IF, please work with > > > them to fix the test to reflect real-world systems and not an idealized > > > system. Last I heard, they wanted nothing to do with Linux systems at > > > all :( > > > > <irony>If the USB-IF were the final authority regarding USB devices, we > > wouldn't have this problem in the first place.</irony> Every device > > would respond properly to the very first port initialization attempt and > > no retries would be needed. > > > > But real hardware doesn't always follow the official spec. And then > > when it fails to work with Linux, _we_ are the people who get blamed. > > > > Alan Stern
On Sat, Sep 19, 2020 at 12:00:10AM +0900, yasushi asano wrote: > Dear Alan, > Could you please proceed with your proposal(Kconfig option)? > After all, Customer products need to get USB certification. > Thank you for considering my request. The patch is below. It is simpler than what we were looking at before, more like what you were working on originally. This actually is a combination of two separate patches, one to replace SET_CONFIG_TRIES with PORT_INIT_TRIES and improve use_new_scheme(), the other to add the USB_FEW_INIT_RETRIES Kconfig option. Even so, it's not very complicated. Alan Stern Index: usb-devel/drivers/usb/core/hub.c =================================================================== --- usb-devel.orig/drivers/usb/core/hub.c +++ usb-devel/drivers/usb/core/hub.c @@ -2705,11 +2705,18 @@ static unsigned hub_is_wusb(struct usb_h } +#ifdef CONFIG_USB_FEW_INIT_RETRIES +#define PORT_RESET_TRIES 2 +#define SET_ADDRESS_TRIES 1 +#define GET_DESCRIPTOR_TRIES 1 +#define PORT_INIT_TRIES 5 + +#else #define PORT_RESET_TRIES 5 #define SET_ADDRESS_TRIES 2 #define GET_DESCRIPTOR_TRIES 2 -#define SET_CONFIG_TRIES (2 * (use_both_schemes + 1)) -#define USE_NEW_SCHEME(i, scheme) ((i) / 2 == (int)(scheme)) +#define PORT_INIT_TRIES 4 +#endif /* CONFIG_USB_FEW_INIT_RETRIES */ #define HUB_ROOT_RESET_TIME 60 /* times are in msec */ #define HUB_SHORT_RESET_TIME 10 @@ -2717,23 +2724,31 @@ static unsigned hub_is_wusb(struct usb_h #define HUB_LONG_RESET_TIME 200 #define HUB_RESET_TIMEOUT 800 -/* - * "New scheme" enumeration causes an extra state transition to be - * exposed to an xhci host and causes USB3 devices to receive control - * commands in the default state. This has been seen to cause - * enumeration failures, so disable this enumeration scheme for USB3 - * devices. - */ static bool use_new_scheme(struct usb_device *udev, int retry, struct usb_port *port_dev) { int old_scheme_first_port = - port_dev->quirks & USB_PORT_QUIRK_OLD_SCHEME; + (port_dev->quirks & USB_PORT_QUIRK_OLD_SCHEME) || + old_scheme_first; + /* + * "New scheme" enumeration causes an extra state transition to be + * exposed to an xhci host and causes USB3 devices to receive control + * commands in the default state. This has been seen to cause + * enumeration failures, so disable this enumeration scheme for USB3 + * devices. + */ if (udev->speed >= USB_SPEED_SUPER) return false; - return USE_NEW_SCHEME(retry, old_scheme_first_port || old_scheme_first); + /* + * If use_both_schemes is set, use the first scheme (whichever + * it is) for the larger half of the retries, then use the other + * scheme. Otherwise, use the first scheme for all the retries. + */ + if (use_both_schemes && retry >= (PORT_INIT_TRIES + 1) / 2) + return old_scheme_first_port; /* Second half */ + return !old_scheme_first_port; /* First half or all */ } /* Is a USB 3.0 port in the Inactive or Compliance Mode state? @@ -4545,6 +4560,7 @@ hub_port_init(struct usb_hub *hub, struc const char *speed; int devnum = udev->devnum; const char *driver_name; + bool do_new_scheme; /* root hub ports have a slightly longer reset period * (from USB 2.0 spec, section 7.1.7.5) @@ -4657,14 +4673,13 @@ hub_port_init(struct usb_hub *hub, struc * first 8 bytes of the device descriptor to get the ep0 maxpacket * value. */ - for (retries = 0; retries < GET_DESCRIPTOR_TRIES; (++retries, msleep(100))) { - bool did_new_scheme = false; + do_new_scheme = use_new_scheme(udev, retry_counter, port_dev); - if (use_new_scheme(udev, retry_counter, port_dev)) { + for (retries = 0; retries < GET_DESCRIPTOR_TRIES; (++retries, msleep(100))) { + if (do_new_scheme) { struct usb_device_descriptor *buf; int r = 0; - did_new_scheme = true; retval = hub_enable_device(udev); if (retval < 0) { dev_err(&udev->dev, @@ -4773,11 +4788,7 @@ hub_port_init(struct usb_hub *hub, struc * - read ep0 maxpacket even for high and low speed, */ msleep(10); - /* use_new_scheme() checks the speed which may have - * changed since the initial look so we cache the result - * in did_new_scheme - */ - if (did_new_scheme) + if (do_new_scheme) break; } @@ -5106,7 +5117,7 @@ static void hub_port_connect(struct usb_ unit_load = 100; status = 0; - for (i = 0; i < SET_CONFIG_TRIES; i++) { + for (i = 0; i < PORT_INIT_TRIES; i++) { /* reallocate for each attempt, since references * to the previous one can escape in various ways @@ -5239,7 +5250,7 @@ loop: break; /* When halfway through our retry count, power-cycle the port */ - if (i == (SET_CONFIG_TRIES / 2) - 1) { + if (i == (PORT_INIT_TRIES / 2) - 1) { dev_info(&port_dev->dev, "attempt power cycle\n"); usb_hub_set_port_power(hdev, hub, port1, false); msleep(2 * hub_power_on_good_delay(hub)); @@ -5770,7 +5781,7 @@ static int usb_reset_and_verify_device(s bos = udev->bos; udev->bos = NULL; - for (i = 0; i < SET_CONFIG_TRIES; ++i) { + for (i = 0; i < PORT_INIT_TRIES; ++i) { /* ep0 maxpacket size may change; let the HCD know about it. * Other endpoints will be handled by re-enumeration. */ Index: usb-devel/drivers/usb/core/Kconfig =================================================================== --- usb-devel.orig/drivers/usb/core/Kconfig +++ usb-devel/drivers/usb/core/Kconfig @@ -32,6 +32,20 @@ config USB_DEFAULT_PERSIST If you have any questions about this, say Y here, only say N if you know exactly what you are doing. +config USB_FEW_INIT_RETRIES + bool "Limit USB device initialization to only a few retries" + help + When a new USB device is detected, the kernel tries very hard + to initialize and enumerate it, with lots of nested retry loops. + This almost always works, but when it fails it can take a long time. + This option tells the kernel to make only a few retry attempts, + so that the total time required for a failed initialization is + no more than 30 seconds (as required by the USB OTG spec). + + Say N here unless you require new-device enumeration failure to + occur within 30 seconds (as might be needed in an embedded + application). + config USB_DYNAMIC_MINORS bool "Dynamic USB minor allocation" help
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index 052d5ac..725e5b6 100644 --- a/drivers/usb/core/hub.c +++ b/drivers/usb/core/hub.c @@ -2706,7 +2706,6 @@ static unsigned hub_is_wusb(struct usb_hub *hub) #define PORT_RESET_TRIES 5 -#define SET_ADDRESS_TRIES 2 #define GET_DESCRIPTOR_TRIES 2 #define SET_CONFIG_TRIES (2 * (use_both_schemes + 1)) #define USE_NEW_SCHEME(i, scheme) ((i) / 2 == (int)(scheme)) @@ -4539,7 +4538,7 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1, struct usb_device *hdev = hub->hdev; struct usb_hcd *hcd = bus_to_hcd(hdev->bus); struct usb_port *port_dev = hub->ports[port1 - 1]; - int retries, operations, retval, i; + int retries, retval, i; unsigned delay = HUB_SHORT_RESET_TIME; enum usb_device_speed oldspeed = udev->speed; const char *speed; @@ -4684,7 +4683,7 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1, * 255 is for WUSB devices, we actually need to use * 512 (WUSB1.0[4.8.1]). */ - for (operations = 0; operations < 3; ++operations) { + if (!((retry_counter % 2 != 0) && (retries != 0))) { buf->bMaxPacketSize0 = 0; r = usb_control_msg(udev, usb_rcvaddr0pipe(), USB_REQ_GET_DESCRIPTOR, USB_DIR_IN, @@ -4704,16 +4703,6 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1, r = -EPROTO; break; } - /* - * Some devices time out if they are powered on - * when already connected. They need a second - * reset. But only on the first attempt, - * lest we get into a time out/reset loop - */ - if (r == 0 || (r == -ETIMEDOUT && - retries == 0 && - udev->speed > USB_SPEED_FULL)) - break; } udev->descriptor.bMaxPacketSize0 = buf->bMaxPacketSize0; @@ -4744,12 +4733,8 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1, * authorization will assign the final address. */ if (udev->wusb == 0) { - for (operations = 0; operations < SET_ADDRESS_TRIES; ++operations) { - retval = hub_set_address(udev, devnum); - if (retval >= 0) - break; - msleep(200); - } + retval = hub_set_address(udev, devnum); + msleep(200); if (retval < 0) { if (retval != -ENODEV) dev_err(&udev->dev, "device not accepting address %d, error %d\n",