Re: [PATCH v5 4/4] ALSA: usb-audio: qcom: fix xfer ring IOMMU unmap on 16K+ page kernels

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] ALSA: usb-audio: qcom: endpoint and stream leak on prepare_qmi_response() err
- [Low] ALSA: usb-audio: qcom: missing card_num kernel-doc in enable_audio_stream()
--

commit 7b2fd8f864f08004dfcddc27ca3dd72dfea2f7d5
Author: Wesley Cheng <wesley.cheng@oss.qualcomm.com>

ALSA: usb-audio: qcom: fix xfer ring IOMMU unmap on 16K+ page kernels

This commit updates uaudio_endpoint_setup() and uaudio_event_ring_setup() to
add the scatterlist intra-page offset to the QMI-reported IOVA and use the
scatterlist entry length as the ring size on kernels with page sizes larger
than 4K. It also returns the raw page-aligned ring IOVA from
uaudio_endpoint_setup() so prepare_qmi_response() can cache and unmap the
exact IOVA that was mapped.

> diff --git a/sound/usb/qcom/qc_audio_offload.c b/sound/usb/qcom/qc_audio_offload.c
> index 48ee551160044..dd904d72baddd 100644
> --- a/sound/usb/qcom/qc_audio_offload.c
> +++ b/sound/usb/qcom/qc_audio_offload.c
> @@ -1178,6 +1178,7 @@ static int uaudio_transfer_buffer_setup(struct snd_usb_substream *subs,
>   * @card_num: uadev index
>   * @mem_info: QMI response info
>   * @ep_desc: QMI ep desc response field
> + * @ring_iova: returns the iova the transfer ring page is mapped at

[Severity: Low]
This isn't a bug, but while updating the kernel-doc for
uaudio_endpoint_setup() here, earlier in the series commit fa22ee4117b1
("ALSA: usb-audio: qcom: tag sideband endpoints before ring allocation")
added the card_num parameter to enable_audio_stream() in
sound/usb/qcom/qc_audio_offload.c without documenting @card_num in its
kernel-doc comment:

sound/usb/qcom/qc_audio_offload.c:enable_audio_stream() {
/**
 * enable_audio_stream() - enable usb snd endpoints
 * @subs: usb substream
 * @pcm_format: pcm format requested
 * @channels: number of channels
 * @cur_rate: sample rate
 * @datainterval: interval
...
static int enable_audio_stream(struct snd_usb_substream *subs,
			       snd_pcm_format_t pcm_format,
			       unsigned int channels, unsigned int cur_rate,
			       int datainterval, unsigned int card_num)

Should @card_num be added to the enable_audio_stream() kernel-doc block to
avoid a W=1 kernel-doc warning?

[ ... ]

> @@ -1470,7 +1486,7 @@ static int prepare_qmi_response(struct snd_usb_substream *subs,
>  	resp->std_as_opr_intf_desc_valid = 1;
>  	ret = uaudio_endpoint_setup(subs, subs->data_endpoint, card_num,
>  				    &resp->xhci_mem_info.tr_data,
> -				    &resp->std_as_data_ep_desc);
> +				    &resp->std_as_data_ep_desc, &data_ring_iova);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -1479,7 +1495,7 @@ static int prepare_qmi_response(struct snd_usb_substream *subs,
>  	if (subs->sync_endpoint) {
>  		ret = uaudio_endpoint_setup(subs, subs->sync_endpoint, card_num,
>  					    &resp->xhci_mem_info.tr_sync,
> -					    &resp->std_as_sync_ep_desc);
> +					    &resp->std_as_sync_ep_desc, &sync_ring_iova);
>  		if (ret < 0)
>  			goto drop_data_ep;
>  

[ ... ]

> @@ -1578,15 +1592,14 @@ static int prepare_qmi_response(struct snd_usb_substream *subs,
>  	usb_offload_put(subs->dev);
>  drop_sync_ep:
>  	if (subs->sync_endpoint) {
> -		uaudio_iommu_unmap(MEM_XFER_RING,
> -				   IOVA_MASK(resp->xhci_mem_info.tr_sync.iova),
> -				   PAGE_SIZE, PAGE_SIZE);
> +		uaudio_iommu_unmap(MEM_XFER_RING, sync_ring_iova, PAGE_SIZE,
> +				   PAGE_SIZE);
>  		xhci_sideband_remove_endpoint(uadev[card_num].sb,
>  			usb_pipe_endpoint(subs->dev, subs->sync_endpoint->pipe));
>  	}
>  drop_data_ep:
> -	uaudio_iommu_unmap(MEM_XFER_RING, IOVA_MASK(resp->xhci_mem_info.tr_data.iova),
> -			   PAGE_SIZE, PAGE_SIZE);
> +	uaudio_iommu_unmap(MEM_XFER_RING, data_ring_iova, PAGE_SIZE,
> +			   PAGE_SIZE);
>  	xhci_sideband_remove_endpoint(uadev[card_num].sb,
>  			usb_pipe_endpoint(subs->dev, subs->data_endpoint->pipe));

[Severity: High]
As reported by Sashiko on v4 [1], does a failure in prepare_qmi_response()
after enable_audio_stream() succeeds leak the xHCI sideband endpoints, the
prepared USB audio endpoints (and their ep->ring allocations), and the USB
device runtime PM reference?

In sound/usb/qcom/qc_audio_offload.c, handle_uaudio_stream_req() calls
enable_audio_stream() before prepare_qmi_response(). Following commit
fa22ee4117b1 ("ALSA: usb-audio: qcom: tag sideband endpoints before ring
allocation"), enable_audio_stream() calls snd_usb_autoresume(),
snd_usb_hw_params(), xhci_sideband_add_endpoint() for both data_ep and
sync_ep (when subs->sync_endpoint is present), and snd_usb_endpoint_prepare()
for both endpoints.

Inside prepare_qmi_response(), if an early check fails before
uaudio_endpoint_setup() (when !uadev[card_num].ctrl_intf,
uaudio_populate_uac_desc() fails, or !data), prepare_qmi_response() returns
directly without calling xhci_sideband_remove_endpoint() for either
subs->data_endpoint or subs->sync_endpoint:

sound/usb/qcom/qc_audio_offload.c:prepare_qmi_response() {
	if (!uadev[card_num].ctrl_intf) {
		dev_err(&subs->dev->dev, "audio ctrl intf info not cached\n");
		return -ENODEV;
	}

	ret = uaudio_populate_uac_desc(subs, resp);
	if (ret < 0)
		return ret;
	...
	data = snd_soc_usb_find_priv_data(uaudio_qdev->auxdev->dev.parent);
	if (!data) {
		dev_err(&subs->dev->dev, "No private data found\n");
		return -ENODEV;
	}
}

Likewise, if uaudio_endpoint_setup() for subs->data_endpoint fails,
prepare_qmi_response() returns ret directly without removing
subs->sync_endpoint from uadev[card_num].sb, and if usb_pipe_endpoint()
returns NULL in uaudio_endpoint_setup(), jumping to exit skips
xhci_sideband_remove_endpoint() for that endpoint as well.

In addition, in handle_uaudio_stream_req(), when prepare_qmi_response()
returns a negative error after enable_audio_stream() succeeded, the error
path only resets subs->opened = 0 without calling disable_audio_stream(subs)
to run snd_usb_hw_free(subs) and snd_usb_autosuspend(chip):

sound/usb/qcom/qc_audio_offload.c:handle_uaudio_stream_req() {
	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, pcm_card_num);

		if (!ret)
			ret = prepare_qmi_response(subs, req_msg, &resp,
						   info_idx);
		if (ret < 0) {
			guard(mutex)(&chip->mutex);
			subs->opened = 0;
		}
	}
}

Because xhci_sideband_add_endpoint() rejects endpoints that already have
ep->sideband or ep->ring set, won't any subsequent offload attempt on the
device fail with -EBUSY until the USB device is disconnected?

[1] https://lore.kernel.org/r/20260909-16k_offload_v1_b4-v4-5-f24a86617597@oss.qualcomm.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103-16k_offload_v1_b4-v5-0-67184126e91c@oss.qualcomm.com?part=4