Message-ID: <CAOuDEK3xzpY7Cr8JgactH=Sy=h7aTEgdTqUiuX8xh6gvUNR5uw@mail.gmail.com>
Date: Wed, 4 Feb 2026 17:15:00 +0800
From: Guan-Yu Lin <guanyulin@...gle.com>
To: Mathias Nyman <mathias.nyman@...ux.intel.com>
Cc: Greg KH <gregkh@...uxfoundation.org>, mathias.nyman@...el.com, 
	stern@...land.harvard.edu, wesley.cheng@....qualcomm.com, 
	linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org, 
	stable@...r.kernel.org
Subject: Re: [RFC PATCH] usb: host: xhci-sideband: fix deadlock in unregister path

On Tue, Feb 3, 2026 at 10:14 PM Mathias Nyman
<mathias.nyman@...ux.intel.com> wrote:
>
> On 2/2/26 12:03, Guan-Yu Lin wrote:
> > On Sat, Jan 31, 2026 at 8:15 PM Greg KH <gregkh@...uxfoundation.org> wrote:
> >>
> >> On Fri, Jan 30, 2026 at 07:47:46AM +0000, Guan-Yu Lin wrote:
> >>> 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
> >
> > 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.
>
> 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

Hi Mathias,

Thanks for the feedback.
I will proceed with Option B as you suggested. Decoupling the offload
logic from the sideband mechanism seems cleaner and places the
responsibility correctly on the class driver (qc_audio_offload) to
manage the offload state.

I will implement the following changes in v2:
1. API Cleanup: As Greg requested, I will rename __usb_offload_* to
usb_offload_* and remove the locked variants. These functions will use
device_lock_assert() to ensure the caller holds the lock.
2. Class Driver Logic: qc_audio_offload will handle locking udev and
calling usb_offload_get/put() directly.

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.

Regards,
Guan-Yu

Powered by blists - more mailing lists