Re: [PATCH] usb: hub: try old enumeration scheme first for high speed devices

[Roger Quadros]

On 14/08/18 10:33, Zengtao (B) wrote:
> Hi Roger:
> 
> Thank you for review
> 
>> -----Original Message-----
>> From: Roger Quadros [mailto:rogerq@xxxxxx]
>> Sent: Friday, August 10, 2018 6:51 PM
>> To: Zengtao (B) <prime.zeng@xxxxxxxxxxxxx>; gregkh@xxxxxxxxxxxxxxxxxxx;
>> stern@xxxxxxxxxxxxxxxxxxx; mathias.nyman@xxxxxxxxxxxxxxx;
>> drinkcat@xxxxxxxxxxxx; felipe.balbi@xxxxxxxxxxxxxxx; drake@xxxxxxxxxxxx;
>> mike.looijmans@xxxxxxxx; joe@xxxxxxxxxxx
>> Cc: linux-usb@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
>> Subject: Re: [PATCH] usb: hub: try old enumeration scheme first for high speed
>> devices
>>
>> Hi,
>>
>> On 10/08/18 18:01, Zeng Tao wrote:
>>> The new scheme is required just to support legacy low and full-speed
>>> devices. For high speed devices, it will slower the enumeration speed.
>>> So in this patch we try the "old" enumeration scheme first for high
>>> speed devices.
>>
>> How slow does it get? Is it significant?
>> Do we risk breaking existing HS devices that work? I don't think we can be sure
>> till we run this through testing.
>>
> 
> We added the new scheme because this is what the windows did , and mainly for 
> legacy low and full speed devices.
> Now for new windows version(8.1 and later), the second port reset has already been
> removed for high speed devices for better enumeration speed.
> And In this patch if use_both_schemes is true, it will fallback to new scheme if the old
> scheme fails.
> 
> So I think it's reasonable to follow the windows behavior again.
> 
>>>
>>> Signed-off-by: Zeng Tao <prime.zeng@xxxxxxxxxxxxx>
>>> ---
>>>  drivers/usb/core/hub.c | 5 ++++-
>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index
>>> 1fb2668..d265b19 100644
>>> --- a/drivers/usb/core/hub.c
>>> +++ b/drivers/usb/core/hub.c
>>> @@ -2661,10 +2661,13 @@ static bool use_new_scheme(struct usb_device
>> *udev, int retry,
>>>  	int old_scheme_first_port =
>>>  		port_dev->quirks & USB_PORT_QUIRK_OLD_SCHEME;
>>>
>>> +	int quick_enumeration = (udev->speed == USB_SPEED_HIGH);
>>> +
>>>  	if (udev->speed >= USB_SPEED_SUPER)
>>>  		return false;
>>
>> how about replacing the above if with
>>
>> 	if (udev->speed >= USB_SPEED_HIGH)
>> 		return false;
> 
> No, for SS device, only use old scheme, but for speed device, we can fallback to new 
> scheme if the old fails.

You are correct.
> 
>>>
>>> -	return USE_NEW_SCHEME(retry, old_scheme_first_port ||
>> old_scheme_first);
>>> +	return USE_NEW_SCHEME(retry, old_scheme_first_port ||
>> old_scheme_first
>>> +			      || quick_enumeration);
>>>  }
>>
>> Now we no longer respect the "old_scheme_first" parameter for most of the
>> devices.
>>
>> It should be clarified in Documentation/admin/kernel-parameters.txt that
>> "old_scheme_first" is only applicable to LOW/FULL speed devices.
>>
> 
> On the contrary, new scheme is only applicable for LOW/FULL speed devices? 

No. What I meant is that the "old_scheme_first" module parameter is now
only applicable for LOW/FULL speed devices.

As HIGH_SPEED devices will always use old scheme first after this patch.

cheers,
-roger

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki