Re: [PATCH 2/5] usb: xhci: correct num_active_eps accounting on allocation failure

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

On Tue, 6 Oct 2026 18:33:01 +0200, Michal Pecio wrote:
> On Tue,  6 Oct 2026 17:24:24 +0200, Niklas Neronin wrote:
> > Some host controllers have a global endpoint limit across all slots,
> > tracked by 'xhci->num_active_eps' when XHCI_EP_LIMIT_QUIRK is set.
> > 
> > During device allocation, EP0 resources are reserved before
> > xhci_alloc_virt_device() is called, and 'num_active_eps' is
> > incremented accordingly. If xhci_alloc_virt_device() subsequently
> > fails, the slot is disabled but the reserved endpoint resource is
> > not released, leaving 'num_active_eps' permanently increased.
> > 
> > Decreasing 'num_active_eps' happens in
> > xhci_handle_cmd_disable_slot(), which is called upon a Disable Slot
> > completion command.  
> 
> Why is 'num_active_eps' not decremented if the slot is disabled and
> Disable Slot completion handler is supposed to decrement it?
> 
> Does this bug really exist? Can you reproduce it by forcing the quirk
> with module parameter and simulating vdev allocation failure here?

Never mind, the bug is real and reproducible. Still, commit message
could include a few words of explanation to prevent such questions.

I think it would be cleaner to fix this by allocating vdev before
ep0 reservation, because allocation is easier to undo:

1. we have a helper for this, no need to dig in xhci members manually
2. we are the only user of this slot_id, so locking is not required

Downside: larger diff. But less code in the end.

And there is another bug: disable_slot uses udev->slot_id, which is
uninitialized in this path. Should use the local slot_id variable.

So overall,

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index e5b39be04b92..9beb8b4b485f 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -4308,27 +4308,29 @@ int xhci_alloc_dev(struct usb_hcd *hcd, struct usb_device *udev)
 
 	xhci_free_command(xhci, command);
 
+	/* Use GFP_NOIO, since this function can be called from
+	 * xhci_discover_or_reset_device(), which may be called as part of
+	 * mass storage driver error handling.
+	 */
+	if (!xhci_alloc_virt_device(xhci, slot_id, udev, GFP_NOIO)) {
+		xhci_warn(xhci, "Could not allocate xHCI USB device data structures\n");
+		goto disable_slot;
+	}
+	vdev = xhci->devs[slot_id];
+
 	if ((xhci->quirks & XHCI_EP_LIMIT_QUIRK)) {
 		spin_lock_irqsave(&xhci->lock, flags);
 		ret = xhci_reserve_host_control_ep_resources(xhci);
+		spin_unlock_irqrestore(&xhci->lock, flags);
 		if (ret) {
-			spin_unlock_irqrestore(&xhci->lock, flags);
 			xhci_warn(xhci, "Not enough host resources, "
 					"active endpoint contexts = %u\n",
 					xhci->num_active_eps);
+			xhci_free_virt_device(xhci, vdev, slot_id);
+			/* without vdev disable slot won't free ep0 resources */
 			goto disable_slot;
 		}
-		spin_unlock_irqrestore(&xhci->lock, flags);
 	}
-	/* Use GFP_NOIO, since this function can be called from
-	 * xhci_discover_or_reset_device(), which may be called as part of
-	 * mass storage driver error handling.
-	 */
-	if (!xhci_alloc_virt_device(xhci, slot_id, udev, GFP_NOIO)) {
-		xhci_warn(xhci, "Could not allocate xHCI USB device data structures\n");
-		goto disable_slot;
-	}
-	vdev = xhci->devs[slot_id];
 	slot_ctx = xhci_get_slot_ctx(xhci, vdev->out_ctx);
 	trace_xhci_alloc_dev(slot_ctx);
 
@@ -4348,7 +4350,7 @@ int xhci_alloc_dev(struct usb_hcd *hcd, struct usb_device *udev)
 	return 1;
 
 disable_slot:
-	xhci_disable_and_free_slot(xhci, udev->slot_id);
+	xhci_disable_slot(xhci, slot_id);
 
 	return 0;
 }