Hi Tim, If you're having trouble patching the kernel, you can pull from my andiry-ring-expansion branch. That has Andiry's patches, along with some debug removal patches that will allow you to actually turn on xHCI debugging without overwhelming the dmesg output. Sarah Sharp On Tue, Dec 20, 2011 at 11:15:31AM -0800, Tim Vlaar wrote: > Yes, the error mentioned (Failed isochronous start. Error: 0x1) is something we should look at. Note that even though it mentions isoch, it's actually bulk. > > With our camera, you can also use it as a USB 2 camera with the same frames sizes, etc., but it will have lower frame rate limits. It can be enumerated as USB 2 by either using a USB 2 cable on a USB 3 card or by plugging it into a USB 2 port. This provides the ability to exercise potentially different code in the xhci driver and/or compare it to how the USB 2 driver works. > > We are currently trying to test with the latest patchset also, but are having challenges getting it installed. Once we work these out, I will report our findings. > > Thanks > Tim > > -----Original Message----- > From: Sarah Sharp [mailto:sarah.a.sharp@xxxxxxxxxxxxxxx] > Sent: Monday, December 19, 2011 5:46 PM > To: Andiry Xu; Tim Vlaar > Cc: linux-usb@xxxxxxxxxxxxxxx > Subject: Re: [PATCH 0/8] xHCI ring expansion patchset > > Hi Andiry, > > I've been testing this patchset, and it seems to work fine for the USB > 2.0 webcams I have. > > However, when I test the Point Grey USB 3.0 webcam, my machine hard-hangs when I unplug the device while the video stream is open. I believe there's a bug that's hit somewhere in the cancellation code that makes the interrupt handler go into an infinite loop. This doesn't happen when the USB 2.0 webcam uses the ring expansion code, and I unplug it when the video stream is active. > > The dmesg is attached. The first start of the "flycap" video capture code always fails with some sort of error, like > > "Source:lidcCameraInternal.cpp(444) Built: Oct 7 2011 15:39:35 Error starting isochronous stream. > +-> From: Iso.cpp(1551) Built Oct 7 2011 15:39:08 - > Failed isochronous start. Error: 0x1" > > I think this is an application level-error that Tim might want to look into, and not an xHCI issue. However, the app works fine when I open it a second time (without removing the device, so that expanded rings are still intact). > > Around timestamp 2684.043289, I unplug the device. A Set TR dequeue pointer command and another Stop Endpoint command are issued around timestamp 2684.372089, and as far as I can tell, we don't get an event for them, or the event handler is hung. > > I've tried reverting the last patch (waiting for the link TRB handover), but the crash still occurs. If I revert the entire patchset, apply a small patch to statically allocate 128 segments for any Point Grey device rings, and then run the same disconnect-while-active test, the crash doesn't occur. I can try to bisect the patches, if you think that would be helpful. > > I'm not sure if we want to queue the expansion patches for 3.3 if they're known to cause a hard-hang. But we're pretty close anyway. :) > > > Also, in looking over the last patch in the series, I'm not sure it's entirely safe. The ring dequeue update code just assumes that the xHCI host controller has actually read and processed the link TRB when we move the dequeue pointer past the TRB before the link TRB. But we don't really know the hardware has read the link TRB until we get an event for a TRB in the next segment. > > If the hardware had paused just after the last normal TRB in the dequeue segment, but didn't process the link TRB, and then we over write the link TRB, the hardware will see the new segments. If the hardware was in the middle of processing a TD, the unchained no-op TRBs in the new segment would be very confusing. Writing the address pointer while the hardware was in the middle of reading the link TRB would be disastrous. > > I see two ways to solve this. We can either 1) set the IOC flag on the link TRB when we have pending URBs queued, or 2) if there is another TD on the next segment, we can wait for that TD to get an event, and then expand the ring. If we went with setting the IOC flag, we'd have to look very carefully at the assumptions around the cancellation code, and we'd *still* be subject to race conditions unless we set the IOC flag on the link TRBs all the time. The second option also sounds racy and complex, and I don't really want to touch the ring cancellation code behavior in order to implement the first option. > > So let's just drop the last patch. Users will get at least some ring expansion support, and we don't have to go through hoops for this special corner case. > > Sarah Sharp > > On Mon, Dec 12, 2011 at 04:47:46PM +0800, Andiry Xu wrote: > > Hi Sarah, > > > > This is xHCI ring expansion patchset v1 for formal submission. > > > > Changelog from RFC v3: > > > > 1. Fix the bug in patch 3 that num_trbs_free is not calculated correctly > > in xhci_queue_isoc_tx(). > > 2. Fix for patch7 "wait for dequeue pointer move pass link TRB": > > 1) Give back all the pending urbs if ring expansion or urb enqueue fail in > > xhci_queue_pending_urbs(). > > 2) Remove waiting_deq_ptr variable in xhci_ring struct. > > 3) In xhci_urb_dequeue(), check it in pending urb list and give it back. > > 3. Move patch7 to the last position. It's up to you to accept it or not, it > > does not affect patchset function, only solve a special case seldom occurs. > > > > The patchset passed isoc device test. > > > > Thanks, > > Andiry > > > > --- > > > > Andiry Xu (8): > > xHCI: store ring's type > > xHCI: store ring's last segment and segment numbers > > xHCI: count free TRBs on transfer ring > > xHCI: factor out segments allocation and free function > > xHCI: set cycle state when allocate rings > > xHCI: dynamic ring expansion > > xHCI: update sg tablesize > > xHCI: wait for dequeue pointer move pass link TRB > > > > drivers/usb/host/xhci-mem.c | 235 ++++++++++++++++++------- > > drivers/usb/host/xhci-ring.c | 402 ++++++++++++++++++++++++++++++------------ > > drivers/usb/host/xhci.c | 61 +++++-- > > drivers/usb/host/xhci.h | 25 +++ > > 4 files changed, 526 insertions(+), 197 deletions(-) > > > > -- > > 1.7.4.1 > > > > -- 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
