Re: [PATCH 3/4] usb: xhci: Give back URBs on halted disconnected endpoints

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] usb: xhci: xhci_handle_halted_endpoint() completes failed URBs with status 0
- [High] usb: xhci: xhci_handle_halted_endpoint() skips killing URBs during Stop EP
--

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

usb: xhci: Give back URBs on halted disconnected endpoints

This commit updates xhci_handle_halted_endpoint() to override soft retries
with EP_HARD_RESET and call xhci_process_cancelled_tds() for non-zero
endpoints on disconnected or link-inactive roothub ports, ensuring URBs are
given back without queuing a Reset Endpoint command.

Link: https://lore.kernel.org/linux-usb/20261010064407.11030-1-yiyu@yiyuzhou.io/
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 49483eb55dc83..b731e59852f58 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -995,13 +995,14 @@ static int xhci_handle_halted_endpoint(struct xhci_hcd *xhci,
>  	int err;
>  
>  	/*
> -	 * Avoid resetting endpoint if link is inactive or device disonnected.
> -	 * Can cause host hang.
> -	 * Device will be reset to recover an inactive link, so don't do anything
> +	 * Flag endpoints behind disconnected RH ports and override soft retries.
> +	 * Restarting them is futile and known to trigger HC bugs on USB3 buses.
>  	 */
>  	if (rhub_port->link_inactive || !rhub_port->connected) {
> +		xhci_dbg(xhci, "Halted slot %d ep %d is lost\n",
> +				slot_id, ep->ep_index);
>  		ep->ep_state |= EP_DROP_PENDING;
> -		return -ENODEV;
> +		reset_type = EP_HARD_RESET;
>  	}

[Severity: High]
Can overriding reset_type from EP_SOFT_RESET to EP_HARD_RESET here cause
failed bulk and interrupt URBs to be completed with status 0 (success)?

When a bulk or interrupt transfer fails with COMP_USB_TRANSACTION_ERROR on a
non-TT endpoint without XHCI_NO_SOFT_RETRY, handle_tx_event() calls
process_bulk_intr_td(), which clears td->status to 0 before requesting a soft
reset and returns before updating td->urb->actual_length:

drivers/usb/host/xhci-ring.c:process_bulk_intr_td()
	case COMP_USB_TRANSACTION_ERROR:
		if (xhci->quirks & XHCI_NO_SOFT_RETRY ||
		    (ep->err_count++ > MAX_SOFT_RETRY) ||
		    le32_to_cpu(slot_ctx->tt_info) & TT_SLOT)
			break;

		td->status = 0;

		xhci_handle_halted_endpoint(xhci, ep, td, EP_SOFT_RESET);
		return;

When xhci_handle_halted_endpoint() upgrades reset_type to EP_HARD_RESET, it
places td onto ep->cancelled_td_list with td->status still set to 0:

	if (reset_type == EP_HARD_RESET) {
		ep->ep_state |= EP_HARD_CLEAR_TOGGLE;
		if (td && list_empty(&td->cancelled_td_list)) {
			list_add_tail(&td->cancelled_td_list, &ep->cancelled_td_list);
			td->cancel_status = TD_HALTED;
		}
	}

Then xhci_process_cancelled_tds() -> xhci_invalidate_cancelled_tds() marks
the TD as TD_CLEARED, and xhci_giveback_invalidated_tds() gives it back using
td->status before xhci_kill_endpoint_urbs() runs:

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

Wouldn't this complete the failed URB to the USB driver with urb->status == 0
and urb->actual_length == 0 instead of an error code?

>  
>  	/* add td to cancelled list and let reset ep handler take care of it */
> @@ -1019,6 +1020,12 @@ static int xhci_handle_halted_endpoint(struct xhci_hcd *xhci,
>  		return 0;
>  	}
>  
> +	/* Don't reset lost endpoints, only EP0 can survive device reset */
> +	if (ep->ep_state & EP_DROP_PENDING && ep->ep_index != 0) {
> +		xhci_process_cancelled_tds(xhci, ep);
> +		return 0;
> +	}

[Severity: High]
What happens here if an endpoint halts on a disconnected or link-inactive
port while a Stop Endpoint command is pending (EP_STOP_CMD_PENDING is set)?

In xhci_giveback_invalidated_tds(), xhci_kill_endpoint_urbs() is skipped
whenever EP_STOP_CMD_PENDING is set:

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

When the Stop Endpoint command completes with COMP_CONTEXT_STATE_ERROR in
EP_STATE_HALTED, xhci_handle_cmd_stop_ep() calls
xhci_handle_halted_endpoint() before clearing EP_STOP_CMD_PENDING:

drivers/usb/host/xhci-ring.c:xhci_handle_cmd_stop_ep()
			/* reset ep, reset handler cleans up cancelled tds */
			err = xhci_handle_halted_endpoint(xhci, ep, td, reset_type);
			xhci_dbg(xhci, "Stop ep completion resetting ep, status %d\n", err);
			if (err)
				break;
reset_done:
			/* Reset EP handler will clean up cancelled TDs */
			ep->ep_state &= ~EP_STOP_CMD_PENDING;
			return;

Because EP_STOP_CMD_PENDING is still set during the call to
xhci_handle_halted_endpoint(), xhci_process_cancelled_tds() ->
xhci_giveback_invalidated_tds() skips xhci_kill_endpoint_urbs(). Then,
because xhci_handle_halted_endpoint() returns 0 instead of an error,
xhci_handle_cmd_stop_ep() jumps to reset_done, clears EP_STOP_CMD_PENDING,
and returns early without reaching the xhci_giveback_invalidated_tds() call
at the end of xhci_handle_cmd_stop_ep().

Doesn't this leave all remaining non-cancelled URBs on the endpoint ring
stranded?

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