Dear Alan,
I tested the patch you provided.
Unfortunately, it takes about 40 seconds to reach the detection of
enumeration failure. so the PET test failed.
Now I'm checking which procedure took time.
:
[ 77.469035] hrtimer: interrupt took 23800 ns
[ 737.812782] *** Connect PET ***
[ 759.854355] *** Exec PET App ***
[ 763.300951] usb 1-1.1: new full-speed USB device number 4 using ehci-platform
[ 763.301248] usb 1-1.1: device descriptor read/64, error -32
[ 763.383966] usb 1-1.1: new full-speed USB device number 5 using ehci-platform
[ 763.384256] usb 1-1.1: device descriptor read/64, error -32
[ 763.390282] usb 1-1-port1: attempt power cycle
[ 765.586566] usb 1-1-port1: unable to enumerate USB device [-107]
[ 816.850692] *** Setting Test Suite ***
[ 835.593181] *** Ready OK Test Start***
[ 838.822953] usb 1-1.1: new full-speed USB device number 7 using
ehci-platform [1]---start---------
[ 844.037032] usb 1-1.1: device descriptor read/64, error -110
[2]....... [2]-[1] = 5.2 second
[ 844.121947] usb 1-1.1: new full-speed USB device number 8 using ehci-platform
[ 849.156628] usb 1-1.1: device descriptor read/64, error -110
[3]....... [3]-[2] = 5.1 second
[ 849.163971] usb 1-1-port1: attempt power cycle
[ 851.102959] usb 1-1.1: new full-speed USB device number 9 using ehci-platform
[ 856.325028] usb 1-1.1: device descriptor read/64, error -110
[4]....... [4]-[3] = 7.2 second
[ 856.409962] usb 1-1.1: new full-speed USB device number 10 using
ehci-platform
[ 867.281957] usb 1-1.1: device not accepting address 10, error -110
[5]....... [5]-[4] = 10.9 second
[ 867.365954] usb 1-1.1: new full-speed USB device number 11 using
ehci-platform
[ 878.545941] usb 1-1.1: device not accepting address 11, error -110
[6]....... [6]-[5] = 11.2 second
[ 878.552808] usb 1-1-port1: unable to enumerate USB device
[7] total [7]-[1] = 39.7 second
[ 899.489808] *** End of Test ***
2020年9月10日(木) 13:49 yasushi asano <yazzep@xxxxxxxxx>:
>
> Dear Alan
> Thank you for your kindness.
> I tried to minimize the amount of change so as not to affect other
> processing, but I understood that my fix was not appropriate.
> I'm testing the patch you offered using PET tool.
> Please wait a while.
>
> 2020年9月9日(水) 4:04 Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>:
> >
> > On Tue, Sep 08, 2020 at 12:50:52AM +0900, Yasushi Asano wrote:
> > > From: Yasushi Asano <yasano@xxxxxxxxxxxxxx>
> > >
> > > According to 6.7.22 A-UUT "Device No Response" for connection timeout
> > > of USB OTG and EH automated compliance plan v1.2, the enumeration
> > > failure has to be detected within 30 seconds. However, the old and new
> > > enumeration schemes made a total of 16 attempts, and each attempt can
> > > take 5 seconds to timeout, so it failed with PET test. Modify it to
> > > reduce the number of attempts to 5 and pass PET test.
> > >
> > > in case of old_schene_first=N and use_both_schemes=Y
> > > attempt 3 * new scheme, then 2 * old scheme
> > > in case of old_schene_first=Y and use_both_schemes=Y
> > > attempt 2 * old scheme, then 3 * new scheme
> >
> > There are several issues this patch does not take into account, such as
> > resets between retries and port-power cycling. Also, you did not
> > restructure the code appropriately.
> >
> > Please review and test the patch below. Does it do what you think it
> > should?
> >
> > 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
> > @@ -2707,9 +2707,7 @@ static unsigned hub_is_wusb(struct usb_h
> >
> > #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 5
> >
> > #define HUB_ROOT_RESET_TIME 60 /* times are in msec */
> > #define HUB_SHORT_RESET_TIME 10
> > @@ -2717,23 +2715,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?
> > @@ -4539,12 +4545,13 @@ hub_port_init(struct usb_hub *hub, struc
> > 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 operations, retval, i;
> > unsigned delay = HUB_SHORT_RESET_TIME;
> > enum usb_device_speed oldspeed = udev->speed;
> > 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,130 +4664,106 @@ 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;
> > -
> > - if (use_new_scheme(udev, retry_counter, port_dev)) {
> > - struct usb_device_descriptor *buf;
> > - int r = 0;
> > + do_new_scheme = use_new_scheme(udev, retry_counter, port_dev);
> >
> > - did_new_scheme = true;
> > - retval = hub_enable_device(udev);
> > - if (retval < 0) {
> > - dev_err(&udev->dev,
> > - "hub failed to enable device, error %d\n",
> > - retval);
> > - goto fail;
> > - }
> > + if (do_new_scheme) {
> > + struct usb_device_descriptor *buf;
> > + int r = 0;
> > +
> > + retval = hub_enable_device(udev);
> > + if (retval < 0) {
> > + dev_err(&udev->dev,
> > + "hub failed to enable device, error %d\n",
> > + retval);
> > + goto fail;
> > + }
> >
> > #define GET_DESCRIPTOR_BUFSIZE 64
> > - buf = kmalloc(GET_DESCRIPTOR_BUFSIZE, GFP_NOIO);
> > - if (!buf) {
> > - retval = -ENOMEM;
> > - continue;
> > - }
> > + buf = kmalloc(GET_DESCRIPTOR_BUFSIZE, GFP_NOIO);
> > + if (!buf) {
> > + retval = -ENOMEM;
> > + goto fail;
> > + }
> >
> > - /* Retry on all errors; some devices are flakey.
> > - * 255 is for WUSB devices, we actually need to use
> > - * 512 (WUSB1.0[4.8.1]).
> > - */
> > - for (operations = 0; operations < 3; ++operations) {
> > - buf->bMaxPacketSize0 = 0;
> > - r = usb_control_msg(udev, usb_rcvaddr0pipe(),
> > - USB_REQ_GET_DESCRIPTOR, USB_DIR_IN,
> > - USB_DT_DEVICE << 8, 0,
> > - buf, GET_DESCRIPTOR_BUFSIZE,
> > - initial_descriptor_timeout);
> > - switch (buf->bMaxPacketSize0) {
> > - case 8: case 16: case 32: case 64: case 255:
> > - if (buf->bDescriptorType ==
> > - USB_DT_DEVICE) {
> > - r = 0;
> > - break;
> > - }
> > - fallthrough;
> > - default:
> > - if (r == 0)
> > - 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;
> > + /*
> > + * 255 is for WUSB devices, we actually need to use
> > + * 512 (WUSB1.0[4.8.1]).
> > + */
> > + buf->bMaxPacketSize0 = 0;
> > + r = usb_control_msg(udev, usb_rcvaddr0pipe(),
> > + USB_REQ_GET_DESCRIPTOR, USB_DIR_IN,
> > + USB_DT_DEVICE << 8, 0,
> > + buf, GET_DESCRIPTOR_BUFSIZE,
> > + initial_descriptor_timeout);
> > + switch (buf->bMaxPacketSize0) {
> > + case 8: case 16: case 32: case 64: case 255:
> > + if (buf->bDescriptorType == USB_DT_DEVICE) {
> > + r = 0;
> > + break;
> > }
> > - udev->descriptor.bMaxPacketSize0 =
> > - buf->bMaxPacketSize0;
> > + fallthrough;
> > + default:
> > + if (r == 0)
> > + r = -EPROTO;
> > + if (r != -ENODEV)
> > + dev_err(&udev->dev, "device descriptor read/64, error %d\n", r);
> > + retval = r;
> > kfree(buf);
> > + goto fail;
> > + }
> > + udev->descriptor.bMaxPacketSize0 = buf->bMaxPacketSize0;
> > + kfree(buf);
> >
> > - retval = hub_port_reset(hub, port1, udev, delay, false);
> > - if (retval < 0) /* error or disconnect */
> > - goto fail;
> > - if (oldspeed != udev->speed) {
> > - dev_dbg(&udev->dev,
> > - "device reset changed speed!\n");
> > - retval = -ENODEV;
> > - goto fail;
> > - }
> > - if (r) {
> > - if (r != -ENODEV)
> > - dev_err(&udev->dev, "device descriptor read/64, error %d\n",
> > - r);
> > - retval = -EMSGSIZE;
> > - continue;
> > - }
> > + retval = hub_port_reset(hub, port1, udev, delay, false);
> > + if (retval < 0) /* error or disconnect */
> > + goto fail;
> > + if (oldspeed != udev->speed) {
> > + dev_dbg(&udev->dev, "device reset changed speed!\n");
> > + retval = -ENODEV;
> > + goto fail;
> > + }
> > #undef GET_DESCRIPTOR_BUFSIZE
> > + }
> > +
> > + /*
> > + * If device is WUSB, we already assigned an
> > + * unauthorized address in the Connect Ack sequence;
> > + * 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);
> > + }
> > + if (retval < 0) {
> > + if (retval != -ENODEV)
> > + dev_err(&udev->dev, "device not accepting address %d, error %d\n",
> > + devnum, retval);
> > + goto fail;
> > + }
> > + if (udev->speed >= USB_SPEED_SUPER) {
> > + devnum = udev->devnum;
> > + dev_info(&udev->dev,
> > + "%s SuperSpeed%s%s USB device number %d using %s\n",
> > + (udev->config) ? "reset" : "new",
> > + (udev->speed == USB_SPEED_SUPER_PLUS) ?
> > + "Plus Gen 2" : " Gen 1",
> > + (udev->rx_lanes == 2 && udev->tx_lanes == 2) ?
> > + "x2" : "",
> > + devnum, driver_name);
> > }
> >
> > /*
> > - * If device is WUSB, we already assigned an
> > - * unauthorized address in the Connect Ack sequence;
> > - * authorization will assign the final address.
> > + * cope with hardware quirkiness:
> > + * - let SET_ADDRESS settle, some device hardware wants it
> > + * - read ep0 maxpacket even for high and low speed,
> > */
> > - if (udev->wusb == 0) {
> > - for (operations = 0; operations < SET_ADDRESS_TRIES; ++operations) {
> > - retval = hub_set_address(udev, devnum);
> > - if (retval >= 0)
> > - break;
> > - msleep(200);
> > - }
> > - if (retval < 0) {
> > - if (retval != -ENODEV)
> > - dev_err(&udev->dev, "device not accepting address %d, error %d\n",
> > - devnum, retval);
> > - goto fail;
> > - }
> > - if (udev->speed >= USB_SPEED_SUPER) {
> > - devnum = udev->devnum;
> > - dev_info(&udev->dev,
> > - "%s SuperSpeed%s%s USB device number %d using %s\n",
> > - (udev->config) ? "reset" : "new",
> > - (udev->speed == USB_SPEED_SUPER_PLUS) ?
> > - "Plus Gen 2" : " Gen 1",
> > - (udev->rx_lanes == 2 && udev->tx_lanes == 2) ?
> > - "x2" : "",
> > - devnum, driver_name);
> > - }
> > -
> > - /* cope with hardware quirkiness:
> > - * - let SET_ADDRESS settle, some device hardware wants it
> > - * - 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)
> > - break;
> > - }
> > + msleep(10);
> > + }
> >
> > + if (!do_new_scheme) {
> > retval = usb_get_device_descriptor(udev, 8);
> > if (retval < 8) {
> > if (retval != -ENODEV)
> > @@ -4804,7 +4787,6 @@ hub_port_init(struct usb_hub *hub, struc
> > retval);
> > retval = 0;
> > }
> > - break;
> > }
> > }
> > if (retval)
> > @@ -5106,7 +5088,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 +5221,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 +5752,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. */
> >
