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@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx
> > Fixes: ef82a4803aab ("xhci: sideband: add api to trace sideband usage")
> > Signed-off-by: Guan-Yu Lin <guanyulin@xxxxxxxxxx>
> > ---
> >  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