usb: host: xhci-sideband: fix deadlock in unregister path

When a USB device is disconnected or a driver is unbound, the USB core invokes the driver's disconnect callback while holding the udev device lock. If the driver calls xhci_sideband_unregister(), it eventually reaches usb_offload_put(), which attempts to acquire the same udev lock, resulting in a self-deadlock.

Introduce lockless variants __usb_offload_get() and __usb_offload_put() to allow modifying the offload usage count when the device lock is already held. These helpers use device_lock_assert() to ensure callers meet the locking requirements.

Quoted 10 lines · Guan-Yu Lin

When a USB device is disconnected or a driver is unbound, the USB core invokes the driver's disconnect callback while holding the udev device lock. If the driver calls xhci_sideband_unregister(), it eventually reaches usb_offload_put(), which attempts to acquire the same udev lock, resulting in a self-deadlock.

Introduce lockless variants __usb_offload_get() and __usb_offload_put() to allow modifying the offload usage count when the device lock is already held. These helpers use device_lock_assert() to ensure callers meet the locking requirements.

Ugh. Didn't I warn about this when the original functions were added?

Adding functions with __ is a mess, please make these names, if you _REALLY_ need them, obvious that this is a no lock function.

And now that you added the lockless functions, are there any in-kernel users of the locked versions? At a quick glance I didn't see them, did I miss it somewhere?

thanks,

greg k-h

Quoted 18 lines · Greg KH

Quoted 10 lines · Guan-Yu Lin

When a USB device is disconnected or a driver is unbound, the USB core invokes the driver's disconnect callback while holding the udev device lock. If the driver calls xhci_sideband_unregister(), it eventually reaches usb_offload_put(), which attempts to acquire the same udev lock, resulting in a self-deadlock.

Introduce lockless variants __usb_offload_get() and __usb_offload_put() to allow modifying the offload usage count when the device lock is already held. These helpers use device_lock_assert() to ensure callers meet the locking requirements.

Ugh. Didn't I warn about this when the original functions were added?

Adding functions with __ is a mess, please make these names, if you _REALLY_ need them, obvious that this is a no lock function.

Quoted 7 lines

And now that you added the lockless functions, are there any in-kernel users of the locked versions? At a quick glance I didn't see them, did I miss it somewhere?

thanks,

greg k-h

Hi Greg,

You are right; xhci-sideband.c is the only in-kernel user of the locked versions. I will rename the __ functions to usb_offload_* and remove the locked variants in the next version to clean up the API.

Regarding the deadlock fix itself, we have analyzed two potential solutions. The core issue is that xhci_sideband_unregister() (and xhci_sideband_remove_interrupter()) needs to decrement the offload usage count (which requires the USB device lock), but it is called from the disconnect path where that lock is already held.

Option A: Fix the Callers of xhci_sideband functions
In this approach, we keep the usb_offload calls inside the xhci_sideband functions but replace the internal usb_lock_device() with device_lock_assert(). We then update callers in qc_audio_offload.c to explicitly acquire the lock.
This ensures the offload count remains tightly coupled with the interrupter's lifecycle, though it effectively changes the API
contract: calling xHCI sideband functions now requires holding the USB device lock.

Option B: Decouple usb_offload functions from xhci_sideband functions In this approach, we strip the usb_offload calls out of xhci_sideband functions entirely. The client driver (qc_audio_offload) becomes responsible for the full transaction: acquiring the lock, managing usb_offload_get/put(), and creating/removing the interrupter. This restores clean encapsulation (xHCI functions only handle hardware), but it places the burden on the client driver to correctly balance the usb_offload calls.

I lean towards Option A to ensure the offload count implies an active interrupter by design, but please let me know if you prefer the cleaner separation of Option B.

Quoted 58 lines · Guan-Yu Lin

Quoted 18 lines · Greg KH

Quoted 10 lines · Guan-Yu Lin

When a USB device is disconnected or a driver is unbound, the USB core invokes the driver's disconnect callback while holding the udev device lock. If the driver calls xhci_sideband_unregister(), it eventually reaches usb_offload_put(), which attempts to acquire the same udev lock, resulting in a self-deadlock.

Introduce lockless variants __usb_offload_get() and __usb_offload_put() to allow modifying the offload usage count when the device lock is already held. These helpers use device_lock_assert() to ensure callers meet the locking requirements.

Ugh. Didn't I warn about this when the original functions were added?

Adding functions with __ is a mess, please make these names, if you _REALLY_ need them, obvious that this is a no lock function.

Quoted 7 lines

And now that you added the lockless functions, are there any in-kernel users of the locked versions? At a quick glance I didn't see them, did I miss it somewhere?

thanks,

greg k-h

Hi Greg,

You are right; xhci-sideband.c is the only in-kernel user of the locked versions. I will rename the __ functions to usb_offload_* and remove the locked variants in the next version to clean up the API.

Regarding the deadlock fix itself, we have analyzed two potential solutions. The core issue is that xhci_sideband_unregister() (and xhci_sideband_remove_interrupter()) needs to decrement the offload usage count (which requires the USB device lock), but it is called from the disconnect path where that lock is already held.

Option A: Fix the Callers of xhci_sideband functions
In this approach, we keep the usb_offload calls inside the xhci_sideband functions but replace the internal usb_lock_device() with device_lock_assert(). We then update callers in qc_audio_offload.c to explicitly acquire the lock.
This ensures the offload count remains tightly coupled with the interrupter's lifecycle, though it effectively changes the API
contract: calling xHCI sideband functions now requires holding the USB device lock.

Option B: Decouple usb_offload functions from xhci_sideband functions In this approach, we strip the usb_offload calls out of xhci_sideband functions entirely. The client driver (qc_audio_offload) becomes responsible for the full transaction: acquiring the lock, managing usb_offload_get/put(), and creating/removing the interrupter. This restores clean encapsulation (xHCI functions only handle hardware), but it places the burden on the client driver to correctly balance the usb_offload calls.

Quoted 3 lines

I lean towards Option A to ensure the offload count implies an active interrupter by design, but please let me know if you prefer the cleaner separation of Option B.

I would prefer option B
Decouple the offload from sideband.

The secondary interrupter in sideband was specifically createad for qc_audio_offload.

Vendors using the xHCI hardware Audio sideband Capability (xHCI 7.9) won't use a secondary interrupter, but might sill want to prevent suspending the device. So it shuold be better to make this decision in the class driver.

The offload count shoudn't be that complicated. Isn't it binary at the moment? We don't allow more than one sideband per device, and it can only have one interrupter.

I unfortunately couldn't participate in the review and development of drivers/usb/core/offload.c, but my original idea before it was implemented was to keep usb core out of sideband as much as possible as its not really a part of usb specification.

This is also why I added the sideband pointer to struct xhci_virt_device. It allows us to figure out if a device is dedicated for sideband use.

so xhci_sideband_check() could be something like

bool xhci_sideband_check(struct xhci_hcd *xhci)

{
        guard(spinlock_irqsave)(&xhci->lock);

        for (int i = 1; i < HC_MAX_SLOTS; i++) {
        if (xhci->devs[i] && xhci->devs[i]->sideband)
                return true;
        }
        return false;
}

Thanks
Mathias

Regarding xhci_sideband_check():
I have a concern regarding power management with the proposed check:

if (xhci->devs[i] && xhci->devs[i]->sideband)

         return true;

vdev->sideband is assigned during xhci_sideband_register(), which happens when the class driver probes (device connection), and it persists until disconnect. If we use this check, the xHCI controller will be prevented from PM suspending (system suspend) as long as the device is connected, even if it is idle (not playing audio). For mobile power optimization, we need to allow the controller to suspend when the sideband is registered but idle.

Since we are proceeding with Option B, the class driver will be maintaining the udev->offload_usage count via usb_offload_get/put(). I propose we still rely on usb_offload_check() (or check offload_usage) within the xHCI sideband check. This ensures we only block or adjust the PM suspend flow only when there is active data transfer.