Re: [PATCH v5 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] Self-deadlock (AB-BA) on tm->write_lock during USB runtime resume.
--

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

This patch adds ALSA controls for output sources on the M62 USB audio
interface, allowing the selection of internal mixes, inputs, or playback buses.
It exposes these options to the user and ensures the device state is updated,
including restoring choices upon resume.

> diff --git a/sound/usb/mixer_topping.c b/sound/usb/mixer_topping.c
> index 2f42b26336266..1743b8ee7facb 100644
> --- a/sound/usb/mixer_topping.c
> +++ b/sound/usb/mixer_topping.c

[ ... ]

> @@ -342,6 +382,66 @@ static int topping_ctl_put(struct snd_kcontrol *kctl,
[ ... ]
> +static int topping_sel_put(struct snd_kcontrol *kctl,
> +			   struct snd_ctl_elem_value *ucontrol)
> +{
[ ... ]
> +	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;
> +
> +	d = &topping_m62_enums[elem->control];
> +	err = topping_send(tm, d->target, d->prop,
> +			   topping_source_value[item]);

[ ... ]

> +static void topping_restore_sel(struct topping_mixer *tm)
> +{
> +	const struct topping_enum_desc *d;
> +	int i;
> +
> +	guard(mutex)(&tm->write_lock);

[Severity: High]
Could this cause a self-deadlock if the control is changed while the device
is in runtime suspend?

If a user modifies the ALSA control while the device is suspended,
topping_sel_put() acquires tm->write_lock and calls topping_send(). 

Inside topping_send(), the USB shutdown lock is acquired which triggers a
synchronous runtime resume. This will invoke the resume callback on the same
thread:

topping_sel_put() 
  -> topping_send()
    -> pm_runtime_get_sync()
      -> usb_audio_resume() 
        -> topping_resume() 
          -> topping_restore_sel()

Will topping_restore_sel() then block indefinitely trying to acquire
tm->write_lock since Linux mutexes are not recursive?

> +	for (i = 0; i < TOPPING_NUM_ENUMS; i++) {
> +		if (!tm->sel[i])
> +			continue;	/* nothing was ever chosen */
> +		d = &topping_m62_enums[i];
> +		topping_send(tm, d->target, d->prop,
> +			     topping_source_value[tm->sel[i]]);
> +	}
> +}

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