Re: [PATCH v3 7/7] platform/x86: thinkpad_acpi: Reconcile EC level on resume

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] platform/x86: thinkpad_acpi: stale level in
  kbdlight_auto_trigger_activate()
- [Medium] platform/x86: thinkpad_acpi: unguarded notify in
  kbdlight_notify_level()
--

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

platform/x86: thinkpad_acpi: Reconcile EC level on resume

This commit reads the keyboard backlight EC level on resume on auto-mode
machines and passes it through kbdlight_notify_change() to synchronize the
cached level and auto trigger state.

> diff --git a/drivers/platform/x86/lenovo/thinkpad_acpi.c b/drivers/platform/x86/lenovo/thinkpad_acpi.c
> index f11e614172881..802e0d53b1312 100644
> --- a/drivers/platform/x86/lenovo/thinkpad_acpi.c
> +++ b/drivers/platform/x86/lenovo/thinkpad_acpi.c
> @@ -5395,13 +5395,22 @@ static void kbdlight_suspend(void)
>  	led_classdev_suspend(led_cdev);
>  }
>  
> +static void kbdlight_notify_change(void);
> +
>  static void kbdlight_resume(void)
>  {
>  	if (!tp_features.kbdlight)
>  		return;
>  
> -	if (tp_features.kbdlight_auto)
> +	if (tp_features.kbdlight_auto) {
> +		/*
> +		 * The firmware may not restore auto mode: after hibernation
> +		 * from level 3 it comes back with the backlight off.  Check
> +		 * the EC level and reconcile.
> +		 */
> +		kbdlight_notify_change();
>  		return;
> +	}

[Severity: Medium]
Does kbdlight_notify_change() fail to deactivate the tpacpi-kbd-auto
trigger on resume when auto mode was enabled via sysfs rather than the
Fn+Space hotkey?

After commit "platform/x86: thinkpad_acpi: Sync LED core state on level
writes" moved the kbdlight_brightness update out of kbdlight_set_level()
into kbdlight_notify_level(), kbdlight_auto_trigger_activate() sets the EC
to TPACPI_KBD_LIGHT_AUTO_LEVEL without updating kbdlight_brightness:

drivers/platform/x86/lenovo/thinkpad_acpi.c:kbdlight_auto_trigger_activate() {
	guard(mutex)(&kbdlight_mutex);

	return kbdlight_set_level(TPACPI_KBD_LIGHT_AUTO_LEVEL);
}

kbdlight_brightness stays at the previous manual level (0, 1, or 2), so
kbdlight_auto_trigger_hw_offloaded() returns false while the trigger is
active, and on resume from hibernation kbdlight_notify_level() sees
was_auto as false (or returns early when kbdlight_brightness was 0) and
skips led_trigger_notify_hw_control_changed().

[Severity: Medium]
Can kbdlight_notify_change() trigger a WARN_ON() in
led_trigger_notify_hw_control_changed() if devm_led_trigger_register()
failed during kbdlight_init()?

When devm_led_trigger_register() fails, kbdlight_init() logs a warning and
leaves tp_features.kbdlight_auto set without setting LED_TRIG_HW_CHANGED,
trigger_type, or hw_control_trigger on tpacpi_led_kbdlight.led_classdev:

drivers/platform/x86/lenovo/thinkpad_acpi.c:kbdlight_init() {
...
	if (tp_features.kbdlight_auto) {
		rc = devm_led_trigger_register(&tpacpi_pdev->dev,
					       &kbdlight_auto_trigger);
		if (rc) {
			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;
...
}

Any transition into or out of TPACPI_KBD_LIGHT_AUTO_LEVEL in
kbdlight_notify_level() then calls led_trigger_notify_hw_control_changed()
on an unconfigured led_cdev and hits the WARN_ON() in
drivers/leds/led-triggers.c.  In addition, THINKPAD_ACPI in Kconfig does
not select LEDS_TRIGGERS_HW_CHANGED, so when CONFIG_LEDS_TRIGGERS_HW_CHANGED
is disabled, led_trigger_notify_hw_control_changed() compiles to a no-op
and kbdlight_resume() leaves the auto trigger active.

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