Re: [PATCH 2/4] usb: xhci: Give back stranded URBs 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 TDs in xhci_invalidate_cancelled_tds()
--

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

usb: xhci: Give back stranded URBs on disconnected endpoints

This commit gives back remaining URBs on endpoints marked with
EP_DROP_PENDING by cancelling remaining TDs before invalidation on EP0 and
calling xhci_kill_endpoint_urbs() after cancelled TD processing on other
endpoints.

> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 698f994e43a55..49483eb55dc83 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c

[ ... ]

> @@ -953,6 +954,11 @@ static void xhci_giveback_invalidated_tds(struct xhci_hcd *xhci, struct xhci_vir
>  		if (xhci->xhc_state & XHCI_STATE_DYING)
>  			return;
>  	}
> +
> +	/* Nuke stranded URBs if there is no one to give them back */
> +	if ((ep->ep_state & EP_DROP_PENDING) &&
> +	    !(ep->ep_state & (EP_HALTED | SET_DEQ_PENDING | EP_STOP_CMD_PENDING)))
> +		xhci_kill_endpoint_urbs(xhci, ep->vdev->slot_id, ep->ep_index);

[Severity: High]
Can this skip xhci_td_cleanup() and leak DMA bounce buffer mappings on
disconnected stream-enabled endpoints when multiple streams have cancelled
TDs stopped at their hardware dequeue pointers?

In drivers/usb/host/xhci-ring.c, xhci_invalidate_cancelled_tds() first
unlinks every TD on ep->cancelled_td_list from its transfer ring via
list_del_init(&td->td_list). When cancelled TDs are stopped at hw_deq across
multiple streams, the first TD becomes cached_td with TD_CLEARING_CACHE,
while TDs in other streams are marked TD_CLEARING_CACHE_DEFERRED:

drivers/usb/host/xhci-ring.c:xhci_invalidate_cancelled_tds() {
	...
	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;
	}
	...
}

Later in xhci_invalidate_cancelled_tds(), the early return for disconnected
non-control endpoints only clears cached_td:

	/* 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;
	}

Any TDs in other streams remain in TD_CLEARING_CACHE_DEFERRED without a
Set TR Dequeue command queued.

When xhci_giveback_invalidated_tds() runs, those deferred TDs are skipped by
the first loop because td->cancel_status != TD_CLEARED, and are then handed
to xhci_kill_endpoint_urbs(). Because they were already removed from
ring->td_list, xhci_kill_ring_urbs() does not see them, and they fall
through to the ep->cancelled_td_list loop at the end of
xhci_kill_endpoint_urbs():

drivers/usb/host/xhci-ring.c:xhci_kill_endpoint_urbs() {
	...
	list_for_each_entry_safe(cur_td, tmp, &ep->cancelled_td_list,
			cancelled_td_list) {
		list_del_init(&cur_td->cancelled_td_list);
		inc_td_cnt(cur_td->urb);

		if (last_td_in_urb(cur_td))
			xhci_giveback_urb_in_irq(xhci, cur_td, -ESHUTDOWN);
	}
}

Unlike xhci_kill_ring_urbs() and xhci_td_cleanup(), this loop does not call
xhci_unmap_td_bounce_buffer() (leaving seg->bounce_dma mapped and
seg->bounce_len non-zero) and overwrites the cancelled URB's td->status with
-ESHUTDOWN. Additionally, if EP_HALTED is set on the endpoint, those
already-cancelled URBs will not be given back here either.

Should the EP_DROP_PENDING && ep->ep_index != 0 path in
xhci_invalidate_cancelled_tds() also mark any TD_CLEARING_CACHE_DEFERRED TDs
as TD_CLEARED (similar to the xhci_move_deq_to_next_td() error path right
below it) so that xhci_giveback_invalidated_tds() cleans them up normally?

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