Re: [PATCH 2/2] platform/x86: ideapad-laptop: add CapsLock/NumLock LED via EC

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Sleeping in atomic context due to improper LED brightness callback assignment.
- [Medium] Unsynchronized read-modify-write on the shared EC keyboard LED register.
- [Medium] Missing default triggers for CapsLock and NumLock LEDs.
--

commit 3bd8490b486c6791548a82ecc890f8f432c92886
Author: Rodnei Cilto <rodnei.cilto@gmail.com>

platform/x86: ideapad-laptop: add CapsLock/NumLock LED via EC

This commit adds support for physical CapsLock and NumLock LEDs on specific
Lenovo IdeaPad laptops (83RR/83SR). It maps the EC's offset 0xA1 bits 5 and
4 to CAPL and NUML and exposes them as led_classdev entries.

> diff --git a/drivers/platform/x86/lenovo/ideapad-laptop.c b/drivers/platform/x86/lenovo/ideapad-laptop.c
> --- a/drivers/platform/x86/lenovo/ideapad-laptop.c
> +++ b/drivers/platform/x86/lenovo/ideapad-laptop.c

[ ... ]

> +static int ideapad_kbd_led_ec_set(u8 bit, bool on)
> +{
> +	u8 val;
> +	int err;
> +
> +	err = ec_read(IDEAPAD_EC_KBD_LED_OFFSET, &val);
> +	if (err)
> +		return err;
> +	if (on)
> +		val |= bit;
> +	else
> +		val &= ~bit;
> +	err = ec_write(IDEAPAD_EC_KBD_LED_OFFSET, val);

[Severity: Medium]
Is this read-modify-write sequence safe against concurrent updates?

Since CapsLock and NumLock are exposed as separate led_classdev devices, the
LED subsystem protects their state with independent locks. If both LEDs are
toggled simultaneously, could two threads concurrently enter
ideapad_kbd_led_ec_set(), read the same initial value, and write it back,
causing one modification to overwrite and lose the other?

> +	if (err)
> +		return err;
> +	acpi_evaluate_object(NULL, IDEAPAD_ACPI_EC0_QDF_PATH, NULL, NULL);
> +	return 0;
> +}

[ ... ]

> +static int ideapad_kbd_leds_init(struct ideapad_private *priv)
> +{
> +	int err;
> +
> +	if (WARN_ON(priv->kbd_leds.initialized))
> +		return -EEXIST;
> +
> +	priv->kbd_leds.capslock.name           = "input::capslock";
> +	priv->kbd_leds.capslock.max_brightness = 1;
> +	priv->kbd_leds.capslock.brightness_set = ideapad_capslock_led_set;

[Severity: Medium]
Will this LED automatically synchronize with the system's actual keyboard lock
state? Without setting .default_trigger = "kbd-capslock", the LED subsystem
might not automatically link the keyboard input state to this LED, requiring
manual sysfs configuration.

[Severity: High]
Does this risk a scheduling while atomic panic? The LED subsystem often calls
the non-blocking brightness_set callback from atomic contexts, like softirq
for keyboard input triggers.

Since ideapad_capslock_led_set() calls ideapad_kbd_led_ec_set(), which uses
ec_read() and ec_write() that acquire mutexes and sleep, should this use
brightness_set_blocking instead to safely defer execution to a workqueue?

> +	priv->kbd_leds.capslock.brightness_get = ideapad_capslock_led_get;
> +	priv->kbd_leds.capslock.flags          = LED_RETAIN_AT_SHUTDOWN;
> +
> +	err = led_classdev_register(&priv->platform_device->dev,
> +				    &priv->kbd_leds.capslock);
> +	if (err)
> +		return err;
> +
> +	priv->kbd_leds.numlock.name            = "input::numlock";
> +	priv->kbd_leds.numlock.max_brightness  = 1;
> +	priv->kbd_leds.numlock.brightness_set  = ideapad_numlock_led_set;

[Severity: Medium]
Similarly, should this have .default_trigger = "kbd-numlock" set to ensure
the NumLock LED automatically tracks the physical keyboard state?

[Severity: High]
As with the CapsLock LED, does this need to use brightness_set_blocking to
avoid sleeping in atomic context when the trigger updates the LED?

> +	priv->kbd_leds.numlock.brightness_get  = ideapad_numlock_led_get;
> +	priv->kbd_leds.numlock.flags           = LED_RETAIN_AT_SHUTDOWN;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260628-ideapad-kbd-leds-v1-0-50b421d94927@gmail.com?part=2