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

["Neronin, Niklas" <niklas.neronin@linux.intel.com>]

On 07/10/2026 0.02, Michal Pecio wrote:
> 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:

That's a good point. I found this while working on a separate series
that splits 'vdev' allocation and slot enabling. That series was not
ready for this merge window, so I decided to submit only the simpler
fixes first to keep the future series smaller and less cluttered.
As you suggested, that series moves 'vdev' allocation before EP
reservation.

> 
> 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.

Good catch. I found the same issue but apparently forgot to include the
fix in this patch set.

Probably best if this patch is not added to this merge window,
instead I'll re-submit it with all the virtual device changes.

Thanks,
Niklas

> 
> 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;
>  }