On Fri, 2014-05-02 at 13:18 -0700, Julius Werner wrote:
> commit 1f81b6d22a5980955b01e08cf27fb745dc9b686f upstream.
>
> We have observed a rare cycle state desync bug after Set TR Dequeue
> Pointer commands on Intel LynxPoint xHCs (resulting in an endpoint that
> doesn't fetch new TRBs and thus an unresponsive USB device). It always
> triggers when a previous Set TR Dequeue Pointer command has set the
> pointer to the final Link TRB of a segment, and then another URB gets
> enqueued and cancelled again before it can be completed. Further
> investigation showed that the xHC had returned the Link TRB in the TRB
> Pointer field of the Transfer Event (CC == Stopped -- Length Invalid),
> but when xhci_find_new_dequeue_state() later accesses the Endpoint
> Context's TR Dequeue Pointer field it is set to the first TRB of the
> next segment.
>
> The driver expects those two values to be the same in this situation,
> and uses the cycle state of the latter together with the address of the
> former. This should be fine according to the XHCI specification, since
> the endpoint ring should be stopped when returning the Transfer Event
> and thus should not advance over the Link TRB before it gets restarted.
> However, real-world XHCI implementations apparently don't really care
> that much about these details, so the driver should follow a more
> defensive approach to try to work around HC spec violations.
>
> This patch removes the stopped_trb variable that had been used to store
> the TRB Pointer from the last Transfer Event of a stopped TRB. Instead,
> xhci_find_new_dequeue_state() now relies only on the Endpoint Context,
> requiring a small amount of additional processing to find the virtual
> address corresponding to the TR Dequeue Pointer. Some other parts of the
> function were slightly rearranged to better fit into this model.
>
> This patch should be backported to kernels as old as 2.6.31 that contain
> the commit ae636747146ea97efa18e04576acd3416e2514f5 "USB: xhci: URB
> cancellation support."
>
> [jwerner@xxxxxxxxxxxx: port back to code without correct stream handling]
> Signed-off-by: Julius Werner <jwerner@xxxxxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Mathias Nyman <mathias.nyman@xxxxxxxxxxxxxxx>
> Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> ---
> drivers/usb/host/xhci-ring.c | 67 ++++++++++++++++++++------------------------
> drivers/usb/host/xhci.c | 1 -
> drivers/usb/host/xhci.h | 2 --
> 3 files changed, 31 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 0ed64eb..dff9b5e 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -550,6 +550,7 @@ void xhci_find_new_dequeue_state(struct xhci_hcd *xhci,
> struct xhci_generic_trb *trb;
> struct xhci_ep_ctx *ep_ctx;
> dma_addr_t addr;
> + u64 hw_dequeue;
>
> ep_ring = xhci_triad_to_transfer_ring(xhci, slot_id,
> ep_index, stream_id);
> @@ -559,56 +560,57 @@ void xhci_find_new_dequeue_state(struct xhci_hcd *xhci,
> stream_id);
> return;
> }
> - state->new_cycle_state = 0;
> - xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
> - "Finding segment containing stopped TRB.");
> - state->new_deq_seg = find_trb_seg(cur_td->start_seg,
> - dev->eps[ep_index].stopped_trb,
> - &state->new_cycle_state);
> - if (!state->new_deq_seg) {
> - WARN_ON(1);
> - return;
> - }
>
> /* Dig out the cycle state saved by the xHC during the stop ep cmd */
> xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
> "Finding endpoint context");
> ep_ctx = xhci_get_ep_ctx(xhci, dev->out_ctx, ep_index);
> - state->new_cycle_state = 0x1 & le64_to_cpu(ep_ctx->deq);
> + hw_dequeue = le64_to_cpu(ep_ctx->deq);
[...]
Should we not pick commit c4bedb77ec4cb42f37cae4cbfddda8283161f7c8
('xhci: For streams the css flag most be read from the stream-ctx on ep
stop') before this?
Ben.
--
Ben Hutchings
Everything should be made as simple as possible, but not simpler.
- Albert Einstein
Attachment:
signature.asc
Description: This is a digitally signed message part