From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751333AbdKEUeg (ORCPT ); Sun, 5 Nov 2017 15:34:36 -0500 Received: from mail-pg0-f43.google.com ([74.125.83.43]:47379 "EHLO mail-pg0-f43.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750750AbdKEUee (ORCPT ); Sun, 5 Nov 2017 15:34:34 -0500 X-Google-Smtp-Source: ABhQp+Ry1VN2EORfj1HWBp0pppmWVxDXlDu+uzFVI3lgPHa6B5ILXcTkQ9v5DQ/oz7OyfhCa+UnyPQ== Subject: Re: [PATCH][RFC] usb: hub: Cycle HUB power when initialization fails To: Mike Looijmans , Alan Stern Cc: linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, gregkh@linuxfoundation.org References: From: Guenter Roeck Message-ID: <0267c3c1-7268-bdac-ccb6-8aebf29311ca@roeck-us.net> Date: Sun, 5 Nov 2017 12:34:31 -0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 11/05/2017 10:41 AM, Mike Looijmans wrote: > On 03-11-17 18:27, Alan Stern wrote: >> On Fri, 3 Nov 2017, Mike Looijmans wrote: >> >>> Sometimes the USB device gets confused about the state of the initialization and >>> the connection fails. In particular, the device thinks that it's already set up >>> and running while the host thinks the device still needs to be configured. To >> >> How do you know that this is really the issue?  How can the device >> think it's already set if it doesn't have an assigned address? > > It seems to me that the device just doesn't react at all on the host requests to assign an address. I've seen this happen with various custom mass-storage like appliances, but also DVB tuners and such. The device won't return to a working state until you unplug it and put it back, or, and that's what the patch does, just power-cycle the USB port, which has the same effect. > We have seen this problem with Ethernet dongles left in a bad/hung state during a previous boot, so it definitely does happen. This happened, for example, with the r8152 driver with upstream commit 2f25abe6bac ("r8152: prevent the driver from transmitting packets with carrier off") missing. Problem though, as mentioned, is that many hubs don't implement this feature, and if I understand correctly root hubs don't implement it either. Guenter >>> work around this issue, power-cycle the hub's output to issue a sort of "reset" >>> to the device. This makes the device restart its state machine and then the >>> initialization succeeds. >>> >>> This fixes problems where the kernel reports a list of errors like this: >>> >>> usb 1-1.3: device not accepting address 19, error -71 >>> >>> The end result is a non-functioning device. After this patch, the sequence >>> becomes like this: >>> >>> usb 1-1.3: new high-speed USB device number 18 using ci_hdrc >>> usb 1-1.3: device not accepting address 18, error -71 >>> usb 1-1.3: new high-speed USB device number 19 using ci_hdrc >>> usb 1-1.3: device not accepting address 19, error -71 >>> usb 1-1-port3: attempt power cycle >>> usb 1-1.3: new high-speed USB device number 21 using ci_hdrc >>> usb-storage 1-1.3:1.2: USB Mass Storage device detected >>> >>> Signed-off-by: Mike Looijmans >>> --- >>> This is a fix I did for a customer which might be appropriate for upstream. What do you think? >>> >>>   drivers/usb/core/hub.c | 13 ++++++++++--- >>>   1 file changed, 10 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c >>> index e9ce6bb..a30c1e7 100644 >>> --- a/drivers/usb/core/hub.c >>> +++ b/drivers/usb/core/hub.c >>> @@ -2611,7 +2611,7 @@ static unsigned hub_is_wusb(struct usb_hub *hub) >>>   #define PORT_RESET_TRIES    5 >>>   #define SET_ADDRESS_TRIES    2 >>>   #define GET_DESCRIPTOR_TRIES    2 >>> -#define SET_CONFIG_TRIES    (2 * (use_both_schemes + 1)) >>> +#define SET_CONFIG_TRIES    (4 * (use_both_schemes + 1)) >> >> We already have too many retry loops.  I am not keen on the idea of >> adding even more.  How about leaving this value the same and adding the >> power cycle? > > I'm fine with that as well. > >> The ideal, however, would be to find out what is wrong with the device >> and see what needs to be done to fix it properly.  This change won't >> work on many computers (desktops and laptops) because they don't have >> real USB port-power switching.  A lot of hubs don't have it either. > > I'm pretty sure it's the device's fault, but they're out there and probably not upgradable anyway if you could get the manufacturer to pick up the phone. > > On desktop/laptop machines the problem isn't as pressing since there's a often a user there who can unplug the thing. It's really nasty on embedded systems, and they tend to have the USB power wired through a supply limiter/switch. > I'm thinking worst that could happen on desktops is that the patch won't have any effect at all. > > So what do you think, is this worth a v2 patch for general consumption or should I keep this a "private" patch for systems that have demonstrated to benefit from it? > > >>>   #define USE_NEW_SCHEME(i)    ((i) / 2 == (int)old_scheme_first) >>>   #define HUB_ROOT_RESET_TIME    60    /* times are in msec */ >>> @@ -4805,7 +4805,6 @@ static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus, >>>       status = 0; >>>       for (i = 0; i < SET_CONFIG_TRIES; i++) { >>> - >> >> Gratuitous whitespace change. >> >>>           /* reallocate for each attempt, since references >>>            * to the previous one can escape in various ways >>>            */ >>> @@ -4935,6 +4934,15 @@ static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus, >>>           usb_put_dev(udev); >>>           if ((status == -ENOTCONN) || (status == -ENOTSUPP)) >>>               break; >>> + >>> +        /* When halfway through our retry count, power-cycle the port */ >>> +        if (i == (SET_CONFIG_TRIES / 2) - 1) { >>> +            dev_info(&port_dev->dev, "attempt power cycle\n"); >>> +            usb_hub_set_port_power(hdev, hub, port1, false); >>> +            msleep(800); >>> +            usb_hub_set_port_power(hdev, hub, port1, true); >>> +            msleep(hub_power_on_good_delay(hub)); >>> +        } >>>       } >>>       if (hub->hdev->parent || >>>               !hcd->driver->port_handed_over || >>> @@ -5476,7 +5484,6 @@ static int usb_reset_and_verify_device(struct usb_device *udev) >>>       udev->bos = NULL; >>>       for (i = 0; i < SET_CONFIG_TRIES; ++i) { >>> - >> >> Another gratuitous change. >> >>>           /* ep0 maxpacket size may change; let the HCD know about it. >>>            * Other endpoints will be handled by re-enumeration. */ >>>           usb_ep0_reinit(udev); >> >> Alan Stern >> > >