[PATCH v7 0/3] HID: generic: add LampArray support via hid-lamparray helper

[PATCH v7 0/3] HID: generic: add LampArray support via hid-lamparray helper

[Aaron Erhardt]

Add a new hid-lamparray helper module and integrate it with the
hid-generic driver.

While more complex lamparray handling should be done in userspace via
hidraw, providing a small module to add basic lamparray support makes it
possible for userspace software to interact with lamparrays by simply
using well-known APIs of the LED subsystem. One use-case would be to
enable desktop environments to support keyboard backlight control out of
the box for HID lamparray devices without having to implement the whole
HID protocol themselves.

This patch is based on previous discussions:
https://lore.kernel.org/all/1fb08a74-62c7-4d0c-ba5d-648e23082dcb@tuxedocomputers.com/

The helper provides basic support for devices exposing a
Lighting/LampArray application collection (usage page 0x59) and
registers a single-zone RGB LED representation via the LED subsystem.

hid-generic now checks for LampArray support after hid_parse() and
optionally registers a lamparray instance. Failures in the helper do not
abort device probe to keep the driver logic otherwise unchanged.

LampArray resources are released on driver remove.

This commit was successfully tested on the Microsoft MacroPad reference
implementation (https://github.com/microsoft/RP2040MacropadHidSample
1d6c3ad) and in combination with the tuxedo_nb04_wmi driver, albeit only
fully functional with a recent fix already on
platform-drivers-x86/for-next
(https://lore.kernel.org/all/20261007080202.81694-1-aer@tuxedocomputers.com).

Others reported the driver to work on the Acer Predator PT14-52T and the
ASUS Prime B650-Plus, with the former requiring some patches on top of
v5, which should now be included in v6.

v7:
- Use white with zero brightness as new default
- Fix measurements in comments and commit message
v6:
- Read attribute reports synchronously (by Cristian Mazotta)
- Raise log level of fatal probe errors (by Cristian Mazotta)
- Transfer control when use_leds_uapi changes (by Cristian Mazotta)
- Blank lamps across suspend and restore on resume (by Cristian Mazotta)
- Add quirk for devices that ignore intensities (reported by Armin Wolf)
- Better locking, fixing rare races in uapi registration
- Use scope guards for locking and allocations where possible
- Remove obsolete calls to led_mc_calc_color_components and
  hid_device_io_start/stop
- Updated and slightly improved documentation
v5:
- Proper hardware detection (no quirks necessary anymore)
- Add documentation for new sysfs knob
- Pass limits of the device to sysfs (intensities & brightness)
- More flexible Kconfig (use tristate)
- Improved locking
- Several memory leak and (de-)initialization fixes
- Don't read current color values from hardware (the HID spec does not
  offer this option)
- Remove redundant report dump functionality
v4:
- Restrict CONFIG_HID_LAMPARRAY to built-in configurations only to fix
  additional randconfig build errors
v3:
- Squash V1 and V2 into one patch
v2:
- Fix Kconfig to avoid build errors when LEDS_CLASS_MULTICOLOR is
  disabled

---
Aaron Erhardt (2):
      HID: lamparray: add new LampArray helper module
      HID: generic: add LampArray support via hid-lamparray helper

Cristian Mazzotta (1):
      HID: lamparray: blank lamps across suspend and restore on resume

 .../ABI/testing/sysfs-driver-hid-lamparray         |  15 +
 drivers/hid/Kconfig                                |  18 +
 drivers/hid/Makefile                               |   2 +
 drivers/hid/hid-generic.c                          |  70 +-
 drivers/hid/hid-lamparray.c                        | 912 +++++++++++++++++++++
 include/linux/hid-lamparray.h                      | 118 +++
 6 files changed, 1134 insertions(+), 1 deletion(-)
---
base-commit: f47d223627f80cb7a1eb14c585fe6cb35de5e984
change-id: 20261020-hid-lamparray-6f76a2791e8b

Best regards,
--  
Aaron Erhardt <aer@tuxedocomputers.com>

[PATCH v7 1/3] HID: lamparray: add new LampArray helper module

[Aaron Erhardt]

Add a new hid-lamparray helper module that provides basic support for
devices exposing a Lighting/LampArray application collection (usage
page 0x59) and registers a single-zone RGB LED representation via the
LED subsystem.

The module can be used as a library in HID drivers to add support for
the HID LampArray protocol. While the API is quite basic as of now
it could be extended in the future.

Co-developed-by: Tim Guttzeit <tgu@tuxedocomputers.com>
Signed-off-by: Tim Guttzeit <tgu@tuxedocomputers.com>
Co-developed-by: Cristian Mazzotta <cmmazzo@icloud.com>
Signed-off-by: Cristian Mazzotta <cmmazzo@icloud.com>
Signed-off-by: Aaron Erhardt <aer@tuxedocomputers.com>
---
 .../ABI/testing/sysfs-driver-hid-lamparray         |  15 +
 drivers/hid/Kconfig                                |  17 +
 drivers/hid/Makefile                               |   2 +
 drivers/hid/hid-lamparray.c                        | 858 +++++++++++++++++++++
 include/linux/hid-lamparray.h                      |  89 +++
 5 files changed, 981 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-driver-hid-lamparray b/Documentation/ABI/testing/sysfs-driver-hid-lamparray
new file mode 100644
index 0000000000000..a118d168b3bad
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-driver-hid-lamparray
@@ -0,0 +1,15 @@
+What:		/sys/bus/hid/devices/<bus>:<vid>:<pid>.<n>/use_leds_uapi
+Date:		December 2026
+KernelVersion:	7.4
+Contact:	aer@tuxedocomputers.com
+Description:
+        If a driver uses the hid-lamparray module and a device supporting
+        LampArray is found, one multicolor LED class device is registered under
+        /sys/class/leds/rgb:<function> to expose the single-zone RGB control.
+
+        Additionally, the use_leds_uapi sysfs attribute to control the LED class
+        device is attached directly to the HID device at
+        /sys/bus/hid/devices/<bus>:<vid>:<pid>.<n>/use_leds_uapi. Writing 0 to
+        use_leds_uapi unregisters the LED class device. The last state is kept
+        cached. Writing 1 registers it again and restores the cached state to
+        hardware.
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index c43e824428f02..934e502ba5058 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -92,6 +92,23 @@ config HID_GENERIC
 
 	If unsure, say Y.
 
+config HID_LAMPARRAY
+	tristate "HID LampArray helper"
+	depends on HID
+	depends on LEDS_CLASS_MULTICOLOR
+	default n
+	help
+	  Helper for HID devices exposing a Lighting/LampArray collection.
+	  Treats LampArray devices as a single-zone device and exposes a sysfs
+	  interface for changing color and intensity values. Also exposes a
+	  sysfs flag to be disabled e.g. by a userspace driver.
+
+	  This can be used as library in existing drivers. The generic HID
+	  driver is extended by default to handle lamp array devices if this
+	  option is enabled.
+
+	  If unsure, say N.
+
 config HID_HAPTIC
 	bool "Haptic touchpad support"
 	default n
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index 48a863b245eed..f95630fa8bd8c 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -13,6 +13,8 @@ obj-$(CONFIG_UHID)		+= uhid.o
 
 obj-$(CONFIG_HID_GENERIC)	+= hid-generic.o
 
+obj-$(CONFIG_HID_LAMPARRAY)	+= hid-lamparray.o
+
 hid-$(CONFIG_HIDRAW)		+= hidraw.o
 
 hid-logitech-y		:= hid-lg.o
diff --git a/drivers/hid/hid-lamparray.c b/drivers/hid/hid-lamparray.c
new file mode 100644
index 0000000000000..fb3082f6d19a0
--- /dev/null
+++ b/drivers/hid/hid-lamparray.c
@@ -0,0 +1,858 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * hid-lamparray.c - HID LampArray helper module (single-zone RGB)
+ *
+ * Helper module for HID drivers supporting devices that expose a Lighting and
+ * Illumination (LampArray) application collection (usage page 0x59).
+ *
+ * The module provides a minimal integration with the LED subsystem and treats
+ * the device as a single zone: all lamps share one RGB value and a global
+ * brightness level. It does not implement multi-zone layouts or hardware
+ * effects.
+ *
+ * If enabled and a device supporting LampArray is found, one multicolor LED
+ * class device is registered under /sys/class/leds/rgb:<function> to expose the
+ * single-zone RGB control.
+ *
+ * The use_leds_uapi sysfs attribute is attached directly to the HID device
+ * under /sys/bus/hid/devices/<HID-ID>/use_leds_uapi. Writing 0 to use_leds_uapi
+ * unregisters the LED class device. The last state is kept cached. Writing 1
+ * registers it again and restores the cached state to hardware. State is cached
+ * as last known RGB + brightness.
+ *
+ * The module does not bind to devices on its own. Instead, a HID driver may
+ * query support via lamparray_is_supported_device() after hid_parse() and
+ * create an instance using lamparray_register().
+ *
+ * Copyright (C) 2026 Tim Guttzeit <tgu@tuxedocomputers.com>
+ * Copyright (C) 2026 Aaron Erhardt <aer@tuxedocomputers.com>
+ */
+
+#include <linux/device.h>
+#include <linux/limits.h>
+#include <linux/minmax.h>
+#include <linux/hid.h>
+#include <linux/leds.h>
+#include <linux/sysfs.h>
+#include <linux/hid-lamparray.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/container_of.h>
+#include <linux/led-class-multicolor.h>
+#include <linux/xarray.h>
+
+/* Constants */
+
+/* HID usages (LampArray, etc.) */
+#define HID_LIGHTING_ILLUMINATION_USAGE_PAGE	0x0059
+
+/* HID usage types */
+#define HID_APPLICATION_COLLECTION_USAGE_TYPE	0x0001
+#define HID_LAMPARRAY_ATTRIBUTES_REPORT		0x0002
+#define HID_LAMP_ATTRIBUTES_RESPONSE_REPORT	0x0022
+#define HID_LAMP_RANGE_UPDATE_REPORT		0x0060
+#define HID_LAMPARRAY_CONTROL_REPORT		0x0070
+
+/* HID attributes */
+#define HID_LAIP_LAMP_COUNT			0x0003
+#define HID_LAIP_LAMPARRAY_KIND			0x0007
+#define HID_LAIP_RED_LEVEL_COUNT		0x0028
+#define HID_LAIP_GREEN_LEVEL_COUNT		0x0029
+#define HID_LAIP_BLUE_LEVEL_COUNT		0x002a
+#define HID_LAIP_INTENSITY_LEVEL_COUNT		0x002b
+#define HID_LAIP_RED_UPDATE_CHANNEL		0x0051
+#define HID_LAIP_GREEN_UPDATE_CHANNEL		0x0052
+#define HID_LAIP_BLUE_UPDATE_CHANNEL		0x0053
+#define HID_LAIP_INTENSITY_UPDATE_CHANNEL	0x0054
+#define HID_LAIP_LAMP_ID_START			0x0061
+#define HID_LAIP_LAMP_ID_END			0x0062
+#define HID_LAIP_AUTONOMOUS_MODE		0x0071
+
+/* LampArrayKind values */
+#define HID_LAMPARRAY_KIND_KEYBOARD		0x0001
+
+/* Helper struct for fields and their indices */
+struct hid_field_value {
+	struct hid_field *field;
+	int index;
+};
+
+/* Helper struct for color fields */
+struct lamparray_color_fields {
+	struct hid_field_value red;
+	struct hid_field_value green;
+	struct hid_field_value blue;
+	struct hid_field_value intensity;
+};
+
+/* Device state */
+struct lamparray_device {
+	struct hid_device *hdev;
+
+	struct lamparray_color_fields color_levels;
+	struct lamparray_color_fields color_update;
+
+	struct hid_field_value autonomous_field;
+	struct hid_field_value range_start;
+	struct hid_field_value range_end;
+	struct hid_field_value lamp_count;
+	struct hid_field_value lamparray_kind;
+
+	u16 lamp_count_value;
+	u32 lamparray_kind_value;
+
+	struct led_classdev_mc mc_cdev;
+	struct mc_subled subleds[3];
+
+	struct mutex dev_lock; /* Protects cached state and HID access */
+
+	u8 max_r;
+	u8 max_g;
+	u8 max_b;
+	u8 max_brightness;
+
+	u8 last_r;
+	u8 last_g;
+	u8 last_b;
+	u8 last_brightness;
+
+	struct mutex sysfs_lock; /* Protects sysfs LED (de-)initialization */
+
+	bool use_leds_uapi;
+	bool led_registered;
+};
+
+/*
+ * Opaque handle exposed to callers via the header.
+ * Keep the actual state in lamparray_device, but return a stable pointer.
+ */
+struct lamparray {
+	struct lamparray_device ldev;
+};
+
+/*
+ * Mapping for hid_device pointers to their lamparray data.
+ * Since there is not guarantee of how the driver using this library
+ * will use its drvdata, the only safe way to retrieve the lamparray
+ * data from a HID device pointer is using this mapping.
+ */
+static DEFINE_XARRAY(lamparray_by_hdev);
+
+/* HID helper functions */
+
+static int get_field_value(struct hid_field_value *field_value)
+{
+	return field_value->field->value[field_value->index];
+}
+
+static u8 get_field_value_as_u8(struct hid_field_value *field_value)
+{
+	return clamp_val(get_field_value(field_value), 0, U8_MAX);
+}
+
+static void set_field_value(struct hid_field_value *field_value, int value)
+{
+	field_value->field->value[field_value->index] = value;
+}
+
+static bool lamparray_color_fields_is_complete(struct lamparray_color_fields *color_fields)
+{
+	return color_fields->red.field && color_fields->green.field &&
+	       color_fields->blue.field && color_fields->intensity.field;
+}
+
+static int lamparray_read_attributes_report(struct lamparray_device *ldev)
+{
+	struct hid_device *hdev = ldev->hdev;
+	struct hid_report *report;
+	int ret;
+	size_t len;
+
+	if (!ldev->lamp_count.field) {
+		hid_warn(hdev, "No LampCount field found\n");
+		return -ENODEV;
+	}
+
+	if (!ldev->lamparray_kind.field) {
+		hid_warn(hdev, "No LampArrayKind field found\n");
+		return -ENODEV;
+	}
+
+	report = ldev->lamp_count.field->report;
+
+	if (!report) {
+		hid_warn(hdev, "LampCount field has no report\n");
+		return -ENODEV;
+	}
+
+	len = hid_report_len(report);
+	u8 *buf __free(kfree) = kmalloc(len, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	guard(mutex)(&ldev->dev_lock);
+
+	/* Update values */
+	ret = hid_hw_raw_request(hdev, report->id, buf, len,
+				 HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
+	if (ret < 0) {
+		hid_warn(hdev, "Failed to request LampArrayAttributesReport: %d\n", ret);
+		return ret;
+	}
+
+	ret = hid_report_raw_event(hdev, HID_FEATURE_REPORT, buf, len, ret, 0);
+	if (ret < 0) {
+		hid_warn(hdev, "Failed to parse LampArrayAttributesReport: %d\n", ret);
+		return ret;
+	}
+
+	ldev->lamp_count_value = get_field_value(&ldev->lamp_count);
+	if (!ldev->lamp_count_value) {
+		hid_warn(hdev, "LampCount is 0 (invalid)\n");
+		return -EINVAL;
+	}
+
+	ldev->lamparray_kind_value = get_field_value(&ldev->lamparray_kind);
+	return 0;
+}
+
+static int lamparray_parse_update_report(struct lamparray_device *ldev)
+{
+	struct hid_device *hdev = ldev->hdev;
+	struct hid_report_enum *re;
+	struct hid_report *report;
+	struct hid_field *field;
+	int i, j;
+	int ret = 0;
+
+	re = &hdev->report_enum[HID_FEATURE_REPORT];
+
+	list_for_each_entry(report, &re->report_list, list) {
+		for (i = 0; i < report->maxfield; i++) {
+			field = report->field[i];
+			if (!field)
+				continue;
+
+			if (!field->usage || !field->maxusage)
+				continue;
+
+			for (j = 0; j < field->maxusage; j++) {
+				u32 usage = field->usage[j].hid;
+				u32 collection_idx = field->usage[j].collection_index;
+				u32 collection_usage = hdev->collection[collection_idx].usage;
+
+				u16 page = (usage & HID_USAGE_PAGE) >> 16;
+				u16 id = usage & HID_USAGE;
+				u16 collection_usage_id = collection_usage & U16_MAX;
+
+				if (page != HID_LIGHTING_ILLUMINATION_USAGE_PAGE)
+					continue;
+
+				if (collection_usage_id == HID_LAMPARRAY_ATTRIBUTES_REPORT) {
+					switch (id) {
+					case HID_LAIP_LAMP_COUNT:
+						ldev->lamp_count.field = field;
+						ldev->lamp_count.index = j;
+						break;
+					case HID_LAIP_LAMPARRAY_KIND:
+						ldev->lamparray_kind.field = field;
+						ldev->lamparray_kind.index = j;
+						break;
+					}
+				} else if (collection_usage_id ==
+					   HID_LAMP_ATTRIBUTES_RESPONSE_REPORT) {
+					switch (id) {
+					case HID_LAIP_RED_LEVEL_COUNT:
+						ldev->color_levels.red.field = field;
+						ldev->color_levels.red.index = j;
+						break;
+					case HID_LAIP_GREEN_LEVEL_COUNT:
+						ldev->color_levels.green.field = field;
+						ldev->color_levels.green.index = j;
+						break;
+					case HID_LAIP_BLUE_LEVEL_COUNT:
+						ldev->color_levels.blue.field = field;
+						ldev->color_levels.blue.index = j;
+						break;
+					case HID_LAIP_INTENSITY_LEVEL_COUNT:
+						ldev->color_levels.intensity.field = field;
+						ldev->color_levels.intensity.index = j;
+						break;
+					}
+				} else if (collection_usage_id == HID_LAMP_RANGE_UPDATE_REPORT) {
+					switch (id) {
+					case HID_LAIP_RED_UPDATE_CHANNEL:
+						ldev->color_update.red.field = field;
+						ldev->color_update.red.index = j;
+						break;
+					case HID_LAIP_GREEN_UPDATE_CHANNEL:
+						ldev->color_update.green.field = field;
+						ldev->color_update.green.index = j;
+						break;
+					case HID_LAIP_BLUE_UPDATE_CHANNEL:
+						ldev->color_update.blue.field = field;
+						ldev->color_update.blue.index = j;
+						break;
+					case HID_LAIP_INTENSITY_UPDATE_CHANNEL:
+						ldev->color_update.intensity.field = field;
+						ldev->color_update.intensity.index = j;
+						break;
+					case HID_LAIP_LAMP_ID_START:
+						ldev->range_start.field = field;
+						ldev->range_start.index = j;
+						break;
+					case HID_LAIP_LAMP_ID_END:
+						ldev->range_end.field = field;
+						ldev->range_end.index = j;
+						break;
+					default:
+						break;
+					}
+				} else if (collection_usage_id == HID_LAMPARRAY_CONTROL_REPORT &&
+					   id == HID_LAIP_AUTONOMOUS_MODE) {
+					ldev->autonomous_field.field = field;
+					ldev->autonomous_field.index = j;
+				}
+			}
+		}
+	}
+
+	if (!ldev->autonomous_field.field ||
+	    !lamparray_color_fields_is_complete(&ldev->color_update))
+		ret = -ENODEV;
+
+	return ret;
+}
+
+static int lamparray_hw_set_autonomous(struct lamparray_device *ldev,
+				       bool enable)
+{
+	struct hid_device *hdev = ldev->hdev;
+	struct hid_field *field = ldev->autonomous_field.field;
+
+	if (!field)
+		return -ENODEV;
+
+	guard(mutex)(&ldev->dev_lock);
+	set_field_value(&ldev->autonomous_field, !!enable);
+	hid_hw_request(hdev, field->report, HID_REQ_SET_REPORT);
+
+	return 0;
+}
+
+/* Make sure to hold dev lock to prevent concurrent access. */
+static int lamparray_hw_set_state(struct lamparray_device *ldev, u8 r, u8 g,
+				  u8 b, u8 intensity)
+{
+	struct hid_device *hdev = ldev->hdev;
+	struct hid_report *report;
+
+	if (!lamparray_color_fields_is_complete(&ldev->color_update))
+		return -ENODEV;
+
+	if (ldev->range_start.field && ldev->range_end.field) {
+		set_field_value(&ldev->range_start, 0);
+		set_field_value(&ldev->range_end, ldev->lamp_count_value - 1);
+	}
+
+	set_field_value(&ldev->color_update.red, r);
+	set_field_value(&ldev->color_update.green, g);
+	set_field_value(&ldev->color_update.blue, b);
+	set_field_value(&ldev->color_update.intensity, intensity);
+
+	report = ldev->color_update.red.field->report;
+	hid_hw_request(hdev, report, HID_REQ_SET_REPORT);
+
+	return 0;
+}
+
+/*
+ * Simple helper to read the color information of the first lamp.
+ * This does not read the state of the whole lamp array since this driver only
+ * exposes one LED anyway, so one color is sufficient here for now.
+ */
+static int lamparray_get_lamp_attributes(struct lamparray_device *ldev)
+{
+	struct hid_device *hdev = ldev->hdev;
+	struct hid_report *report;
+	int ret;
+	size_t len;
+
+	if (!lamparray_color_fields_is_complete(&ldev->color_levels))
+		return -ENODEV;
+
+	/*
+	 * All four fields must share the same report since the
+	 * attributes are fetched with a single GET_REPORT below.
+	 */
+	report = ldev->color_levels.red.field->report;
+	if (!report ||
+	    ldev->color_levels.green.field->report != report ||
+	    ldev->color_levels.blue.field->report != report ||
+	    ldev->color_levels.intensity.field->report != report)
+		return -ENODEV;
+
+	len = hid_report_len(report);
+	u8 *buf __free(kfree) = kmalloc(len, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	guard(mutex)(&ldev->dev_lock);
+
+	/* Update values */
+	ret = hid_hw_raw_request(hdev, report->id, buf, len,
+				 HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
+	if (ret < 0) {
+		hid_warn(hdev, "Failed to request LampAttributesResponseReport: %d\n", ret);
+		return ret;
+	}
+
+	ret = hid_report_raw_event(hdev, HID_FEATURE_REPORT, buf, len, ret, 0);
+	if (ret < 0) {
+		hid_warn(hdev, "Failed to parse LampAttributesResponseReport: %d\n", ret);
+		return ret;
+	}
+
+	ldev->max_r = get_field_value_as_u8(&ldev->color_levels.red);
+	ldev->max_g = get_field_value_as_u8(&ldev->color_levels.green);
+	ldev->max_b = get_field_value_as_u8(&ldev->color_levels.blue);
+	ldev->max_brightness = get_field_value_as_u8(&ldev->color_levels.intensity);
+
+	if (!ldev->max_r || !ldev->max_g || !ldev->max_b || !ldev->max_brightness) {
+		hid_warn(hdev, "LampArray device has no color levels\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+/* Helper functions */
+
+static int lamparray_restore_state(struct lamparray_device *ldev)
+{
+	u8 r, g, b;
+	int ret;
+	enum led_brightness brightness;
+
+	if (!ldev->use_leds_uapi)
+		return 0;
+
+	guard(mutex)(&ldev->dev_lock);
+
+	r = ldev->last_r;
+	g = ldev->last_g;
+	b = ldev->last_b;
+	brightness = ldev->last_brightness;
+
+	ldev->mc_cdev.subled_info[0].intensity = r;
+	ldev->mc_cdev.subled_info[1].intensity = g;
+	ldev->mc_cdev.subled_info[2].intensity = b;
+	ldev->mc_cdev.led_cdev.brightness = brightness;
+
+	/*
+	 * Quirk for devices ignoring intensity values, see
+	 * lamparray_led_brightness_set for more information.
+	 */
+	if (brightness)
+		ret = lamparray_hw_set_state(ldev, r, g, b, brightness);
+	else
+		ret = lamparray_hw_set_state(ldev, 0, 0, 0, 0);
+
+	return ret;
+}
+
+/* LEDs API */
+
+static int lamparray_led_brightness_set(struct led_classdev *cdev,
+					enum led_brightness brightness)
+{
+	struct led_classdev_mc *mc = lcdev_to_mccdev(cdev);
+	struct lamparray_device *ldev =
+		container_of_const(mc, struct lamparray_device, mc_cdev);
+	u8 r, g, b;
+	int ret;
+
+	/*
+	 * Brightness is handled by the LampArray device if supported,
+	 * so we can pass the raw intensity values.
+	 */
+	r = mc->subled_info[0].intensity;
+	g = mc->subled_info[1].intensity;
+	b = mc->subled_info[2].intensity;
+	mc->led_cdev.brightness = brightness;
+
+	guard(mutex)(&ldev->dev_lock);
+
+	/*
+	 * Some devices completely ignore the intensity (brightness)
+	 * value and instead rely on zeros for each RGB channel to turn
+	 * the LEDs off (e.g. the ASUS Prime B650-Plus). Since it doesn't
+	 * hurt spec-compliant devices to quirk this behavior, we turn
+	 * off the RGB channels unconditionally when the brightness is
+	 * zero.
+	 */
+	if (brightness)
+		ret = lamparray_hw_set_state(ldev, r, g, b, brightness);
+	else
+		ret = lamparray_hw_set_state(ldev, 0, 0, 0, 0);
+
+	if (ret) {
+		hid_err(ldev->hdev, "Failed to send LampArray update: %d\n",
+			ret);
+		return ret;
+	}
+
+	ldev->last_r = r;
+	ldev->last_g = g;
+	ldev->last_b = b;
+	ldev->last_brightness = brightness;
+
+	return 0;
+}
+
+static enum led_brightness
+lamparray_led_brightness_get(struct led_classdev *cdev)
+{
+	struct led_classdev_mc *mc = lcdev_to_mccdev(cdev);
+	struct lamparray_device *ldev =
+	    container_of_const(mc, struct lamparray_device, mc_cdev);
+
+	return ldev->last_brightness;
+}
+
+/* Make sure to hold sysfs lock to prevent concurrent access. */
+static int lamparray_register_led(struct lamparray_device *ldev)
+{
+	struct device *dev = &ldev->hdev->dev;
+	struct led_classdev *cdev = &ldev->mc_cdev.led_cdev;
+	int ret;
+
+	if (ldev->led_registered)
+		return 0;
+
+	if (!cdev->name) {
+		/* Fallback value */
+		const char *function = LED_FUNCTION_STATUS;
+
+		/* Some heuristics for choosing a better LED function. */
+		if (ldev->lamparray_kind_value == HID_LAMPARRAY_KIND_KEYBOARD)
+			function = LED_FUNCTION_KBD_BACKLIGHT;
+
+		cdev->name = kasprintf(GFP_KERNEL, "rgb:%s", function);
+		if (!cdev->name)
+			return -ENOMEM;
+	}
+
+	mutex_lock(&ldev->dev_lock);
+	/* Setup */
+	cdev->max_brightness = ldev->max_brightness;
+	cdev->brightness_set_blocking = lamparray_led_brightness_set;
+	cdev->brightness_get = lamparray_led_brightness_get;
+	/* Don't do anything automatically on shutdown or suspend. */
+	cdev->flags |= LED_RETAIN_AT_SHUTDOWN;
+
+	ldev->subleds[0].color_index = LED_COLOR_ID_RED;
+	ldev->subleds[0].max_intensity = ldev->max_r;
+	ldev->subleds[1].color_index = LED_COLOR_ID_GREEN;
+	ldev->subleds[1].max_intensity = ldev->max_g;
+	ldev->subleds[2].color_index = LED_COLOR_ID_BLUE;
+	ldev->subleds[2].max_intensity = ldev->max_b;
+
+	/* Set values */
+	ldev->subleds[0].intensity = ldev->last_r;
+	ldev->subleds[1].intensity = ldev->last_g;
+	ldev->subleds[2].intensity = ldev->last_b;
+	cdev->brightness = ldev->last_brightness;
+
+	ldev->mc_cdev.subled_info = ldev->subleds;
+	ldev->mc_cdev.num_colors = ARRAY_SIZE(ldev->subleds);
+
+	mutex_unlock(&ldev->dev_lock);
+
+	ret = led_classdev_multicolor_register(dev, &ldev->mc_cdev);
+	if (ret) {
+		hid_warn(ldev->hdev, "Failed to register LED classdev for lamparray\n");
+		kfree(cdev->name);
+		cdev->name = NULL;
+		return ret;
+	}
+
+	ldev->led_registered = true;
+
+	return 0;
+}
+
+/* Make sure to hold sysfs lock to prevent concurrent access. */
+static void lamparray_unregister_led(struct lamparray_device *ldev)
+{
+	struct led_classdev *cdev = &ldev->mc_cdev.led_cdev;
+
+	if (ldev->led_registered)
+		led_classdev_multicolor_unregister(&ldev->mc_cdev);
+
+	ldev->led_registered = false;
+	kfree(cdev->name);
+	cdev->name = NULL;
+}
+
+/* Sysfs */
+
+static struct lamparray_device *
+lamparray_ldev_from_sysfs_dev(struct device *dev)
+{
+	struct hid_device *hdev = to_hid_device(dev);
+
+	return xa_load(&lamparray_by_hdev, (unsigned long)hdev);
+}
+
+static ssize_t use_leds_uapi_show(struct device *dev,
+				  struct device_attribute *attr, char *buf)
+{
+	struct lamparray_device *ldev = lamparray_ldev_from_sysfs_dev(dev);
+
+	if (!ldev)
+		return -ENODEV;
+
+	return sysfs_emit(buf, "%d\n", ldev->use_leds_uapi);
+}
+
+static ssize_t use_leds_uapi_store(struct device *dev,
+				   struct device_attribute *attr,
+				   const char *buf, size_t count)
+{
+	struct lamparray_device *ldev = lamparray_ldev_from_sysfs_dev(dev);
+	bool val, old_val;
+	int ret;
+
+	if (!ldev)
+		return -ENODEV;
+
+	ret = kstrtobool(buf, &val);
+	if (ret)
+		return ret;
+
+	/* Make sure only one concurrent write is possible */
+	guard(mutex)(&ldev->sysfs_lock);
+	old_val = ldev->use_leds_uapi;
+
+	if (val == old_val)
+		return count;
+
+	ldev->use_leds_uapi = val;
+
+	/*
+	 * Take the device out of autonomous mode before exposing the LED
+	 * class device, and hand it back afterwards, so that control is
+	 * transferred in both directions rather than only at probe.
+	 */
+	if (val == 1) {
+		ret = lamparray_hw_set_autonomous(ldev, false);
+		if (ret)
+			goto err_revert;
+		ret = lamparray_register_led(ldev);
+		if (ret) {
+			lamparray_hw_set_autonomous(ldev, true);
+			goto err_revert;
+		}
+		ret = lamparray_restore_state(ldev);
+		if (ret) {
+			hid_warn(ldev->hdev, "Could not restore state: %d\n", ret);
+			lamparray_unregister_led(ldev);
+			lamparray_hw_set_autonomous(ldev, true);
+			goto err_revert;
+		}
+	} else {
+		lamparray_unregister_led(ldev);
+		ret = lamparray_hw_set_autonomous(ldev, true);
+		if (ret) {
+			hid_warn(ldev->hdev, "Could not enable autonomous mode: %d\n", ret);
+			return count;
+		}
+	}
+
+	return count;
+
+err_revert:
+	ldev->use_leds_uapi = old_val;
+	return ret;
+}
+static DEVICE_ATTR_RW(use_leds_uapi);
+
+static int lamparray_register_sysfs(struct lamparray_device *ldev)
+{
+	struct device *dev = &ldev->hdev->dev;
+	int ret;
+
+	ret = device_create_file(dev, &dev_attr_use_leds_uapi);
+	if (ret)
+		hid_err(ldev->hdev,
+			"Failed to create lamparray sysfs group: %d\n", ret);
+
+	return ret;
+}
+
+static void lamparray_remove_sysfs(struct lamparray_device *ldev)
+{
+	device_remove_file(&ldev->hdev->dev, &dev_attr_use_leds_uapi);
+}
+
+/* Public API */
+
+bool lamparray_is_supported_device(struct hid_device *hdev)
+{
+	unsigned int i;
+
+	hid_dbg(hdev, "lamparray: walking %u collections\n",
+		hdev->maxcollection);
+
+	for (i = 0; i < hdev->maxcollection; i++) {
+		struct hid_collection *col = &hdev->collection[i];
+		u16 page = (col->usage & HID_USAGE_PAGE) >> 16;
+		u16 code = col->usage & HID_USAGE;
+
+		hid_dbg(hdev,
+			"lamparray:  collection[%u]: type=%u level=%u usage=0x%08x page=0x%04x code=0x%04x\n",
+			i, col->type, col->level, col->usage, page, code);
+
+		if (col->type == HID_COLLECTION_APPLICATION &&
+		    page == HID_LIGHTING_ILLUMINATION_USAGE_PAGE &&
+		    code == HID_APPLICATION_COLLECTION_USAGE_TYPE) {
+			return true;
+		}
+	}
+	return false;
+}
+EXPORT_SYMBOL_GPL(lamparray_is_supported_device);
+
+struct lamparray *
+lamparray_register(struct hid_device *hdev,
+		   const struct lamparray_init_state *led_init_state)
+{
+	int ret;
+	struct lamparray_device *ldev;
+
+	if (!hdev)
+		return ERR_PTR(-ENODEV);
+
+	struct lamparray *la __free(kfree) = kzalloc_obj(*la, GFP_KERNEL);
+	if (!la)
+		return ERR_PTR(-ENOMEM);
+
+	ldev = &la->ldev;
+
+	mutex_init(&ldev->dev_lock);
+	mutex_init(&ldev->sysfs_lock);
+	ldev->hdev = hdev;
+	ldev->use_leds_uapi = true;
+	ldev->led_registered = false;
+
+	ret = lamparray_parse_update_report(ldev);
+	if (ret) {
+		hid_err(hdev, "No LampArray update report found: %d\n", ret);
+		goto err_free;
+	}
+
+	ret = lamparray_read_attributes_report(ldev);
+	if (ret) {
+		hid_err(hdev,
+			"Could not determine LampCount: %d\n",
+			ret);
+		goto err_free;
+	}
+
+	ret = lamparray_get_lamp_attributes(ldev);
+	if (ret) {
+		hid_err(hdev,
+			"Faulty device. Could not query lamp attributes.\n");
+		goto err_free;
+	}
+
+	/* Use black (white with zero brightness) as default. */
+	if (led_init_state) {
+		ldev->last_r = min(led_init_state->r, ldev->max_r);
+		ldev->last_g = min(led_init_state->g, ldev->max_g);
+		ldev->last_b = min(led_init_state->b, ldev->max_b);
+		ldev->last_brightness = min(led_init_state->brightness,
+					    ldev->max_brightness);
+	} else {
+		ldev->last_r = ldev->max_r;
+		ldev->last_g = ldev->max_g;
+		ldev->last_b = ldev->max_b;
+		ldev->last_brightness = 0;
+	}
+
+	/* No locking needed, no races are possible before sysfs setup. */
+	ret = lamparray_register_led(ldev);
+	if (ret) {
+		hid_warn(hdev, "Failed to register LED UAPI: %d\n", ret);
+		ldev->use_leds_uapi = false;
+	}
+
+	ret = xa_err(xa_store(&lamparray_by_hdev, (unsigned long)hdev, ldev,
+			      GFP_KERNEL));
+	if (ret)
+		goto err_unregister_led;
+
+	ret = lamparray_register_sysfs(ldev);
+	if (ret)
+		goto err_xa_erase;
+
+	ret = lamparray_hw_set_autonomous(ldev, false);
+	if (ret) {
+		hid_err(hdev, "Could not disable autonomous mode: %d", ret);
+		goto err_remove_sysfs;
+	}
+
+	hid_info(hdev, "LampArray device registered\n");
+
+	ret = lamparray_restore_state(ldev);
+	if (ret) {
+		hid_err(hdev, "Failed to set default state: %d", ret);
+		goto err_remove_sysfs;
+	}
+
+	return_ptr(la);
+
+err_remove_sysfs:
+	lamparray_remove_sysfs(ldev);
+err_xa_erase:
+	xa_erase(&lamparray_by_hdev, (unsigned long)hdev);
+err_unregister_led:
+	mutex_lock(&ldev->sysfs_lock);
+	lamparray_unregister_led(ldev);
+	mutex_unlock(&ldev->sysfs_lock);
+err_free:
+	mutex_destroy(&ldev->dev_lock);
+	mutex_destroy(&ldev->sysfs_lock);
+	return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(lamparray_register);
+
+void lamparray_unregister(struct lamparray *la)
+{
+	struct lamparray_device *ldev;
+
+	if (!la)
+		return;
+
+	ldev = &la->ldev;
+
+	lamparray_hw_set_autonomous(ldev, true);
+
+	lamparray_remove_sysfs(ldev);
+	xa_erase(&lamparray_by_hdev, (unsigned long)ldev->hdev);
+
+	mutex_lock(&ldev->sysfs_lock);
+	lamparray_unregister_led(ldev);
+	mutex_unlock(&ldev->sysfs_lock);
+
+	mutex_destroy(&ldev->dev_lock);
+	mutex_destroy(&ldev->sysfs_lock);
+	kfree(la);
+}
+EXPORT_SYMBOL_GPL(lamparray_unregister);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Tim Guttzeit <tgu@tuxedocomputers.com>");
+MODULE_AUTHOR("Aaron Erhardt <aer@tuxedocomputers.com>");
+MODULE_DESCRIPTION("HID LampArray helper module (single-zone RGB)");
diff --git a/include/linux/hid-lamparray.h b/include/linux/hid-lamparray.h
new file mode 100644
index 0000000000000..6124878859392
--- /dev/null
+++ b/include/linux/hid-lamparray.h
@@ -0,0 +1,89 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#ifndef _HID_LAMPARRAY_H
+#define _HID_LAMPARRAY_H
+
+#include <linux/hid.h>
+#include <linux/err.h>
+#include <linux/types.h>
+
+struct lamparray;
+
+/*
+ * Optional initial LED state for lamparray_register().
+ * Used to define the initial state of a LampArray's LEDs.
+ */
+struct lamparray_init_state {
+	u8 r;
+	u8 g;
+	u8 b;
+	u8 brightness;
+};
+
+#if IS_ENABLED(CONFIG_HID_LAMPARRAY)
+
+/**
+ * lamparray_is_supported_device() - check whether a HID device supports LampArray
+ * @hdev: HID device to inspect
+ *
+ * Check whether the given HID device exposes a Lighting/LampArray application
+ * collection as defined by the HID Lighting specification.
+ *
+ * This helper can be used by HID drivers to determine whether LampArray
+ * functionality should be enabled for a device.
+ *
+ * Return: %true if LampArray support is detected, %false otherwise.
+ */
+bool lamparray_is_supported_device(struct hid_device *hdev);
+
+/**
+ * lamparray_register() - initialize LampArray support for a HID device
+ * @hdev: HID device
+ * @led_init_state: Optional LED state at init specification
+ *
+ * Allocate and initialize internal LampArray state for the given HID device.
+ * The function parses required HID reports and fields and registers the
+ * associated miscdevice and sysfs attributes.
+ *
+ * Registers a multicolor LED class device to expose the LampArray functionality
+ * via the LED subsystem. If specified, the desired initial LED state is
+ * applied. If led_init_state is NULL, a default state is applied (white with
+ * zero brightness).
+ *
+ * Return: pointer to a LampArray handle on success, or ERR_PTR() on failure.
+ */
+struct lamparray *lamparray_register(struct hid_device *hdev,
+				     const struct lamparray_init_state *led_init_state);
+
+/**
+ * lamparray_unregister() - tear down LampArray support
+ * @la: LampArray handle returned by lamparray_register()
+ *
+ * Remove all resources associated with a LampArray instance.
+ *
+ * This unregisters the LED class device (if present), removes the miscdevice
+ * and sysfs interfaces and frees all internal state associated with @la.
+ */
+void lamparray_unregister(struct lamparray *la);
+
+#else /* !CONFIG_HID_LAMPARRAY */
+
+static inline bool lamparray_is_supported_device(struct hid_device *hdev)
+{
+	return false;
+}
+
+static inline struct lamparray *
+lamparray_register(struct hid_device *hdev,
+		   const struct lamparray_init_state *led_init_state)
+{
+	return ERR_PTR(-EOPNOTSUPP);
+}
+
+static inline void lamparray_unregister(struct lamparray *la)
+{
+}
+
+#endif /* CONFIG_HID_LAMPARRAY */
+
+#endif /* _HID_LAMPARRAY_H */

-- 
2.43.0

Re: [PATCH v7 1/3] HID: lamparray: add new LampArray helper module

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [High] HID: lamparray: broken feature report parsing with hid_report_raw_event()
- [High] HID: lamparray: missing LampUpdateComplete flag in lamparray_hw_set_state()
- [Medium] HID: lamparray: LED_UNREGISTERING flag left set in lamparray_register_led()
- [Medium] HID: lamparray: brightness and intensity races in LED callbacks and restore
- [Medium] HID: lamparray: premature sysfs/LED exposure in lamparray_register()
- [Medium] HID: lamparray: inverted teardown ordering in lamparray_unregister()
- [Medium] HID: lamparray: mixing __free() and guard() cleanup with goto labels
- [Medium] HID: lamparray: missing LampId request in lamparray_get_lamp_attributes()
- [Low] HID: lamparray: stale miscdevice references in hid-lamparray.h kernel-doc
--

commit 5808d3d964362ce7a1a1b72a82eceb2d7c489b7a
Author: Aaron Erhardt <aer@tuxedocomputers.com>

HID: lamparray: add new LampArray helper module

This commit adds a hid-lamparray helper module that provides basic single-zone
RGB LED support for HID devices exposing a Lighting/LampArray application
collection (usage page 0x59).

> diff --git a/drivers/hid/hid-lamparray.c b/drivers/hid/hid-lamparray.c
> new file mode 100644
> index 0000000000000..fb3082f6d19a0
> --- /dev/null
> +++ b/drivers/hid/hid-lamparray.c
> @@ -0,0 +1,858 @@

[ ... ]

> +static int lamparray_read_attributes_report(struct lamparray_device *ldev)
> +{
> +	struct hid_device *hdev = ldev->hdev;
> +	struct hid_report *report;
> +	int ret;
> +	size_t len;
> +
> +	if (!ldev->lamp_count.field) {
> +		hid_warn(hdev, "No LampCount field found\n");
> +		return -ENODEV;
> +	}
> +
> +	if (!ldev->lamparray_kind.field) {
> +		hid_warn(hdev, "No LampArrayKind field found\n");
> +		return -ENODEV;
> +	}
> +
> +	report = ldev->lamp_count.field->report;
> +
> +	if (!report) {
> +		hid_warn(hdev, "LampCount field has no report\n");
> +		return -ENODEV;
> +	}
> +
> +	len = hid_report_len(report);
> +	u8 *buf __free(kfree) = kmalloc(len, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	guard(mutex)(&ldev->dev_lock);
> +
> +	/* Update values */
> +	ret = hid_hw_raw_request(hdev, report->id, buf, len,
> +				 HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
> +	if (ret < 0) {
> +		hid_warn(hdev, "Failed to request LampArrayAttributesReport: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = hid_report_raw_event(hdev, HID_FEATURE_REPORT, buf, len, ret, 0);

[Severity: High]
Can this sequence in lamparray_read_attributes_report() and
lamparray_get_lamp_attributes() misparse feature reports or leak uninitialized
heap bytes?

First, buf is allocated with kmalloc(len, GFP_KERNEL) without zeroing it or
setting buf[0] = report->id, and buf[0] is not validated after
hid_hw_raw_request(). If a low-level transport driver does not write buf[0] on
HID_REQ_GET_REPORT (for example, handle_lamp_array_attributes_report() and
handle_lamp_attributes_response_report() in
drivers/platform/x86/tuxedo/nb04/wmi_ab.c where report_id is const u8, or
ishtp_raw_request() on timeout), buf[0] retains uninitialized heap memory.
When hid_report_raw_event() runs:

drivers/hid/hid-core.c:hid_report_raw_event() {
	report = hid_get_report(report_enum, data);
	...
	if (hid->claimed & HID_CLAIMED_HIDRAW) {
		ret = hidraw_report_event(hid, data, size);
		if (ret)
			return ret;
	}
}

hid_get_report() reads the uninitialized buf[0] to index
report_enum->report_id_hash[n] (failing to match the report or updating a
different report), and hidraw_report_event() copies the uninitialized buffer
and synchronous GET_REPORT reply to userspace hidraw clients.

Second, if the feature report is unnumbered (report->id == 0),
hid_report_len(report) returns DIV_ROUND_UP(report->size, 8) without a report
ID byte. However, low-level raw_request implementations such as
usbhid_get_raw_report() and i2c_hid_get_raw_report() store 0 into buf[0],
advance buf++, and decrement count-- when reportnum == 0. Without allocating an
extra byte and stripping buf[0] before parsing (as __hid_request() in
drivers/hid/hid-core.c and vivaldi_feature_mapping() in
drivers/hid/hid-vivaldi-common.c do), the transfer buffer is 1 byte too short
(-EOVERFLOW or truncation) and hid_report_raw_event() parses the stuffed 0x00
byte at buf[0] as the first byte of the payload.

Third, hid_report_raw_event() only calls hid_process_report() when
hid->claimed != HID_CLAIMED_HIDRAW:

drivers/hid/hid-core.c:hid_report_raw_event() {
	if (hid->claimed != HID_CLAIMED_HIDRAW && report->maxfield) {
		hid_process_report(hid, report, cdata, interrupt);
		...
	}
}

Because the LampArray application usage (0x00590001) is not an input
application, a standalone LampArray device started via
hid_hw_start(hdev, HID_CONNECT_DEFAULT) without HID_CONNECT_DRIVER on non-USB
buses (or with CONFIG_USB_HIDDEV=n) has hdev->claimed == HID_CLAIMED_HIDRAW.
In that case hid_report_raw_event() skips hid_process_report(), leaving
ldev->lamp_count at 0 and failing with -EINVAL.

Would using kzalloc() (or hid_alloc_report_buf()) and hid_extract_bits() (or
handling the report->id == 0 offset and HID_CLAIMED_DRIVER requirement) avoid
these issues?

> +	if (ret < 0) {
> +		hid_warn(hdev, "Failed to parse LampArrayAttributesReport: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ldev->lamp_count_value = get_field_value(&ldev->lamp_count);
> +	if (!ldev->lamp_count_value) {
> +		hid_warn(hdev, "LampCount is 0 (invalid)\n");
> +		return -EINVAL;
> +	}
> +
> +	ldev->lamparray_kind_value = get_field_value(&ldev->lamparray_kind);
> +	return 0;
> +}
> +
> +static int lamparray_parse_update_report(struct lamparray_device *ldev)
> +{

[ ... ]

> +				} else if (collection_usage_id == HID_LAMP_RANGE_UPDATE_REPORT) {
> +					switch (id) {
> +					case HID_LAIP_RED_UPDATE_CHANNEL:
> +						ldev->color_update.red.field = field;
> +						ldev->color_update.red.index = j;
> +						break;
> +					case HID_LAIP_GREEN_UPDATE_CHANNEL:
> +						ldev->color_update.green.field = field;
> +						ldev->color_update.green.index = j;
> +						break;
> +					case HID_LAIP_BLUE_UPDATE_CHANNEL:
> +						ldev->color_update.blue.field = field;
> +						ldev->color_update.blue.index = j;
> +						break;
> +					case HID_LAIP_INTENSITY_UPDATE_CHANNEL:
> +						ldev->color_update.intensity.field = field;
> +						ldev->color_update.intensity.index = j;
> +						break;
> +					case HID_LAIP_LAMP_ID_START:
> +						ldev->range_start.field = field;
> +						ldev->range_start.index = j;
> +						break;
> +					case HID_LAIP_LAMP_ID_END:
> +						ldev->range_end.field = field;
> +						ldev->range_end.index = j;
> +						break;
> +					default:
> +						break;
> +					}
> +				} else if (collection_usage_id == HID_LAMPARRAY_CONTROL_REPORT &&
> +					   id == HID_LAIP_AUTONOMOUS_MODE) {
> +					ldev->autonomous_field.field = field;
> +					ldev->autonomous_field.index = j;
> +				}
> +			}
> +		}
> +	}
> +
> +	if (!ldev->autonomous_field.field ||
> +	    !lamparray_color_fields_is_complete(&ldev->color_update))
> +		ret = -ENODEV;
> +
> +	return ret;
> +}
> +
> +static int lamparray_hw_set_autonomous(struct lamparray_device *ldev,
> +				       bool enable)
> +{
> +	struct hid_device *hdev = ldev->hdev;
> +	struct hid_field *field = ldev->autonomous_field.field;
> +
> +	if (!field)
> +		return -ENODEV;
> +
> +	guard(mutex)(&ldev->dev_lock);
> +	set_field_value(&ldev->autonomous_field, !!enable);
> +	hid_hw_request(hdev, field->report, HID_REQ_SET_REPORT);
> +
> +	return 0;
> +}
> +
> +/* Make sure to hold dev lock to prevent concurrent access. */
> +static int lamparray_hw_set_state(struct lamparray_device *ldev, u8 r, u8 g,
> +				  u8 b, u8 intensity)
> +{
> +	struct hid_device *hdev = ldev->hdev;
> +	struct hid_report *report;
> +
> +	if (!lamparray_color_fields_is_complete(&ldev->color_update))
> +		return -ENODEV;
> +
> +	if (ldev->range_start.field && ldev->range_end.field) {
> +		set_field_value(&ldev->range_start, 0);
> +		set_field_value(&ldev->range_end, ldev->lamp_count_value - 1);
> +	}
> +
> +	set_field_value(&ldev->color_update.red, r);
> +	set_field_value(&ldev->color_update.green, g);
> +	set_field_value(&ldev->color_update.blue, b);
> +	set_field_value(&ldev->color_update.intensity, intensity);
> +
> +	report = ldev->color_update.red.field->report;
> +	hid_hw_request(hdev, report, HID_REQ_SET_REPORT);

[Severity: High]
Should lamparray_parse_update_report() and lamparray_hw_set_state() also parse
and set LampUpdateFlags (usage 0x0055) in LampRangeUpdateReport (0x0060)?

Per the HID Lighting and Illumination specification, LampRangeUpdateReport
contains LampUpdateFlags where bit 0 (LampUpdateComplete = 1) instructs the
device to commit buffered updates to the physical LEDs (as seen with
LAMP_UPDATE_FLAGS_LAMP_UPDATE_COMPLETE in
drivers/platform/x86/tuxedo/nb04/wmi_ab.c). Without setting LampUpdateFlags to
LampUpdateComplete before calling hid_hw_request(), won't spec-compliant
devices that buffer updates until LampUpdateComplete is asserted never apply
the color or brightness changes?

> +
> +	return 0;
> +}
> +
> +/*
> + * Simple helper to read the color information of the first lamp.
> + * This does not read the state of the whole lamp array since this driver only
> + * exposes one LED anyway, so one color is sufficient here for now.
> + */
> +static int lamparray_get_lamp_attributes(struct lamparray_device *ldev)
> +{
> +	struct hid_device *hdev = ldev->hdev;
> +	struct hid_report *report;
> +	int ret;
> +	size_t len;
> +
> +	if (!lamparray_color_fields_is_complete(&ldev->color_levels))
> +		return -ENODEV;
> +
> +	/*
> +	 * All four fields must share the same report since the
> +	 * attributes are fetched with a single GET_REPORT below.
> +	 */
> +	report = ldev->color_levels.red.field->report;
> +	if (!report ||
> +	    ldev->color_levels.green.field->report != report ||
> +	    ldev->color_levels.blue.field->report != report ||
> +	    ldev->color_levels.intensity.field->report != report)
> +		return -ENODEV;
> +
> +	len = hid_report_len(report);
> +	u8 *buf __free(kfree) = kmalloc(len, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	guard(mutex)(&ldev->dev_lock);
> +
> +	/* Update values */
> +	ret = hid_hw_raw_request(hdev, report->id, buf, len,
> +				 HID_FEATURE_REPORT, HID_REQ_GET_REPORT);

[Severity: Medium]
Does lamparray_get_lamp_attributes() need to send a LampAttributesRequestReport
(usage 0x0020) with LampId (0x0021) set to 0 before requesting
LampAttributesResponseReport (0x0022)?

In the HID Lighting and Illumination specification, reading
LampAttributesResponseReport returns the attributes of the lamp selected by a
preceding LampAttributesRequestReport and advances the device's internal LampId
cursor on each read (for example, handle_lamp_attributes_response_report() in
drivers/platform/x86/tuxedo/nb04/wmi_ab.c reads and increments
driver_data->next_lamp_id). If the device's internal LampId cursor was already
advanced by a prior probe, userspace hidraw access, or warm reboot, or if the
firmware requires an explicit request before populating the response report,
could this read attributes from a different lamp index or fail?

> +	if (ret < 0) {
> +		hid_warn(hdev, "Failed to request LampAttributesResponseReport: %d\n", ret);
> +		return ret;
> +	}

[ ... ]

> +static int lamparray_restore_state(struct lamparray_device *ldev)
> +{
> +	u8 r, g, b;
> +	int ret;
> +	enum led_brightness brightness;
> +
> +	if (!ldev->use_leds_uapi)
> +		return 0;
> +
> +	guard(mutex)(&ldev->dev_lock);
> +
> +	r = ldev->last_r;
> +	g = ldev->last_g;
> +	b = ldev->last_b;
> +	brightness = ldev->last_brightness;
> +
> +	ldev->mc_cdev.subled_info[0].intensity = r;
> +	ldev->mc_cdev.subled_info[1].intensity = g;
> +	ldev->mc_cdev.subled_info[2].intensity = b;
> +	ldev->mc_cdev.led_cdev.brightness = brightness;
> +
> +	/*
> +	 * Quirk for devices ignoring intensity values, see
> +	 * lamparray_led_brightness_set for more information.
> +	 */
> +	if (brightness)
> +		ret = lamparray_hw_set_state(ldev, r, g, b, brightness);
> +	else
> +		ret = lamparray_hw_set_state(ldev, 0, 0, 0, 0);
> +
> +	return ret;
> +}
> +
> +/* LEDs API */
> +
> +static int lamparray_led_brightness_set(struct led_classdev *cdev,
> +					enum led_brightness brightness)
> +{
> +	struct led_classdev_mc *mc = lcdev_to_mccdev(cdev);
> +	struct lamparray_device *ldev =
> +		container_of_const(mc, struct lamparray_device, mc_cdev);
> +	u8 r, g, b;
> +	int ret;
> +
> +	/*
> +	 * Brightness is handled by the LampArray device if supported,
> +	 * so we can pass the raw intensity values.
> +	 */
> +	r = mc->subled_info[0].intensity;
> +	g = mc->subled_info[1].intensity;
> +	b = mc->subled_info[2].intensity;
> +	mc->led_cdev.brightness = brightness;
> +
> +	guard(mutex)(&ldev->dev_lock);
> +
> +	/*
> +	 * Some devices completely ignore the intensity (brightness)
> +	 * value and instead rely on zeros for each RGB channel to turn
> +	 * the LEDs off (e.g. the ASUS Prime B650-Plus). Since it doesn't
> +	 * hurt spec-compliant devices to quirk this behavior, we turn
> +	 * off the RGB channels unconditionally when the brightness is
> +	 * zero.
> +	 */
> +	if (brightness)
> +		ret = lamparray_hw_set_state(ldev, r, g, b, brightness);
> +	else
> +		ret = lamparray_hw_set_state(ldev, 0, 0, 0, 0);
> +
> +	if (ret) {
> +		hid_err(ldev->hdev, "Failed to send LampArray update: %d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	ldev->last_r = r;
> +	ldev->last_g = g;
> +	ldev->last_b = b;
> +	ldev->last_brightness = brightness;
> +
> +	return 0;
> +}
> +
> +static enum led_brightness
> +lamparray_led_brightness_get(struct led_classdev *cdev)
> +{
> +	struct led_classdev_mc *mc = lcdev_to_mccdev(cdev);
> +	struct lamparray_device *ldev =
> +	    container_of_const(mc, struct lamparray_device, mc_cdev);
> +
> +	return ldev->last_brightness;
> +}

[Severity: Medium]
Can lamparray_led_brightness_get(), lamparray_led_brightness_set(), and
lamparray_restore_state() race with the LED core and corrupt the cached
brightness and subled intensities?

Because lamparray_register_led() sets cdev->brightness_set_blocking =
lamparray_led_brightness_set (with cdev->brightness_set = NULL), writes to
brightness or multi_intensity in sysfs call led_set_brightness(), which updates
led_cdev->brightness and schedules led_cdev->set_brightness_work on leds_wq.
However, ldev->last_brightness is only updated at the end of
lamparray_led_brightness_set() after hid_hw_request() completes.

If userspace reads the brightness sysfs attribute while set_brightness_work is
still queued or running, brightness_show() calls led_update_brightness():

drivers/leds/led-core.c:led_update_brightness() {
	if (led_cdev->brightness_get) {
		ret = led_cdev->brightness_get(led_cdev);
		if (ret >= 0) {
			led_cdev->brightness = ret;
			return 0;
		}
	}
}

This invokes lamparray_led_brightness_get(), which reads ldev->last_brightness
unlocked and overwrites led_cdev->brightness with the stale value. A subsequent
multi_intensity_store() then passes that stale led_cdev->brightness back to
led_set_brightness().

In addition, if software blinking (LED_BLINK_SW) is active,
led_timer_function() calls led_set_brightness_nosleep(led_cdev, LED_OFF) on the
off-phase, which runs lamparray_led_brightness_set(cdev, 0) and overwrites
ldev->last_brightness with 0.

Finally, lamparray_led_brightness_set() reads mc->subled_info[0..2].intensity
and writes mc->led_cdev.brightness before acquiring ldev->dev_lock, while
lamparray_restore_state() modifies ldev->mc_cdev.subled_info[0..2].intensity
and ldev->mc_cdev.led_cdev.brightness after the LED classdev is already
registered in sysfs without holding cdev->led_access, racing with concurrent
brightness_store() and multi_intensity_store() calls.

Since the hardware state is not read back in lamparray_led_brightness_get(), is
the brightness_get callback needed at all, and should subled_info and
brightness accesses be synchronized cleanly before or during LED registration?

> +
> +/* Make sure to hold sysfs lock to prevent concurrent access. */
> +static int lamparray_register_led(struct lamparray_device *ldev)
> +{
> +	struct device *dev = &ldev->hdev->dev;
> +	struct led_classdev *cdev = &ldev->mc_cdev.led_cdev;
> +	int ret;
> +
> +	if (ldev->led_registered)
> +		return 0;
> +
> +	if (!cdev->name) {
> +		/* Fallback value */
> +		const char *function = LED_FUNCTION_STATUS;
> +
> +		/* Some heuristics for choosing a better LED function. */
> +		if (ldev->lamparray_kind_value == HID_LAMPARRAY_KIND_KEYBOARD)
> +			function = LED_FUNCTION_KBD_BACKLIGHT;
> +
> +		cdev->name = kasprintf(GFP_KERNEL, "rgb:%s", function);
> +		if (!cdev->name)
> +			return -ENOMEM;
> +	}
> +
> +	mutex_lock(&ldev->dev_lock);
> +	/* Setup */
> +	cdev->max_brightness = ldev->max_brightness;
> +	cdev->brightness_set_blocking = lamparray_led_brightness_set;
> +	cdev->brightness_get = lamparray_led_brightness_get;
> +	/* Don't do anything automatically on shutdown or suspend. */
> +	cdev->flags |= LED_RETAIN_AT_SHUTDOWN;

[Severity: Medium]
When the LED classdev is unregistered and re-registered via
use_leds_uapi_store(), does cdev->flags retain LED_UNREGISTERING?

Writing 0 to use_leds_uapi calls:

use_leds_uapi_store()
  -> lamparray_unregister_led()
    -> led_classdev_multicolor_unregister(&ldev->mc_cdev)
      -> led_classdev_unregister(&mcled_cdev->led_cdev)

In drivers/leds/led-class.c, led_classdev_unregister() sets:

	led_cdev->flags |= LED_UNREGISTERING;

When 1 is later written to use_leds_uapi, lamparray_register_led() re-registers
the same embedded ldev->mc_cdev structure and only does
cdev->flags |= LED_RETAIN_AT_SHUTDOWN, leaving LED_UNREGISTERING permanently
set on the re-registered LED device. Should cdev->flags be assigned
(cdev->flags = LED_RETAIN_AT_SHUTDOWN) or have LED_UNREGISTERING cleared?

> +
> +	ldev->subleds[0].color_index = LED_COLOR_ID_RED;
> +	ldev->subleds[0].max_intensity = ldev->max_r;

[ ... ]

> +static ssize_t use_leds_uapi_store(struct device *dev,
> +				   struct device_attribute *attr,
> +				   const char *buf, size_t count)
> +{
> +	struct lamparray_device *ldev = lamparray_ldev_from_sysfs_dev(dev);
> +	bool val, old_val;
> +	int ret;
> +
> +	if (!ldev)
> +		return -ENODEV;
> +
> +	ret = kstrtobool(buf, &val);
> +	if (ret)
> +		return ret;
> +
> +	/* Make sure only one concurrent write is possible */
> +	guard(mutex)(&ldev->sysfs_lock);
> +	old_val = ldev->use_leds_uapi;
> +
> +	if (val == old_val)
> +		return count;
> +
> +	ldev->use_leds_uapi = val;
> +
> +	/*
> +	 * Take the device out of autonomous mode before exposing the LED
> +	 * class device, and hand it back afterwards, so that control is
> +	 * transferred in both directions rather than only at probe.
> +	 */
> +	if (val == 1) {
> +		ret = lamparray_hw_set_autonomous(ldev, false);
> +		if (ret)
> +			goto err_revert;
> +		ret = lamparray_register_led(ldev);
> +		if (ret) {
> +			lamparray_hw_set_autonomous(ldev, true);
> +			goto err_revert;
> +		}
> +		ret = lamparray_restore_state(ldev);
> +		if (ret) {
> +			hid_warn(ldev->hdev, "Could not restore state: %d\n", ret);
> +			lamparray_unregister_led(ldev);
> +			lamparray_hw_set_autonomous(ldev, true);
> +			goto err_revert;
> +		}
> +	} else {
> +		lamparray_unregister_led(ldev);
> +		ret = lamparray_hw_set_autonomous(ldev, true);
> +		if (ret) {
> +			hid_warn(ldev->hdev, "Could not enable autonomous mode: %d\n", ret);
> +			return count;
> +		}
> +	}
> +
> +	return count;
> +
> +err_revert:
> +	ldev->use_leds_uapi = old_val;
> +	return ret;
> +}

[Severity: Medium]
In use_leds_uapi_store() (which uses guard(mutex)(&ldev->sysfs_lock) together
with goto err_revert) and in lamparray_register() below (which uses
struct lamparray *la __free(kfree) together with goto err_remove_sysfs,
err_xa_erase, err_unregister_led, and err_free), this mixes scope-based cleanup
helpers with goto error paths in the same function.

Per include/linux/cleanup.h, goto and cleanup helpers should not be mixed in
the same routine. Could these functions be structured to avoid mixing
__free()/guard() with goto labels?

[ ... ]

> +struct lamparray *
> +lamparray_register(struct hid_device *hdev,
> +		   const struct lamparray_init_state *led_init_state)
> +{
> +	int ret;
> +	struct lamparray_device *ldev;
> +
> +	if (!hdev)
> +		return ERR_PTR(-ENODEV);
> +
> +	struct lamparray *la __free(kfree) = kzalloc_obj(*la, GFP_KERNEL);
> +	if (!la)
> +		return ERR_PTR(-ENOMEM);

[ ... ]

> +	/* No locking needed, no races are possible before sysfs setup. */
> +	ret = lamparray_register_led(ldev);
> +	if (ret) {
> +		hid_warn(hdev, "Failed to register LED UAPI: %d\n", ret);
> +		ldev->use_leds_uapi = false;
> +	}
> +
> +	ret = xa_err(xa_store(&lamparray_by_hdev, (unsigned long)hdev, ldev,
> +			      GFP_KERNEL));
> +	if (ret)
> +		goto err_unregister_led;
> +
> +	ret = lamparray_register_sysfs(ldev);
> +	if (ret)
> +		goto err_xa_erase;
> +
> +	ret = lamparray_hw_set_autonomous(ldev, false);
> +	if (ret) {
> +		hid_err(hdev, "Could not disable autonomous mode: %d", ret);
> +		goto err_remove_sysfs;
> +	}
> +
> +	hid_info(hdev, "LampArray device registered\n");
> +
> +	ret = lamparray_restore_state(ldev);
> +	if (ret) {
> +		hid_err(hdev, "Failed to set default state: %d", ret);
> +		goto err_remove_sysfs;
> +	}
> +
> +	return_ptr(la);
> +
> +err_remove_sysfs:
> +	lamparray_remove_sysfs(ldev);
> +err_xa_erase:
> +	xa_erase(&lamparray_by_hdev, (unsigned long)hdev);
> +err_unregister_led:
> +	mutex_lock(&ldev->sysfs_lock);
> +	lamparray_unregister_led(ldev);
> +	mutex_unlock(&ldev->sysfs_lock);
> +err_free:
> +	mutex_destroy(&ldev->dev_lock);
> +	mutex_destroy(&ldev->sysfs_lock);
> +	return ERR_PTR(ret);
> +}

[Severity: Medium]
Does the registration order in lamparray_register() expose the LED classdev and
use_leds_uapi sysfs attribute before the hardware is initialized?

Here, lamparray_register_led(), xa_store(), and lamparray_register_sysfs()
expose /sys/class/leds/rgb:<function> and
/sys/bus/hid/devices/<id>/use_leds_uapi before
lamparray_hw_set_autonomous(ldev, false) and lamparray_restore_state(ldev) are
called, and without holding ldev->sysfs_lock:

1. If userspace writes 0 to use_leds_uapi immediately after
   lamparray_register_sysfs(), use_leds_uapi_store() unregisters the LED and
   enables autonomous mode, and then lamparray_register() calls
   lamparray_hw_set_autonomous(ldev, false), leaving autonomous mode disabled
   while use_leds_uapi is 0. Likewise, lamparray_restore_state() checks
   !ldev->use_leds_uapi without holding sysfs_lock or dev_lock, and will
   overwrite any brightness or color set by udev or LED triggers after
   lamparray_register_led().

2. If lamparray_register_led() fails, ldev->use_leds_uapi is set to false, but
   lamparray_register() still calls lamparray_hw_set_autonomous(ldev, false)
   even though no LED class device was registered.

3. If lamparray_restore_state() fails after
   lamparray_hw_set_autonomous(ldev, false) succeeded, the err_remove_sysfs
   error path does not call lamparray_hw_set_autonomous(ldev, true) to restore
   autonomous mode.

Should autonomous mode be disabled and the initial state applied before
registering the LED classdev and sysfs attribute?

> +EXPORT_SYMBOL_GPL(lamparray_register);
> +
> +void lamparray_unregister(struct lamparray *la)
> +{
> +	struct lamparray_device *ldev;
> +
> +	if (!la)
> +		return;
> +
> +	ldev = &la->ldev;
> +
> +	lamparray_hw_set_autonomous(ldev, true);
> +
> +	lamparray_remove_sysfs(ldev);
> +	xa_erase(&lamparray_by_hdev, (unsigned long)ldev->hdev);
> +
> +	mutex_lock(&ldev->sysfs_lock);
> +	lamparray_unregister_led(ldev);
> +	mutex_unlock(&ldev->sysfs_lock);

[Severity: Medium]
In lamparray_unregister(), isn't the teardown order inverted relative to
use_leds_uapi_store()?

lamparray_hw_set_autonomous(ldev, true) is called before
lamparray_remove_sysfs(), xa_erase(), and lamparray_unregister_led(). While the
sysfs attribute and LED classdev are still registered, a concurrent write of 1
to use_leds_uapi_store() can disable autonomous mode again, or pending
cdev->set_brightness_work / blink timers (which are only flushed inside
led_classdev_unregister() during lamparray_unregister_led()) can run
lamparray_led_brightness_set() -> lamparray_hw_set_state() after autonomous
mode was re-enabled.

Also, because lamparray_hw_set_autonomous() submits HID_REQ_SET_REPORT via
hid_hw_request() (which is asynchronous on transports like usbhid) without
calling hid_hw_wait(ldev->hdev), if the caller invokes hid_hw_stop(hdev)
immediately after lamparray_unregister(), can usbhid_stop() cancel the
in-flight control request before autonomous mode is restored?

> +
> +	mutex_destroy(&ldev->dev_lock);
> +	mutex_destroy(&ldev->sysfs_lock);
> +	kfree(la);
> +}
> +EXPORT_SYMBOL_GPL(lamparray_unregister);

[ ... ]

> diff --git a/include/linux/hid-lamparray.h b/include/linux/hid-lamparray.h
> new file mode 100644
> index 0000000000000..6124878859392
> --- /dev/null
> +++ b/include/linux/hid-lamparray.h
> @@ -0,0 +1,89 @@

[ ... ]

> +/**
> + * lamparray_register() - initialize LampArray support for a HID device
> + * @hdev: HID device
> + * @led_init_state: Optional LED state at init specification
> + *
> + * Allocate and initialize internal LampArray state for the given HID device.
> + * The function parses required HID reports and fields and registers the
> + * associated miscdevice and sysfs attributes.
> + *
> + * Registers a multicolor LED class device to expose the LampArray functionality
> + * via the LED subsystem. If specified, the desired initial LED state is
> + * applied. If led_init_state is NULL, a default state is applied (white with
> + * zero brightness).
> + *
> + * Return: pointer to a LampArray handle on success, or ERR_PTR() on failure.
> + */
> +struct lamparray *lamparray_register(struct hid_device *hdev,
> +				     const struct lamparray_init_state *led_init_state);
> +
> +/**
> + * lamparray_unregister() - tear down LampArray support
> + * @la: LampArray handle returned by lamparray_register()
> + *
> + * Remove all resources associated with a LampArray instance.
> + *
> + * This unregisters the LED class device (if present), removes the miscdevice
> + * and sysfs interfaces and frees all internal state associated with @la.
> + */
> +void lamparray_unregister(struct lamparray *la);

[Severity: Low]
This isn't a bug, but the kernel-doc comments for lamparray_register() and
lamparray_unregister() mention registering and removing a miscdevice, which
does not exist in the implementation.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020-hid-lamparray-v7-0-47418b667b47@tuxedocomputers.com?part=1

[PATCH v7 2/3] HID: generic: add LampArray support via hid-lamparray helper

[Aaron Erhardt]

The hid-generic driver now checks for LampArray support after
hid_parse() and optionally registers a lamparray instance. Failures in
the helper do not abort device probe to keep the device unchanged.

LampArray resources are released on driver remove.

This patch was successfully tested on the Microsoft MacroPad reference
implementation (https://github.com/microsoft/RP2040MacropadHidSample
1d6c3ad) and in combination with the tuxedo_nb04_wmi driver, albeit only
functional with a recent fix posted to the LKML.

Link:
https://lore.kernel.org/all/20260728115918.125349-2-aer@tuxedocomputers.com

Co-developed-by: Tim Guttzeit <tgu@tuxedocomputers.com>
Signed-off-by: Tim Guttzeit <tgu@tuxedocomputers.com>
Signed-off-by: Aaron Erhardt <aer@tuxedocomputers.com>
---
 drivers/hid/Kconfig       |  1 +
 drivers/hid/hid-generic.c | 41 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+)

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 934e502ba5058..919d495090d97 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -82,6 +82,7 @@ config UHID
 
 config HID_GENERIC
 	tristate "Generic HID driver"
+	depends on HID_LAMPARRAY if HID_LAMPARRAY
 	default HID
 	help
 	Support for generic devices on the HID bus. This includes most
diff --git a/drivers/hid/hid-generic.c b/drivers/hid/hid-generic.c
index c2de916747dec..fde772c5014bb 100644
--- a/drivers/hid/hid-generic.c
+++ b/drivers/hid/hid-generic.c
@@ -20,6 +20,7 @@
 #include <asm/byteorder.h>
 
 #include <linux/hid.h>
+#include <linux/hid-lamparray.h>
 
 static struct hid_driver hid_generic;
 
@@ -60,6 +61,7 @@ static int hid_generic_probe(struct hid_device *hdev,
 			     const struct hid_device_id *id)
 {
 	int ret;
+	struct lamparray *la;
 
 	hdev->quirks |= HID_QUIRK_INPUT_PER_APP;
 
@@ -67,6 +69,34 @@ static int hid_generic_probe(struct hid_device *hdev,
 	if (ret)
 		return ret;
 
+	/*
+	 * Optional: attach LampArray support if present.
+	 * Never fail probe on LampArray errors; keep device functional.
+	 */
+	if (IS_ENABLED(CONFIG_HID_LAMPARRAY) && lamparray_is_supported_device(hdev)) {
+		/*
+		 * Use HID_CONNECT_DRIVER to make sure requests are
+		 * processed in hid_report_raw_event. Without this,
+		 * hid-core will skip parsing for devices that are only
+		 * claimed by hidraw, thus making it impossible to query
+		 * the required information.
+		 * See 7704ac937345 for more information about this flag
+		 * and why it is necessary.
+		 */
+		ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT | HID_CONNECT_DRIVER);
+		if (ret)
+			return ret;
+
+		la = lamparray_register(hdev, NULL);
+		if (IS_ERR(la)) {
+			hid_hw_stop(hdev);
+			hid_warn(hdev, "LampArray init failed: %ld\n", PTR_ERR(la));
+		} else {
+			hid_set_drvdata(hdev, la);
+			return 0;
+		}
+	}
+
 	return hid_hw_start(hdev, HID_CONNECT_DEFAULT);
 }
 
@@ -78,6 +108,16 @@ static int hid_generic_reset_resume(struct hid_device *hdev)
 	return 0;
 }
 
+static void hid_generic_remove(struct hid_device *hdev)
+{
+	struct lamparray *la = hid_get_drvdata(hdev);
+
+	if (IS_ENABLED(CONFIG_HID_LAMPARRAY) && la)
+		lamparray_unregister(la);
+
+	hid_hw_stop(hdev);
+}
+
 static const struct hid_device_id hid_table[] = {
 	{ HID_DEVICE(HID_BUS_ANY, HID_GROUP_ANY, HID_ANY_ID, HID_ANY_ID) },
 	{ }
@@ -90,6 +130,7 @@ static struct hid_driver hid_generic = {
 	.match = hid_generic_match,
 	.probe = hid_generic_probe,
 	.reset_resume = hid_generic_reset_resume,
+	.remove = hid_generic_remove,
 };
 module_hid_driver(hid_generic);
 

-- 
2.43.0

Re: [PATCH v7 2/3] HID: generic: add LampArray support via hid-lamparray helper

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] HID: generic: drvdata clobber and uninitialized read in hid_generic_remove()
- [High] HID: generic: double hid_hw_start() in hid_generic_probe() corrupts reports
--

commit 977659b4786943b7473279419965b0035b45922c
Author: Aaron Erhardt <aer@tuxedocomputers.com>

HID: generic: add LampArray support via hid-lamparray helper

This commit checks for LampArray support in hid_generic_probe() after
hid_parse() and registers a lamparray instance when present. On failure, it
stops the hardware and falls back to starting the device without LampArray,
and unregisters the lamparray instance in hid_generic_remove().

Link: https://lore.kernel.org/all/20260728115918.125349-2-aer@tuxedocomputers.com

[ ... ]

> diff --git a/drivers/hid/hid-generic.c b/drivers/hid/hid-generic.c
> index c2de916747dec..fde772c5014bb 100644
> --- a/drivers/hid/hid-generic.c
> +++ b/drivers/hid/hid-generic.c

[ ... ]

> @@ -67,6 +69,34 @@ static int hid_generic_probe(struct hid_device *hdev,
>  	if (ret)
>  		return ret;
>  
> +	/*
> +	 * Optional: attach LampArray support if present.
> +	 * Never fail probe on LampArray errors; keep device functional.
> +	 */
> +	if (IS_ENABLED(CONFIG_HID_LAMPARRAY) && lamparray_is_supported_device(hdev)) {
> +		/*
> +		 * Use HID_CONNECT_DRIVER to make sure requests are
> +		 * processed in hid_report_raw_event. Without this,
> +		 * hid-core will skip parsing for devices that are only
> +		 * claimed by hidraw, thus making it impossible to query
> +		 * the required information.
> +		 * See 7704ac937345 for more information about this flag
> +		 * and why it is necessary.
> +		 */
> +		ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT | HID_CONNECT_DRIVER);
> +		if (ret)
> +			return ret;
> +
> +		la = lamparray_register(hdev, NULL);
> +		if (IS_ERR(la)) {
> +			hid_hw_stop(hdev);
> +			hid_warn(hdev, "LampArray init failed: %ld\n", PTR_ERR(la));
> +		} else {
> +			hid_set_drvdata(hdev, la);
> +			return 0;
> +		}
> +	}
> +
>  	return hid_hw_start(hdev, HID_CONNECT_DEFAULT);
>  }

[Severity: High]
Does calling hid_hw_start() a second time on the fallback path when
lamparray_register() fails leak report->field_entries and duplicate entries
in report->field_entry_list?

The first hid_hw_start() calls hid_connect() -> hid_process_ordering() ->
hid_report_process_ordering() for every HID_INPUT_REPORT:

drivers/hid/hid-core.c:hid_report_process_ordering() {
	...
	/* allocate the memory to process the fields */
	entries = kzalloc_objs(*entries, count);
	if (!entries)
		return;

	report->field_entries = entries;
	...
}

And appends each entry to report->field_entry_list (which was initialized
once during hid_parse() -> hid_register_report()).

When lamparray_register() fails, hid_generic_probe() calls hid_hw_stop() ->
hid_disconnect(), which does not free report->field_entries or reinitialize
report->field_entry_list because those are only cleaned up in
hid_close_report() -> hid_free_report():

drivers/hid/hid-core.c:hid_free_report() {
	unsigned n;

	kfree(report->field_entries);

	for (n = 0; n < report->maxfield; n++)
		kvfree(report->field[n]);
	kfree(report);
}

When hid_generic_probe() falls through to the second hid_hw_start() on the
same parsed reports, hid_report_process_ordering() allocates a new entries
array, overwrites report->field_entries without freeing the previous array,
and links a second set of entries into report->field_entry_list alongside
the still-linked entries from the first leaked array.

Wouldn't this permanently leak the first report->field_entries allocation and
cause hid_process_report() to process every input field twice per report?

> @@ -78,6 +108,16 @@ static int hid_generic_reset_resume(struct hid_device *hdev)
>  	return 0;
>  }
>  
> +static void hid_generic_remove(struct hid_device *hdev)
> +{
> +	struct lamparray *la = hid_get_drvdata(hdev);
> +
> +	if (IS_ENABLED(CONFIG_HID_LAMPARRAY) && la)
> +		lamparray_unregister(la);
> +
> +	hid_hw_stop(hdev);
> +}

[Severity: Critical]
Can using hid_set_drvdata() and hid_get_drvdata() here collide with low-level
HID transport drivers that store their own private data in
hdev->dev.driver_data?

For example, drivers/hid/hid-hyperv.c sets drvdata before registering the HID
device:

drivers/hid/hid-hyperv.c:mousevsc_probe() {
	...
	hid_set_drvdata(hid_dev, device);

	ret = hid_add_device(hid_dev);
	...
}

And drivers/platform/x86/tuxedo/nb04/wmi_ab.c sets drvdata inside its start
callback during hid_hw_start():

drivers/platform/x86/tuxedo/nb04/wmi_ab.c:tux_ll_start() {
	...
	dev_set_drvdata(&hdev->dev, driver_data);

	return ret;
}

Because hid_generic_probe() only calls hid_set_drvdata(hdev, la) when
lamparray_register() succeeds and leaves hdev->dev.driver_data untouched on
all other paths, if a device on such a transport does not support LampArray
or fails lamparray_register(), won't hid_generic_remove() retrieve the
transport driver's pointer via hid_get_drvdata(hdev) and pass it to
lamparray_unregister(la), corrupting memory and calling kfree() on the
transport driver's structure?

Conversely, if lamparray_register() succeeds on tuxedo_nb04_wmi, won't
hid_set_drvdata(hdev, la) overwrite the struct tux_hdev_driver_data_t pointer
stored by tux_ll_start(), causing subsequent tux_ll_raw_request() calls to
dereference struct lamparray as struct tux_hdev_driver_data_t?

[ ... ]

>
>
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020-hid-lamparray-v7-0-47418b667b47@tuxedocomputers.com?part=2

[PATCH v7 3/3] HID: lamparray: blank lamps across suspend and restore on resume

[Aaron Erhardt]

From: Cristian Mazzotta <cmmazzo@icloud.com>

The helper installs no PM callbacks, so lamps keep their last state
across suspend. Firmware is not required to turn them off in low power
states, and on devices where it does not, they stay lit for the whole
suspend.

On an Acer Predator PT14-52T, system power draw during s2idle is 12.35W
with the lamps lit and 3.14W with them blanked, measured at the charger.

Add lamparray_suspend(), which writes zeroes to the color channels
while leaving the cached RGB and brightness untouched, and
lamparray_resume(), which restores that cache. Both return early when
use_leds_uapi is 0 so a userspace driver keeps full control. hid-generic
gains suspend and resume callbacks to drive them, and calls
lamparray_resume() from reset_resume as well.

Some devices return to firmware control across a hibernate transition
while still reporting AutonomousMode = 0, and ignore host lamp updates
until the value genuinely transitions. Force a 1 -> 0 on resume
before restoring state.

Co-developed-by: Aaron Erhardt <aer@tuxedocomputers.com>
Signed-off-by: Aaron Erhardt <aer@tuxedocomputers.com>
Signed-off-by: Cristian Mazzotta <cmmazzo@icloud.com>
---
 drivers/hid/hid-generic.c     | 29 ++++++++++++++++++++++-
 drivers/hid/hid-lamparray.c   | 54 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/hid-lamparray.h | 29 +++++++++++++++++++++++
 3 files changed, 111 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-generic.c b/drivers/hid/hid-generic.c
index fde772c5014bb..f78c62f796fbb 100644
--- a/drivers/hid/hid-generic.c
+++ b/drivers/hid/hid-generic.c
@@ -102,9 +102,34 @@ static int hid_generic_probe(struct hid_device *hdev,
 
 static int hid_generic_reset_resume(struct hid_device *hdev)
 {
+	struct lamparray *la = hid_get_drvdata(hdev);
+
 	if (hdev->claimed & HID_CLAIMED_INPUT)
 		hidinput_reset_resume(hdev);
 
+	if (IS_ENABLED(CONFIG_HID_LAMPARRAY) && la)
+		lamparray_resume(la);
+
+	return 0;
+}
+
+static int hid_generic_suspend(struct hid_device *hdev, pm_message_t message)
+{
+	struct lamparray *la = hid_get_drvdata(hdev);
+
+	if (IS_ENABLED(CONFIG_HID_LAMPARRAY) && la)
+		lamparray_suspend(la);
+
+	return 0;
+}
+
+static int hid_generic_resume(struct hid_device *hdev)
+{
+	struct lamparray *la = hid_get_drvdata(hdev);
+
+	if (IS_ENABLED(CONFIG_HID_LAMPARRAY) && la)
+		lamparray_resume(la);
+
 	return 0;
 }
 
@@ -129,8 +154,10 @@ static struct hid_driver hid_generic = {
 	.id_table = hid_table,
 	.match = hid_generic_match,
 	.probe = hid_generic_probe,
-	.reset_resume = hid_generic_reset_resume,
 	.remove = hid_generic_remove,
+	.reset_resume = pm_ptr(hid_generic_reset_resume),
+	.suspend = pm_ptr(hid_generic_suspend),
+	.resume = pm_ptr(hid_generic_resume),
 };
 module_hid_driver(hid_generic);
 
diff --git a/drivers/hid/hid-lamparray.c b/drivers/hid/hid-lamparray.c
index fb3082f6d19a0..5c97cafbd25d2 100644
--- a/drivers/hid/hid-lamparray.c
+++ b/drivers/hid/hid-lamparray.c
@@ -852,6 +852,60 @@ void lamparray_unregister(struct lamparray *la)
 }
 EXPORT_SYMBOL_GPL(lamparray_unregister);
 
+/*
+ * Blank all lamps on suspend rather than handing control back to the firmware,
+ * which may not turn them off in low power states. On an Acer Predator PT14-52T,
+ * system power draw during s2idle was ~12.35W with lamps lit, and ~3.14W with
+ * them blanked; the lighting accounted for ~75% of the power draw during suspend.
+ * Since writing zeroes is well defined on all lamparray devices, always do it.
+ * This is ignored if use_leds_uapi is 0; let userspace keep full control.
+ */
+void lamparray_suspend(struct lamparray *la)
+{
+	struct lamparray_device *ldev;
+
+	if (!la)
+		return;
+
+	ldev = &la->ldev;
+
+	if (!ldev->use_leds_uapi)
+		return;
+
+	mutex_lock(&ldev->dev_lock);
+	lamparray_hw_set_state(ldev, 0, 0, 0, 0);
+	mutex_unlock(&ldev->dev_lock);
+
+	hid_hw_wait(ldev->hdev);
+}
+EXPORT_SYMBOL_GPL(lamparray_suspend);
+
+void lamparray_resume(struct lamparray *la)
+{
+	struct lamparray_device *ldev;
+
+	if (!la)
+		return;
+
+	ldev = &la->ldev;
+
+	if (!ldev->use_leds_uapi)
+		return;
+
+	/*
+	 * After a S4 transition, some devices report
+	 * AutonomousMode = 0 while still ignoring host lamp updates.
+	 * Writing 0 again does nothing; forcing a 1 -> 0
+	 * will guarantee the device will update.
+	 */
+	lamparray_hw_set_autonomous(ldev, true);
+	hid_hw_wait(ldev->hdev);
+	lamparray_hw_set_autonomous(ldev, false);
+
+	lamparray_restore_state(ldev);
+}
+EXPORT_SYMBOL_GPL(lamparray_resume);
+
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Tim Guttzeit <tgu@tuxedocomputers.com>");
 MODULE_AUTHOR("Aaron Erhardt <aer@tuxedocomputers.com>");
diff --git a/include/linux/hid-lamparray.h b/include/linux/hid-lamparray.h
index 6124878859392..dc9df938e5b7e 100644
--- a/include/linux/hid-lamparray.h
+++ b/include/linux/hid-lamparray.h
@@ -66,6 +66,27 @@ struct lamparray *lamparray_register(struct hid_device *hdev,
  */
 void lamparray_unregister(struct lamparray *la);
 
+/**
+ * lamparray_suspend() - blank all lamps ahead of sleep transition
+ * @la: LampArray handle returned by lamparray_register()
+ *
+ * Writes zeroes to the rgb and brightness values, unless the use_leds_uapi
+ * sysfs attribute is 0, in which case, it will return early before writing
+ * anything. The cached state is left untouched so lamparray_resume() can
+ * restore it.
+ */
+void lamparray_suspend(struct lamparray *la);
+
+/**
+ * lamparray_resume() - restore host control and LampArray state
+ * @la: LampArray handle returned by lamparray_register()
+ *
+ * Disables autonomous mode (in case device returns to firmware control after suspend)
+ * and restores the cached state of the device. If the use_leds_uapi attribute is 0,
+ * it will return early and prevent any unwanted writing.
+ */
+void lamparray_resume(struct lamparray *la);
+
 #else /* !CONFIG_HID_LAMPARRAY */
 
 static inline bool lamparray_is_supported_device(struct hid_device *hdev)
@@ -84,6 +105,14 @@ static inline void lamparray_unregister(struct lamparray *la)
 {
 }
 
+static inline void lamparray_suspend(struct lamparray *la)
+{
+}
+
+static inline void lamparray_resume(struct lamparray *la)
+{
+}
+
 #endif /* CONFIG_HID_LAMPARRAY */
 
 #endif /* _HID_LAMPARRAY_H */

-- 
2.43.0

Re: [PATCH v7 3/3] HID: lamparray: blank lamps across suspend and restore on resume

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] HID: hid-generic: missing PMSG_IS_AUTO() check in hid_generic_suspend()
- [Medium] HID: lamparray: missing LED_SUSPENDED and work flush in lamparray_suspend()
- [Medium] HID: lamparray: race on use_leds_uapi in suspend and resume paths
--

commit eae25839ebd677bf288de15dd8a2d0952e978e6d
Author: Cristian Mazzotta <cmmazzo@icloud.com>

HID: lamparray: blank lamps across suspend and restore on resume

This commit adds lamparray_suspend() and lamparray_resume() helpers to blank
LampArray lighting before system sleep and restore the cached RGB and
brightness state on resume, wiring them up in hid-generic.

> diff --git a/drivers/hid/hid-generic.c b/drivers/hid/hid-generic.c
> index fde772c5014bb..f78c62f796fbb 100644
> --- a/drivers/hid/hid-generic.c
> +++ b/drivers/hid/hid-generic.c
> @@ -102,9 +102,34 @@ static int hid_generic_probe(struct hid_device *hdev,
>  
>  static int hid_generic_reset_resume(struct hid_device *hdev)
>  {
> +	struct lamparray *la = hid_get_drvdata(hdev);
> +
>  	if (hdev->claimed & HID_CLAIMED_INPUT)
>  		hidinput_reset_resume(hdev);
>  
> +	if (IS_ENABLED(CONFIG_HID_LAMPARRAY) && la)
> +		lamparray_resume(la);
> +
> +	return 0;
> +}
> +
> +static int hid_generic_suspend(struct hid_device *hdev, pm_message_t message)
> +{
> +	struct lamparray *la = hid_get_drvdata(hdev);
> +
> +	if (IS_ENABLED(CONFIG_HID_LAMPARRAY) && la)
> +		lamparray_suspend(la);
> +
> +	return 0;
> +}
> +
> +static int hid_generic_resume(struct hid_device *hdev)
> +{
> +	struct lamparray *la = hid_get_drvdata(hdev);
> +
> +	if (IS_ENABLED(CONFIG_HID_LAMPARRAY) && la)
> +		lamparray_resume(la);
> +
>  	return 0;
>  }

[Severity: High]
How does this behave with USB runtime autosuspend when PMSG_IS_AUTO(message)
is true?

In drivers/hid/usbhid/hid-core.c, hid_suspend() counts only EV_LED usages in
HID_OUTPUT_REPORT via hidinput_count_leds(), not LampArray HID_FEATURE_REPORT
LEDs. When a USB LampArray interface enters runtime autosuspend in S0,
hid_suspend() sets HID_SUSPENDED on usbhid->iofl before calling
hid_driver_suspend():

drivers/hid/usbhid/hid-core.c:hid_suspend() {
	if (PMSG_IS_AUTO(message)) {
		ledcount = hidinput_count_leds(hid);
		spin_lock_irq(&usbhid->lock);	/* Sync with error handler */
		if (!test_bit(HID_RESET_PENDING, &usbhid->iofl)
		    && !test_bit(HID_CLEAR_HALT, &usbhid->iofl)
		    && !test_bit(HID_OUT_RUNNING, &usbhid->iofl)
		    && !test_bit(HID_CTRL_RUNNING, &usbhid->iofl)
		    && !test_bit(HID_KEYS_PRESSED, &usbhid->iofl)
		    && (!ledcount || ignoreled))
		{
			set_bit(HID_SUSPENDED, &usbhid->iofl);
			spin_unlock_irq(&usbhid->lock);
			status = hid_driver_suspend(hid, message);
}

Because hid_generic_suspend() ignores message and calls lamparray_suspend(),
lamparray_hw_set_state() -> hid_hw_request() -> __usbhid_submit_report()
queues the (0, 0, 0, 0) report in usbhid->ctrl[] and calls
usbhid_restart_ctrl_queue(), which returns early because HID_SUSPENDED is
already set:

drivers/hid/usbhid/hid-core.c:usbhid_restart_ctrl_queue() {
	if (!hid || test_bit(HID_RESET_PENDING, &usbhid->iofl) ||
			test_bit(HID_SUSPENDED, &usbhid->iofl))
		return 0;
}

hid_hw_wait() then sees HID_CTRL_RUNNING == 0 and returns immediately while
the blanking report remains unsent in usbhid->ctrl[].

If the system subsequently enters s2idle or S3 while the USB device is already
runtime-suspended, usb_suspend_both() skips usb_suspend_interface() and the
blanking report is never transmitted to the hardware before system sleep. In
S0, when the device runtime-resumes via hid_resume(), hid_restart_io()
transmits the stale (0, 0, 0, 0) report and hid_generic_resume() calls
lamparray_resume(), cycling AutonomousMode (1 -> 0) and restoring state on
every runtime wakeup.

Should hid_generic_suspend() and hid_generic_resume() skip LampArray
blanking/restoration during runtime PM (or should runtime autosuspend be
prevented while LampArray LEDs are active)?

[ ... ]

> diff --git a/drivers/hid/hid-lamparray.c b/drivers/hid/hid-lamparray.c
> index fb3082f6d19a0..5c97cafbd25d2 100644
> --- a/drivers/hid/hid-lamparray.c
> +++ b/drivers/hid/hid-lamparray.c
> @@ -852,6 +852,60 @@ void lamparray_unregister(struct lamparray *la)
>  }
>  EXPORT_SYMBOL_GPL(lamparray_unregister);
>  
> +/*
> + * Blank all lamps on suspend rather than handing control back to the firmware,
> + * which may not turn them off in low power states. On an Acer Predator PT14-52T,
> + * system power draw during s2idle was ~12.35W with lamps lit, and ~3.14W with
> + * them blanked; the lighting accounted for ~75% of the power draw during suspend.
> + * Since writing zeroes is well defined on all lamparray devices, always do it.
> + * This is ignored if use_leds_uapi is 0; let userspace keep full control.
> + */
> +void lamparray_suspend(struct lamparray *la)
> +{
> +	struct lamparray_device *ldev;
> +
> +	if (!la)
> +		return;
> +
> +	ldev = &la->ldev;
> +
> +	if (!ldev->use_leds_uapi)
> +		return;
> +
> +	mutex_lock(&ldev->dev_lock);
> +	lamparray_hw_set_state(ldev, 0, 0, 0, 0);
> +	mutex_unlock(&ldev->dev_lock);
> +
> +	hid_hw_wait(ldev->hdev);
> +}
> +EXPORT_SYMBOL_GPL(lamparray_suspend);

[Severity: Medium]
Could pending LED work or software blink timers run after lamparray_suspend()
blanks the hardware, un-blanking the lamps and overwriting ldev->last_*?

lamparray_register_led() registers ldev->mc_cdev.led_cdev with
brightness_set_blocking = lamparray_led_brightness_set and without
LED_CORE_SUSPENDRESUME. All non-sleeping brightness updates (sysfs writes,
led_timer_function() software blink, and in-kernel LED triggers) go through
led_set_brightness_nosleep() -> led_set_brightness_nopm(), which queues
cdev->set_brightness_work onto leds_wq (a non-freezable workqueue) unless
LED_SUSPENDED is set on cdev->flags.

Because lamparray_suspend() does not set LED_SUSPENDED on
ldev->mc_cdev.led_cdev.flags or call
flush_work(&ldev->mc_cdev.led_cdev.set_brightness_work), a queued
set_brightness_work or a blink/trigger timer firing during dpm_suspend() or
s2idle can run lamparray_led_brightness_set() after lamparray_suspend() drops
ldev->dev_lock:

drivers/hid/hid-lamparray.c:lamparray_led_brightness_set() {
	...
	guard(mutex)(&ldev->dev_lock);
	...
	if (brightness)
		ret = lamparray_hw_set_state(ldev, r, g, b, brightness);
	else
		ret = lamparray_hw_set_state(ldev, 0, 0, 0, 0);
	...
	ldev->last_r = r;
	ldev->last_g = g;
	ldev->last_b = b;
	ldev->last_brightness = brightness;
}

This sends a new HID_REQ_SET_REPORT during suspend and clobbers the cached
ldev->last_r, ldev->last_g, ldev->last_b, and ldev->last_brightness values.
Similarly, during lamparray_resume(), set_brightness_work can interleave
between lamparray_hw_set_autonomous() and lamparray_restore_state() while
ldev->dev_lock is dropped.

> +
> +void lamparray_resume(struct lamparray *la)
> +{
> +	struct lamparray_device *ldev;
> +
> +	if (!la)
> +		return;
> +
> +	ldev = &la->ldev;
> +
> +	if (!ldev->use_leds_uapi)
> +		return;
> +
> +	/*
> +	 * After a S4 transition, some devices report
> +	 * AutonomousMode = 0 while still ignoring host lamp updates.
> +	 * Writing 0 again does nothing; forcing a 1 -> 0
> +	 * will guarantee the device will update.
> +	 */
> +	lamparray_hw_set_autonomous(ldev, true);
> +	hid_hw_wait(ldev->hdev);
> +	lamparray_hw_set_autonomous(ldev, false);
> +
> +	lamparray_restore_state(ldev);
> +}
> +EXPORT_SYMBOL_GPL(lamparray_resume);

[Severity: Medium]
Can lamparray_suspend() and lamparray_resume() race with concurrent writes to
use_leds_uapi_store()?

In use_leds_uapi_store(), transitions of ldev->use_leds_uapi, LED
registration/unregistration, and AutonomousMode updates are serialized under
ldev->sysfs_lock:

drivers/hid/hid-lamparray.c:use_leds_uapi_store() {
	guard(mutex)(&ldev->sysfs_lock);
	old_val = ldev->use_leds_uapi;

	if (val == old_val)
		return count;

	ldev->use_leds_uapi = val;
	...
}

Neither lamparray_suspend() nor lamparray_resume() acquires ldev->sysfs_lock,
and lamparray_resume() drops ldev->dev_lock between
lamparray_hw_set_autonomous(ldev, true), hid_hw_wait(),
lamparray_hw_set_autonomous(ldev, false), and lamparray_restore_state().

If use_leds_uapi_store() transitions from 1 to 0 while lamparray_resume() is
sleeping in hid_hw_wait() (for example, during USB runtime resume),
use_leds_uapi_store() unregisters the LED and sets AutonomousMode = 1, after
which lamparray_resume() calls lamparray_hw_set_autonomous(ldev, false)
without re-checking use_leds_uapi, leaving AutonomousMode disabled while
use_leds_uapi is 0.

Conversely, during a 0 to 1 transition in use_leds_uapi_store(),
ldev->use_leds_uapi = val is written before lamparray_register_led()
completes. If initial LED registration failed during lamparray_register() and
lamparray_resume() observes ldev->use_leds_uapi == true before
lamparray_register_led() finishes initializing ldev->mc_cdev.subled_info,
lamparray_restore_state() can dereference a NULL ldev->mc_cdev.subled_info
pointer.

Should lamparray_suspend() and lamparray_resume() serialize against
use_leds_uapi_store() with ldev->sysfs_lock?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020-hid-lamparray-v7-0-47418b667b47@tuxedocomputers.com?part=3