Re: [PATCH 5/5] usb: xhci: Rework and improve the TD matching and skipping logic

[Michal Pecio <michal.pecio@gmail.com>]

On Wed, 5 Aug 2026 20:39:12 +0300, Mathias Nyman wrote:
> On 8/4/26 13:05, Michal Pecio wrote:
> > 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.
> > 
> > While altering the error message, drop a pointless goto.
> > 
> > Signed-off-by: Michal Pecio <michal.pecio@gmail.com>  
> 
> 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.

> 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).

> 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.

> 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.

> 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;

This could equally well be (*missed_tds)++.

> 	}
> 
> 	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) {

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.

> 		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.

> 			if (td == passed_td)
> 				break;
> 		}
> 	}
> 
> -Mathias