Re: [RFC v2] usb: Fix xHCI host issues on remote wakeup.
On Wed, Aug 28, 2013 at 10:26:01AM -0700, Greg KH wrote: > On Wed, Aug 28, 2013 at 08:27:48AM -0700, Sarah Sharp wrote: > > On Tue, Aug 27, 2013 at 05:41:19PM -0700, Greg KH wrote: > > > On Mon, Aug 26, 2013 at 06:59:22PM -0700, Sarah Sharp wrote: > > > > This patch should be backported to kernels as old as 2.6.37, that > > > > contain the commit 561925318725a41189a69f36ebe99199b3fb84c4 "USB: xHCI: > > > > port remote wakeup implementation". It may be difficult to backport > > > > this due to xhci->bus_state being unavailable in older kernels, and the > > > > code to split xHCI roothubs into a USB 2.0 roothub and a USB 3.0 roothub > > > > not going in until 2.6.39. > > > > > > If only ChromeOS is affected, why backport it for any stable tree? > > > > I don't understand this logic. > > > > The xHCI driver is violating the xHCI spec by not waiting for the port > > status change to indicate the port is actually in U0 (active state) > > before telling the USB core it's safe to communicate with the port. > > Which OS happens to trigger this bug is not important in my mind, since > > it's still a bug in the driver. > > > > ChromeOS can trigger this bug, whether it's running on the 3.8.11 > > kernel, or the 3.10.7 kernel. Therefore I would think we would want > > this fix in stable kernels, case some other distribution happened to hit > > the combination of sysfs writes that causes ChromeOS to trigger this > > bug. > > > > Or are you arguing that we shouldn't backport this because it's just > > triggered on one distribution? Am I required to verify all bugs are > > triggered on two distributions before they should be considered for > > stable? > > No, my point was if no one can see the bug, then it's not a bug that > needs to be fixed in a stable kernel (see > Documentation/stable_kernel_rules.txt for details.) As no one else has > run into this before now, and no older kernels have shown the problem to > anyone, it's not all that "important" to backport this type of thing, > right? > > I'm not saying it's not good to push it to older kernels, just the need > is quite low, right? Ok, yes, I see your logic now. Backporting this patch wouldn't be high priority for distros that don't trigger this bug. So yes, the need for it to get backported is low. I'll remove the stable tagging. Sarah Sharp -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v2] usb: Fix xHCI host issues on remote wakeup.
On Wed, Aug 28, 2013 at 08:27:48AM -0700, Sarah Sharp wrote: > On Tue, Aug 27, 2013 at 05:41:19PM -0700, Greg KH wrote: > > On Mon, Aug 26, 2013 at 06:59:22PM -0700, Sarah Sharp wrote: > > > I attempted to replicate this bug with Ubuntu 12.04, but could not. I > > > used Ubuntu 12.04 on the same platform, with the same BIOS that the bug > > > was triggered on ChromeOS with. I also changed the USB sysfs settings > > > as described above, but still could not reproduce the bug under Ubuntu. > > > It may be that ChromeOS userspace triggers this bug through additional > > > settings. > > > > Did you test this on ChromeOS to determine that the patch fixes the bug? > > Yes, it does fix the bug. Great, I didn't get that from the changelog entry, sorry. > > > This patch should be backported to kernels as old as 2.6.37, that > > > contain the commit 561925318725a41189a69f36ebe99199b3fb84c4 "USB: xHCI: > > > port remote wakeup implementation". It may be difficult to backport > > > this due to xhci->bus_state being unavailable in older kernels, and the > > > code to split xHCI roothubs into a USB 2.0 roothub and a USB 3.0 roothub > > > not going in until 2.6.39. > > > > If only ChromeOS is affected, why backport it for any stable tree? > > I don't understand this logic. > > The xHCI driver is violating the xHCI spec by not waiting for the port > status change to indicate the port is actually in U0 (active state) > before telling the USB core it's safe to communicate with the port. > Which OS happens to trigger this bug is not important in my mind, since > it's still a bug in the driver. > > ChromeOS can trigger this bug, whether it's running on the 3.8.11 > kernel, or the 3.10.7 kernel. Therefore I would think we would want > this fix in stable kernels, case some other distribution happened to hit > the combination of sysfs writes that causes ChromeOS to trigger this > bug. > > Or are you arguing that we shouldn't backport this because it's just > triggered on one distribution? Am I required to verify all bugs are > triggered on two distributions before they should be considered for > stable? No, my point was if no one can see the bug, then it's not a bug that needs to be fixed in a stable kernel (see Documentation/stable_kernel_rules.txt for details.) As no one else has run into this before now, and no older kernels have shown the problem to anyone, it's not all that "important" to backport this type of thing, right? I'm not saying it's not good to push it to older kernels, just the need is quite low, right? thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v2] usb: Fix xHCI host issues on remote wakeup.
On Tue, Aug 27, 2013 at 05:41:19PM -0700, Greg KH wrote: > On Mon, Aug 26, 2013 at 06:59:22PM -0700, Sarah Sharp wrote: > > I attempted to replicate this bug with Ubuntu 12.04, but could not. I > > used Ubuntu 12.04 on the same platform, with the same BIOS that the bug > > was triggered on ChromeOS with. I also changed the USB sysfs settings > > as described above, but still could not reproduce the bug under Ubuntu. > > It may be that ChromeOS userspace triggers this bug through additional > > settings. > > Did you test this on ChromeOS to determine that the patch fixes the bug? Yes, it does fix the bug. > > This patch should be backported to kernels as old as 2.6.37, that > > contain the commit 561925318725a41189a69f36ebe99199b3fb84c4 "USB: xHCI: > > port remote wakeup implementation". It may be difficult to backport > > this due to xhci->bus_state being unavailable in older kernels, and the > > code to split xHCI roothubs into a USB 2.0 roothub and a USB 3.0 roothub > > not going in until 2.6.39. > > If only ChromeOS is affected, why backport it for any stable tree? I don't understand this logic. The xHCI driver is violating the xHCI spec by not waiting for the port status change to indicate the port is actually in U0 (active state) before telling the USB core it's safe to communicate with the port. Which OS happens to trigger this bug is not important in my mind, since it's still a bug in the driver. ChromeOS can trigger this bug, whether it's running on the 3.8.11 kernel, or the 3.10.7 kernel. Therefore I would think we would want this fix in stable kernels, case some other distribution happened to hit the combination of sysfs writes that causes ChromeOS to trigger this bug. Or are you arguing that we shouldn't backport this because it's just triggered on one distribution? Am I required to verify all bugs are triggered on two distributions before they should be considered for stable? Confused, Sarah Sharp -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v2] usb: Fix xHCI host issues on remote wakeup.
On Mon, Aug 26, 2013 at 06:59:22PM -0700, Sarah Sharp wrote: > When a device signals remote wakeup on a roothub, and the suspend change > bit is set, the host controller driver must not give control back to the > USB core until the port goes back into the active state. > > EHCI accomplishes this by waiting in the get port status function until > the PORT_RESUME bit is cleared: > > /* stop resume signaling */ > temp &= ~(PORT_RWC_BITS | PORT_SUSPEND | PORT_RESUME); > ehci_writel(ehci, temp, status_reg); > clear_bit(wIndex, &ehci->resuming_ports); > retval = ehci_handshake(ehci, status_reg, > PORT_RESUME, 0, 2000 /* 2msec */); > > Similarly, the xHCI host should wait until the port goes into U0, before > passing control up to the USB core. When the port transitions from the > RExit state to U0, the xHCI driver will get a port status change event. > We need to wait for that event before passing control up to the USB > core. > > After the port transitions to the active state, the USB core should time > a recovery interval before it talks to the device. The length of that > recovery interval is TRSMRCY, 10 ms, mentioned in the USB 2.0 spec, > section 7.1.7.7. The previous xHCI code (which did not wait for the > port to go into U0) would cause the USB core to violate that recovery > interval. > > This bug caused numerous USB device disconnects on remote wakeup under > ChromeOS and a Lynx Point LP xHCI host that takes up to 20 ms to move > from RExit to U0. ChromeOS is very aggressive about power savings, and > sets the autosuspend_delay to 100 ms, and disables USB persist. > > I attempted to replicate this bug with Ubuntu 12.04, but could not. I > used Ubuntu 12.04 on the same platform, with the same BIOS that the bug > was triggered on ChromeOS with. I also changed the USB sysfs settings > as described above, but still could not reproduce the bug under Ubuntu. > It may be that ChromeOS userspace triggers this bug through additional > settings. Did you test this on ChromeOS to determine that the patch fixes the bug? > This patch should be backported to kernels as old as 2.6.37, that > contain the commit 561925318725a41189a69f36ebe99199b3fb84c4 "USB: xHCI: > port remote wakeup implementation". It may be difficult to backport > this due to xhci->bus_state being unavailable in older kernels, and the > code to split xHCI roothubs into a USB 2.0 roothub and a USB 3.0 roothub > not going in until 2.6.39. If only ChromeOS is affected, why backport it for any stable tree? thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC v2] usb: Fix xHCI host issues on remote wakeup.
When a device signals remote wakeup on a roothub, and the suspend change
bit is set, the host controller driver must not give control back to the
USB core until the port goes back into the active state.
EHCI accomplishes this by waiting in the get port status function until
the PORT_RESUME bit is cleared:
/* stop resume signaling */
temp &= ~(PORT_RWC_BITS | PORT_SUSPEND | PORT_RESUME);
ehci_writel(ehci, temp, status_reg);
clear_bit(wIndex, &ehci->resuming_ports);
retval = ehci_handshake(ehci, status_reg,
PORT_RESUME, 0, 2000 /* 2msec */);
Similarly, the xHCI host should wait until the port goes into U0, before
passing control up to the USB core. When the port transitions from the
RExit state to U0, the xHCI driver will get a port status change event.
We need to wait for that event before passing control up to the USB
core.
After the port transitions to the active state, the USB core should time
a recovery interval before it talks to the device. The length of that
recovery interval is TRSMRCY, 10 ms, mentioned in the USB 2.0 spec,
section 7.1.7.7. The previous xHCI code (which did not wait for the
port to go into U0) would cause the USB core to violate that recovery
interval.
This bug caused numerous USB device disconnects on remote wakeup under
ChromeOS and a Lynx Point LP xHCI host that takes up to 20 ms to move
from RExit to U0. ChromeOS is very aggressive about power savings, and
sets the autosuspend_delay to 100 ms, and disables USB persist.
I attempted to replicate this bug with Ubuntu 12.04, but could not. I
used Ubuntu 12.04 on the same platform, with the same BIOS that the bug
was triggered on ChromeOS with. I also changed the USB sysfs settings
as described above, but still could not reproduce the bug under Ubuntu.
It may be that ChromeOS userspace triggers this bug through additional
settings.
This patch should be backported to kernels as old as 2.6.37, that
contain the commit 561925318725a41189a69f36ebe99199b3fb84c4 "USB: xHCI:
port remote wakeup implementation". It may be difficult to backport
this due to xhci->bus_state being unavailable in older kernels, and the
code to split xHCI roothubs into a USB 2.0 roothub and a USB 3.0 roothub
not going in until 2.6.39.
Signed-off-by: Sarah Sharp
Cc: sta...@vger.kernel.org
---
drivers/usb/host/xhci-hub.c | 37 +
drivers/usb/host/xhci-mem.c | 2 ++
drivers/usb/host/xhci-ring.c | 13 +
drivers/usb/host/xhci.h | 10 ++
4 files changed, 54 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 187a3ec..b2fb67d 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -634,21 +634,42 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq,
u16 wValue,
goto error;
if (time_after_eq(jiffies,
bus_state->resume_done[wIndex])) {
+ int time_left;
+
xhci_dbg(xhci, "Resume USB2 port %d\n",
wIndex + 1);
bus_state->resume_done[wIndex] = 0;
clear_bit(wIndex, &bus_state->resuming_ports);
+
+ set_bit(wIndex, &bus_state->rexit_ports);
xhci_set_link_state(xhci, port_array, wIndex,
XDEV_U0);
- xhci_dbg(xhci, "set port %d resume\n",
- wIndex + 1);
- slot_id = xhci_find_slot_id_by_port(hcd, xhci,
-wIndex + 1);
- if (!slot_id) {
- xhci_dbg(xhci, "slot_id is zero\n");
- goto error;
+
+ spin_unlock_irqrestore(&xhci->lock, flags);
+ time_left = wait_for_completion_timeout(
+ &bus_state->rexit_done[wIndex],
+ msecs_to_jiffies(
+
XHCI_MAX_REXIT_TIMEOUT));
+ spin_lock_irqsave(&xhci->lock, flags);
+
+ if (time_left) {
+ slot_id = xhci_find_slot_id_by_port(hcd,
+ xhci, wIndex + 1);
+ if (!slot_id) {
+ xhci_dbg(xhci, "slot_id is
zero\n");
+ goto error;
+
