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
