Re: [PATCH] platform/x86: asus-wmi: Clamp keyboard backlight level from hotkeys

[Denis Benato]

On 10/16/26 03:51, Alex Bogusta wrote:
> Pressing the keyboard backlight down key at level 0 drives the cached
> level negative, so the backlight then needs one extra up press per
> extra down press before it turns back on. brightness_hw_changed also
> reports out-of-range values: 4 after pressing up at the maximum level,
> and ENODATA or values like 4294967294 below 0.
>
> kbd_led_set_by_kbd() stores the new level without clamping it, and
> kbd_led_update_all() skips do_kbd_led_set() for negative levels and
> notifies userspace with the unclamped value. The level is computed in
> enum led_brightness, which is unsigned, so the subtraction at level 0
> wraps around.
>
> Compute the level as int and clamp it in kbd_led_set_by_kbd(), which
> covers every hotkey path.

Hi Alex,

thanks for taking a look at this!


> Fixes: fac55d29581f ("platform/x86: asus-wmi: Add support for multiple kbd led handlers")
> Signed-off-by: Alex Bogusta <alex@xxxxxxxxxxx>
> ---
>  drivers/platform/x86/asus-wmi.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index 9fb3fe4b161c..599a73862903 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -1949,10 +1949,10 @@ static int kbd_led_set(struct led_classdev *led_cdev, enum led_brightness value)
>  	return 0;
>  }
>  
> -static void kbd_led_set_by_kbd(struct asus_wmi *asus, enum led_brightness value)
> +static void kbd_led_set_by_kbd(struct asus_wmi *asus, int value)

Is the change of type really necessary? I think clamp_value does everything required already.

>  {
>  	scoped_guard(spinlock_irqsave, &asus_ref.lock) {
> -		asus->kbd_led_wk = value;
> +		asus->kbd_led_wk = clamp_val(value, 0, ASUS_EV_MAX_BRIGHTNESS);
>  		asus->kbd_led_notify = true;
>  	}
>  	queue_work(asus->led_workqueue, &asus->kbd_led_work);
> @@ -4668,9 +4668,9 @@ static int asus_wmi_get_event_code(union acpi_object *obj)
>  
>  static void asus_wmi_handle_event_code(int code, struct asus_wmi *asus)
>  {
> -	enum led_brightness led_value;
>  	unsigned int key_value = 1;
>  	bool autorelease = 1;
> +	int led_value;
>  

Honestly I don't like the change of type... I would prefer something that prevents the
bad thing from happening without changing the type.

>  	if (asus->driver->key_filter) {
>  		asus->driver->key_filter(asus->driver, &code, &key_value,
>
> base-commit: 98f039265e580291d5e76a11b2072541fbb1c393

Best regards,
Denis Benato