On Tue, Sep 27, 2011 at 01:00:44PM +0800, Andiry Xu wrote:
> On Mon, 2011-09-26 at 09:00 -0700, Sarah Sharp wrote:
> > On Mon, Sep 26, 2011 at 05:41:05PM +0800, Andiry Xu wrote:
> When expand a ring, I try to use a cached ring first; if a suitable
> cached ring is available, the ring structure needs to be freed anyway.
> If I need to allocate new segments, use xhci_ring_alloc() is simple and
> makes the code look cleaner, I don't need to call segment alloc and link
> functions directly.
>
> Maybe factor out the segment allocation and linking part in
> xhci_ring_alloc() as a single function is a good idea, so you do not
> need to duplicate the code.
Yeah, refactoring out the segment allocation and linking would probably
be better than calling xhci_ring_alloc and then freeing the xhci_ring
structure.
> > At that point, you've given control the link TRB to the hardware by
> > toggling the cycle bit. If you try to overwrite a link TRB that's given
> > over to the hardware, one of two things will happen:
> >
> > 1. The hardware has already cached the link TRB, and the write will
> > never be noticed. It will just go back to the top of the original
> > segment after processing TD A, TRB 3. But adding that new segment won't help
> > you enqueue TD B.
> >
> > 2. The hardware hasn't cached the link TRB and it will go fetch TRBs in
> > the new segment. If the new segments have the cycle bit set to software
> > owned (which I think you do that in this patch), then hardware will stop
> > on the first TRB of the new segment (since it doesn't own that TRB) and
> > fail to execute the rest of TD A. Then the cancellation code will run,
> > and goodness knows what it will make of the ring structure and where the
> > hardware thinks it is stopped.
> >
>
> Oh, I see your point. Yes, you are right; I failed to realize this
> scenario. So we can not expand a ring if "the software has toggled the
> cycle while the hardware has not", right? If that's the case, preemptive
> ring expansion is necessary.
Only software toggles the cycle bit. The hardware dequeue pointer is
tracked by what events are placed on the event ring; it doesn't touch
the cycle bit for rings where it is the consumer. So yes, preemptive
ring expansion is necessary. :)
> > So you can't just blindly assume you'll always be expanding the ring
> > before you wrap around the link TRB at the end of the segment. I would
> > highly recommend you not pursue the option of adding link TRBs in the
> > middle of the segment, as none of the ring walking code, cancellation
> > code, or enqueuing code is set up to handle that. Because adding link
> > TRBs in the middle would be such a radical change, I think you really
> > need pre-emptive ring expansion before overwriting link TRBs.
> >
>
> When you say "adding link TRBs in the middle of segment", do you mean
> adding link TRB not as the last the TRB of a segment, say, the 64th TRB?
> I don't think I have done that sort of things.
Yes, that's what I mean. And no, your patches don't do that. It would
be an option to expand the rings, but I was pointing it out as one of
the tougher options to go with.
> > <snip>
> > > - if (!room_on_ring(xhci, ep_ring, num_trbs)) {
> > > - /* FIXME allocate more room */
> > > - xhci_err(xhci, "ERROR no room on ep ring\n");
> > > - return -ENOMEM;
> > > - }
> > > +
> > > + while (1) {
> > > + if (room_on_ring(xhci, ep_ring, num_trbs))
> > > + break;
> > > +
> > > + if (ep_ring->num_segs >= 256) {
> > > + xhci_err(xhci, "ERROR no room on ring "
> > > + "after ring expansion\n");
> > > + return -ENOMEM;
> > > + }
> >
> > Please don't place arbitrary limits on ring expansion like this. I've
> > heard from at least one customer that they'll may need ring segments in the
> > thousands, so I've stopped assuming I know what ring size is "too big".
> > Just let the kernel run out of memory if drivers attempt to queue too
> > many big transfers.
> >
>
> Ah, I'm not aware of that. I thought 256 segments is already big enough
> for any transfer...
I thought so too (actually I thought 100 would be enough), but
apparently some devices need to queue a large number of very big bulk
transfers. So let's not limit the size of a ring. :)
Sarah Sharp
--
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
