On Sun, May 04, 2014 at 07:22:53PM +0100, Ben Hutchings wrote:
> 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?
The only driver that uses streams (uas) is marked as CONFIG_BROKEN. The
streams code will never be exercised in stable kernels, so it's not
urgent to backport that commit. The broken status has been removed in
3.15.
Basically, pick it if it makes backporting easier, but it's not
necessary.
Sarah Sharp
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html