Re: [PATCH v5 10/12] platform/x86: ideapad-laptop: Decouple hardware & classdev brightness for 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:
> 
> Pre-existing issues:
> - [High] The conversion of `last_hw_brightness` to an `atomic_t` fails to prevent a TOCTOU (Time-Of-Check to Time-Of-Use) race condition between ACPI notifications and sysfs brightness writes, violating core kernel locking rules.

Please see my reply to Sashiko's review to PATCH 12.

Thanks,
Rong

> --
> 
> --- Patch [10]: [PATCH v5 10/12] platform/x86: ideapad-laptop: Decouple hardware & classdev brightness for keyboard backlight ---
> commit b73007a6498db545468f511c09a496e09dfcac31
> Author: Rong Zhang <[email protected]>
> 
> platform/x86: ideapad-laptop: Decouple hardware & classdev brightness for keyboard backlight
> 
> This commit decouples hardware brightness from LED classdev brightness for
> the keyboard backlight. It also converts last_hw_brightness into an atomic_t
> to help prevent potential race conditions.
> 
> > diff --git a/drivers/platform/x86/lenovo/ideapad-laptop.c b/drivers/platform/x86/lenovo/ideapad-laptop.c
> > --- a/drivers/platform/x86/lenovo/ideapad-laptop.c
> > +++ b/drivers/platform/x86/lenovo/ideapad-laptop.c
> [ ... ]
> > @@ -1637,32 +1676,37 @@ static enum led_brightness ideapad_kbd_bl_led_cdev_brightness_get(struct led_cla
> >  	return ideapad_kbd_bl_brightness_get(priv);
> >  }
> >  
> > -static int ideapad_kbd_bl_brightness_set(struct ideapad_private *priv, unsigned int brightness)
> > +static int ideapad_kbd_bl_hw_brightness_set(struct ideapad_private *priv, int hw_brightness)
> >  {
> [ ... ]
> >  	if (err)
> >  		return err;
> >  
> > -	priv->kbd_bl.last_brightness = brightness;
> > +	atomic_set(&priv->kbd_bl.last_hw_brightness, hw_brightness);
> >  
> >  	return 0;
> >  }
> [ ... ]
> > @@ -1673,26 +1717,29 @@ static int ideapad_kbd_bl_led_cdev_brightness_set(struct led_classdev *led_cdev,
> >  
> >  static void ideapad_kbd_bl_notify(struct ideapad_private *priv)
> >  {
> > -	int brightness;
> > +	int hw_brightness, brightness, last_hw_brightness;
> >  
> >  	if (!priv->kbd_bl.initialized)
> >  		return;
> >  
> > -	brightness = ideapad_kbd_bl_brightness_get(priv);
> > -	if (brightness < 0)
> > +	hw_brightness = ideapad_kbd_bl_hw_brightness_get(priv);
> > +	if (hw_brightness < 0)
> >  		return;
> >  
> > -	if (brightness == priv->kbd_bl.last_brightness)
> > -		return;
> > +	brightness = ideapad_kbd_bl_brightness_parse(priv, hw_brightness);
> > +	if (brightness < 0)
> > +		return; /* Reject insane values early. */
> >  
> > -	priv->kbd_bl.last_brightness = brightness;
> > +	last_hw_brightness = atomic_xchg(&priv->kbd_bl.last_hw_brightness, hw_brightness);
> 
> [Severity: High]
> This is a pre-existing issue, but does converting last_hw_brightness to an
> atomic_t fully prevent a time-of-check to time-of-use race condition here?
> 
> Since the hardware read and the atomic_xchg() are separate steps, could
> the following sequence occur?
> 
> CPU1 in ideapad_kbd_bl_notify():
>     hw_brightness = ideapad_kbd_bl_hw_brightness_get(priv);
> 
> CPU2 in ideapad_kbd_bl_hw_brightness_set() via concurrent sysfs write:
>     exec_kblc(priv->adev->handle, value);
>     atomic_set(&priv->kbd_bl.last_hw_brightness, hw_brightness);
> 
> CPU1 resumes in ideapad_kbd_bl_notify():
>     last_hw_brightness = atomic_xchg(&priv->kbd_bl.last_hw_brightness, hw_brightness);
> 
> In this scenario, wouldn't CPU1 overwrite the software cache with a stale
> hardware state, causing it to become permanently desynchronized from the
> actual hardware?
> 
> Although follow-up patches in this series introduce a notif_mutex, does
> the sysfs write path ever acquire this mutex to serialize these updates
> properly?
> 
> > +	if (hw_brightness == last_hw_brightness)
> > +		return;
> >  
> >  	led_classdev_notify_brightness_hw_changed(&priv->kbd_bl.led, brightness);
> >  }