Re: [PATCH v5 3/4] ALSA: usb-audio: qcom: tag sideband endpoints before ring allocation

[sashiko-bot@kernel.org]

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

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

ALSA: usb-audio: qcom: tag sideband endpoints before ring allocation

This commit moves the xhci_sideband_add_endpoint() calls from
uaudio_endpoint_setup() into enable_audio_stream() so that endpoints are
tagged as sideband before snd_usb_endpoint_prepare() triggers transfer ring
allocation. It also introduces uaudio_find_host_endpoint() to look up the
usb_host_endpoint from the altsetting descriptor table before the interface
altsetting is activated.

> diff --git a/sound/usb/qcom/qc_audio_offload.c b/sound/usb/qcom/qc_audio_offload.c
> index e4bfd43a2488b..48ee551160044 100644
> --- a/sound/usb/qcom/qc_audio_offload.c
> +++ b/sound/usb/qcom/qc_audio_offload.c

[ ... ]

> @@ -958,8 +992,9 @@ static void uaudio_dev_release(struct kref *kref)
>  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)
> +			       int datainterval, unsigned int card_num)
>  {

[Severity: Low]
This isn't a bug, but should @card_num be added to the kernel-doc comment
block above enable_audio_stream() in sound/usb/qcom/qc_audio_offload.c to
avoid a W=1 kernel-doc warning?

> +	struct usb_host_endpoint *data_ep = NULL, *sync_ep = NULL;
>  	struct snd_pcm_hw_params params;
>  	struct snd_usb_audio *chip;
>  	struct snd_interval *i;
> @@ -997,15 +1032,45 @@ static int enable_audio_stream(struct snd_usb_substream *subs,
>  			goto detach_ep;
>  		}
>  
> +		data_ep = uaudio_find_host_endpoint(subs, subs->data_endpoint);
> +		if (!data_ep) {
> +			dev_err(&subs->dev->dev, "data ep # %d not found\n",
> +				subs->data_endpoint->ep_num);
> +			ret = -ENODEV;
> +			goto detach_ep;
> +		}
> +
> +		ret = xhci_sideband_add_endpoint(uadev[card_num].sb, data_ep);
> +		if (ret < 0) {
> +			dev_err(&subs->dev->dev,
> +				"failed to add data ep to sideband: %d\n", ret);
> +			goto detach_ep;
> +		}
> +
>  		if (subs->sync_endpoint) {
> +			sync_ep = uaudio_find_host_endpoint(subs, subs->sync_endpoint);
> +			if (!sync_ep) {
> +				dev_err(&subs->dev->dev, "sync ep # %d not found\n",
> +					subs->sync_endpoint->ep_num);
> +				ret = -ENODEV;
> +				goto remove_data_ep;
> +			}
> +
> +			ret = xhci_sideband_add_endpoint(uadev[card_num].sb, sync_ep);
> +			if (ret < 0) {
> +				dev_err(&subs->dev->dev,
> +					"failed to add sync ep to sideband: %d\n", ret);
> +				goto remove_data_ep;
> +			}
> +
>  			ret = snd_usb_endpoint_prepare(chip, subs->sync_endpoint);
>  			if (ret < 0)
> -				goto detach_ep;
> +				goto remove_sync_ep;
>  		}
>  
>  		ret = snd_usb_endpoint_prepare(chip, subs->data_endpoint);
>  		if (ret < 0)
> -			goto detach_ep;
> +			goto remove_sync_ep;

[ ... ]

> @@ -1140,14 +1210,6 @@ uaudio_endpoint_setup(struct snd_usb_substream *subs,
>  
>  	memcpy(ep_desc, &ep->desc, sizeof(ep->desc));
>  
> -	ret = xhci_sideband_add_endpoint(uadev[card_num].sb, ep);
> -	if (ret < 0) {
> -		dev_err(&subs->dev->dev,
> -			"failed to add data ep to sec intr: %d\n", ret);
> -		ret = -ENODEV;
> -		goto exit;
> -	}
> -
>  	sgt = xhci_sideband_get_endpoint_buffer(uadev[card_num].sb, ep);
>  	if (!sgt) {
>  		dev_err(&subs->dev->dev,
> @@ -1637,7 +1699,7 @@ static void handle_uaudio_stream_req(struct qmi_handle *handle,
>  		ret = enable_audio_stream(subs,
>  					  map_pcm_format(req_msg->audio_format),
>  					  req_msg->number_of_ch, req_msg->bit_rate,
> -					  datainterval);
> +					  datainterval, pcm_card_num);
>  
>  		if (!ret)
>  			ret = prepare_qmi_response(subs, req_msg, &resp,

[Severity: High]
As reported by Sashiko on v4 [1], now that enable_audio_stream() calls
xhci_sideband_add_endpoint() for both data_ep and sync_ep before
handle_uaudio_stream_req() calls prepare_qmi_response(), can failure paths
in prepare_qmi_response() and uaudio_endpoint_setup() leak the sideband
endpoint registrations in uadev[card_num].sb?

In sound/usb/qcom/qc_audio_offload.c, prepare_qmi_response() has early error
returns before uaudio_endpoint_setup() that do not call
xhci_sideband_remove_endpoint() for either endpoint, and if the first
uaudio_endpoint_setup() call for subs->data_endpoint fails, it returns
without removing 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;
	}
	...
	ret = uaudio_endpoint_setup(subs, subs->data_endpoint, card_num,
				    &resp->xhci_mem_info.tr_data,
				    &resp->std_as_data_ep_desc);
	if (ret < 0)
		return ret;
}

Additionally, in uaudio_endpoint_setup(), if usb_pipe_endpoint() returns
NULL, it jumps to exit instead of remove_ep, leaving that endpoint registered
in uadev[card_num].sb:

sound/usb/qcom/qc_audio_offload.c:uaudio_endpoint_setup() {
	ep = usb_pipe_endpoint(subs->dev, endpoint->pipe);
	if (!ep) {
		dev_err(uaudio_qdev->data->dev, "data ep # %d context is null\n",
			subs->data_endpoint->ep_num);
		goto exit;
	}
}

Since xhci_sideband_add_endpoint() rejects endpoints when sb->eps[ep_index]
or ep->sideband is already set, wouldn't any of these failures leave the
endpoints registered and cause subsequent stream enable requests to fail with
-EBUSY?

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

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