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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] usb: xhci: handle_tx_event() halts >1.0 hosts on bogus MSE pointers
--

commit cc3554d902e447131e844c7826abc9b1b4f75f82
Author: Michal Pecio <michal.pecio@gmail.com>

    usb: xhci: Rework and improve the TD matching and skipping logic
    
    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.

> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 7597ef8105c65..243b1fd2b2f68 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
[ ... ]
> @@ -2858,66 +2861,50 @@ static int handle_tx_event(struct xhci_hcd *xhci,
[ ... ]
> +	td = find_td_by_dma(ep_ring, &missed_tds, ep_trb_dma);
>  
> -			xhci_dbg(xhci,
> -				 "Found td. Clear skip flag for slot %u ep %u.\n",
> -				 slot_id, ep_index);
> +	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;

[Severity: High]
Does this code inadvertently halt hosts > 1.0 on bogus MSE pointers?

By restricting the leniency for bogus Missed Service Error (MSE) pointers
strictly to xHCI 1.0, controllers > 1.0 that generate a bogus pointer for MSE
will fall through and hit the "HC is busted" block below.

[ ... ]
>  	/* Handle events not referencing the current TD */
> -	if (!trb_in_td(td, ep_trb_dma)) {
> +	if (!td || missed_tds) {
>  		/* TD was queued after xrun, maybe xrun was on a link, don't panic yet */
>  		if (ring_xrun_event)
>  			return 0;
[ ... ]
>  		/* 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;
>  	}

[Severity: High]
Will falling through to this block cause the host controller to fatally shut
down (-ESHUTDOWN) when a bogus MSE pointer is received on >= 1.1 hardware?

Isochronous transfers frequently experience MSEs under load, and some USB
firmware is known to issue bogus TRB pointers. Before this patch, the driver
unconditionally and gracefully ignored MSE events with unrecognized pointers,
letting the next event sort it out.

The commit message mentions a goal to log a scary error message for future
TDs when the skip flag is clear, but does not justify intentionally panicking
>= 1.1 controllers for this quirk.

Could the commit message clarify if punishing >= 1.1 chips with a fatal host
shutdown for a non-fatal, recoverable event was intended?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009151624.618967-1-mathias.nyman@linux.intel.com?part=9