* [PATCH] [RFC] USB: hub.c: decrease the number of attempts of enumeration scheme
[not found] <CAEt1RjrQsb6=reKUKV9uJTG4JoJXErhJFj=2TdVx=N1_Ad1GVg@mail.gmail.com>
@ 2020-08-08 6:40 ` Yasushi Asano
2020-08-08 15:16 ` Alan Stern
0 siblings, 1 reply; 29+ messages in thread
From: Yasushi Asano @ 2020-08-08 6:40 UTC (permalink / raw)
To: stern, gregkh
Cc: linux-usb, erosca, andrew_gabbasov, jim_baxter, wnatsume,
nnishiguchi, yasano
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.
Signed-off-by: Yasushi Asano <yasano@jp.adit-jv.com>
---
drivers/usb/core/hub.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 052d5ac..ebf6931 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -2707,9 +2707,9 @@ 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))
+#define GET_DESCRIPTOR_TRIES 1
+#define SET_CONFIG_TRIES (use_both_schemes + 1)
+#define USE_NEW_SCHEME(i, scheme) ((i) == (int)(scheme))
#define HUB_ROOT_RESET_TIME 60 /* times are in msec */
#define HUB_SHORT_RESET_TIME 10
--
2.7.4
^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH] [RFC] USB: hub.c: decrease the number of attempts of enumeration scheme
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
0 siblings, 1 reply; 29+ messages in thread
From: Alan Stern @ 2020-08-08 15:16 UTC (permalink / raw)
To: Yasushi Asano
Cc: gregkh, linux-usb, erosca, andrew_gabbasov, jim_baxter, wnatsume,
nnishiguchi, yasano
On Sat, Aug 08, 2020 at 03:40:14PM +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.
This description should explain that there will be 3 attempts using the
new scheme and 2 attempts using the old scheme (is that right?), at
most.
>
> Signed-off-by: Yasushi Asano <yasano@jp.adit-jv.com>
> ---
> drivers/usb/core/hub.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 052d5ac..ebf6931 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -2707,9 +2707,9 @@ 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))
> +#define GET_DESCRIPTOR_TRIES 1
Instead of changing the number of loop iterations to 1, you should get
rid of the loop entirely.
> +#define SET_CONFIG_TRIES (use_both_schemes + 1)
> +#define USE_NEW_SCHEME(i, scheme) ((i) == (int)(scheme))
>
> #define HUB_ROOT_RESET_TIME 60 /* times are in msec */
> #define HUB_SHORT_RESET_TIME 10
> --
> 2.7.4
Alan Stern
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v2] [RFC] USB: hub.c: decrease the number of attempts of enumeration
2020-08-08 15:16 ` Alan Stern
@ 2020-08-10 0:19 ` Yasushi Asano
2020-08-10 0:19 ` [PATCH v2] [RFC] USB: hub.c: decrease the number of attempts of enumeration scheme Yasushi Asano
0 siblings, 1 reply; 29+ messages in thread
From: Yasushi Asano @ 2020-08-10 0:19 UTC (permalink / raw)
To: stern, gregkh
Cc: linux-usb, erosca, andrew_gabbasov, jim_baxter, wnatsume,
nnishiguchi, yasano
From: Yasushi Asano <yasano@jp.adit-jv.com>
Hello Alan,
Thank you for the feedback. I added the information in the commit.
I reconsidered the reduction place in the number of attempts.
Since I don't want to miss 2nd reset[1] or do not change the timing of power cycling the port[2],
I changed to reduce attempts from the nearest enclosing loop.
I'm sorry to trouble you, please confirm it again.
[1]
/*
* 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;
[2]
/* When halfway through our retry count, power-cycle the port */
if (i == (SET_CONFIG_TRIES / 2) - 1) {
Yasushi Asano (1):
[RFC] USB: hub.c: decrease the number of attempts of enumeration
scheme
drivers/usb/core/hub.c | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v2] [RFC] USB: hub.c: decrease the number of attempts of enumeration scheme
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 ` Yasushi Asano
2020-08-10 7:46 ` Greg KH
0 siblings, 1 reply; 29+ messages in thread
From: Yasushi Asano @ 2020-08-10 0:19 UTC (permalink / raw)
To: stern, gregkh
Cc: linux-usb, erosca, andrew_gabbasov, jim_baxter, wnatsume,
nnishiguchi, yasano
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
Signed-off-by: Yasushi Asano <yasano@jp.adit-jv.com>
---
drivers/usb/core/hub.c | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 052d5ac..5b15278c 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,
@@ -4714,7 +4713,10 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1,
retries == 0 &&
udev->speed > USB_SPEED_FULL))
break;
+ } else {
+ break;
}
+
udev->descriptor.bMaxPacketSize0 =
buf->bMaxPacketSize0;
kfree(buf);
@@ -4744,12 +4746,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",
--
2.7.4
^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH v2] [RFC] USB: hub.c: decrease the number of attempts of enumeration scheme
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-08-12 17:09 ` [PATCH v2] " Yasushi Asano
0 siblings, 2 replies; 29+ messages in thread
From: Greg KH @ 2020-08-10 7:46 UTC (permalink / raw)
To: Yasushi Asano
Cc: stern, linux-usb, erosca, andrew_gabbasov, jim_baxter, wnatsume,
nnishiguchi, yasano
On Mon, Aug 10, 2020 at 09:19:35AM +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
>
> Signed-off-by: Yasushi Asano <yasano@jp.adit-jv.com>
> ---
> drivers/usb/core/hub.c | 16 +++++++---------
> 1 file changed, 7 insertions(+), 9 deletions(-)
Why is there a "[RFC]" on the subject line, do you not think this works
properly? Does it work for your devices and solve the problem for you?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2] [RFC] USB: hub.c: decrease the number of attempts of enumeration scheme
2020-08-10 7:46 ` Greg KH
@ 2020-08-11 15:20 ` yasushi asano
2020-09-07 15:50 ` [PATCH v3] " Yasushi Asano
2020-08-12 17:09 ` [PATCH v2] " Yasushi Asano
1 sibling, 1 reply; 29+ messages in thread
From: yasushi asano @ 2020-08-11 15:20 UTC (permalink / raw)
To: Greg KH
Cc: Alan Stern, linux-usb, Rosca, Eugeniu (ADITG/ESM1),
andrew_gabbasov, Baxter Jim, Natsume, Wataru (ADITJ/SWG),
Nishiguchi, Naohiro (ADITJ/SWG), 浅野恭史
Hello Greg, I'm sorry for the inconvenience. I was not sure how to use
[RFC] well. If the patch works well, we already don't need to add RFC
in the subject line? This patch works fine on my target. If it is not
needed [PFC] ,I will send the patch again without RFC in the patch
title and subject line in email later.
Best regards
Yasushi Asano
2020年8月10日(月) 16:45 Greg KH <gregkh@linuxfoundation.org>:
>
> On Mon, Aug 10, 2020 at 09:19:35AM +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
> >
> > Signed-off-by: Yasushi Asano <yasano@jp.adit-jv.com>
> > ---
> > drivers/usb/core/hub.c | 16 +++++++---------
> > 1 file changed, 7 insertions(+), 9 deletions(-)
>
> Why is there a "[RFC]" on the subject line, do you not think this works
> properly? Does it work for your devices and solve the problem for you?
>
> thanks,
>
> greg k-h
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v2] USB: hub.c: decrease the number of attempts of enumeration scheme
2020-08-10 7:46 ` Greg KH
2020-08-11 15:20 ` yasushi asano
@ 2020-08-12 17:09 ` Yasushi Asano
1 sibling, 0 replies; 29+ messages in thread
From: Yasushi Asano @ 2020-08-12 17:09 UTC (permalink / raw)
To: stern, gregkh
Cc: linux-usb, erosca, andrew_gabbasov, jim_baxter, wnatsume,
nnishiguchi, yasano
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
Signed-off-by: Yasushi Asano <yasano@jp.adit-jv.com>
---
drivers/usb/core/hub.c | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 052d5ac..5b15278c 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,
@@ -4714,7 +4713,10 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1,
retries == 0 &&
udev->speed > USB_SPEED_FULL))
break;
+ } else {
+ break;
}
+
udev->descriptor.bMaxPacketSize0 =
buf->bMaxPacketSize0;
kfree(buf);
@@ -4744,12 +4746,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",
--
2.7.4
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v3] USB: hub.c: decrease the number of attempts of enumeration scheme
2020-08-11 15:20 ` yasushi asano
@ 2020-09-07 15:50 ` Yasushi Asano
2020-09-07 15:50 ` Yasushi Asano
0 siblings, 1 reply; 29+ messages in thread
From: Yasushi Asano @ 2020-09-07 15:50 UTC (permalink / raw)
To: stern, gregkh
Cc: linux-usb, erosca, andrew_gabbasov, jim_baxter, wnatsume,
nnishiguchi, yasano
From: Yasushi Asano <yasano@jp.adit-jv.com>
Dear Alan
Dear Greg
I added the log which was not applied this patch and the log which
was applied this patch to the commit statement. When this patch is
applied, enumeration failure is detected within 30 seconds and
the PET test is passed.
In addition, I also fixed the error output which is pointed out
from the kernel test robot.
I would appreciate it if you would advance this PR.
Thank you in advance.
Yasushi Asano (1):
USB: hub.c: decrease the number of attempts of enumeration scheme
drivers/usb/core/hub.c | 23 ++++-------------------
1 file changed, 4 insertions(+), 19 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v3] USB: hub.c: decrease the number of attempts of enumeration scheme
2020-09-07 15:50 ` [PATCH v3] " Yasushi Asano
@ 2020-09-07 15:50 ` Yasushi Asano
2020-09-08 19:04 ` Alan Stern
0 siblings, 1 reply; 29+ messages in thread
From: Yasushi Asano @ 2020-09-07 15:50 UTC (permalink / raw)
To: stern, gregkh
Cc: linux-usb, erosca, andrew_gabbasov, jim_baxter, wnatsume,
nnishiguchi, yasano, kernel test robot
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
Before applying the patch. please see [2]-[1].
The PETtest fails during device enumeration procedure[3] because the device
enumeration failure[4] can not be detected within 30 seconds of starting
the test.
[ 99.864751] *** Ready OK Test Start***
[ 102.806542] usb 1-1.1: new full-speed USB device number 7 using ehci-platform ...[1]
[ 118.608538] usb 1-1.1: device descriptor read/64, error -110
[ 134.480542] usb 1-1.1: device descriptor read/64, error -110
[ 134.669542] usb 1-1.1: new full-speed USB device number 8 using ehci-platform
[ 150.352537] usb 1-1.1: device descriptor read/64, error -110 ...[2]
[ 152.071247] *** End of Test *** ...[3]
[ 154.290867] usb 1-1-port1: unable to enumerate USB device ...[4]
After applying this patch. please see [6]-[5].
device enumeration failure[6] is detected within 30 seconds after
starting the PETtest. The test passed.
[ 134.632165] *** Ready OK Test Start***
[ 137.880705] usb 1-1.1: new full-speed USB device number 11 using ehci-platform ..[5]
[ 143.184698] usb 1-1.1: device descriptor read/64, error -110
[ 148.815699] usb 1-1.1: device descriptor read/64, error -110
[ 149.005691] usb 1-1.1: new full-speed USB device number 12 using ehci-platform
[ 154.448695] usb 1-1.1: device descriptor read/64, error -110
[ 154.561696] usb 1-1.1: Invalid ep0 maxpacket: 0
[ 154.567354] usb 1-1-port1: attempt power cycle
[ 156.445704] usb 1-1.1: new full-speed USB device number 13 using ehci-platform
[ 161.745697] usb 1-1.1: device not accepting address 13, error -110
[ 161.827694] usb 1-1.1: new full-speed USB device number 14 using ehci-platform
[ 167.377693] usb 1-1.1: device not accepting address 14, error -110
[ 167.395415] usb 1-1-port1: unable to enumerate USB device ..[6]
[ 176.678418] *** End of Test ***
------
[7] https://www.usb.org/sites/default/files/otgeh_compliance_plan_1_2.pdf
6.7.22 A-UUT "Device No Response" for connection timeout
6.7.22.2 Test Procedure for A-UUT which does not support sessions
1. Start with cable still attached, PET applying 10 micro F capacitance and
10K Ohm pull-down resistance between VBUS and ground, data lines not
pulled up.
2. Wait 1s.
3. PET connects using D+.
4. Display Message "Click OK when 'Device No Response' indication
displayed on UUT".
5. If operator clicks OK before 30s elapses since VBUS went on, then
UUT passes test.
6. If 30s elapses first, then UUT fails test.
7. PET leaves 10 micro F capacitance and 10K Ohm pull-down resistance
across VBUS.
8. PET disconnects D+.
9. Wait 2s. to allow disconnection to be recognized.
Reported-by: kernel test robot <rong.a.chen@intel.com>
Signed-off-by: Yasushi Asano <yasano@jp.adit-jv.com>
---
drivers/usb/core/hub.c | 23 ++++-------------------
1 file changed, 4 insertions(+), 19 deletions(-)
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",
--
2.7.4
^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH v3] USB: hub.c: decrease the number of attempts of enumeration scheme
2020-09-07 15:50 ` Yasushi Asano
@ 2020-09-08 19:04 ` Alan Stern
2020-09-10 4:49 ` yasushi asano
0 siblings, 1 reply; 29+ messages in thread
From: Alan Stern @ 2020-09-08 19:04 UTC (permalink / raw)
To: Yasushi Asano
Cc: gregkh, linux-usb, erosca, andrew_gabbasov, jim_baxter, wnatsume,
nnishiguchi, yasano, kernel test robot
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. */
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3] USB: hub.c: decrease the number of attempts of enumeration scheme
2020-09-08 19:04 ` Alan Stern
@ 2020-09-10 4:49 ` yasushi asano
2020-09-11 8:33 ` yasushi asano
0 siblings, 1 reply; 29+ messages in thread
From: yasushi asano @ 2020-09-10 4:49 UTC (permalink / raw)
To: Alan Stern
Cc: Greg KH, linux-usb, Rosca, Eugeniu (ADITG/ESM1),
andrew_gabbasov, Baxter Jim, Natsume, Wataru (ADITJ/SWG),
Nishiguchi, Naohiro (ADITJ/SWG), 浅野恭史,
kernel test robot
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. */
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3] USB: hub.c: decrease the number of attempts of enumeration scheme
2020-09-10 4:49 ` yasushi asano
@ 2020-09-11 8:33 ` yasushi asano
2020-09-11 15:12 ` Alan Stern
0 siblings, 1 reply; 29+ messages in thread
From: yasushi asano @ 2020-09-11 8:33 UTC (permalink / raw)
To: Alan Stern
Cc: Greg KH, linux-usb, Rosca, Eugeniu (ADITG/ESM1),
andrew_gabbasov, Baxter Jim, Natsume, Wataru (ADITJ/SWG),
Nishiguchi, Naohiro (ADITJ/SWG), 浅野恭史,
kernel test robot
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. */
> >
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3] USB: hub.c: decrease the number of attempts of enumeration scheme
2020-09-11 8:33 ` yasushi asano
@ 2020-09-11 15:12 ` Alan Stern
2020-09-15 9:45 ` Eugeniu Rosca
0 siblings, 1 reply; 29+ messages in thread
From: Alan Stern @ 2020-09-11 15:12 UTC (permalink / raw)
To: yasushi asano
Cc: Greg KH, linux-usb, Rosca, Eugeniu (ADITG/ESM1),
andrew_gabbasov, Baxter Jim, Natsume, Wataru (ADITJ/SWG),
Nishiguchi, Naohiro (ADITJ/SWG), 浅野恭史,
kernel test robot
On Fri, Sep 11, 2020 at 05:33:18PM +0900, yasushi asano wrote:
> 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.
> [ 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 ***
The problem is that I forgot to remove the Set-Address loop. Here's a
revised patch for you to test.
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
Index: usb-devel/drivers/usb/core/hub.c
===================================================================
--- usb-devel.orig/drivers/usb/core/hub.c
+++ usb-devel/drivers/usb/core/hub.c
@@ -2706,10 +2706,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 +2714,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 +4544,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 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 +4663,101 @@ 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) {
+ retval = hub_set_address(udev, devnum);
+ 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 +4781,6 @@ hub_port_init(struct usb_hub *hub, struc
retval);
retval = 0;
}
- break;
}
}
if (retval)
@@ -5106,7 +5082,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 +5215,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 +5746,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. */
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3] USB: hub.c: decrease the number of attempts of enumeration scheme
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:48 ` [PATCH v3] USB: hub.c: decrease the number of attempts of enumeration scheme Alan Stern
0 siblings, 2 replies; 29+ messages in thread
From: Eugeniu Rosca @ 2020-09-15 9:45 UTC (permalink / raw)
To: Alan Stern, Greg KH
Cc: linux-usb, linux-kernel, andrew_gabbasov, Baxter Jim, Natsume,
Wataru (ADITJ/SWG), Nishiguchi, Naohiro (ADITJ/SWG),
浅野恭史,
kernel test robot, yasushi asano, Martin Mueller, Eugeniu Rosca,
Eugeniu Rosca
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3] USB: hub.c: decrease the number of attempts of enumeration scheme
2020-09-15 9:45 ` Eugeniu Rosca
@ 2020-09-15 11:01 ` Greg KH
2020-09-15 14:52 ` Alan Stern
2020-09-15 14:48 ` [PATCH v3] USB: hub.c: decrease the number of attempts of enumeration scheme Alan Stern
1 sibling, 1 reply; 29+ messages in thread
From: Greg KH @ 2020-09-15 11:01 UTC (permalink / raw)
To: Eugeniu Rosca
Cc: Alan Stern, linux-usb, linux-kernel, andrew_gabbasov, Baxter Jim,
Natsume, Wataru (ADITJ/SWG), Nishiguchi, Naohiro (ADITJ/SWG),
浅野恭史,
kernel test robot, yasushi asano, Martin Mueller, 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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3] USB: hub.c: decrease the number of attempts of enumeration scheme
2020-09-15 9:45 ` Eugeniu Rosca
2020-09-15 11:01 ` Greg KH
@ 2020-09-15 14:48 ` Alan Stern
1 sibling, 0 replies; 29+ messages in thread
From: Alan Stern @ 2020-09-15 14:48 UTC (permalink / raw)
To: Eugeniu Rosca
Cc: Greg KH, linux-usb, linux-kernel, andrew_gabbasov, Baxter Jim,
Natsume, Wataru (ADITJ/SWG), Nishiguchi, Naohiro (ADITJ/SWG),
浅野恭史,
kernel test robot, yasushi asano, Martin Mueller, 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 [*]?
>
> 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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3] USB: hub.c: decrease the number of attempts of enumeration scheme
2020-09-15 11:01 ` Greg KH
@ 2020-09-15 14:52 ` Alan Stern
2020-09-16 10:16 ` yasushi asano
0 siblings, 1 reply; 29+ messages in thread
From: Alan Stern @ 2020-09-15 14:52 UTC (permalink / raw)
To: Greg KH
Cc: Eugeniu Rosca, linux-usb, linux-kernel, andrew_gabbasov,
Baxter Jim, Natsume, Wataru (ADITJ/SWG),
Nishiguchi, Naohiro (ADITJ/SWG), 浅野恭史,
kernel test robot, yasushi asano, Martin Mueller, Eugeniu Rosca
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3] USB: hub.c: decrease the number of attempts of enumeration scheme
2020-09-15 14:52 ` Alan Stern
@ 2020-09-16 10:16 ` yasushi asano
2020-09-18 15:00 ` yasushi asano
0 siblings, 1 reply; 29+ messages in thread
From: yasushi asano @ 2020-09-16 10:16 UTC (permalink / raw)
To: Alan Stern
Cc: Greg KH, Eugeniu Rosca, linux-usb, linux-kernel, andrew_gabbasov,
Baxter Jim, Natsume, Wataru (ADITJ/SWG),
Nishiguchi, Naohiro (ADITJ/SWG), 浅野恭史,
kernel test robot, Martin Mueller, Eugeniu Rosca
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3] USB: hub.c: decrease the number of attempts of enumeration scheme
2020-09-16 10:16 ` yasushi asano
@ 2020-09-18 15:00 ` yasushi asano
2020-09-20 19:21 ` Alan Stern
0 siblings, 1 reply; 29+ messages in thread
From: yasushi asano @ 2020-09-18 15:00 UTC (permalink / raw)
To: Alan Stern
Cc: Greg KH, Eugeniu Rosca, linux-usb, linux-kernel, andrew_gabbasov,
Baxter Jim, Natsume, Wataru (ADITJ/SWG),
Nishiguchi, Naohiro (ADITJ/SWG), 浅野恭史,
kernel test robot, Martin Mueller, Eugeniu Rosca
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3] USB: hub.c: decrease the number of attempts of enumeration scheme
2020-09-18 15:00 ` yasushi asano
@ 2020-09-20 19:21 ` Alan Stern
2020-09-21 14:03 ` [PATCH] " Yasushi Asano
0 siblings, 1 reply; 29+ messages in thread
From: Alan Stern @ 2020-09-20 19:21 UTC (permalink / raw)
To: yasushi asano
Cc: Greg KH, Eugeniu Rosca, linux-usb, linux-kernel, andrew_gabbasov,
Baxter Jim, Natsume, Wataru (ADITJ/SWG),
Nishiguchi, Naohiro (ADITJ/SWG), 浅野恭史,
kernel test robot, Martin Mueller, Eugeniu Rosca
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH] Re: [PATCH v3] USB: hub.c: decrease the number of attempts of enumeration scheme
2020-09-20 19:21 ` Alan Stern
@ 2020-09-21 14:03 ` Yasushi Asano
2020-09-21 14:48 ` Alan Stern
0 siblings, 1 reply; 29+ messages in thread
From: Yasushi Asano @ 2020-09-21 14:03 UTC (permalink / raw)
To: stern, gregkh
Cc: linux-usb, erosca, andrew_gabbasov, jim_baxter, wnatsume,
nnishiguchi, yasano
From: Yasushi Asano <yasano@jp.adit-jv.com>
Dear Alan
Thank you very much for providing the patch.
I really appreciate your kindness.
When I added a pseudo error code and checked it,
the for statement of the "operation" in the new scheme
runs unconditionally three times. Therefore It doesn't
seem to meet the requirements (30seconds).
After applying your patch, I added a patch that can
change the loop number of "operation" as shown below,
and it worked fine in the pseudo error environment.
Is this modification acceptable?
If it is good, I'll add this patch and test it.
Since the PET tool is only in the customer,
I will ask the customer to test it. I will report the
result when I receive the results.
Signed-off-by: Yasushi Asano <yasano@jp.adit-jv.com>
---
drivers/usb/core/hub.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 61effd5..2f07f7c 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -2709,12 +2709,14 @@ static unsigned hub_is_wusb(struct usb_hub *hub)
#define PORT_RESET_TRIES 2
#define SET_ADDRESS_TRIES 1
#define GET_DESCRIPTOR_TRIES 1
+#define GET_DESCRIPTOR_OPERATIONS 1
#define PORT_INIT_TRIES 5
#else
#define PORT_RESET_TRIES 5
#define SET_ADDRESS_TRIES 2
#define GET_DESCRIPTOR_TRIES 2
+#define GET_DESCRIPTOR_OPERATIONS 3
#define PORT_INIT_TRIES 4
#endif /* CONFIG_USB_FEW_INIT_RETRIES */
@@ -4699,7 +4701,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) {
+ for (operations = 0; operations < GET_DESCRIPTOR_OPERATIONS; ++operations) {
buf->bMaxPacketSize0 = 0;
r = usb_control_msg(udev, usb_rcvaddr0pipe(),
USB_REQ_GET_DESCRIPTOR, USB_DIR_IN,
--
2.7.4
^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH] Re: [PATCH v3] USB: hub.c: decrease the number of attempts of enumeration scheme
2020-09-21 14:03 ` [PATCH] " Yasushi Asano
@ 2020-09-21 14:48 ` Alan Stern
[not found] ` <CAEt1Rjq-DOwN0+_7F0m-kqUHTzm5YPUaXqUOpTszCsqrfLRt5w@mail.gmail.com>
0 siblings, 1 reply; 29+ messages in thread
From: Alan Stern @ 2020-09-21 14:48 UTC (permalink / raw)
To: Yasushi Asano
Cc: gregkh, linux-usb, erosca, andrew_gabbasov, jim_baxter, wnatsume,
nnishiguchi, yasano
On Mon, Sep 21, 2020 at 11:03:42PM +0900, Yasushi Asano wrote:
> From: Yasushi Asano <yasano@jp.adit-jv.com>
>
> Dear Alan
>
> Thank you very much for providing the patch.
> I really appreciate your kindness.
>
> When I added a pseudo error code and checked it,
> the for statement of the "operation" in the new scheme
> runs unconditionally three times. Therefore It doesn't
> seem to meet the requirements (30seconds).
>
> After applying your patch, I added a patch that can
> change the loop number of "operation" as shown below,
> and it worked fine in the pseudo error environment.
> Is this modification acceptable?
Ah, I missed that change. Yes, this is the right approach.
> If it is good, I'll add this patch and test it.
> Since the PET tool is only in the customer,
> I will ask the customer to test it. I will report the
> result when I receive the results.
>
> Signed-off-by: Yasushi Asano <yasano@jp.adit-jv.com>
> ---
> drivers/usb/core/hub.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 61effd5..2f07f7c 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -2709,12 +2709,14 @@ static unsigned hub_is_wusb(struct usb_hub *hub)
> #define PORT_RESET_TRIES 2
> #define SET_ADDRESS_TRIES 1
> #define GET_DESCRIPTOR_TRIES 1
> +#define GET_DESCRIPTOR_OPERATIONS 1
> #define PORT_INIT_TRIES 5
>
> #else
> #define PORT_RESET_TRIES 5
> #define SET_ADDRESS_TRIES 2
> #define GET_DESCRIPTOR_TRIES 2
> +#define GET_DESCRIPTOR_OPERATIONS 3
> #define PORT_INIT_TRIES 4
> #endif /* CONFIG_USB_FEW_INIT_RETRIES */
>
> @@ -4699,7 +4701,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) {
> + for (operations = 0; operations < GET_DESCRIPTOR_OPERATIONS; ++operations) {
> buf->bMaxPacketSize0 = 0;
> r = usb_control_msg(udev, usb_rcvaddr0pipe(),
> USB_REQ_GET_DESCRIPTOR, USB_DIR_IN,
Okay. I will merge your change into my patch.
Alan Stern
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] Re: [PATCH v3] USB: hub.c: decrease the number of attempts of enumeration scheme
[not found] ` <CAEt1Rjq-DOwN0+_7F0m-kqUHTzm5YPUaXqUOpTszCsqrfLRt5w@mail.gmail.com>
@ 2020-09-21 15:06 ` Alan Stern
2020-09-25 1:05 ` yasushi asano
0 siblings, 1 reply; 29+ messages in thread
From: Alan Stern @ 2020-09-21 15:06 UTC (permalink / raw)
To: yasushi asano
Cc: andrew_gabbasov, erosca, gregkh, jim_baxter, linux-usb,
nnishiguchi, wnatsume, yasano
On Mon, Sep 21, 2020 at 11:59:31PM +0900, yasushi asano wrote:
> Dear Alan
>
> Thank you very much for the reply.
> please merge my modification to your patch.
Yes.
I will wait to hear the result of your test before I submit the changes.
Alan Stern
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] Re: [PATCH v3] USB: hub.c: decrease the number of attempts of enumeration scheme
2020-09-21 15:06 ` Alan Stern
@ 2020-09-25 1:05 ` yasushi asano
2020-09-25 17:21 ` yasushi asano
0 siblings, 1 reply; 29+ messages in thread
From: yasushi asano @ 2020-09-25 1:05 UTC (permalink / raw)
To: Alan Stern
Cc: andrew_gabbasov, Rosca, Eugeniu (ADITG/ESM1),
Greg KH, Baxter Jim, linux-usb, Nishiguchi, Naohiro (ADITJ/SWG),
Natsume, Wataru (ADITJ/SWG), 浅野恭史
Dear Alan,
I am waiting for the test result from the customer since Tuesday, but
I have not received a reply yet. I will inform you as soon as I
receive the result.
Best regards
Yasushi Asano
2020年9月22日(火) 0:06 Alan Stern <stern@rowland.harvard.edu>:
>
> On Mon, Sep 21, 2020 at 11:59:31PM +0900, yasushi asano wrote:
> > Dear Alan
> >
> > Thank you very much for the reply.
> > please merge my modification to your patch.
>
> Yes.
>
> I will wait to hear the result of your test before I submit the changes.
>
> Alan Stern
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] Re: [PATCH v3] USB: hub.c: decrease the number of attempts of enumeration scheme
2020-09-25 1:05 ` yasushi asano
@ 2020-09-25 17:21 ` yasushi asano
2020-09-25 18:41 ` Alan Stern
0 siblings, 1 reply; 29+ messages in thread
From: yasushi asano @ 2020-09-25 17:21 UTC (permalink / raw)
To: Alan Stern
Cc: andrew_gabbasov, Rosca, Eugeniu (ADITG/ESM1),
Greg KH, Baxter Jim, linux-usb, Nishiguchi, Naohiro (ADITJ/SWG),
Natsume, Wataru (ADITJ/SWG), 浅野恭史
Dear Alan,
I received the test result of applying your patch and
my change from customer.
It was tested using PETtool. it barely passed the test.
It took 29.497 seconds. The patch worked well.
The following is an excerpt from dmesg.
Could you please merge my change and proceed with this PR?
I really appreciate your kindness.
[2] - [1] = 869.860348 - 840.363348 = 29.497
[ 829.307248] *** Setting Test Suite ***
[ 838.173580] *** Ready OK Test Start***
[ 840.363348] usb 1-1.1: new full-speed USB device number 7 using
ehci-platform ... [1]
[ 845.650354] usb 1-1.1: device descriptor read/64, error -110
[ 845.841346] usb 1-1.1: new full-speed USB device number 8 using ehci-platform
[ 851.283362] usb 1-1.1: device descriptor read/64, error -110
[ 851.397049] usb 1-1-port1: attempt power cycle
[ 853.216340] usb 1-1.1: new full-speed USB device number 9 using ehci-platform
[ 858.451345] usb 1-1.1: device descriptor read/64, error -110
[ 858.640352] usb 1-1.1: new full-speed USB device number 10 using
ehci-platform
[ 864.212349] usb 1-1.1: device not accepting address 10, error -110
[ 864.295350] usb 1-1.1: new full-speed USB device number 11 using
ehci-platform
[ 869.844342] usb 1-1.1: device not accepting address 11, error -110
[ 869.860348] usb 1-1-port1: unable to enumerate USB device ... [2]
[ 885.407051] *** End of Test ***
Best Regards
Yasushi Asano
2020年9月25日(金) 10:05 yasushi asano <yazzep@gmail.com>:
>
> Dear Alan,
> I am waiting for the test result from the customer since Tuesday, but
> I have not received a reply yet. I will inform you as soon as I
> receive the result.
>
> Best regards
> Yasushi Asano
>
> 2020年9月22日(火) 0:06 Alan Stern <stern@rowland.harvard.edu>:
> >
> > On Mon, Sep 21, 2020 at 11:59:31PM +0900, yasushi asano wrote:
> > > Dear Alan
> > >
> > > Thank you very much for the reply.
> > > please merge my modification to your patch.
> >
> > Yes.
> >
> > I will wait to hear the result of your test before I submit the changes.
> >
> > Alan Stern
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] Re: [PATCH v3] USB: hub.c: decrease the number of attempts of enumeration scheme
2020-09-25 17:21 ` yasushi asano
@ 2020-09-25 18:41 ` Alan Stern
2020-09-27 15:43 ` yasushi asano
0 siblings, 1 reply; 29+ messages in thread
From: Alan Stern @ 2020-09-25 18:41 UTC (permalink / raw)
To: yasushi asano
Cc: andrew_gabbasov, Rosca, Eugeniu (ADITG/ESM1),
Greg KH, Baxter Jim, linux-usb, Nishiguchi, Naohiro (ADITJ/SWG),
Natsume, Wataru (ADITJ/SWG), 浅野恭史
On Sat, Sep 26, 2020 at 02:21:50AM +0900, yasushi asano wrote:
> Dear Alan,
> I received the test result of applying your patch and
> my change from customer.
> It was tested using PETtool. it barely passed the test.
> It took 29.497 seconds. The patch worked well.
That's awfully close to the limit. I think PORT_INIT_TRIES should be
reduced to 4.
> The following is an excerpt from dmesg.
> Could you please merge my change and proceed with this PR?
> I really appreciate your kindness.
I'll go ahead and submit the patches.
Alan Stern
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] Re: [PATCH v3] USB: hub.c: decrease the number of attempts of enumeration scheme
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
0 siblings, 1 reply; 29+ messages in thread
From: yasushi asano @ 2020-09-27 15:43 UTC (permalink / raw)
To: Alan Stern
Cc: andrew_gabbasov, Rosca, Eugeniu (ADITG/ESM1),
Greg KH, Baxter Jim, linux-usb, Nishiguchi, Naohiro (ADITJ/SWG),
Natsume, Wataru (ADITJ/SWG), 浅野恭史
Dear Alan,
>That's awfully close to the limit. I think PORT_INIT_TRIES should be
>reduced to 4.
I understood. I agree with you.
>I'll go ahead and submit the patches.
Thank you very much.
Best regards
Yasushi Asano
2020年9月26日(土) 3:41 Alan Stern <stern@rowland.harvard.edu>:
>
> On Sat, Sep 26, 2020 at 02:21:50AM +0900, yasushi asano wrote:
> > Dear Alan,
> > I received the test result of applying your patch and
> > my change from customer.
> > It was tested using PETtool. it barely passed the test.
> > It took 29.497 seconds. The patch worked well.
>
> That's awfully close to the limit. I think PORT_INIT_TRIES should be
> reduced to 4.
>
> > The following is an excerpt from dmesg.
> > Could you please merge my change and proceed with this PR?
> > I really appreciate your kindness.
>
> I'll go ahead and submit the patches.
>
> Alan Stern
^ permalink raw reply [flat|nested] 29+ messages in thread
* [Patch 1/2]: USB: hub: Clean up use of port initialization schemes and retries
2020-09-27 15:43 ` yasushi asano
@ 2020-09-28 15:20 ` Alan Stern
2020-09-28 15:22 ` [Patch 2/2]: USB: hub: Add Kconfig option to reduce number of port initialization retries Alan Stern
0 siblings, 1 reply; 29+ messages in thread
From: Alan Stern @ 2020-09-28 15:20 UTC (permalink / raw)
To: Greg KH
Cc: yasushi asano, andrew_gabbasov, Rosca, Eugeniu (ADITG/ESM1),
Baxter Jim, linux-usb, Nishiguchi, Naohiro (ADITJ/SWG),
Natsume, Wataru (ADITJ/SWG), 浅野恭史
The SET_CONFIG_TRIES macro in hub.c is badly named; it controls the
number of port-initialization retry attempts rather than the number of
Set-Configuration attempts. Furthermore, the USE_NEW_SCHEME macro and
use_new_scheme() function are written in a very confusing manner,
making it almost impossible to figure out exactly what they do or
check that they are correct.
This patch renames SET_CONFIG_TRIES to PORT_INIT_TRIES, removes
USE_NEW_SCHEME entirely, and rewrites use_new_scheme() to be much more
transparent, with added comments explaining how it works. The patch
also pulls the single call site of use_new_scheme() out from the
Get-Descriptor retry loop (where it returns the same value each time)
and renames the local variable used to store the result.
The overall effect is a minor cleanup. However, there is one
functional change: If the "use_both_schemes" module parameter isn't
set (by default it is set), the existing code does only two retry
iterations. After this patch it will always perform four, regardless
of the parameter's value.
Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
---
[as1944]
drivers/usb/core/hub.c | 49 ++++++++++++++++++++++++++-----------------------
1 file changed, 26 insertions(+), 23 deletions(-)
Index: usb-devel/drivers/usb/core/hub.c
===================================================================
--- usb-devel.orig/drivers/usb/core/hub.c
+++ usb-devel/drivers/usb/core/hub.c
@@ -2708,8 +2708,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 4
#define HUB_ROOT_RESET_TIME 60 /* times are in msec */
#define HUB_SHORT_RESET_TIME 10
@@ -2717,23 +2716,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 +4552,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 +4665,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 +4780,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 +5109,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 +5242,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 - 1) / 2) {
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 +5773,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. */
^ permalink raw reply [flat|nested] 29+ messages in thread
* [Patch 2/2]: USB: hub: Add Kconfig option to reduce number of port initialization retries
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 ` Alan Stern
0 siblings, 0 replies; 29+ messages in thread
From: Alan Stern @ 2020-09-28 15:22 UTC (permalink / raw)
To: Greg KH
Cc: yasushi asano, andrew_gabbasov, Rosca, Eugeniu (ADITG/ESM1),
Baxter Jim, linux-usb, Nishiguchi, Naohiro (ADITJ/SWG),
Natsume, Wataru (ADITJ/SWG), 浅野恭史
Description based on one by Yasushi Asano:
According to 6.7.22 A-UUT “Device No Response” for connection timeout
of USB OTG and EH automated compliance plan v1.2, enumeration failure
has to be detected within 30 seconds. However, the old and new
enumeration schemes each make a total of 12 attempts, and each attempt
can take 5 seconds to time out, so the PET test fails.
This patch adds a new Kconfig option (CONFIG_USB_FEW_INIT_RETRIES);
when the option is set all the initialization retry loops except the
outermost are reduced to a single iteration. This reduces the total
number of attempts to four, allowing Linux hosts to pass the PET test.
The new option is disabled by default to preserve the existing
behavior. The reduced number of retries may fail to initialize a few
devices that currently do work, but for the most part there should be
no change. And in cases where the initialization does fail, it will
fail much more quickly.
Reported-and-tested-by: yasushi asano <yazzep@gmail.com>
Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
---
[as1945]
drivers/usb/core/Kconfig | 14 ++++++++++++++
drivers/usb/core/hub.c | 13 ++++++++++++-
2 files changed, 26 insertions(+), 1 deletion(-)
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
Index: usb-devel/drivers/usb/core/hub.c
===================================================================
--- usb-devel.orig/drivers/usb/core/hub.c
+++ usb-devel/drivers/usb/core/hub.c
@@ -2705,10 +2705,20 @@ 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 GET_MAXPACKET0_TRIES 1
+#define PORT_INIT_TRIES 4
+
+#else
#define PORT_RESET_TRIES 5
#define SET_ADDRESS_TRIES 2
#define GET_DESCRIPTOR_TRIES 2
+#define GET_MAXPACKET0_TRIES 3
#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
@@ -4691,7 +4701,8 @@ hub_port_init(struct usb_hub *hub, struc
* 255 is for WUSB devices, we actually need to use
* 512 (WUSB1.0[4.8.1]).
*/
- for (operations = 0; operations < 3; ++operations) {
+ for (operations = 0; operations < GET_MAXPACKET0_TRIES;
+ ++operations) {
buf->bMaxPacketSize0 = 0;
r = usb_control_msg(udev, usb_rcvaddr0pipe(),
USB_REQ_GET_DESCRIPTOR, USB_DIR_IN,
^ permalink raw reply [flat|nested] 29+ messages in thread
end of thread, other threads:[~2020-09-28 15:22 UTC | newest]
Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[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
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
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.