xhci: Sort out the TD skipping business
Subsystems (2)
cover xhci: Sort out the TD skipping business
Hi,
This series is motivated by a recently found rare regression due to my commit from last year and the solution suggested by Mathias Nyman.
https://lore.kernel.org/linux-usb/al_hchyOdPoPWKEo@spiral/
I think it's a good solution not only for this specific case, but also in general, because the next event after Missed Service Error almost always references some TD - exceptions are Ring Underrun, which we have special handling for, and Stopped - Length Invalid, which would be a rare occurrence, not currently supported anyway, and possible to support within the proposed framework by making find_td_by_dma() calculate accurate 'missed_tds' value while still returning NULL.
The first two patches fix bugs, because I found another obscure one. The next two patches prepare for the last one by simplifying things.
The last patch implements the big change, the whole matching/skipping loop is replaced with a more straightforward and robust version. New functionality is paid for with a net increase of 4 LOC, not too bad.
I gave this a bit of testing and it seems to be working, including weird cases like: Missed Service Error retires a waiting TD with error_mid_td, then skipping is triggered by another Transaction Error immediately afterwards, and it turns out that two TDs were missed.
[ 1665.224921] xhci_hcd 0000:0a:00.0: Transfer error for slot 1 ep 2 on endpoint [ 1665.225151] xhci_hcd 0000:0a:00.0: Missed Service Error for slot 1 ep 2, skip 1, try now 0 [ 1665.225156] xhci_hcd 0000:0a:00.0: Missing TD completion event after mid TD error [ 1665.225305] xhci_hcd 0000:0a:00.0: Transfer error for slot 1 ep 2 on endpoint [ 1665.225308] xhci_hcd 0000:0a:00.0: Skipped 2 TDs on slot 1 ep 2 comp_code 4, TD found 1, skip flag 0
Additional testing of patch 1 in isolation would be appreciated from the reporter of the regression (Cc). That patch would go to v6.18.
Regards,
Michal
Michal Pecio (5):
usb: xhci: Handle bogus TRB pointers in Missed Service Error events
usb: xhci: Guarantee URB giveback on Ring Underrun/Overrun
usb: xhci: Don't set the skip flag on non-isoc endpoints
usb: xhci: Shorten the TD skipping loop
usb: xhci: Rework and improve the TD matching and skipping logic
drivers/usb/host/xhci-ring.c | 200 ++++++++++++++++++----------------- 1 file changed, 102 insertions(+), 98 deletions(-)
Signature · 1 line
2.48.1
PATCH 1/5 usb: xhci: Handle bogus TRB pointers in Missed Service Error events
xHCI 1.0 allowed these pointers to be zero. Some Intel chipsets from the era usually set it to zero, but sometimes (apparently) to the next TRB after the one referenced by the previous transfer event on the endpoint.
Usually that's indeed the missed TD, but it may also be the last TRB of a two-TRB TD already completed with Short Packet on its first TRB. Then the driver skips all pending TDs, failing to find a match.
When handling Missed Service Error, scan TD list twice and only really skip TDs in the second pass if the first pass found a match. This won't catch bogus pointers to wrong TDs, but such a bug would be practically impossible to detect automatically and isn't known to exist.
--- drivers/usb/host/xhci-ring.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)
drivers/usb/host/xhci-ring.c +16 -2
@@ -2605,6 +2605,17 @@ static bool xhci_spurious_success_tx_event(struct xhci_hcd *xhci, } } +static struct xhci_td *find_td_by_dma (struct xhci_ring *ep_ring,dma_addr_t dma) +{ +struct xhci_td *td; + +if (dma) +list_for_each_entry (td, &ep_ring->td_list, td_list) +if (trb_in_td (td, dma)) +return td; +return NULL ; +} +/* * If this function returns an error condition, it means it got a Transfer * event with a corrupted Slot ID, Endpoint ID, or TRB DMA address.
@@ -2799,8 +2810,11 @@ static int handle_tx_event(struct xhci_hcd *xhci,xhci_dequeue_td (xhci, td, ep_ring, td->status); } -/* If the TRB pointer is NULL, missed TDs will be skipped on the next event */ -if (trb_comp_code ==COMP_MISSED_SERVICE_ERROR && !ep_trb_dma) +/* +* We don't know how many TDs were missed when ep_trb_dma is zero (as permitted by +* xHCI 1.0) or bogus. Bail out leaving ep->skip set, next event will sort it out. +*/ +if (trb_comp_code ==COMP_MISSED_SERVICE_ERROR && !find_td_by_dma (ep_ring, ep_trb_dma))return 0 ;if (list_empty (&ep_ring->td_list)) {
Signature · 1 line
2.48.1
Re: [PATCH 1/5] usb: xhci: Handle bogus TRB pointers in Missed Service Error events
Quoted 13 lines · Michal Pecio
xHCI 1.0 allowed these pointers to be zero. Some Intel chipsets from the era usually set it to zero, but sometimes (apparently) to the next TRB after the one referenced by the previous transfer event on the endpoint.
Usually that's indeed the missed TD, but it may also be the last TRB of a two-TRB TD already completed with Short Packet on its first TRB. Then the driver skips all pending TDs, failing to find a match.
When handling Missed Service Error, scan TD list twice and only really skip TDs in the second pass if the first pass found a match. This won't catch bogus pointers to wrong TDs, but such a bug would be practically impossible to detect automatically and isn't known to exist.
Quoted 7 lines
Reported-by Bart Nagel <[email protected]>Suggested-by Mathias Nyman <[email protected]>Fixes d0b619599e52 ("usb: xhci: Expedite skipping missed isoch TDs on modern HCs")Signed-off-by Michal Pecio <[email protected]>---
Thanks, I'll send this [PATCH 1/5] forward to 7.3 with rest of patches in for-usb-next
-Mathias
PATCH 2/5 usb: xhci: Guarantee URB giveback on Ring Underrun/Overrun
In reply to Michal PecioIn this case we know that the xHC has released ownership of all missed TDs, we only don't know which were missed and which were queued later.
URBs are queued atomically, so we can safely give back all TDs of the currently executing URB. Unlike the previous policy, this does actually ensure that the class driver will learn about the error and won't see all of its URBs still in progress when all TDs are missed on xHCI 1.0.
--- drivers/usb/host/xhci-ring.c | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-)
drivers/usb/host/xhci-ring.c +19 -13
@@ -2630,6 +2630,7 @@ static int handle_tx_event(struct xhci_hcd *xhci,unsigned int slot_id;int ep_index;struct xhci_td *td =NULL ; +struct urb *missed_urb =NULL ;dma_addr_t ep_trb_dma;union xhci_trb *ep_trb;int status = -EINPROGRESS ;
@@ -2849,26 +2850,31 @@ static int handle_tx_event(struct xhci_hcd *xhci,return 0 ;/* -* TD was missed, skip it. Core already initialized frame->status -* to -EXDEV and frame->actual_length to 0, nothing more to do. +* If skip flag is still set at xrun, we are on xHCI 1.0 and our TRB +* pointer is zero again. All missed TDs can be given back, but we +* don't know which were missed and which were queued after the xrun +* occurred. We can safely give back the first pending URB. */ -xhci_dequeue_td (xhci, td, ep_ring,0 ); +if (ring_xrun_event) { +if (!missed_urb) + missed_urb = td->urb; -if (!list_empty (&ep_ring->td_list)) { -if (ring_xrun_event) { -/* -* If we are here, we are on xHCI 1.0 host with no -* idea how many TDs were missed or where the xrun -* occurred. New TDs may have been added after the -* xrun, so skip only one TD to be safe. -*/ -xhci_dbg (xhci,"Skipped one TD for slot %u ep %u" , +if (td->urb != missed_urb) { +xhci_dbg (xhci,"Skipped one URB for slot %u ep %u" , slot_id, ep_index);return 0 ; } -continue ; } +/* +* TD was missed, skip it. Core already initialized frame->status +* to -EXDEV and frame->actual_length to 0, nothing more to do. +*/ +xhci_dequeue_td (xhci, td, ep_ring,0 ); + +if (!list_empty (&ep_ring->td_list)) +continue ; +xhci_dbg (xhci,"All TDs skipped for slot %u ep %u. Clear skip flag.\n" , slot_id, ep_index); ep->skip = false;
Signature · 1 line
2.48.1
PATCH 3/5 usb: xhci: Don't set the skip flag on non-isoc endpoints
In reply to Michal PecioThese events are unique to isochronous endpoints, ignore them otherwise. Update debug messages to reflect new policies. We could also log invalid events as errors, but it seems nobody has ever had problems with that, so don't bother.
This allows dropping the isoc check when skipping TDs.
--- drivers/usb/host/xhci-ring.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)
drivers/usb/host/xhci-ring.c +9 -7
@@ -2759,16 +2759,18 @@ static int handle_tx_event(struct xhci_hcd *xhci,* Set skip flag of the ep_ring; Complete the missed tds as * short transfer when process the ep_ring next time. */ - ep->skip = true; +if (ep_ring->type ==TYPE_ISOC ) + ep->skip = true;xhci_dbg (xhci, -"Miss service interval error for slot %u ep %u, set skip flag%s\n" , - slot_id, ep_index, ep_trb_dma ?", skip now" :"" ); +"Missed Service Error for slot %u ep %u, skip %d, try now %d\n" , + slot_id, ep_index, ep->skip, !!ep_trb_dma);break ;case COMP_NO_PING_RESPONSE_ERROR : - ep->skip = true; +if (ep_ring->type ==TYPE_ISOC ) + ep->skip = true;xhci_dbg (xhci, -"No Ping response error for slot %u ep %u, Skip one Isoc TD\n" , - slot_id, ep_index); +"No Ping response error for slot %u ep %u, skip %d\n" , + slot_id, ep_index, ep->skip);return 0 ;case COMP_INCOMPATIBLE_DEVICE_ERROR :
@@ -2844,7 +2846,7 @@ static int handle_tx_event(struct xhci_hcd *xhci,/* Is this TRB not part of the currently executing TD? */ if (!trb_in_td (td, ep_trb_dma)) { -if (ep->skip &&usb_endpoint_xfer_isoc (&td->urb->ep->desc)) { +if (ep->skip) {/* this event is unlikely to match any TD, don't skip them all */ if (trb_comp_code ==COMP_STOPPED_LENGTH_INVALID )return 0 ;
Signature · 1 line
2.48.1
PATCH 4/5 usb: xhci: Shorten the TD skipping loop
In reply to Michal PecioHalf of this loop is code which only executes once to deal with cases where no TD matches the event and then it returns. This code needs not to be in any kind of loop, so get it out.
Optimize conditionals remaining in the loop body.
--- drivers/usb/host/xhci-ring.c | 69 +++++++++++++++++------------------- 1 file changed, 33 insertions(+), 36 deletions(-)
drivers/usb/host/xhci-ring.c +33 -36
@@ -2843,10 +2843,9 @@ static int handle_tx_event(struct xhci_hcd *xhci, td = list_first_entry(&ep_ring->td_list, struct xhci_td, td_list); -/* Is this TRB not part of the currently executing TD? */ -if (!trb_in_td (td, ep_trb_dma)) { +if (ep->skip) { -if (ep->skip) { +if (!trb_in_td (td, ep_trb_dma)) {/* this event is unlikely to match any TD, don't skip them all */ if (trb_comp_code ==COMP_STOPPED_LENGTH_INVALID )return 0 ;
@@ -2884,38 +2883,6 @@ static int handle_tx_event(struct xhci_hcd *xhci, goto check_endpoint_halted; } -/* TD was queued after xrun, maybe xrun was on a link, don't panic yet */ -if (ring_xrun_event) -return 0 ; - -/* -* Skip the Force Stopped Event. The 'ep_trb' of FSE is not in the current -* TD pointed by 'ep_ring->dequeue' because that the hardware dequeue -* pointer still at the previous TRB of the current TD. The previous TRB -* maybe a Link TD or the last TRB of the previous TD. The command -* completion handle will take care the rest. -*/ -if (trb_comp_code ==COMP_STOPPED || - trb_comp_code ==COMP_STOPPED_LENGTH_INVALID ) { -return 0 ; - } - -/* -* Some hosts give a spurious success event after a short -* transfer or error on last TRB. Ignore it. -*/ -if (xhci_spurious_success_tx_event (xhci, ep_ring)) { -xhci_dbg (xhci,"Spurious event dma %pad, comp_code %u after %u\n" , - &ep_trb_dma, trb_comp_code, ep_ring->old_trb_comp_code); - ep_ring->old_trb_comp_code =0 ; -return 0 ; - } - -/* HC is busted, give up! */ - goto debug_finding_td; - } - -if (ep->skip) {xhci_dbg (xhci,"Found td. Clear skip flag for slot %u ep %u.\n" , slot_id, ep_index);
@@ -2932,10 +2899,40 @@ static int handle_tx_event(struct xhci_hcd *xhci, ep_ring->old_trb_comp_code = trb_comp_code; -/* Get out if a TD was queued at enqueue after the xrun occurred */ +/* +* Underrun handling ends here. Any TD pointed by the event was enqueued after the event +* occurred, so wait for its completion now. And it's not a bug if no such TD exists. +*/ if (ring_xrun_event)return 0 ; +/* Handle events not referencing the current TD */ +if (!trb_in_td (td, ep_trb_dma)) { +/* +* Skip the Force Stopped Event. The 'ep_trb' of FSE is not in the current +* TD pointed by 'ep_ring->dequeue' because that the hardware dequeue +* pointer still at the previous TRB of the current TD. The previous TRB +* maybe a Link TD or the last TRB of the previous TD. The command +* completion handle will take care the rest. +*/ +if (trb_comp_code ==COMP_STOPPED || trb_comp_code ==COMP_STOPPED_LENGTH_INVALID ) +return 0 ; + +/* +* Some hosts give a spurious success event after a short +* transfer or error on last TRB. Ignore it. +*/ +if (xhci_spurious_success_tx_event (xhci, ep_ring)) { +xhci_dbg (xhci,"Spurious event dma %pad, comp_code %u after %u\n" , + &ep_trb_dma, trb_comp_code, ep_ring->old_trb_comp_code); + ep_ring->old_trb_comp_code =0 ; +return 0 ; + } + +/* HC is busted, give up! */ + goto debug_finding_td; + } +trace_xhci_handle_transfer (ep_ring, (struct xhci_generic_trb *) ep_trb, ep_trb_dma);/*
Signature · 1 line
2.48.1
Re: [PATCH 4/5] usb: xhci: Shorten the TD skipping loop
Quoted 7 lines · Michal Pecio
Half of this loop is code which only executes once to deal with cases where no TD matches the event and then it returns. This code needs not to be in any kind of loop, so get it out.
Optimize conditionals remaining in the loop body.
Signed-off-by Michal Pecio <[email protected]>
Much needed refactoring
Thanks
-Mathias
PATCH 5/5 usb: xhci: Rework and improve the TD matching and skipping logic
In reply to Michal PecioMatching events with TDs and giving back missed TDs is carried out by a complicated loop. Replace it with a simpler linear logic:
0. Having verified that 'td_list' isn't empty,
1. Scan it to find the matching TD and count missed TDs,
2. Perform necessary adjustments for corner cases,
3. Give back missed TDs, if applicable, using a short and tidy loop,
4. Check if the event refers to the expected TD and proceed as usual.
Besides cleaning up the code, this provides a few improvements:
- when the skip flag is set, no TD is given back unless we found a match or otherwise know how many TDs should be given back
- when the skip flag is clear, we know if the event refers to a "future" TD so we can log this in the Scary Error Message to aid debugging.
While altering the error message, drop a pointless goto.
--- drivers/usb/host/xhci-ring.c | 137 ++++++++++++++++------------------- 1 file changed, 61 insertions(+), 76 deletions(-)
drivers/usb/host/xhci-ring.c +61 -76
@@ -125,11 +125,16 @@ static bool link_trb_toggles_cycle(union xhci_trb *trb)return le32_to_cpu (trb->link.control) &LINK_TOGGLE ; } -static bool last_td_in_urb (struct xhci_td *td) +static int num_tds_not_done (struct urb *urb) { -struct urb_priv *urb_priv = td->urb->hcpriv; +struct urb_priv *urb_priv = urb->hcpriv; -return urb_priv->num_tds_done == urb_priv->num_tds; +return urb_priv->num_tds - urb_priv->num_tds_done; +} + +static bool last_td_in_urb (struct xhci_td *td) +{ +return !num_tds_not_done (td->urb); }static bool unhandled_event_trb (struct xhci_ring *ring)
@@ -2605,14 +2610,19 @@ static bool xhci_spurious_success_tx_event(struct xhci_hcd *xhci, } } -static struct xhci_td *find_td_by_dma (struct xhci_ring *ep_ring,dma_addr_t dma) +static struct xhci_td *find_td_by_dma (struct xhci_ring *ep_ring,int *missed_tds,dma_addr_t dma) {struct xhci_td *td; -if (dma) +if (dma) {list_for_each_entry (td, &ep_ring->td_list, td_list)if (trb_in_td (td, dma))return td; +else + (*missed_tds)++; + } + + *missed_tds =0 ;return NULL ; }
@@ -2629,8 +2639,8 @@ static int handle_tx_event(struct xhci_hcd *xhci,struct xhci_ring *ep_ring;unsigned int slot_id;int ep_index; -struct xhci_td *td =NULL ; -struct urb *missed_urb =NULL ; +struct xhci_td *td; +int missed_tds =0 ;dma_addr_t ep_trb_dma;union xhci_trb *ep_trb;int status = -EINPROGRESS ;
@@ -2813,13 +2823,6 @@ static int handle_tx_event(struct xhci_hcd *xhci,xhci_dequeue_td (xhci, td, ep_ring, td->status); } -/* -* We don't know how many TDs were missed when ep_trb_dma is zero (as permitted by -* xHCI 1.0) or bogus. Bail out leaving ep->skip set, next event will sort it out. -*/ -if (trb_comp_code ==COMP_MISSED_SERVICE_ERROR && !find_td_by_dma (ep_ring, ep_trb_dma)) -return 0 ; -if (list_empty (&ep_ring->td_list)) {/* * Don't print wanings if ring is empty due to a stopped endpoint generating an
@@ -2839,63 +2842,47 @@ static int handle_tx_event(struct xhci_hcd *xhci, goto check_endpoint_halted; } -do { - td = list_first_entry(&ep_ring->td_list, struct xhci_td, - td_list); - -if (ep->skip) { - -if (!trb_in_td (td, ep_trb_dma)) { -/* this event is unlikely to match any TD, don't skip them all */ -if (trb_comp_code ==COMP_STOPPED_LENGTH_INVALID ) -return 0 ; - -/* -* If skip flag is still set at xrun, we are on xHCI 1.0 and our TRB -* pointer is zero again. All missed TDs can be given back, but we -* don't know which were missed and which were queued after the xrun -* occurred. We can safely give back the first pending URB. -*/ -if (ring_xrun_event) { -if (!missed_urb) - missed_urb = td->urb; - -if (td->urb != missed_urb) { -xhci_dbg (xhci,"Skipped one URB for slot %u ep %u" , - slot_id, ep_index); -return 0 ; - } - } - -/* -* TD was missed, skip it. Core already initialized frame->status -* to -EXDEV and frame->actual_length to 0, nothing more to do. -*/ -xhci_dequeue_td (xhci, td, ep_ring,0 ); + td =find_td_by_dma (ep_ring, &missed_tds, ep_trb_dma); -if (!list_empty (&ep_ring->td_list)) -continue ; +if (ep->skip) { +if (!td) { +/* +* xHCI 1.0 allowed MSE events to have zero TRB pointers. Some old chips +* also generate bogus non-zero pointers. We know, don't bother warning. +* Missed TDs will be given back by the next event with a valid pointer. +*/ +if (trb_comp_code ==COMP_MISSED_SERVICE_ERROR && + xhci->hci_version <=0x100 ) +return 0 ; +/* +* If skip flag is still set at xrun, we are on xHCI 1.0 and our TRB pointer +* is zero again. All missed TDs can be given back, but we don't know which +* were missed and which were queued after the xrun occurred. We can safely +* give back the first pending URB to let the class driver know. +*/ +if (ring_xrun_event) + missed_tds =num_tds_not_done (list_first_entry (&ep_ring->td_list, + struct xhci_td, td_list)->urb); +/* In other cases missed_tds is zero */ + } -xhci_dbg (xhci,"All TDs skipped for slot %u ep %u. Clear skip flag.\n" , - slot_id, ep_index); - ep->skip = false; - td =NULL ; - goto check_endpoint_halted; - } +/* +* Give back missed TDs. Core already initialized their frame->status to -EXDEV +* and frame->actual_length to 0, nothing more to do. +*/ +for (int i =0 ; i < missed_tds; i++) +xhci_dequeue_td (xhci, + list_first_entry(&ep_ring->td_list, struct xhci_td, td_list), + ep_ring,0 ); -xhci_dbg (xhci, -"Found td. Clear skip flag for slot %u ep %u.\n" , - slot_id, ep_index); +/* the list may become empty on ring_xrun_event */ +if (td ||list_empty (&ep_ring->td_list)) ep->skip = false; - } -/* -* If ep->skip is set, it means there are missed tds on the -* endpoint ring need to take care of. -* Process them as short transfer until reach the td pointed by -* the event. -*/ - }while (ep->skip); +xhci_dbg (xhci,"Skipped %d TDs on slot %u ep %u comp_code %u, TD found %d, skip flag %d\n" , + missed_tds, slot_id, ep_index, trb_comp_code, !!td, ep->skip); + missed_tds =0 ; + } ep_ring->old_trb_comp_code = trb_comp_code;
@@ -2907,7 +2894,7 @@ static int handle_tx_event(struct xhci_hcd *xhci,return 0 ;/* Handle events not referencing the current TD */ -if (!trb_in_td (td, ep_trb_dma)) { +if (!td || missed_tds) {/* * Skip the Force Stopped Event. The 'ep_trb' of FSE is not in the current * TD pointed by 'ep_ring->dequeue' because that the hardware dequeue
@@ -2930,7 +2917,13 @@ static int handle_tx_event(struct xhci_hcd *xhci, }/* HC is busted, give up! */ - goto debug_finding_td; + td = list_first_entry(&ep_ring->td_list, struct xhci_td, td_list); +xhci_err (xhci,"Event dma %pad for ep %d comp_code %u not part of TD at %016llx - %016llx, missed %d\n" , + &ep_trb_dma, ep_index, trb_comp_code, + (u64 )xhci_trb_virt_to_dma (td->start_seg, td->start_trb), + (u64 )xhci_trb_virt_to_dma (td->end_seg, td->end_trb), + missed_tds); +return -ESHUTDOWN ; }trace_xhci_handle_transfer (ep_ring, (struct xhci_generic_trb *) ep_trb, ep_trb_dma);
@@ -2962,14 +2955,6 @@ static int handle_tx_event(struct xhci_hcd *xhci,return 0 ; -debug_finding_td: -xhci_err (xhci,"Event dma %pad for ep %d status %d not part of TD at %016llx - %016llx\n" , - &ep_trb_dma, ep_index, trb_comp_code, - (unsigned long long )xhci_trb_virt_to_dma (td->start_seg, td->start_trb), - (unsigned long long )xhci_trb_virt_to_dma (td->end_seg, td->end_trb)); - -return -ESHUTDOWN ; - err_out:xhci_err (xhci,"@%016llx %08x %08x %08x %08x\n" , (unsigned long long) xhci_trb_virt_to_dma(
Signature · 1 line
2.48.1
Re: [PATCH 5/5] usb: xhci: Rework and improve the TD matching and skipping logic
Quoted 15 lines · Michal Pecio
Matching events with TDs and giving back missed TDs is carried out by a complicated loop. Replace it with a simpler linear logic:
0. Having verified that '
td_list' isn't empty,
1. Scan it to find the matching TD and count missed TDs,
2. Perform necessary adjustments for corner cases,
3. Give back missed TDs, if applicable, using a short and tidy loop,
4. Check if the event refers to the expected TD and proceed as usual.Besides cleaning up the code, this provides a few improvements:
- when the skip flag is set, no TD is given back unless we found a match or otherwise know how many TDs should be given back
- when the skip flag is clear, we know if the event refers to a "future" TD so we can log this in the Scary Error Message to aid debugging.
Quoted 3 lines
While altering the error message, drop a pointless goto.
Signed-off-by Michal Pecio <[email protected]>
How about modifying step 1 a bit and store the last passed td instead of count missed tds?
If the event points to a valid trb ahead of last trb in td, but before the enqueue pointer, then we know hardware has passed this td and we can give it back.
This should work even if event trb points to a link trb or no-op trb.
It should also cover the "td->error_mid_td && !trb_in_td(td, ep_trb_dma))" case
something like:
static struct xhci_td *find_td_by_dma(struct xhci_ring *ring, struct xhci_td **passed_td, dma_addr_t dma)
{
struct xhci_td *td;
if (!dma)
return NULL;
list_for_each_entry(td, &ring->td_list, td_list) {
if (trb_in_td(td, dma))
return td;
/* event points to a valid trb passed this td */
else if (dma_in_range(dma, td->end_seg, td->end_trb,
ring->enq_seg, ring->enqueue))
*passed_td = td;
}
return NULL;
}static int handle_tx_event(struct xhci_hcd *xhci,
struct xhci_interrupter *ir, struct xhci_transfer_event *event)
{
...
struct xhci_td *passed_td = NULL;
...
td = find_td_by_dma(ep_ring, &passed_td, ep_trb_dma);
if (passed_td) {
struct xhci_td *tmp_td;
list_for_each_entry_safe(td, tmp_td, &ep_ring->td_list, td_list)
{
xhci_dequeue_td(xhci, td, ep_ring, td->status);
if (td == passed_td)
break;
}
}-Mathias
Re: [PATCH 5/5] usb: xhci: Rework and improve the TD matching and skipping logic
Quoted 16 lines · Mathias Nyman
Quoted 15 lines · Michal Pecio
Matching events with TDs and giving back missed TDs is carried out by a complicated loop. Replace it with a simpler linear logic:
0. Having verified that '
td_list' isn't empty,
1. Scan it to find the matching TD and count missed TDs,
2. Perform necessary adjustments for corner cases,
3. Give back missed TDs, if applicable, using a short and tidy loop,
4. Check if the event refers to the expected TD and proceed as usual.Besides cleaning up the code, this provides a few improvements:
- when the skip flag is set, no TD is given back unless we found a match or otherwise know how many TDs should be given back
- when the skip flag is clear, we know if the event refers to a "future" TD so we can log this in the Scary Error Message to aid debugging.
Quoted 6 lines
Quoted 3 lines
While altering the error message, drop a pointless goto.
Signed-off-by Michal Pecio <[email protected]>How about modifying step 1 a bit and store the last passed td instead of count missed tds?
End result would be about the same, but I think counting results in the simplest possible giveback loop. And I can easily log the number of dropped TDs, which is something I was sometimes curious to know.
Quoted 3 lines
If the event points to a valid trb ahead of last trb in td, but before the enqueue pointer, then we know hardware has passed this td and we can give it back.
I mentioned possibility of such change in the cover letter, but kept it out of this patch, because it's something that has never been done before and, like "Expedite skipping missed TDs on modern hosts", it could bring unwanted side effects on less than perfect HW.
For the record, I know that ASMedia can sporadically generate Stopped events with semi-random, bogus TRB pointers, at least with streams:
[ 4541.443642] xhci_hcd 0000:02:00.0: Transfer event 26 for unknown stream ring slot 5 ep 6
But not sure if with streams only. Without streams, the driver has no trouble finding the transfer ring and silently ignores bogus Stopped events. If they (hypothetically) accidentally match some TD, the only result is updating urb->actual_length (which may even get fixed later if the TD receives a proper event).
Quoted 1 line
This should work even if event trb points to a link trb or no-op trb.
Counting could do it too. I consider the two above issues separate.
Quoted 1 line
It should also cover the "
td->error_mid_td&& !trb_in_td(td,ep_trb_dma))" case
Now, that's a third separate issue :)
I had this idea that isoc error could immediately give back the TD to minimize latency, and only keep around enough metadata (basically td->end_trb) to recognize future events and confidently ignore them. I even sent a patch, but it was dropped due to last minute conflicts.
Quoted 16 lines
something like:
static struct
xhci_td*find_td_by_dma(structxhci_ring*ring, structxhci_td**passed_td,dma_addr_tdma){ struct xhci_td *td; if (!dma) return NULL; list_for_each_entry(td, &ring->td_list, td_list) { if (trb_in_td(td, dma)) return td; /* event points to a valid trb passed this td */ else if (dma_in_range(dma, td->end_seg, td->end_trb, ring->enq_seg, ring->enqueue)) *passed_td = td;
This could equally well be (*missed_tds)++.
Quoted 8 lines
} return NULL; }static int handle_tx_event(struct
xhci_hcd*xhci,
structxhci_interrupter*ir, structxhci_transfer_event*event)
Quoted 8 lines
{
...
struct xhci_td *passed_td = NULL;
...
td = find_td_by_dma(ep_ring, &passed_td, ep_trb_dma);
if (passed_td) {This seems to be a fourth independent change - getting rid of ep->skip. Not sure if worthwile, the cost of managing ep->skip state is just a few LOC, and it provides some degree of sanity checking.
It would be sad if an avoidable SW bug went unnoticed until it causes a "user impact", because the driver silently ignored obvious problems. Tolerating bugs that don't exist seems to invite people to create them.
Quoted 4 lines
struct xhci_td *tmp_td;
list_for_each_entry_safe(td, tmp_td, &ep_ring->td_list, td_list)
{
xhci_dequeue_td(xhci, td, ep_ring, td->status);On isoc, you can replace td->status with 0, it will be ignored.
Non-isoc - is dropping TDs like that a can of worms worth opening? Also, this td->status would be assigned to urb->status, so it could become zero, EINPROGRESS, or whatever the initial value is.
Quoted 6 lines
if (td == passed_td) break; } }-Mathias
Re: [PATCH 5/5] usb: xhci: Rework and improve the TD matching and skipping logic
Quoted 4 lines · Mathias Nyman
If the event points to a valid trb ahead of last trb in td, but before the enqueue pointer, then we know hardware has passed this td and we can give it back.
Actually, we don't, because of a race condition (and driver bug).
A very long TD completes with Short Packet on the first TRB. We give back the TD and advance ep_ring->dequeue past it (bug). Some event is generated later in the TD but we don't know yet. We queue a lot of TRBs and move enqueue into the completed TD.
We handle the event and it appears to be "before" enqueue. But the HW not only hasn't passed, it hasn't even begun executing any TD after the initially completed one.
We give them back and later get their events, or IOMMU faults.
Regards,
Michal
Re: [PATCH 5/5] usb: xhci: Rework and improve the TD matching and skipping logic
Quoted 12 lines · Michal Pecio
Quoted 4 lines · Mathias Nyman
If the event points to a valid trb ahead of last trb in td, but before the enqueue pointer, then we know hardware has passed this td and we can give it back.
Actually, we don't, because of a race condition (and driver bug).
A very long TD completes with Short Packet on the first TRB. We give back the TD and advance
ep_ring->dequeuepast it (bug). Some event is generated later in the TD but we don't know yet. We queue a lot of TRBs and move enqueue into the completed TD.
Quoted 6 lines
We handle the event and it appears to be "before" enqueue. But the HW not only hasn't passed, it hasn't even begun executing any TD after the initially completed one.
We give them back and later get their events, or IOMMU faults.
This isn't a realistic scenario.
Ring expansion needs to fail and class driver need to fill entire ring buffer in the extremely short time between the short transfer event mid TD, and the final completion event for the same TD.
xHC isn't really processing the rest of the TD's TRBs (no data transfer) after a short transfer event.
A second event for that TD will likely point to the last TRB as it has the IOC flag set, and that event will come almost instantly, probably so close that both events are handled during one interrupt handler call.
We would already now see severe issues if this could happen. Queuing TRBs beyond hw dequeue on a full ring would overwrite the cycle bit and hardware would likely just stop processing any TRBs
I do support changing the software dequeue to better match hardware dequeue.
Thanks
Mathias
At 2026-08-04 12:01:10 +0200, Michal Pecio wrote:
...
Quoted 2 lines
Additional testing of patch 1 in isolation would be appreciated from the reporter of the regression (Cc). That patch would go to v6.18.
That's me! I'm new here so please forgive me if I'm asking stupid questions:
I found the patch applies cleanly to the linux-6.18.y branch in the stable repo; am I in the right place?
And then am I looking for anything in particular in the logs while running it, or do you simply want an observation on whether I can reproduce the bug or not?
Thanks.
Quoted 10 lines · Bart Nagel
At 2026-08-04 12:01:10 +0200, Michal Pecio wrote:
...Quoted 2 lines
Additional testing of patch 1 in isolation would be appreciated from the reporter of the regression (Cc). That patch would go to v6.18.
That's me! I'm new here so please forgive me if I'm asking stupid questions:
I found the patch applies cleanly to the linux-6.18.y branch in the stable repo; am I in the right place?
That's fine, there were hardly any changes in this area this year so 6.18 should be about equivalent to any 7.x. And it's the only one of them which will stay supported (here) for a few years.
Quoted 3 lines
And then am I looking for anything in particular in the logs while running it, or do you simply want an observation on whether I can reproduce the bug or not?
At this point just see if it works. These patches don't affect HW behavior, they only ignore bogus events to avoid creating problems. And this patch should ignore every case ignored by the old one.
Regard,
Michal
Quoted 16 lines · Michal Pecio
Quoted 10 lines · Bart Nagel
At 2026-08-04 12:01:10 +0200, Michal Pecio wrote:
...Quoted 2 lines
Additional testing of patch 1 in isolation would be appreciated from the reporter of the regression (Cc). That patch would go to v6.18.
That's me! I'm new here so please forgive me if I'm asking stupid questions:
I found the patch applies cleanly to the linux-6.18.y branch in the stable repo; am I in the right place?
That's fine, there were hardly any changes in this area this year so 6.18 should be about equivalent to any 7.x. And it's the only one of them which will stay supported (here) for a few years.
Quoted 7 lines
Quoted 3 lines
And then am I looking for anything in particular in the logs while running it, or do you simply want an observation on whether I can reproduce the bug or not?
At this point just see if it works. These patches don't affect HW behavior, they only ignore bogus events to avoid creating problems. And this patch should ignore every case ignored by the old one.
OK, I ran it with high system load (which as discussed earlier made the bug pop up more readily) for a little over 12 hours, and no freeze.
In case logs are interesting, they are temporarily (~48h) here:
dmesg log with handle_tx_event, but with "spurious event" messages filtered out otherwise the file is 400 times the size:
https://tmpfiles.org/wvw5Fxo00DQt/dmesg-6.18.42-patch-2026-08-04-no-spurious.txt
Filtered debugfs trace as directed by Michal earlier in the process:
https://tmpfiles.org/w2wTFcoQ02zV/trace-6.18.42-patch-2026-08-04.txt
Thanks for all your work.