Re: [RFC PATCH v3 1/2] HID: topping-m62: driver for the M62's vendor controls

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Lockless access to newly initialized ALSA controls (m62->kctl) without memory barriers causing a data race.
- [Low] Mixing goto-based error handling with scope-based cleanup helpers (`scoped_guard`) in the same function.
--

commit 0bcb87ac2e7106ce707298feca287dd58398496f
Author: Mikhail Gavrilov <mikhail.v.gavrilov@gmail.com>

HID: topping-m62: driver for the M62's vendor controls

This commit adds a driver for the Topping M62 USB audio interface to expose
vendor controls (such as analogue input gains, output volumes, and output
source selectors) through a HID-class interface. It registers a component
that interacts with the snd-usb-audio driver to add the controls to the
master sound card.

> diff --git a/drivers/hid/hid-topping-m62.c b/drivers/hid/hid-topping-m62.c
> new file mode 100644
> index 0000000000000..3c1b8e7626f69
> --- /dev/null
> +++ b/drivers/hid/hid-topping-m62.c

[ ... ]

> +static int topping_raw_event(struct hid_device *hdev,
> +			     struct hid_report *report, u8 *data, int size)
> +{

[ ... ]

> +	guard(spinlock_irqsave)(&m62->lock);
> +
> +	if (m62->val[idx] == value)
> +		return 0;
> +	m62->val[idx] = value;
> +
> +	if (m62->card && m62->kctl[idx])
> +		snd_ctl_notify(m62->card, SNDRV_CTL_EVENT_MASK_VALUE,
> +			       &m62->kctl[idx]->id);

[Severity: High]
Can this lockless read of m62->kctl[idx] race with the initialization
in topping_add_kctl()?

Because hid_device_io_start() is called in topping_probe() before the
component binds, USB interrupts can arrive while topping_bind() is running.

In topping_bind(), m62->card is set early under a lock:

        scoped_guard(spinlock_irqsave, &m62->lock)
                m62->card = card;

This exposes it to the interrupt handler. When topping_bind() subsequently
allocates controls, it publishes them to m62->kctl locklessly in
topping_add_kctl():

        m62->kctl[slot] = kctl;

Without memory barriers or lock synchronization here, could the CPU reorder
the initialization, causing this interrupt handler to observe a non-NULL
pointer but crash or misbehave when snd_ctl_notify() dereferences it?

> +
> +	return 0;
> +}

[ ... ]

> +static int topping_bind(struct device *comp, struct device *master,
> +			void *master_data)
> +{
> +	struct hid_device *hdev = to_hid_device(comp);
> +	struct topping_m62 *m62 = hid_get_drvdata(hdev);
> +	struct snd_card *card = master_data;
> +	int i, err;
> +
> +	scoped_guard(spinlock_irqsave, &m62->lock)
> +		m62->card = card;
> +
> +	for (i = 0; i < TOPPING_NUM_CTLS; i++) {
> +		err = topping_add_kctl(m62, &topping_ctl,
> +				       topping_m62_ctls[i].name, i, i,
> +				       topping_m62_ctls[i].tlv);
> +		if (err < 0)
> +			goto err_drop;

[Severity: Low]
This is a stylistic issue, but does this function mix goto-based error
handling with scope-based cleanup helpers?

According to the kernel's cleanup subsystem guidelines, functions should not
mix goto labels (like err_drop) with scope-based cleanups (like
scoped_guard). This is to prevent confusing ownership semantics and fragile
cleanup paths. Should this function be converted entirely to scope-based
cleanups or revert to traditional locking?

> +	}

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