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

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

[Weitao Wang]

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>
---
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);
-- 
2.32.0

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

[Mathias Nyman]

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

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

[Mathias Nyman]

On 29.7.2025 20.25, WeitaoWang-oc@zhaoxin.com wrote:
> On 2025/7/28 21:16, Mathias Nyman wrote:
>>
>> 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);
>> }
> 
> Hi Mathias,
> 
> Yes, your suggestion is a better revision, I made some modifications
> to the patch which is listed below. Please help to review again.
> Thanks for your help.
> 
> ---
>   drivers/usb/host/xhci-hub.c  |  3 +--
>   drivers/usb/host/xhci-mem.c  | 21 ++++++++++-----------
>   drivers/usb/host/xhci-ring.c |  9 +++++++--
>   drivers/usb/host/xhci.c      | 23 ++++++++++++++++-------
>   drivers/usb/host/xhci.h      |  3 ++-
>   5 files changed, 36 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
> index 92bb84f8132a..b3a59ce1b3f4 100644
> --- a/drivers/usb/host/xhci-hub.c
> +++ b/drivers/usb/host/xhci-hub.c
> @@ -704,8 +704,7 @@ static int xhci_enter_test_mode(struct xhci_hcd *xhci,
>           if (!xhci->devs[i])
>               continue;
> 
> -        retval = xhci_disable_slot(xhci, i);
> -        xhci_free_virt_device(xhci, i);
> +        retval = xhci_disable_and_free_slot(xhci, i);
>           if (retval)
>               xhci_err(xhci, "Failed to disable slot %d, %d. Enter test mode anyway\n",
>                    i, retval);
> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> index 6680afa4f596..fc4aca2e65bc 100644
> --- a/drivers/usb/host/xhci-mem.c
> +++ b/drivers/usb/host/xhci-mem.c
> @@ -865,21 +865,18 @@ int xhci_alloc_tt_info(struct xhci_hcd *xhci,
>    * will be manipulated by the configure endpoint, allocate device, or update
>    * hub functions while this function is removing the TT entries from the list.
>    */
> -void xhci_free_virt_device(struct xhci_hcd *xhci, int slot_id)
> +void xhci_free_virt_device(struct xhci_hcd *xhci, struct xhci_virt_device *dev,
> +        int slot_id)
>   {
> -    struct xhci_virt_device *dev;
>       int i;
>       int old_active_eps = 0;
> 
>       /* Slot ID 0 is reserved */
> -    if (slot_id == 0 || !xhci->devs[slot_id])
> +    if (slot_id == 0 || !dev)
>           return;
> 
> -    dev = xhci->devs[slot_id];
> -
> -    xhci->dcbaa->dev_context_ptrs[slot_id] = 0;
> -    if (!dev)
> -        return;
> +    if (xhci->dcbaa->dev_context_ptrs[slot_id] == dev->out_ctx->dma)

forgot that dev_context_ptrs[] values are stored as le64 while
out_ctx->dma  is in whatever cpu uses.

So above should be:
if (xhci->dcbaa->dev_context_ptrs[slot_id] == cpu_to_le64(dev->out_ctx->dma))

Otherwise it looks good to me

Thanks
Mathias

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

[WeitaoWang-oc]

On 2025/7/28 21:16, Mathias Nyman wrote:
> 
> 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);
> }

Hi Mathias,

Yes, your suggestion is a better revision, I made some modifications
to the patch which is listed below. Please help to review again.
Thanks for your help.

---
  drivers/usb/host/xhci-hub.c  |  3 +--
  drivers/usb/host/xhci-mem.c  | 21 ++++++++++-----------
  drivers/usb/host/xhci-ring.c |  9 +++++++--
  drivers/usb/host/xhci.c      | 23 ++++++++++++++++-------
  drivers/usb/host/xhci.h      |  3 ++-
  5 files changed, 36 insertions(+), 23 deletions(-)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 92bb84f8132a..b3a59ce1b3f4 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -704,8 +704,7 @@ static int xhci_enter_test_mode(struct xhci_hcd *xhci,
  		if (!xhci->devs[i])
  			continue;

-		retval = xhci_disable_slot(xhci, i);
-		xhci_free_virt_device(xhci, i);
+		retval = xhci_disable_and_free_slot(xhci, i);
  		if (retval)
  			xhci_err(xhci, "Failed to disable slot %d, %d. Enter test mode anyway\n",
  				 i, retval);
diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 6680afa4f596..fc4aca2e65bc 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -865,21 +865,18 @@ int xhci_alloc_tt_info(struct xhci_hcd *xhci,
   * will be manipulated by the configure endpoint, allocate device, or update
   * hub functions while this function is removing the TT entries from the list.
   */
-void xhci_free_virt_device(struct xhci_hcd *xhci, int slot_id)
+void xhci_free_virt_device(struct xhci_hcd *xhci, struct xhci_virt_device *dev,
+		int slot_id)
  {
-	struct xhci_virt_device *dev;
  	int i;
  	int old_active_eps = 0;

  	/* Slot ID 0 is reserved */
-	if (slot_id == 0 || !xhci->devs[slot_id])
+	if (slot_id == 0 || !dev)
  		return;

-	dev = xhci->devs[slot_id];
-
-	xhci->dcbaa->dev_context_ptrs[slot_id] = 0;
-	if (!dev)
-		return;
+	if (xhci->dcbaa->dev_context_ptrs[slot_id] == dev->out_ctx->dma)
+		xhci->dcbaa->dev_context_ptrs[slot_id] = 0;

  	trace_xhci_free_virt_device(dev);

@@ -920,8 +917,10 @@ void xhci_free_virt_device(struct xhci_hcd *xhci, int slot_id)
  		dev->udev->slot_id = 0;
  	if (dev->rhub_port && dev->rhub_port->slot_id == slot_id)
  		dev->rhub_port->slot_id = 0;
-	kfree(xhci->devs[slot_id]);
-	xhci->devs[slot_id] = NULL;
+	if (xhci->devs[slot_id] == dev)
+		xhci->devs[slot_id] = NULL;
+	kfree(dev);
  }

  /*
@@ -962,7 +961,7 @@ static void xhci_free_virt_devices_depth_first(struct xhci_hcd *xhci, 
int slot_i
  out:
  	/* we are now at a leaf device */
  	xhci_debugfs_remove_slot(xhci, slot_id);
-	xhci_free_virt_device(xhci, slot_id);
+	xhci_free_virt_device(xhci, vdev, slot_id);
  }

  int xhci_alloc_virt_device(struct xhci_hcd *xhci, int slot_id,
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 94c9c9271658..7a440ec52ff6 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,10 @@ 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->dcbaa->dev_context_ptrs[slot_id] = 0;
+		xhci->devs[slot_id] = NULL;
+	}
  }

  static void xhci_handle_cmd_config_ep(struct xhci_hcd *xhci, int slot_id)
@@ -1853,7 +1858,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..b3b39b2498f6 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -3930,8 +3930,7 @@ static int xhci_discover_or_reset_device(struct usb_hcd *hcd,
  		 * Obtaining a new device slot to inform the xHCI host that
  		 * the USB device has been reset.
  		 */
-		ret = xhci_disable_slot(xhci, udev->slot_id);
-		xhci_free_virt_device(xhci, udev->slot_id);
+		ret = xhci_disable_and_free_slot(xhci, udev->slot_id);
  		if (!ret) {
  			ret = xhci_alloc_dev(hcd, udev);
  			if (ret == 1)
@@ -4088,7 +4087,7 @@ static void xhci_free_dev(struct usb_hcd *hcd, struct usb_device *udev)
  	xhci_disable_slot(xhci, udev->slot_id);

  	spin_lock_irqsave(&xhci->lock, flags);
-	xhci_free_virt_device(xhci, udev->slot_id);
+	xhci_free_virt_device(xhci, virt_dev, udev->slot_id);
  	spin_unlock_irqrestore(&xhci->lock, flags);

  }
@@ -4137,6 +4136,18 @@ int xhci_disable_slot(struct xhci_hcd *xhci, u32 slot_id)
  	return 0;
  }

+int xhci_disable_and_free_slot(struct xhci_hcd *xhci, u32 slot_id)
+{
+	struct xhci_virt_device *vdev = xhci->devs[slot_id];
+	int ret;
+
+	ret = xhci_disable_slot(xhci, slot_id);
+	xhci_free_virt_device(xhci, vdev, slot_id);
+	return ret;
+}
+
  /*
   * Checks if we have enough host controller resources for the default control
   * endpoint.
@@ -4243,8 +4254,7 @@ 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);
+	xhci_disable_and_free_slot(xhci, udev->slot_id);

  	return 0;
  }
@@ -4380,8 +4390,7 @@ static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device 
*udev,
  		dev_warn(&udev->dev, "Device not responding to setup %s.\n", act);

  		mutex_unlock(&xhci->mutex);
-		ret = xhci_disable_slot(xhci, udev->slot_id);
-		xhci_free_virt_device(xhci, udev->slot_id);
+		ret = xhci_disable_and_free_slot(xhci, udev->slot_id);
  		if (!ret) {
  			if (xhci_alloc_dev(hcd, udev) == 1)
  				xhci_setup_addressable_virt_dev(xhci, udev);
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index a20f4e7cd43a..85d5b964bf1e 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1791,7 +1791,7 @@ void xhci_dbg_trace(struct xhci_hcd *xhci, void (*trace)(struct 
va_format *),
  /* xHCI memory management */
  void xhci_mem_cleanup(struct xhci_hcd *xhci);
  int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags);
-void xhci_free_virt_device(struct xhci_hcd *xhci, int slot_id);
+void xhci_free_virt_device(struct xhci_hcd *xhci, struct xhci_virt_device *dev, int slot_id);
  int xhci_alloc_virt_device(struct xhci_hcd *xhci, int slot_id, struct usb_device *udev, 
gfp_t flags);
  int xhci_setup_addressable_virt_dev(struct xhci_hcd *xhci, struct usb_device *udev);
  void xhci_copy_ep0_dequeue_into_input_ctx(struct xhci_hcd *xhci,
@@ -1888,6 +1888,7 @@ void xhci_reset_bandwidth(struct usb_hcd *hcd, struct usb_device *udev);
  int xhci_update_hub_device(struct usb_hcd *hcd, struct usb_device *hdev,
  			   struct usb_tt *tt, gfp_t mem_flags);
  int xhci_disable_slot(struct xhci_hcd *xhci, u32 slot_id);
+int xhci_disable_and_free_slot(struct xhci_hcd *xhci, u32 slot_id);
  int xhci_ext_cap_init(struct xhci_hcd *xhci);

  int xhci_suspend(struct xhci_hcd *xhci, bool do_wakeup);
-- 

Best Regards,
weitao

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

[WeitaoWang-oc]

On 2025/7/29 23:00, Mathias Nyman wrote:
> 
> On 29.7.2025 20.25, WeitaoWang-oc@zhaoxin.com wrote:
>> On 2025/7/28 21:16, Mathias Nyman wrote:
>>>
>>> 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);
>>> }
>>
>> Hi Mathias,
>>
>> Yes, your suggestion is a better revision, I made some modifications
>> to the patch which is listed below. Please help to review again.
>> Thanks for your help.
>>
>> ---
>>   drivers/usb/host/xhci-hub.c  |  3 +--
>>   drivers/usb/host/xhci-mem.c  | 21 ++++++++++-----------
>>   drivers/usb/host/xhci-ring.c |  9 +++++++--
>>   drivers/usb/host/xhci.c      | 23 ++++++++++++++++-------
>>   drivers/usb/host/xhci.h      |  3 ++-
>>   5 files changed, 36 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
>> index 92bb84f8132a..b3a59ce1b3f4 100644
>> --- a/drivers/usb/host/xhci-hub.c
>> +++ b/drivers/usb/host/xhci-hub.c
>> @@ -704,8 +704,7 @@ static int xhci_enter_test_mode(struct xhci_hcd *xhci,
>>           if (!xhci->devs[i])
>>               continue;
>>
>> -        retval = xhci_disable_slot(xhci, i);
>> -        xhci_free_virt_device(xhci, i);
>> +        retval = xhci_disable_and_free_slot(xhci, i);
>>           if (retval)
>>               xhci_err(xhci, "Failed to disable slot %d, %d. Enter test mode anyway\n",
>>                    i, retval);
>> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
>> index 6680afa4f596..fc4aca2e65bc 100644
>> --- a/drivers/usb/host/xhci-mem.c
>> +++ b/drivers/usb/host/xhci-mem.c
>> @@ -865,21 +865,18 @@ int xhci_alloc_tt_info(struct xhci_hcd *xhci,
>>    * will be manipulated by the configure endpoint, allocate device, or update
>>    * hub functions while this function is removing the TT entries from the list.
>>    */
>> -void xhci_free_virt_device(struct xhci_hcd *xhci, int slot_id)
>> +void xhci_free_virt_device(struct xhci_hcd *xhci, struct xhci_virt_device *dev,
>> +        int slot_id)
>>   {
>> -    struct xhci_virt_device *dev;
>>       int i;
>>       int old_active_eps = 0;
>>
>>       /* Slot ID 0 is reserved */
>> -    if (slot_id == 0 || !xhci->devs[slot_id])
>> +    if (slot_id == 0 || !dev)
>>           return;
>>
>> -    dev = xhci->devs[slot_id];
>> -
>> -    xhci->dcbaa->dev_context_ptrs[slot_id] = 0;
>> -    if (!dev)
>> -        return;
>> +    if (xhci->dcbaa->dev_context_ptrs[slot_id] == dev->out_ctx->dma)
> 
> forgot that dev_context_ptrs[] values are stored as le64 while
> out_ctx->dma  is in whatever cpu uses.
> 
> So above should be:
> if (xhci->dcbaa->dev_context_ptrs[slot_id] == cpu_to_le64(dev->out_ctx->dma))
> 
> Otherwise it looks good to me

Ok, I got it. I'll submit a new version with this issue fix.

Best Regards,
weitao