usb: xhci: Fix Command Ring Stopped Event handling [chromiumos/third_party/kernel : chromeos-3.4]

[Julius Werner (Code Review)]

Hello Vincent Palatin,

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

https://gerrit.chromium.org/gerrit/56557

to review the following change.

Change subject: usb: xhci: Fix Command Ring Stopped Event handling
......................................................................

usb: xhci: Fix Command Ring Stopped Event handling

The current XHCI code treats a command completion event with the
COMP_CMD_STOP code as a slightly different version of COMP_CMD_ABORT. In
particular, it puts the pointed-to command TRB through the normal
command completion handlers. This is not how this event works.

As XHCI spec 4.6.1.1 describes, unlike other command completion events
Command Ring Stopped sets the Command TRB Pointer to the *current*
Command Ring Dequeue Pointer. This is the command TRB that the XHCI will
execute next, and not a command that has already been executed. The
driver's internal command ring dequeue pointer should not be increased
after this event, since it does not really mark a command completion...
it's just a hint for the driver that execution is stopped now and where
it will be picked up again if the ring gets restarted.

This patch gets rid of the handle_stopped_command_ring() function and
splits its behavior into two distinct parts for COMP_CMD_STOP and
COMP_CMD_ABORT events. It ensures that the former no longer messes with
the dequeue pointer, while the latter's behavior is unchanged. This
prevents the hardware and software dequeue pointer from going out of
sync during command cancellations, which can lead to a variety of
problems (up to a total HCD death if the next command after the one that
was cancelled is Stop Endpoint, and the stop_endpoint_watchdog won't see
the command's completion because it got skipped by the software dequeue
pointer).

This patch should be backported to kernels as far as 3.0 that contain
the commit b63f4053cc8aa22a98e3f9a97845afe6c15d0a0d "xHCI: handle
command after aborting the command ring".

BUG=chrome-os-partner:19579
TEST=Manual

The fix in the other CL for this bug prevents situations like this from
happening (at least those that I know of), so while it's still nice to
fix it's not urgent anymore and I can wait until it goes in upstream.

Change-Id: Ia30e6fa7fd24908fbb8f48dd9f2d6fa3f6db4a75
Signed-off-by: Julius Werner <jwe...@chromium.org>
---
M drivers/usb/host/xhci-ring.c
1 file changed, 33 insertions(+), 52 deletions(-)


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

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index d29de99..b4d3931 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1314,47 +1314,11 @@
return 0;
}

-/*
- * If the cmd_trb_comp_code is COMP_CMD_ABORT, we just check whether the
- * trb pointed by the command ring dequeue pointer is the trb we want to
- * cancel or not. And if the cmd_trb_comp_code is COMP_CMD_STOP, we will
- * traverse the cancel_cmd_list to trun the all of the commands according
- * to command descriptor to NO-OP trb.
- */
-static int handle_stopped_cmd_ring(struct xhci_hcd *xhci,
- int cmd_trb_comp_code)
-{
- int cur_trb_is_good = 0;
-
- /* Searching the cmd trb pointed by the command ring dequeue
- * pointer in command descriptor list. If it is found, free it.
- */
- cur_trb_is_good = xhci_search_cmd_trb_in_cd_list(xhci,
- xhci->cmd_ring->dequeue);
-
- if (cmd_trb_comp_code == COMP_CMD_ABORT)
- xhci->cmd_ring_state = CMD_RING_STATE_STOPPED;
- else if (cmd_trb_comp_code == COMP_CMD_STOP) {
- /* traversing the cancel_cmd_list and canceling
- * the command according to command descriptor
- */
- xhci_cancel_cmd_in_cd_list(xhci);
-
- xhci->cmd_ring_state = CMD_RING_STATE_RUNNING;
- /*
- * ring command ring doorbell again to restart the
- * command ring
- */
- if (xhci->cmd_ring->dequeue != xhci->cmd_ring->enqueue)
- xhci_ring_cmd_db(xhci);
- }
- return cur_trb_is_good;
-}
-
static void handle_cmd_completion(struct xhci_hcd *xhci,
struct xhci_event_cmd *event)
{
int slot_id = TRB_TO_SLOT_ID(le32_to_cpu(event->flags));
+ int comp_code = GET_COMP_CODE(le32_to_cpu(event->status));
u64 cmd_dma;
dma_addr_t cmd_dequeue_dma;
struct xhci_input_control_ctx *ctrl_ctx;
@@ -1377,16 +1341,34 @@
return;
}

- if ((GET_COMP_CODE(le32_to_cpu(event->status)) == COMP_CMD_ABORT) ||
- (GET_COMP_CODE(le32_to_cpu(event->status)) == COMP_CMD_STOP)) {
- /* If the return value is 0, we think the trb pointed by
- * command ring dequeue pointer is a good trb. The good
- * trb means we don't want to cancel the trb, but it have
- * been stopped by host. So we should handle it normally.
- * Otherwise, driver should invoke inc_deq() and return.
- */
- if (handle_stopped_cmd_ring(xhci,
- GET_COMP_CODE(le32_to_cpu(event->status)))) {
+ /*
+ * Command Ring Stopped events point at the xHC's *current* dequeue
+ * pointer, i.e. the next command that will be executed. That TRB may
+ * or may not have been issued yet. Just overwrite all canceled commands
+ * with NOOPs and restart the ring, leaving our internal dequeue pointer
+ * as it is (we will get another event for that position later, when
+ * it has actually been executed).
+ */
+ if (comp_code == COMP_CMD_STOP) {
+ xhci_cancel_cmd_in_cd_list(xhci);
+ xhci->cmd_ring_state = CMD_RING_STATE_RUNNING;
+ if (xhci->cmd_ring->dequeue != xhci->cmd_ring->enqueue)
+ xhci_ring_cmd_db(xhci);
+ return;
+ }
+
+ /*
+ * If we aborted a command, we check if it is one of the commands we
+ * meant to cancel. In that case, it will be freed and we just finish
+ * up right here. If we aborted something else instead, we run it
+ * through the normal handlers below. At any rate, the command ring is
+ * stopped now, but the xHC will issue a Command Ring Stopped event
+ * after this that will cause us to restart it.
+ */
+ if (comp_code == COMP_CMD_ABORT) {
+ xhci->cmd_ring_state = CMD_RING_STATE_STOPPED;
+ if (xhci_search_cmd_trb_in_cd_list(xhci,
+ xhci->cmd_ring->dequeue)) {
inc_deq(xhci, xhci->cmd_ring);
return;
}
@@ -1395,7 +1377,7 @@
switch (le32_to_cpu(xhci->cmd_ring->dequeue->generic.field[3])
& TRB_TYPE_BITMASK) {
case TRB_TYPE(TRB_ENABLE_SLOT):
- if (GET_COMP_CODE(le32_to_cpu(event->status)) == COMP_SUCCESS)
+ if (comp_code == COMP_SUCCESS)
xhci->slot_id = slot_id;
else
xhci->slot_id = 0;
@@ -1451,19 +1433,18 @@
}
bandwidth_change:
xhci_dbg(xhci, "Completed config ep cmd\n");
- xhci->devs[slot_id]->cmd_status =
- GET_COMP_CODE(le32_to_cpu(event->status));
+ xhci->devs[slot_id]->cmd_status = comp_code;
complete(&xhci->devs[slot_id]->cmd_completion);
break;
case TRB_TYPE(TRB_EVAL_CONTEXT):
virt_dev = xhci->devs[slot_id];
if (handle_cmd_in_cmd_wait_list(xhci, virt_dev, event))
break;
- xhci->devs[slot_id]->cmd_status = GET_COMP_CODE(le32_to_cpu(event->status));
+ xhci->devs[slot_id]->cmd_status = comp_code;
complete(&xhci->devs[slot_id]->cmd_completion);
break;
case TRB_TYPE(TRB_ADDR_DEV):
- xhci->devs[slot_id]->cmd_status = GET_COMP_CODE(le32_to_cpu(event->status));
+ xhci->devs[slot_id]->cmd_status = comp_code;
complete(&xhci->addr_dev);
break;
case TRB_TYPE(TRB_STOP_RING):

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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia30e6fa7fd24908fbb8f48dd9f2d6fa3f6db4a75
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.4
Gerrit-Owner: Julius Werner <jwe...@chromium.org>
Gerrit-Reviewer: Vincent Palatin <vpal...@chromium.org>

[Vincent Palatin (Code Review)]

Vincent Palatin has posted comments on this change.

Change subject: usb: xhci: Fix Command Ring Stopped Event handling
......................................................................

Patch Set 1: Looks good to me, approved

looks sensible but I have no expertise in those USB3 details.
let see what upstream says.

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

Gerrit-MessageType: comment

Gerrit-Change-Id: Ia30e6fa7fd24908fbb8f48dd9f2d6fa3f6db4a75
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: Vincent Palatin <vpal...@chromium.org>