Re: [PATCH v3] HID: Expose LattePanda IOTA UPS as a power_supply device

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The driver unconditionally binds to the generic Arduino Leonardo VID/PID, breaking standard HID input functionality for all such devices.
- [Medium] Device removal can block the USB disconnect process for up to 3 seconds if the device is unplugged during initialization.
- [Medium] The Kconfig dependency incorrectly requires `USB_HIDDEV`, preventing the driver from being built on modern systems.
- [Low] Unsynchronized access to the `ups->psu` pointer causes a C data race.
--

commit 9862889f837997957a0680fd4f3667a6d06c2828
Author: Andrew Maney <andrewmaney05@gmail.com>

HID: Expose LattePanda IOTA UPS as a power_supply device

This commit introduces a new HID driver to expose the LattePanda IOTA UPS
as a power_supply device. It reports battery status and capacity via HID
reports from a custom Arduino Leonardo firmware.

> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index ff2f580b660ba..21ffc2fd06096 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -510,6 +510,16 @@ config HID_KYSONA
>  	Say Y here if you have a Kysona M600 mouse
>  	and want to be able to read its battery capacity.
>  
> +config HID_LATTEPANDA_IOTA_UPS
> +	tristate "LattePanda IOTA UPS"
> +	depends on USB_HID && USB_HIDDEV && X86 && POWER_SUPPLY

[Severity: Medium]
Does this driver actually require the legacy /dev/usb/hiddevX interface?

Since the driver connects using HID_CONNECT_HIDRAW, depending on USB_HIDDEV
could unnecessarily prevent the driver from being enabled or compiled on
distributions that have disabled legacy hiddev support.

> +	help
> +	Support for the LattePanda IOTA UPS (DFRobot, VID 0x2341 PID 0x8036).
> +	Exposes the battery status and capacity via the power_supply interface.

[ ... ]

> diff --git a/drivers/hid/hid-lattepanda-iota-ups.c b/drivers/hid/hid-lattepanda-iota-ups.c
> new file mode 100644
> index 0000000000000..f5d522695081e
> --- /dev/null
> +++ b/drivers/hid/hid-lattepanda-iota-ups.c
> @@ -0,0 +1,409 @@
[ ... ]
> +static const struct hid_device_id iota_ups_devices[] = {
> +	{ HID_USB_DEVICE(USB_VENDOR_ID_LATTEPANDA_IOTA,
> +		USB_DEVICE_ID_LATTEPANDA_IOTA_UPS) },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(hid, iota_ups_devices);

[Severity: High]
Does this driver unconditionally bind to the generic Arduino Leonardo VID/PID
(0x2341/0x8036)?

Since it matches the standard development board ID and uses HID_CONNECT_HIDRAW
without HID_CONNECT_HIDINPUT, will this prevent any Arduino Leonardo acting as
a standard keyboard, mouse, or gamepad from functioning properly?

The driver might need to inspect the HID report descriptor during probe()
(e.g., checking for specific report IDs or usages) to confirm it is actually
communicating with the custom UPS firmware before returning success.

[ ... ]

> +static int iota_ups_raw_event(struct hid_device *hdev,
> +			      struct hid_report *report,
> +			      u8 *data, int size)
> +{
[ ... ]
> +	/*
> +	 * Notify the power_supply core outside the spinlock to avoid
> +	 * a deadlock; power_supply_changed() may call back into
> +	 * get_property() which acquires the same lock.
> +	 */
> +	if (changed && ups->psu)
> +		power_supply_changed(ups->psu);

[Severity: Low]
Could there be a data race when accessing ups->psu here?

In iota_ups_register_work(), ups->psu is assigned without synchronization.
Concurrently, iota_ups_raw_event() reads ups->psu outside of the spinlock.

While x86 aligned pointer writes are atomic (preventing partial pointer reads),
this is undefined behavior in C. Using WRITE_ONCE() and READ_ONCE(), or safely
reading the pointer inside the existing ups->lock critical section, would
prevent this.

> +
> +	return 0;
> +}

[ ... ]

> +static void iota_ups_remove(struct hid_device *hdev)
> +{
> +	struct iota_ups *ups = hid_get_drvdata(hdev);
> +
> +	cancel_work_sync(&ups->register_work);

[Severity: Medium]
Will device removal block the USB disconnect process for up to 3 seconds if
the device is unplugged during initialization?

If the device is abruptly disconnected while iota_ups_register_work() is still
waiting in wait_for_completion_timeout(), cancel_work_sync() will block here.

Since the physically removed device cannot send reports to trigger complete(),
the wait will always take the full 3 seconds. The driver likely needs to signal
completion proactively during removal to abort the wait before calling
cancel_work_sync().

> +	hid_hw_close(hdev);
> +	hid_hw_stop(hdev);
> +}

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