On Tue, Dec 17, 2013 at 10:15:46AM -0000, David Laight wrote:
> > From: Sarah Sharp
> > On Mon, Dec 16, 2013 at 03:20:28PM -0000, David Laight wrote:
> > > > I know all these difference clearly, inc_deq() is indeed a common
> > > > function for different rings, but lasr_trb() & last_trb_on_last_seg()
> > > > inside it use different condition to determine the last trb in an
> > > > event ring and an non-event ring; and sorry, i still not find why last
> > > > trb in an event ring skipped by H/W according to the current logic.
> > > >
> > > > Thanks!
> > > >
> > >
> > > Read the specs and the code.
> >
> > State your objections clearly.
> >
> > Based on my analysis, this patch will produce correct behavior with the
> > event ring:
> >
> > http://marc.info/?l=linux-usb&m=138697807827548&w=2
> >
> > Am I missing something?
>
> > -/* Is this TRB a link TRB or was the last TRB the last TRB in this event
> > ring
> > - * segment? I.e. would the updated event TRB pointer step off the end of
> > the
> > - * event seg?
> > +/* Is this TRB a link TRB or was the last TRB in this event ring segment?
> > + * I.e. would the updated event TRB pointer step off the end of the event
> > + * seg?
> > */
> > static int last_trb(struct xhci_hcd *xhci, struct xhci_ring *ring,
> > struct xhci_segment *seg, union xhci_trb *trb)
> > {
> > if (ring == xhci->event_ring)
> > - return trb == &seg->trbs[TRBS_PER_SEGMENT];
> > + return trb == &seg->trbs[TRBS_PER_SEGMENT - 1];
> > else
> > return TRB_TYPE_LINK_LE32(trb->link.control);
> > }
>
>
> It would be extremely confusing for the above function to behave
> differently for event and other rings.
> Actually last_trb() could just be:
> return trb == seg->last_trb;
> provided an appropriate 'last_trb' field was set.
>
> If we assume that there are no empty ring segments (ie adjacent LINK TRBs),
> and that it is never actually called with a pointer to a link trb.
That second assumption is not true, unfortunately. inc_deq() is not the
only code that manipulates the ring->dequeue function. If we issue a
Set TR dequeue command to move the hardware dequeue pointer to a link
TRB (which we can do if the TD to be canceled ended just before the link
TRB), update_ring_for_set_deq_completion() will leave ring->dequeue
pointing to a link TRB. That means inc_deq() could be called with a
link TRB. So the code below would have to be changed to avoid updating
num_trbs_free in that case.
> Then the inc_deq() code could just be:
>
> ring->deq_updates++;
> ring->num_trbs_free++;
>
> if (last_trb(xhci, ring, ring->deq_seg, ++ring->dequeue)) {
> ring->deq_seg = ring->deq_seg->next;
> ring->dequeue = ring->deq_seg->trbs;
> if (ring->deq_seg == ring->first_seg) /* not sure of name */
> ring->cycle_state ^= 1;
> }
The cycle state is only changed when the enqueue pointer makes it past
the segment with the toggle bit. It shouldn't be modified for the
dequeue pointer.
And yes, inc_deq() could do with some simplification. David, are you
interested in creating a patch to simplify this code? I haven't gotten
v2 revisions from a couple of patches you previously sent, so I wasn't
sure if you're still interested in creating xHCI patches.
Sarah Sharp
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
