Re: [RFT PATCH 0/2] xhci: clean up endpoint rings after disconnect detect

[Michal Pecio <michal.pecio@gmail.com>]

On Tue, 27 Oct 2026 21:21:25 +0200, Mathias Nyman wrote:
> Yiyu Zhou wrote PATCH 1/2, fixing a regression in USB HID device
> enumeration caused by the endpoint recovery prevention code added in
> v7.3-rc1. Some HID devices that used to enumerate after a brief
> disconnect during enumeration now got stuck until URBs timed out.
> 
> PATCH 1/2 gives back the pending URBs right away, which solves most of
> the problem, but it leaves the TRBs of the given back URBs on the ep0
> ring. ep0 survives a brief disconnect during a port reset, so the xHC
> may later process those TRBs during an enumeration retry, writing into
> buffers that were already given back, or ep0 stays halted and the next
> control transfer times out.
> 
> PATCH 2/2 cleans up the ep0 ring by resetting ep0 and moving its
> dequeue pointer past the cancelled TDs, even if a disconnect or link
> error is detected. Only Reset Endpoint and Set TR Dequeue Pointer
> commands are issued; ep0 is not restarted while the disconnect is
> pending. The rapid endpoint reset, restart and stop sequence that made
> some xHC hosts fail Stop Endpoint commands is still avoided. Other
> endpoints are still given back without resetting them, but their TRBs
> are turned into no-ops first.
> 
> If you have seen the HID enumeration issue, or the xHC Stop Endpoint
> failures with rapid endpoint state changes, testing these two patches
> would be greatly appreciated.
> 
> If these patches work, they might be squashed into one before going
> upstream.

Might? :)

1/2 is a possibly dangereous regression and shouldn't be used alone.

Anyway, I think it all can be done with a bit less code, while also
handling disconnection during unlink. Particularly on EP0, multiple
URBs may be submitted by different drivers and/or userspace. Just
because one of them unlinks its URB doesn't mean that the others are
aware of anything being wrong and won't stall for <N> seconds.

This below is a rudimentarily tested RFC but it seems to work. It's
simple enough that hopefully it might even be correct. I will test it
more and write a changelog later today if there are no objections.

I tweaked the logic to return everything with -ESHUTDOWN and not
bother to assign proper status to the URB which halted. Intuition:
we want them to give up, not call clear_halt() or reset_device().

---

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index fa6684746305..b37056cb8576 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -987,14 +987,11 @@ 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
+	 * Mark endpoints behind disconnected or SS.Inactive root hub ports.
+	 * Restarting them is futile and known to triggers HC bugs on USB3.
 	 */
-	if (rhub_port->link_inactive || !rhub_port->connected) {
+	if (rhub_port->link_inactive || !rhub_port->connected)
 		ep->ep_state |= EP_DROP_PENDING;
-		return -ENODEV;
-	}
 
 	/* add td to cancelled list and let reset ep handler take care of it */
 	if (reset_type == EP_HARD_RESET) {
@@ -1011,6 +1008,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) {
+		xhci_process_cancelled_tds(xhci, ep);
+		return 0;
+	}
+
 	err = xhci_reset_halted_ep(xhci, slot_id, ep->ep_index, reset_type);
 	if (err)
 		return err;
@@ -1022,6 +1025,9 @@ static int xhci_handle_halted_endpoint(struct xhci_hcd *xhci,
 	return 0;
 }
 
+static void xhci_kill_endpoint_urbs(struct xhci_hcd *xhci, int slot_id, int ep_index);
+
+
 /*
  * Fix up the ep ring first, so HW stops executing cancelled TDs.
  * We have the xHCI lock, so nothing can modify this list until we drop it.
@@ -1047,6 +1053,22 @@ static int xhci_invalidate_cancelled_tds(struct xhci_hcd *xhci, struct xhci_virt
 	if (ep->ep_state & SET_DEQ_PENDING)
 		return 0;
 
+	if (ep->ep_state & EP_DROP_PENDING) {
+		xhci_dbg(xhci, "Nuking dead slot %d ep %d\n", ep->vdev->slot_id, ep->ep_index);
+		if (ep->ep_index) {
+			xhci_kill_endpoint_urbs(xhci, ep->vdev->slot_id, ep->ep_index);
+			return 0;
+		}
+		/* EP0 may be reused after device reset, clean up the ring properly */
+		list_for_each_entry(td, &ep->ring->td_list, td_list) {
+			td->status = -ESHUTDOWN;
+			if (list_empty(&td->cancelled_td_list)) {
+				list_add_tail(&td->cancelled_td_list, &ep->cancelled_td_list);
+				td->cancel_status = TD_DIRTY;	// for clarity, it's redundant
+			}
+		}
+	}
+
 	list_for_each_entry_safe(td, tmp_td, &ep->cancelled_td_list, cancelled_td_list) {
 		xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
 			       "Removing canceled TD starting at 0x%llx (dma) in stream %u URB %p",
@@ -1061,12 +1083,6 @@ 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) {
-			td->cancel_status = TD_CLEARED;
-			continue;
-		}
-
 		/*
 		 * If a ring stopped on the TD we need to cancel then we have to
 		 * move the xHC endpoint ring dequeue pointer past this TD.