Re: [PATCH] usb: hub: Allow reset retry for USB2 devices on connect bounce

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

On 16.10.2017 22:34, Alan Stern wrote:
> On Mon, 16 Oct 2017, Mathias Nyman wrote:
>
>> If the connect status change is set during reset signaling, but
>> the status remains connected just retry port reset.
>>
>> This solves an issue with connecting a 90W HP Thunderbolt 3 dock
>> with a Lenovo Carbon x1 (5th generation) which causes a 30min loop
>> of a high speed device being re-discovererd before usb ports starts
>> working.
>>
>> [...]
>> [ 389.023845] usb 3-1: new high-speed USB device number 55 using xhci_hcd
>> [ 389.491841] usb 3-1: new high-speed USB device number 56 using xhci_hcd
>> [ 389.959928] usb 3-1: new high-speed USB device number 57 using xhci_hcd
>> [...]
>>
>> This is caused by a high speed device that doesn't successfully go to the
>> enabled state after the second port reset. Instead the connection bounces
>> (connected, with connect status change), bailing out completely from
>> enumeration just to restart from scratch.
>>
>> Link: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1716332
>>
>> Cc: Stable <stable@vger.kernel.org>
>> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
>> ---
>>   drivers/usb/core/hub.c | 10 +++++++---
>>   1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
>> index 41eaf0b..3461347 100644
>> --- a/drivers/usb/core/hub.c
>> +++ b/drivers/usb/core/hub.c
>> @@ -2710,13 +2710,13 @@ static int hub_port_wait_reset(struct usb_hub *hub, int port1,
>>   	if (!(portstatus & USB_PORT_STAT_CONNECTION))
>>   		return -ENOTCONN;
>>
>> -	/* bomb out completely if the connection bounced.  A USB 3.0
>> -	 * connection may bounce if multiple warm resets were issued,
>> +	/* Retry if connect change is set but status is still connected.
>> +	 * A USB 3.0 connection may bounce if multiple warm resets were issued,
>>   	 * but the device may have successfully re-connected. Ignore it.
>>   	 */
>>   	if (!hub_is_superspeed(hub->hdev) &&
>>   			(portchange & USB_PORT_STAT_C_CONNECTION))
>> -		return -ENOTCONN;
>> +		return -EAGAIN;
>>
>>   	if (!(portstatus & USB_PORT_STAT_ENABLE))
>>   		return -EBUSY;
>> @@ -2789,6 +2789,10 @@ static int hub_port_reset(struct usb_hub *hub, int port1,
>>   				dev_dbg(hub->intfdev,
>>   						"port_wait_reset: err = %d\n",
>>   						status);
>> +			/* USB2 connection bounced, but remains connected */
>> +			if (status == -EAGAIN)
>> +				usb_clear_port_feature(hub->hdev, port1,
>> +						USB_PORT_FEAT_C_CONNECTION);
>
> There's something very awkward about issuing this call here.  For one
> thing, we don't really know at this point that the C_CONNECTION port
> status bit is set; we only know that the reset needs to be retried.
> For another, about 14 lines lower down the routine already clears that
> status bit anyway (although not for the USB-2.0 case).

Agree, but no other change bits were cleared in hub_port_wait_reset().
So I cleared it where the others were.

>
> In fact, it's awkward that we issue a bunch of unconditional
> Clear-Port-Feature calls just below this point, without knowing whether
> the corresponding status bits are set.  Ideally, hub_port_wait_reset()
> would return its portstatus and portchange values.  Or it would clear
> the extra status bits by itself.  Maybe that can all be cleaned up in a
> later patch.

True
hub_port_wait_reset() is only called by hub_port_reset() so I don't see any
reason why we couldn't clean up the change bits in hub_port_wait_reset().

Especially the connect change bit which we now want to clear in both
usb2 and usb3 case if set. I'll change the patch.

>
> Also, does this fully solve the problem?  I can see that it would
> prevent the lengthy repeated rediscoveries, but wouldn't it also
> prevent the device from working at all after the first few resets
> failed?
>

It does, the device enumerates fine after one more reset.
I don't know if the device just fails every second reset, or if it only fails to re-reset
after it just reached enable state.

-Mathias