Re: [PATCH 10/14] usb: xhci: Don't set the skip flag on non-isoc endpoints

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] usb: xhci: invalid completion of URB with -EINPROGRESS on COMP_MISSED_SERVICE_ERROR
--

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

usb: xhci: Don't set the skip flag on non-isoc endpoints

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

> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 8b915a1d5b256..2dd11732bb87c 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -2778,16 +2778,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;

[Severity: High]
In handle_tx_event() in drivers/usb/host/xhci-ring.c, for non-isochronous
endpoints, does using break instead of return 0 here cause the event to
improperly fall through into the rest of the handler?

If the event's DMA matches the current TD, it appears it is passed to
process_bulk_intr_td() or process_ctrl_td(). Since those functions do not
handle this completion code, does the URB's status remain initialized to
-EINPROGRESS, completing the URB with an illegal status code?

Additionally, if the event DMA does not match the current TD, does the code
fall back to the debug_finding_td error path, fatally shutting down the host
controller with -ESHUTDOWN?

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

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