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-googOn 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
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
> 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
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
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
© 2016 - 2026 Red Hat, Inc.