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

[Wesley Cheng <wesley.cheng@oss.qualcomm.com>]

On 3/18/2026 4:21 PM, Guan-Yu Lin wrote:
> On Tue, Mar 17, 2026 at 4:17 PM Wesley Cheng
> <wesley.cheng@oss.qualcomm.com> 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?
> 

I think I prefer the active check over RPM versus forcing a device resume.

>>>        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?
> 

I think that is what I was getting to.  Now, instead of having to rely on 
the udev lock, you can protect the counter using a local mutex, which 
should avoid the deadlock mentioned by Oppo.  You can avoid also having the 
class driver worry about locking requirements, etc..

Thanks
Wesley Cheng