CHROMIUM: usb: xhci: Use non-interruptible sleep for XHCI co... [chromiumos/third_party/kernel : chromeos-3.4]

12 views
Skip to first unread message

Julius Werner (Code Review)

unread,
May 23, 2013, 10:39:53 PM5/23/13
to Pawel Osciak, Justin Chuang, Vincent Palatin, Olof Johansson
Hello Pawel Osciak, Justin Chuang, Vincent Palatin,

I'd like you to do a code review. Please visit

https://gerrit.chromium.org/gerrit/56555

to review the following change.

Change subject: CHROMIUM: usb: xhci: Use non-interruptible sleep for XHCI commands
......................................................................

CHROMIUM: usb: xhci: Use non-interruptible sleep for XHCI commands

The XHCI stack usually uses wait_for_completion_interruptible_timeout()
to wait for the completion of an XHCI command, and treats both timeouts
and interruptions as errors. This is a bad idea, since these commands
are often essential for the correct operation of the USB stack, and
their failure can leave devices in a misconfigured and/or unusable
state. While a timeout probably means a real problem that needs to be
dealt with, a random signal to the controlling user-space process should
not cause such harsh consequences.

This patch changes that behavior to use uninterruptible waits instead.
It fixes an easy to reproduce bug that occurs when you kill -9 a
process that reads from a UVC camera. The UVC driver's release code
tries to set the camera's alternate setting back to 0, but the lingering
SIGKILL instantly aborts any Stop Endpoint or Configure Endpoint command
sent to the xHC. The code dies halfway through the bandwidth allocation
process, leaving the device in an active configuration and preventing
future access to it due to the now out-of-sync bandwidth calculation.

This patch should be backported to kernels as far 2.6.31 that contain
the commit 3ffbba9511b4148cbe1f6b6238686adaeaca8feb "USB: xhci: Allocate
and address USB devices".

BUG=chrome-os-partner:19579
TEST=Do a cv2.VideoCapture(0).read() from python, then kill -9 the
python process. Make sure camera still works (on Spring or Stout).

Marking this CHROMIUM for now as it's needed to fix a factory test
problem, but I intend to upstream it immediately, too.

Change-Id: Idbef2ea30e20af854b73342a577c09757e7a542b
Signed-off-by: Julius Werner <jwe...@chromium.org>
---
M drivers/usb/host/xhci-hub.c
M drivers/usb/host/xhci.c
2 files changed, 14 insertions(+), 19 deletions(-)


git pull ssh://gerrit.chromium.org:29418/chromiumos/third_party/kernel refs/changes/55/56555/1

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index cc7e042..b2ad660 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -294,12 +294,11 @@
spin_unlock_irqrestore(&xhci->lock, flags);

/* Wait for last stop endpoint command to finish */
- timeleft = wait_for_completion_interruptible_timeout(
+ timeleft = wait_for_completion_timeout(
cmd->completion,
- USB_CTRL_SET_TIMEOUT);
+ XHCI_CMD_DEFAULT_TIMEOUT);
if (timeleft <= 0) {
- xhci_warn(xhci, "%s while waiting for stop endpoint command\n",
- timeleft == 0 ? "Timeout" : "Signal");
+ xhci_warn(xhci, "Timeout while waiting for Stop Endpoint\n");
spin_lock_irqsave(&xhci->lock, flags);
/* The timeout might have raced with the event ring handler, so
* only delete from the list if the item isn't poisoned.
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index d62b429..ae7f1c6 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -2474,15 +2474,14 @@
spin_unlock_irqrestore(&xhci->lock, flags);

/* Wait for the configure endpoint command to complete */
- timeleft = wait_for_completion_interruptible_timeout(
+ timeleft = wait_for_completion_timeout(
cmd_completion,
XHCI_CMD_DEFAULT_TIMEOUT);
if (timeleft <= 0) {
- xhci_warn(xhci, "%s while waiting for %s command\n",
- timeleft == 0 ? "Timeout" : "Signal",
+ xhci_warn(xhci, "Timeout while waiting for %s\n",
ctx_change == 0 ?
- "configure endpoint" :
- "evaluate context");
+ "Configure Endpoint" :
+ "Evaluate Context");
/* cancel the configure endpoint command */
ret = xhci_cancel_cmd(xhci, command, cmd_trb);
if (ret < 0)
@@ -3260,12 +3259,11 @@
spin_unlock_irqrestore(&xhci->lock, flags);

/* Wait for the Reset Device command to finish */
- timeleft = wait_for_completion_interruptible_timeout(
+ timeleft = wait_for_completion_timeout(
reset_device_cmd->completion,
- USB_CTRL_SET_TIMEOUT);
+ XHCI_CMD_DEFAULT_TIMEOUT);
if (timeleft <= 0) {
- xhci_warn(xhci, "%s while waiting for reset device command\n",
- timeleft == 0 ? "Timeout" : "Signal");
+ xhci_warn(xhci, "Timeout while waiting for Reset Device\n");
spin_lock_irqsave(&xhci->lock, flags);
/* The timeout might have raced with the event ring handler, so
* only delete from the list if the item isn't poisoned.
@@ -3449,11 +3447,10 @@
spin_unlock_irqrestore(&xhci->lock, flags);

/* XXX: how much time for xHC slot assignment? */
- timeleft = wait_for_completion_interruptible_timeout(&xhci->addr_dev,
+ timeleft = wait_for_completion_timeout(&xhci->addr_dev,
XHCI_CMD_DEFAULT_TIMEOUT);
if (timeleft <= 0) {
- xhci_warn(xhci, "%s while waiting for a slot\n",
- timeleft == 0 ? "Timeout" : "Signal");
+ xhci_warn(xhci, "Timeout while waiting for a slot\n");
/* cancel the enable slot request */
return xhci_cancel_cmd(xhci, NULL, cmd_trb);
}
@@ -3567,15 +3564,14 @@
spin_unlock_irqrestore(&xhci->lock, flags);

/* ctrl tx can take up to 5 sec; XXX: need more time for xHC? */
- timeleft = wait_for_completion_interruptible_timeout(&xhci->addr_dev,
+ timeleft = wait_for_completion_timeout(&xhci->addr_dev,
XHCI_CMD_DEFAULT_TIMEOUT);
/* FIXME: From section 4.3.4: "Software shall be responsible for timing
* the SetAddress() "recovery interval" required by USB and aborting the
* command on a timeout.
*/
if (timeleft <= 0) {
- xhci_warn(xhci, "%s while waiting for address device command\n",
- timeleft == 0 ? "Timeout" : "Signal");
+ xhci_warn(xhci, "Timeout while waiting for Address Device\n");
/* cancel the address device command */
ret = xhci_cancel_cmd(xhci, NULL, cmd_trb);
if (ret < 0)

--
To view, visit https://gerrit.chromium.org/gerrit/56555
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Idbef2ea30e20af854b73342a577c09757e7a542b
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.4
Gerrit-Owner: Julius Werner <jwe...@chromium.org>
Gerrit-Reviewer: Justin Chuang <jch...@chromium.org>
Gerrit-Reviewer: Pawel Osciak <pos...@chromium.org>
Gerrit-Reviewer: Vincent Palatin <vpal...@chromium.org>

Pawel Osciak (Code Review)

unread,
May 24, 2013, 12:07:08 PM5/24/13
to Julius Werner, ChromeBot, Vincent Palatin, Justin Chuang
Pawel Osciak has posted comments on this change.

Change subject: CHROMIUM: usb: xhci: Use non-interruptible sleep for XHCI commands
......................................................................


Patch Set 1: (1 inline comment)

....................................................
File drivers/usb/host/xhci-hub.c
Line 300
Is it feasible to somehow instead split this into timeleft == 0 and timeleft == -ERESTARTSYS to detect the signal and in that case put the device in a good state, possibly waiting a bit more, and return?
Although I'm suspecting that this putting the device in a good state would also require some waiting with a timeout?


--
To view, visit https://gerrit.chromium.org/gerrit/56555
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idbef2ea30e20af854b73342a577c09757e7a542b
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.4
Gerrit-Owner: Julius Werner <jwe...@chromium.org>
Gerrit-Reviewer: ChromeBot <chrom...@google.com>

Julius Werner (Code Review)

unread,
May 24, 2013, 1:04:27 PM5/24/13
to ChromeBot, Vincent Palatin, Pawel Osciak, Justin Chuang
Julius Werner has posted comments on this change.

Change subject: CHROMIUM: usb: xhci: Use non-interruptible sleep for XHCI commands
......................................................................


Patch Set 1: (1 inline comment)

....................................................
File drivers/usb/host/xhci-hub.c
Line 300
Exactly. The problem is that this is already trying to put the device back in the unconfigured default state. The only way to do that is through XHCI commands.

Also, this is kinda pointless. XHCI commands should never really take more than a millisecond unless the whole controller is broken (only Address Device can really time out under normal operation, and I think that is only run from khubd anyway). The USB stack already uses uninterruptible sleeps for device communication in many other places, too.


--
To view, visit https://gerrit.chromium.org/gerrit/56555
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idbef2ea30e20af854b73342a577c09757e7a542b
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.4
Gerrit-Owner: Julius Werner <jwe...@chromium.org>
Gerrit-Reviewer: ChromeBot <chrom...@google.com>
Gerrit-Reviewer: Julius Werner <jwe...@chromium.org>

Pawel Osciak (Code Review)

unread,
May 24, 2013, 1:17:26 PM5/24/13
to Julius Werner, ChromeBot, Vincent Palatin, Pawel Osciak, Justin Chuang
Pawel Osciak has posted comments on this change.

Change subject: CHROMIUM: usb: xhci: Use non-interruptible sleep for XHCI commands
......................................................................


Patch Set 1: Looks good to me, approved

(4 inline comments)

lgtm % nits

....................................................
File drivers/usb/host/xhci.c
Line 2481: xhci_warn(xhci, "Timeout while waiting for %s\n",
nit: s/%s/%s command/


Line 3266: xhci_warn(xhci, "Timeout while waiting for Reset Device\n");
nit: s/Reset Device/Reset Device command/


Line 3574: xhci_warn(xhci, "Timeout while waiting for Address Device\n");
nit: s/Address Device/Address Device command/


....................................................
File drivers/usb/host/xhci-hub.c
Line 300
Yeah, ok.

Btw nit: since wait_for_completion_timeout doesn't return <0, I wonder if you'd want to just

if (!wait_for_completion_timeout) {

Up to you.


--
To view, visit https://gerrit.chromium.org/gerrit/56555
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idbef2ea30e20af854b73342a577c09757e7a542b
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.4
Gerrit-Owner: Julius Werner <jwe...@chromium.org>
Gerrit-Reviewer: ChromeBot <chrom...@google.com>
Gerrit-Reviewer: Julius Werner <jwe...@chromium.org>
Gerrit-Reviewer: Justin Chuang <jch...@chromium.org>
Gerrit-Reviewer: Pawel Osciak <pos...@chromium.org>
Gerrit-Reviewer: Pawel Osciak <pos...@google.com>
Gerrit-Reviewer: Vincent Palatin <vpal...@chromium.org>

Julius Werner (Code Review)

unread,
May 24, 2013, 1:25:09 PM5/24/13
to ChromeBot, Vincent Palatin, Pawel Osciak, Pawel Osciak, Justin Chuang
Julius Werner has posted comments on this change.

Change subject: CHROMIUM: usb: xhci: Use non-interruptible sleep for XHCI commands
......................................................................


Patch Set 1: (2 inline comments)

....................................................
File drivers/usb/host/xhci.c
Line 2481: xhci_warn(xhci, "Timeout while waiting for %s\n",
These were intentional since it didn't fit the 80 char line anymore in one case. Means the same, really...


....................................................
File drivers/usb/host/xhci-hub.c
Line 300
Meh... I guess I usually prefer to keep undefined results within the "error" branch, just in case.


--
To view, visit https://gerrit.chromium.org/gerrit/56555
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idbef2ea30e20af854b73342a577c09757e7a542b
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.4
Gerrit-Owner: Julius Werner <jwe...@chromium.org>
Gerrit-Reviewer: ChromeBot <chrom...@google.com>
Gerrit-Reviewer: Julius Werner <jwe...@chromium.org>
Gerrit-Reviewer: Justin Chuang <jch...@chromium.org>
Gerrit-Reviewer: Pawel Osciak <pos...@chromium.org>

Julius Werner (Code Review)

unread,
May 24, 2013, 8:43:02 PM5/24/13
to ChromeBot, Vincent Palatin, Pawel Osciak, Pawel Osciak, Justin Chuang
Julius Werner has posted comments on this change.

Change subject: CHROMIUM: usb: xhci: Use non-interruptible sleep for XHCI commands
......................................................................


Patch Set 1: Ready; Verified

Throwing this in there since I think it's kinda urgent... Vincent, please let me know if you have additional comments before I upstream it.

--
To view, visit https://gerrit.chromium.org/gerrit/56555
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idbef2ea30e20af854b73342a577c09757e7a542b
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.4
Gerrit-Owner: Julius Werner <jwe...@chromium.org>
Gerrit-Reviewer: ChromeBot <chrom...@google.com>
Gerrit-Reviewer: Julius Werner <jwe...@chromium.org>
Gerrit-Reviewer: Justin Chuang <jch...@chromium.org>
Gerrit-Reviewer: Pawel Osciak <pos...@chromium.org>

Vincent Palatin (Code Review)

unread,
May 24, 2013, 8:51:20 PM5/24/13
to Julius Werner, ChromeBot, Vincent Palatin, Pawel Osciak, Pawel Osciak, Justin Chuang
Vincent Palatin has posted comments on this change.

Change subject: CHROMIUM: usb: xhci: Use non-interruptible sleep for XHCI commands
......................................................................


Patch Set 1: Looks good to me, approved

--
To view, visit https://gerrit.chromium.org/gerrit/56555
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idbef2ea30e20af854b73342a577c09757e7a542b
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.4
Gerrit-Owner: Julius Werner <jwe...@chromium.org>
Gerrit-Reviewer: ChromeBot <chrom...@google.com>
Gerrit-Reviewer: Julius Werner <jwe...@chromium.org>
Gerrit-Reviewer: Justin Chuang <jch...@chromium.org>
Gerrit-Reviewer: Pawel Osciak <pos...@chromium.org>

Justin Chuang (Code Review)

unread,
May 26, 2013, 1:55:01 PM5/26/13
to Julius Werner, ChromeBot, Vincent Palatin, Pawel Osciak, Pawel Osciak
Justin Chuang has posted comments on this change.

Change subject: CHROMIUM: usb: xhci: Use non-interruptible sleep for XHCI commands
......................................................................


Patch Set 2:

Julius, thanks for the patch. If you are confident of no side effects, please cherry-pick to factory-spring-4131.B branch.

However, it's okay not to cherrypick it into factory branch because now it doesn't happen in factory branch due to some workaround in Python level.

--
To view, visit https://gerrit.chromium.org/gerrit/56555
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idbef2ea30e20af854b73342a577c09757e7a542b
Gerrit-PatchSet: 2
Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.4
Gerrit-Owner: Julius Werner <jwe...@chromium.org>
Gerrit-Reviewer: ChromeBot <chrom...@google.com>
Gerrit-Reviewer: Julius Werner <jwe...@chromium.org>
Gerrit-Reviewer: Justin Chuang <jch...@chromium.org>
Gerrit-Reviewer: Pawel Osciak <pos...@chromium.org>
Reply all
Reply to author
Forward
0 new messages