Re: xhci ASMedia lockups - a theory and a patch
On Thu, Jan 16, 2014 at 10:21:11AM +, David Laight wrote: From: David Laight From: David Laight From: Alan Stern On Wed, 15 Jan 2014, David Laight wrote: I have a theory, I'll try to write a non-invasive patch. ... Doesn't this mean you shouldn't change the ownership of a LINK TRB until after you change the ownership of the TRB it points to? That is what I assume. In practise this means that the 'first_trb' (whose ownership is set last) has to be the one that is valid when prepare_ring() is called. The plan for the patch is: - Save the enq pointer in prepare_ring (in the ep_ring structure). - When writing a trb set the ownership unless it is the saved one (ignoring the value set by the caller). - At the end invert the ownership on the saved entry. Below is a possible patch, I've only compile tested it. I've minimalised the patch by not removing all the code that saves 'start_trb' and modifies the TRB_CYCLE bit. If the patch works those parts can also be tidied up. I've had some feedback (in a private email) that the patch helps. This was using the ASMedia controller with the asx88179_178a ethernet device. David, please post the contents of that private email to the list and Cc me. We don't debug in private in the kernel. So the theory was definitely on the right lines. And I managed to write the patch without any silly mistakes! Your theory about the ASMedia host may be correct. There are other host controllers (Synopsys, I think) that needed us to not give a link TRB over to the host until the TRB it pointed had the cycle bit set to hardware owned. ISTR that this was only triggered with USB 3.0 streams, at least on that Synopsys host. (BTW, you can find out why any part of the code was written by using `git blame filename`, although the original commit that fixed this bug has an entirely unhelpful description.) So, I agree that this needs to get fixed, especially since full UAS and streams support will be landing in the 3.15 kernel. However, I like the simple patch you posted much better than the second patch. The second patch is much too big to be back ported to stable, and we need to get this backported to stable. Please resend your previous patch. 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: xhci ASMedia lockups - a theory and a patch
From: David Laight From: David Laight From: Alan Stern On Wed, 15 Jan 2014, David Laight wrote: I have a theory, I'll try to write a non-invasive patch. ... Doesn't this mean you shouldn't change the ownership of a LINK TRB until after you change the ownership of the TRB it points to? That is what I assume. In practise this means that the 'first_trb' (whose ownership is set last) has to be the one that is valid when prepare_ring() is called. The plan for the patch is: - Save the enq pointer in prepare_ring (in the ep_ring structure). - When writing a trb set the ownership unless it is the saved one (ignoring the value set by the caller). - At the end invert the ownership on the saved entry. Below is a possible patch, I've only compile tested it. I've minimalised the patch by not removing all the code that saves 'start_trb' and modifies the TRB_CYCLE bit. If the patch works those parts can also be tidied up. I've had some feedback (in a private email) that the patch helps. This was using the ASMedia controller with the asx88179_178a ethernet device. So the theory was definitely on the right lines. And I managed to write the patch without any silly mistakes! David -- 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: xhci ASMedia lockups - a theory and a patch
From: David Laight
From: Alan Stern
On Wed, 15 Jan 2014, David Laight wrote:
I have a theory, I'll try to write a non-invasive patch.
...
Doesn't this mean you shouldn't change the ownership of a LINK TRB
until after you change the ownership of the TRB it points to?
That is what I assume.
In practise this means that the 'first_trb' (whose ownership is set
last) has to be the one that is valid when prepare_ring() is called.
The plan for the patch is:
- Save the enq pointer in prepare_ring (in the ep_ring structure).
- When writing a trb set the ownership unless it is the saved one
(ignoring the value set by the caller).
- At the end invert the ownership on the saved entry.
Below is a possible patch, I've only compile tested it.
I've minimalised the patch by not removing all the code that saves
'start_trb' and modifies the TRB_CYCLE bit.
If the patch works those parts can also be tidied up.
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 53c2e29..589d336 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -2928,6 +2928,11 @@ static void queue_trb(struct xhci_hcd *xhci, struct
xhci_ring *ring,
struct xhci_generic_trb *trb;
trb = ring-enqueue-generic;
+
+ field4 = (field4 ~TRB_CYCLE) | ring-cycle_state;
+ if (trb == ring-enqueue_first-generic)
+ field4 ^= TRB_CYCLE;
+
trb-field[0] = cpu_to_le32(field1);
trb-field[1] = cpu_to_le32(field2);
trb-field[2] = cpu_to_le32(field3);
@@ -2972,6 +2977,8 @@ static int prepare_ring(struct xhci_hcd *xhci, struct
xhci_ring *ep_ring,
return -EINVAL;
}
+ /* Save entry whose owner needs flipping at the end */
+ ep_ring-enqueue_first = ep_ring-enqueue;
while (1) {
if (room_on_ring(xhci, ep_ring, num_trbs)) {
union xhci_trb *trb = ep_ring-enqueue;
@@ -3014,13 +3021,16 @@ static int prepare_ring(struct xhci_hcd *xhci, struct
xhci_ring *ep_ring,
nop_cmd = cpu_to_le32(TRB_TYPE(TRB_TR_NOOP) |
ep_ring-cycle_state);
ep_ring-num_trbs_free -= usable;
- do {
+ trb-generic.field[3] = nop_cmd ^
cpu_to_le32(TRB_CYCLE);
+ for (;;) {
trb-generic.field[0] = 0;
trb-generic.field[1] = 0;
trb-generic.field[2] = 0;
- trb-generic.field[3] = nop_cmd;
trb++;
- } while (--usable);
+ if (!--usable)
+ break;
+ trb-generic.field[3] = nop_cmd;
+ }
ep_ring-enqueue = trb;
if (room_on_ring(xhci, ep_ring, num_trbs))
break;
@@ -3059,7 +3069,8 @@ static int prepare_ring(struct xhci_hcd *xhci, struct
xhci_ring *ep_ring,
next-link.control |= cpu_to_le32(TRB_CHAIN);
wmb();
- next-link.control ^= cpu_to_le32(TRB_CYCLE);
+ if (next != ep_ring-enqueue_first)
+ next-link.control ^= cpu_to_le32(TRB_CYCLE);
/* Toggle the cycle bit after the last ring segment. */
if (last_trb_on_last_seg(xhci, ring, ring-enq_seg,
next)) {
@@ -3096,11 +3107,13 @@ static int prepare_transfer(struct xhci_hcd *xhci,
return -EINVAL;
}
- ret = prepare_ring(xhci, ep_ring,
- le32_to_cpu(ep_ctx-ep_info) EP_STATE_MASK,
- num_trbs, mem_flags);
- if (ret)
- return ret;
+ if (td_index == 0) {
+ ret = prepare_ring(xhci, ep_ring,
+ le32_to_cpu(ep_ctx-ep_info) EP_STATE_MASK,
+ num_trbs, mem_flags);
+ if (ret)
+ return ret;
+ }
urb_priv = urb-hcpriv;
td = urb_priv-td[td_index];
@@ -3175,19 +3188,24 @@ static void check_trb_math(struct urb *urb, int
num_trbs, int running_total)
}
static void giveback_first_trb(struct xhci_hcd *xhci, int slot_id,
- unsigned int ep_index, unsigned int stream_id, int start_cycle,
- struct xhci_generic_trb *start_trb)
+ unsigned int ep_index, struct xhci_ring *ring)
{
/*
* Pass all the TRBs to the hardware at once and make sure this write
* isn't reordered.
*/
wmb();
- if (start_cycle)
- start_trb-field[3] |= cpu_to_le32(start_cycle);
- else
- start_trb-field[3] = cpu_to_le32(~TRB_CYCLE);
-
