Re: [PATCH] usb: hub: avoid warm port reset during USB3 disconnect

[Mathias Nyman <mathias.nyman@linux.intel.com>]

On 10.12.2021 21.12, Alan Stern wrote:
> On Fri, Dec 10, 2021 at 01:16:53PM +0200, Mathias Nyman wrote:
>> During disconnect USB-3 ports often go via SS.Inactive link error state
>> before the missing terminations are noticed, and link finally goes to
>> RxDetect state
>>
>> Avoid immediately warm-resetting ports in SS.Inactive state.
>> Let ports settle for a while and re-read the link status a few times 20ms
>> apart to see if the ports transitions out of SS.Inactive.
>>
>> According to USB 3.x spec 7.5.2, a port in SS.Inactive should
>> automatically check for missing far-end receiver termination every
>> 12 ms (SSInactiveQuietTimeout)
>>
>> The futile multiple warm reset retries of a disconnected device takes
>> a lot of time, also the resetting of a removed devices has caused cases
>> where the reset bit got stuck for a long time on xHCI roothub.
>> This lead to issues in detecting new devices connected to the same port
>> shortly after.
>>
>> Tested-by: Mark Pearson <markpearson@lenovo.com>
>> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
>> ---
>>  drivers/usb/core/hub.c | 24 +++++++++++++++++++-----
>>  1 file changed, 19 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
>> index 00070a8a6507..e907dfa0ca6d 100644
>> --- a/drivers/usb/core/hub.c
>> +++ b/drivers/usb/core/hub.c
>> @@ -2777,6 +2777,8 @@ static unsigned hub_is_wusb(struct usb_hub *hub)
>>  #define PORT_INIT_TRIES		4
>>  #endif	/* CONFIG_USB_FEW_INIT_RETRIES */
>>  
>> +#define DETECT_DISCONNECT_TRIES 5
>> +
>>  #define HUB_ROOT_RESET_TIME	60	/* times are in msec */
>>  #define HUB_SHORT_RESET_TIME	10
>>  #define HUB_BH_RESET_TIME	50
>> @@ -5543,6 +5545,7 @@ static void port_event(struct usb_hub *hub, int port1)
>>  	struct usb_device *udev = port_dev->child;
>>  	struct usb_device *hdev = hub->hdev;
>>  	u16 portstatus, portchange;
>> +	int i = 0;
>>  
>>  	connect_change = test_bit(port1, hub->change_bits);
>>  	clear_bit(port1, hub->event_bits);
>> @@ -5619,17 +5622,27 @@ static void port_event(struct usb_hub *hub, int port1)
>>  		connect_change = 1;
>>  
>>  	/*
>> -	 * Warm reset a USB3 protocol port if it's in
>> -	 * SS.Inactive state.
>> +	 * Avoid trying to recover a USB3 SS.Inactive port with a warm reset if
>> +	 * the device was disconnected. A 12ms disconnect detect timer in
>> +	 * SS.Inactive state transitions the port to RxDetect automatically.
>> +	 * SS.Inactive link error state is common during device disconnect.
>>  	 */
>> -	if (hub_port_warm_reset_required(hub, port1, portstatus)) {
>> -		dev_dbg(&port_dev->dev, "do warm reset\n");
>> -		if (!udev || !(portstatus & USB_PORT_STAT_CONNECTION)
>> +	while (hub_port_warm_reset_required(hub, port1, portstatus)) {
>> +		if ((i++ < DETECT_DISCONNECT_TRIES) && udev) {
>> +			u16 unused;
>> +
>> +			msleep(20);
>> +			hub_port_status(hub, port1, &portstatus, &unused);
>> +			dev_dbg(&port_dev->dev, "Wait for inactive link disconnect detect\n");
>> +			continue;
> 
> This may be bikeshedding, and you should feel free to ignore the 
> following suggestion if you dislike it.
> 
> Don't you think it would be a lot clearer if the new "while" loop 
> covered only the code above, and the two sections below (port-only or 
> full-device warm reset) came after the end of the loop?  I had to reread 
> the patch a few times to figure out what it was really doing.
> 
> Alan Stern
>

I did first write this as:

while (hub_port_warm_reset_required(portstatus) && dev && still_retry--) {
	msleep();
	hub_port_status(&portstatus);
}

if (hub_port_warm_reset_required(portstatus)) {
	if (!dev || ...)
		hub_port_reset()
	else
		usb_reset_device()
}

But then decided it might be clearer to check if warm reset is required
in just one place.
Maybe it got a bit unnecessary complex to read inside that while loop,
even if this patch doesn't add any more code than the other way.

That said, this patch was tested by other external teams than just
Mark Pearson, all giving their thumbs up.
Not sure I can get the same amount of testing on a new version, and for
that reason, if there are no stronger objections, I'd like to keep this
as is. 

Thanks
-Mathias