[PATCH] [RFC] USB: hub.c: Add the retry count module parameter for usbcore

[PATCH] [RFC] USB: hub.c: Add the retry count module parameter for usbcore

[Yasushi Asano]

From: Yasushi Asano <yasano@jp.adit-jv.com>

Dear Alan
Dear Greg

I would like to have a consultation with you. The customer's product
board needs to get a USB certification and compliance, but it can not
pass the test using the current USB hub driver. According to the USB
compliance plan[1], the“6.7.22 Device No Response” test requires the
detection of device enumeration failure within 30 seconds. The
kernel(USB hub driver) must notify userspace of the enumeration failure
within 30 seconds.

In the current hub driver, the place to detect device enumeration
failure is [2]. I have modified the hub driver to issue a uevent here,
but the procedure of device enumeration (new_scheme+old_scheme) takes
too long to execute, it couldn't reach [2] within 30 seconds after
starting the test.  According to the result of PETtool [3], the state of
"Device No response" is that usb_control_msg(USB_REQ_GET_DESCRIPTOR) [4]
in hub_port_initn times out. That means r == -ETIMEDOUT.  because of the
default setting of initial_descriptor_timeout is 5000 msec[5],
usb_control_msg() took 5000msec until -ETIMEDOUT.

I will try to describe the flow of device enumeration processing
below[6].  If my understanding is correct, the enumeration failure [2]
will be reached after performing all the loops of [7][8][9]+[7][10].  In
the new scheme, 12 times check will be performed ([7]/2*[8]*[9] => 2*2*3
=> 12).  In the old scheme , 4 times check will be performed ([7]/2*[10]
=> 2*2 => 4).  In total, it checks 16 times, and it took 5000msec to
ETIMEDOUT every time. Therefore, It takes about 80 seconds(16*5000msec)
to reach [2]. This does not pass the "Device No response" test.

If I set the module parameter old_schene_first=Y and use_both_schemes=N,
it can be reached [2] within 30 seconds. However, the new_scheme will no
longer be performed. I think we can't choose this, as
previously-detected devices may become undetectable.  new_scheme is
taking too long and I think 12 times checks are redundant. So, I
confirmed the USB specification.

This is the only description of the read of the device descriptor[12].
I couldn't find the description related retry count or timing here and
anywhere in this specification.  And I couldn't find the description
related timing in the “No silent failures" requirements in other
documents[13] also.

Because I'm not sure where this number of loop count is decided, I'm not
sure how should it be modified the driver to pass the USB compliance
test. Is it possible for me to receive a proposal for a solution?  As my
thought, the number of loops may be redundant, so I think if the user
can change it arbitrarily, the problem will be solved. Currently, the
timeout value can be adjusted with a module parameter, but the loop
count cannot. Is there any problem if it is changed like this patch?
The original handling of the driver is unchanged by this modification. I
think the user will be able to adjust the time to enumeration failure
freely. Is this patch acceptable? I would appreciate it much if I could
receive the feedback from you.

----------------------------------------------------------------------------------------------------------------------------------------
[1] 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
   - 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.
----------------------------------------------------------------------------------------------------------------------------------------
[2] hub_port_connect()

        if (hub->hdev->parent ||
                        !hcd->driver->port_handed_over ||
                        !(hcd->driver->port_handed_over)(hcd, port1)) {
                if (status != -ENOTCONN && status != -ENODEV)
                        dev_err(&port_dev->dev,
                                        "unable to enumerate USB device\n");
        }
----------------------------------------------------------------------------------------------------------------------------------------
[3] http://www.mqp.com/usbcomp.htm
----------------------------------------------------------------------------------------------------------------------------------------
[4] hub_port_init()

  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);
----------------------------------------------------------------------------------------------------------------------------------------
[5]
static int initial_descriptor_timeout = USB_CTRL_GET_TIMEOUT;
include/linux/usb.h:#define USB_CTRL_GET_TIMEOUT        5000
----------------------------------------------------------------------------------------------------------------------------------------
[6] The flow of device enumeration processing
drivers/usb/core/hub.c

hub_port_connect(){
       for (x = 0; x < SET_CONFIG_TRIES; x++) {                  ...[7] SET_CONFIG_TRIES is 4 as default setting
               hub_port_init(){
                       if( x < 2 ) {
                             ------ new scheme ------
                             for ( y= 0; y < 2; ++y ) {          ...[8] 2==GET_DESCRIPTOR_TRIES
                                    for ( z = 0; z < 3; ++z ) {  ...[9]
                                           usb_control_msg()     ...[3] ETIMEDOUT(-110) is detected. Timieout value=5000msec.
                                    }
                             }
                       }
                       else {
                             ------ old scheme ------
                             for ( y= 0; y < 2; ++y ) {          ...[10] 2==SET_ADDRESS_TRIES
                                    hub_set_address()            ...[11] ETIMEDOUT(-110) is detected. Timieout value=5000msec.
                             }
                       }
               }
       }
       unable to enumerate USB device.                           ...[2]
}
----------------------------------------------------------------------------------------------------------------------------------------
[12] https://www.usb.org/document-library/usb-20-specification
Universal Serial Bus Specification (usb_20.pdf)
9.1.2 Bus Enumeration
6. Before the USB device receives a unique address, its Default Control Pipe is still accessible via the default address.
   The host reads the device descriptor to determine what actual maximum data payload size this USB device's default pipe can use.
----------------------------------------------------------------------------------------------------------------------------------------
[13] https://www.usb.org/document-library/usb-20-specification
On-The-Go and Embedded Host Supplement to the USB Revision 2.0 Specification (USB_OTG_and_EH_2-0-version 1_1a.pdf)
3.5 No Silent Failures
----------------------------------------------------------------------------------------------------------------------------------------

Signed-off-by: Yasushi Asano <yasano@jp.adit-jv.com>
---
 drivers/usb/core/hub.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 052d5ac..01c2b2d 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -99,6 +99,18 @@ MODULE_PARM_DESC(use_both_schemes,
 		"try the other device initialization scheme if the "
 		"first one fails");
 
+static int get_descriptor_tries = 2;
+module_param(get_descriptor_tries, int, S_IRUGO | S_IWUSR);
+MODULE_PARM_DESC(get_descriptor_tries,
+		"The number of a 64-byte GET_DESCRIPTOR request tries "
+		"(default 2)");
+
+static int get_descriptor_operations = 3;
+module_param(get_descriptor_operations, int, S_IRUGO | S_IWUSR);
+MODULE_PARM_DESC(get_descriptor_operations,
+		"The number of a 64-byte GET_DESCRIPTOR operation "
+		"(default 3)");
+
 /* Mutual exclusion for EHCI CF initialization.  This interferes with
  * port reset on some companion controllers.
  */
@@ -2707,7 +2719,8 @@ 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 GET_DESCRIPTOR_TRIES	get_descriptor_tries
+#define GET_DESCRIPTOR_OPERATIONS	get_descriptor_operations
 #define SET_CONFIG_TRIES	(2 * (use_both_schemes + 1))
 #define USE_NEW_SCHEME(i, scheme)	((i) / 2 == (int)(scheme))
 
@@ -4684,7 +4697,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

Re: [PATCH] [RFC] USB: hub.c: Add the retry count module parameter for usbcore

[Greg KH]

On Thu, Jul 30, 2020 at 07:42:26PM +0900, Yasushi Asano wrote:
> From: Yasushi Asano <yasano@jp.adit-jv.com>
> 
> Dear Alan
> Dear Greg
> 
> I would like to have a consultation with you. The customer's product
> board needs to get a USB certification and compliance, but it can not
> pass the test using the current USB hub driver. According to the USB
> compliance plan[1], the“6.7.22 Device No Response” test requires the
> detection of device enumeration failure within 30 seconds. The
> kernel(USB hub driver) must notify userspace of the enumeration failure
> within 30 seconds.

Odd, we have passed the USB certification tests in the past, what
changed recently to cause this?

> In the current hub driver, the place to detect device enumeration
> failure is [2]. I have modified the hub driver to issue a uevent here,
> but the procedure of device enumeration (new_scheme+old_scheme) takes
> too long to execute, it couldn't reach [2] within 30 seconds after
> starting the test.  According to the result of PETtool [3], the state of
> "Device No response" is that usb_control_msg(USB_REQ_GET_DESCRIPTOR) [4]
> in hub_port_initn times out. That means r == -ETIMEDOUT.  because of the
> default setting of initial_descriptor_timeout is 5000 msec[5],
> usb_control_msg() took 5000msec until -ETIMEDOUT.
> 
> I will try to describe the flow of device enumeration processing
> below[6].  If my understanding is correct, the enumeration failure [2]
> will be reached after performing all the loops of [7][8][9]+[7][10].  In
> the new scheme, 12 times check will be performed ([7]/2*[8]*[9] => 2*2*3
> => 12).  In the old scheme , 4 times check will be performed ([7]/2*[10]
> => 2*2 => 4).  In total, it checks 16 times, and it took 5000msec to
> ETIMEDOUT every time. Therefore, It takes about 80 seconds(16*5000msec)
> to reach [2]. This does not pass the "Device No response" test.
> 
> If I set the module parameter old_schene_first=Y and use_both_schemes=N,
> it can be reached [2] within 30 seconds. However, the new_scheme will no
> longer be performed. I think we can't choose this, as
> previously-detected devices may become undetectable.  new_scheme is
> taking too long and I think 12 times checks are redundant. So, I
> confirmed the USB specification.
> 
> This is the only description of the read of the device descriptor[12].
> I couldn't find the description related retry count or timing here and
> anywhere in this specification.  And I couldn't find the description
> related timing in the “No silent failures" requirements in other
> documents[13] also.
> 
> Because I'm not sure where this number of loop count is decided, I'm not
> sure how should it be modified the driver to pass the USB compliance
> test. Is it possible for me to receive a proposal for a solution?  As my
> thought, the number of loops may be redundant, so I think if the user
> can change it arbitrarily, the problem will be solved. Currently, the
> timeout value can be adjusted with a module parameter, but the loop
> count cannot. Is there any problem if it is changed like this patch?
> The original handling of the driver is unchanged by this modification. I
> think the user will be able to adjust the time to enumeration failure
> freely. Is this patch acceptable? I would appreciate it much if I could
> receive the feedback from you.
> 
> ----------------------------------------------------------------------------------------------------------------------------------------
> [1] 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
>    - 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.
> ----------------------------------------------------------------------------------------------------------------------------------------
> [2] hub_port_connect()
> 
>         if (hub->hdev->parent ||
>                         !hcd->driver->port_handed_over ||
>                         !(hcd->driver->port_handed_over)(hcd, port1)) {
>                 if (status != -ENOTCONN && status != -ENODEV)
>                         dev_err(&port_dev->dev,
>                                         "unable to enumerate USB device\n");
>         }
> ----------------------------------------------------------------------------------------------------------------------------------------
> [3] http://www.mqp.com/usbcomp.htm
> ----------------------------------------------------------------------------------------------------------------------------------------
> [4] hub_port_init()
> 
>   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);
> ----------------------------------------------------------------------------------------------------------------------------------------
> [5]
> static int initial_descriptor_timeout = USB_CTRL_GET_TIMEOUT;
> include/linux/usb.h:#define USB_CTRL_GET_TIMEOUT        5000
> ----------------------------------------------------------------------------------------------------------------------------------------
> [6] The flow of device enumeration processing
> drivers/usb/core/hub.c
> 
> hub_port_connect(){
>        for (x = 0; x < SET_CONFIG_TRIES; x++) {                  ...[7] SET_CONFIG_TRIES is 4 as default setting
>                hub_port_init(){
>                        if( x < 2 ) {
>                              ------ new scheme ------
>                              for ( y= 0; y < 2; ++y ) {          ...[8] 2==GET_DESCRIPTOR_TRIES
>                                     for ( z = 0; z < 3; ++z ) {  ...[9]
>                                            usb_control_msg()     ...[3] ETIMEDOUT(-110) is detected. Timieout value=5000msec.
>                                     }
>                              }
>                        }
>                        else {
>                              ------ old scheme ------
>                              for ( y= 0; y < 2; ++y ) {          ...[10] 2==SET_ADDRESS_TRIES
>                                     hub_set_address()            ...[11] ETIMEDOUT(-110) is detected. Timieout value=5000msec.
>                              }
>                        }
>                }
>        }
>        unable to enumerate USB device.                           ...[2]
> }
> ----------------------------------------------------------------------------------------------------------------------------------------
> [12] https://www.usb.org/document-library/usb-20-specification
> Universal Serial Bus Specification (usb_20.pdf)
> 9.1.2 Bus Enumeration
> 6. Before the USB device receives a unique address, its Default Control Pipe is still accessible via the default address.
>    The host reads the device descriptor to determine what actual maximum data payload size this USB device's default pipe can use.
> ----------------------------------------------------------------------------------------------------------------------------------------
> [13] https://www.usb.org/document-library/usb-20-specification
> On-The-Go and Embedded Host Supplement to the USB Revision 2.0 Specification (USB_OTG_and_EH_2-0-version 1_1a.pdf)
> 3.5 No Silent Failures
> ----------------------------------------------------------------------------------------------------------------------------------------
> 
> Signed-off-by: Yasushi Asano <yasano@jp.adit-jv.com>
> ---
>  drivers/usb/core/hub.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 052d5ac..01c2b2d 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -99,6 +99,18 @@ MODULE_PARM_DESC(use_both_schemes,
>  		"try the other device initialization scheme if the "
>  		"first one fails");
>  
> +static int get_descriptor_tries = 2;
> +module_param(get_descriptor_tries, int, S_IRUGO | S_IWUSR);
> +MODULE_PARM_DESC(get_descriptor_tries,
> +		"The number of a 64-byte GET_DESCRIPTOR request tries "
> +		"(default 2)");
> +
> +static int get_descriptor_operations = 3;
> +module_param(get_descriptor_operations, int, S_IRUGO | S_IWUSR);
> +MODULE_PARM_DESC(get_descriptor_operations,
> +		"The number of a 64-byte GET_DESCRIPTOR operation "
> +		"(default 3)");
> +
>  /* Mutual exclusion for EHCI CF initialization.  This interferes with
>   * port reset on some companion controllers.
>   */
> @@ -2707,7 +2719,8 @@ 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 GET_DESCRIPTOR_TRIES	get_descriptor_tries
> +#define GET_DESCRIPTOR_OPERATIONS	get_descriptor_operations

No need for these defines now that you have a variable being used,
right?

>  #define SET_CONFIG_TRIES	(2 * (use_both_schemes + 1))
>  #define USE_NEW_SCHEME(i, scheme)	((i) / 2 == (int)(scheme))
>  
> @@ -4684,7 +4697,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) {

module parameters are not ok, they work for all devices/hubs in the
system, and no one knows how to change them or not.

Any other way we can "just always do it correctly" here?

thanks,

greg k-h

Re: [PATCH] [RFC] USB: hub.c: Add the retry count module parameter for usbcore

[Alan Stern]

On Thu, Jul 30, 2020 at 07:42:26PM +0900, Yasushi Asano wrote:
> From: Yasushi Asano <yasano@jp.adit-jv.com>
> 
> Dear Alan
> Dear Greg
> 
> I would like to have a consultation with you. The customer's product
> board needs to get a USB certification and compliance, but it can not
> pass the test using the current USB hub driver. According to the USB
> compliance plan[1], the“6.7.22 Device No Response” test requires the
> detection of device enumeration failure within 30 seconds. The
> kernel(USB hub driver) must notify userspace of the enumeration failure
> within 30 seconds.
> 
> In the current hub driver, the place to detect device enumeration
> failure is [2]. I have modified the hub driver to issue a uevent here,
> but the procedure of device enumeration (new_scheme+old_scheme) takes
> too long to execute, it couldn't reach [2] within 30 seconds after
> starting the test.  According to the result of PETtool [3], the state of
> "Device No response" is that usb_control_msg(USB_REQ_GET_DESCRIPTOR) [4]
> in hub_port_initn times out. That means r == -ETIMEDOUT.  because of the
> default setting of initial_descriptor_timeout is 5000 msec[5],
> usb_control_msg() took 5000msec until -ETIMEDOUT.
> 
> I will try to describe the flow of device enumeration processing
> below[6].  If my understanding is correct, the enumeration failure [2]
> will be reached after performing all the loops of [7][8][9]+[7][10].  In
> the new scheme, 12 times check will be performed ([7]/2*[8]*[9] => 2*2*3
> => 12).  In the old scheme , 4 times check will be performed ([7]/2*[10]
> => 2*2 => 4).  In total, it checks 16 times, and it took 5000msec to
> ETIMEDOUT every time. Therefore, It takes about 80 seconds(16*5000msec)
> to reach [2]. This does not pass the "Device No response" test.

I agree with Greg, we don't want to add module parameters.

The problem is that we make up to 16 attempts, and each attempt can take 
5 seconds.  We need to decrease the number of attempts to 6; then the 
total time will be 30 seconds.  We also need to keep both the old and 
new schemes.

So let's change the code to do 3 tries with each scheme.  For example,

	3 * new scheme, then 3 * old scheme, or else

	3 * old scheme, then 3 * new_scheme,

depending on the old_scheme_first parameter.  Change the loops in [7], 
[8], [9], and [10] so that each iteration does the next item on this 
list instead of starting back at the beginning.

(Or maybe it would work better with

	2 * scheme, then 2 * old scheme, then 1 * new scheme,
		then 1 * old scheme

with old and new swapped if old_scheme_first is set.  I don't know.)

Anyway, can you write a patch to do this?

Alan Stern

RE: [PATCH] [RFC] USB: hub.c: Add the retry count module parameter for usbcore

[Asano, Yasushi (ADITJ/SWG)]

Dear Alan
Dear Greg

Thank you for your feedback.
I really appreciate your concrete proposal.

> So let's change the code to do 3 tries with each scheme.
I understood. I will try to modify it so that the number of 
attempts will decrease. It is 6 attempts in total both old and 
new schemes, but msleep is executed at various places in 
hub_port_connect and hub_port_init. apart from a timeout.
 
For example, msleep(100) is executed every time in the loop of 
GET_DESCRIPTOR_TRIES[8] of new scheme. and In the old scheme, 
msleep(200) is executed in the loop of SET_ADDRESS_TRIES[10].
From my measurement, it does not subside within 30 seconds, 
but it is around 32 seconds.

From these things, I would like you to reconsider the number of attempts. 
Is it OK to set the new scheme to 3 times and the old scheme to 
2 times(no change as it is)? In other words 

[plan 1]
3 * new scheme, then 2 * old scheme, or else
2 * old scheme, then 3 * new_scheme,
depending on the old_scheme_first parameter.

Also, although it is a "better plan", the original processing is in the following.

6 * new scheme, then 6 * new scheme, 
then 2 * old scheme, then 2 * old scheme

if it will be modified from above to below, It seems that the structure 
of the loop has to be greatly revised. I think.

2 * new scheme, then 2 * old scheme, 
then 1 * new scheme, then 1 * old scheme

The fix is likely to be large, so Can I proceed with a patch in plan 1?
I will post the patch after confirming the behavior of the patch with 
the customer board with the PET tool. please give me a little time.

Best Regards
Yasushi Asano

On Thu, Jul 30, 2020 at 07:42:26PM +0900, Yasushi Asano wrote:
> From: Yasushi Asano <yasano@jp.adit-jv.com>
> 
> Dear Alan
> Dear Greg
> 
> I would like to have a consultation with you. The customer's product
> board needs to get a USB certification and compliance, but it can not
> pass the test using the current USB hub driver. According to the USB
> compliance plan[1], the“6.7.22 Device No Response” test requires the
> detection of device enumeration failure within 30 seconds. The
> kernel(USB hub driver) must notify userspace of the enumeration failure
> within 30 seconds.
> 
> In the current hub driver, the place to detect device enumeration
> failure is [2]. I have modified the hub driver to issue a uevent here,
> but the procedure of device enumeration (new_scheme+old_scheme) takes
> too long to execute, it couldn't reach [2] within 30 seconds after
> starting the test.  According to the result of PETtool [3], the state of
> "Device No response" is that usb_control_msg(USB_REQ_GET_DESCRIPTOR) [4]
> in hub_port_initn times out. That means r == -ETIMEDOUT.  because of the
> default setting of initial_descriptor_timeout is 5000 msec[5],
> usb_control_msg() took 5000msec until -ETIMEDOUT.
> 
> I will try to describe the flow of device enumeration processing
> below[6].  If my understanding is correct, the enumeration failure [2]
> will be reached after performing all the loops of [7][8][9]+[7][10].  In
> the new scheme, 12 times check will be performed ([7]/2*[8]*[9] => 2*2*3
> => 12).  In the old scheme , 4 times check will be performed ([7]/2*[10]
> => 2*2 => 4).  In total, it checks 16 times, and it took 5000msec to
> ETIMEDOUT every time. Therefore, It takes about 80 seconds(16*5000msec)
> to reach [2]. This does not pass the "Device No response" test.

I agree with Greg, we don't want to add module parameters.

The problem is that we make up to 16 attempts, and each attempt can take 
5 seconds.  We need to decrease the number of attempts to 6; then the 
total time will be 30 seconds.  We also need to keep both the old and 
new schemes.

So let's change the code to do 3 tries with each scheme.  For example,

	3 * new scheme, then 3 * old scheme, or else

	3 * old scheme, then 3 * new_scheme,

depending on the old_scheme_first parameter.  Change the loops in [7], 
[8], [9], and [10] so that each iteration does the next item on this 
list instead of starting back at the beginning.

(Or maybe it would work better with

	2 * scheme, then 2 * old scheme, then 1 * new scheme,
		then 1 * old scheme

with old and new swapped if old_scheme_first is set.  I don't know.)

Anyway, can you write a patch to do this?

Alan Stern

Re: [PATCH] [RFC] USB: hub.c: Add the retry count module parameter for usbcore

[Alan Stern]

On Thu, Aug 06, 2020 at 05:43:54AM +0000, Asano, Yasushi (ADITJ/SWG) wrote:
> Dear Alan
> Dear Greg
> 
> Thank you for your feedback.
> I really appreciate your concrete proposal.
> 
> > So let's change the code to do 3 tries with each scheme.
> I understood. I will try to modify it so that the number of 
> attempts will decrease. It is 6 attempts in total both old and 
> new schemes, but msleep is executed at various places in 
> hub_port_connect and hub_port_init. apart from a timeout.
>  
> For example, msleep(100) is executed every time in the loop of 
> GET_DESCRIPTOR_TRIES[8] of new scheme. and In the old scheme, 
> msleep(200) is executed in the loop of SET_ADDRESS_TRIES[10].
> From my measurement, it does not subside within 30 seconds, 
> but it is around 32 seconds.
> 
> From these things, I would like you to reconsider the number of attempts. 
> Is it OK to set the new scheme to 3 times and the old scheme to 
> 2 times(no change as it is)? In other words 
> 
> [plan 1]
> 3 * new scheme, then 2 * old scheme, or else
> 2 * old scheme, then 3 * new_scheme,
> depending on the old_scheme_first parameter.

Yes, that's all right.  Although you might want to make the second case 
be: 3 * old scheme, then 2 * new scheme.

> Also, although it is a "better plan", the original processing is in the following.
> 
> 6 * new scheme, then 6 * new scheme, 
> then 2 * old scheme, then 2 * old scheme
> 
> if it will be modified from above to below, It seems that the structure 
> of the loop has to be greatly revised. I think.
> 
> 2 * new scheme, then 2 * old scheme, 
> then 1 * new scheme, then 1 * old scheme

If you want to use only five attempts, you'll have to get rid of the 
last one.

> The fix is likely to be large, so Can I proceed with a patch in plan 1?

Okay.

Alan Stern

> I will post the patch after confirming the behavior of the patch with 
> the customer board with the PET tool. please give me a little time.
> 
> Best Regards
> Yasushi Asano

[PATCH] [RFC] USB: hub.c: decrease the number of attempts of enumeration scheme

[Yasushi Asano]

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

[PATCH] [RFC] USB: hub.c: decrease the number of attempts of enumeration scheme

[Yasushi Asano]

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

Re: [PATCH] [RFC] USB: hub.c: decrease the number of attempts of enumeration scheme

[Alan Stern]

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