Re: 答复: [PATCH] xhci: sideband: check vdev liveness before removing endpoints on unregister

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

On 9/14/26 15:24, 胡连勤 wrote:
> Hi Michal,
> 
>>> Thanks for the analysis. A clarification on the hub_event() reference
>>> in my patch:
>>>
>>> The crash trace shows hub_event() at the top because that's the actual
>>> crash call stack from the failing device. The full sequence is:
>>>
>>>    hub_event()
>>>      -> port_event()                           [hub.c:5966]
>>>         -> usb_reset_device(udev)              [hub.c:5875]
>>>            -> usb_reset_and_verify_device()    [hub.c:6183]
>>>               -> hub_port_init()               [hub.c:6228]
>>>                  -> hcd->driver->address_device()  [hub.c:4781]
>>>                     -> xhci_setup_device()     <-- COMP_USB_TRANSACTION_ERROR
>>>                        -> xhci_disable_and_free_slot()  [xhci.c:4438]
>>>                           -> xhci_free_virt_device()   <-- frees vdev here
>>
>> So not a mistake and this is indeed a dangerous case. And AFAICT, in
>> this path udev's pre_reset() routine isn't called and therefore can't
>> be used to fix your issue, unless USB core is patched to call it.
>>
> Actually it is. The crash path goes through usb_reset_device()
> (hub.c:5875), which calls drv->pre_reset() at hub.c:6412 before
> usb_reset_and_verify_device(). So pre_reset/post_reset is viable —
> if the sideband client is a USB interface driver.
> 
>> But xhci_discover_or_reset_device() is called: before hub_port_init()
>> calls problematic hub_enable_device() / hub_address_device() functions,
>> it calls hub_port_reset(), which calls hcd->driver->reset_device().
>>
>> But I'm not entirely sure what happens if reset fails before this call
>> is made and then hub_port_init() jumps to re_enumerate. The function
>> bails out, but sooner or later somebody will try to free this device in
>> some manner, I suppose, so what happens then?
>>
> By the time we reach re_enumerate (hub.c:6235), xhci_setup_device()
> has already freed vdev+rings via xhci_disable_and_free_slot()
> (xhci.c:4438) without notifying sideband. The later xhci_free_dev()
> is a no-op because xhci->devs[slot_id] is already NULL. So the
> dangerous window is between xhci_discover_or_reset_device() (with
> callback) and xhci_setup_device() failure (without callback).
> Given pre_reset() is available, the proposed fix:
> 
> 1. Sideband client implements pre_reset() to unregister and stop
>      ring access before reset.

Sounds good, call xhci_sideband_remove_endpoint() for every offloaded endpoint.
If possible then maybe even unregister sideband for this device completely here.


> 2. Add a sideband callback in xhci_free_virt_device() for defense
>      in depth.

Selvarasu Ganesan pointed out that xhci 'core' in fact doesn't include
xhci-sideband.h yet. If possible I'd like to keep it that way.

Setting xhci->sideband->vdev to NULL, or calling a callback here changes this
and is the first time we then intertwine xhci core with sideband.

Long term solution is to not reallocate vdev just because we try to disable and
re-enable the slot to recover from a failed address device command.
Usb core doesn't free and reallocate udev during device reset either.

Niklas just started looking at decoupling vdev allocation and initalization.
Meanwhile we could try a bandaid like:

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index a9e47e178c28..0b5152a1a301 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -4435,11 +4435,15 @@ 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_and_free_slot(xhci, udev->slot_id);
-		if (!ret) {
-			if (xhci_alloc_dev(hcd, udev) == 1)
-				xhci_setup_addressable_virt_dev(xhci, udev);
+
+		if (!virt_dev->sideband) {
+			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);
+			}
  		}
+
  		kfree(command->completion);
  		kfree(command);
  		return -EPROTO;

Does this work in your case?
Can you see any negative side-effects with this solution like never re-enumerating and
recovering after a failed address device command?

Thanks
Mathias