Re: [PATCH v5 1/2] HID: flydigi: add support for vader 5 pro

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] hid: flydigi: I/O on stopped hardware during probe error path
- [Medium] hid: flydigi: devm_power_supply_register error code is masked
- [Low] hid: flydigi: missing Kconfig dependency on POWER_SUPPLY
--

commit 31540c0ea898a7ef0b58eb15e340ebc735efebe9
Author: Denis Benato <denis.benato@linux.dev>

HID: flydigi: add support for vader 5 pro

The Flydigi Vader Pro 5 is a gamepad with additional buttons and a gyroscope
that connects to a PC via its own USB dongle. This commit adds support for it
by spawning two evdev nodes that share the same unique ID, allowing SDL to
expose them as a single controller.

> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index a81bf51cbcf10..3f080d59a897b 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -1205,6 +1205,14 @@ config STEAM_FF
>  	Say Y here if you want to enable force feedback support for the Steam
>  	Deck.
>  
> +config HID_FLYDIGI
> +	tristate "Flydigi game controller support"
> +	depends on USB_HID
> +	select INPUT_FF_MEMLESS

[Severity: Low]
Since the driver unconditionally uses power supply APIs like
devm_power_supply_register(), does this Kconfig entry need a
'depends on POWER_SUPPLY' dependency? 

If a user builds with CONFIG_HID_FLYDIGI=y and CONFIG_POWER_SUPPLY=n, it
appears the build will fail with unresolved symbols because the power supply
APIs do not provide static dummy implementations for that configuration.

> diff --git a/drivers/hid/hid-flydigi.c b/drivers/hid/hid-flydigi.c
> new file mode 100644
> index 0000000000000..b6d57080fed8f
> --- /dev/null
> +++ b/drivers/hid/hid-flydigi.c
[ ... ]
> +static int flydigi_register_battery(struct flydigi_device *fd)
> +{
[ ... ]
> +	fd->battery = devm_power_supply_register(&hdev->dev, &fd->battery_desc,
> +						 &psy_cfg);
> +	if (IS_ERR(fd->battery)) {
> +		/*
> +		 * flydigi_report_battery() only checks for NULL, so packets
> +		 * received while the probe failure unwinds must not find an
> +		 * error pointer here.
> +		 */
> +		fd->battery = NULL;
> +		return PTR_ERR(fd->battery);

[Severity: Medium]
By setting fd->battery to NULL before returning PTR_ERR(fd->battery), doesn't
this cause the function to always return 0 (success) on failure? 

It seems this will mask the actual error code (like -ENOMEM) and leave the
device partially initialized when devm_power_supply_register() fails.
Consider storing the error code in a temporary variable before nullifying the
pointer.

[ ... ]
> +static int flydigi_probe(struct hid_device *hdev, const struct hid_device_id *id)
> +{
[ ... ]
> +err_stop:
> +	hid_hw_stop(hdev);
> +	return ret;
> +}

[Severity: High]
If probe fails after hid_hw_start() succeeds (for example, in
flydigi_register_sensors or hid_hw_open), could this error path lead to I/O
on stopped hardware?

Since fd->removed is not set to true here, when devres subsequently unregisters
the gamepad, it will trigger flydigi_gamepad_close(). Looking at
flydigi_gamepad_close():

	/* Stop the motors right away on close. */
	if (!removed)
		mod_delayed_work(fd->wq, &fd->rumble_work, 0);

This will enqueue the rumble worker because removed is false. The rumble worker
then executes:

static void flydigi_rumble_worker(struct work_struct *work)
{
	...
	if (removed)
		return;

	flydigi_send_rumble(fd, strong >> 8, weak >> 8);
}

This appears to call into the HID transport (hid_hw_output_report) after
hid_hw_stop() has already shut down the hardware, which could lead to URB
submission errors or use-after-free in the lower-level transport driver.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260915171425.11058-1-denis.benato@linux.dev?part=1