Re: [PATCH] HID: santroller: add driver for Santroller controllers

[Sanjay Govind]

On Fri, 16 Oct 2026 03:06:58 +0000, sashiko-bot@xxxxxxxxxx wrote:
> > +	for (i = 0; i < SANTROLLER_NUM_PLAYER_LEDS; i++) {
> > +		if (sc->player_leds[i].brightness) {
> > +			player = i;
> > +			break;
> > +		}
> > +	}
> > +
> > +	guard(spinlock_irqsave)(&sc->lock);
> > +
> > +	sc->player = player;
>
> Since the brightness array is read outside sc->lock, one thread might read a
> stale array, get preempted, and then overwrite the hardware state with an
> outdated player index after the other thread has updated the state. Moving
> the array read inside the lock would prevent this.

Agreed. The LED core only serialises writes to the same LED, so two
player LEDs can be set at once. I'll move the scan under the lock in
v2.

> In the driver core, dev_groups assigned to the driver structure are
> evaluated by device_add_groups() before the driver's probe() function is
> executed. Because hid_set_drvdata() is only called later inside
> santroller_probe(), drvdata is strictly NULL when this visibility check
> is performed. This causes it to return 0 and skip attribute creation.

This one is a false positive. really_probe() adds drv->dev_groups
after call_driver_probe() returns, so drvdata is already set when
is_visible runs. In testing the attribute is present on the gamepad
interfaces, and hidden on the configuration interface as intended.

> > +	scoped_guard(spinlock_irqsave, &sc->lock)
> > +		sc->output_enabled = true;
> > +
> > +	ret = hid_connect(hdev, HID_CONNECT_DEFAULT);
> > +	if (ret) {
> > +		hid_err(hdev, "failed to connect HID device: %d\n", ret);
> > +		goto err_stop_output;
> > +	}
>
> Does this mix goto-based error handling with scope-based cleanup?

The guard can't be jumped across here, but I'll move it into a
santroller_start_output() helper in v2 so probe only uses goto.

Thanks,
Sanjay