From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga05.intel.com ([192.55.52.43]:36502 "EHLO mga05.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754581AbdJQIgi (ORCPT ); Tue, 17 Oct 2017 04:36:38 -0400 Subject: Re: [PATCH] usb: hub: Allow reset retry for USB2 devices on connect bounce To: Alan Stern References: Cc: gregkh@linuxfoundation.org, linux-usb@vger.kernel.org, Stable From: Mathias Nyman Message-ID: <59E5C202.4060305@linux.intel.com> Date: Tue, 17 Oct 2017 11:40:34 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: stable-owner@vger.kernel.org List-ID: 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 >> Signed-off-by: Mathias Nyman >> --- >> 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