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

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 [off-list ref]

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.

[ ... ]

+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