Re: [PATCH v3 2/2] ALSA: usb-audio: let the M62's outputs say what they listen to

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Hardware selector state desynchronization on device resume preventing state restoration.
--

--- Patch [2]: [PATCH v3 2/2] ALSA: usb-audio: let the M62's outputs say what they listen to ---
commit be8957abcd6e5ddab4c6d827af25bced12462261
Author: Mikhail Gavrilov <mikhail.v.gavrilov@gmail.com>

ALSA: usb-audio: let the M62's outputs say what they listen to

This adds source selector controls for the M62 sound card to handle
its output routing. It uses an "Unknown" default item since the card
never reports the active selector.

> diff --git a/sound/usb/mixer_topping.c b/sound/usb/mixer_topping.c
> index 2f2365b9b4762..8bf5fe483ccf1 100644
> --- a/sound/usb/mixer_topping.c
> +++ b/sound/usb/mixer_topping.c
[ ... ]
> @@ -374,6 +374,60 @@ static int topping_sel_put(struct snd_kcontrol *kctl,
> +			   struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct usb_mixer_elem_info *elem = kctl->private_data;
> +	struct topping_mixer *tm = elem->head.mixer->private_data;
> +	const struct topping_enum_desc *d;
> +	unsigned int item;
> +	int err;
> +
> +	item = ucontrol->value.enumerated.item[0];
> +	if (item >= ARRAY_SIZE(topping_sources))
> +		return -EINVAL;
> +
> +	guard(mutex)(&tm->write_lock);
> +
> +	/*
> +	 * "Unknown" is what this control reports until a hand has chosen,
> +	 * and alsactl stores and restores it like any other value.  It is
> +	 * not a choice, so writing it changes nothing -- quietly, rather
> +	 * than failing a restore of the driver's own report.
> +	 */
> +	if (!item || tm->sel[elem->control] == item)
> +		return 0;

[Severity: High]
If the system suspends and loses USB power, the hardware resets its selector
to the default.

Because topping_add_sel() sets elem->val_type = USB_MIXER_BESPOKEN, the ALSA
core's restore_mixer_value() skips restoring this control on resume.

Since topping_resume() doesn't manually push tm->sel back to the hardware,
tm->sel retains the pre-suspend value while the hardware has reset.

If the user then tries to re-apply their previous setting, won't this check
evaluate to true and silently block the write, permanently desynchronizing
the hardware and driver state until a different setting is applied first?

[ ... ]
> +static int topping_add_sel(struct topping_mixer *tm, int idx)
> +{
> +	struct usb_mixer_elem_info *elem;
> +	struct snd_kcontrol *kctl;
> +
> +	elem = kzalloc_obj(*elem);
> +	if (!elem)
> +		return -ENOMEM;
> +
> +	elem->head.mixer = tm->mixer;
> +	elem->head.id = 0;
> +	elem->control = idx;
> +	elem->channels = 1;
> +	elem->val_type = USB_MIXER_BESPOKEN;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260823194822.29430-1-mikhail.v.gavrilov@gmail.com?part=2