Re: [PATCH v2] ALSA: usb-audio: Skip broken playback feature unit on XiiSound chips

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] ALSA: usb-audio: missing unit 5 re-pinning in
  snd_usb_mixer_resume_quirk()
- [Medium] ALSA: usb-audio: missing short-read check in
  snd_xiisound_init_fu_pins()
--

commit 599ffb39dcedddf5b2afe433d25ebe88e2031e78
Author: Cristiano Souza <cristianocps@gmail.com>

ALSA: usb-audio: Skip broken playback feature unit on XiiSound chips

This commit hides the broken playback feature unit (unit 5) on Weltrend
XiiSound devices (040b:0897) via usbmix_name_map and pins its volume to
maximum with mute cleared during mixer creation.

> diff --git a/sound/usb/mixer_quirks.c b/sound/usb/mixer_quirks.c
> index 5839719444f8b..ee12bc7d1cacc 100644
> --- a/sound/usb/mixer_quirks.c
> +++ b/sound/usb/mixer_quirks.c
> @@ -4603,6 +4603,59 @@ static int snd_audiofuse_controls_create(struct usb_mixer_interface *mixer)
>  	return err;
>  }
>  
> +/*
> + * Weltrend XiiSound chips (040b:0897, e.g. Redragon H878):
> + * the playback feature unit (id 5) is broken; its volume controls act like
> + * mute switches below the maximum value and the channel mapping is
> + * unreliable, so the unit is hidden from userspace entirely (see the
> + * quirk in mixer_maps.c) and userspace falls back to software volume.
> + * Pin the unit's master and channel volumes to the reported maximum
> + * and unmute them, so the audio path stays fully open.
> + */
> +static int snd_xiisound_init_fu_pins(struct usb_mixer_interface *mixer)
> +{
> +	struct snd_usb_audio *chip = mixer->chip;
> +	unsigned char buf[2];
> +	int ifnum, ch, err;
> +
> +	if (mixer->protocol != UAC_VERSION_1)
> +		return 0;
> +
> +	ifnum = snd_usb_ctrl_intf(chip->ctrl_intf);
> +
> +	/* unit 5 is the playback feature unit on these chips */
> +	for (ch = 0; ch <= 2; ch++) {
> +		err = snd_usb_ctl_msg(chip->dev, usb_rcvctrlpipe(chip->dev, 0),
> +				      UAC_GET_MAX,
> +				      USB_DIR_IN | USB_TYPE_CLASS |
> +				      USB_RECIP_INTERFACE,
> +				      (UAC_FU_VOLUME << 8) | ch,
> +				      (5 << 8) | ifnum, buf, 2);
> +		if (err < 0)
> +			return 0; /* no such unit/channels, nothing to pin */
> +
> +		err = snd_usb_ctl_msg(chip->dev, usb_sndctrlpipe(chip->dev, 0),
> +				      UAC_SET_CUR,
> +				      USB_DIR_OUT | USB_TYPE_CLASS |
> +				      USB_RECIP_INTERFACE,
> +				      (UAC_FU_VOLUME << 8) | ch,
> +				      (5 << 8) | ifnum, buf, 2);

[Severity: Medium]
Can a short read from UAC_GET_MAX cause UAC_SET_CUR to send uninitialized
stack bytes from buf[]?

snd_usb_ctl_msg() returns the transferred byte count from usb_control_msg().
If UAC_GET_MAX returns 0 or 1 bytes, checking only err < 0 lets execution
proceed to UAC_SET_CUR, writing uninitialized stack memory on ch == 0 (or
buf[0] overwritten with 0 by UAC_FU_MUTE on ch > 0) to the hardware volume
control.  Should this check err < 2 instead?

> +		if (err < 0)
> +			return err;
> +
> +		buf[0] = 0; /* unmute */
> +		err = snd_usb_ctl_msg(chip->dev, usb_sndctrlpipe(chip->dev, 0),
> +				      UAC_SET_CUR,
> +				      USB_DIR_OUT | USB_TYPE_CLASS |
> +				      USB_RECIP_INTERFACE,
> +				      (UAC_FU_MUTE << 8) | ch,
> +				      (5 << 8) | ifnum, buf, 1);
> +		if (err < 0)
> +			return err;
> +	}
> +	return 0;
> +}
> +
>  int snd_usb_mixer_apply_create_quirk(struct usb_mixer_interface *mixer)
>  {
>  	int err = 0;
> @@ -4616,6 +4669,9 @@ int snd_usb_mixer_apply_create_quirk(struct usb_mixer_interface *mixer)
>  	case USB_ID(0x0644, 0x8047):
>  		err = snd_us16x08_controls_create(mixer);
>  		break;
> +	case USB_ID(0x040b, 0x0897):
> +		err = snd_xiisound_init_fu_pins(mixer);
> +		break;

[Severity: Medium]
Should snd_xiisound_init_fu_pins() also be called from
snd_usb_mixer_resume_quirk(), similar to dell_dock_mixer_init() for
USB_ID(0x0bda, 0x4014)?

Because xiisound_map ignores unit 5, __build_feature_ctl() never registers an
element in mixer->id_elems[5], so snd_usb_mixer_resume() ->
restore_mixer_value() will not restore unit 5 after a suspend/resume cycle
that resets the device, leaving playback audio muted or broken with no userspace
controls to recover it.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261107020622.119529-1-cristianocps@gmail.com?part=1