| Message ID | 20250220234719.5dc47877@foxbook (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | xhci: ring queuing cleanups | expand |
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. > > Note: I don't know if any 0.95 xHC ever reached series production, but > "AMD 0.96 host" appears to be the "Llano" family APU. Example dmesg at > https://linux-hardware.org/?probe=79d5cfd4fd&log=dmesg > > pci 0000:00:10.0: [1022:7812] type 00 class 0x0c0330 > hcc params 0x014042c3 hci version 0x96 quirks 0x0000000000000608 > > Signed-off-by: Michal Pecio <michal.pecio@gmail.com> > --- > drivers/usb/host/xhci-ring.c | 135 +++++++++++++++-------------------- > 1 file changed, 58 insertions(+), 77 deletions(-) > > diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c > index 46ca98066856..f400ba85ebc5 100644 > --- a/drivers/usb/host/xhci-ring.c > +++ b/drivers/usb/host/xhci-ring.c > @@ -208,6 +208,52 @@ void inc_deq(struct xhci_hcd *xhci, struct xhci_ring *ring) > return; > } > > +/* > + * If enqueue points at a link TRB, follow links until an ordinary TRB is reached. > + * Toggle the cycle bit of passed link TRBs and optionally chain them. > + */ > +static void inc_enq_past_link(struct xhci_hcd *xhci, struct xhci_ring *ring, u32 chain) > +{ > + unsigned int link_trb_count = 0; > + > + while (trb_is_link(ring->enqueue)) { > + > + /* > + * Section 6.4.4.1 of the 0.95 spec says link TRBs cannot have the chain bit > + * set, but other sections talk about dealing with the chain bit set. This was > + * fixed in the 0.96 specification errata, but we have to assume that all 0.95 > + * xHCI hardware can't handle the chain bit being cleared on a link TRB. > + * > + * If we're dealing with 0.95 hardware or isoc rings on AMD 0.96 host, override > + * the chain bit to always be set. > + */ > + if (!xhci_link_chain_quirk(xhci, ring->type)) { > + ring->enqueue->link.control &= cpu_to_le32(~TRB_CHAIN); > + ring->enqueue->link.control |= cpu_to_le32(chain); > + } else { > + ring->enqueue->link.control |= cpu_to_le32(TRB_CHAIN); > + } > + > + /* Give this link TRB to the hardware */ > + wmb(); > + ring->enqueue->link.control ^= cpu_to_le32(TRB_CYCLE); > + > + /* Toggle the cycle bit after the last ring segment. */ > + if (link_trb_toggles_cycle(ring->enqueue)) > + ring->cycle_state ^= 1; > + > + ring->enq_seg = ring->enq_seg->next; > + ring->enqueue = ring->enq_seg->trbs; > + > + trace_xhci_inc_enq(ring); > + > + if (link_trb_count++ > ring->num_segs) { > + xhci_warn(xhci, "Link TRB loop at enqueue\n"); > + break; > + } > + } > +} > + > /* > * See Cycle bit rules. SW is the consumer for the event ring only. > * > @@ -216,11 +262,6 @@ void inc_deq(struct xhci_hcd *xhci, struct xhci_ring *ring) > * If we've enqueued the last TRB in a TD, make sure the following link TRBs > * have their chain bit cleared (so that each Link TRB is a separate TD). > * > - * Section 6.4.4.1 of the 0.95 spec says link TRBs cannot have the chain bit > - * set, but other sections talk about dealing with the chain bit set. This was > - * fixed in the 0.96 specification errata, but we have to assume that all 0.95 > - * xHCI hardware can't handle the chain bit being cleared on a link TRB. > - * > * @more_trbs_coming: Will you enqueue more TRBs before calling > * prepare_transfer()? > */ > @@ -228,8 +269,6 @@ static void inc_enq(struct xhci_hcd *xhci, struct xhci_ring *ring, > bool more_trbs_coming) > { > u32 chain; > - union xhci_trb *next; > - unsigned int link_trb_count = 0; > > chain = le32_to_cpu(ring->enqueue->generic.field[3]) & TRB_CHAIN; > > @@ -238,48 +277,16 @@ static void inc_enq(struct xhci_hcd *xhci, struct xhci_ring *ring, > return; > } > > - next = ++(ring->enqueue); > - > - /* Update the dequeue pointer further if that was a link TRB */ > - while (trb_is_link(next)) { > - > - /* > - * If the caller doesn't plan on enqueueing more TDs before > - * ringing the doorbell, then we don't want to give the link TRB > - * to the hardware just yet. We'll give the link TRB back in > - * prepare_ring() just before we enqueue the TD at the top of > - * the ring. > - */ > - if (!chain && !more_trbs_coming) > - break; > - > - /* If we're not dealing with 0.95 hardware or isoc rings on > - * AMD 0.96 host, carry over the chain bit of the previous TRB > - * (which may mean the chain bit is cleared). > - */ > - if (!xhci_link_chain_quirk(xhci, ring->type)) { > - next->link.control &= cpu_to_le32(~TRB_CHAIN); > - next->link.control |= cpu_to_le32(chain); > - } > - /* Give this link TRB to the hardware */ > - wmb(); > - next->link.control ^= cpu_to_le32(TRB_CYCLE); > - > - /* Toggle the cycle bit after the last ring segment. */ > - if (link_trb_toggles_cycle(next)) > - ring->cycle_state ^= 1; > - > - ring->enq_seg = ring->enq_seg->next; > - ring->enqueue = ring->enq_seg->trbs; > - next = ring->enqueue; > - > - trace_xhci_inc_enq(ring); > - > - if (link_trb_count++ > ring->num_segs) { > - xhci_warn(xhci, "%s: Ring link TRB loop\n", __func__); > - break; > - } > - } > + ring->enqueue++; > + > + /* > + * If we are in the middle of a TD or the caller plans to enqueue more > + * TDs as one transfer (eg. control), traverse any link TRBs right now. > + * Otherwise, enqueue can stay on a link until the next prepare_ring(). > + * This avoids enqueue entering deq_seg and simplifies ring expansion. > + */ > + if (chain || more_trbs_coming)> + inc_enq_past_link(xhci, ring, chain); 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. > } > > /* > @@ -3177,7 +3184,6 @@ static void queue_trb(struct xhci_hcd *xhci, struct xhci_ring *ring, > static int prepare_ring(struct xhci_hcd *xhci, struct xhci_ring *ep_ring, > u32 ep_state, unsigned int num_trbs, gfp_t mem_flags) > { > - unsigned int link_trb_count = 0; > unsigned int new_segs = 0; > > /* Make sure the endpoint has been added to xHC schedule */ > @@ -3225,33 +3231,8 @@ static int prepare_ring(struct xhci_hcd *xhci, struct xhci_ring *ep_ring, > } > } > > - while (trb_is_link(ep_ring->enqueue)) { > - /* If we're not dealing with 0.95 hardware or isoc rings > - * on AMD 0.96 host, clear the chain bit. > - */ > - if (!xhci_link_chain_quirk(xhci, ep_ring->type)) > - ep_ring->enqueue->link.control &= > - cpu_to_le32(~TRB_CHAIN); > - else > - ep_ring->enqueue->link.control |= > - cpu_to_le32(TRB_CHAIN); > - > - wmb(); > - ep_ring->enqueue->link.control ^= cpu_to_le32(TRB_CYCLE); > - > - /* Toggle the cycle bit after the last ring segment. */ > - if (link_trb_toggles_cycle(ep_ring->enqueue)) > - ep_ring->cycle_state ^= 1; > - > - ep_ring->enq_seg = ep_ring->enq_seg->next; > - ep_ring->enqueue = ep_ring->enq_seg->trbs; > - > - /* prevent infinite loop if all first trbs are link trbs */ > - if (link_trb_count++ > ep_ring->num_segs) { > - xhci_warn(xhci, "Ring is an endless link TRB loop\n"); > - return -EINVAL; > - } > - } > + /* Ensure that new TRBs won't overwrite a link */ Maybe add: if (trb_is_link(ep_ring->enqueue)) check here as well > + inc_enq_past_link(xhci, ep_ring, 0); > > if (last_trb_on_seg(ep_ring->enq_seg, ep_ring->enqueue)) { > xhci_warn(xhci, "Missing link TRB at end of ring segment\n"); Otherwise this is a nice cleanup, and the chain bit seems correct, in short: if quirky host, set chain bit in link TRB before a TD, otherwise clear it. if quirky host, set chain bit in link TRB mid TD, otherwise set chain bit to same value as previous TRB.
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
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
On Mon, 24 Feb 2025 13:49:29 +0200, Mathias Nyman wrote: > Interesting, I wonder if setting the link TRB chain bit would > also help with the TRB prefetch issue on VIA VL805 hosts. Good idea, but unfortunately not. With xhci_hcd quirks=1, which is XHCI_LINK_TRB_QUIRK: [ 0.543465] pci 0000:0a:00.0: [1106:3483] type 00 class 0x0c0330 PCIe Endpoint [ 406.745905] xhci_hcd 0000:0a:00.0: xHCI Host Controller [ 406.745916] xhci_hcd 0000:0a:00.0: new USB bus registered, assigned bus number 11 [ 406.747265] xhci_hcd 0000:0a:00.0: hcc params 0x002841eb hci version 0x100 quirks 0x0000000000000891 [ 407.475672] usb 11-1.4: Found UVC 1.00 device USB2.0 Camera (1e4e:0103) [ 407.697768] usb 11-1.4: Selecting alternate setting 12 (3060 B/frame bandwidth) [ 407.700432] usb 11-1.4: Allocated 5 URB buffers of 32x3060 bytes each [ 407.732047] xhci_hcd 0000:0a:00.0: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x0005 address=0xffccc000 flags=0x0000] [ 407.732122] xhci_hcd 0000:0a:00.0: WARNING: Host System Error [ 407.732133] xhci_hcd 0000:0a:00.0: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x0005 address=0xffccc000 flags=0x0000] [ 407.732151] xhci_hcd 0000:0a:00.0: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x0005 address=0xffccc000 flags=0x0000] Link TRBs from debugfs: 0 0x00000000ffccbff0: LINK 00000000ffcca000 intr 0 type 'Link' flags i:C:t:C 1 0x00000000ffccaff0: LINK 00000000ffccb000 intr 0 type 'Link' flags i:C:T:c
On 24.2.2025 23.01, Michał Pecio wrote: > On Mon, 24 Feb 2025 13:49:29 +0200, Mathias Nyman wrote: >> Interesting, I wonder if setting the link TRB chain bit would >> also help with the TRB prefetch issue on VIA VL805 hosts. > > Good idea, but unfortunately not. > > With xhci_hcd quirks=1, which is XHCI_LINK_TRB_QUIRK: > > [ 0.543465] pci 0000:0a:00.0: [1106:3483] type 00 class 0x0c0330 PCIe Endpoint > > [ 406.745905] xhci_hcd 0000:0a:00.0: xHCI Host Controller > [ 406.745916] xhci_hcd 0000:0a:00.0: new USB bus registered, assigned bus number 11 > [ 406.747265] xhci_hcd 0000:0a:00.0: hcc params 0x002841eb hci version 0x100 quirks 0x0000000000000891 > > [ 407.475672] usb 11-1.4: Found UVC 1.00 device USB2.0 Camera (1e4e:0103) > > [ 407.697768] usb 11-1.4: Selecting alternate setting 12 (3060 B/frame bandwidth) > [ 407.700432] usb 11-1.4: Allocated 5 URB buffers of 32x3060 bytes each > [ 407.732047] xhci_hcd 0000:0a:00.0: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x0005 address=0xffccc000 flags=0x0000] > [ 407.732122] xhci_hcd 0000:0a:00.0: WARNING: Host System Error > [ 407.732133] xhci_hcd 0000:0a:00.0: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x0005 address=0xffccc000 flags=0x0000] > [ 407.732151] xhci_hcd 0000:0a:00.0: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x0005 address=0xffccc000 flags=0x0000] > > Link TRBs from debugfs: > > 0 0x00000000ffccbff0: LINK 00000000ffcca000 intr 0 type 'Link' flags i:C:t:C > 1 0x00000000ffccaff0: LINK 00000000ffccb000 intr 0 type 'Link' flags i:C:T:c It was worth a shot, thanks for trying it out I'll send the original fix to Greg -Mathias
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c index 46ca98066856..f400ba85ebc5 100644 --- a/drivers/usb/host/xhci-ring.c +++ b/drivers/usb/host/xhci-ring.c @@ -208,6 +208,52 @@ void inc_deq(struct xhci_hcd *xhci, struct xhci_ring *ring) return; } +/* + * If enqueue points at a link TRB, follow links until an ordinary TRB is reached. + * Toggle the cycle bit of passed link TRBs and optionally chain them. + */ +static void inc_enq_past_link(struct xhci_hcd *xhci, struct xhci_ring *ring, u32 chain) +{ + unsigned int link_trb_count = 0; + + while (trb_is_link(ring->enqueue)) { + + /* + * Section 6.4.4.1 of the 0.95 spec says link TRBs cannot have the chain bit + * set, but other sections talk about dealing with the chain bit set. This was + * fixed in the 0.96 specification errata, but we have to assume that all 0.95 + * xHCI hardware can't handle the chain bit being cleared on a link TRB. + * + * If we're dealing with 0.95 hardware or isoc rings on AMD 0.96 host, override + * the chain bit to always be set. + */ + if (!xhci_link_chain_quirk(xhci, ring->type)) { + ring->enqueue->link.control &= cpu_to_le32(~TRB_CHAIN); + ring->enqueue->link.control |= cpu_to_le32(chain); + } else { + ring->enqueue->link.control |= cpu_to_le32(TRB_CHAIN); + } + + /* Give this link TRB to the hardware */ + wmb(); + ring->enqueue->link.control ^= cpu_to_le32(TRB_CYCLE); + + /* Toggle the cycle bit after the last ring segment. */ + if (link_trb_toggles_cycle(ring->enqueue)) + ring->cycle_state ^= 1; + + ring->enq_seg = ring->enq_seg->next; + ring->enqueue = ring->enq_seg->trbs; + + trace_xhci_inc_enq(ring); + + if (link_trb_count++ > ring->num_segs) { + xhci_warn(xhci, "Link TRB loop at enqueue\n"); + break; + } + } +} + /* * See Cycle bit rules. SW is the consumer for the event ring only. * @@ -216,11 +262,6 @@ void inc_deq(struct xhci_hcd *xhci, struct xhci_ring *ring) * If we've enqueued the last TRB in a TD, make sure the following link TRBs * have their chain bit cleared (so that each Link TRB is a separate TD). * - * Section 6.4.4.1 of the 0.95 spec says link TRBs cannot have the chain bit - * set, but other sections talk about dealing with the chain bit set. This was - * fixed in the 0.96 specification errata, but we have to assume that all 0.95 - * xHCI hardware can't handle the chain bit being cleared on a link TRB. - * * @more_trbs_coming: Will you enqueue more TRBs before calling * prepare_transfer()? */ @@ -228,8 +269,6 @@ static void inc_enq(struct xhci_hcd *xhci, struct xhci_ring *ring, bool more_trbs_coming) { u32 chain; - union xhci_trb *next; - unsigned int link_trb_count = 0; chain = le32_to_cpu(ring->enqueue->generic.field[3]) & TRB_CHAIN; @@ -238,48 +277,16 @@ static void inc_enq(struct xhci_hcd *xhci, struct xhci_ring *ring, return; } - next = ++(ring->enqueue); - - /* Update the dequeue pointer further if that was a link TRB */ - while (trb_is_link(next)) { - - /* - * If the caller doesn't plan on enqueueing more TDs before - * ringing the doorbell, then we don't want to give the link TRB - * to the hardware just yet. We'll give the link TRB back in - * prepare_ring() just before we enqueue the TD at the top of - * the ring. - */ - if (!chain && !more_trbs_coming) - break; - - /* If we're not dealing with 0.95 hardware or isoc rings on - * AMD 0.96 host, carry over the chain bit of the previous TRB - * (which may mean the chain bit is cleared). - */ - if (!xhci_link_chain_quirk(xhci, ring->type)) { - next->link.control &= cpu_to_le32(~TRB_CHAIN); - next->link.control |= cpu_to_le32(chain); - } - /* Give this link TRB to the hardware */ - wmb(); - next->link.control ^= cpu_to_le32(TRB_CYCLE); - - /* Toggle the cycle bit after the last ring segment. */ - if (link_trb_toggles_cycle(next)) - ring->cycle_state ^= 1; - - ring->enq_seg = ring->enq_seg->next; - ring->enqueue = ring->enq_seg->trbs; - next = ring->enqueue; - - trace_xhci_inc_enq(ring); - - if (link_trb_count++ > ring->num_segs) { - xhci_warn(xhci, "%s: Ring link TRB loop\n", __func__); - break; - } - } + ring->enqueue++; + + /* + * If we are in the middle of a TD or the caller plans to enqueue more + * TDs as one transfer (eg. control), traverse any link TRBs right now. + * Otherwise, enqueue can stay on a link until the next prepare_ring(). + * This avoids enqueue entering deq_seg and simplifies ring expansion. + */ + if (chain || more_trbs_coming) + inc_enq_past_link(xhci, ring, chain); } /* @@ -3177,7 +3184,6 @@ static void queue_trb(struct xhci_hcd *xhci, struct xhci_ring *ring, static int prepare_ring(struct xhci_hcd *xhci, struct xhci_ring *ep_ring, u32 ep_state, unsigned int num_trbs, gfp_t mem_flags) { - unsigned int link_trb_count = 0; unsigned int new_segs = 0; /* Make sure the endpoint has been added to xHC schedule */ @@ -3225,33 +3231,8 @@ static int prepare_ring(struct xhci_hcd *xhci, struct xhci_ring *ep_ring, } } - while (trb_is_link(ep_ring->enqueue)) { - /* If we're not dealing with 0.95 hardware or isoc rings - * on AMD 0.96 host, clear the chain bit. - */ - if (!xhci_link_chain_quirk(xhci, ep_ring->type)) - ep_ring->enqueue->link.control &= - cpu_to_le32(~TRB_CHAIN); - else - ep_ring->enqueue->link.control |= - cpu_to_le32(TRB_CHAIN); - - wmb(); - ep_ring->enqueue->link.control ^= cpu_to_le32(TRB_CYCLE); - - /* Toggle the cycle bit after the last ring segment. */ - if (link_trb_toggles_cycle(ep_ring->enqueue)) - ep_ring->cycle_state ^= 1; - - ep_ring->enq_seg = ep_ring->enq_seg->next; - ep_ring->enqueue = ep_ring->enq_seg->trbs; - - /* prevent infinite loop if all first trbs are link trbs */ - if (link_trb_count++ > ep_ring->num_segs) { - xhci_warn(xhci, "Ring is an endless link TRB loop\n"); - return -EINVAL; - } - } + /* Ensure that new TRBs won't overwrite a link */ + inc_enq_past_link(xhci, ep_ring, 0); if (last_trb_on_seg(ep_ring->enq_seg, ep_ring->enqueue)) { xhci_warn(xhci, "Missing link TRB at end of ring segment\n");
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. Note: I don't know if any 0.95 xHC ever reached series production, but "AMD 0.96 host" appears to be the "Llano" family APU. Example dmesg at https://linux-hardware.org/?probe=79d5cfd4fd&log=dmesg pci 0000:00:10.0: [1022:7812] type 00 class 0x0c0330 hcc params 0x014042c3 hci version 0x96 quirks 0x0000000000000608 Signed-off-by: Michal Pecio <michal.pecio@gmail.com> --- drivers/usb/host/xhci-ring.c | 135 +++++++++++++++-------------------- 1 file changed, 58 insertions(+), 77 deletions(-)