Re: [PATCH v2] xhci: give back TDs and clean up ep0 after disconnect

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] xhci: stale rhub_port status across reset re-triggers EP_DROP_PENDING
--

commit 28b1e3f2ba757ac7bec832f907ed7d9693752c9e
Author: Yiyu Zhou <yiyu@yiyuzhou.io>

xhci: give back TDs and clean up ep0 after disconnect

This commit cancels and gives back TDs on halted endpoints behind
disconnected or link-inactive ports, hard-resets and cleans up ep0 so it
can survive brief disconnects during enumeration, and blocks new URB
submissions in xhci_urb_enqueue() while EP_DROP_PENDING is set.

Closes: https://lore.kernel.org/all/20261010071811.20483-1-yiyu@yiyuzhou.io/
Closes: https://lore.kernel.org/linux-usb/4MjB9K-2SoiA85WbI9iEig@dsine.de/

> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index c23434001e9cb..a1fa188680f58 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c

[ ... ]

> @@ -993,25 +1012,53 @@ static int xhci_handle_halted_endpoint(struct xhci_hcd *xhci,
>  {
>  	struct xhci_port *rhub_port = ep->vdev->rhub_port;
>  	unsigned int slot_id = ep->vdev->slot_id;
> +	bool disconnected = false;
> +	struct xhci_ring *ring;
>  	int err;
>  
> -	/*
> -	 * Avoid resetting endpoint if link is inactive or device disconnected.
> -	 * Can cause host hang.
> -	 * Device will be reset to recover an inactive link, so don't do anything
> -	 */
> +	/* link is inactive or device disconnected */
>  	if (rhub_port->link_inactive || !rhub_port->connected) {
>  		ep->ep_state |= EP_DROP_PENDING;
> -		return -ENODEV;
> +		disconnected = true;
> +		if (!td && ep->ep_index != 0) {
> +			xhci_kill_endpoint_urbs(xhci, slot_id, ep->ep_index);
> +			return -ENODEV;
> +		}
>  	}

[ ... ]

> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 30ac1bc4559b7..b50a582be1fbf 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -1665,12 +1665,6 @@ static int xhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag

[ ... ]

> @@ -1680,6 +1674,13 @@ static int xhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
>  
>  	ep_state = &xhci->devs[slot_id]->eps[ep_index].ep_state;
>  
> +	if (xhci->devs[slot_id]->rhub_port->link_inactive ||
> +	    *ep_state & EP_DROP_PENDING) {
> +		xhci_dbg(xhci, "Can't queue urb, port disconnect or link inactive\n");
> +		ret = -ENODEV;
> +		goto free_priv;
> +	}

[Severity: High]
Can a stale rhub_port->connected or rhub_port->link_inactive state across a
port reset cause xhci_urb_enqueue() to permanently reject ep0 URBs with
-ENODEV after a brief disconnect?

In drivers/usb/host/xhci-ring.c, port->connected and port->link_inactive are
only updated in handle_port_status() when a Port Status Change Event TRB is
received:

	if (!(portsc & PORT_RESET)) {
		port->link_inactive = (pls == XDEV_INACTIVE);
		port->connected = !!(portsc & PORT_CONNECT);
	}

Per xHCI specification section 4.19.2, the xHC only generates a Port Status
Change Event TRB when the logical OR of all PORTSC status-change bits
(CSC | PEC | OCC | PRC | WRC | PLC | CEC) transitions from 0 to 1.

When a device briefly disconnects during enumeration in hub_port_init() (or
enters XDEV_INACTIVE), PORT_CSC (or PORT_PLC) transitions from 0 to 1 and
handle_port_status() sets rhub_port->connected = 0 (or
rhub_port->link_inactive = 1). Because hub_wq is busy in hub_port_init(),
port_event() does not run to clear PORT_CSC.

When the device reconnects and hub_port_reset() resets the port to recover
it, PORT_CSC (or PORT_PLC) is already 1 when reconnect and PORT_RC occur:

1. On USB 2.0 ports, the first reset attempt sets PORT_RC = 1 while PORT_CSC
   is still 1 (no 0-to-1 transition of the change-bit OR).
   hub_port_wait_reset() sees USB_PORT_STAT_C_CONNECTION, clears PORT_CSC
   while PORT_RC remains 1, and returns -EAGAIN before hub_port_reset()
   clears PORT_RC. The second reset attempt then completes while PORT_RC is
   already 1 and clears PORT_RC to 0, so no Port Status Change Event TRB is
   generated after the reconnect.

2. On USB 3.0 ports, hub_port_wait_reset() ignores
   USB_PORT_STAT_C_CONNECTION, so the reset completes while PORT_CSC or
   PORT_PLC is still 1 (no 0-to-1 transition) and hub_port_reset() then
   clears PORT_RC, PORT_WRC, PORT_PLC, and PORT_CSC.

At the end of the reset, xhci_discover_or_reset_device() clears
EP_DROP_PENDING without refreshing rhub_port->connected or
rhub_port->link_inactive:

command_cleanup:
	for (i = 0; i < EP_CTX_PER_DEV; i++)
		virt_dev->eps[i].ep_state &= ~EP_DROP_PENDING;

If rhub_port->link_inactive is still 1, xhci_urb_enqueue() will immediately
reject all URBs with -ENODEV.

If rhub_port->connected is still 0, then as soon as ep0 hits any normal
protocol stall (COMP_STALL_ERROR, such as fetching USB_DT_DEVICE_QUALIFIER,
USB_DT_BOS, or HID_REQ_SET_IDLE) or an URB unlink via
xhci_handle_cmd_stop_ep(), xhci_handle_halted_endpoint() sees
!rhub_port->connected, overwrites td->status from -EPIPE to -EPROTO, and
sets ep->ep_state |= EP_DROP_PENDING on ep0. Because EP_DROP_PENDING is
never cleared by xhci_handle_cmd_reset_ep() or xhci_handle_cmd_set_deq(),
wouldn't xhci_urb_enqueue() then reject all subsequent ep0 control transfers
with -ENODEV?

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