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

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

[Mathias Nyman]

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);
 		}
 
 		/* Check for disconnect or reset */
-- 
2.7.4

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

[Alan Stern]

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).

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.

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?

Alan Stern

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

[Mathias Nyman]

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