Hi Harald,
It looks like the warm port reset took longer to complete than the USB
core expected. The xHCI driver tried to issue an address device command
while the reset was still going on, so it's no wonder it failed.
Can you try applying the attached patches? The first one extends the
time the USB core will wait for a warm reset, and the second one makes
the USB core issue a warm reset if a disconnect is reported with the
link in Inactive during a hot reset.
Sarah Sharp
On Thu, Sep 08, 2011 at 08:05:54AM -0700, Sarah Sharp wrote:
> (Switching to email)
>
> On Thu, Sep 08, 2011 at 12:04:06PM +0000, bugzilla-daemon@xxxxxxxxxxxxxxxxxxx wrote:
> > https://bugzilla.kernel.org/show_bug.cgi?id=42472
> >
> >
> >
> >
> >
> > --- Comment #2 from Harald Brennich <harald.brennich@xxxxxx> 2011-09-08 12:03:56 ---
> > Hello Sarah,
> > the Renesas (formerly NEC) uPD720200 USB 3.0 Host works fine with the following
> > change in function hub_port_wait_reset:
>
> Look, I really don't think this is the correct solution. If a device
> disconnects in the middle of a port reset, then we need to treat it as
> if it's a new device. Someone could have yanked out the device during
> the port reset and inserted a new one.
>
> Let me take a look at why the warm port reset patch didn't work and get
> back to you next week, after I'm back from a conference.
>
> Do other USB 3.0 devices work under this host controller? Is it just
> this device?
>
> Sarah Sharp
>
> > Current version:
> > static int hub_port_wait_reset(struct usb_hub *hub, int port1,
> > struct usb_device *udev, unsigned int delay)
> > {
> > int delay_time, ret;
> > u16 portstatus;
> > u16 portchange;
> >
> > for (delay_time = 0;
> > delay_time < HUB_RESET_TIMEOUT;
> > delay_time += delay) {
> > /* wait to give the device a chance to reset */
> > msleep(delay);
> >
> > /* read and decode port status */
> > ret = hub_port_status(hub, port1, &portstatus, &portchange);
> > if (ret < 0)
> > return ret;
> >
> > /* Device went away? */
> > if (!(portstatus & USB_PORT_STAT_CONNECTION))
> > return -ENOTCONN;
> >
> > /* bomb out completely if the connection bounced */
> > if ((portchange & USB_PORT_STAT_C_CONNECTION))
> > return -ENOTCONN;
> >
> > /* if we`ve finished resetting, then break out of the loop */
> > if (!(portstatus & USB_PORT_STAT_RESET) &&
> > (portstatus & USB_PORT_STAT_ENABLE)) {
> > if (hub_is_wusb(hub))
> > udev->speed = USB_SPEED_WIRELESS;
> > else if (hub_is_superspeed(hub->hdev))
> > udev->speed = USB_SPEED_SUPER;
> > else if (portstatus & USB_PORT_STAT_HIGH_SPEED)
> > udev->speed = USB_SPEED_HIGH;
> > else if (portstatus & USB_PORT_STAT_LOW_SPEED)
> > udev->speed = USB_SPEED_LOW;
> > else
> > udev->speed = USB_SPEED_FULL;
> > return 0;
> > }
> >
> > /* switch to the long delay after two short delay failures */
> > if (delay_time >= 2 * HUB_SHORT_RESET_TIME)
> > delay = HUB_LONG_RESET_TIME;
> >
> > dev_dbg (hub->intfdev,
> > "port %d not reset yet, waiting %dms\n",
> > port1, delay);
> > }
> > }
> >
> > Modified version:
> > In the first modification (see report 41752), the portchange was ignored. Then
> > the external drive could be used, but the disconnect was not detected. This is
> > now resolved with checking portstatus for USB_PORT_STAT_C_CONNECTION and
> > USB_PORT_FEAT_C_PORT_LINK_STATE (the flags set by the uPD720200).
> > Is there a part in the USB spec that disallows disconnecting a port by the hub
> > during RESET? Or do you have another solution?
> >
> > static int hub_port_wait_reset(struct usb_hub *hub, int port1,
> > struct usb_device *udev, unsigned int delay)
> > {
> > int delay_time, ret;
> > u16 portstatus;
> > u16 portchange;
> >
> > for (delay_time = 0;
> > delay_time < HUB_RESET_TIMEOUT;
> > delay_time += delay) {
> > /* wait to give the device a chance to reset */
> > msleep(delay);
> >
> > /* read and decode port status */
> > ret = hub_port_status(hub, port1, &portstatus, &portchange);
> > if (ret < 0)
> > return ret;
> >
> > /* if we`ve finished resetting, then break out of the loop */
> > if (!(portstatus & USB_PORT_STAT_RESET) &&
> > (portstatus & USB_PORT_STAT_ENABLE)) {
> > /* ADDED 2011.09.08 BRE BEGIN */
> > /* Device went away? */
> > if (!(portstatus & USB_PORT_STAT_CONNECTION))
> > return -ENOTCONN;
> > if (portchange & USB_PORT_STAT_C_CONNECTION)
> > clear_port_feature(hub->hdev, port1,
> > USB_PORT_FEAT_C_CONNECTION);
> > if (portchange & USB_PORT_STAT_C_LINK_STATE)
> > clear_port_feature(hub->hdev, port1,
> > USB_PORT_FEAT_C_PORT_LINK_STATE);
> > /* ADDED 2011.09.08 BRE END */
> > if (hub_is_wusb(hub))
> > udev->speed = USB_SPEED_WIRELESS;
> > else if (hub_is_superspeed(hub->hdev))
> > udev->speed = USB_SPEED_SUPER;
> > else if (portstatus & USB_PORT_STAT_HIGH_SPEED)
> > udev->speed = USB_SPEED_HIGH;
> > else if (portstatus & USB_PORT_STAT_LOW_SPEED)
> > udev->speed = USB_SPEED_LOW;
> > else
> > udev->speed = USB_SPEED_FULL;
> > return 0;
> > }
> >
> > /* switch to the long delay after two short delay failures */
> > if (delay_time >= 2 * HUB_SHORT_RESET_TIME)
> > delay = HUB_LONG_RESET_TIME;
> >
> > dev_dbg (hub->intfdev,
> > "port %d not reset yet, waiting %dms\n",
> > port1, delay);
> > }
> >
> > /* ADDED 2011.09.05 BRE BEGIN */
> > /* Device went away? */
> > if (!(portstatus & USB_PORT_STAT_CONNECTION))
> > return -ENOTCONN;
> > /* bomb out completely if the connection bounced */
> > if ((portchange & USB_PORT_STAT_C_CONNECTION))
> > return -ENOTCONN;
> > /* ADDED 2011.09.05 BRE END */
> > return -EBUSY;
> > }
> >
> > --
> > Configure bugmail: https://bugzilla.kernel.org/userprefs.cgi?tab=email
> > ------- You are receiving this mail because: -------
> > You are the assignee for the bug.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>From d21130dd5a1960e8a7cbd32f09905dc67a908786 Mon Sep 17 00:00:00 2001
From: Andiry Xu <andiry.xu@xxxxxxx>
Date: Tue, 30 Aug 2011 18:01:45 +0800
Subject: [PATCH 1/2] usbcore: refine warm reset logic
Current waiting time for warm(BH) reset in hub_port_warm_reset() is too short
for xHC host to complete the warm reset and report a BH reset change.
This patch increases the waiting time for warm reset and merges the function
into hub_port_reset(), so it can handle both cold reset and warm reset, and
factor out hub_port_finish_reset() to make the code looks cleaner.
This fixes the issue that driver fails to clear BH reset change and port is
"dead".
Signed-off-by: Andiry Xu <andiry.xu@xxxxxxx>
Acked-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
Signed-off-by: Sarah Sharp <sarah.a.sharp@xxxxxxxxxxxxxxx>
---
drivers/usb/core/hub.c | 216 +++++++++++++++++++++++++-----------------------
1 files changed, 112 insertions(+), 104 deletions(-)
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index a428aa0..96c079d 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -2030,11 +2030,12 @@ static unsigned hub_is_wusb(struct usb_hub *hub)
#define HUB_ROOT_RESET_TIME 50 /* times are in msec */
#define HUB_SHORT_RESET_TIME 10
+#define HUB_BH_RESET_TIME 50
#define HUB_LONG_RESET_TIME 200
#define HUB_RESET_TIMEOUT 500
static int hub_port_wait_reset(struct usb_hub *hub, int port1,
- struct usb_device *udev, unsigned int delay)
+ struct usb_device *udev, unsigned int delay, bool warm)
{
int delay_time, ret;
u16 portstatus;
@@ -2051,28 +2052,39 @@ static int hub_port_wait_reset(struct usb_hub *hub, int port1,
if (ret < 0)
return ret;
- /* Device went away? */
- if (!(portstatus & USB_PORT_STAT_CONNECTION))
- return -ENOTCONN;
-
- /* bomb out completely if the connection bounced */
- if ((portchange & USB_PORT_STAT_C_CONNECTION))
- return -ENOTCONN;
-
- /* if we`ve finished resetting, then break out of the loop */
- if (!(portstatus & USB_PORT_STAT_RESET) &&
- (portstatus & USB_PORT_STAT_ENABLE)) {
- if (hub_is_wusb(hub))
- udev->speed = USB_SPEED_WIRELESS;
- else if (hub_is_superspeed(hub->hdev))
- udev->speed = USB_SPEED_SUPER;
- else if (portstatus & USB_PORT_STAT_HIGH_SPEED)
- udev->speed = USB_SPEED_HIGH;
- else if (portstatus & USB_PORT_STAT_LOW_SPEED)
- udev->speed = USB_SPEED_LOW;
- else
- udev->speed = USB_SPEED_FULL;
- return 0;
+ /*
+ * Some buggy devices require a warm reset to be issued even
+ * when the port appears not to be connected.
+ */
+ if (!warm) {
+ /* Device went away? */
+ if (!(portstatus & USB_PORT_STAT_CONNECTION))
+ return -ENOTCONN;
+
+ /* bomb out completely if the connection bounced */
+ if ((portchange & USB_PORT_STAT_C_CONNECTION))
+ return -ENOTCONN;
+
+ /* if we`ve finished resetting, then break out of
+ * the loop
+ */
+ if (!(portstatus & USB_PORT_STAT_RESET) &&
+ (portstatus & USB_PORT_STAT_ENABLE)) {
+ if (hub_is_wusb(hub))
+ udev->speed = USB_SPEED_WIRELESS;
+ else if (hub_is_superspeed(hub->hdev))
+ udev->speed = USB_SPEED_SUPER;
+ else if (portstatus & USB_PORT_STAT_HIGH_SPEED)
+ udev->speed = USB_SPEED_HIGH;
+ else if (portstatus & USB_PORT_STAT_LOW_SPEED)
+ udev->speed = USB_SPEED_LOW;
+ else
+ udev->speed = USB_SPEED_FULL;
+ return 0;
+ }
+ } else {
+ if (portchange & USB_PORT_STAT_C_BH_RESET)
+ return 0;
}
/* switch to the long delay after two short delay failures */
@@ -2080,35 +2092,84 @@ static int hub_port_wait_reset(struct usb_hub *hub, int port1,
delay = HUB_LONG_RESET_TIME;
dev_dbg (hub->intfdev,
- "port %d not reset yet, waiting %dms\n",
- port1, delay);
+ "port %d not %sreset yet, waiting %dms\n",
+ port1, warm ? "warm " : "", delay);
}
return -EBUSY;
}
+static void hub_port_finish_reset(struct usb_hub *hub, int port1,
+ struct usb_device *udev, int *status, bool warm)
+{
+ switch (*status) {
+ case 0:
+ if (!warm) {
+ struct usb_hcd *hcd;
+ /* TRSTRCY = 10 ms; plus some extra */
+ msleep(10 + 40);
+ update_devnum(udev, 0);
+ hcd = bus_to_hcd(udev->bus);
+ if (hcd->driver->reset_device) {
+ *status = hcd->driver->reset_device(hcd, udev);
+ if (*status < 0) {
+ dev_err(&udev->dev, "Cannot reset "
+ "HCD device state\n");
+ break;
+ }
+ }
+ }
+ /* FALL THROUGH */
+ case -ENOTCONN:
+ case -ENODEV:
+ clear_port_feature(hub->hdev,
+ port1, USB_PORT_FEAT_C_RESET);
+ /* FIXME need disconnect() for NOTATTACHED device */
+ if (warm) {
+ clear_port_feature(hub->hdev, port1,
+ USB_PORT_FEAT_C_BH_PORT_RESET);
+ clear_port_feature(hub->hdev, port1,
+ USB_PORT_FEAT_C_PORT_LINK_STATE);
+ } else {
+ usb_set_device_state(udev, *status
+ ? USB_STATE_NOTATTACHED
+ : USB_STATE_DEFAULT);
+ }
+ break;
+ }
+}
+
+/* Handle port reset and port warm(BH) reset (for USB3 protocol ports) */
static int hub_port_reset(struct usb_hub *hub, int port1,
- struct usb_device *udev, unsigned int delay)
+ struct usb_device *udev, unsigned int delay, bool warm)
{
int i, status;
- struct usb_hcd *hcd;
- hcd = bus_to_hcd(udev->bus);
- /* Block EHCI CF initialization during the port reset.
- * Some companion controllers don't like it when they mix.
- */
- down_read(&ehci_cf_port_reset_rwsem);
+ if (!warm) {
+ /* Block EHCI CF initialization during the port reset.
+ * Some companion controllers don't like it when they mix.
+ */
+ down_read(&ehci_cf_port_reset_rwsem);
+ } else {
+ if (!hub_is_superspeed(hub->hdev)) {
+ dev_err(hub->intfdev, "only USB3 hub support "
+ "warm reset\n");
+ return -EINVAL;
+ }
+ }
/* Reset the port */
for (i = 0; i < PORT_RESET_TRIES; i++) {
- status = set_port_feature(hub->hdev,
- port1, USB_PORT_FEAT_RESET);
- if (status)
+ status = set_port_feature(hub->hdev, port1, (warm ?
+ USB_PORT_FEAT_BH_PORT_RESET :
+ USB_PORT_FEAT_RESET));
+ if (status) {
dev_err(hub->intfdev,
- "cannot reset port %d (err = %d)\n",
- port1, status);
- else {
- status = hub_port_wait_reset(hub, port1, udev, delay);
+ "cannot %sreset port %d (err = %d)\n",
+ warm ? "warm " : "", port1, status);
+ } else {
+ status = hub_port_wait_reset(hub, port1, udev, delay,
+ warm);
if (status && status != -ENOTCONN)
dev_dbg(hub->intfdev,
"port_wait_reset: err = %d\n",
@@ -2116,34 +2177,14 @@ static int hub_port_reset(struct usb_hub *hub, int port1,
}
/* return on disconnect or reset */
- switch (status) {
- case 0:
- /* TRSTRCY = 10 ms; plus some extra */
- msleep(10 + 40);
- update_devnum(udev, 0);
- if (hcd->driver->reset_device) {
- status = hcd->driver->reset_device(hcd, udev);
- if (status < 0) {
- dev_err(&udev->dev, "Cannot reset "
- "HCD device state\n");
- break;
- }
- }
- /* FALL THROUGH */
- case -ENOTCONN:
- case -ENODEV:
- clear_port_feature(hub->hdev,
- port1, USB_PORT_FEAT_C_RESET);
- /* FIXME need disconnect() for NOTATTACHED device */
- usb_set_device_state(udev, status
- ? USB_STATE_NOTATTACHED
- : USB_STATE_DEFAULT);
+ if (status == 0 || status == -ENOTCONN || status == -ENODEV) {
+ hub_port_finish_reset(hub, port1, udev, &status, warm);
goto done;
}
dev_dbg (hub->intfdev,
- "port %d not enabled, trying reset again...\n",
- port1);
+ "port %d not enabled, trying %sreset again...\n",
+ port1, warm ? "warm " : "");
delay = HUB_LONG_RESET_TIME;
}
@@ -2151,45 +2192,11 @@ static int hub_port_reset(struct usb_hub *hub, int port1,
"Cannot enable port %i. Maybe the USB cable is bad?\n",
port1);
- done:
- up_read(&ehci_cf_port_reset_rwsem);
- return status;
-}
-
-/* Warm reset a USB3 protocol port */
-static int hub_port_warm_reset(struct usb_hub *hub, int port)
-{
- int ret;
- u16 portstatus, portchange;
-
- if (!hub_is_superspeed(hub->hdev)) {
- dev_err(hub->intfdev, "only USB3 hub support warm reset\n");
- return -EINVAL;
- }
-
- /* Warm reset the port */
- ret = set_port_feature(hub->hdev,
- port, USB_PORT_FEAT_BH_PORT_RESET);
- if (ret) {
- dev_err(hub->intfdev, "cannot warm reset port %d\n", port);
- return ret;
- }
-
- msleep(20);
- ret = hub_port_status(hub, port, &portstatus, &portchange);
-
- if (portchange & USB_PORT_STAT_C_RESET)
- clear_port_feature(hub->hdev, port, USB_PORT_FEAT_C_RESET);
-
- if (portchange & USB_PORT_STAT_C_BH_RESET)
- clear_port_feature(hub->hdev, port,
- USB_PORT_FEAT_C_BH_PORT_RESET);
-
- if (portchange & USB_PORT_STAT_C_LINK_STATE)
- clear_port_feature(hub->hdev, port,
- USB_PORT_FEAT_C_PORT_LINK_STATE);
+done:
+ if (!warm)
+ up_read(&ehci_cf_port_reset_rwsem);
- return ret;
+ return status;
}
/* Check if a port is power on */
@@ -2819,7 +2826,7 @@ hub_port_init (struct usb_hub *hub, struct usb_device *udev, int port1,
/* Reset the device; full speed may morph to high speed */
/* FIXME a USB 2.0 device may morph into SuperSpeed on reset. */
- retval = hub_port_reset(hub, port1, udev, delay);
+ retval = hub_port_reset(hub, port1, udev, delay, false);
if (retval < 0) /* error or disconnect */
goto fail;
/* success, speed is known */
@@ -2949,7 +2956,7 @@ hub_port_init (struct usb_hub *hub, struct usb_device *udev, int port1,
buf->bMaxPacketSize0;
kfree(buf);
- retval = hub_port_reset(hub, port1, udev, delay);
+ retval = hub_port_reset(hub, port1, udev, delay, false);
if (retval < 0) /* error or disconnect */
goto fail;
if (oldspeed != udev->speed) {
@@ -3570,7 +3577,8 @@ static void hub_events(void)
(portstatus & USB_PORT_STAT_LINK_STATE)
== USB_SS_PORT_LS_SS_INACTIVE) {
dev_dbg(hub_dev, "warm reset port %d\n", i);
- hub_port_warm_reset(hub, i);
+ hub_port_reset(hub, i, NULL,
+ HUB_BH_RESET_TIME, true);
}
if (connect_change)
--
1.7.4.1
>From e7572a392574ef401e13d7cbc9a5d90fce2ecc48 Mon Sep 17 00:00:00 2001
From: Sarah Sharp <sarah.a.sharp@xxxxxxxxxxxxxxx>
Date: Tue, 6 Sep 2011 09:53:01 -0700
Subject: [PATCH 2/2] USB: When hot reset for USB3 fails, try warm reset.
When a hot reset (standard USB port reset) fails on a USB 3.0 port, the
host controller is supposed to automatically try a warm port reset.
During this transition, the device will be reported as disconnected, and
the link state will be reported as "Inactive". If we find a device on a
USB 3.0 hub is disconnected in hub_port_wait_reset (which does the hot
reset), then we should wait for the warm port reset to complete if the
link state is inactive.
This may fix https://bugzilla.kernel.org/show_bug.cgi?id=41752
Signed-off-by: Sarah Sharp <sarah.a.sharp@xxxxxxxxxxxxxxx>
Cc: Harald Brennich <harald.brennich@xxxxxx>
---
drivers/usb/core/hub.c | 27 ++++++++++++++++++++++++++-
1 files changed, 26 insertions(+), 1 deletions(-)
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 96c079d..f9c060b 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -2034,6 +2034,17 @@ static unsigned hub_is_wusb(struct usb_hub *hub)
#define HUB_LONG_RESET_TIME 200
#define HUB_RESET_TIMEOUT 500
+static int hub_port_reset(struct usb_hub *hub, int port1,
+ struct usb_device *udev, unsigned int delay, bool warm);
+
+/* Is a USB 3.0 port in the Inactive state? */
+static bool hub_port_inactive(struct usb_hub *hub, u16 portstatus)
+{
+ return hub_is_superspeed(hub->hdev) &&
+ (portstatus & USB_PORT_STAT_LINK_STATE) ==
+ USB_SS_PORT_LS_SS_INACTIVE;
+}
+
static int hub_port_wait_reset(struct usb_hub *hub, int port1,
struct usb_device *udev, unsigned int delay, bool warm)
{
@@ -2058,8 +2069,22 @@ static int hub_port_wait_reset(struct usb_hub *hub, int port1,
*/
if (!warm) {
/* Device went away? */
- if (!(portstatus & USB_PORT_STAT_CONNECTION))
+ /*
+ * Some USB 3.0 host controllers do not automatically
+ * issue a warm port reset if hot reset fails. If we
+ * find the device disconnected but the port link state
+ * is "Inactive" we need to issue a warm port reset.
+ */
+ if (!(portstatus & USB_PORT_STAT_CONNECTION)) {
+ if (hub_port_inactive(hub, portstatus)) {
+ dev_dbg(hub->intfdev, "warm reset port %d\n",
+ port1);
+ return hub_port_reset(hub, port1,
+ udev, HUB_BH_RESET_TIME,
+ true);
+ }
return -ENOTCONN;
+ }
/* bomb out completely if the connection bounced */
if ((portchange & USB_PORT_STAT_C_CONNECTION))
--
1.7.4.1
