On Wed, May 28, 2014 at 12:47 PM, Mathias Nyman <mathias.nyman@xxxxxxxxx> wrote:
> I'm interested in knowing if we get the slot_id mangled, or the TRB's we are
> comparing out of sync.
>
> Grant's trace didn't show anything, can you try to add the same trace?
>
> Or even better if you can apply this patch and send me the output (dmesg) after
> boot?
>
> Thanks
> -Mathias
>
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 7a0e3c7..5575433 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -1611,6 +1611,11 @@ static void handle_cmd_completion(struct xhci_hcd *xhci,
> xhci_handle_cmd_reset_ep(xhci, slot_id, cmd_trb, cmd_comp_code);
> break;
> case TRB_RESET_DEV:
> + xhci_err(xhci, "RESET_DEV dq:%p dqslot:%d event:%p evslot %d\n",
> + cmd_trb,
> + TRB_TO_SLOT_ID(le32_to_cpu(cmd_trb->generic.field[3])),
> + le64_to_cpu(event->cmd_trb),
> + slot_id);
> WARN_ON(slot_id != TRB_TO_SLOT_ID(
> le32_to_cpu(cmd_trb->generic.field[3])));
> xhci_handle_cmd_reset_dev(xhci, slot_id, event);
> @@ -4014,6 +4019,11 @@ static int queue_command(struct xhci_hcd *xhci, u32 field1,
> u32 field2,
> "unfailable commands failed.\n");
> return ret;
> }
> + if (TRB_FIELD_TO_TYPE(field4) == TRB_RESET_DEV)
> + xhci_err(xhci, "Queue reset dev cmd for slot %d at trb %p\n",
> + TRB_TO_SLOT_ID(field4),
> + xhci->cmd_ring->enqueue);
> +
> queue_trb(xhci, xhci->cmd_ring, false, field1, field2, field3,
> field4 | xhci->cmd_ring->cycle_state);
> return 0;
>
dmesg | grep RESET_DEV
[ 3.544697] xhci_hcd 0000:02:00.0: RESET_DEV dq:ffff88051017e020
dqslot:1 event:000000051017e020 evslot 0
dmesg | grep "Queue reset"
[ 3.541142] xhci_hcd 0000:02:00.0: Queue reset dev cmd for slot 1
at trb ffff88051017e020
Event TRB's Slot ID differs from cmd_ring's. In trying to see if this
is in violation of xHCI spec,
I briefly skimmed through it, and there appears to be a contradiction
in the spec about
Slot ID value for an event for a Host Controller Command command.
In Sec 6.4.2.2 (Command Completion Event TRB) on Page 371 -
"Slot ID. The Slot ID field shall be updated by the xHC to reflect the
slot associated with the command that generated the event,
with the following exceptions:
...
If this Event is due to a Host Controller Command, then this field
shall be cleared to 0."
For TRB_SET_DEQ and TRB_RESET_EP, Sec 4.6.10 and Sec 4.6.8 respectively
ask software to issue a Host Controller Command, and ask xHC to set
Slot ID to command's Slot ID in response. In the case of TRB_STOP_RING,
Sec 4.6.1.1 is less clear on the details but seems to imply
the same behaviour. For TRB_RESET_DEV, Sec 4.6.11, it asks xHC to clear
all (remaining) fields of event TRB to 0. I think this is in contradiction with
Sec 6.4.2.2.
If the above contradiction-in-spec is the case, unless I am mistaken,
it might just be better off using using cmd_ring's dequeue TRB's slot_id
instead of event TRB's for all four cases.
The patch below works for me.
N.B. I didn't know anything about USB until today and the above
observations are only from perfunctory reading of the spec.
Please verify if it makes sense (esp. for cases other than TRB_RESET_DEV).
Thanks
---
>From 58e8298988b2cb0d0d2854a216134614e79c3332 Mon Sep 17 00:00:00 2001
From: Saran Neti <sarannmr@xxxxxxxxx>
Date: Thu, 29 May 2014 01:11:45 -0400
Subject: [PATCH] xHCI Slot ID mismatch in Command, Event. Fix Regression.
TRB_SET_DEQ, TRB_RESET_EP, TRB_STOP_RING and TRB_RESET_DEV are all
issued as Host Controller Command, for which response xHC's behaviour
appears to be contradictory in the xHCI 1.1 spec.
According to Sec 6.4.2.2 (Command Completion Event TRB):
If this Event is due to a Host Controller Command, then this field
shall be cleared to 0. But in Sec 4.6.10 (Set TR Dequeue Pointer),
the spec asks xHC to copy command's Slot ID to its event TRB's Slot ID.
Similarly for others.
Differing cmd_ring and event Slot ID values were causing regression
in TRB_RESET_DEV starting 20e7acb13ff48. This patch fixes it by using
the original cmd_ring TRB Slot ID instead of event's for these cases.
---
drivers/usb/host/xhci-ring.c | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 7a0e3c7..bb5e84d 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1593,26 +1593,27 @@ static void handle_cmd_completion(struct xhci_hcd *xhci,
case TRB_ADDR_DEV:
xhci_handle_cmd_addr_dev(xhci, slot_id, cmd_comp_code);
break;
+ /* Event trb's slot id can be zero for Host Controller Command
+ and may not match the corresponding command's slot id.
+ Spec seems to be contradictory. Use cmd_ring's dequeue trb's
+ slot_id.
+ */
case TRB_STOP_RING:
- WARN_ON(slot_id != TRB_TO_SLOT_ID(
- le32_to_cpu(cmd_trb->generic.field[3])));
+ slot_id =TRB_TO_SLOT_ID(le32_to_cpu(cmd_trb->generic.field[3]));
xhci_handle_cmd_stop_ep(xhci, slot_id, cmd_trb, event);
break;
case TRB_SET_DEQ:
- WARN_ON(slot_id != TRB_TO_SLOT_ID(
- le32_to_cpu(cmd_trb->generic.field[3])));
+ slot_id =TRB_TO_SLOT_ID(le32_to_cpu(cmd_trb->generic.field[3]));
xhci_handle_cmd_set_deq(xhci, slot_id, cmd_trb, cmd_comp_code);
break;
case TRB_CMD_NOOP:
break;
case TRB_RESET_EP:
- WARN_ON(slot_id != TRB_TO_SLOT_ID(
- le32_to_cpu(cmd_trb->generic.field[3])));
+ slot_id =TRB_TO_SLOT_ID(le32_to_cpu(cmd_trb->generic.field[3]));
xhci_handle_cmd_reset_ep(xhci, slot_id, cmd_trb, cmd_comp_code);
break;
case TRB_RESET_DEV:
- WARN_ON(slot_id != TRB_TO_SLOT_ID(
- le32_to_cpu(cmd_trb->generic.field[3])));
+ slot_id =TRB_TO_SLOT_ID(le32_to_cpu(cmd_trb->generic.field[3]));
xhci_handle_cmd_reset_dev(xhci, slot_id, event);
break;
case TRB_NEC_GET_FW:
--
1.9.3
--
Saran
--
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
