Re: [PATCH v3 1/2] usb: core: use dedicated spinlock for offload state

[Guan-Yu Lin]

On Thu, Mar 26, 2026 at 6:58 PM Mathias Nyman
<mathias.nyman@xxxxxxxxxxxxxxx> wrote:
>
> Hi
>
> On 3/24/26 22:38, Guan-Yu Lin wrote:
> > -     /*
> > -      * 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);
> > -             return ret;
> > +     spin_lock(&udev->offload_lock);
> > +
> > +     if (udev->offload_pm_locked) {
>
> Could we get rid of 'udev->offload_pm_locked' and 'usb_offload_set_pm_locked()'
> by calling a synchronous pm_runtime_get_sync() or pm_runtime_resume_and_get()?
>
> This way we can ensure udev->offload_usage isn't modified mid runtime suspend or
> resume as resume is guaranteed to have finished and suspend won't be called,
> at least not for the runtime case.
>

We previously considered manipulating runtime PM directly, but the
USB-specific runtime API (usb_autoresume_device()) necessitates
holding the device lock. Since holding this lock causes a deadlock
issue as reported, we implemented a check-and-return-error approach to
remain deadlock-free while staying consistent with USB locking
requirements.

The `offload_pm_locked` flag ensures that `offload_usage` remains
static throughout the system suspend/resume cycle. Because
modifications to usage counts directly impact the USB power management
flow, all updates must occur only when the device is fully active.

> > -     /*
> > -      * 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);
> > -             return ret;
> > +     spin_lock(&udev->offload_lock);
> > +
> > +     if (udev->offload_pm_locked) {
> > +             ret = -EAGAIN;
> > +             goto err;
>
>
> Ending up here is about unlucky timing, i.e. usb_offload_put() is called while
> device is pretending to suspend/resume. Result here is that udev->offload_usage is
> not decremented, and usb device won't properly suspend anymore even if device is
> no longer offloaded.
>

The current implementation requires the class driver to either handle
the error code properly or ensure the system is fully resumed from
system suspend before calling `usb_offload_get()/put()`.

If we use the idea from pm_runtime_put_sync(): "decremented usage
count in all cases, even if it returns an error code", this will lead
to usage_count changes during system suspend. The system might use
suspend logic with active USB offloaded devices system to suspend, but
uses logic without active offloaded devices since the usage count has
been changed during the suspend period. To avoid this, a
check-and-return-error approach is adapted here.

> >   bool usb_offload_check(struct usb_device *udev) __must_hold(&udev->dev->mutex)
> >   {
> >       struct usb_device *child;
> > -     bool active;
> > +     bool active = false;
> >       int port1;
> >
> > +     spin_lock(&udev->offload_lock);
> > +     if (udev->offload_usage)
> > +             active = true;
> > +     spin_unlock(&udev->offload_lock);
> > +> +  if (active)
> > +             return true;
>
> Not sure what the purpose of the spinlock is above
>

The spinlock was originally intended to prevent a race condition
between `usb_offload_get()/put()` and `usb_offload_check()`. However,
because `usb_offload_check()` is only invoked while
`offload_pm_locked` is set, the race is already resolved. I’m removing
the spinlock and updating the function description accordingly. Thanks
for identifying this.

> > +void usb_offload_set_pm_locked(struct usb_device *udev, bool locked)
> > +{
> > +     spin_lock(&udev->offload_lock);
> > +     udev->offload_pm_locked = locked;
> > +     spin_unlock(&udev->offload_lock);
> >
>
> spinlock usage unclear here as well
>
> Thanks
> Mathias
>

The spinlock ensures that `offload_usage` is modified only while
`offload_pm_locked` remains false. Without this synchronization,
`offload_pm_locked` could change during the execution of
`usb_offload_get()/put()`, leading to a race condition.

Regards,
Guan-Yu