On Fri, Nov 02, 2012 at 10:54:38AM -0700, Sarah Sharp wrote:
> On Fri, Nov 02, 2012 at 01:38:26PM -0400, Alan Stern wrote:
> > On Fri, 2 Nov 2012, Sarah Sharp wrote:
> >
> > > > I don't like that idea. It would be much simpler to put the port back
> > > > into a useful state. For example, disable the port, then put it into
> > > > RxDetect, then clear any change bits, and set the port's bit in
> > > > hub->removed_bits.
> > > >
> > > > (hub->removed_bits was meant to help implement Windows' "Safely remove
> > > > hardware" feature. When a port's bit is set, khubd will ignore any
> > > > device attached to that port until it sees a connect-change.)
Hi Alan,
I've been working on the warm reset fix patch, and I've run into a
couple questions. A draft patch is attached, but it's not in any way
done. :)
In hub_events, we check whether the hub port is in the Inactive or
Compliance Mode state, and issue a warm port reset. If there was a
device attached to that port, don't we need to call the driver's
prereset hook before we reset the device?
I'm wondering if we should just remove this bit of code in hub_events:
/* Warm reset a USB3 protocol port if it's in
- * SS.Inactive state.
+ * SS.Inactive state or Compliance Mode.
*/
if (hub_port_warm_reset_required(hub, portstatus)) {
- dev_dbg(hub_dev, "warm reset port %d\n", i);
- hub_port_reset(hub, i, NULL,
+ int status = hub_port_reset(hub, i,
+ hub->ports[i - 1]->child,
HUB_BH_RESET_TIME, true);
+ /* XXX: Not sure if we should set device state.
+ * Should this be passed a 0 or 1?
+ */
+ if (status)
+ hub_port_disable(hub, i, 1);
+ connect_change = 1;
}
and just let hub_port_connect_change() issue the warm port reset,
disconnect the device, etc.
Also, if the warm rest fails, I'm also not sure if I should ask
hub_port_disable to set the device state:
+ /* XXX: Not sure if we should set device state.
+ * Should this be passed a 0 or 1?
+ */
+ if (status)
+ hub_port_disable(hub, i, 1);
I added some test code to the xHCI hub code to test the patch out, and I
verified that when a port in Compliance Mode is detected, it will warm
reset the port several times, and then fall back to disabling and
re-enabling the port. A device connect after that port disable is
detected and the device is enumerated. So the patch works, it just
needs some cleanup/tweaking.
Sarah Sharp
>From 181a80f59f859a7b0f0b02e59ddaad63264e5eae Mon Sep 17 00:00:00 2001
From: Sarah Sharp <sarah.a.sharp@xxxxxxxxxxxxxxx>
Date: Thu, 1 Nov 2012 11:20:44 -0700
Subject: [PATCH] USB: Rework buggy warm reset support.
When John Covici boots with a USB 3.0 device attached to his 0.96 xHCI
host controller, the port goes into Compliance Mode during host
controller initialization. Warm reset fails, and the USB core goes into
an endless loop attempting to enumerate the device, because the warm
reset completion code assumes the reset will always be successful. Fix
this.
First, extend the port reset timeout, because the NEC xHCI host seems to
need a slightly longer timeout. The USB 3.0 spec says that the warm
port reset should complete within 200ms, or the link state should
transition to the Disconnected state. Two seconds should be more than
enough.
Next, clean up a bunch of bugs in the port reset code:
- A hot reset can migrate to a warm reset, so hub_finish_port_reset
needs to unconditionally clear the warm reset and link change bits.
- Rip out the recursive call to hub_port_reset in hub_wait_port_reset.
Instead, make hub_port_reset try a warm reset if the hot reset fails.
- If the port link state is SS.Inactive or Compliance Mode, make
hub_port_reset try several warm resets before giving up and reporting
the disconnect event.
- Make the USB core ignore the return value of the xHCI Reset Device
command. If we need to reset the port multiple times in a row, the
xHC will give us a context error starting with the second Reset
Device command, since it considers the device to already be reset.
When a warm port reset fails, make the USB core disable the port.
Currently, we avoid disabling any USB 3.0 ports because a disabled USB
3.0 port will not report any new USB connections. The USB core expects
a port status change event on a new connect (like with USB 2.0 hubs),
which will never come. We need to be able to disable a USB 3.0 port for
this case where a device is completely unresponsive during enumeration
and keeps connecting.
When the USB core wants to disable a USB 3.0 port, disable it, wait for
the port to settle into the disabled state, and then transition it into
the RxDetect state. This will cause new connect events to be reported.
We also set the port bit in the parent hub's removed_bits. This will
cause khubd to deal with any port status changes, but not act on a
connected status until a disconnect status and then a connect status is
reported.
This commit should be backported to kernels as old as 3.2, that contain
the commit 75d7cf72ab9fa01dc70877aa5c68e8ef477229dc "usbcore: refine
warm reset logic". This is a large patch, so please wait until 3.8 is
released before backporting this patch.
Signed-off-by: Sarah Sharp <sarah.a.sharp@xxxxxxxxxxxxxxx>
Reported-by: John Covici <covici@xxxxxxxxxxxxxx>
Cc: stable@xxxxxxxxxxxxxxx
---
drivers/usb/core/hub.c | 237 +++++++++++++++++++++++++++----------------
drivers/usb/host/xhci-hub.c | 22 ++++-
2 files changed, 169 insertions(+), 90 deletions(-)
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 1af04bd..67f1997 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -876,6 +876,61 @@ static int hub_hub_status(struct usb_hub *hub,
return ret;
}
+static int hub_set_port_link_state(struct usb_hub *hub, int port1,
+ unsigned int link_status)
+{
+ return set_port_feature(hub->hdev,
+ port1 | (link_status << 3),
+ USB_PORT_FEAT_LINK_STATE);
+}
+
+/*
+ * If USB 3.0 ports are placed into the Disabled state, they will no longer
+ * detect any device connects or disconnects. This is generally not what the
+ * USB core wants, since it expects a disabled port to produce a port status
+ * change event when a new device connects.
+ *
+ * Instead, set the link state to Disabled, wait for the link to settle into
+ * that state, and then put the port into the RxDetect state.
+ */
+static int hub_usb3_port_disable(struct usb_hub *hub, int port1)
+{
+ int ret;
+ int total_time;
+ u16 portchange, portstatus;
+
+ if (!hub_is_superspeed(hub->hdev))
+ return -EINVAL;
+
+ ret = hub_set_port_link_state(hub, port1, USB_SS_PORT_LS_SS_DISABLED);
+ if (ret) {
+ dev_err(hub->intfdev, "cannot disable port %d (err = %d)\n",
+ port1, ret);
+ return ret;
+ }
+
+ /* Wait for the link to enter the disabled state. */
+ for (total_time = 0; ; total_time += HUB_DEBOUNCE_STEP) {
+ ret = hub_port_status(hub, port1, &portstatus, &portchange);
+ if (ret < 0)
+ return ret;
+
+ if ((portstatus & USB_PORT_STAT_LINK_STATE) ==
+ USB_SS_PORT_LS_SS_DISABLED)
+ break;
+ if (total_time >= HUB_DEBOUNCE_TIMEOUT)
+ break;
+ msleep(HUB_DEBOUNCE_STEP);
+ }
+ if (total_time >= HUB_DEBOUNCE_TIMEOUT) {
+ dev_warn(hub->intfdev, "Could not disable port %d after %dms\n",
+ port1, total_time);
+ return -ETIMEDOUT;
+ }
+
+ return hub_set_port_link_state(hub, port1, USB_SS_PORT_LS_RX_DETECT);
+}
+
static int hub_port_disable(struct usb_hub *hub, int port1, int set_state)
{
struct usb_device *hdev = hub->hdev;
@@ -884,8 +939,23 @@ static int hub_port_disable(struct usb_hub *hub, int port1, int set_state)
if (hub->ports[port1 - 1]->child && set_state)
usb_set_device_state(hub->ports[port1 - 1]->child,
USB_STATE_NOTATTACHED);
- if (!hub->error && !hub_is_superspeed(hub->hdev))
- ret = clear_port_feature(hdev, port1, USB_PORT_FEAT_ENABLE);
+ if (!hub->error) {
+ /*
+ * A disabled SuperSpeed port will not report new device
+ * connects. Instead, we need to disable the port, and bring it
+ * back into the RxDetect state. Set its removed bit, so that
+ * the USB core will clear all port status changes, but not act
+ * on any connect events until it sees a disconnect and
+ * reconnect.
+ */
+ if (hub_is_superspeed(hub->hdev)) {
+ set_bit(port1, hub->removed_bits);
+ ret = hub_usb3_port_disable(hub, port1);
+ } else {
+ ret = clear_port_feature(hdev, port1,
+ USB_PORT_FEAT_ENABLE);
+ }
+ }
if (ret)
dev_err(hub->intfdev, "cannot disable port %d (err = %d)\n",
port1, ret);
@@ -2401,7 +2471,7 @@ static unsigned hub_is_wusb(struct usb_hub *hub)
#define HUB_SHORT_RESET_TIME 10
#define HUB_BH_RESET_TIME 50
#define HUB_LONG_RESET_TIME 200
-#define HUB_RESET_TIMEOUT 500
+#define HUB_RESET_TIMEOUT 2000
static int hub_port_reset(struct usb_hub *hub, int port1,
struct usb_device *udev, unsigned int delay, bool warm);
@@ -2436,71 +2506,33 @@ static int hub_port_wait_reset(struct usb_hub *hub, int port1,
if (ret < 0)
return ret;
- /*
- * Some buggy devices require a warm reset to be issued even
- * when the port appears not to be connected.
- */
- if (!warm) {
- /*
- * Some buggy devices can cause an NEC host controller
- * to transition to the "Error" state after a hot port
- * reset. This will show up as the port state in
- * "Inactive", and the port may also report a
- * disconnect. Forcing a warm port reset seems to make
- * the device work.
- *
- * See https://bugzilla.kernel.org/show_bug.cgi?id=41752
- */
- if (hub_port_warm_reset_required(hub, portstatus)) {
- int ret;
-
- 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);
- if (portchange & USB_PORT_STAT_C_RESET)
- clear_port_feature(hub->hdev, port1,
- USB_PORT_FEAT_C_RESET);
- dev_dbg(hub->intfdev, "hot reset failed, warm reset port %d\n",
- port1);
- ret = hub_port_reset(hub, port1,
- udev, HUB_BH_RESET_TIME,
- true);
- if ((portchange & USB_PORT_STAT_C_CONNECTION))
- clear_port_feature(hub->hdev, port1,
- USB_PORT_FEAT_C_CONNECTION);
- return ret;
- }
- /* Device went away? */
- if (!(portstatus & USB_PORT_STAT_CONNECTION))
- return -ENOTCONN;
+ /* 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;
+ /* 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)
+ /* if we've finished resetting, then break out of
+ * the loop
+ */
+ if (!(portstatus & USB_PORT_STAT_RESET) &&
+ (portstatus & USB_PORT_STAT_ENABLE)) {
+ if (!udev)
return 0;
+
+ 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 */
@@ -2516,41 +2548,37 @@ static int hub_port_wait_reset(struct usb_hub *hub, int port1,
}
static void hub_port_finish_reset(struct usb_hub *hub, int port1,
- struct usb_device *udev, int *status, bool warm)
+ struct usb_device *udev, int *status)
{
switch (*status) {
case 0:
- if (!warm) {
- struct usb_hcd *hcd;
- /* TRSTRCY = 10 ms; plus some extra */
- msleep(10 + 40);
+ /* TRSTRCY = 10 ms; plus some extra */
+ msleep(10 + 40);
+ if (udev) {
+ struct usb_hcd *hcd = bus_to_hcd(udev->bus);
+
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;
- }
- }
+ /* The xHC may think the device is already reset,
+ * so ignore the status.
+ */
+ if (hcd->driver->reset_device)
+ hcd->driver->reset_device(hcd, udev);
}
/* 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) {
+ if (hub_is_superspeed(hub->hdev)) {
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 {
+ }
+ if (udev)
usb_set_device_state(udev, *status
? USB_STATE_NOTATTACHED
: USB_STATE_DEFAULT);
- }
break;
}
}
@@ -2560,6 +2588,7 @@ static int hub_port_reset(struct usb_hub *hub, int port1,
struct usb_device *udev, unsigned int delay, bool warm)
{
int i, status;
+ u16 portchange, portstatus;
if (!warm) {
/* Block EHCI CF initialization during the port reset.
@@ -2592,10 +2621,34 @@ static int hub_port_reset(struct usb_hub *hub, int port1,
status);
}
- /* return on disconnect or reset */
+ /* Check for disconnect or reset */
if (status == 0 || status == -ENOTCONN || status == -ENODEV) {
- hub_port_finish_reset(hub, port1, udev, &status, warm);
- goto done;
+ hub_port_finish_reset(hub, port1, udev, &status);
+
+ if (!hub_is_superspeed(hub->hdev))
+ goto done;
+
+ /*
+ * USB 3.0 devices may be able to benefit from a warm
+ * reset when hot reset failed. Also, the link may have
+ * migrated to error states, and we need to handle that.
+ */
+ if (hub_port_status(hub, port1,
+ &portstatus, &portchange) < 0)
+ goto done;
+
+ if (!hub_port_warm_reset_required(hub, portstatus))
+ goto done;
+
+ /*
+ * If the port is in SS.Inactive or Compliance Mode, the
+ * hot or warm reset failed. Try another warm reset.
+ */
+ if (!warm) {
+ dev_dbg(hub->intfdev, "hot reset failed, warm reset port %d\n",
+ port1);
+ warm = true;
+ }
}
dev_dbg (hub->intfdev,
@@ -4569,12 +4622,18 @@ static void hub_events(void)
}
/* Warm reset a USB3 protocol port if it's in
- * SS.Inactive state.
+ * SS.Inactive state or Compliance Mode.
*/
if (hub_port_warm_reset_required(hub, portstatus)) {
- dev_dbg(hub_dev, "warm reset port %d\n", i);
- hub_port_reset(hub, i, NULL,
+ int status = hub_port_reset(hub, i,
+ hub->ports[i - 1]->child,
HUB_BH_RESET_TIME, true);
+ /* XXX: Not sure if we should set device state.
+ * Should this be passed a 0 or 1?
+ */
+ if (status)
+ hub_port_disable(hub, i, 1);
+ connect_change = 1;
}
if (connect_change)
diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index a686cf4..85315cb 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -761,12 +761,32 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
break;
case USB_PORT_FEAT_LINK_STATE:
temp = xhci_readl(xhci, port_array[wIndex]);
+
+ /* Disable port */
+ if (link_state == USB_SS_PORT_LS_SS_DISABLED) {
+ xhci_dbg(xhci, "Disable port %d\n", wIndex);
+ temp = xhci_port_state_to_neutral(temp);
+ xhci_writel(xhci, temp | PORT_PE,
+ port_array[wIndex]);
+ temp = xhci_readl(xhci, port_array[wIndex]);
+ break;
+ }
+
+ /* Put link in RxDetect (enable port) */
+ if (link_state == USB_SS_PORT_LS_RX_DETECT) {
+ xhci_dbg(xhci, "Enable port %d\n", wIndex);
+ xhci_set_link_state(xhci, port_array, wIndex,
+ link_state);
+ temp = xhci_readl(xhci, port_array[wIndex]);
+ break;
+ }
+
/* Software should not attempt to set
* port link state above '5' (Rx.Detect) and the port
* must be enabled.
*/
if ((temp & PORT_PE) == 0 ||
- (link_state > USB_SS_PORT_LS_RX_DETECT)) {
+ (link_state > USB_SS_PORT_LS_U3)) {
xhci_warn(xhci, "Cannot set link state.\n");
goto error;
}
--
1.7.9
