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

[Mathias Nyman]

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@xxxxxxxxx>

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