On 24.2.2025 1.45, Michał Pecio wrote:
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.
Interesting, I wonder if setting the link TRB chain bit would
also help with the TRB prefetch issue on VIA VL805 hosts.
Maybe we could avoid allocating the dummy page by just setting this
quirk instead.
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.
I don't think that loop is needed. The xhci driver is the only creator
of link TRBs, and at the moment they are placed exactly as you said.
Thanks
Mathias
