Thanks a lot for the test. On 12/20/2011 09:46 AM, Sarah Sharp wrote: > Hi Andiry, > > I've been testing this patchset, and it seems to work fine for the USB > 2.0 webcams I have. > Does the USB2.0 webcam trigger the ring expansion code? i.e. does it need more than one transfer ring? > 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. > There is some code related to Set TRDP command completion, in patch 3 "count free TRBs on transfer ring", when process a set TRDP event, driver will update num_trbs_free by walking over the ring when update dequeue pointer. Can you check the code to see if that's right? > 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. :) > No, let's resolve the issue first. KrishnaMohan Bandi also reports some issue regarding the patchset, but I've not got feedback when asking for more info. I suggest we can use some temporary workaround for 3.3 (allocate more segments statically). That will resolve some people's request. > > 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. > Hmm, you're right. Driver must guarantee the host has walked over the Link TRB. Only report the event before Link TRB is not enough. > 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. > Option 1 would impact current driver behaviour. Option 2 needs another TD on the next segment. What happens if there is no TD on the next segment? Suppose there is only one segment, enqueue pointer is at the first TRB, and waiting for dequeue pointer walk over Link TRB. After dequeue pointer walk over Link TRB, it stops at the first TRB too, without a transfer event. Now both pointer wait for each other, the ring will not get expanded. > 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 > OK, thanks for your help. BTW, can you look at the patch "xHCI: BESL calculation based on USB2.0 LPM errata" I sent on 12/12? It's a errata update of USB2.0 LPM patch. Thanks, Andiry > 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
