On Tue, Dec 20, 2011 at 04:01:32PM +0800, Andiry Xu wrote: > 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? Yes. Both webcams need the ring expansion, although neither of them seem to be triggering the last patch. The USB 2.0 webcam starts out with 1 segment, goes to 2 segments, and then 4 segments. > > 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'll take a look at that. > > 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. Greg has shot down any patches to statically allocate more endpoint rings if they add new API (e.g. a module parameter). So I think this will just remain broken until dynamic ring expansion gets in. > > 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. Yep, that is an issue. So I think option 1 is the only full solution. But as I said, that would change the cancellation code behavior, and we would have to be really careful about that. It's easier just to drop the last patch and see if anyone still complains. > > 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. I can take a look at it, but I don't think it's going to get into 3.3, since it's so late in the cycle (-rc5). BTW, does it support both types of host controller register encoding, or only the HIRD encoding? There's an Intel white paper (written by the folks that did the USB 2.0 LPM errata) that I've been meaning to look at, since it has requirements for host controller behavior for USB 2.0 LPM. I'll ping John Howard and see if that white paper is public somewhere so you can look at it too. 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
