On Mon, Sep 26, 2011 at 05:41:02PM +0800, Andiry Xu wrote: > Hi Sarah, > > This is the patchset for xHCI dynamic ring expansion. > > When room_on_ring() check fails, try to expand the ring with double segment > numbers. It's achieved by either finding a cached ring or allocating a new > ring with the same number of sgements as the old ring, then link the old ring > to the new ring. It fixes the "ERROR: no room on ring" error unless the > driver askes for a ring with more than 256 segments. I don't think this patchset is sufficient (and there's a couple bugs as well). If you're only checking whether you run out of space when room_on_ring fails, then it's pretty trivial to work the driver into a situation where it can't expand the rings. For example, say you have a one-segment ring, and someone queues a 3-TRB TD, which completes. Then they try to queue two 61-TRB TDs in a row. For the first one, room_on_ring() succeeds, the TD is queued, and the link TRB at the end of the segment is given to the hardware (the ownership bit is toggled). Then room_on_ring() fails for the second TD. The driver can't overwrite the link TRB that it just gave to the hardware (details for why is in my response to patch 3), so unless it adds a link TRB to a new segment in the middle of the ring, it can't expand the ring and must fail that particular transfer. But later transfers could succeed after the second TD completes and you can successfully expand the ring. That will play havoc with applications that submit big transfers. What I'd like to see a ring expansion patchset do is this: 1. Modify the ring handling code to keep track of how many TRBs are actually queued to the ring. This will help us not *walk the ring* every time we need to queue a transfer, which is a time-waster for larger transfers. The tricky bit will be handling updates to that queued TRB number when a Set TR Dequeue Pointer command completes. 2. Before giving control of a link TRB to the hardware, check if the number of queued TRBs is greater than half the capacity of the ring. If so, preemptively double the ring, adding the new segments in between the current enqueue segment and the next segment. If we know the current TD to be queued requires more ring segments than a doubled-in-size ring would hold, allocate as many segments as that transfer needs (maybe adding one more segment for good measure). What do you think? I'm open to other suggestions, but I really think we need to pre-emptively expand the ring, rather than waiting for room_on_ring to fail. Sarah Sharp > It's tested with an isoc device which requires at least 32 segments. > The patchset is based on AMD isoc link TRB quirk patch. > > Thanks, > Andiry > > --- > > Andiry Xu (3): > xHCI: store ring's last segment and segment numbers > xHCI: set cycle state when allocate rings > xHCI: dynamic ring expansion > > drivers/usb/host/xhci-mem.c | 132 +++++++++++++++++++++++++++++++++++------- > drivers/usb/host/xhci-ring.c | 38 +++++++++---- > drivers/usb/host/xhci.h | 5 ++ > 3 files changed, 142 insertions(+), 33 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
