From: yasushi asano <yazzep@gmail.com>
To: Alan Stern <stern@rowland.harvard.edu>
Cc: "Greg KH" <gregkh@linuxfoundation.org>,
linux-usb@vger.kernel.org, "Rosca,
Eugeniu (ADITG/ESM1)" <erosca@de.adit-jv.com>,
andrew_gabbasov@mentor.com, "Baxter Jim" <jim_baxter@mentor.com>,
"Natsume, Wataru (ADITJ/SWG)" <wnatsume@jp.adit-jv.com>,
"Nishiguchi, Naohiro (ADITJ/SWG)" <nnishiguchi@jp.adit-jv.com>,
浅野恭史 <yasano@jp.adit-jv.com>,
"kernel test robot" <rong.a.chen@intel.com>
Subject: Re: [PATCH v3] USB: hub.c: decrease the number of attempts of enumeration scheme
Date: Fri, 11 Sep 2020 17:33:18 +0900 [thread overview]
Message-ID: <CAEt1RjpGcZ4T70tr83pmcD--PzAMboBkbv55qFcRfMz11ZUggw@mail.gmail.com> (raw)
In-Reply-To: <CAEt1RjquJZzTctN6dNQSDbUZ9YG2FnEtzTZsoA3a9RtXHxwUmA@mail.gmail.com>
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@gmail.com>:
>
> 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@rowland.harvard.edu>:
> >
> > On Tue, Sep 08, 2020 at 12:50:52AM +0900, Yasushi Asano wrote:
> > > From: Yasushi Asano <yasano@jp.adit-jv.com>
> > >
> > > 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. */
> >
next prev parent reply other threads:[~2020-09-11 8:34 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <CAEt1RjrQsb6=reKUKV9uJTG4JoJXErhJFj=2TdVx=N1_Ad1GVg@mail.gmail.com>
2020-08-08 6:40 ` [PATCH] [RFC] USB: hub.c: decrease the number of attempts of enumeration scheme Yasushi Asano
2020-08-08 15:16 ` Alan Stern
2020-08-10 0:19 ` [PATCH v2] [RFC] USB: hub.c: decrease the number of attempts of enumeration Yasushi Asano
2020-08-10 0:19 ` [PATCH v2] [RFC] USB: hub.c: decrease the number of attempts of enumeration scheme Yasushi Asano
2020-08-10 7:46 ` Greg KH
2020-08-11 15:20 ` yasushi asano
2020-09-07 15:50 ` [PATCH v3] " Yasushi Asano
2020-09-07 15:50 ` Yasushi Asano
2020-09-08 19:04 ` Alan Stern
2020-09-10 4:49 ` yasushi asano
2020-09-11 8:33 ` yasushi asano [this message]
2020-09-11 15:12 ` Alan Stern
2020-09-15 9:45 ` Eugeniu Rosca
2020-09-15 11:01 ` Greg KH
2020-09-15 14:52 ` Alan Stern
2020-09-16 10:16 ` yasushi asano
2020-09-18 15:00 ` yasushi asano
2020-09-20 19:21 ` Alan Stern
2020-09-21 14:03 ` [PATCH] " Yasushi Asano
2020-09-21 14:48 ` Alan Stern
[not found] ` <CAEt1Rjq-DOwN0+_7F0m-kqUHTzm5YPUaXqUOpTszCsqrfLRt5w@mail.gmail.com>
2020-09-21 15:06 ` Alan Stern
2020-09-25 1:05 ` yasushi asano
2020-09-25 17:21 ` yasushi asano
2020-09-25 18:41 ` Alan Stern
2020-09-27 15:43 ` yasushi asano
2020-09-28 15:20 ` [Patch 1/2]: USB: hub: Clean up use of port initialization schemes and retries Alan Stern
2020-09-28 15:22 ` [Patch 2/2]: USB: hub: Add Kconfig option to reduce number of port initialization retries Alan Stern
2020-09-15 14:48 ` [PATCH v3] USB: hub.c: decrease the number of attempts of enumeration scheme Alan Stern
2020-08-12 17:09 ` [PATCH v2] " Yasushi Asano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAEt1RjpGcZ4T70tr83pmcD--PzAMboBkbv55qFcRfMz11ZUggw@mail.gmail.com \
--to=yazzep@gmail.com \
--cc=andrew_gabbasov@mentor.com \
--cc=erosca@de.adit-jv.com \
--cc=gregkh@linuxfoundation.org \
--cc=jim_baxter@mentor.com \
--cc=linux-usb@vger.kernel.org \
--cc=nnishiguchi@jp.adit-jv.com \
--cc=rong.a.chen@intel.com \
--cc=stern@rowland.harvard.edu \
--cc=wnatsume@jp.adit-jv.com \
--cc=yasano@jp.adit-jv.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.