linux-kernel - Re: [PATCH][RFC] usb: hub: Cycle HUB power when initialization fails

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Pine.LNX.4.44L0.1711031320290.1743-100000@iolanthe.rowland.org>
Date:   Fri, 3 Nov 2017 13:27:30 -0400 (EDT)
From:   Alan Stern <stern@...land.harvard.edu>
To:     Mike Looijmans <mike.looijmans@...ic.nl>
cc:     linux-usb@...r.kernel.org, <linux-kernel@...r.kernel.org>,
        <gregkh@...uxfoundation.org>, <linux@...ck-us.net>
Subject: Re: [PATCH][RFC] usb: hub: Cycle HUB power when initialization fails

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?

> 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 <mike.looijmans@...ic.nl>
> ---
> 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?

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.

>  #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