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

[Rong Zhang <i@rong.moe>]

Hi Qiling,

On Mon, 2026-11-02 at 23:10 +0800, Qiling Zhu wrote:
> Recent ThinkPads with an ambient light sensor (ALS) support an auto mode
> for the keyboard backlight: the EC controls the backlight according to
> ambient light. The mode is EC keyboard backlight level 3, above the two
> fixed levels exposed so far, and is currently reachable only through the
> Fn+Space shortcut, which is handled directly by the firmware.
> 
> Wire the auto mode up as a private hardware control trigger, named
> "tpacpi-kbd-auto", on top of the LED hw control trigger framework:
> 
>   - selecting the trigger writes level 3 (activate); the offloaded state
>     is derived from the current EC level
>   - when Fn+Space cycles into or out of auto mode, the driver notifies
>     the LED core about the hardware-initiated hardware control
>     transition, so the trigger state (shown bracketed in the trigger
>     sysfs attribute) follows along
>   - if the EC is already in auto mode at driver initialization, the
>     trigger is set as the default one
> 
> The auto mode is not a brightness level: max_brightness stays 2 and the
> LED core is always told a brightness of 0 while in auto mode. The proc
> interface keeps reporting the raw EC level, including 3, and keeps
> rejecting writes of 3 with -EINVAL; writing a regular level through it
> while in auto mode drops the trigger, like writing a brightness through
> sysfs does. In auto mode suspend and resume are left to the EC and the
> firmware; on resume the restored EC level is observed and reconciled.
> 
> LEDS_TRIGGERS is selected to keep the trigger functional in
> stripped-down configurations.
> 
> Deliberately unchanged: module unload still resets the backlight to off;
> retaining auto mode across unload is left for future work.
> 
> Note that Documentation/admin-guide/laptops/thinkpad-acpi.rst has no
> kbdlight section at all, so there is no in-tree documentation to update;
> userspace-facing semantics of hardware control triggers are documented
> in Documentation/leds/leds-class.rst.
> 
> Signed-off-by: Qiling Zhu <kmatzkaczor244@gmail.com>
> ---
>  drivers/platform/x86/lenovo/Kconfig         |   1 +
>  drivers/platform/x86/lenovo/thinkpad_acpi.c | 152 ++++++++++++++++++--
>  2 files changed, 141 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/platform/x86/lenovo/Kconfig b/drivers/platform/x86/lenovo/Kconfig
> index 516594993..77d159a3a 100644
> --- a/drivers/platform/x86/lenovo/Kconfig
> +++ b/drivers/platform/x86/lenovo/Kconfig
> @@ -83,6 +83,7 @@ config THINKPAD_ACPI
>  	select NVRAM
>  	select NEW_LEDS
>  	select LEDS_CLASS
> +	select LEDS_TRIGGERS
>  	select INPUT_SPARSEKMAP
>  	help
>  	  This is a driver for the IBM and Lenovo ThinkPad laptops. It adds
> diff --git a/drivers/platform/x86/lenovo/thinkpad_acpi.c b/drivers/platform/x86/lenovo/thinkpad_acpi.c
> index 3260eaa62..7b46ad6f9 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;

I don't think the device needs this, as kbdlight_set_level() should be
idempotent.

> +
> +	if (kbdlight_set_level(TPACPI_KBD_LIGHT_AUTO_LEVEL))
> +		return -EIO;
> +
> +	/*
> +	 * 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;

Nah. Please wait until Vishnu and Mark answer about the approach to
determine the support for auto mode.

> +
> +	return 0;
>  }
>  
> +static bool kbdlight_auto_trigger_hw_offloaded(struct led_classdev *led_cdev)
> +{
> +	return kbdlight_get_level() == TPACPI_KBD_LIGHT_AUTO_LEVEL;
> +}
> +
> +static struct led_trigger kbdlight_auto_trigger = {
> +	.name		= "tpacpi-kbd-auto",
> +	.trigger_type	= &kbdlight_auto_trigger_type,
> +	.activate	= kbdlight_auto_trigger_activate,
> +	.hw_offloaded	= kbdlight_auto_trigger_hw_offloaded,
> +};
> +
>  static struct tpacpi_led_classdev tpacpi_led_kbdlight = {
>  	.led_classdev = {
>  		.name		= "tpacpi::kbd_backlight",
>  		.max_brightness	= 2,
> -		.flags		= LED_BRIGHT_HW_CHANGED,
> +		.flags		= LED_BRIGHT_HW_CHANGED | LED_TRIG_HW_CHANGED,
>  		.brightness_set_blocking = &kbdlight_sysfs_set,
>  		.brightness_get	= &kbdlight_sysfs_get,
> +		.trigger_type	= &kbdlight_auto_trigger_type,
>  	}
>  };
>  
> @@ -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;
>  	tp_features.kbdlight = 1;
>  
> +	/*
> +	 * Register the auto-mode trigger before the LED classdev: if the
> +	 * EC is already in auto mode, the trigger can then be set as the
> +	 * default one and gets activated during LED classdev registration.
> +	 */
> +	rc = devm_led_trigger_register(&tpacpi_pdev->dev,
> +				       &kbdlight_auto_trigger);
> +	if (rc) {
> +		tp_features.kbdlight = 0;
> +		return rc;
> +	}
> +
> +	tpacpi_led_kbdlight.led_classdev.hw_control_trigger =
> +		kbdlight_auto_trigger.name;
> +
> +	if (kbdlight_brightness == TPACPI_KBD_LIGHT_AUTO_LEVEL)
> +		tpacpi_led_kbdlight.led_classdev.default_trigger =
> +			kbdlight_auto_trigger.name;
> +
>  	rc = led_classdev_register(&tpacpi_pdev->dev,
>  				   &tpacpi_led_kbdlight.led_classdev);
>  	if (rc < 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)) {

You don't need this, and it causes a TOCTOU issue between
is_hw_controlled() and remove_hw_control().

> +		led_trigger_remove_hw_control(led_cdev);
> +		led_set_brightness(led_cdev, level);

???

Can't you simply refactor and call kbdlight_notify_change() here?

In detail:

- PATCH 1: split kbdlight_notify_change() into a function that accepts a
hw brightness level and a wrapper that queries the hardware
- PATCH 2: call the former in kbdlight_set_level_and_update(), like what
commit 07f48f668fac ("platform/x86: ideapad-laptop: add FnLock LED class
device") did
- PATCH 3: add support for auto mode

With those changes, kbdlight_set_level_and_update() should automatically
removes the triggers without the need of poking the LED core harshly.

Besides, IMO the attribute should really be deprecated with a warning
stating that writing to the attribute may desynchronize the LED core, so
that we can reduce the maintenance burden instead of infinitely working
around all sorts of such issues [1]. I am not an expert at this driver,
so I will defer it to other maintainers.

> +		return 0;
> +	}
> +
> +	ret = kbdlight_set_level(level);
> +
>  	if (ret == 0 && !(led_cdev->flags & LED_SUSPENDED))
>  		led_cdev->brightness = level;

[1]: for example, here is a pre-existing race condition.

>  
> @@ -5293,6 +5382,8 @@ static int kbdlight_write(char *buf)
>  	return kbdlight_set_level_and_update(level);
>  }
>  
> +static void kbdlight_notify_change(void);
> +
>  static void kbdlight_suspend(void)
>  {
>  	struct led_classdev *led_cdev;
> @@ -5301,38 +5392,75 @@ static void kbdlight_suspend(void)
>  		return;
>  
>  	led_cdev = &tpacpi_led_kbdlight.led_classdev;
> +
> +	/*
> +	 * In auto mode the EC drives the backlight and the firmware turns
> +	 * it off for suspend and restores it on resume by itself; keep the
> +	 * LED core out of it entirely.
> +	 */
> +	if (kbdlight_get_level() == TPACPI_KBD_LIGHT_AUTO_LEVEL)
> +		return;
> +

As I've said, getting rid of led_classdev_{suspend,resume}() completely
when the device supports auto mode will make our life much easier.

>  	led_update_brightness(led_cdev);
>  	led_classdev_suspend(led_cdev);
>  }
>  
>  static void kbdlight_resume(void)
>  {
> +	struct led_classdev *led_cdev;
> +
>  	if (!tp_features.kbdlight)
>  		return;
>  
> -	led_classdev_resume(&tpacpi_led_kbdlight.led_classdev);
> +	led_cdev = &tpacpi_led_kbdlight.led_classdev;
> +
> +	/* Only balance a suspend that actually suspended the LED. */
> +	if (led_cdev->flags & LED_SUSPENDED) {
> +		led_classdev_resume(led_cdev);
> +		return;
> +	}
> +
> +	/*
> +	 * Auto mode was left running over suspend; observe what the
> +	 * firmware actually restored.
> +	 */
> +	kbdlight_notify_change();

If you'd like to do so, it should be in a separated patch, too.

Thanks,
Rong

>  }
>  
>  /*
> - * The EC cycles the keyboard backlight level itself on Fn+Space; check
> - * the current level and update the LED core state.
> + * The EC cycles the keyboard backlight level (and auto mode) itself on
> + * Fn+Space; check the current level and update the LED core state.
>   */
>  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);
>  }
>