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

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

[Mathias Nyman]

From: Yiyu Zhou <yiyu@yiyuzhou.io>

Fix a regression in v7.3 introduced by the commits in the Fixes tags.
Those patches avoid endpoint recovery and rapid state changes after
the root port link is lost.

This caused a regression in device enumeration.

It turns out that a subset of USB HID devices briefly disconnect during
enumeration. USB core manages to resuscitate these devices without
reallocating them, and thus continue using the same udev, xhci vdev and
default control endpoint (ep0) of the device.
xhci didn't give back or clean up halted TDs behind disconnected ports
as it assumed that the disconnect cancels all URBs, and drops and frees
all endpoints.

This is not the case, so cancel and give back TDs on halted endpoint
behind disconnected ports, make sure ep0 is cleared and in a proper state
to continue working even after a link error or disconnect is detected.
Also prevent new URBs until device is reset.

One issue related to the TDs not being given back was seen on a PFU
HHKB Professional HYBRID Type-S keyboard (04fe:0021).
It drops off the bus about 300 ms after its first enumeration, while
usbhid is still probing its third interface. The probe runs in hub_wq,
so the disconnect is not handled until the probe returns.
On a root port the HID descriptor request and its four retries in
hid_get_class_descriptor() each wait for the full 5 second
USB_CTRL_GET_TIMEOUT:

  [57685.006036] usb 3-2: new full-speed USB device number 24 using xhci_hcd
  ...
  [57685.291304] hid-generic 0003:04FE:0021.0017: input,hidraw1: USB HID v1.11 Keyboard [PFU Limited HHKB-Hybrid] on usb-0000:00:14.0-2/input1
  [57711.221083] usbhid 3-2:1.2: can't add hid device: -110
  [57711.221613] usbhid 3-2:1.2: probe with driver usbhid failed with error -110
  [57711.221842] usb 3-2: USB disconnect, device number 24

[mn: merge EP_DROP_PENDING and link_inactive checks in xhci_urb_enqueue(),
 return TDs and clean up ep0 on disconnect, rewrite commit message]

Fixes: 042aad8d0db6 ("xhci: prevent endpoint recovery after roothub disconnect")
Fixes: 7e0ef4332ed9 ("xhci: avoid xHC endpoint changes after disconnect or link error.")
Closes: https://lore.kernel.org/all/20261010071811.20483-1-yiyu@yiyuzhou.io/
Closes: https://lore.kernel.org/linux-usb/4MjB9K-2SoiA85WbI9iEig@dsine.de/
Tested-by: Yiyu Zhou <yiyu@yiyuzhou.io>
Tested-by: Dennis Wissel <dennis@dsine.de>
Signed-off-by: Yiyu Zhou <yiyu@yiyuzhou.io>
Co-developed-by: Mathias Nyman <mathias.nyman@linux.intel.com>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---

 v2 changes:
 - rebase on usb-linus
 - remove Assisted-by: LLM as suggested by latest checkpatch

 drivers/usb/host/xhci-ring.c | 73 ++++++++++++++++++++++++++++++------
 drivers/usb/host/xhci.c      | 13 ++++---
 2 files changed, 69 insertions(+), 17 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index c23434001e9c..a1fa188680f5 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -65,6 +65,7 @@
 static int queue_command(struct xhci_hcd *xhci, struct xhci_command *cmd,
 			 u32 field1, u32 field2,
 			 u32 field3, u32 field4, bool command_must_succeed);
+static void xhci_kill_endpoint_urbs(struct xhci_hcd *xhci, int slot_id, int ep_index);
 
 /*
  * Returns zero if the TRB isn't in this segment, otherwise it returns the DMA
@@ -962,6 +963,24 @@ static void xhci_giveback_invalidated_tds(struct xhci_hcd *xhci, struct xhci_vir
 	}
 }
 
+static void xhci_cancel_ring_tds(struct xhci_hcd *xhci, struct xhci_virt_ep *ep,
+				 struct xhci_ring *ring, int td_status)
+{
+	struct xhci_td *tmp_td;
+	struct xhci_td *td;
+
+	/* Add all TDs on ring to cancelled td list */
+	list_for_each_entry_safe(td, tmp_td, &ring->td_list, td_list) {
+		if (list_empty(&td->cancelled_td_list)) {
+			list_add_tail(&td->cancelled_td_list,
+				      &ep->cancelled_td_list);
+			td->cancel_status = TD_DIRTY;
+			if (!td->status)
+				td->status = td_status;
+		}
+	}
+}
+
 static int xhci_reset_halted_ep(struct xhci_hcd *xhci, unsigned int slot_id,
 				unsigned int ep_index, enum xhci_ep_reset_type reset_type)
 {
@@ -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;
+		}
 	}
 
-	/* add td to cancelled list and let reset ep handler take care of it */
-	if (reset_type == EP_HARD_RESET) {
-		ep->ep_state |= EP_HARD_CLEAR_TOGGLE;
+	/* Add halted td to cancelled list and let reset ep handler take care of it */
+	if (reset_type == EP_HARD_RESET || disconnected) {
 		if (td && list_empty(&td->cancelled_td_list)) {
 			list_add_tail(&td->cancelled_td_list, &ep->cancelled_td_list);
 			td->cancel_status = TD_HALTED;
+			if (disconnected && !td->status)
+				td->status = -EPROTO;
+		}
+	}
+
+	/*
+	 * Avoid resetting non-ep0 endpoints if link is inactive or device
+	 * disconnected. Can cause host hang.
+	 * Endpoints other than ep0 won't restart before the device is reset or
+	 * the endpoint is dropped, so cancel all TDs, and give back non-ep0 TDs
+	 * now instead of letting them time out. Ep0 can survive a short device
+	 * disconnect during enumeration so reset ep0 normally and clean it up.
+	 */
+	if (disconnected) {
+		ring = td ? xhci_urb_to_transfer_ring(xhci, td->urb) : ep->ring;
+		xhci_cancel_ring_tds(xhci, ep, ring, -ESHUTDOWN);
+		if (ep->ep_index != 0) {
+			xhci_dbg(xhci, "Not resetting ep %u slot %u after disconnect\n",
+				 ep->ep_index, slot_id);
+
+			xhci_process_cancelled_tds(xhci, ep);
+			xhci_kill_endpoint_urbs(xhci, slot_id, ep->ep_index);
+			return -ENODEV;
 		}
+		/* hard-reset a halted disconnected ep0 */
+		xhci_dbg(xhci, "Reset and clean up ep0 slot %u after disconnect\n",
+			 slot_id);
+		reset_type = EP_HARD_RESET;
 	}
 
 	if (ep->ep_state & EP_HALTED) {
@@ -1020,6 +1067,9 @@ static int xhci_handle_halted_endpoint(struct xhci_hcd *xhci,
 		return 0;
 	}
 
+	if (reset_type == EP_HARD_RESET)
+		ep->ep_state |= EP_HARD_CLEAR_TOGGLE;
+
 	err = xhci_reset_halted_ep(xhci, slot_id, ep->ep_index, reset_type);
 	if (err)
 		return err;
@@ -1070,8 +1120,9 @@ 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) {
+		/* device disconnected or link error, non-ep0 endpoints will be dropped */
+		if (ep->ep_state & EP_DROP_PENDING && ep->ep_index != 0) {
+			td_to_noop(xhci, ep, td, false);
 			td->cancel_status = TD_CLEARED;
 			continue;
 		}
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 30ac1bc4559b..b50a582be1fb 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;
+	}
+
 	if (*ep_state & (EP_GETTING_STREAMS | EP_GETTING_NO_STREAMS)) {
 		xhci_warn(xhci, "WARN: Can't enqueue URB, ep in streams transition state %x\n",
 			  *ep_state);
-- 
2.43.0

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

[Mathias Nyman]

From: Yiyu Zhou <yiyu@yiyuzhou.io>

Fix a regression in v7.3 introduced by the commits in the Fixes tags.
Those patches avoid endpoint recovery and rapid state changes after
the root port link is lost.

This caused a regression in device enumeration.

It turns out that a subset of USB HID devices briefly disconnect during
enumeration. USB core manages to resuscitate these devices without
reallocating them, and thus continue using the same udev, xhci vdev and
default control endpoint (ep0) of the device.
xhci didn't give back or clean up halted TDs behind disconnected ports
as it assumed that the disconnect cancels all URBs, and drops and frees
all endpoints.

This is not the case, so cancel and give back TDs on halted endpoint
behind disconnected ports, make sure ep0 is cleared and in a proper state
to continue working even after a link error or disconnect is detected.
Also prevent new URBs until device is reset.

One issue related to the TDs not being given back was seen on a PFU
HHKB Professional HYBRID Type-S keyboard (04fe:0021).
It drops off the bus about 300 ms after its first enumeration, while
usbhid is still probing its third interface. The probe runs in hub_wq,
so the disconnect is not handled until the probe returns.
On a root port the HID descriptor request and its four retries in
hid_get_class_descriptor() each wait for the full 5 second
USB_CTRL_GET_TIMEOUT:

  [57685.006036] usb 3-2: new full-speed USB device number 24 using xhci_hcd
  ...
  [57685.291304] hid-generic 0003:04FE:0021.0017: input,hidraw1: USB HID v1.11 Keyboard [PFU Limited HHKB-Hybrid] on usb-0000:00:14.0-2/input1
  [57711.221083] usbhid 3-2:1.2: can't add hid device: -110
  [57711.221613] usbhid 3-2:1.2: probe with driver usbhid failed with error -110
  [57711.221842] usb 3-2: USB disconnect, device number 24

[mn: merge EP_DROP_PENDING and link_inactive checks in xhci_urb_enqueue(),
 return TDs and clean up ep0 on disconnect, rewrite commit message]

Fixes: 042aad8d0db6 ("xhci: prevent endpoint recovery after roothub disconnect")
Fixes: 7e0ef4332ed9 ("xhci: avoid xHC endpoint changes after disconnect or link error.")
Closes: https://lore.kernel.org/all/20261010071811.20483-1-yiyu@yiyuzhou.io/
Closes: https://lore.kernel.org/linux-usb/4MjB9K-2SoiA85WbI9iEig@dsine.de/
Tested-by: Yiyu Zhou <yiyu@yiyuzhou.io>
Tested-by: Dennis Wissel <dennis@dsine.de>
Signed-off-by: Yiyu Zhou <yiyu@yiyuzhou.io>
Co-developed-by: Mathias Nyman <mathias.nyman@linux.intel.com>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---

 v2 changes:
 - rebase on usb-linus
 - remove Assisted-by: LLM as suggested by latest checkpatch

 drivers/usb/host/xhci-ring.c | 73 ++++++++++++++++++++++++++++++------
 drivers/usb/host/xhci.c      | 13 ++++---
 2 files changed, 69 insertions(+), 17 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index c23434001e9c..a1fa188680f5 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -65,6 +65,7 @@
 static int queue_command(struct xhci_hcd *xhci, struct xhci_command *cmd,
 			 u32 field1, u32 field2,
 			 u32 field3, u32 field4, bool command_must_succeed);
+static void xhci_kill_endpoint_urbs(struct xhci_hcd *xhci, int slot_id, int ep_index);
 
 /*
  * Returns zero if the TRB isn't in this segment, otherwise it returns the DMA
@@ -962,6 +963,24 @@ static void xhci_giveback_invalidated_tds(struct xhci_hcd *xhci, struct xhci_vir
 	}
 }
 
+static void xhci_cancel_ring_tds(struct xhci_hcd *xhci, struct xhci_virt_ep *ep,
+				 struct xhci_ring *ring, int td_status)
+{
+	struct xhci_td *tmp_td;
+	struct xhci_td *td;
+
+	/* Add all TDs on ring to cancelled td list */
+	list_for_each_entry_safe(td, tmp_td, &ring->td_list, td_list) {
+		if (list_empty(&td->cancelled_td_list)) {
+			list_add_tail(&td->cancelled_td_list,
+				      &ep->cancelled_td_list);
+			td->cancel_status = TD_DIRTY;
+			if (!td->status)
+				td->status = td_status;
+		}
+	}
+}
+
 static int xhci_reset_halted_ep(struct xhci_hcd *xhci, unsigned int slot_id,
 				unsigned int ep_index, enum xhci_ep_reset_type reset_type)
 {
@@ -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;
+		}
 	}
 
-	/* add td to cancelled list and let reset ep handler take care of it */
-	if (reset_type == EP_HARD_RESET) {
-		ep->ep_state |= EP_HARD_CLEAR_TOGGLE;
+	/* Add halted td to cancelled list and let reset ep handler take care of it */
+	if (reset_type == EP_HARD_RESET || disconnected) {
 		if (td && list_empty(&td->cancelled_td_list)) {
 			list_add_tail(&td->cancelled_td_list, &ep->cancelled_td_list);
 			td->cancel_status = TD_HALTED;
+			if (disconnected && !td->status)
+				td->status = -EPROTO;
+		}
+	}
+
+	/*
+	 * Avoid resetting non-ep0 endpoints if link is inactive or device
+	 * disconnected. Can cause host hang.
+	 * Endpoints other than ep0 won't restart before the device is reset or
+	 * the endpoint is dropped, so cancel all TDs, and give back non-ep0 TDs
+	 * now instead of letting them time out. Ep0 can survive a short device
+	 * disconnect during enumeration so reset ep0 normally and clean it up.
+	 */
+	if (disconnected) {
+		ring = td ? xhci_urb_to_transfer_ring(xhci, td->urb) : ep->ring;
+		xhci_cancel_ring_tds(xhci, ep, ring, -ESHUTDOWN);
+		if (ep->ep_index != 0) {
+			xhci_dbg(xhci, "Not resetting ep %u slot %u after disconnect\n",
+				 ep->ep_index, slot_id);
+
+			xhci_process_cancelled_tds(xhci, ep);
+			xhci_kill_endpoint_urbs(xhci, slot_id, ep->ep_index);
+			return -ENODEV;
 		}
+		/* hard-reset a halted disconnected ep0 */
+		xhci_dbg(xhci, "Reset and clean up ep0 slot %u after disconnect\n",
+			 slot_id);
+		reset_type = EP_HARD_RESET;
 	}
 
 	if (ep->ep_state & EP_HALTED) {
@@ -1020,6 +1067,9 @@ static int xhci_handle_halted_endpoint(struct xhci_hcd *xhci,
 		return 0;
 	}
 
+	if (reset_type == EP_HARD_RESET)
+		ep->ep_state |= EP_HARD_CLEAR_TOGGLE;
+
 	err = xhci_reset_halted_ep(xhci, slot_id, ep->ep_index, reset_type);
 	if (err)
 		return err;
@@ -1070,8 +1120,9 @@ 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) {
+		/* device disconnected or link error, non-ep0 endpoints will be dropped */
+		if (ep->ep_state & EP_DROP_PENDING && ep->ep_index != 0) {
+			td_to_noop(xhci, ep, td, false);
 			td->cancel_status = TD_CLEARED;
 			continue;
 		}
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 30ac1bc4559b..b50a582be1fb 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;
+	}
+
 	if (*ep_state & (EP_GETTING_STREAMS | EP_GETTING_NO_STREAMS)) {
 		xhci_warn(xhci, "WARN: Can't enqueue URB, ep in streams transition state %x\n",
 			  *ep_state);
-- 
2.43.0

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

[sashiko-bot]

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

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

[Greg KH]

On Thu, Oct 29, 2026 at 10:43:24PM +0200, Mathias Nyman wrote:
> From: Yiyu Zhou <yiyu@yiyuzhou.io>
> 
> Fix a regression in v7.3 introduced by the commits in the Fixes tags.
> Those patches avoid endpoint recovery and rapid state changes after
> the root port link is lost.
> 
> This caused a regression in device enumeration.

I see this patch twice?

And sashiko seems to object to it, is that response correct?

thanks,

greg k-h

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

[Mathias Nyman]

On 10/30/26 07:22, Greg KH wrote:
> On Thu, Oct 29, 2026 at 10:43:24PM +0200, Mathias Nyman wrote:
>> From: Yiyu Zhou <yiyu@yiyuzhou.io>
>>
>> Fix a regression in v7.3 introduced by the commits in the Fixes tags.
>> Those patches avoid endpoint recovery and rapid state changes after
>> the root port link is lost.
>>
>> This caused a regression in device enumeration.
> 
> I see this patch twice?
> 

My mistake, bash history shows I added the same file twice after git send-email

> And sashiko seems to object to it, is that response correct?
> 

It does find one potential case where this patch does not fix the enumeration
of devices that disconnect during reset.

This patch however is tested and does resolve the two real life regressions
where devices briefly disconnect during enumeration.

There's a risk for more real life regression if I too hastily fix this patch
to cover that sashiko case.

Thanks
Mathias

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

[Michal Pecio]

On Fri, 30 Oct 2026 13:05:16 +0200, Mathias Nyman wrote:
> > And sashiko seems to object to it, is that response correct?
> >   
> 
> It does find one potential case where this patch does not fix the
> enumeration of devices that disconnect during reset.

Honestly, is this 7.3 code being fixed here reliable at all?

As Sashiko points out (and I did before), we are 100% at the mercy of
hardware and USB core with those "connected" and "link error" flags.
Sashiko thinks that we may fail to clear these flags in some cases.
Is it guaranteed that we can't clear them prematurely at other times?

If USB3 link comes back after core has cleared all status change bits
(brief disconnection), we will get another Port Status Change Event,
observe the port in "Connected/U0" state and reset these flags.

Is it guaranteed that core won't clear change bits until it finishes
disconnection of affected devices so that all URBs are unlinked and
we can no longer run into those HW bugs anymore?

If this fails to work, it's another regression vs 7.2 and prior. The
old VDEV_PORT_ERROR flag used to stick for device lifetime. And it
could have been extended to all disconnected devices, not just the
one directly attached to the root port, to fix disconnection of whole
trees with hubs while avoiding the above race condition.

The reported USB2 regressions would have also been avoided, because
old code only applied to USB3 ports and there is no actual need to
generalize it to USB2 at this time. The bug still affected USB3, but
it seems nobody uses USB3 devices that trigger it.

I suppose I could fix the Renesas disconnection issue which the 7.3
rework tried to address with an alternative patch smaller than this
fixup, provided that the whole series is reverted. The preexisting
USB3 bug would remain unsolved for now, however.

Regards,
Michal

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

[Mathias Nyman]

On 10/30/26 14:05, Michal Pecio wrote:
> On Fri, 30 Oct 2026 13:05:16 +0200, Mathias Nyman wrote:
>>> And sashiko seems to object to it, is that response correct?
>>>    
>>
>> It does find one potential case where this patch does not fix the
>> enumeration of devices that disconnect during reset.
> 
> Honestly, is this 7.3 code being fixed here reliable at all?
> 
> As Sashiko points out (and I did before), we are 100% at the mercy of
> hardware and USB core with those "connected" and "link error" flags.
> Sashiko thinks that we may fail to clear these flags in some cases.
> Is it guaranteed that we can't clear them prematurely at other times?

'connected' and 'link error' flags can't be cleared prematurely, they
can be cleared late in case xHC doesn't generate port change events
for port changes. (change bits uncleared, hub driver is polling)

Sashiko reports the exact same issue in your 4/4 patch.

We can fix that, ask Dennis Wissel and Yiyu Zhou nicely if they still
can run one more test round. Maybe even make sashiko happy.


> 
> If USB3 link comes back after core has cleared all status change bits
> (brief disconnection), we will get another Port Status Change Event,
> observe the port in "Connected/U0" state and reset these flags.

This is what we want.
These flags are tracking root port states, not trying to track usb device
states

> 
> Is it guaranteed that core won't clear change bits until it finishes
> disconnection of affected devices so that all URBs are unlinked and
> we can no longer run into those HW bugs anymore?

No, and that is not the goal.
We are trying to avoid excessive endpoint state mangling after devices
are disconnected. We avoided too much in 7.3 when we left ep0 in a bad
state, assuming it will be dropped and freed after a disconnect.

> 
> If this fails to work, it's another regression vs 7.2 and prior. The
> old VDEV_PORT_ERROR flag used to stick for device lifetime. And it
> could have been extended to all disconnected devices, not just the
> one directly attached to the root port, to fix disconnection of whole
> trees with hubs while avoiding the above race condition.

moving the flags to all child vdev devices won't help, you
have all the same issues, plus the fact that vdevs are sometines
reallocated mid enumeration, and sometimes will stick even if device
briefly disconnects mid enumeration, just like ep0.

> 
> The reported USB2 regressions would have also been avoided, because
> old code only applied to USB3 ports and there is no actual need to
> generalize it to USB2 at this time. The bug still affected USB3, but
> it seems nobody uses USB3 devices that trigger it.

It's more about the hosts than the devices.

It appears some xHC hosts stop reporting endpoint states and changes
correctly some time after roothub connection is gone.

I recall some xHC had to disable soft retry because it almost immediately
went to some internal sleep state after last usb device was disconnected.

xhci driver first tried to soft-reset endpoint a few times, then hard-reset,
set-TR deq, restart ring, then hub driver noticed disconnect, cancelled URBs
which lead to trying to stopping the endpoint again.

At this point xHC no longer responded to stop endpoint command,
so xhci driver assumed host was broken.

once a new device was connected to host it immediately 'woke up'
and processed all endpoint related commands correctly.

> 
> I suppose I could fix the Renesas disconnection issue which the 7.3
> rework tried to address with an alternative patch smaller than this
> fixup, provided that the whole series is reverted. The preexisting
> USB3 bug would remain unsolved for now, however.

Doesn't make sense to start from scratch because of this sashiko report.
You'll just run into a new set of issues and regressions.

Thanks
Mathias

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

[sashiko-bot]

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