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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] HID: santroller: stale brightness read in
  santroller_player_led_set()
- [Low] HID: santroller: USB profile naming compiled out when USB_HID=m
  and HID_SANTROLLER=y
--

commit 28369893e9b4de37e159a34ce2a7a1ca4302cd7d
Author: Sanjay Govind <sanjay.govind9@gmail.com>

HID: santroller: add driver for Santroller controllers

This commit adds a HID driver for Santroller USB and Bluetooth game
controllers and instruments, supporting profile naming, rumble, player
LEDs, RGB indicator LEDs, and turntable euphoria LEDs.

> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index 2481bc01c435b..2fab7003ce18f 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -1138,6 +1138,18 @@ config HID_SAMSUNG
>  	help
>  	Support for Samsung InfraRed remote control or keyboards.
>  
> +config HID_SANTROLLER
> +	tristate "Santroller game controller support"
> +	depends on LEDS_CLASS_MULTICOLOR
> +	select INPUT_FF_MEMLESS

[Severity: Low]
Should this also depend on USB_HID || !USB_HID?  When CONFIG_HID_SANTROLLER=y
and CONFIG_USB_HID=m, IS_REACHABLE(CONFIG_USB_HID) in
santroller_profile_name() evaluates to false and silently compiles out USB
profile naming even though USB HID support is enabled.

> diff --git a/drivers/hid/hid-santroller.c b/drivers/hid/hid-santroller.c
> new file mode 100644
> index 0000000000000..7dcc5d7eae604
> --- /dev/null
> +++ b/drivers/hid/hid-santroller.c

[ ... ]

> +static void santroller_player_led_set(struct led_classdev *led,
> +				      enum led_brightness value)
> +{
> +	struct hid_device *hdev = to_hid_device(led->dev->parent);
> +	struct santroller *sc = hid_get_drvdata(hdev);
> +	int i;
> +
> +	guard(spinlock_irqsave)(&sc->lock);
> +
> +	sc->player = SANTROLLER_PLAYER_LED_OFF;
> +	for (i = 0; i < SANTROLLER_NUM_PLAYER_LEDS; i++) {
> +		if (sc->player_leds[i].brightness) {
> +			sc->player = i;
> +			break;
> +		}
> +	}
> +	sc->player_dirty = true;
> +	santroller_schedule_output(sc);
> +}

[Severity: Medium]
Can santroller_player_led_set() send a stale player LED state by ignoring
value and reading sc->player_leds[i].brightness?

When software blinking (such as the timer trigger) is stopped while the LED
is in its ON phase, led_set_brightness(led_cdev, LED_OFF) defers the update
to set_brightness_delayed() without clearing led_cdev->brightness:

drivers/leds/led-core.c:set_brightness_delayed() {
    ...
	if (test_and_clear_bit(LED_SET_BRIGHTNESS_OFF, &led_cdev->work_flags)) {
		set_brightness_delayed_set_brightness(led_cdev, LED_OFF);
    ...
}

set_brightness_delayed_set_brightness() calls __led_set_brightness(led_cdev,
LED_OFF) -> santroller_player_led_set(led_cdev, LED_OFF) while
led->brightness is still 1, leaving the hardware player LED lit.  Using
value for the entry matching led (or updating led->brightness = value under
sc->lock before the loop) would keep the state in sync.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261107234444.960207-2-sanjay.govind9@gmail.com?part=1