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

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing PM resumption callbacks permanently disable hardware event notifications after system suspend.
- [Medium] HID interface is never released on error paths or during driver unbind, breaking subsequent binds.
- [Medium] Missing serialization in ALSA control put callbacks causes TOCTOU and data races.
--

--- Patch [1]: [PATCH v2 1/2] ALSA: usb-audio: expose the Topping M62's analogue gains as mixer controls ---
commit 18f648280996252310709cd8402415d38f18a8be
Author: Mikhail Gavrilov <[email protected]>

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

This commit introduces a mixer quirk driver for the Topping M62 audio
interface. It exposes analog gains and output volumes via a vendor protocol
over a HID-class interface by directly communicating with the device endpoints
and parsing unsolicited state reports.

> diff --git a/sound/usb/mixer_topping.c b/sound/usb/mixer_topping.c
> new file mode 100644
> index 0000000000000..3405f28dec66c
> --- /dev/null
> +++ b/sound/usb/mixer_topping.c
[ ... ]
> +static int topping_ctl_put(struct snd_kcontrol *kctl,
> +			   struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct usb_mixer_elem_info *elem = kctl->private_data;
> +	struct usb_mixer_interface *mixer = elem->head.mixer;
> +	struct topping_mixer *tm = mixer->private_data;
> +	const struct topping_ctl_desc *d = &tm->ctls[elem->control];
> +	int value, err;
> +
> +	value = ucontrol->value.integer.value[0];
> +	if (value < d->min || value > d->max)
> +		return -EINVAL;
> +
> +	scoped_guard(spinlock_irqsave, &tm->lock)
> +		if (tm->val[elem->control] == value)
> +			return 0;
> +
> +	err = topping_send(tm, d->target, d->prop, value);

[Severity: Medium]
Does dropping the lock across hardware transmission introduce a race condition?

The spinlock is used to check the value but is released before calling
topping_send(). If two threads simultaneously write different values to the
same control, the order of hardware execution might differ from the order of
the software state updates at the end of the function, causing get operations
to report incorrect values.

[ ... ]
> +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;
> +	if (!item)
> +		return -EINVAL;	/* "Unknown" is a report, not a choice */
> +	if (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]);
> +	if (err < 0)
> +		return err;
> +
> +	tm->sel[elem->control] = item;

[Severity: Medium]
Is it possible for concurrent writes to corrupt the cached state here?

This function lacks locking when reading and updating tm->sel[elem->control],
which could allow concurrent threads to write different values, leading to a
race where the software state drifts from the hardware state.

> +	return 1;
> +}
[ ... ]
> +static void topping_private_free(struct usb_mixer_interface *mixer)
> +{
> +	struct topping_mixer *tm = mixer->private_data;
> +
> +	if (!tm)
> +		return;
> +	cancel_delayed_work_sync(&tm->keepalive);
> +	if (tm->urb) {
> +		usb_kill_urb(tm->urb);
> +		usb_free_coherent(mixer->chip->dev, TOPPING_EP_BUF,
> +				  tm->inbuf, tm->inbuf_dma);
> +		usb_free_urb(tm->urb);
> +	}
> +	kfree(tm->val);
> +	kfree(tm->kctl);
> +	kfree(tm);
> +	mixer->private_data = NULL;

[Severity: Medium]
Does this code leak the claimed USB interface?

snd_usb_claim_iface() was called during initialization, but
usb_driver_release_interface() is not called here. If the driver is unbound
and rebound via sysfs, the subsequent probe will fail at the
usb_interface_claimed() check and skip creating the controls.

> +}
[ ... ]
> +int snd_topping_init(struct usb_mixer_interface *mixer)
> +{
> +	struct snd_usb_audio *chip = mixer->chip;
> +	struct usb_interface *iface;
> +	struct topping_mixer *tm;
> +	int ep_in = 0, ep_out = 0, interval = 5;
> +	int i, err;
[ ... ]
> +	err = snd_usb_claim_iface(chip, iface);
> +	if (err < 0)
> +		goto fail;
[ ... ]
> +	mixer->private_data = tm;
> +	mixer->private_free = topping_private_free;

[Severity: High]
Does this setup permanently lose hardware notifications after system suspend?

The URB completion handler (topping_urb_complete) stops on suspend errors, but
there is no mixer->private_resume assigned here to restart the URB on resume.
Without it, the driver won't receive unsolicited hardware state changes after a
suspend/resume cycle.

[ ... ]
> +fail:
> +	if (tm->inbuf)
> +		usb_free_coherent(chip->dev, TOPPING_EP_BUF, tm->inbuf,
> +				  tm->inbuf_dma);
> +	usb_free_urb(tm->urb);
> +	kfree(tm->val);
> +	kfree(tm->kctl);
> +	kfree(tm);
> +	return err;

[Severity: Medium]
If a memory allocation fails after claiming the interface, does this error path
leave the interface permanently claimed?

It appears we jump to fail without calling usb_driver_release_interface() to
undo the snd_usb_claim_iface() call made earlier.

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1