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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] sysfs: subtype attribute is never created because drvdata is evaluated before probe
- [Medium] hid: santroller: TOCTOU race in santroller_player_led_set()
- [Low] cleanup: mixing goto-based error handling and scope-based guard in santroller_probe
--

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

HID: santroller: add driver for Santroller controllers

This commit adds a new driver for game controllers and instruments running
the open source Santroller firmware. It supports inputs, rumble, various
device LEDs, and exposes the specific controller subtype to userspace via
a sysfs attribute.

> diff --git a/drivers/hid/hid-santroller.c b/drivers/hid/hid-santroller.c
> new file mode 100644
> index 0000000000000..00adeb741a886
> --- /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);
> +	u8 player = SANTROLLER_PLAYER_LED_OFF;
> +	int i;
> +
> +	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;

[Severity: Medium]
Is there a potential regression here? If multiple player LEDs' brightnesses
are updated concurrently via sysfs, their respective callbacks could run in
parallel.

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.

[ ... ]

> +/* Interfaces left to the generic HID handling have no subtype */
> +static umode_t santroller_attr_is_visible(struct kobject *kobj,
> +					  struct attribute *attr, int n)
> +{
> +	struct device *dev = kobj_to_dev(kobj);
> +
> +	return hid_get_drvdata(to_hid_device(dev)) ? attr->mode : 0;
> +}

[Severity: Medium]
Could this visibility check cause a regression where the attribute is never
created?

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.

[ ... ]

> +static int santroller_probe(struct hid_device *hdev,
> +			    const struct hid_device_id *id)
> +{

[ ... ]

> +	hid_device_io_stop(hdev);
> +
> +	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;
> +	}

[Severity: Low]
Does this mix goto-based error handling with scope-based cleanup?

The kernel's cleanup subsystem guidelines explicitly state that a function
must either convert all resources to scope-based cleanup or use goto
exclusively. Mixing goto labels alongside scoped_guard can cause a
regression by creating confusing ownership semantics.

[ ... ]

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