Re: [RFC PATCH v2 2/2] platform/x86: thinkpad_acpi: Add support for auto keyboard backlight

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] platform/x86: thinkpad_acpi: bad kbdlight_auto_trigger_hw_offloaded() check
- [High] platform/x86: thinkpad_acpi: race in kbdlight_auto_trigger_activate()
- [Medium] platform/x86: thinkpad_acpi: unsigned enum check in kbdlight_init()
- [Medium] platform/x86: thinkpad_acpi: unchecked level in kbdlight_notify_change()
- [Medium] platform/x86: thinkpad_acpi: stale level when auto trigger activate fails
- [Medium] platform/x86: thinkpad_acpi: async race in kbdlight_set_level_and_update()
--

commit 58813975928928e80e772abf45a021a9664adf5b
Author: Qiling Zhu <kmatzkaczor244@gmail.com>

platform/x86: thinkpad_acpi: Add support for auto keyboard backlight

This commit adds support for the ambient light sensor (ALS) automatic
keyboard backlight mode (EC level 3) on ThinkPads by exposing a private
"tpacpi-kbd-auto" hardware control LED trigger and synchronizing EC level
changes with the LED core.

> diff --git a/drivers/platform/x86/lenovo/thinkpad_acpi.c b/drivers/platform/x86/lenovo/thinkpad_acpi.c
> index 3260eaa620a0e..7b46ad6f9618c 100644
> --- a/drivers/platform/x86/lenovo/thinkpad_acpi.c
> +++ b/drivers/platform/x86/lenovo/thinkpad_acpi.c
> @@ -5102,6 +5102,14 @@ static struct ibm_struct video_driver_data = {
>   * Keyboard backlight subdriver
>   */
>  
> +/* EC level that selects ALS-controlled auto mode for the keyboard backlight */
> +#define TPACPI_KBD_LIGHT_AUTO_LEVEL	3
> +
> +/*
> + * Cached EC level, including TPACPI_KBD_LIGHT_AUTO_LEVEL. All state
> + * tracking compares EC levels; the auto mode is translated to a
> + * brightness value only when reporting to the LED core.
> + */
>  static enum led_brightness kbdlight_brightness;
>  static DEFINE_MUTEX(kbdlight_mutex);
>  
> @@ -5180,6 +5188,15 @@ static bool kbdlight_is_supported(void)
>  	return status & BIT(9);
>  }
>  
> +/* The auto mode is not a brightness level; report it as off to the LED core. */
> +static enum led_brightness kbdlight_level_to_brightness(int level)
> +{
> +	if (level == TPACPI_KBD_LIGHT_AUTO_LEVEL)
> +		return 0;
> +
> +	return level;
> +}
> +
>  static int kbdlight_sysfs_set(struct led_classdev *led_cdev,
>  			enum led_brightness brightness)
>  {
> @@ -5194,16 +5211,54 @@ static enum led_brightness kbdlight_sysfs_get(struct led_classdev *led_cdev)
>  	if (level < 0)
>  		return 0;
>  
> -	return level;
> +	return kbdlight_level_to_brightness(level);
> +}
> +
> +static struct led_hw_trigger_type kbdlight_auto_trigger_type;
> +
> +static int kbdlight_auto_trigger_activate(struct led_classdev *led_cdev)
> +{
> +	/*
> +	 * The EC may already be in auto mode (cycled there on Fn+Space, or
> +	 * active since before the driver loaded), in which case there is
> +	 * nothing to program.
> +	 */
> +	if (kbdlight_get_level() == TPACPI_KBD_LIGHT_AUTO_LEVEL)
> +		return 0;
> +
> +	if (kbdlight_set_level(TPACPI_KBD_LIGHT_AUTO_LEVEL))
> +		return -EIO;

[Severity: High]
Can a rapid Fn+Space sequence race with the asynchronous trigger activation
worker here and leave the EC stuck in auto mode while the trigger is detached?

When Fn+Space cycles into auto mode (level 3), kbdlight_notify_change() calls
led_trigger_notify_hw_control_changed(led_cdev, true), which schedules
trigger_hw_changed_work. Contrary to the comment in kbdlight_notify_change(),
led_trigger_hw_control_changed_worker() in drivers/leds/led-triggers.c does
not call hw_offloaded(); instead it runs:

  led_trigger_hw_control_changed_worker()
    led_trigger_do_hw_control_transition()
      __led_trigger_set(led_cdev, hc_trig, true)
        synchronize_rcu()
        flush_work(&led_cdev->set_brightness_work)
        kbdlight_auto_trigger_activate()

If a second Fn+Space press cycles the EC from level 3 to level 0 while the
first transition is pending or sleeping in synchronize_rcu(),
kbdlight_notify_change() sets trigger_hw_changed = false and re-queues
trigger_hw_changed_work.

When the first worker reaches kbdlight_auto_trigger_activate() in
drivers/platform/x86/lenovo/thinkpad_acpi.c, kbdlight_get_level() returns 0,
so kbdlight_auto_trigger_activate() calls
kbdlight_set_level(TPACPI_KBD_LIGHT_AUTO_LEVEL) and programs the EC back to
level 3. Immediately afterwards, the re-queued worker runs with
activate = false and calls __led_trigger_set(led_cdev, NULL, true), which
detaches the trigger without changing the hardware level (because
hw_triggered is true), leaving the hardware in auto mode while sysfs reports
the [none] trigger.

> +
> +	/*
> +	 * Check that the EC actually accepted the auto mode; on error the
> +	 * trigger core reverts the LED to the "none" trigger.
> +	 */
> +	if (kbdlight_get_level() != TPACPI_KBD_LIGHT_AUTO_LEVEL)
> +		return -EIO;

[Severity: Medium]
If the EC does not support auto mode and rejects level 3 when a user selects
tpacpi-kbd-auto via sysfs, does this leave kbdlight_brightness corrupted with
TPACPI_KBD_LIGHT_AUTO_LEVEL?

In drivers/platform/x86/lenovo/thinkpad_acpi.c, kbdlight_init() registers
kbdlight_auto_trigger unconditionally on all ThinkPads with a keyboard
backlight. When kbdlight_auto_trigger_activate() calls
kbdlight_set_level(TPACPI_KBD_LIGHT_AUTO_LEVEL), kbdlight_set_level()
unconditionally updates kbdlight_brightness:

	if (!acpi_evalf(hkey_handle, NULL, "MLCS", "dd", level))
		return -EIO;

	kbdlight_brightness = level;
	return 0;

When kbdlight_get_level() != TPACPI_KBD_LIGHT_AUTO_LEVEL then fails and
returns -EIO, kbdlight_brightness remains set to 3. Because __led_trigger_set()
only queues set_brightness_work asynchronously on activation failure (and skips
it if LED_SUSPENDED is set), a concurrent or subsequent kbdlight_notify_change()
call can observe kbdlight_brightness == 3 while the EC is at a non-auto level
and emit spurious hardware control and brightness change notifications.

> +
> +	return 0;
> +}
> +
> +static bool kbdlight_auto_trigger_hw_offloaded(struct led_classdev *led_cdev)
> +{
> +	return kbdlight_get_level() == TPACPI_KBD_LIGHT_AUTO_LEVEL;
> +}

[Severity: High]
Does returning false from kbdlight_auto_trigger_hw_offloaded() when the EC
is no longer at level 3 prevent the trigger from being removed?

In drivers/leds/led-triggers.c, __led_trigger_is_hw_controlled() already
checks that led_cdev->trigger is attached and matches
led_cdev->hw_control_trigger before calling trig->hw_offloaded(led_cdev).
The hw_offloaded() callback is meant to distinguish hardware offload from
software fallback for hybrid triggers, rather than re-polling whether the
hardware is still in auto mode.

Because kbdlight_auto_trigger_hw_offloaded() in
drivers/platform/x86/lenovo/thinkpad_acpi.c returns false as soon as the EC
leaves level 3, __led_trigger_is_hw_controlled() also returns false even
though kbdlight_auto_trigger is still the active trigger on led_cdev:

1. If CONFIG_LEDS_TRIGGERS_HW_CHANGED is disabled (THINKPAD_ACPI only selects
   LEDS_TRIGGERS), pressing Fn+Space to exit auto mode changes the EC level to
   0 while led_cdev->trigger remains kbdlight_auto_trigger. Subsequent writes
   to /sys/class/leds/tpacpi::kbd_backlight/brightness (via
   led_trigger_remove_hw_control()) or /proc/acpi/ibm/kbdlight (via
   kbdlight_set_level_and_update()) see led_trigger_is_hw_controlled() == false
   and never detach the trigger.

2. Even with CONFIG_LEDS_TRIGGERS_HW_CHANGED enabled, if the EC leaves level 3
   (for example across suspend/resume or via Fn+Space before the async notification
   runs) and userspace writes the matching level to /proc/acpi/ibm/kbdlight
   before kbdlight_notify_change() executes, kbdlight_set_level_and_update()
   sees led_trigger_is_hw_controlled() == false, skips
   led_trigger_remove_hw_control(), and calls kbdlight_set_level(level),
   updating kbdlight_brightness = level. When kbdlight_notify_change() runs
   next, kbdlight_brightness == level is already true, so it returns early
   without calling led_trigger_notify_hw_control_changed(led_cdev, false),
   leaving the LED stuck on tpacpi-kbd-auto.

[ ... ]

> @@ -5221,9 +5276,30 @@ static int __init kbdlight_init(struct ibm_init_struct *iibm)
>  		return -ENODEV;
>  	}
>  
> -	kbdlight_brightness = kbdlight_sysfs_get(NULL);
> +	kbdlight_brightness = kbdlight_get_level();
> +	if (kbdlight_brightness < 0)
> +		kbdlight_brightness = 0;

[Severity: Medium]
Since kbdlight_brightness is declared as enum led_brightness, is the
kbdlight_brightness < 0 check in kbdlight_init() always false?

In include/linux/leds.h, enum led_brightness only defines non-negative
constants (LED_OFF = 0 through LED_FULL = 255), so its underlying type is
unsigned int. Previously, kbdlight_sysfs_get() stored the return value of
kbdlight_get_level() in a signed int local variable and checked level < 0
before returning. Assigning kbdlight_get_level() directly to
kbdlight_brightness in drivers/platform/x86/lenovo/thinkpad_acpi.c means a
negative error code from kbdlight_get_level() wraps to a large unsigned value
and is not clamped to 0.

[ ... ]

> @@ -5246,9 +5322,22 @@ static int kbdlight_set_level_and_update(int level)
>  	int ret;
>  	struct led_classdev *led_cdev;
>  
> -	ret = kbdlight_set_level(level);
>  	led_cdev = &tpacpi_led_kbdlight.led_classdev;
>  
> +	/*
> +	 * Leaving auto mode through the proc interface has to drop the
> +	 * trigger, like writing a brightness through sysfs does. Route the
> +	 * new level through the LED core as well, so that it is applied
> +	 * after the LED_OFF queued by dropping the trigger.
> +	 */
> +	if (led_trigger_is_hw_controlled(led_cdev)) {
> +		led_trigger_remove_hw_control(led_cdev);
> +		led_set_brightness(led_cdev, level);
> +		return 0;
> +	}
> +
> +	ret = kbdlight_set_level(level);
> +
>  	if (ret == 0 && !(led_cdev->flags & LED_SUSPENDED))
>  		led_cdev->brightness = level;

[Severity: Medium]
Because tpacpi_led_kbdlight uses brightness_set_blocking, calling
led_trigger_remove_hw_control() and led_set_brightness() in
kbdlight_set_level_and_update() clears led_cdev->trigger and queues
set_brightness_work on leds_wq asynchronously without holding
led_cdev->led_access or flushing set_brightness_work.

Could this race with a second procfs write or with kbdlight_suspend()?

If a second write to /proc/acpi/ibm/kbdlight arrives before
set_brightness_work runs, led_trigger_is_hw_controlled(led_cdev) is already
false, so the second write calls kbdlight_set_level() synchronously, and
set_brightness_work from the first write then overwrites the EC level out of
order. Similarly, if kbdlight_suspend() runs before set_brightness_work
executes, kbdlight_get_level() in kbdlight_suspend() still sees
TPACPI_KBD_LIGHT_AUTO_LEVEL and returns early without calling
led_classdev_suspend(), allowing the pending work to program the backlight
during suspend.

[ ... ]

> @@ -5301,38 +5392,75 @@ static void kbdlight_suspend(void)

[ ... ]

>  static void kbdlight_notify_change(void)
>  {
>  	struct led_classdev *led_cdev = &tpacpi_led_kbdlight.led_classdev;
>  	enum led_brightness brightness;
> +	bool was_auto, is_auto;
> +	int level;
>  
>  	guard(mutex)(&kbdlight_mutex);
>  
>  	/*
> -	 * Check the brightness actually changed, setting the brightness
> -	 * through kbdlight_set_level() also triggers this event.
> +	 * Check the level actually changed, setting the brightness through
> +	 * kbdlight_set_level() also triggers this event.
>  	 */
> -	brightness = kbdlight_sysfs_get(NULL);
> -	if (kbdlight_brightness == brightness)
> +	level = kbdlight_get_level();
> +	if (kbdlight_brightness == level)
>  		return;
>  
> -	kbdlight_brightness = brightness;
> +	/*
> +	 * Notifying the trigger core is safe while holding
> +	 * kbdlight_mutex: the transition is only queued, and the worker
> +	 * re-reads the EC level (via hw_offloaded) after this critical
> +	 * section has been left.
> +	 */
> +	is_auto = level == TPACPI_KBD_LIGHT_AUTO_LEVEL;
> +	was_auto = kbdlight_brightness == TPACPI_KBD_LIGHT_AUTO_LEVEL;
> +	if (is_auto != was_auto)
> +		led_trigger_notify_hw_control_changed(led_cdev, is_auto);
> +
> +	brightness = kbdlight_level_to_brightness(level);
> +	kbdlight_brightness = level;
>  	led_classdev_notify_brightness_hw_changed(led_cdev, brightness);
>  }

[Severity: Medium]
What happens in kbdlight_notify_change() if kbdlight_get_level() returns a
negative error code on a hotkey notification or during kbdlight_resume()?

Previously, kbdlight_notify_change() called kbdlight_sysfs_get(NULL), which
checked if (level < 0). Now kbdlight_get_level() is called directly without an
error check. If kbdlight_get_level() fails and returns a negative value (such
as -EIO or -ENXIO):

1. is_auto evaluates to false, so if was_auto was true,
   led_trigger_notify_hw_control_changed(led_cdev, false) is called and drops
   the active auto trigger.

2. kbdlight_level_to_brightness(level) returns the negative error value cast to
   enum led_brightness, which is then stored in kbdlight_brightness and passed
   to led_classdev_notify_brightness_hw_changed(led_cdev, brightness), exposing
   a wrapped unsigned error value through brightness_hw_changed in sysfs.

Should kbdlight_notify_change() bail out when level < 0?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102151211.40701-1-kmatzkaczor244@gmail.com?part=2