Re: [PATCH 1/4] usb: xhci: Fix TD invalidation on disconnected endpoints

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] usb: xhci: uncleared deferred stream TDs in xhci_invalidate_cancelled_tds()
--

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

usb: xhci: Fix TD invalidation on disconnected endpoints

This commit moves the EP_DROP_PENDING check in
xhci_invalidate_cancelled_tds() after the loop over ep->cancelled_td_list
and restricts the early return to non-EP0 endpoints so that EP0 TRBs are
turned into no-ops and the dequeue pointer is moved to enqueue when a
disconnected device is resuscitated by reset.

Link: https://lore.kernel.org/linux-usb/SrQQcLIwRWyg9PdEIpc0eg@dsine.de/

> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index ec278a9f95404..698f994e43a55 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -1063,12 +1063,6 @@ static int xhci_invalidate_cancelled_tds(struct xhci_hcd *xhci, struct xhci_virt
>  			continue;
>  		}
>  
> -		/* device disconnected or link error, ep will be dropped */
> -		if (ep->ep_state & EP_DROP_PENDING) {
> -			td->cancel_status = TD_CLEARED;
> -			continue;
> -		}
> -
>  		/*
>  		 * If a ring stopped on the TD we need to cancel then we have to
>  		 * move the xHC endpoint ring dequeue pointer past this TD.
> @@ -1120,6 +1114,12 @@ static int xhci_invalidate_cancelled_tds(struct xhci_hcd *xhci, struct xhci_virt
>  	if (!cached_td)
>  		return 0;
>  
> +	/* Don't bother fixing disconnected endpoints except EP0 */
> +	if (ep->ep_state & EP_DROP_PENDING && ep->ep_index != 0) {
> +		cached_td->cancel_status = TD_CLEARED;
> +		return 0;
> +	}

[Severity: High]
Can this leave deferred TDs uncleared on multi-stream endpoints?

When a non-EP0 multi-stream endpoint has EP_DROP_PENDING set and multiple
streams have cancelled TDs at their hardware dequeue pointers
(trb_in_td(td, hw_deq)), the loop in
drivers/usb/host/xhci-ring.c:xhci_invalidate_cancelled_tds() unlinks every
cancelled TD from ring->td_list via list_del_init(&td->td_list), assigns
the first stopped stream's TD to cached_td with TD_CLEARING_CACHE, and
marks subsequent stopped TDs on other streams as TD_CLEARING_CACHE_DEFERRED:

	if (cached_td) {
		if (cached_td->urb->stream_id != td->urb->stream_id) {
			/* Multiple streams case, defer move dq */
			xhci_dbg(xhci,
				 "Move dq deferred: stream %u URB %p\n",
				 td->urb->stream_id, td->urb);
			td->cancel_status = TD_CLEARING_CACHE_DEFERRED;
			break;
		}

When the new EP_DROP_PENDING check after the loop returns early without
calling xhci_move_deq_to_next_td(), only cached_td->cancel_status is set to
TD_CLEARED, leaving any TD_CLEARING_CACHE_DEFERRED TDs on
ep->cancelled_td_list uncleared.

In xhci_giveback_invalidated_tds(), only TDs with cancel_status == TD_CLEARED
are cleaned up and given back:

	if (td->cancel_status == TD_CLEARED) {
		xhci_dbg(xhci, "%s: Giveback cancelled URB %p TD\n",
			 __func__, td->urb);
		xhci_td_cleanup(xhci, td, ring, td->status);
	} else {
		xhci_dbg(xhci, "%s: Keep cancelled URB %p TD as cancel_status is %d\n",
			 __func__, td->urb, td->cancel_status);
	}

Since SET_DEQ_PENDING is never set, nothing re-triggers invalidation,
stranding those deferred TDs in usb_kill_urb() or usb_hcd_flush_endpoint().

Even with the later patches in this series ("usb: xhci: Give back stranded
URBs on disconnected endpoints" and "usb: xhci: Give back URBs on halted
disconnected endpoints") that add a fallback call to
xhci_kill_endpoint_urbs(), could this still cause problems in two ways?

First, if xhci_giveback_invalidated_tds() runs while EP_STOP_CMD_PENDING or
EP_HALTED is still set (for example via xhci_handle_cmd_stop_ep() ->
xhci_handle_halted_endpoint() -> xhci_process_cancelled_tds(), before
EP_STOP_CMD_PENDING is cleared at reset_done), xhci_kill_endpoint_urbs() is
skipped and the deferred TDs remain stranded.

Second, when xhci_kill_endpoint_urbs() does run, those deferred TDs were
already unlinked from ring->td_list by xhci_invalidate_cancelled_tds(), so
they bypass both xhci_td_cleanup() and xhci_kill_ring_urbs() and fall into
the ep->cancelled_td_list loop in xhci_kill_endpoint_urbs(), which does not
call xhci_unmap_td_bounce_buffer() and overwrites td->status with
-ESHUTDOWN.

Should this path also clear any TD_CLEARING_CACHE_DEFERRED TDs on
ep->cancelled_td_list, similar to the error handling path when
xhci_move_deq_to_next_td() fails?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029075409.4b203658.michal.pecio@gmail.com?part=1