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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] platform/x86: thinkpad_acpi: kbdlight_sysfs_set() emits
  hw_changed events
- [Medium] platform/x86: thinkpad_acpi: kbdlight_notify_level() skips
  brightness sync
--

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

platform/x86: thinkpad_acpi: Sync LED core state on level writes

This commit routes both the sysfs and proc keyboard backlight write paths
through kbdlight_set_level_and_update(), moving kbdlight_mutex acquisition
and kbdlight_notify_level() there instead of directly updating
led_cdev->brightness.

> diff --git a/drivers/platform/x86/lenovo/thinkpad_acpi.c b/drivers/platform/x86/lenovo/thinkpad_acpi.c
> index c8098f3175e51..657a19d11efd1 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.
> + */
>  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);
> +
>  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)

[Severity: Medium]
Does routing the LED class brightness_set_blocking callback
kbdlight_sysfs_set() through kbdlight_set_level_and_update() ->
kbdlight_notify_level() cause software-initiated brightness changes to
emit brightness_hw_changed notifications?

Per Documentation/ABI/testing/sysfs-class-led, brightness_hw_changed
reports only brightness changes initiated by hardware or firmware and must
return -ENODATA until a hardware change occurs.  Because the LED core
also invokes kbdlight_sysfs_set() during led_classdev_suspend() and
led_classdev_resume(), suspending with the keyboard backlight enabled now
updates kbdlight_brightness to 0 and wakes userspace poll() listeners on
both suspend and resume.

> @@ -5245,18 +5247,21 @@ static void kbdlight_exit(void)
>  	led_classdev_unregister(&tpacpi_led_kbdlight.led_classdev);
>  }
>  
> +static void kbdlight_notify_level(int level);
> +
>  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);
>  
> -	return ret;
> +	return 0;
>  }
>  
>  static int kbdlight_read(struct seq_file *m)

[Severity: Medium]
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.

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