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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Kernel panic due to invalid type cast when transport is not USB (e.g. uhid).
- [High] Kernel panic from dereferencing an ERR_PTR in the raw event handler.
- [High] Exposing fabricated 0 values for unsupported TIME_TO_EMPTY properties causes immediate system shutdown.
- [Medium] Hardcoded sysfs name prevents multiple devices and causes probe failure.
- [Medium] Data race on `ups->charge_limit`.
- [Low] Missing `POWER_SUPPLY` dependency in Kconfig causes build failures.
- [Low] Driver synchronously blocks the probe path for 3 seconds.
--

commit 7d140ce589ec6c637bec5ce8b0c27bf8634322c6
Author: Andrew Maney <andrewmaney05@gmail.com>
Subject: HID: Expose LattePanda IOTA UPS as a power_supply device

This commit adds a driver for the LattePanda IOTA UPS. It exposes battery
status, capacity, and charging status via the power_supply subsystem by parsing
custom HID reports.

> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -105,6 +105,16 @@ config HID_HAPTIC
>  
>  menu "Special HID drivers"
>  
> +config HID_LATTEPANDA_IOTA_UPS
> +	tristate "LattePanda IOTA UPS"
> +	depends on USB_HID && X86

If the kernel is configured with CONFIG_POWER_SUPPLY=m and
CONFIG_HID_LATTEPANDA_IOTA_UPS=y, will the built-in HID driver fail to link
against the modular power supply core? Should this include a dependency on
POWER_SUPPLY?

> diff --git a/drivers/hid/hid-lattepanda-iota-ups.c b/drivers/hid/hid-lattepanda-iota-ups.c
> --- /dev/null
> +++ b/drivers/hid/hid-lattepanda-iota-ups.c
[ ... ]
> +static enum power_supply_property iota_ups_properties[] = {
> +	POWER_SUPPLY_PROP_STATUS,
> +	POWER_SUPPLY_PROP_CAPACITY,
> +	POWER_SUPPLY_PROP_PRESENT,
> +	POWER_SUPPLY_PROP_ONLINE,
> +	POWER_SUPPLY_PROP_SCOPE,
> +	POWER_SUPPLY_PROP_TECHNOLOGY,
> +	POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD,
> +	POWER_SUPPLY_PROP_CAPACITY_LEVEL,
> +	POWER_SUPPLY_PROP_TIME_TO_EMPTY_NOW,
> +	POWER_SUPPLY_PROP_TIME_TO_FULL_NOW,

If the hardware doesn't support reporting time remaining, is it safe to
include these properties and return 0? 

Returning 0 seconds remaining while discharging can signal to userspace power
daemons like UPower or systemd-logind that the battery is empty, which might
trigger an immediate emergency suspend or shutdown. Could these unsupported
properties be omitted from the array entirely?

[ ... ]
> +static int iota_ups_set_property(struct power_supply *supply,
> +				 enum power_supply_property psp,
> +				 const union power_supply_propval *val)
> +{
> +	struct iota_ups *ups = power_supply_get_drvdata(supply);
> +
> +	if (psp == POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD) {
> +		/*
> +		 * V1.0 supports 80% and 100% charge limits only, set via
> +		 * DIP switch on the board. This property allows userspace
> +		 * to inform the driver which limit is configured.
> +		 */
> +		if (val->intval != 80 && val->intval != 100)
> +			return -EINVAL;
> +		ups->charge_limit = val->intval;

Does this introduce a data race on ups->charge_limit? 

This field is modified here without holding ups->lock, but it is concurrently
read in iota_ups_raw_event() and iota_ups_get_property() where it is
protected by spin_lock_irqsave(). Should this assignment also be
protected by the spinlock?

[ ... ]
> +static int iota_ups_raw_event(struct hid_device *hdev,
> +			      struct hid_report *report,
> +			      u8 *data, int size)
> +{
> +	struct iota_ups *ups = hid_get_drvdata(hdev);
[ ... ]
> +	/*
> +	 * Notify the power_supply core outside the spinlock. This
> +	 * triggers UPower's PropertiesChanged signal with the new values.
> +	 */
> +	if (changed && ups->psu)
> +		power_supply_changed(ups->psu);

Could ups->psu contain an error pointer here?

In iota_ups_probe(), if devm_power_supply_register() fails, it returns an
error pointer like ERR_PTR(-ENOMEM), which gets directly assigned to ups->psu.

Because hid_hw_start() is called earlier, raw HID events can be processed
concurrently before the hardware is stopped in the error path. Since an error
pointer is not NULL, the check (changed && ups->psu) would pass, and
power_supply_changed() would be called with the error pointer, leading to a
crash.

Would it be safer to assign the return value of devm_power_supply_register()
to a local variable first, and only update ups->psu on success?

[ ... ]
> +static int iota_ups_probe(struct hid_device *hdev,
> +			  const struct hid_device_id *id)
> +{
[ ... ]
> +	/* Retrieve serial number from the USB descriptor */
> +	udev = to_usb_device(hdev->dev.parent->parent);
> +	if (udev->serial)
> +		strscpy(ups->serial, udev->serial, sizeof(ups->serial));

Can this cause a kernel panic if the HID device is instantiated via userspace
mock drivers like uhid?

When using uhid (which allows setting the bus type to BUS_USB), the parent
device is the uhid character device, not a usb_interface. Dereferencing the
resulting struct usb_device pointer would perform an out-of-bounds memory
read.

Should the driver verify the transport layer using hid_is_usb() before
dereferencing USB-specific structures, or simply use hdev->uniq instead?

[ ... ]
> +	/*
> +	 * Wait for both status and capacity reports before registering.
> +	 * The device sends reports every ~1 second; 3 seconds is safe.
> +	 */
> +	wait_for_completion_timeout(&ups->got_initial_data,
> +				    msecs_to_jiffies(3000));

Does blocking the probe path for 3 seconds cause issues for device
enumeration? 

Calling wait_for_completion_timeout() synchronously stalls the kernel's
device enumeration thread for that bus, which can unnecessarily delay the
overall system boot process or stall the handling of other devices during
hotplug.

Could the registration be deferred, or could the device be registered
immediately with an unknown state instead of blocking?

> +
> +	ups->psu_desc.name			= "lattepanda-iota-ups";

Is it possible to have multiple of these UPS devices connected to a single
system? 

If a second device is plugged in, it will attempt to register a sysfs
directory with the exact same hardcoded name, which fails with -EEXIST.

This probe failure would then trigger the error pointer panic mentioned
earlier. Should the driver dynamically construct a unique name using
devm_kasprintf() with dev_name(&hdev->dev)?

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