Re: [PATCH] ALSA: usb-audio: add quirk for Novation UltraNova (1235:0011)

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] ALSA: usb-audio: missing .ep_idx = 1 in UltraNova capture audioformat
--

commit aa83c7ced6942ef370a66b06613c33f74107f796
Author: Shawn Rainey <rainey.shawn+kaudio@gmail.com>

ALSA: usb-audio: add quirk for Novation UltraNova (1235:0011)

This commit adds a composite USB audio quirk for the Novation UltraNova
(1235:0011) with fixed playback and capture audio formats on interface 0
and a raw MIDI quirk on interface 1, while leaving the vendor control
interfaces unbound.

> diff --git a/sound/usb/quirks-table.h b/sound/usb/quirks-table.h
> index 6343a50fb2732..0b8bea76eca5b 100644
> --- a/sound/usb/quirks-table.h
> +++ b/sound/usb/quirks-table.h
> @@ -2359,6 +2359,69 @@ YAMAHA_DEVICE(0x7010, "UB99"),
>  		}
>  	}
>  },
> +{
> +	/*
> +	 * Novation UltraNova: vendor-specific class descriptors, fixed
> +	 * audio and MIDI layouts.  Only the audio and MIDI interfaces
> +	 * (0 and 1) are grabbed; the vendor control interfaces (2/3)
> +	 * are left unbound for userspace access.
> +	 */
> +	USB_DEVICE_INTERFACE_NUMBER(0x1235, 0x0011, 0),
> +	QUIRK_DRIVER_INFO {
> +		.vendor_name = "Novation",
> +		.product_name = "UltraNova",
> +		QUIRK_DATA_COMPOSITE {
> +			{
> +				QUIRK_DATA_AUDIOFORMAT(0) {
> +					/* playback */
> +					.formats = SNDRV_PCM_FMTBIT_S16_LE,
> +					.channels = 4,
> +					.iface = 0,
> +					.altsetting = 1,
> +					.altset_idx = 1,
> +					.attributes = UAC_EP_CS_ATTR_SAMPLE_RATE,
> +					.endpoint = 0x01,
> +					.ep_attr = USB_ENDPOINT_XFER_ISOC |
> +						   USB_ENDPOINT_SYNC_SYNC,
> +					.maxpacksize = 392,
> +					.rates = SNDRV_PCM_RATE_44100 |
> +						 SNDRV_PCM_RATE_48000,
> +					.rate_min = 44100,
> +					.rate_max = 48000,
> +					.nr_rates = 2,
> +					.rate_table = (unsigned int[]) {
> +						44100, 48000
> +					}
> +				}
> +			},
> +			{
> +				QUIRK_DATA_AUDIOFORMAT(0) {
> +					/* capture */
> +					.formats = SNDRV_PCM_FMTBIT_S24_3LE,
> +					.channels = 2,
> +					.iface = 0,
> +					.altsetting = 1,
> +					.altset_idx = 1,
> +					.attributes = UAC_EP_CS_ATTR_SAMPLE_RATE,
> +					.endpoint = 0x82,
> +					.ep_attr = USB_ENDPOINT_XFER_ISOC |
> +						   USB_ENDPOINT_SYNC_SYNC,

[Severity: Medium]
Should the capture QUIRK_DATA_AUDIOFORMAT(0) entry in usb_audio_ids in
sound/usb/quirks-table.h also set .ep_idx = 1?

Since capture endpoint 0x82 shares interface 0 altsetting 1 with playback
endpoint 0x01, endpoint 0x82 is the second endpoint descriptor on that
altsetting. Leaving .ep_idx unset defaults fp->ep_idx to 0.

When usb_audio_probe() -> snd_usb_create_quirk() ->
create_composite_quirk() -> create_fixed_stream_quirk() runs in
sound/usb/quirks.c:

	alts = &iface->altsetting[fp->altset_idx];
	altsd = get_iface_desc(alts);
	if (altsd->bNumEndpoints <= fp->ep_idx) {
		err = -EINVAL;
		goto error;
	}

having fp->ep_idx == 0 only checks altsd->bNumEndpoints <= 0 and does not
verify that a second endpoint descriptor exists on altsetting 1.

Next, in create_fixed_stream_quirk() -> add_audio_stream_from_fixed_fmt() ->
snd_usb_audioformat_set_sync_ep() in sound/usb/pcm.c:

	if (fmt->ep_idx > 0 || altsd->bNumEndpoints < 2)
		return 0;

	is_playback = !(get_endpoint(alts, 0)->bEndpointAddress & USB_DIR_IN);
	attr = fmt->ep_attr & USB_ENDPOINT_SYNCTYPE;
	if ((is_playback && (attr == USB_ENDPOINT_SYNC_SYNC ||
			     attr == USB_ENDPOINT_SYNC_ADAPTIVE)) ||
	    (!is_playback && attr != USB_ENDPOINT_SYNC_ADAPTIVE))
		return 0;

because fmt->ep_idx is 0 rather than 1, snd_usb_audioformat_set_sync_ep()
does not return early at fmt->ep_idx > 0 and instead checks
get_endpoint(alts, 0) (playback endpoint 0x01), treating the capture
audioformat as is_playback = true. That also appears to be why omitting
USB_ENDPOINT_SYNC_SYNC caused generic sync-EP detection to pair 0x82 as a
sync endpoint for the capture direction as described in the commit message.

Additionally, when opening the capture endpoint via snd_usb_hw_params() ->
snd_usb_endpoint_open() in sound/usb/endpoint.c:

		} else {
			ep->iface = fp->iface;
			ep->altsetting = fp->altsetting;
			ep->ep_idx = fp->ep_idx;
		}

ep->ep_idx for endpoint 0x82 is recorded as 0 instead of 1.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022164340.78664-1-rainey.shawn+kaudio@gmail.com?part=1