Re: [PATCH v7 1/2] ALSA: usb-audio: expose the Topping M62's analogue gains as mixer controls

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The keepalive worker reschedules itself indefinitely and causes log spam every 2 seconds if the device is unplugged while userspace holds the ALSA card open.
- [Medium] Cache desynchronization race between `topping_ctl_put` and concurrent unsolicited hardware events.
--

--- Patch [1]: [PATCH v7 1/2] ALSA: usb-audio: expose the Topping M62's analogue gains as mixer controls ---
commit 97e5c28d5c9df80af2f3a0a9199b11cf828931a5
Author: Mikhail Gavrilov <mikhail.v.gavrilov@gmail.com>

ALSA: usb-audio: expose the Topping M62's analogue gains as mixer controls

This commit introduces a mixer driver for the Topping M62 to expose analogue
input gains and output volumes hidden behind a vendor HID protocol. It
subscribes to device state changes and maintains an internal cache updated
by a URB completion handler.

> diff --git a/sound/usb/mixer_topping.c b/sound/usb/mixer_topping.c
> new file mode 100644
> --- /dev/null
> +++ b/sound/usb/mixer_topping.c
[ ... ]
> +static int topping_send(struct topping_mixer *tm, u8 target, u8 prop,
> +			s32 value)
> +{
[ ... ]
> +	err = usb_interrupt_msg(tm->mixer->chip->dev, tm->pipe_out,
> +				buf, TOPPING_FRAME_LEN, &actual, 1000);
> +	if (err < 0)
> +		usb_audio_err(tm->mixer->chip,
> +			      "Topping: write %02x/%02x failed: %d\n",
> +			      target, prop, err);
> +	return err;
> +}
[ ... ]
> +static void topping_keepalive(struct work_struct *work)
> +{
> +	struct topping_mixer *tm = container_of(work, struct topping_mixer,
> +						keepalive.work);
> +
> +	topping_send(tm, TOPPING_TT_DEVICE, TOPPING_PP_SUBSCRIBE, 1);
> +	schedule_delayed_work(&tm->keepalive,
> +			      msecs_to_jiffies(TOPPING_KEEPALIVE_MS));
> +}

[Severity: Medium]
What prevents this worker from rescheduling itself indefinitely if the
device is unplugged?

If the ALSA card is kept open by userspace, topping_send() will continually
return an error (like -ENODEV) when the device is disconnected, which is then
unconditionally logged every 2 seconds. The worker then proceeds to call
schedule_delayed_work() here without checking if the device is gone.

Should it check the return value of topping_send() or check something like
tm->mixer->disconnected before rescheduling?

[ ... ]
> +static int topping_ctl_put(struct snd_kcontrol *kctl,
> +			   struct snd_ctl_elem_value *ucontrol)
> +{
[ ... ]
> +	err = topping_send(tm, d->target, d->prop, value);
> +	if (err < 0)
> +		return err;
> +	if (d->target_pair) {
> +		/*
> +		 * The device announces only one of a pair, so the other
> +		 * would drift away unheard.
> +		 */
> +		err = topping_send(tm, d->target_pair, d->prop, value);
> +		if (err < 0)
> +			return err;
> +	}
> +
> +	scoped_guard(spinlock_irqsave, &tm->lock)
> +		tm->val[elem->control] = value;
> +	return 1;
> +}

[Severity: Medium]
Could this blind update of tm->val desynchronize the cache from the actual
hardware state?

The tm->lock is released while calling topping_send(), which can sleep. If a
user turns a physical knob on the device concurrently, the device emits an
unsolicited URB. The URB completion handler (topping_urb_complete()) runs,
acquires the spinlock, and correctly updates the cache to the true hardware
state.

However, once topping_send() returns, this code reacquires the lock and
unconditionally overwrites tm->val[elem->control] with its own original
value. This ignores any fresh hardware events that arrived during the window.

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