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 state after reset sets EP_DROP_PENDING on ep0
--

commit 3214a21a67ec03e6fc45b610d6ba5543f36b65ab
Author: Yiyu Zhou <yiyu@yiyuzhou.io>

xhci: give back TDs and clean up ep0 after disconnect

This commit fixes a device enumeration regression where USB devices that
briefly disconnect during enumeration leave halted TDs on rings or leave ep0
uncleaned. It cancels and gives back TDs on halted endpoints behind
disconnected ports, hard-resets ep0 so it can continue working after a short
disconnect, and blocks new URB submissions via EP_DROP_PENDING until the
device is reset.

> 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;
[ ... ]
> 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
>  		goto free_priv;
>  	}
>  
> -	if (xhci->devs[slot_id]->rhub_port->link_inactive) {
> -		xhci_dbg(xhci, "Can't queue urb, port error, link inactive\n");
> -		ret = -ENODEV;
> -		goto free_priv;
> -	}
> -
>  	if (xhci->xhc_state & XHCI_STATE_DYING) {
>  		xhci_dbg(xhci, "Ep 0x%x: URB %p submitted for non-responsive xHCI host.\n",
>  			 urb->ep->desc.bEndpointAddress, urb);
> @@ -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 rhub_port->connected or rhub_port->link_inactive remain stale after a
port reset, causing xhci_urb_enqueue() to permanently reject subsequent ep0
URBs with -ENODEV?

In drivers/usb/host/xhci-ring.c, handle_port_status() appears to be the only
place where port->connected and port->link_inactive are updated:

drivers/usb/host/xhci-ring.c:handle_port_status() {
	...
	if (!(portsc & PORT_RESET)) {
		port->link_inactive = (pls == XDEV_INACTIVE);
		port->connected = !!(portsc & PORT_CONNECT);
	}
	...
}

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, the
xHC generates a Port Status Change Event (TRB_PORT_STATUS), and
handle_port_status() sets port->connected = 0 (or port->link_inactive = 1)
without clearing the PORTSC change bits.

When the device reconnects and hub_port_init() calls hub_port_reset(), a new
Port Status Change Event is only generated by the xHC when the logical OR of
all PORTSC status-change bits (CSC | PEC | WRC | OCC | RC | PLC | CEC)
transitions from 0 to 1. Because PORT_CSC (or PORT_PLC) is already 1 when
the device reconnects and when the reset completes (setting PORT_RC = 1), no
TRB_PORT_STATUS event is generated:

- On USB2, hub_port_wait_reset() sees USB_PORT_STAT_C_CONNECTION, clears
  PORT_CSC while PORT_RC is still 1 (keeping the logical OR of change bits
  at 1), and returns -EAGAIN. hub_port_reset() then issues a second reset
  while PORT_RC is still 1 and clears PORT_RC upon completion, transitioning
  the logical OR of change bits from 1 to 0.

- On USB3, hub_port_reset() clears C_RESET, C_BH_PORT_RESET,
  C_PORT_LINK_STATE, and C_CONNECTION together after reset completion, also
  transitioning the logical OR of change bits from 1 to 0.

Neither xhci_get_port_status(), xhci_clear_port_change_bit(), nor
xhci_discover_or_reset_device() refreshes port->connected or
port->link_inactive:

drivers/usb/host/xhci.c:xhci_discover_or_reset_device() {
	...
command_cleanup:
	for (i = 0; i < EP_CTX_PER_DEV; i++)
		virt_dev->eps[i].ep_state &= ~EP_DROP_PENDING;
	...
}

After xhci_discover_or_reset_device() clears EP_DROP_PENDING, if
rhub_port->link_inactive remained 1, xhci_urb_enqueue() will immediately
reject all URBs with -ENODEV. If rhub_port->connected remained 0,
enumeration continues on the same udev, vdev, and ep0 until any normal
protocol stall (COMP_STALL_ERROR) occurs on ep0 (for example,
check_highspeed() requesting USB_DT_DEVICE_QUALIFIER on a full-speed USB 2.0
device, or hid_set_idle() during usbhid probe) or an URB is unlinked via
xhci_handle_cmd_stop_ep(). At that point, xhci_handle_halted_endpoint() (or
xhci_handle_cmd_stop_ep()) sees !rhub_port->connected and sets
EP_DROP_PENDING on ep0.

Because a protocol stall or URB unlink does not trigger a device reset,
wouldn't EP_DROP_PENDING remain set on ep0 and cause the new check in
xhci_urb_enqueue() to reject all subsequent ep0 URBs with -ENODEV?

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