Hi Sarah, Thanks for the quick response. A couple of us here have had a chance to test the link-trb-fix branch, and it appears to be working well. Our tests are passing, no more kernel panics :) Thank you! --Micah On Mon, Jul 05, 2010 at 08:49:38AM -0700, Sarah Sharp wrote: > 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
