Re: [PATCH 3/3] usb: xhci: Unify duplicate inc_enq() code

[Michał Pecio]

On Fri, 21 Feb 2025 16:54:11 +0200, Mathias Nyman wrote:
> On 21.2.2025 0.47, Michal Pecio wrote:
> > Remove a block of code copied from inc_enq(). As often happens, the
> > two copies have diverged somewhat - in this case, inc_enq() has a
> > bug where it may leave the chain bit of a link TRB unset on a
> > quirky HC. Fix this. Remove the pointless 'next' variable which
> > only aliases ring->enqueue.  
> 
> The linnk TRB chain bit should be set when the ring is created, and
> never cleared on those quirky hosts.

OK, I see, there is stuff in xhci-mem.c. I'll remove the above text
and any code which touches the bit on quirky HCs.

Speaking of which, I have some evidence that NEC uPD720200 has the
exact same bug as AMD, namely after a Missed Service Error near the
end of a segment it fetches TRBs out of bounds and trips the IOMMU
or stops with Ring Underrun. Link chain quirk seems to fix it.

> maybe
> 
> if (trb_is_link(ring->enqueue) && (chain || more_trbs_coming))
> 	inc_eng_past_link(xhci, ring, chain);
> 
> Avoids calling inc_enq_past_link() every time we increase enqueue,
> and explains why we call it.

I can do that too. By the way, do we actually want this while loop in
inc_enq_past_link() at all? Currently links only exist at the end of a
segment and always point to the beginning of the next segment.

I noticed that per xHCI 4.11.7, "Software shall not define consecutive
Link TRBs within a TD". I suppose "consecutive" means "one pointing to
another". And if it's prohibited inside a TD, it will likely always be
easier to avoid doing it at all than try to manage special cases.

Regards,
Michal