Re: [PATCH v2 1/2] usb: offload: move device locking to callers in offload.c

[Guan-Yu Lin]

On Wed, Mar 11, 2026 at 5:26 AM Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On Mon, Mar 09, 2026 at 02:22:04AM +0000, Guan-Yu Lin wrote:
> > Update usb_offload_get() and usb_offload_put() to require that the
> > caller holds the USB device lock. Remove the internal call to
> > usb_lock_device() and add device_lock_assert() to ensure synchronization
> > is handled by the caller. These functions continue to manage the
> > device's power state via autoresume/autosuspend and update the
> > offload_usage counter.
> >
> > Additionally, decouple the xHCI sideband interrupter lifecycle from the
> > offload usage counter by removing the calls to usb_offload_get() and
> > usb_offload_put() from the interrupter creation and removal paths. This
> > allows interrupters to be managed independently of the device's offload
> > activity status.
> >
> > Cc: stable@xxxxxxxxxxxxxxx
> > Fixes: ef82a4803aab ("xhci: sideband: add api to trace sideband usage")
> > Signed-off-by: Guan-Yu Lin <guanyulin@xxxxxxxxxx>
> > Tested-by: Hailong Liu <hailong.liu@xxxxxxxx>
> > ---
> >  drivers/usb/core/offload.c       | 34 +++++++++++---------------------
> >  drivers/usb/host/xhci-sideband.c | 14 +------------
> >  2 files changed, 13 insertions(+), 35 deletions(-)
> >
> > diff --git a/drivers/usb/core/offload.c b/drivers/usb/core/offload.c
> > index 7c699f1b8d2b..e13a4c21d61b 100644
> > --- a/drivers/usb/core/offload.c
> > +++ b/drivers/usb/core/offload.c
> > @@ -20,6 +20,7 @@
> >   * enabled on this usb_device; that is, another entity is actively handling USB
> >   * transfers. This information allows the USB driver to adjust its power
> >   * management policy based on offload activity.
> > + * The caller must hold @udev's device lock.
>
> Ok, but:
>
> >   *
> >   * Return: 0 on success. A negative error code otherwise.
> >   */
> > @@ -27,31 +28,25 @@ int usb_offload_get(struct usb_device *udev)
>
> Why are you not using the __must_hold() definition here?
>

Thanks for the suggestion, __must_hold() will be added in the next version.

> >  {
> >       int ret;
> >
> > -     usb_lock_device(udev);
> > -     if (udev->state == USB_STATE_NOTATTACHED) {
> > -             usb_unlock_device(udev);
> > +     device_lock_assert(&udev->dev);
>
> That's going to splat at runtime, not compile time, which is when you
> really want to check for this, right?
>
> And I thought all of the locking was messy before, and you cleaned it up
> to be nicer here, why go back to the "old" way?  Having a caller be
> forced to have a lock held is ripe for problems...
>

The challenge is that the USB stack automatically holds the lock
during the hardware/software USB connection change. But USB locks are
not held when we create/remove xhci sideband interrupters. Hence, we
need to manipulate the locks by ourselves to distinguish between these
2 usecases. What's your suggestion on this sceneario? Do you have
other options in mind?

> You also are not changing any callers to usb_offload_get() in this
> patch, so does this leave the kernel tree in a broken state?  If not,
> why not?  If so, that's not ok :(
>

The current upstream implementation triggers deadlocks in some cases.
This patch simply disassociates the offload counter manipulation from
sideband interrupt creation to address the deadlock. After applying
the patch, the offload counter won't update until the next patch in
this series is applied. Is this considered a broken state? Should I
squash the two commits into one, or keep them as they were?

>
> > +
> > +     if (udev->state == USB_STATE_NOTATTACHED)
> >               return -ENODEV;
> > -     }
> >
> >       if (udev->state == USB_STATE_SUSPENDED ||
> > -                udev->offload_at_suspend) {
> > -             usb_unlock_device(udev);
> > +         udev->offload_at_suspend)
>
> Can't that really all be on one line?
>

Sure, Let me change it to one line.

> >               return -EBUSY;
> > -     }
> >
> >       /*
> >        * offload_usage could only be modified when the device is active, since
> >        * it will alter the suspend flow of the device.
> >        */
> >       ret = usb_autoresume_device(udev);
> > -     if (ret < 0) {
> > -             usb_unlock_device(udev);
> > +     if (ret < 0)
> >               return ret;
> > -     }
> >
> >       udev->offload_usage++;
> >       usb_autosuspend_device(udev);
> > -     usb_unlock_device(udev);
> >
> >       return ret;
> >  }
> > @@ -64,6 +59,7 @@ EXPORT_SYMBOL_GPL(usb_offload_get);
> >   * The inverse operation of usb_offload_get, which drops the offload_usage of
> >   * a USB device. This information allows the USB driver to adjust its power
> >   * management policy based on offload activity.
> > + * The caller must hold @udev's device lock.
> >   *
> >   * Return: 0 on success. A negative error code otherwise.
> >   */
> > @@ -71,33 +67,27 @@ int usb_offload_put(struct usb_device *udev)
>
> Again, use __must_hold() here, to catch build time issues.
>
> And again, I don't see any code changes to reflect this new requirement
> :(
>
> thanks,
>
> greg k-h

Thanks for the suggestion, The __must_hold() macro will be adaped in
the next version.

Regards,
Guan-Yu