diff mbox

[U-Boot] usb: hub: don't check CONNECTION in hub_port_reset()

Message ID 1407452879-4875-1-git-send-email-swarren@wwwdotorg.org
State Accepted
Delegated to: Marek Vasut
Headers show

Commit Message

Stephen Warren Aug. 7, 2014, 11:07 p.m. UTC 
From: Stephen Warren <swarren@nvidia.com>

One specific USB 3.0 device behaves strangely when reset by
usb_new_device()'s call to hub_port_reset(). For some reason, the device
appears to briefly drop off the bus when this second bus reset is
executed, yet if we retry this loop, it'll eventually come back after
another two resets.

If USB bus reset is executed over and over within usb_new_device()'s call
to hub_port_reset(), I see the following sequence of results, which
repeats as long as you want:

1) STAT_C_CONNECTION = 1 STAT_CONNECTION = 0  USB_PORT_STAT_ENABLE 0
2) STAT_C_CONNECTION = 1 STAT_CONNECTION = 1  USB_PORT_STAT_ENABLE 0
3) STAT_C_CONNECTION = 1 STAT_CONNECTION = 1  USB_PORT_STAT_ENABLE 1

The device in question is a SanDisk Ultra USB 3.0 16GB memory stick with
USB VID/PID 0x0781/0x5581.

In order to allow this device to work with U-Boot, ignore the
{C_,}CONNECTION bits in the status/change registers, and only use the
ENABLE bit to determine if the reset was successful.

To be honest, extensive investigation has failed to determine why this
problem occurs. I'd love to know! I don't know if it's caused by:
* A HW bug in the device
* A HW bug in the Tegra USB controller
* A SW bug in the U-Boot Tegra USB driver
* A SW bug in the U-Boot USB core

This issue only occurs when the device's USB3 pins are attached to the
host; if only the USB2 pins are connected the issue does not occur. The
USB3 controller on Tegra is in reset, so is not actively communicating
with the device at all - a USB3 analyzer confirms this. Slightly
unplugging the device (so the USB3 pins don't contact) or using a USB2
cable or hub as an intermediary avoids the problem. For some reason,
the Linux kernel (either on the same Tegra board, or on an x86 host)
has no issue with the device, and I observe no disconnections during
reset.

This change won't affect any USB device that already works, since such
devices could not currently be triggering the error return this patch
removes, or they wouldn't be working currently.

However, this patch is quite reliable in practice, hence I hope it's
acceptable to solve the problem.

The only potential fallout I can see from this patch is:

* A broken device that triggers C_CONNECTION/!CONNECTION now causes the
  loop in hub_port_reset() to run multiple times. If it never succeeds,
  this will cause "usb start" to take roughly 1s extra to execute.

* If the user unplugs a device while hub_port_reset() is executing, and
  very quickly swaps in a new device, hub_port_reset() might succeed on
  the new device. This would mean that any information cached about the
  original device (from the descriptor read in usb_new_device(), which
  simply caches the max packet size) might be invalid, which would cause
  problems talking to the new device. However, without this change, the
  new device wouldn't work anyway, so this is probably not much of a
  loss.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 common/usb_hub.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

Comments

Marek Vasut Aug. 29, 2014, 9:27 a.m. UTC | #1 
On Friday, August 08, 2014 at 01:07:59 AM, Stephen Warren wrote:
> From: Stephen Warren <swarren@nvidia.com>
> 
> One specific USB 3.0 device behaves strangely when reset by
> usb_new_device()'s call to hub_port_reset(). For some reason, the device
> appears to briefly drop off the bus when this second bus reset is
> executed, yet if we retry this loop, it'll eventually come back after
> another two resets.
> 
> If USB bus reset is executed over and over within usb_new_device()'s call
> to hub_port_reset(), I see the following sequence of results, which
> repeats as long as you want:
> 
> 1) STAT_C_CONNECTION = 1 STAT_CONNECTION = 0  USB_PORT_STAT_ENABLE 0
> 2) STAT_C_CONNECTION = 1 STAT_CONNECTION = 1  USB_PORT_STAT_ENABLE 0
> 3) STAT_C_CONNECTION = 1 STAT_CONNECTION = 1  USB_PORT_STAT_ENABLE 1
> 
> The device in question is a SanDisk Ultra USB 3.0 16GB memory stick with
> USB VID/PID 0x0781/0x5581.
> 
> In order to allow this device to work with U-Boot, ignore the
> {C_,}CONNECTION bits in the status/change registers, and only use the
> ENABLE bit to determine if the reset was successful.
> 
> To be honest, extensive investigation has failed to determine why this
> problem occurs. I'd love to know! I don't know if it's caused by:
> * A HW bug in the device
> * A HW bug in the Tegra USB controller
> * A SW bug in the U-Boot Tegra USB driver
> * A SW bug in the U-Boot USB core
> 
> This issue only occurs when the device's USB3 pins are attached to the
> host; if only the USB2 pins are connected the issue does not occur. The
> USB3 controller on Tegra is in reset, so is not actively communicating
> with the device at all - a USB3 analyzer confirms this. Slightly
> unplugging the device (so the USB3 pins don't contact) or using a USB2
> cable or hub as an intermediary avoids the problem. For some reason,
> the Linux kernel (either on the same Tegra board, or on an x86 host)
> has no issue with the device, and I observe no disconnections during
> reset.
> 
> This change won't affect any USB device that already works, since such
> devices could not currently be triggering the error return this patch
> removes, or they wouldn't be working currently.
> 
> However, this patch is quite reliable in practice, hence I hope it's
> acceptable to solve the problem.
> 
> The only potential fallout I can see from this patch is:
> 
> * A broken device that triggers C_CONNECTION/!CONNECTION now causes the
>   loop in hub_port_reset() to run multiple times. If it never succeeds,
>   this will cause "usb start" to take roughly 1s extra to execute.
> 
> * If the user unplugs a device while hub_port_reset() is executing, and
>   very quickly swaps in a new device, hub_port_reset() might succeed on
>   the new device. This would mean that any information cached about the
>   original device (from the descriptor read in usb_new_device(), which
>   simply caches the max packet size) might be invalid, which would cause
>   problems talking to the new device. However, without this change, the
>   new device wouldn't work anyway, so this is probably not much of a
>   loss.
> 
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---

I'll apply this and see if we get any breakage. I don't expect any though.
diff mbox

Patch

diff --git a/common/usb_hub.c b/common/usb_hub.c
index 2add4b97920f..c416e5e0b317 100644
--- a/common/usb_hub.c
+++ b/common/usb_hub.c
@@ -209,9 +209,22 @@  int hub_port_reset(struct usb_device *dev, int port,
 		      (portstatus & USB_PORT_STAT_CONNECTION) ? 1 : 0,
 		      (portstatus & USB_PORT_STAT_ENABLE) ? 1 : 0);
 
-		if ((portchange & USB_PORT_STAT_C_CONNECTION) ||
-		    !(portstatus & USB_PORT_STAT_CONNECTION))
-			return -1;
+		/*
+		 * Perhaps we should check for the following here:
+		 * - C_CONNECTION hasn't been set.
+		 * - CONNECTION is still set.
+		 *
+		 * Doing so would ensure that the device is still connected
+		 * to the bus, and hasn't been unplugged or replaced while the
+		 * USB bus reset was going on.
+		 *
+		 * However, if we do that, then (at least) a San Disk Ultra
+		 * USB 3.0 16GB device fails to reset on (at least) an NVIDIA
+		 * Tegra Jetson TK1 board. For some reason, the device appears
+		 * to briefly drop off the bus when this second bus reset is
+		 * executed, yet if we retry this loop, it'll eventually come
+		 * back after another reset or two.
+		 */
 
 		if (portstatus & USB_PORT_STAT_ENABLE)
 			break;