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

Hi all,

On Fri, 2026-08-21 at 17:27 +0000, [email protected] wrote:
> 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.

Well, the concern is valid, Thread 1 could disable the private trigger
after Thread 2 activating it, but the hardware state is still auto due to
__led_trigger_set being called with hw_triggered == true.

led_trigger_notify_hw_control_changed() calls
ideapad_kbd_bl_hw_brightness_set(), that's why the latter is not
protected by the newly introduced notif_mutex.

Maybe we would need to introduce a workqueue deferral mechanism to
resolve that, as Sashiko suggested in the reply to v4...

Thanks,
Rong

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