Re: [PATCH RFC v3 09/11] platform/x86: ideapad-laptop: Decouple hardware & classdev brightness for keyboard backlight

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The refactored initialization path drops hardware brightness validation, allowing registration of buggy hardware states.
--

--- Patch [9]: [PATCH RFC v3 09/11] platform/x86: ideapad-laptop: Decouple hardware & classdev brightness for keyboard backlight ---
commit c04be41ffaebded5f3d78556c570769ff717d019
Author: Rong Zhang <[email protected]>

platform/x86: ideapad-laptop: Decouple hardware & classdev brightness for keyboard backlight

This commit decouples the hardware brightness from the LED classdev
brightness for keyboard backlights. It converts the last hardware
brightness state into an atomic_t to prevent race conditions and refactors
the initialization path.

> 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

[ ... ]

> @@ -1700,21 +1745,30 @@ static int ideapad_kbd_bl_init(struct ideapad_private *priv)
>  	if (WARN_ON(priv->kbd_bl.initialized))
>  		return -EEXIST;
>  
> -	if (ideapad_kbd_bl_check_tristate(priv->kbd_bl.type))
> -		priv->kbd_bl.led.max_brightness = 2;
> -	else
> -		priv->kbd_bl.led.max_brightness = 1;
> +	hw_brightness = ideapad_kbd_bl_hw_brightness_get(priv);
> +	if (hw_brightness < 0)
> +		return hw_brightness;
>  
> -	brightness = ideapad_kbd_bl_brightness_get(priv);
> -	if (brightness < 0)
> -		return brightness;
> +	atomic_set(&priv->kbd_bl.last_hw_brightness, hw_brightness);

[Severity: Medium]
Does this refactored initialization path drop the validation of the initial
hardware brightness?

In the old code, ideapad_kbd_bl_brightness_get() validated the brightness
against max_brightness, returning -EINVAL if the firmware returned an invalid
value. This safely aborted the initialization.

In this new flow, ideapad_kbd_bl_hw_brightness_get() just returns the raw
hardware value without validation, because max_brightness is not set until
later in this function, and ideapad_kbd_bl_brightness_parse() is bypassed.

If the firmware boots in a buggy state with an out-of-range brightness, will
this allow the LED class device to register successfully, only to spam dmesg
with "Unknown keyboard backlight value" warnings on every subsequent user-space
read?

> -	priv->kbd_bl.last_brightness = brightness;
>  	priv->kbd_bl.led.name                    = "platform::" LED_FUNCTION_KBD_BACKLIGHT;
>  	priv->kbd_bl.led.brightness_get          = ideapad_kbd_bl_led_cdev_brightness_get;
>  	priv->kbd_bl.led.brightness_set_blocking = ideapad_kbd_bl_led_cdev_brightness_set;
>  	priv->kbd_bl.led.flags                   = LED_BRIGHT_HW_CHANGED | LED_RETAIN_AT_SHUTDOWN;
>  
> +	switch (priv->kbd_bl.type) {
> +	case KBD_BL_TRISTATE_AUTO:
> +	case KBD_BL_TRISTATE:
> +		priv->kbd_bl.led.max_brightness = 2;
> +		break;
> +	case KBD_BL_STANDARD:
> +		priv->kbd_bl.led.max_brightness = 1;
> +		break;
> +	default:
> +		/* This has already been validated by ideapad_check_features(). */
> +		unreachable();
> +	}
> +
>  	err = led_classdev_register(&priv->platform_device->dev, &priv->kbd_bl.led);
>  	if (err)
>  		return err;

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