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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxxxx>
> Signed-off-by: Mathias Nyman <mathias.nyman@xxxxxxxxxxxxxxx>
> ---
>  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




[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]