Re: [PATCH v3 6/7] platform/x86: thinkpad_acpi: Add support for auto keyboard backlight

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] platform/x86: thinkpad_acpi: stale cache in
  kbdlight_auto_trigger_activate()
- [Medium] platform/x86: thinkpad_acpi: WARN_ON in
  kbdlight_notify_level() without trigger
- [Medium] platform/x86: thinkpad_acpi: kbdlight_sysfs_set() undoes
  trigger activation
--

commit 02361acafbf4f75fad24437cece57aebc1b2b8d5
Author: Qiling Zhu <kmatzkaczor244@gmail.com>

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

This commit registers a private "tpacpi-kbd-auto" LED hardware control
trigger for ThinkPad keyboard backlight auto mode (EC level 3) and wires up
hardware-initiated trigger and brightness state notifications.

> diff --git a/drivers/platform/x86/lenovo/thinkpad_acpi.c b/drivers/platform/x86/lenovo/thinkpad_acpi.c
> index f9e2d324db727..f11e614172881 100644
> --- a/drivers/platform/x86/lenovo/thinkpad_acpi.c
> +++ b/drivers/platform/x86/lenovo/thinkpad_acpi.c
> @@ -5106,7 +5106,12 @@ static struct ibm_struct video_driver_data = {
>  /* EC level that selects the auto mode for the keyboard backlight */
>  #define TPACPI_KBD_LIGHT_AUTO_LEVEL 3
>  
> -static enum led_brightness kbdlight_brightness;
> +/*
> + * 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 int kbdlight_brightness;
>  static DEFINE_MUTEX(kbdlight_mutex);
>  
>  /*
> @@ -5191,6 +5196,38 @@ static bool kbdlight_is_supported(void)
>  	return status & BIT(9);
>  }
>  
> +static struct led_hw_trigger_type kbdlight_auto_trigger_type;
> +
> +static enum led_brightness kbdlight_level_to_brightness(int level)
> +{
> +	/* Auto mode is not a brightness level; report it as off. */
> +	if (level == TPACPI_KBD_LIGHT_AUTO_LEVEL)
> +		return 0;
> +
> +	return level;
> +}
> +
> +static int kbdlight_auto_trigger_activate(struct led_classdev *led_cdev)
> +{
> +	guard(mutex)(&kbdlight_mutex);
> +
> +	return kbdlight_set_level(TPACPI_KBD_LIGHT_AUTO_LEVEL);
> +}
> +
> +static bool kbdlight_auto_trigger_hw_offloaded(struct led_classdev *led_cdev)
> +{
> +	guard(mutex)(&kbdlight_mutex);
> +
> +	return kbdlight_brightness == TPACPI_KBD_LIGHT_AUTO_LEVEL;
> +}

[Severity: High]
Does kbdlight_auto_trigger_activate() leave kbdlight_brightness stale when
switching into auto mode from a manual level?

Since commit "platform/x86: thinkpad_acpi: Sync LED core state on level
writes", kbdlight_set_level() only evaluates MLCS without updating
kbdlight_brightness.  When userspace selects "tpacpi-kbd-auto" via sysfs
while at level 0, 1, or 2, kbdlight_brightness remains at that manual
level.  kbdlight_auto_trigger_hw_offloaded() then returns false while the EC
is in auto mode, and a subsequent manual brightness write or Fn+Space press
fails to see was_auto == true in kbdlight_notify_level() (or returns early
at level == kbdlight_brightness), leaving the trigger state out of sync with
hardware.

[ ... ]

> @@ -5235,9 +5272,37 @@ 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;
>  
> +	if (tp_features.kbdlight_auto) {
> +		rc = devm_led_trigger_register(&tpacpi_pdev->dev,
> +					       &kbdlight_auto_trigger);
> +		if (rc) {
> +			/*
> +			 * Auto-mode trigger unavailable; basic backlight
> +			 * control keeps working.
> +			 */
> +			dev_warn(&tpacpi_pdev->dev,
> +				 "auto-mode trigger unavailable\n");
> +		} else {
> +			struct led_classdev *led_cdev =
> +				&tpacpi_led_kbdlight.led_classdev;
> +
> +			led_cdev->flags |= LED_TRIG_HW_CHANGED;
> +			led_cdev->trigger_type = &kbdlight_auto_trigger_type;
> +			led_cdev->hw_control_trigger =
> +				kbdlight_auto_trigger.name;
> +
> +			if (kbdlight_brightness == TPACPI_KBD_LIGHT_AUTO_LEVEL)
> +				led_cdev->default_trigger =
> +					kbdlight_auto_trigger.name;
> +		}
> +	}

[ ... ]

> @@ -5347,6 +5412,8 @@ static void kbdlight_resume(void)
>  static void kbdlight_notify_level(int level)
>  {
>  	struct led_classdev *led_cdev = &tpacpi_led_kbdlight.led_classdev;
> +	bool is_auto, was_auto;
> +	int brightness;
>  
>  	/*
>  	 * Check the brightness actually changed, setting the brightness
> @@ -5355,15 +5422,29 @@ static void kbdlight_notify_level(int level)
>  	if (level == kbdlight_brightness)
>  		return;
>  
> +	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);

[Severity: Medium]
Can this trigger the WARN_ON() in led_trigger_notify_hw_control_changed()
when the trigger was not registered?

If devm_led_trigger_register() fails in kbdlight_init() (or firmware cycles
through level 3 without reporting MLCG bit 20), LED_TRIG_HW_CHANGED,
hw_control_trigger, and trigger_type are not set on led_cdev, so calling
led_trigger_notify_hw_control_changed() hits:

drivers/leds/led-triggers.c:led_trigger_notify_hw_control_changed() {
	if (WARN_ON(!(led_cdev->flags & LED_TRIG_HW_CHANGED) ||
		    !led_cdev->hw_control_trigger || !led_cdev->trigger_type))
		return;
    ...
}

[Severity: Medium]
Can calling led_trigger_notify_hw_control_changed() from software brightness
writes undo trigger activation?

kbdlight_sysfs_set() calls kbdlight_set_level_and_update() ->
kbdlight_notify_level().  When userspace writes "tpacpi-kbd-auto" (or
"default") to the trigger sysfs attribute while the backlight is already in
auto mode, __led_trigger_set() tears down the active trigger via
led_set_brightness(led_cdev, LED_OFF) and flushes set_brightness_work right
before calling trig->activate():

__led_trigger_set(led_cdev, &kbdlight_auto_trigger, false)
  down_write(&led_cdev->trigger_lock)
  led_set_brightness(led_cdev, LED_OFF)
    queue_work(..., &led_cdev->set_brightness_work)
  flush_work(&led_cdev->set_brightness_work)
    ...
      kbdlight_sysfs_set(led_cdev, 0)
        kbdlight_notify_level(0)
          led_trigger_notify_hw_control_changed(led_cdev, false)
            schedule_work(&led_cdev->trigger_hw_changed_work)
  kbdlight_auto_trigger_activate(led_cdev)
    kbdlight_set_level(TPACPI_KBD_LIGHT_AUTO_LEVEL)
  up_write(&led_cdev->trigger_lock)

led_trigger_hw_control_changed_worker()
  __led_trigger_set(led_cdev, NULL, true)  <-- detaches trigger

Once trigger_lock is released, trigger_hw_changed_work runs with
activate == false and removes the trigger in software while leaving the EC
hardware in auto mode.

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