Re: [PATCH v5 1/2] HID: lamparray: add new LampArray helper module

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [High] Division by zero during LampArray LED registration if the device reports zero intensity levels.
- [High] `lamparray_register` forcibly releases the HID core's `driver_input_lock` during driver probe.
- [Medium] Concurrent sysfs writes can cause the internal state flag (`use_leds_uapi`) to completely desynchronize from the actual LED registration state due to prematurely dropping `dev_lock`.
- [Medium] The hardcoded LED name violates the LED subsystem naming conventions and will cause a sysfs `-EEXIST` error when multiple identical devices are connected.
- [Medium] The driver assumes all color and range fields reside within the same HID report, leading to incomplete state updates if they are split.
- [Medium] Hardware is left in a non-autonomous state on the `lamparray_register` error path if `lamparray_restore_state` fails.
- [Medium] Lamp Range Update reports are defined as Output reports in the HID specification, but the driver only searches Feature reports.
- [Medium] Incorrect teardown ordering in `lamparray_unregister` allows concurrent hardware writes in autonomous mode.
- [Low] The driver prints a success log message during initialization, which explicitly violates the subsystem's logging rules.
--

commit f90eeb3c7fb481eba03a666b2a5bcb8dd92f3e9c
Author: Aaron Erhardt <aer@tuxedocomputers.com>
Subject: HID: lamparray: add new LampArray helper module

This commit adds a new hid-lamparray helper module for devices exposing a
Lighting/LampArray application collection (usage page 0x59). It registers a
single-zone RGB LED representation via the LED subsystem.

> diff --git a/drivers/hid/hid-lamparray.c b/drivers/hid/hid-lamparray.c
> new file mode 100644
> index 0000000000000..9a438aa2d305e
> --- /dev/null
> +++ b/drivers/hid/hid-lamparray.c

[ ... ]

> @@ -176,14 +176,12 @@ static int lamparray_parse_update_report(struct lamparray_device *ldev)
>  	int i, j;
>  	int ret = 0;
>  
>  	mutex_lock(&ldev->dev_lock);
>  
>  	re = &hdev->report_enum[HID_FEATURE_REPORT];
>  
>  	list_for_each_entry(report, &re->report_list, list) {
>  		for (i = 0; i < report->maxfield; i++) {

[Severity: Medium]
This restricts the search strictly to feature reports.

Since the HID specification defines Lamp Range Update reports as output
reports, does this mean the driver will fail to probe compliant devices
because it cannot find their output reports?

[ ... ]

> @@ -340,11 +340,9 @@ static int lamparray_hw_set_state(struct lamparray_device *ldev, u8 r, u8 g,
>  	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);
>  	hid_hw_wait(hdev);

[Severity: Medium]
This hardware request is dispatched exclusively for the report containing
the 'red' field.

If a device descriptor splits the color channels across multiple report IDs,
wouldn't this leave the other colors unsent and result in incomplete state
updates?

[ ... ]

> @@ -386,10 +386,9 @@ static int lamparray_get_lamp_attributes(struct lamparray_device *ldev)
>  	hid_hw_request(hdev, report, HID_REQ_GET_REPORT);
>  	hid_hw_wait(hdev);
>  
>  	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);

[Severity: High]
This reads the maximum brightness directly from the device's GET_REPORT
response without validating it against zero.

Later, when led_mc_calc_color_components() is called to apply the brightness,
this max_brightness value acts as a divisor. If a device reports zero
intensity levels, does this trigger a kernel panic due to division by zero?

[ ... ]

> @@ -497,10 +497,8 @@ static int lamparray_register_led(struct lamparray_device *ldev)
>  		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) {
>  			mutex_unlock(&ldev->sysfs_lock);
>  			return -ENOMEM;
>  		}

[Severity: Medium]
This hardcodes the LED name based solely on the function.

If multiple identical LampArray devices are connected, won't this cause a
deterministic sysfs naming collision (-EEXIST) that prevents the second
device's LED interface from registering?

[ ... ]

> @@ -608,13 +608,10 @@ static ssize_t use_leds_uapi_store(struct device *dev,
>  
>  	if (val == old_val) {
>  		mutex_unlock(&ldev->dev_lock);
>  		return count;
>  	}
>  
>  	ldev->use_leds_uapi = val;
>  	mutex_unlock(&ldev->dev_lock);
>  
>  	if (val == 1) {
>  		ret = lamparray_register_led(ldev);

[Severity: Medium]
The protective dev_lock is dropped here before executing the actual
registration side effects.

Does this leave a race window where concurrent sysfs writes could cause the
internal use_leds_uapi state to completely desynchronize from the actual LED
registration state?

[ ... ]

> @@ -706,10 +706,8 @@ struct lamparray *
>  	ldev->use_leds_uapi = true;
>  	ldev->led_registered = false;
>  
>  	/* Make sure the driver lock gets released for probing. */
>  	hid_device_io_start(hdev);

[Severity: High]
Calling hid_device_io_start() here forcibly releases the HID core's
driver_input_lock during the probe sequence.

Could this allow incoming hardware interrupt events to race with the
unfinished probe() initialization of the parent driver, potentially leading
to uninitialized memory accesses or a use-after-free?

[ ... ]

> @@ -772,16 +772,13 @@ struct lamparray *
>  	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");

[Severity: Low]
This prints a success log message during normal device initialization.
Should this be removed to comply with the subsystem's logging rules against
printing success messages?

>  
>  	ret = lamparray_restore_state(ldev);
>  	if (ret) {
>  		hid_err(hdev, "Failed to set default state: %d", ret);
>  		goto err_remove_sysfs;
>  	}

[Severity: Medium]
If lamparray_restore_state() fails, this error path routes to
err_remove_sysfs. However, this bypasses restoring the hardware back to
autonomous mode via lamparray_hw_set_autonomous().

Does this leave the hardware in a frozen, non-autonomous state if the
initial state restoration times out or fails?

[ ... ]

> @@ -794,11 +794,9 @@ void lamparray_unregister(struct lamparray *la)
>  	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);
>  	lamparray_unregister_led(ldev);

[Severity: Medium]
This sets the hardware to autonomous mode before unregistering the userspace
interfaces (the LED class device and sysfs attributes).

Could userspace write to the sysfs brightness attribute simultaneously while
the driver is being unbound, sending manual LED commands while the hardware
is configured to operate autonomously?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903073602.3815258-1-aer@tuxedocomputers.com?part=1