All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH][RFC] usb: hub: Cycle HUB power when initialization fails
@ 2017-11-03 11:11 Mike Looijmans
  2017-11-03 17:27 ` Alan Stern
  0 siblings, 1 reply; 6+ messages in thread
From: Mike Looijmans @ 2017-11-03 11:11 UTC (permalink / raw)
  To: linux-usb; +Cc: linux-kernel, gregkh, stern, linux, Mike Looijmans

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
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@topic.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))
 #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++) {
-
 		/* 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) {
-
 		/* ep0 maxpacket size may change; let the HCD know about it.
 		 * Other endpoints will be handled by re-enumeration. */
 		usb_ep0_reinit(udev);
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH][RFC] usb: hub: Cycle HUB power when initialization fails
  2017-11-03 11:11 [PATCH][RFC] usb: hub: Cycle HUB power when initialization fails Mike Looijmans
@ 2017-11-03 17:27 ` Alan Stern
  2017-11-05 18:41   ` Mike Looijmans
  2017-11-09 16:23   ` [PATCH v2] " Alan Stern
  0 siblings, 2 replies; 6+ messages in thread
From: Alan Stern @ 2017-11-03 17:27 UTC (permalink / raw)
  To: Mike Looijmans; +Cc: linux-usb, linux-kernel, gregkh, linux

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@topic.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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH][RFC] usb: hub: Cycle HUB power when initialization fails
  2017-11-03 17:27 ` Alan Stern
@ 2017-11-05 18:41   ` Mike Looijmans
  2017-11-05 20:34     ` Guenter Roeck
  2017-11-06 18:40     ` Alan Stern
  2017-11-09 16:23   ` [PATCH v2] " Alan Stern
  1 sibling, 2 replies; 6+ messages in thread
From: Mike Looijmans @ 2017-11-05 18:41 UTC (permalink / raw)
  To: Alan Stern; +Cc: linux-usb, linux-kernel, gregkh, linux

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.

>> 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@topic.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?

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
> 


-- 
Mike Looijmans


Kind regards,

Mike Looijmans
System Expert

TOPIC Products
Materiaalweg 4, NL-5681 RJ Best
Postbus 440, NL-5680 AK Best
Telefoon: +31 (0) 499 33 69 79
E-mail: mike.looijmans@topicproducts.com
Website: www.topicproducts.com

Please consider the environment before printing this e-mail

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH][RFC] usb: hub: Cycle HUB power when initialization fails
  2017-11-05 18:41   ` Mike Looijmans
@ 2017-11-05 20:34     ` Guenter Roeck
  2017-11-06 18:40     ` Alan Stern
  1 sibling, 0 replies; 6+ messages in thread
From: Guenter Roeck @ 2017-11-05 20:34 UTC (permalink / raw)
  To: Mike Looijmans, Alan Stern; +Cc: linux-usb, linux-kernel, gregkh

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH][RFC] usb: hub: Cycle HUB power when initialization fails
  2017-11-05 18:41   ` Mike Looijmans
  2017-11-05 20:34     ` Guenter Roeck
@ 2017-11-06 18:40     ` Alan Stern
  1 sibling, 0 replies; 6+ messages in thread
From: Alan Stern @ 2017-11-06 18:40 UTC (permalink / raw)
  To: Mike Looijmans; +Cc: linux-usb, linux-kernel, gregkh, linux

On Sun, 5 Nov 2017, 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.
> 
> >> 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@topic.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?
> 
> 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?

We might as well apply a v2 patch, since it will help on some systems.

Alan stern

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] usb: hub: Cycle HUB power when initialization fails
  2017-11-03 17:27 ` Alan Stern
  2017-11-05 18:41   ` Mike Looijmans
@ 2017-11-09 16:23   ` Alan Stern
  1 sibling, 0 replies; 6+ messages in thread
From: Alan Stern @ 2017-11-09 16:23 UTC (permalink / raw)
  To: Mike Looijmans; +Cc: linux-usb, linux-kernel, gregkh, linux

On Thu, 9 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
> 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@topic.nl>
> ---
> v2: On feedback from Alan Stern:
>       No blank line changes
>       Don't further increase the retry count
>     Delay 2x the power-up delay (100ms minimum) instead of magic "800"
> 
>  drivers/usb/core/hub.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index e9ce6bb..8f7d942 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -4935,6 +4935,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(2 * hub_power_on_good_delay(hub));
> +			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 ||

Acked-by: Alan Stern <stern@rowland.harvard.edu>

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2017-11-09 16:23 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-03 11:11 [PATCH][RFC] usb: hub: Cycle HUB power when initialization fails Mike Looijmans
2017-11-03 17:27 ` Alan Stern
2017-11-05 18:41   ` Mike Looijmans
2017-11-05 20:34     ` Guenter Roeck
2017-11-06 18:40     ` Alan Stern
2017-11-09 16:23   ` [PATCH v2] " Alan Stern

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.