Re: [PATCH v2] usb:xhci:Fix slot_id resource race conflict

[Mathias Nyman <mathias.nyman@linux.intel.com>]

On 25.7.2025 21.51, Weitao Wang wrote:
> In such a scenario, device-A with slot_id equal to 1 is disconnecting
> while device-B is enumerating, device-B will fail to enumerate in the
> follow sequence.
> 
> 1.[device-A] send disable slot command
> 2.[device-B] send enable slot command
> 3.[device-A] disable slot command completed and wakeup waiting thread
> 4.[device-B] enable slot command completed with slot_id equal to 1 and
> wakeup waiting thread
> 5.[device-B] driver check this slot_id was used by someone(device-A) in
> xhci_alloc_virt_device, this device fails to enumerate as this conflict
> 6.[device-A] xhci->devs[slot_id] set to NULL in xhci_free_virt_device
> 
> To fix driver's slot_id resources conflict, let the xhci_free_virt_device
> functionm call in the interrupt handler when disable slot command success.
> 
> Cc: stable@vger.kernel.org
> Fixes: 7faac1953ed1 ("xhci: avoid race between disable slot command and host runtime suspend")
> Signed-off-by: Weitao Wang <WeitaoWang-oc@zhaoxin.com>

Nice catch, good to get this fixed.

This however has the downside of doing a lot in interrupt context.

what if we only clear some strategic pointers in the interrupt context,
and then do all the actual unmapping and endpoint ring segments freeing,
contexts freeing ,etc later?

Pseudocode:

xhci_handle_cmd_disable_slot(xhci, slot_id, comp_code)
{
     if (cmd_comp_code == COMP_SUCCESS) {
         xhci->dcbaa->dev_context_ptrs[slot_id] = 0;
         xhci->devs[slot_id] = NULL;
     }
}

xhci_disable_and_free_slot(xhci, slot_id)
{
     struct xhci_virt_device *vdev = xhci->devs[slot_id];

     xhci_disable_slot(xhci, slot_id);
     xhci_free_virt_device(xhci, vdev, slot_id);
}

xhci_free_virt_device(xhci, vdev, slot_id)
{
     if (xhci->dcbaa->dev_context_ptrs[slot_id] == vdev->out_ctx->dma)
         xhci->dcbaa->dev_context_ptrs[slot_id] = 0;

     // free and unmap things just like before
     ...

     if (xhci->devs[slot_id] == vdev)
         xhci->devs[slot_id] = NULL;

     kfee(vdev);
}


> ---
> v1->v2
>   - Adjust the lock position in the function xhci_free_dev.
> 
>   drivers/usb/host/xhci-hub.c  |  5 +++--
>   drivers/usb/host/xhci-ring.c |  7 +++++--
>   drivers/usb/host/xhci.c      | 35 +++++++++++++++++++++++++----------
>   3 files changed, 33 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
> index 92bb84f8132a..fd8a64aa5779 100644
> --- a/drivers/usb/host/xhci-hub.c
> +++ b/drivers/usb/host/xhci-hub.c
> @@ -705,10 +705,11 @@ static int xhci_enter_test_mode(struct xhci_hcd *xhci,
>   			continue;
>   
>   		retval = xhci_disable_slot(xhci, i);
> -		xhci_free_virt_device(xhci, i);
> -		if (retval)
> +		if (retval) {
>   			xhci_err(xhci, "Failed to disable slot %d, %d. Enter test mode anyway\n",
>   				 i, retval);
> +			xhci_free_virt_device(xhci, i);
> +		}
>   	}
>   	spin_lock_irqsave(&xhci->lock, *flags);
>   	/* Put all ports to the Disable state by clear PP */
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 94c9c9271658..93dc28399c3c 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -1589,7 +1589,8 @@ static void xhci_handle_cmd_enable_slot(int slot_id, struct xhci_command *comman
>   		command->slot_id = 0;
>   }
>   
> -static void xhci_handle_cmd_disable_slot(struct xhci_hcd *xhci, int slot_id)
> +static void xhci_handle_cmd_disable_slot(struct xhci_hcd *xhci, int slot_id,
> +					u32 cmd_comp_code)
>   {
>   	struct xhci_virt_device *virt_dev;
>   	struct xhci_slot_ctx *slot_ctx;
> @@ -1604,6 +1605,8 @@ static void xhci_handle_cmd_disable_slot(struct xhci_hcd *xhci, int slot_id)
>   	if (xhci->quirks & XHCI_EP_LIMIT_QUIRK)
>   		/* Delete default control endpoint resources */
>   		xhci_free_device_endpoint_resources(xhci, virt_dev, true);
> +	if (cmd_comp_code == COMP_SUCCESS)
> +		xhci_free_virt_device(xhci, slot_id);
>   }
>   
>   static void xhci_handle_cmd_config_ep(struct xhci_hcd *xhci, int slot_id)
> @@ -1853,7 +1856,7 @@ static void handle_cmd_completion(struct xhci_hcd *xhci,
>   		xhci_handle_cmd_enable_slot(slot_id, cmd, cmd_comp_code);
>   		break;
>   	case TRB_DISABLE_SLOT:
> -		xhci_handle_cmd_disable_slot(xhci, slot_id);
> +		xhci_handle_cmd_disable_slot(xhci, slot_id, cmd_comp_code);
>   		break;
>   	case TRB_CONFIG_EP:
>   		if (!cmd->completion)
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 8a819e853288..6c6f6ebb8953 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -3931,13 +3931,14 @@ static int xhci_discover_or_reset_device(struct usb_hcd *hcd,
>   		 * the USB device has been reset.
>   		 */
>   		ret = xhci_disable_slot(xhci, udev->slot_id);
> -		xhci_free_virt_device(xhci, udev->slot_id);
>   		if (!ret) {
>   			ret = xhci_alloc_dev(hcd, udev);
>   			if (ret == 1)
>   				ret = 0;
>   			else
>   				ret = -EINVAL;
> +		} else {
> +			xhci_free_virt_device(xhci, udev->slot_id);
>   		}
>   		return ret;
>   	}
> @@ -4085,11 +4086,12 @@ static void xhci_free_dev(struct usb_hcd *hcd, struct usb_device *udev)
>   	for (i = 0; i < 31; i++)
>   		virt_dev->eps[i].ep_state &= ~EP_STOP_CMD_PENDING;
>   	virt_dev->udev = NULL;
> -	xhci_disable_slot(xhci, udev->slot_id);
> -
> -	spin_lock_irqsave(&xhci->lock, flags);
> -	xhci_free_virt_device(xhci, udev->slot_id);
> -	spin_unlock_irqrestore(&xhci->lock, flags);
> +	ret = xhci_disable_slot(xhci, udev->slot_id);
> +	if (ret) {
> +		spin_lock_irqsave(&xhci->lock, flags);
> +		xhci_free_virt_device(xhci, udev->slot_id);
> +		spin_unlock_irqrestore(&xhci->lock, flags);
> +	}
>   
>   }
>   
> @@ -4128,9 +4130,20 @@ int xhci_disable_slot(struct xhci_hcd *xhci, u32 slot_id)
>   
>   	wait_for_completion(command->completion);
>   
> -	if (command->status != COMP_SUCCESS)
> +	if (command->status != COMP_SUCCESS) {
>   		xhci_warn(xhci, "Unsuccessful disable slot %u command, status %d\n",
>   			  slot_id, command->status);
> +		switch (command->status) {
> +		case COMP_COMMAND_ABORTED:
> +		case COMP_COMMAND_RING_STOPPED:
> +			xhci_warn(xhci, "Timeout while waiting for disable slot command\n");
> +			ret = -ETIME;
> +			break;
> +		default:
> +			ret = -EINVAL;
> +			break;
> +		}
> +	}
>   
>   	xhci_free_command(xhci, command);
>   
> @@ -4243,8 +4256,9 @@ int xhci_alloc_dev(struct usb_hcd *hcd, struct usb_device *udev)
>   	return 1;
>   
>   disable_slot:
> -	xhci_disable_slot(xhci, udev->slot_id);
> -	xhci_free_virt_device(xhci, udev->slot_id);
> +	ret = xhci_disable_slot(xhci, udev->slot_id);
> +	if (ret)
> +		xhci_free_virt_device(xhci, udev->slot_id);
>   
>   	return 0;
>   }
> @@ -4381,10 +4395,11 @@ static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev,
>   
>   		mutex_unlock(&xhci->mutex);
>   		ret = xhci_disable_slot(xhci, udev->slot_id);
> -		xhci_free_virt_device(xhci, udev->slot_id);
>   		if (!ret) {
>   			if (xhci_alloc_dev(hcd, udev) == 1)
>   				xhci_setup_addressable_virt_dev(xhci, udev);
> +		} else {
> +			xhci_free_virt_device(xhci, udev->slot_id);
>   		}
>   		kfree(command->completion);
>   		kfree(command);