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. */
>
