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

[Guan-Yu Lin]

On Tue, Mar 17, 2026 at 4:17 PM Wesley Cheng
<wesley.cheng@xxxxxxxxxxxxxxxx> wrote:
>
> On 3/8/2026 7:22 PM, Guan-Yu Lin wrote:
> >
> > @@ -27,31 +28,25 @@ int usb_offload_get(struct usb_device *udev)
> >   {
> >       int ret;
> >
> > -     usb_lock_device(udev);
> > -     if (udev->state == USB_STATE_NOTATTACHED) {
> > -             usb_unlock_device(udev);
> > +     device_lock_assert(&udev->dev);
> > +
> > +     if (udev->state == USB_STATE_NOTATTACHED)
> >               return -ENODEV;
> > -     }

Could be removed. Since the udev is in USB_STATE_NOTATTACHED. I expect
the data structure being cleaned afterwards, so actually counter value
might not be important at this moment.

> >
> >       if (udev->state == USB_STATE_SUSPENDED ||
> > -                udev->offload_at_suspend) {
> > -             usb_unlock_device(udev);
> > +         udev->offload_at_suspend)
> >               return -EBUSY;
> > -     }
> >

This check is still required. Because the suspend/resume process
depends on the counter value, we can't modify the counter value while
the device is suspended. If we do so, we will have an unbalanced
suspend resume operation.

However, we might only need to check for udev->offload_at_suspend (if
we ensure the device is active when we want to incremant the counter):
1. If the offload_usage_count is 0, we won't decrement counts at this moment.
2. If the offload_usage_count is not 0, the offload_at_suspend flag
will be true anyway.

>
> Do we really need to be explicitly checking for the usb device state before
> we touch the offload_usage count?  In the end, its a reference count that
> determines how many consumers are active for a specific interrupter, so my
> question revolves around if we need to have such strict checks.
>

Please find the explanation for each check above.

> >       /*
> >        * 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;
> > -     }
> >
>
> IMO this should be handled already by the class driver, and if not, what is
> the harm?
>

We can only increment the usage count when the device is active. For
counter decrement, the device could be in any state.

My initial design is to resume the device and then modify the usage
count. Another option is to check only whether the USB device is
active via pm_runtime_get_if_active, and leave the device-resuming
effort to the class driver. Do you think this is the better approach?

> >       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)
> >   {
> >       int ret;
> >
> > -     usb_lock_device(udev);
> > -     if (udev->state == USB_STATE_NOTATTACHED) {
> > -             usb_unlock_device(udev);
> > +     device_lock_assert(&udev->dev);
> > +
> > +     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)
> >               return -EBUSY;
> > -     }
> >
>
> During your testing, did you ever run into any unbalanced counter issues
> due to the above early exit conditions?
>
> I guess these are all just questions to see if we can remove the need to
> lock the udev mutex, and move to a local mutex for the offload framework.
> That would address the locking concerns being brought up by Greg, etc...
>
> Thanks
> Wesley Cheng
>

While developing the initial patch set, I did encounter the counter imbalance.

Following the discussion, we could move the device resume effort to
the class driver. This way we only need to check if the device is
active before manipulating the offload usage counter, which doesn't
require a device lock. Is there any concern with this approach?

Regards,
Guan-Yu