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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] HID: santroller: completion token consumed in santroller_get_features()
- [Medium] HID: santroller: santroller_resume() clobbers dirty flag and skips input check
- [Medium] HID: santroller: unlocked color calculation in santroller_rgb_led_set()
- [Low] HID: santroller: missing USB_HID dependency for hid_is_usb()
--

commit 5d9a23d03a81b34363d47e701c2dd2ced730bac9
Author: Sanjay Govind <sanjay.govind9@gmail.com>

HID: santroller: add driver for Santroller controllers

This commit adds a HID driver for game controllers and instruments running
the open source Santroller firmware. It queries a vendor features report to
configure force-feedback rumble, player LEDs, a multicolor RGB indicator LED,
and the DJ Hero turntable euphoria LED.

> 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]
In drivers/hid/hid-santroller.c, santroller_profile_name() unconditionally
calls hid_is_usb(hdev):

	if (!hid_is_usb(hdev))
		return NULL;

However, include/linux/hid.h declares hid_is_usb() as an extern symbol with
no inline fallback when CONFIG_USB_HID is disabled, and its definition in
drivers/hid/usbhid/hid-core.c is only built when CONFIG_USB_HID is enabled.

Can building with CONFIG_USB_HID=n (or CONFIG_USB_HID=m and
CONFIG_HID_SANTROLLER=y) fail at link time with an undefined reference to
hid_is_usb()? Should HID_SANTROLLER depend on USB_HID (or guard the USB
helper with IS_ENABLED(CONFIG_USB_HID))?

> diff --git a/drivers/hid/hid-santroller.c b/drivers/hid/hid-santroller.c
> new file mode 100644
> index 0000000000000..a2048e4ea3215
> --- /dev/null
> +++ b/drivers/hid/hid-santroller.c
[ ... ]
> +static void santroller_rgb_led_set(struct led_classdev *led,
> +				   enum led_brightness brightness)
> +{
> +	struct led_classdev_mc *mc = lcdev_to_mccdev(led);
> +	struct santroller *sc = container_of(mc, struct santroller, rgb_led);
> +	int i;
> +
> +	led_mc_calc_color_components(mc, brightness);
> +
> +	guard(spinlock_irqsave)(&sc->lock);
> +
> +	for (i = 0; i < ARRAY_SIZE(sc->rgb); i++)
> +		sc->rgb[i] = mc->subled_info[i].brightness;
> +	sc->rgb_dirty = true;
> +	santroller_schedule_output(sc);
> +}

[Severity: Medium]
Since santroller_rgb_led_set() is registered as the non-blocking
led->brightness_set callback, it can be invoked concurrently from software
blink timers, triggers, or sysfs writes without led_access held.

Because led_mc_calc_color_components(mc, brightness) updates
mc->subled_info[i].brightness in place before sc->lock is acquired, can two
concurrent calls to santroller_rgb_led_set() race on
mc->subled_info[i].brightness and copy stale or mixed RGB values into
sc->rgb[]? Should led_mc_calc_color_components() be called inside the
sc->lock critical section?

[ ... ]
> +static int santroller_raw_event(struct hid_device *hdev,
> +				struct hid_report *report, u8 *data, int size)
> +{
> +	struct santroller *sc = hid_get_drvdata(hdev);
> +
> +	if (sc && data[0] == SANTROLLER_REPORT_ID_FEATURES &&
> +	    size >= SANTROLLER_FEATURES_REPORT_SIZE &&
> +	    !completion_done(&sc->features_done)) {
> +		sc->caps = data[2];
> +		complete(&sc->features_done);
> +	}
> +
> +	return 0;
> +}
> +
> +/* The device answers the features command with an input report */
> +static int santroller_get_features(struct santroller *sc)
> +{
> +	u8 *buf __free(kfree) = kzalloc(2, GFP_KERNEL);
> +	unsigned long timeout = SANTROLLER_FEATURES_TIMEOUT;
> +	int i, ret = -ETIMEDOUT;
> +
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	if (sc->hdev->bus == BUS_BLUETOOTH)
> +		timeout = SANTROLLER_FEATURES_TIMEOUT_BT;
> +
> +	for (i = 0; i < SANTROLLER_FEATURES_RETRIES; i++) {
> +		reinit_completion(&sc->features_done);
> +
> +		buf[0] = SANTROLLER_REPORT_ID_FEATURES;
> +		buf[1] = 0;
> +		ret = santroller_send_report(sc->hdev, buf, 2);
> +		if (ret < 0)
> +			continue;
> +
> +		if (wait_for_completion_timeout(&sc->features_done, timeout))
> +			return 0;
> +
> +		ret = -ETIMEDOUT;
> +	}
> +
> +	return ret;
> +}

[Severity: Medium]
When santroller_raw_event() receives SANTROLLER_REPORT_ID_FEATURES, it calls
complete(&sc->features_done), which increments sc->features_done.done from 0
to 1. When wait_for_completion_timeout(&sc->features_done, timeout) wakes up
in santroller_get_features(), do_wait_for_common() consumes that token and
decrements sc->features_done.done back to 0.

In santroller_probe(), complete(&sc->features_done) is only called when
santroller_get_features() fails:

	ret = santroller_get_features(sc);
	if (ret == -ENOMEM)
		goto err_close;
	if (ret) {
		hid_warn(hdev, "failed to get device features: %d\n", ret);
		/* Ignore a late reply, the caps are settled from here on */
		complete(&sc->features_done);
	}

When santroller_get_features() succeeds, sc->features_done.done remains 0,
so completion_done(&sc->features_done) in santroller_raw_event() continues to
return false for the lifetime of the device. In addition, reinit_completion()
in the retry loop can run concurrently with santroller_raw_event() if a reply
to an earlier attempt arrives just as a timeout expires.

Can a delayed retry response or a later SANTROLLER_REPORT_ID_FEATURES report
overwrite sc->caps in santroller_raw_event() without synchronization while
santroller_probe() or santroller_resume() is reading sc->caps? Would
complete_all() (or guarding sc->caps with a flag under sc->lock) avoid this?

[ ... ]
> +static int santroller_resume(struct hid_device *hdev)
> +{
> +	struct santroller *sc = hid_get_drvdata(hdev);
> +
> +	if (!sc)
> +		return 0;
> +
> +	/* The device may have lost its LED state, so send it again */
> +	guard(spinlock_irqsave)(&sc->lock);
> +
> +	sc->player_dirty = sc->caps & SANTROLLER_CAP_PLAYER_LEDS;
> +	sc->rgb_dirty = sc->caps & SANTROLLER_CAP_RGB_LED;
> +	sc->rumble_dirty = sc->caps & SANTROLLER_CAP_EUPHORIA_LED;
> +	santroller_schedule_output(sc);
> +
> +	return 0;
> +}

[Severity: Medium]
There appear to be two issues in santroller_resume():

First, sc->rumble_dirty is shared between FF_RUMBLE in
santroller_play_effect() and the euphoria LED in
santroller_euphoria_led_set(). Assigning sc->rumble_dirty with '=' instead of
'|=' clears sc->rumble_dirty to false on controllers that support
SANTROLLER_CAP_RUMBLE but not SANTROLLER_CAP_EUPHORIA_LED. If a rumble update
was queued before santroller_output_work() ran, will santroller_resume() drop
that pending rumble command?

Second, in santroller_probe(), if sc->input is NULL, the driver jumps to out
without registering any LEDs or initializing sc->player (leaving sc->player
at 0 rather than SANTROLLER_PLAYER_LED_OFF):

	/* The LEDs are named after the gamepad input device */
	if (!sc->input)
		goto out;

Because santroller_resume() does not check sc->input before marking the LEDs
dirty, can resuming a device where sc->input is NULL send uninitialized LED
state to the controller?

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