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 majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
