Re: [PATCH RFC 2/2] HID: lenovo-legion: Add keyboard lighting driver for Legion Gen 10

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential
issues to consider:
- [Medium] HID: lenovo-legion: stale led_cdev.brightness after hotkey
  events
- [Medium] HID: lenovo-legion: missing color application when pairing in
  legion_kbd_probe()
- [Medium] HID: lenovo-legion: broken color_set and rgb state tracking
  on write and error
- [Medium] HID: lenovo-legion: missing controller owner hand-back in
  legion_lamps_remove()
- [Medium] HID: lenovo-legion: race between legion_brightness_set() and
  lighting_mode_store()
--

commit f6469bc79f96d1ad122443bdd5d1613264a3381f
Author: Ciro Pedrini <ciro.pedrini@gmail.com>

HID: lenovo-legion: Add keyboard lighting driver for Legion Gen 10

This commit adds a HID driver for the keyboard and chassis lighting of
2025 Lenovo Legion laptops, pairing the ITE 8258 keyboard controller and
its companion HID LampArray into a single multicolor LED device.

> diff --git a/drivers/hid/hid-lenovo-legion.c b/drivers/hid/hid-lenovo-legion.c
> new file mode 100644
> index 0000000000000..923f03d4ed342
> --- /dev/null
> +++ b/drivers/hid/hid-lenovo-legion.c

[ ... ]

> +static int legion_brightness_set(struct led_classdev *led,
> +				 enum led_brightness value)
> +{
> +	struct led_classdev_mc *mc = lcdev_to_mccdev(led);
> +	struct legion_kbd *kbd = container_of(mc, struct legion_kbd, mc);
> +	u8 level = min_t(unsigned int, value, LEGION_MAX_BRIGHTNESS);
> +	u8 rgb[3];
> +	int i, ret;
> +
> +	mutex_lock(&kbd->lock);
> +	ret = legion_exchange(kbd, LEGION_OP_SET_BRIGHTNESS, &level, 1, NULL);
> +	mutex_unlock(&kbd->lock);
> +	if (ret)
> +		return ret;
> +
> +	/* A multi_intensity write ends up here too: apply a changed colour. */
> +	for (i = 0; i < 3; i++)
> +		rgb[i] = READ_ONCE(mc->subled_info[i].intensity);
> +
> +	mutex_lock(&legion_pair_lock);
> +	if (memcmp(rgb, kbd->rgb, sizeof(rgb))) {
> +		memcpy(kbd->rgb, rgb, sizeof(rgb));
> +		kbd->color_set = true;
> +		ret = legion_apply_color(kbd);
> +	}
> +	mutex_unlock(&legion_pair_lock);
> +
> +	return ret;
> +}

[Severity: Medium]
Does checking only memcmp(rgb, kbd->rgb, sizeof(rgb)) prevent switching to
"color" mode when the written multi_intensity matches kbd->rgb?

legion_kbd_probe() initializes both subleds[i].intensity and kbd->rgb[i]
to 255 while starting in "firmware" mode (color_set = false), and
switching back to "firmware" mode in lighting_mode_store() leaves kbd->rgb
untouched.  Writing 255 255 255 after probe, or re-writing the previous
colour after switching to "firmware" mode, finds memcmp() == 0 and leaves
the controller in firmware mode.

In addition, both legion_brightness_set() and lighting_mode_store() update
kbd->rgb and set kbd->color_set = true before legion_apply_color(kbd)
succeeds.  If the USB transfer in legion_apply_color() fails,
lighting_mode falsely reports "color" and retries of the same colour via
multi_intensity are skipped.

[ ... ]

> +static void legion_hw_changed_work(struct work_struct *work)
> +{
> +	struct legion_kbd *kbd = container_of(work, struct legion_kbd,
> +					      hw_changed_work);
> +
> +	led_classdev_notify_brightness_hw_changed(&kbd->mc.led_cdev,
> +						  READ_ONCE(kbd->hw_brightness));
> +}

[Severity: Medium]
When Fn+Up or Fn+Down changes the controller's brightness, does
kbd->mc.led_cdev.brightness stay at its old value?

led_classdev_notify_brightness_hw_changed() records the value in
led_cdev->brightness_hw_changed and notifies sysfs pollers, but does not
update led_cdev->brightness:

drivers/leds/led-class-multicolor.c:multi_intensity_store() {
    ...
	if (!test_bit(LED_BLINK_SW, &led_cdev->work_flags))
		led_set_brightness(led_cdev, led_cdev->brightness);
    ...
}

When userspace later writes a colour to multi_intensity,
multi_intensity_store() calls legion_brightness_set() with the stale
led_cdev->brightness and sends LEGION_OP_SET_BRIGHTNESS to the controller,
overwriting the hotkey-adjusted brightness (for example, turning the LEDs
back off if brightness started at 0 and was raised with Fn+Up).

[ ... ]

> +static ssize_t lighting_mode_store(struct device *dev,
> +				   struct device_attribute *attr,
> +				   const char *buf, size_t count)
> +{
> +	struct legion_kbd *kbd = led_dev_to_kbd(dev);
> +	struct led_classdev_mc *mc = &kbd->mc;
> +	int i, ret = 0;
> +
> +	mutex_lock(&legion_pair_lock);
> +	if (sysfs_streq(buf, "color")) {
> +		for (i = 0; i < 3; i++)
> +			kbd->rgb[i] = READ_ONCE(mc->subled_info[i].intensity);
> +		kbd->color_set = true;
> +		ret = legion_apply_color(kbd);
> +	} else if (sysfs_streq(buf, "firmware")) {
> +		kbd->color_set = false;
> +		legion_hand_back(kbd);
> +	} else {
> +		ret = -EINVAL;
> +	}
> +	mutex_unlock(&legion_pair_lock);
> +
> +	return ret ? ret : count;
> +}

[Severity: Medium]
Can an in-flight set_brightness_work race with lighting_mode_store() and
revert a switch to "firmware" mode?

Because the LED uses brightness_set_blocking, multi_intensity_store()
updates mc->subled_info[i].intensity, schedules set_brightness_work on
leds_wq, and returns immediately:

CPU0 (sysfs)                      CPU1 (leds_wq)
multi_intensity_store()
  subled_info[i].intensity = new
  schedule_work(set_brightness_work)
                                  legion_brightness_set()
                                    legion_exchange(SET_BRIGHTNESS)
                                    rgb[i] = subled_info[i].intensity
lighting_mode_store("firmware")
  mutex_lock(&legion_pair_lock)
  kbd->color_set = false
  legion_hand_back(kbd)
  mutex_unlock(&legion_pair_lock)
                                    mutex_lock(&legion_pair_lock)
                                    memcmp(rgb, kbd->rgb) != 0
                                    kbd->color_set = true
                                    legion_apply_color(kbd)  <-- reverts mode

Similarly, if LEGION_OP_SET_BRIGHTNESS fails during a multi_intensity
write, legion_brightness_set() returns before updating kbd->rgb, so a
subsequent brightness change while in "firmware" mode sees memcmp(rgb,
kbd->rgb) != 0 and switches lighting_mode to "color".

[ ... ]

> +	ret = led_classdev_multicolor_register(&hdev->dev, &kbd->mc);
> +	if (ret)
> +		goto err_close;
> +
> +	/* Added afterwards: the multicolor class replaces led_cdev.groups. */
> +	ret = device_add_groups(kbd->mc.led_cdev.dev, legion_led_groups);
> +	if (ret)
> +		goto err_unregister;
> +
> +	spin_lock_irq(&kbd->event_lock);
> +	kbd->ready = true;
> +	spin_unlock_irq(&kbd->event_lock);
> +
> +	mutex_lock(&legion_pair_lock);
> +	list_add(&kbd->node, &legion_kbds);
> +	list_for_each_entry(lamps, &legion_lamps_list, node) {
> +		if (lamps->bus == kbd->bus && !lamps->kbd) {
> +			lamps->kbd = kbd;
> +			kbd->lamps = lamps;
> +			hid_info(hdev, "paired with %s\n", dev_name(&lamps->hdev->dev));
> +			break;
> +		}
> +	}
> +	mutex_unlock(&legion_pair_lock);

[Severity: Medium]
What happens if userspace writes to multi_intensity or lighting_mode right
after the LED is registered, before legion_kbd_probe() pairs kbd with an
already-probed LampArray?

CPU0 (legion_kbd_probe)           CPU1 (udev / sysfs)
led_classdev_multicolor_register()
device_add_groups()
                                  lighting_mode_store("color")
                                    mutex_lock(&legion_pair_lock)
                                    kbd->color_set = true
                                    legion_apply_color(kbd)
                                      if (!kbd->lamps)
                                        return 0;
                                    mutex_unlock(&legion_pair_lock)
mutex_lock(&legion_pair_lock)
kbd->lamps = lamps
/* missing legion_apply_color() */
mutex_unlock(&legion_pair_lock)

Unlike legion_lamps_probe(), the pairing loop in legion_kbd_probe() does
not call legion_apply_color(kbd) when kbd->color_set is already true, so
the hardware stays in firmware mode while lighting_mode reports "color".
Calling legion_apply_color(kbd) when kbd->color_set is set upon pairing
(or pairing before registering the LED) would avoid this.

[ ... ]

> +static void legion_lamps_remove(struct hid_device *hdev)
> +{
> +	struct legion_lamps *lamps = hid_get_drvdata(hdev);
> +
> +	mutex_lock(&legion_pair_lock);
> +	if (lamps->kbd) {
> +		if (lamps->kbd->lamparray_owns)
> +			lamps_set_autonomous(lamps, true);
> +		lamps->kbd->lamps = NULL;
> +	}
> +	list_del(&lamps->node);
> +	mutex_unlock(&legion_pair_lock);
> +
> +	hid_hw_stop(hdev);
> +}

[Severity: Medium]
When the LampArray device is removed or unbound while lamparray_owns is
true, shouldn't legion_lamps_remove() also hand LED ownership back to the
keyboard controller?

legion_lamps_remove() puts the LampArray into autonomous mode, but leaves
the keyboard controller in LEGION_OWNER_LAMPARRAY (0xAA 00) and leaves
lamps->kbd->lamparray_owns set to true, which leaves the LEDs dark instead
of restoring the controller's effects as legion_hand_back() does.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109-legion-lighting-rfc-v1-0-a87e1db8c5e3@gmail.com?part=2