Hi Micah, I'm currently at a conference, so responses may be a bit delayed. :) You are correct that there is a bug (two, really) with the link TRB activation patch that John wrote. However, removing the code like your patch does causes the host controller to stop responding to transfers when stream rings are enabled. So the code your patch removes must stay, but the bugs need to be fixed. I have two patches that I think fully fix the issues with unchained TRBs before link TRBs. One has already been queued by Greg KH, and the other I haven't sent off to him yet. Can you please check out the two patches at the HEAD of the link-trb-fix branch and see if it fixes your issues? http://git.kernel.org/?p=linux/kernel/git/sarah/xhci.git;a=shortlog;h=refs/heads/link-trb-fix Sarah Sharp On Fri, Jul 02, 2010 at 01:16:39PM -0700, Micah Dowty wrote: > Hi, > > I believe I may have found a bug in the xHCI driver, specifically in > xhci-ring's inc_enq(). If you insert a non-chained TRB immediately > prior to a link TRB, some logic kicks in to avoid having chained link > TRBs (which are not allowed in xHCI 0.95). However, I think this logic > may be faulty- it actually causes inc_enq() to ignore the link TRB > entirely. > > The ring's enqueue pointer remains pointing to the link TRB, and the > next enqueued TRB on this ring overwrites it. Then, subsequent TRBs > write past the bounds of the ring. At best, this will cause "suspect > DMA" errors when handling completions. At worst, it's a kernel panic. > > I'm not sure what the proper solution is to this, since I'm not > familiar with the way link TRBs and chain bits were defined in version > 0.95 of the spec. But I've been using a very simple patch for my own > testing, and it seems to successfully fix this problem. > > Thank you in advance for any comments on this problem or the patch :) > > Signed-off-by: Micah Dowty <micah@xxxxxxxxxx> > --- > drivers/usb/host/xhci-ring.c | 12 +++++------- > 1 files changed, 5 insertions(+), 7 deletions(-) > > diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c > index b03b0f1..88c3d5b 100644 > --- a/drivers/usb/host/xhci-ring.c > +++ b/drivers/usb/host/xhci-ring.c > @@ -194,14 +194,12 @@ static void inc_enq(struct xhci_hcd *xhci, struct xhci_ring *ring, bool consumer > if (!consumer) { > if (ring != xhci->event_ring) { > if (chain) { > - next->link.control |= TRB_CHAIN; > - > - /* Give this link TRB to the hardware */ > - wmb(); > - next->link.control ^= TRB_CYCLE; > - } else { > - break; > + next->link.control |= TRB_CHAIN; > } > + > + /* Give this link TRB to the hardware */ > + wmb(); > + next->link.control ^= TRB_CYCLE; > } > /* Toggle the cycle bit after the last ring segment. */ > if (last_trb_on_last_seg(xhci, ring, ring->enq_seg, next)) { > -- > 1.6.3.3 > -- 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
