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

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

[Andrew Maney]

This driver exposes the DFRobot LattePanda IOTA UPS board as a standard
power_supply device, allowing desktop environments and power management
tools such as UPower and systemd-logind to display battery status,
remaining capacity, and charging status without any special
configuration. It also enables automatic suspend or shutdown on low
battery and power profile configuration via any tool that supports the
standard power_supply interface.

The UPS presents itself as an Arduino Leonardo HID device running custom
firmware (VID 0x2341, PID 0x8036). It reports status and capacity via
HID reports 0x07 and 0x0C respectively.

The charge limit (80% or 100%) is configured via a physical DIP switch
on the UPS board and cannot be detected automatically. Userspace can
inform the driver of the configured limit via
charge_control_end_threshold.

Known issue: the driver occasionally reports 0% capacity briefly on
initial load before the first valid HID report is received. I am
investigating the cause.

Signed-off-by: Andrew Maney <andrewmaney05@gmail.com>

---

Changes in v2:
- Rebased on top of the current tree
- Moved vendor and device IDs to drivers/hid/hid-ids.h
- Added Kconfig entry under HID bus support -> Special HID drivers
- Added build rule to drivers/hid/Makefile
---
 MAINTAINERS                           |   6 +
 drivers/hid/Kconfig                   |  10 +
 drivers/hid/Makefile                  |   1 +
 drivers/hid/hid-ids.h                 |   3 +
 drivers/hid/hid-lattepanda-iota-ups.c | 354 ++++++++++++++++++++++++++
 5 files changed, 374 insertions(+)
 create mode 100644 drivers/hid/hid-lattepanda-iota-ups.c

diff --git a/MAINTAINERS b/MAINTAINERS
index b2040011a..fd2947a80 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11410,6 +11410,12 @@ F:	include/uapi/linux/hid*
 F:	samples/hid/
 F:	tools/testing/selftests/hid/
 
+HID LATTEPANDA IOTA UPS DRIVER
+M:	Andrew Maney <andrewmaney05@gmail.com>
+L:	linux-input@vger.kernel.org
+S:	Maintained
+F:	drivers/hid/hid-lattepanda-iota-ups.c
+
 HID LOGITECH DRIVERS
 R:	Filipe Laíns <lains@riseup.net>
 L:	linux-input@vger.kernel.org
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index ff2f580b6..3c1efef3d 100644
--- 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
+	help
+	Support for the LattePanda IOTA UPS (DFRobot, VID 0x2341 PID 0x8036).
+	Exposes the battery status and capacity via the power_supply interface.
+
+	To compile as a module, choose M here: the module will be
+	called hid-lattepanda-iota-ups.
+
 config HID_A4TECH
 	tristate "A4TECH mice"
 	help
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index 0597fd6a4..7ba44223c 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -24,6 +24,7 @@ hid-logitech-$(CONFIG_LOGIWHEELS_FF)	+= hid-lg4ff.o
 hid-wiimote-y		:= hid-wiimote-core.o hid-wiimote-modules.o
 hid-wiimote-$(CONFIG_DEBUG_FS)	+= hid-wiimote-debug.o
 
+obj-$(CONFIG_HID_LATTEPANDA_IOTA_UPS)	+= hid-lattepanda-iota-ups.o
 obj-$(CONFIG_HID_A4TECH)	+= hid-a4tech.o
 obj-$(CONFIG_HID_ACCUTOUCH)	+= hid-accutouch.o
 obj-$(CONFIG_HID_ALPS)		+= hid-alps.o
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 0cf637423..3b69b072c 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -25,6 +25,9 @@
 #define USB_VENDOR_ID_8BITDO		0x2dc8
 #define USB_DEVICE_ID_8BITDO_PRO_3	0x6009
 
+#define USB_VENDOR_ID_LATTEPANDA_IOTA	0x2341
+#define USB_DEVICE_ID_LATTEPANDA_IOTA_UPS	0x8036
+
 #define USB_VENDOR_ID_A4TECH		0x09da
 #define USB_DEVICE_ID_A4TECH_WCP32PU	0x0006
 #define USB_DEVICE_ID_A4TECH_X5_005D	0x000a
diff --git a/drivers/hid/hid-lattepanda-iota-ups.c b/drivers/hid/hid-lattepanda-iota-ups.c
new file mode 100644
index 000000000..cb0c2a6f3
--- /dev/null
+++ b/drivers/hid/hid-lattepanda-iota-ups.c
@@ -0,0 +1,354 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/completion.h>
+#include <linux/hid.h>
+#include <linux/module.h>
+#include <linux/power_supply.h>
+#include <linux/spinlock.h>
+#include <linux/usb.h>
+#include "hid-ids.h"
+
+#define REPORT_ID_STATUS	0x07
+#define REPORT_ID_CAPACITY	0x0C
+
+#define STATUS_PLUGGED_IN	BIT(0)
+#define STATUS_DISCHARGING	BIT(1)
+#define STATUS_CHARGING		BIT(2)
+
+MODULE_AUTHOR("Andrew Maney");
+MODULE_DESCRIPTION("LattePanda IOTA UPS power supply driver");
+MODULE_LICENSE("GPL");
+
+struct iota_ups {
+	struct hid_device		*hiddev;
+	struct power_supply		*psu;
+	struct power_supply_desc	 psu_desc;
+	spinlock_t			 lock; /* Protects all cached HID report values */
+
+	/* Cached values updated from HID reports */
+	char	serial[64];
+	bool	plugged_in;
+	int	psu_status;
+	int	capacity;
+	int	charge_limit;
+
+	/*
+	 * Wait for both status and capacity reports before registering
+	 * with the power_supply core, so initial values are correct.
+	 */
+	struct completion	got_initial_data;
+	bool			data_ready;
+	bool			got_status;
+	bool			got_capacity;
+};
+
+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,
+	POWER_SUPPLY_PROP_HEALTH,
+	POWER_SUPPLY_PROP_VOLTAGE_NOW,
+	POWER_SUPPLY_PROP_MANUFACTURER,
+	POWER_SUPPLY_PROP_MODEL_NAME,
+	POWER_SUPPLY_PROP_SERIAL_NUMBER,
+};
+
+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);
+
+static int iota_ups_get_property(struct power_supply *supply,
+				 enum power_supply_property psp,
+				 union power_supply_propval *val)
+{
+	struct iota_ups *ups = power_supply_get_drvdata(supply);
+	unsigned long flags;
+
+	spin_lock_irqsave(&ups->lock, flags);
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_STATUS:
+		val->intval = ups->psu_status;
+		break;
+	/* Remaining capacity as a percentage, 0-100 */
+	case POWER_SUPPLY_PROP_CAPACITY:
+		val->intval = ups->capacity;
+		break;
+	/* Always present if the driver is loaded */
+	case POWER_SUPPLY_PROP_PRESENT:
+		val->intval = 1;
+		break;
+	/* Whether mains power is connected */
+	case POWER_SUPPLY_PROP_ONLINE:
+		val->intval = ups->plugged_in ? 1 : 0;
+		break;
+	/* System-level UPS, not a laptop battery */
+	case POWER_SUPPLY_PROP_SCOPE:
+		val->intval = POWER_SUPPLY_SCOPE_SYSTEM;
+		break;
+	/* V1.0 only accepts 18650 Li-ion cells */
+	case POWER_SUPPLY_PROP_TECHNOLOGY:
+		val->intval = POWER_SUPPLY_TECHNOLOGY_LION;
+		break;
+	/* 80% or 100%, configured via DIP switch on the board */
+	case POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD:
+		val->intval = ups->charge_limit;
+		break;
+	/* V1.0 does not report capacity level via HID */
+	case POWER_SUPPLY_PROP_CAPACITY_LEVEL:
+		val->intval = POWER_SUPPLY_CAPACITY_LEVEL_NORMAL;
+		break;
+	/* V1.0 does not report time remaining */
+	case POWER_SUPPLY_PROP_TIME_TO_EMPTY_NOW:
+	case POWER_SUPPLY_PROP_TIME_TO_FULL_NOW:
+		val->intval = 0;
+		break;
+	/* V1.0 does not report health; assume good */
+	case POWER_SUPPLY_PROP_HEALTH:
+		val->intval = POWER_SUPPLY_HEALTH_GOOD;
+		break;
+	/* 3.7V in microvolts, typical Li-ion resting voltage */
+	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
+		val->intval = 3700000;
+		break;
+	case POWER_SUPPLY_PROP_MANUFACTURER:
+		val->strval = "DFRobot";
+		break;
+	case POWER_SUPPLY_PROP_MODEL_NAME:
+		val->strval = "LattePanda IOTA UPS";
+		break;
+	/* Retrieved from the USB descriptor */
+	case POWER_SUPPLY_PROP_SERIAL_NUMBER:
+		val->strval = ups->serial;
+		break;
+	default:
+		spin_unlock_irqrestore(&ups->lock, flags);
+		return -EINVAL;
+	}
+
+	spin_unlock_irqrestore(&ups->lock, flags);
+	return 0;
+}
+
+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;
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+static int iota_ups_property_is_writable(struct power_supply *supply,
+					 enum power_supply_property psp)
+{
+	return psp == POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD;
+}
+
+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);
+	unsigned long flags;
+	bool changed = false;
+
+	/* All UPS reports are at least 2 bytes */
+	if (size < 2)
+		return 0;
+
+	spin_lock_irqsave(&ups->lock, flags);
+
+	switch (data[0]) {
+	case REPORT_ID_STATUS: {
+		u8 status = data[1];
+		int new_status;
+		bool plugged_in = !!(status & STATUS_PLUGGED_IN);
+
+		if (status & STATUS_CHARGING) {
+			if (ups->capacity >= ups->charge_limit)
+				new_status = POWER_SUPPLY_STATUS_FULL;
+			else
+				new_status = POWER_SUPPLY_STATUS_CHARGING;
+		} else if (status & STATUS_DISCHARGING) {
+			new_status = POWER_SUPPLY_STATUS_DISCHARGING;
+		} else if (plugged_in) {
+			new_status = POWER_SUPPLY_STATUS_FULL;
+		} else {
+			new_status = POWER_SUPPLY_STATUS_UNKNOWN;
+		}
+
+		if (new_status != ups->psu_status ||
+		    plugged_in != ups->plugged_in) {
+			ups->plugged_in = plugged_in;
+			ups->psu_status = new_status;
+			changed = true;
+		}
+
+		ups->got_status = true;
+		break;
+	}
+	case REPORT_ID_CAPACITY: {
+		int new_cap = clamp((int)data[1], 0, 100);
+
+		if (new_cap != ups->capacity) {
+			ups->capacity = new_cap;
+			changed = true;
+		}
+
+		ups->got_capacity = true;
+		break;
+	}
+	}
+
+	/*
+	 * Signal ready only once both status and capacity have been
+	 * received, so the power_supply is registered with valid data.
+	 */
+	if (!ups->data_ready && ups->got_status && ups->got_capacity) {
+		ups->data_ready = true;
+		complete(&ups->got_initial_data);
+	}
+
+	spin_unlock_irqrestore(&ups->lock, flags);
+
+	/*
+	 * 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);
+
+	return 0;
+}
+
+static int iota_ups_probe(struct hid_device *hdev,
+			  const struct hid_device_id *id)
+{
+	struct power_supply_config psu_config = {};
+	struct usb_device *udev;
+	struct iota_ups *ups;
+	int ret;
+
+	ups = devm_kzalloc(&hdev->dev, sizeof(*ups), GFP_KERNEL);
+	if (!ups)
+		return -ENOMEM;
+
+	ups->hiddev		= hdev;
+	ups->psu_status		= POWER_SUPPLY_STATUS_UNKNOWN;
+	ups->capacity		= 50;
+	/*
+	 * Default to 100% — the DIP switch may be set to 80% but there
+	 * is no way to detect this automatically from HID reports.
+	 * Userspace can update this via charge_control_end_threshold.
+	 */
+	ups->charge_limit	= 100;
+	ups->data_ready		= false;
+	ups->got_status		= false;
+	ups->got_capacity	= false;
+
+	init_completion(&ups->got_initial_data);
+	spin_lock_init(&ups->lock);
+	hid_set_drvdata(hdev, ups);
+
+	/* 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));
+	else
+		strscpy(ups->serial, "Unknown", sizeof(ups->serial));
+
+	ret = hid_parse(hdev);
+	if (ret) {
+		hid_err(hdev, "HID parse failed: %d\n", ret);
+		return ret;
+	}
+
+	ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
+	if (ret) {
+		hid_err(hdev, "HID hw start failed: %d\n", ret);
+		return ret;
+	}
+
+	ret = hid_hw_open(hdev);
+	if (ret) {
+		hid_err(hdev, "HID hw open failed: %d\n", ret);
+		goto err_stop;
+	}
+
+	/*
+	 * 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));
+
+	ups->psu_desc.name			= "lattepanda-iota-ups";
+	ups->psu_desc.type			= POWER_SUPPLY_TYPE_BATTERY;
+	ups->psu_desc.properties		= iota_ups_properties;
+	ups->psu_desc.num_properties		= ARRAY_SIZE(iota_ups_properties);
+	ups->psu_desc.get_property		= iota_ups_get_property;
+	ups->psu_desc.set_property		= iota_ups_set_property;
+	ups->psu_desc.property_is_writeable	= iota_ups_property_is_writable;
+	psu_config.drv_data = ups;
+
+	ups->psu = devm_power_supply_register(&hdev->dev,
+					      &ups->psu_desc,
+					      &psu_config);
+	if (IS_ERR(ups->psu)) {
+		ret = PTR_ERR(ups->psu);
+		hid_err(hdev, "power_supply register failed: %d\n", ret);
+		goto err_close;
+	}
+
+	/*
+	 * Force an immediate notification so UPower reads the correct
+	 * initial state right after registration.
+	 */
+	power_supply_changed(ups->psu);
+
+	hid_info(hdev, "LattePanda IOTA UPS registered as power supply\n");
+	return 0;
+
+err_close:
+	hid_hw_close(hdev);
+err_stop:
+	hid_hw_stop(hdev);
+	return ret;
+}
+
+static void iota_ups_remove(struct hid_device *hdev)
+{
+	hid_hw_close(hdev);
+	hid_hw_stop(hdev);
+}
+
+static struct hid_driver iota_ups_driver = {
+	.name		= "lattepanda-iota-ups",
+	.id_table	= iota_ups_devices,
+	.probe		= iota_ups_probe,
+	.remove		= iota_ups_remove,
+	.raw_event	= iota_ups_raw_event,
+};
+module_hid_driver(iota_ups_driver);
-- 
2.54.0

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

[sashiko-bot]

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

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

[Andrew Maney]

On Thu, May 14, 2026 at 2:50 AM <sashiko-bot@kernel.org> wrote:
>
> 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

Thanks for the detailed review! I'll do the following in v3:
* Remove TIME_TO_EMPTY_NOW and TIME_TO_FULL_NOW to avoid spurious
shutdowns. The UPS board doesn't currently have any way to report
these values as of right now.
* Add the POWER_SUPPLY requirement to Kconfig.
* Fix the data race for ups->charge_limit in set_property.
* Address the 3 second probe delay, it may not be needed. Registering
the UPS immediately with an unknown state may not be safe if its
reports are misinterpreted.
* Fix ERR_PTR panic with a local variable.
* Fix uhid panic with hid_is_usb() before dereferencing.
* Use devm_kasprintf() for a unique sysfs name. It is not currently
possible to have more than one UPS board connected to the IOTA, but
I'll implement this as a safeguard in case future hardware revisions
add support for multiple boards.

Thanks,
Andrew