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

[Takashi Iwai <tiwai@suse.de>]

On Wed, 25 Feb 2026 07:45:51 +0100,
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>

Mostly it looks good to me, but a slight concern is the
usb_offload_put() call in the error path of prepare_qmi_response().
IIUC, the bitmap is cleared only at uaudio_event_ring_cleanup_free(),
and you have also the usb_offload_put() call there.  I wonder whether
this might lead to the refcount unbalance.

I'm not sure whether the original driver code handles it well, and
this could be already a bug there calling
xhci_sideband_remove_interrupter(), though.


thanks,

Takashi

> ---
>  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.414.gf7e9f6c205-goog
>