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

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

[Guan-Yu Lin]

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@vger.kernel.org
Fixes: ef82a4803aab ("xhci: sideband: add api to trace sideband usage")
Signed-off-by: Guan-Yu Lin <guanyulin@google.com>
Tested-by: Hailong Liu <hailong.liu@oppo.com>
---
 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.
  *
  * Return: 0 on success. A negative error code otherwise.
  */
@@ -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;
-	}
 
 	if (udev->state == USB_STATE_SUSPENDED ||
-		   udev->offload_at_suspend) {
-		usb_unlock_device(udev);
+	    udev->offload_at_suspend)
 		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)
 {
 	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;
-	}
 
 	/*
 	 * 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;
-	}
 
 	/* Drop the count when it wasn't 0, ignore the operation otherwise. */
 	if (udev->offload_usage)
 		udev->offload_usage--;
 	usb_autosuspend_device(udev);
-	usb_unlock_device(udev);
 
 	return ret;
 }
diff --git a/drivers/usb/host/xhci-sideband.c b/drivers/usb/host/xhci-sideband.c
index 2bd77255032b..6fc0ad658d66 100644
--- a/drivers/usb/host/xhci-sideband.c
+++ b/drivers/usb/host/xhci-sideband.c
@@ -93,8 +93,6 @@ __xhci_sideband_remove_endpoint(struct xhci_sideband *sb, struct xhci_virt_ep *e
 static void
 __xhci_sideband_remove_interrupter(struct xhci_sideband *sb)
 {
-	struct usb_device *udev;
-
 	lockdep_assert_held(&sb->mutex);
 
 	if (!sb->ir)
@@ -102,10 +100,6 @@ __xhci_sideband_remove_interrupter(struct xhci_sideband *sb)
 
 	xhci_remove_secondary_interrupter(xhci_to_hcd(sb->xhci), sb->ir);
 	sb->ir = NULL;
-	udev = sb->vdev->udev;
-
-	if (udev->state != USB_STATE_NOTATTACHED)
-		usb_offload_put(udev);
 }
 
 /* sideband api functions */
@@ -328,9 +322,6 @@ int
 xhci_sideband_create_interrupter(struct xhci_sideband *sb, int num_seg,
 				 bool ip_autoclear, u32 imod_interval, int intr_num)
 {
-	int ret = 0;
-	struct usb_device *udev;
-
 	if (!sb || !sb->xhci)
 		return -ENODEV;
 
@@ -348,12 +339,9 @@ xhci_sideband_create_interrupter(struct xhci_sideband *sb, int num_seg,
 	if (!sb->ir)
 		return -ENOMEM;
 
-	udev = sb->vdev->udev;
-	ret = usb_offload_get(udev);
-
 	sb->ir->ip_autoclear = ip_autoclear;
 
-	return ret;
+	return 0;
 }
 EXPORT_SYMBOL_GPL(xhci_sideband_create_interrupter);
 
-- 
2.53.0.473.g4a7958ca14-goog

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

[Greg KH]

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@vger.kernel.org
> Fixes: ef82a4803aab ("xhci: sideband: add api to trace sideband usage")
> Signed-off-by: Guan-Yu Lin <guanyulin@google.com>
> Tested-by: Hailong Liu <hailong.liu@oppo.com>
> ---
>  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?

>  {
>  	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...

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 :(



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

>  		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

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@linuxfoundation.org> 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@vger.kernel.org
> > Fixes: ef82a4803aab ("xhci: sideband: add api to trace sideband usage")
> > Signed-off-by: Guan-Yu Lin <guanyulin@google.com>
> > Tested-by: Hailong Liu <hailong.liu@oppo.com>
> > ---
> >  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

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

[Wesley Cheng]

On 3/8/2026 7:22 PM, 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@vger.kernel.org
> Fixes: ef82a4803aab ("xhci: sideband: add api to trace sideband usage")
> Signed-off-by: Guan-Yu Lin <guanyulin@google.com>
> Tested-by: Hailong Liu <hailong.liu@oppo.com>
> ---
>   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.
>    *
>    * Return: 0 on success. A negative error code otherwise.
>    */
> @@ -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;
> -	}
>   
>   	if (udev->state == USB_STATE_SUSPENDED ||
> -		   udev->offload_at_suspend) {
> -		usb_unlock_device(udev);
> +	    udev->offload_at_suspend)
>   		return -EBUSY;
> -	}
>   

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.

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

>   	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

>   	/*
>   	 * 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;
> -	}
>   
>   	/* Drop the count when it wasn't 0, ignore the operation otherwise. */
>   	if (udev->offload_usage)
>   		udev->offload_usage--;
>   	usb_autosuspend_device(udev);
> -	usb_unlock_device(udev);
>   
>   	return ret;
>   }
> diff --git a/drivers/usb/host/xhci-sideband.c b/drivers/usb/host/xhci-sideband.c
> index 2bd77255032b..6fc0ad658d66 100644
> --- a/drivers/usb/host/xhci-sideband.c
> +++ b/drivers/usb/host/xhci-sideband.c
> @@ -93,8 +93,6 @@ __xhci_sideband_remove_endpoint(struct xhci_sideband *sb, struct xhci_virt_ep *e
>   static void
>   __xhci_sideband_remove_interrupter(struct xhci_sideband *sb)
>   {
> -	struct usb_device *udev;
> -
>   	lockdep_assert_held(&sb->mutex);
>   
>   	if (!sb->ir)
> @@ -102,10 +100,6 @@ __xhci_sideband_remove_interrupter(struct xhci_sideband *sb)
>   
>   	xhci_remove_secondary_interrupter(xhci_to_hcd(sb->xhci), sb->ir);
>   	sb->ir = NULL;
> -	udev = sb->vdev->udev;
> -
> -	if (udev->state != USB_STATE_NOTATTACHED)
> -		usb_offload_put(udev);
>   }
>   
>   /* sideband api functions */
> @@ -328,9 +322,6 @@ int
>   xhci_sideband_create_interrupter(struct xhci_sideband *sb, int num_seg,
>   				 bool ip_autoclear, u32 imod_interval, int intr_num)
>   {
> -	int ret = 0;
> -	struct usb_device *udev;
> -
>   	if (!sb || !sb->xhci)
>   		return -ENODEV;
>   
> @@ -348,12 +339,9 @@ xhci_sideband_create_interrupter(struct xhci_sideband *sb, int num_seg,
>   	if (!sb->ir)
>   		return -ENOMEM;
>   
> -	udev = sb->vdev->udev;
> -	ret = usb_offload_get(udev);
> -
>   	sb->ir->ip_autoclear = ip_autoclear;
>   
> -	return ret;
> +	return 0;
>   }
>   EXPORT_SYMBOL_GPL(xhci_sideband_create_interrupter);
>   

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

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

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

[Wesley Cheng]

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

[PATCH v2 2/2] ALSA: usb: qcom: manage offload device usage

[Guan-Yu Lin]

The Qualcomm USB audio offload driver currently does not report its offload
activity to the USB core. This prevents the USB core from properly tracking
active offload sessions, which could allow the device to auto-suspend while
audio offloading is in progress.

Integrate usb_offload_get() and usb_offload_put() calls into the offload
stream setup and teardown paths. Specifically, call usb_offload_get() when
initializing the event ring and usb_offload_put() when freeing it.

Since the updated usb_offload_get() and usb_offload_put() APIs require the
caller to hold the USB device lock, add the necessary device locking in
handle_uaudio_stream_req() and qmi_stop_session() to satisfy this
requirement.

Cc: stable@vger.kernel.org
Fixes: ef82a4803aab ("xhci: sideband: add api to trace sideband usage")
Signed-off-by: Guan-Yu Lin <guanyulin@google.com>
---
 sound/usb/qcom/qc_audio_offload.c | 102 ++++++++++++++++++------------
 1 file changed, 60 insertions(+), 42 deletions(-)

diff --git a/sound/usb/qcom/qc_audio_offload.c b/sound/usb/qcom/qc_audio_offload.c
index cfb30a195364..1da243662327 100644
--- a/sound/usb/qcom/qc_audio_offload.c
+++ b/sound/usb/qcom/qc_audio_offload.c
@@ -699,6 +699,7 @@ static void uaudio_event_ring_cleanup_free(struct uaudio_dev *dev)
 		uaudio_iommu_unmap(MEM_EVENT_RING, IOVA_BASE, PAGE_SIZE,
 				   PAGE_SIZE);
 		xhci_sideband_remove_interrupter(uadev[dev->chip->card->number].sb);
+		usb_offload_put(dev->udev);
 	}
 }
 
@@ -750,6 +751,7 @@ static void qmi_stop_session(void)
 	struct snd_usb_substream *subs;
 	struct usb_host_endpoint *ep;
 	struct snd_usb_audio *chip;
+	struct usb_device *udev;
 	struct intf_info *info;
 	int pcm_card_num;
 	int if_idx;
@@ -791,8 +793,13 @@ static void qmi_stop_session(void)
 			disable_audio_stream(subs);
 		}
 		atomic_set(&uadev[idx].in_use, 0);
-		guard(mutex)(&chip->mutex);
-		uaudio_dev_cleanup(&uadev[idx]);
+
+		udev = uadev[idx].udev;
+		if (udev) {
+			guard(device)(&udev->dev);
+			guard(mutex)(&chip->mutex);
+			uaudio_dev_cleanup(&uadev[idx]);
+		}
 	}
 }
 
@@ -1183,11 +1190,15 @@ static int uaudio_event_ring_setup(struct snd_usb_substream *subs,
 	er_pa = 0;
 
 	/* event ring */
+	ret = usb_offload_get(subs->dev);
+	if (ret < 0)
+		goto exit;
+
 	ret = xhci_sideband_create_interrupter(uadev[card_num].sb, 1, false,
 					       0, uaudio_qdev->data->intr_num);
 	if (ret < 0) {
 		dev_err(&subs->dev->dev, "failed to fetch interrupter\n");
-		goto exit;
+		goto put_offload;
 	}
 
 	sgt = xhci_sideband_get_event_buffer(uadev[card_num].sb);
@@ -1219,6 +1230,8 @@ static int uaudio_event_ring_setup(struct snd_usb_substream *subs,
 	mem_info->dma = 0;
 remove_interrupter:
 	xhci_sideband_remove_interrupter(uadev[card_num].sb);
+put_offload:
+	usb_offload_put(subs->dev);
 exit:
 	return ret;
 }
@@ -1483,6 +1496,7 @@ static int prepare_qmi_response(struct snd_usb_substream *subs,
 	uaudio_iommu_unmap(MEM_EVENT_RING, IOVA_BASE, PAGE_SIZE, PAGE_SIZE);
 free_sec_ring:
 	xhci_sideband_remove_interrupter(uadev[card_num].sb);
+	usb_offload_put(subs->dev);
 drop_sync_ep:
 	if (subs->sync_endpoint) {
 		uaudio_iommu_unmap(MEM_XFER_RING,
@@ -1528,6 +1542,7 @@ static void handle_uaudio_stream_req(struct qmi_handle *handle,
 	u8 pcm_card_num;
 	u8 pcm_dev_num;
 	u8 direction;
+	struct usb_device *udev = NULL;
 	int ret = 0;
 
 	if (!svc->client_connected) {
@@ -1597,50 +1612,53 @@ static void handle_uaudio_stream_req(struct qmi_handle *handle,
 
 	uadev[pcm_card_num].ctrl_intf = chip->ctrl_intf;
 
-	if (req_msg->enable) {
-		ret = enable_audio_stream(subs,
-					  map_pcm_format(req_msg->audio_format),
-					  req_msg->number_of_ch, req_msg->bit_rate,
-					  datainterval);
-
-		if (!ret)
-			ret = prepare_qmi_response(subs, req_msg, &resp,
-						   info_idx);
-		if (ret < 0) {
-			guard(mutex)(&chip->mutex);
-			subs->opened = 0;
-		}
-	} else {
-		info = &uadev[pcm_card_num].info[info_idx];
-		if (info->data_ep_pipe) {
-			ep = usb_pipe_endpoint(uadev[pcm_card_num].udev,
-					       info->data_ep_pipe);
-			if (ep) {
-				xhci_sideband_stop_endpoint(uadev[pcm_card_num].sb,
-							    ep);
-				xhci_sideband_remove_endpoint(uadev[pcm_card_num].sb,
-							      ep);
+	udev = subs->dev;
+	scoped_guard(device, &udev->dev) {
+		if (req_msg->enable) {
+			ret = enable_audio_stream(subs,
+						map_pcm_format(req_msg->audio_format),
+						req_msg->number_of_ch, req_msg->bit_rate,
+						datainterval);
+
+			if (!ret)
+				ret = prepare_qmi_response(subs, req_msg, &resp,
+							info_idx);
+			if (ret < 0) {
+				guard(mutex)(&chip->mutex);
+				subs->opened = 0;
+			}
+		} else {
+			info = &uadev[pcm_card_num].info[info_idx];
+			if (info->data_ep_pipe) {
+				ep = usb_pipe_endpoint(uadev[pcm_card_num].udev,
+							info->data_ep_pipe);
+				if (ep) {
+					xhci_sideband_stop_endpoint(uadev[pcm_card_num].sb,
+									ep);
+					xhci_sideband_remove_endpoint(uadev[pcm_card_num].sb,
+									ep);
+				}
+
+				info->data_ep_pipe = 0;
 			}
 
-			info->data_ep_pipe = 0;
-		}
-
-		if (info->sync_ep_pipe) {
-			ep = usb_pipe_endpoint(uadev[pcm_card_num].udev,
-					       info->sync_ep_pipe);
-			if (ep) {
-				xhci_sideband_stop_endpoint(uadev[pcm_card_num].sb,
-							    ep);
-				xhci_sideband_remove_endpoint(uadev[pcm_card_num].sb,
-							      ep);
+			if (info->sync_ep_pipe) {
+				ep = usb_pipe_endpoint(uadev[pcm_card_num].udev,
+							info->sync_ep_pipe);
+				if (ep) {
+					xhci_sideband_stop_endpoint(uadev[pcm_card_num].sb,
+									ep);
+					xhci_sideband_remove_endpoint(uadev[pcm_card_num].sb,
+									ep);
+				}
+
+				info->sync_ep_pipe = 0;
 			}
 
-			info->sync_ep_pipe = 0;
+			disable_audio_stream(subs);
+			guard(mutex)(&chip->mutex);
+			subs->opened = 0;
 		}
-
-		disable_audio_stream(subs);
-		guard(mutex)(&chip->mutex);
-		subs->opened = 0;
 	}
 
 response:
-- 
2.53.0.473.g4a7958ca14-goog

Re: [PATCH v2 2/2] ALSA: usb: qcom: manage offload device usage

[Greg KH]

On Mon, Mar 09, 2026 at 02:22:05AM +0000, Guan-Yu Lin wrote:
> The Qualcomm USB audio offload driver currently does not report its offload
> activity to the USB core. This prevents the USB core from properly tracking
> active offload sessions, which could allow the device to auto-suspend while
> audio offloading is in progress.
> 
> Integrate usb_offload_get() and usb_offload_put() calls into the offload
> stream setup and teardown paths. Specifically, call usb_offload_get() when
> initializing the event ring and usb_offload_put() when freeing it.
> 
> Since the updated usb_offload_get() and usb_offload_put() APIs require the
> caller to hold the USB device lock, add the necessary device locking in
> handle_uaudio_stream_req() and qmi_stop_session() to satisfy this
> requirement.
> 
> Cc: stable@vger.kernel.org
> Fixes: ef82a4803aab ("xhci: sideband: add api to trace sideband usage")
> Signed-off-by: Guan-Yu Lin <guanyulin@google.com>
> ---
>  sound/usb/qcom/qc_audio_offload.c | 102 ++++++++++++++++++------------
>  1 file changed, 60 insertions(+), 42 deletions(-)
> 
> diff --git a/sound/usb/qcom/qc_audio_offload.c b/sound/usb/qcom/qc_audio_offload.c
> index cfb30a195364..1da243662327 100644
> --- a/sound/usb/qcom/qc_audio_offload.c
> +++ b/sound/usb/qcom/qc_audio_offload.c
> @@ -699,6 +699,7 @@ static void uaudio_event_ring_cleanup_free(struct uaudio_dev *dev)
>  		uaudio_iommu_unmap(MEM_EVENT_RING, IOVA_BASE, PAGE_SIZE,
>  				   PAGE_SIZE);
>  		xhci_sideband_remove_interrupter(uadev[dev->chip->card->number].sb);
> +		usb_offload_put(dev->udev);
>  	}
>  }
>  
> @@ -750,6 +751,7 @@ static void qmi_stop_session(void)
>  	struct snd_usb_substream *subs;
>  	struct usb_host_endpoint *ep;
>  	struct snd_usb_audio *chip;
> +	struct usb_device *udev;
>  	struct intf_info *info;
>  	int pcm_card_num;
>  	int if_idx;
> @@ -791,8 +793,13 @@ static void qmi_stop_session(void)
>  			disable_audio_stream(subs);
>  		}
>  		atomic_set(&uadev[idx].in_use, 0);
> -		guard(mutex)(&chip->mutex);
> -		uaudio_dev_cleanup(&uadev[idx]);
> +
> +		udev = uadev[idx].udev;
> +		if (udev) {
> +			guard(device)(&udev->dev);
> +			guard(mutex)(&chip->mutex);
> +			uaudio_dev_cleanup(&uadev[idx]);

Two locks?  For one structure?  Is this caller verifying that those
locks are held?

> +		}
>  	}
>  }
>  
> @@ -1183,11 +1190,15 @@ static int uaudio_event_ring_setup(struct snd_usb_substream *subs,
>  	er_pa = 0;
>  
>  	/* event ring */
> +	ret = usb_offload_get(subs->dev);
> +	if (ret < 0)
> +		goto exit;

Where is the lock being held here?

This pushing the lock for USB calls "higher" up the call chain is rough,
and almost impossible to audit, given changes like this:

> @@ -1597,50 +1612,53 @@ static void handle_uaudio_stream_req(struct qmi_handle *handle,
>  
>  	uadev[pcm_card_num].ctrl_intf = chip->ctrl_intf;
>  
> -	if (req_msg->enable) {
> -		ret = enable_audio_stream(subs,
> -					  map_pcm_format(req_msg->audio_format),
> -					  req_msg->number_of_ch, req_msg->bit_rate,
> -					  datainterval);
> -
> -		if (!ret)
> -			ret = prepare_qmi_response(subs, req_msg, &resp,
> -						   info_idx);
> -		if (ret < 0) {
> -			guard(mutex)(&chip->mutex);
> -			subs->opened = 0;
> -		}
> -	} else {
> -		info = &uadev[pcm_card_num].info[info_idx];
> -		if (info->data_ep_pipe) {
> -			ep = usb_pipe_endpoint(uadev[pcm_card_num].udev,
> -					       info->data_ep_pipe);
> -			if (ep) {
> -				xhci_sideband_stop_endpoint(uadev[pcm_card_num].sb,
> -							    ep);
> -				xhci_sideband_remove_endpoint(uadev[pcm_card_num].sb,
> -							      ep);
> +	udev = subs->dev;
> +	scoped_guard(device, &udev->dev) {
> +		if (req_msg->enable) {
> +			ret = enable_audio_stream(subs,
> +						map_pcm_format(req_msg->audio_format),
> +						req_msg->number_of_ch, req_msg->bit_rate,
> +						datainterval);
> +
> +			if (!ret)
> +				ret = prepare_qmi_response(subs, req_msg, &resp,
> +							info_idx);
> +			if (ret < 0) {
> +				guard(mutex)(&chip->mutex);
> +				subs->opened = 0;
> +			}
> +		} else {
> +			info = &uadev[pcm_card_num].info[info_idx];
> +			if (info->data_ep_pipe) {
> +				ep = usb_pipe_endpoint(uadev[pcm_card_num].udev,
> +							info->data_ep_pipe);
> +				if (ep) {
> +					xhci_sideband_stop_endpoint(uadev[pcm_card_num].sb,
> +									ep);
> +					xhci_sideband_remove_endpoint(uadev[pcm_card_num].sb,
> +									ep);
> +				}
> +
> +				info->data_ep_pipe = 0;
>  			}
>  
> -			info->data_ep_pipe = 0;
> -		}
> -
> -		if (info->sync_ep_pipe) {
> -			ep = usb_pipe_endpoint(uadev[pcm_card_num].udev,
> -					       info->sync_ep_pipe);
> -			if (ep) {
> -				xhci_sideband_stop_endpoint(uadev[pcm_card_num].sb,
> -							    ep);
> -				xhci_sideband_remove_endpoint(uadev[pcm_card_num].sb,
> -							      ep);
> +			if (info->sync_ep_pipe) {
> +				ep = usb_pipe_endpoint(uadev[pcm_card_num].udev,
> +							info->sync_ep_pipe);
> +				if (ep) {
> +					xhci_sideband_stop_endpoint(uadev[pcm_card_num].sb,
> +									ep);
> +					xhci_sideband_remove_endpoint(uadev[pcm_card_num].sb,
> +									ep);
> +				}
> +
> +				info->sync_ep_pipe = 0;
>  			}
>  
> -			info->sync_ep_pipe = 0;
> +			disable_audio_stream(subs);
> +			guard(mutex)(&chip->mutex);
> +			subs->opened = 0;
>  		}
> -
> -		disable_audio_stream(subs);
> -		guard(mutex)(&chip->mutex);
> -		subs->opened = 0;

You have multiple levels of locks here, which is always a sign that
something has gone really wrong.  This looks even more fragile and easy
to get wrong than before.  Are you _SURE_ this is the only way to solve
this?  The whole usb_offload_get() api seems more wrong to me than
before (and I didn't like it even then...)

In other words, this patch set feels rough, and adds more complexity
overall, requiring a reviewer to "know" where locks are held and not
held while before they did not.  That's a lot to push onto us for
something that feels like is due to a broken hardware design?

thanks,

greg k-h

Re: [PATCH v2 2/2] ALSA: usb: qcom: manage offload device usage

[Guan-Yu Lin]

On Wed, Mar 11, 2026 at 5:31 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Mon, Mar 09, 2026 at 02:22:05AM +0000, Guan-Yu Lin wrote:
> > The Qualcomm USB audio offload driver currently does not report its offload
> > activity to the USB core. This prevents the USB core from properly tracking
> > active offload sessions, which could allow the device to auto-suspend while
> > audio offloading is in progress.
> >
> > Integrate usb_offload_get() and usb_offload_put() calls into the offload
> > stream setup and teardown paths. Specifically, call usb_offload_get() when
> > initializing the event ring and usb_offload_put() when freeing it.
> >
> > Since the updated usb_offload_get() and usb_offload_put() APIs require the
> > caller to hold the USB device lock, add the necessary device locking in
> > handle_uaudio_stream_req() and qmi_stop_session() to satisfy this
> > requirement.
> >
> > Cc: stable@vger.kernel.org
> > Fixes: ef82a4803aab ("xhci: sideband: add api to trace sideband usage")
> > Signed-off-by: Guan-Yu Lin <guanyulin@google.com>
> > ---
> >  sound/usb/qcom/qc_audio_offload.c | 102 ++++++++++++++++++------------
> >  1 file changed, 60 insertions(+), 42 deletions(-)
> >
> > diff --git a/sound/usb/qcom/qc_audio_offload.c b/sound/usb/qcom/qc_audio_offload.c
> > index cfb30a195364..1da243662327 100644
> > --- a/sound/usb/qcom/qc_audio_offload.c
> > +++ b/sound/usb/qcom/qc_audio_offload.c
> > @@ -699,6 +699,7 @@ static void uaudio_event_ring_cleanup_free(struct uaudio_dev *dev)
> >               uaudio_iommu_unmap(MEM_EVENT_RING, IOVA_BASE, PAGE_SIZE,
> >                                  PAGE_SIZE);
> >               xhci_sideband_remove_interrupter(uadev[dev->chip->card->number].sb);
> > +             usb_offload_put(dev->udev);
> >       }
> >  }
> >
> > @@ -750,6 +751,7 @@ static void qmi_stop_session(void)
> >       struct snd_usb_substream *subs;
> >       struct usb_host_endpoint *ep;
> >       struct snd_usb_audio *chip;
> > +     struct usb_device *udev;
> >       struct intf_info *info;
> >       int pcm_card_num;
> >       int if_idx;
> > @@ -791,8 +793,13 @@ static void qmi_stop_session(void)
> >                       disable_audio_stream(subs);
> >               }
> >               atomic_set(&uadev[idx].in_use, 0);
> > -             guard(mutex)(&chip->mutex);
> > -             uaudio_dev_cleanup(&uadev[idx]);
> > +
> > +             udev = uadev[idx].udev;
> > +             if (udev) {
> > +                     guard(device)(&udev->dev);
> > +                     guard(mutex)(&chip->mutex);
> > +                     uaudio_dev_cleanup(&uadev[idx]);
>
> Two locks?  For one structure?  Is this caller verifying that those
> locks are held?
>

The device lock is used for usb_offload_get/put apis below. We want to
maintain a strict lock ordering to aviod ABBA deadlocks.
The caller wasn't verifying the &chip->mutex lock before. Do you want
me to add it as well?

> > +             }
> >       }
> >  }
> >
> > @@ -1183,11 +1190,15 @@ static int uaudio_event_ring_setup(struct snd_usb_substream *subs,
> >       er_pa = 0;
> >
> >       /* event ring */
> > +     ret = usb_offload_get(subs->dev);
> > +     if (ret < 0)
> > +             goto exit;
>
> Where is the lock being held here?
>

It's held in the handle_uaudio_stream_req function above.

> This pushing the lock for USB calls "higher" up the call chain is rough,
> and almost impossible to audit, given changes like this:
>
> > @@ -1597,50 +1612,53 @@ static void handle_uaudio_stream_req(struct qmi_handle *handle,
> >
> >       uadev[pcm_card_num].ctrl_intf = chip->ctrl_intf;
> >
> > -     if (req_msg->enable) {
> > -             ret = enable_audio_stream(subs,
> > -                                       map_pcm_format(req_msg->audio_format),
> > -                                       req_msg->number_of_ch, req_msg->bit_rate,
> > -                                       datainterval);
> > -
> > -             if (!ret)
> > -                     ret = prepare_qmi_response(subs, req_msg, &resp,
> > -                                                info_idx);
> > -             if (ret < 0) {
> > -                     guard(mutex)(&chip->mutex);
> > -                     subs->opened = 0;
> > -             }
> > -     } else {
> > -             info = &uadev[pcm_card_num].info[info_idx];
> > -             if (info->data_ep_pipe) {
> > -                     ep = usb_pipe_endpoint(uadev[pcm_card_num].udev,
> > -                                            info->data_ep_pipe);
> > -                     if (ep) {
> > -                             xhci_sideband_stop_endpoint(uadev[pcm_card_num].sb,
> > -                                                         ep);
> > -                             xhci_sideband_remove_endpoint(uadev[pcm_card_num].sb,
> > -                                                           ep);
> > +     udev = subs->dev;
> > +     scoped_guard(device, &udev->dev) {
> > +             if (req_msg->enable) {
> > +                     ret = enable_audio_stream(subs,
> > +                                             map_pcm_format(req_msg->audio_format),
> > +                                             req_msg->number_of_ch, req_msg->bit_rate,
> > +                                             datainterval);
> > +
> > +                     if (!ret)
> > +                             ret = prepare_qmi_response(subs, req_msg, &resp,
> > +                                                     info_idx);
> > +                     if (ret < 0) {
> > +                             guard(mutex)(&chip->mutex);
> > +                             subs->opened = 0;
> > +                     }
> > +             } else {
> > +                     info = &uadev[pcm_card_num].info[info_idx];
> > +                     if (info->data_ep_pipe) {
> > +                             ep = usb_pipe_endpoint(uadev[pcm_card_num].udev,
> > +                                                     info->data_ep_pipe);
> > +                             if (ep) {
> > +                                     xhci_sideband_stop_endpoint(uadev[pcm_card_num].sb,
> > +                                                                     ep);
> > +                                     xhci_sideband_remove_endpoint(uadev[pcm_card_num].sb,
> > +                                                                     ep);
> > +                             }
> > +
> > +                             info->data_ep_pipe = 0;
> >                       }
> >
> > -                     info->data_ep_pipe = 0;
> > -             }
> > -
> > -             if (info->sync_ep_pipe) {
> > -                     ep = usb_pipe_endpoint(uadev[pcm_card_num].udev,
> > -                                            info->sync_ep_pipe);
> > -                     if (ep) {
> > -                             xhci_sideband_stop_endpoint(uadev[pcm_card_num].sb,
> > -                                                         ep);
> > -                             xhci_sideband_remove_endpoint(uadev[pcm_card_num].sb,
> > -                                                           ep);
> > +                     if (info->sync_ep_pipe) {
> > +                             ep = usb_pipe_endpoint(uadev[pcm_card_num].udev,
> > +                                                     info->sync_ep_pipe);
> > +                             if (ep) {
> > +                                     xhci_sideband_stop_endpoint(uadev[pcm_card_num].sb,
> > +                                                                     ep);
> > +                                     xhci_sideband_remove_endpoint(uadev[pcm_card_num].sb,
> > +                                                                     ep);
> > +                             }
> > +
> > +                             info->sync_ep_pipe = 0;
> >                       }
> >
> > -                     info->sync_ep_pipe = 0;
> > +                     disable_audio_stream(subs);
> > +                     guard(mutex)(&chip->mutex);
> > +                     subs->opened = 0;
> >               }
> > -
> > -             disable_audio_stream(subs);
> > -             guard(mutex)(&chip->mutex);
> > -             subs->opened = 0;
>
> You have multiple levels of locks here, which is always a sign that
> something has gone really wrong.  This looks even more fragile and easy
> to get wrong than before.  Are you _SURE_ this is the only way to solve
> this?  The whole usb_offload_get() api seems more wrong to me than
> before (and I didn't like it even then...)
>
> In other words, this patch set feels rough, and adds more complexity
> overall, requiring a reviewer to "know" where locks are held and not
> held while before they did not.  That's a lot to push onto us for
> something that feels like is due to a broken hardware design?
>
> thanks,
>
> greg k-h

A known deadlock exists in the current design, caused by USB locks
sometimes being held in the higher function (e.g. during the USB
connection change). Due to this nature, I think if we want a design to
cover these two scenarios, certain degree of lock mainpulation is
required. I agree this is not an elegant way to address the issue.
What approach do you think better addresses this problem?

Regards,
Guan-Yu

Re: [PATCH v2 2/2] ALSA: usb: qcom: manage offload device usage

[Guan-Yu Lin]

> On Wed, Mar 11, 2026 at 5:31 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > You have multiple levels of locks here, which is always a sign that
> > something has gone really wrong.  This looks even more fragile and easy
> > to get wrong than before.  Are you _SURE_ this is the only way to solve
> > this?  The whole usb_offload_get() api seems more wrong to me than
> > before (and I didn't like it even then...)
> >
> > In other words, this patch set feels rough, and adds more complexity
> > overall, requiring a reviewer to "know" where locks are held and not
> > held while before they did not.  That's a lot to push onto us for
> > something that feels like is due to a broken hardware design?
> >
> > thanks,
> >
> > greg k-h
>

Hi Greg,

Thank you for the feedback. I understand the concern regarding locking
complexity and the reviewer burden. The usb_offload_get/put functions
track sideband activity that runtime PM cannot reflect. This is
necessary to prevent the USB stack from suspending the device while a
sideband stream is active. Ensuring atomicity requires orchestrating
three asynchronous subsystems: System PM, Runtime PM, and USB Core.

To address the concerns effectively in the next iteration, I would
appreciate clarification on your primary concern:
1. Preference for fine-grained locking:
Using USB device lock ensures atomicity across these subsystems, which
is inherently a device-wide requirement. A fine-grained approach risks
races during concurrent software events, such as a reset occurring
during a runtime suspend transition.
2. Preference for improved transparency:
If the coarse lock is acceptable but the implementation is too opaque,
I will refactor the next version to be more explicit. I plan to
include device_lock_assert() calls, __must_hold() macros, and add a
"Locking Hierarchy" comment block documenting the vendor-mutex and
USB-core lock interactions.

To clarify the "broken hardware" point: this isn't a hardware bug.
These races are triggered by standard software events, such as a reset
occurring while a sideband stream is active. Please let me know which
direction you think is more appropriate for the kernel, and I will
refactor the next version accordingly.

Regards,
Guan-Yu

Re: [PATCH v2 2/2] ALSA: usb: qcom: manage offload device usage

[Greg KH]

On Tue, Mar 17, 2026 at 04:45:00PM -0400, Guan-Yu Lin wrote:
> > On Wed, Mar 11, 2026 at 5:31 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > >
> > > You have multiple levels of locks here, which is always a sign that
> > > something has gone really wrong.  This looks even more fragile and easy
> > > to get wrong than before.  Are you _SURE_ this is the only way to solve
> > > this?  The whole usb_offload_get() api seems more wrong to me than
> > > before (and I didn't like it even then...)
> > >
> > > In other words, this patch set feels rough, and adds more complexity
> > > overall, requiring a reviewer to "know" where locks are held and not
> > > held while before they did not.  That's a lot to push onto us for
> > > something that feels like is due to a broken hardware design?
> > >
> > > thanks,
> > >
> > > greg k-h
> >
> 
> Hi Greg,
> 
> Thank you for the feedback. I understand the concern regarding locking
> complexity and the reviewer burden. The usb_offload_get/put functions
> track sideband activity that runtime PM cannot reflect. This is
> necessary to prevent the USB stack from suspending the device while a
> sideband stream is active. Ensuring atomicity requires orchestrating
> three asynchronous subsystems: System PM, Runtime PM, and USB Core.
> 
> To address the concerns effectively in the next iteration, I would
> appreciate clarification on your primary concern:
> 1. Preference for fine-grained locking:
> Using USB device lock ensures atomicity across these subsystems, which
> is inherently a device-wide requirement. A fine-grained approach risks
> races during concurrent software events, such as a reset occurring
> during a runtime suspend transition.
> 2. Preference for improved transparency:
> If the coarse lock is acceptable but the implementation is too opaque,
> I will refactor the next version to be more explicit. I plan to
> include device_lock_assert() calls, __must_hold() macros, and add a
> "Locking Hierarchy" comment block documenting the vendor-mutex and
> USB-core lock interactions.

At the very least, this is going to have to be required so that we can
catch any future changes and ensure that changes do not create locking
inversions and the like.  I guess we are stuck with this for now, due to
this being "outside" of the normal runtime PM, which still feels wrong
to me as runtime PM _should_ be able to handle this (if not, why is this
case somehow unique from all other hardware types?)

> To clarify the "broken hardware" point: this isn't a hardware bug.

It was described as triggering when a shock happened to the system to
cause the system to reset in places, which is a hardware issue :)

> These races are triggered by standard software events, such as a reset
> occurring while a sideband stream is active. Please let me know which
> direction you think is more appropriate for the kernel, and I will
> refactor the next version accordingly.

And how exactly can a "reset while active" happen as just a normal
software driven event?

thanks,

greg k-h

Re: [PATCH v2 2/2] ALSA: usb: qcom: manage offload device usage

[Guan-Yu Lin]

On Wed, Mar 18, 2026 at 12:59 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Tue, Mar 17, 2026 at 04:45:00PM -0400, Guan-Yu Lin wrote:
> >
> > Hi Greg,
> >
> > Thank you for the feedback. I understand the concern regarding locking
> > complexity and the reviewer burden. The usb_offload_get/put functions
> > track sideband activity that runtime PM cannot reflect. This is
> > necessary to prevent the USB stack from suspending the device while a
> > sideband stream is active. Ensuring atomicity requires orchestrating
> > three asynchronous subsystems: System PM, Runtime PM, and USB Core.
> >
> > To address the concerns effectively in the next iteration, I would
> > appreciate clarification on your primary concern:
> > 1. Preference for fine-grained locking:
> > Using USB device lock ensures atomicity across these subsystems, which
> > is inherently a device-wide requirement. A fine-grained approach risks
> > races during concurrent software events, such as a reset occurring
> > during a runtime suspend transition.
> > 2. Preference for improved transparency:
> > If the coarse lock is acceptable but the implementation is too opaque,
> > I will refactor the next version to be more explicit. I plan to
> > include device_lock_assert() calls, __must_hold() macros, and add a
> > "Locking Hierarchy" comment block documenting the vendor-mutex and
> > USB-core lock interactions.
>
> At the very least, this is going to have to be required so that we can
> catch any future changes and ensure that changes do not create locking
> inversions and the like.  I guess we are stuck with this for now, due to
> this being "outside" of the normal runtime PM, which still feels wrong
> to me as runtime PM _should_ be able to handle this (if not, why is this
> case somehow unique from all other hardware types?)
>

The runtime pm doesn't apply here because when a sideband instance
accesses the controller, the main system could suspend. The runtime pm
only reflects whether the "main system" is using the controller,
whereas a sideband instance might still be using the controller when
the main system has suspended. Runtime pm couldn't reflect the
sideband's status. If runtime pm reflects sidebands activity on the
controller, it will mark the controller as active, which prevents the
entire "main system" from suspending. Does that sound right to you, or
is there anything I can clarify?

> > To clarify the "broken hardware" point: this isn't a hardware bug.
>
> It was described as triggering when a shock happened to the system to
> cause the system to reset in places, which is a hardware issue :)
>
> > These races are triggered by standard software events, such as a reset
> > occurring while a sideband stream is active. Please let me know which
> > direction you think is more appropriate for the kernel, and I will
> > refactor the next version accordingly.
>
> And how exactly can a "reset while active" happen as just a normal
> software driven event?
>
> thanks,
>
> greg k-h

I'm not sure what the OPPO team has encountered. In our experiments,
we saw the following call stack:
[  721.889147][  T228]  qc_usb_audio_offload_disconnect
[  721.889284][  T228]  usb_audio_disconnect+0x7c/0x268
[  721.889302][  T228]  usb_unbind_interface+0xc4/0x2f8
[  721.889313][  T228]  device_release_driver_internal+0x1c4/0x2bc
[  721.889333][  T228]  device_release_driver+0x18/0x28
[  721.889347][  T228]  usb_forced_unbind_intf+0x60/0x80
[  721.889358][  T228]  usb_reset_device+0xbc/0x23c
[  721.889375][  T228]  __usb_queue_reset_device+0x3c/0x64
[  721.889386][  T228]  process_scheduled_works+0x1b8/0x8ec
[  721.889405][  T228]  worker_thread+0x1b0/0x470
[  721.889418][  T228]  kthread+0x11c/0x158
[  721.889429][  T228]  ret_from_fork+0x10/0x20

In addition, if a process interacts with the kernel using
`usbdev_do_ioctl` and then `usbdev_do_ioctl ` calls
`usb_driver_release_interface`, we could also encounter a deadlock
issue.

Regards,
Guan-Yu