Re: [PATCH v5 12/12] platform/x86: ideapad-laptop: Fully support auto keyboard backlight

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Hardware brightness state falls out of sync with `last_hw_brightness` due to a TOCTOU race between ACPI notify and sysfs write.
--

--- Patch [12]: [PATCH v5 12/12] platform/x86: ideapad-laptop: Fully support auto keyboard backlight ---
commit 775eb668f48ee8e4ee7f513342cb65f613086809
Author: Rong Zhang <[email protected]>

platform/x86: ideapad-laptop: Fully support auto keyboard backlight

This commit adds support for the auto brightness mode of the keyboard
backlight on IdeaPad laptops. It introduces a private hardware control
trigger to handle the auto mode and blocks brightness changes on exit
to retain state across boots.

> diff --git a/drivers/platform/x86/lenovo/ideapad-laptop.c b/drivers/platform/x86/lenovo/ideapad-laptop.c
> index 7f7b1e30c3263..5442cbfe3969b 100644
> --- a/drivers/platform/x86/lenovo/ideapad-laptop.c
> +++ b/drivers/platform/x86/lenovo/ideapad-laptop.c

[ ... ]

> @@ -1739,6 +1790,8 @@ static void ideapad_kbd_bl_notify(struct ideapad_private *priv)
>  	if (hw_brightness == last_hw_brightness)
>  		return;
>  
> +	ideapad_kbd_bl_notify_hw_control(priv, hw_brightness, last_hw_brightness);
> +

[Severity: Medium]
Can a race condition here cause the hardware brightness state to permanently
fall out of sync with the internal tracker?

Looking at ideapad_kbd_bl_notify(), it reads the current hardware brightness
and then updates the tracker using atomic_xchg():

ideapad_kbd_bl_notify() {
    ...
    hw_brightness = ideapad_kbd_bl_hw_brightness_get(priv);
    ...
    last_hw_brightness = atomic_xchg(&priv->kbd_bl.last_hw_brightness, hw_brightness);
}

If a concurrent sysfs write triggers ideapad_kbd_bl_hw_brightness_set()
in the middle of this process, could we see this sequence:

Thread 1 (ideapad_kbd_bl_notify):
    Reads hw_brightness (e.g., 1)

Thread 2 (sysfs write):
    Calls ideapad_kbd_bl_hw_brightness_set()
    Sets actual hardware brightness to 3 (auto mode)
    Calls atomic_set(&priv->kbd_bl.last_hw_brightness, 3)

Thread 1 (ideapad_kbd_bl_notify resumes):
    Calls atomic_xchg(&priv->kbd_bl.last_hw_brightness, 1)

Wouldn't Thread 1 blindly overwrite the correct atomic state (3) with the
stale read state (1)? 

This seems like it would permanently desync the last_hw_brightness tracker
from the actual hardware, breaking ideapad_kbd_bl_auto_trigger_offloaded()
which would now falsely report that the trigger is not offloaded.

>  	led_classdev_notify_brightness_hw_changed(&priv->kbd_bl.led, brightness);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=12