Re: [PATCH v3 3/7] platform/x86: thinkpad_acpi: Sync LED core state on level writes

[Rong Zhang <i@rong.moe>]

Hi Qiling,

The title is confusing. How about:

   Don't poke keyboard backlight's LED core brightness

On Sun, 2026-11-15 at 01:48 +0800, Qiling Zhu wrote:
> kbdlight_set_level_and_update() pokes the led_classdev brightness cache
> directly after writing the new EC level, without notifying userspace and
> without any locking.  Commit 06da5325d02e ("platform/x86: thinkpad_acpi:
> Stop setting led_classdev brightness directly") already removed this
> pattern from the sysfs path but had to keep it for the proc interface,
> which has no LED core path of its own.
> 
> Move the LED core state update into kbdlight_notify_level(): stop
> updating the cached level in kbdlight_set_level() and have both the
> sysfs and the proc write paths go through kbdlight_set_level_and_update(),
> which writes the EC level and updates the cache under kbdlight_mutex,
> and reports the new level through the LED core, like commit
> 07f48f668fac ("platform/x86: ideapad-laptop: add FnLock LED class
> device") did for the ideapad FnLock LED.
> 
> As a result, writing a level through either interface now emits a
> brightness_hw_changed notification, so userspace listeners see level
> changes from both of them.
> 
> Signed-off-by: Qiling Zhu <kmatzkaczor244@gmail.com>

Sashiko complains:

   Does replacing the led_cdev->brightness update with
   kbdlight_notify_level() leave tpacpi_led_kbdlight.led_classdev.brightness
   stale after writes to /proc/acpi/ibm/kbdlight?
   
   kbdlight_notify_level() calls
   led_classdev_notify_brightness_hw_changed(), which only sets led_cdev-
   >brightness_hw_changed and never updates led_cdev->brightness.  When
   kbdlight_write() changes the level via procfs, in-kernel consumers that
   read led_cdev->brightness without calling led_update_brightness(), such
   as led_set_software_blink(), will see the old brightness value.

IMO this is acceptable. Please add a section separated with `---' after
your Signed-off-by, explaining why this is acceptable.

> ---
>  drivers/platform/x86/lenovo/thinkpad_acpi.c | 23 +++++++++++++--------
>  1 file changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/platform/x86/lenovo/thinkpad_acpi.c b/drivers/platform/x86/lenovo/thinkpad_acpi.c
> index c8098f317..657a19d11 100644
> --- a/drivers/platform/x86/lenovo/thinkpad_acpi.c
> +++ b/drivers/platform/x86/lenovo/thinkpad_acpi.c
> @@ -5108,17 +5108,17 @@ static struct ibm_struct video_driver_data = {
>  static enum led_brightness kbdlight_brightness;
>  static DEFINE_MUTEX(kbdlight_mutex);
>  
> +/*
> + * Caller must hold kbdlight_mutex.
> + */

Annotate it with lockdep.

>  static int kbdlight_set_level(int level)
>  {
>  	if (!hkey_handle)
>  		return -ENXIO;
>  
> -	guard(mutex)(&kbdlight_mutex);
> -
>  	if (!acpi_evalf(hkey_handle, NULL, "MLCS", "dd", level))
>  		return -EIO;
>  
> -	kbdlight_brightness = level;
>  	return 0;
>  }
>  
> @@ -5183,10 +5183,12 @@ static bool kbdlight_is_supported(void)
>  	return status & BIT(9);
>  }
>  
> +static int kbdlight_set_level_and_update(int level);

The function is modified a lot. Let's simply move it.

> +
>  static int kbdlight_sysfs_set(struct led_classdev *led_cdev,
>  			enum led_brightness brightness)
>  {
> -	return kbdlight_set_level(brightness);
> +	return kbdlight_set_level_and_update(brightness);
>  }
>  
>  static enum led_brightness kbdlight_sysfs_get(struct led_classdev *led_cdev)
> @@ -5245,18 +5247,21 @@ static void kbdlight_exit(void)
>  	led_classdev_unregister(&tpacpi_led_kbdlight.led_classdev);
>  }
>  
> +static void kbdlight_notify_level(int level);

What not add it to the right position in PATCH 2?

> +
>  static int kbdlight_set_level_and_update(int level)
>  {
>  	int ret;
> -	struct led_classdev *led_cdev;
> +
> +	guard(mutex)(&kbdlight_mutex);
>  
>  	ret = kbdlight_set_level(level);
> -	led_cdev = &tpacpi_led_kbdlight.led_classdev;
> +	if (ret)
> +		return ret;
>  
> -	if (ret == 0 && !(led_cdev->flags & LED_SUSPENDED))
> -		led_cdev->brightness = level;
> +	kbdlight_notify_level(level);

Nah. I didn't mean this. We only want to notify brightness_hw_changed on
procfs kbdlight writes, but you are notifying it on sysfs LEDs writes.

Please call kbdlight_notify_level() in kbdlight_write() instead of
messing up with this function.

This function should only update kbdlight_brightness (when
!LED_SUSPENDED).

Thanks,
Rong

>  
> -	return ret;
> +	return 0;
>  }
>  
>  static int kbdlight_read(struct seq_file *m)