On Thu, Jul 30, 2020 at 07:42:26PM +0900, Yasushi Asano wrote: > From: Yasushi Asano <yasano@xxxxxxxxxxxxxx> > > 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@xxxxxxxxxxxxxx> > --- > 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
