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

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

[Alex Bogusta]

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.

Fixes: fac55d29581f ("platform/x86: asus-wmi: Add support for multiple kbd led handlers")
Signed-off-by: Alex Bogusta <alex@bogusta.com>
---
 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)
 {
 	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;
 
 	if (asus->driver->key_filter) {
 		asus->driver->key_filter(asus->driver, &code, &key_value,

base-commit: 98f039265e580291d5e76a11b2072541fbb1c393
-- 
2.56.0

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@bogusta.com>
> ---
>  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

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

[alex]

Hi Denis, 
thanks for the review!

On Monday, October 19th, 2026 at 5:52 PM, Denis Benato <denis.benato@linux.dev> wrote:

> Is the change of type really necessary? I think clamp_value does everything required already.
Hi Denis, thanks for the reply,

clamp_val() without the type change wouldn't fix the problem, as
at level 0, led_value - 1 is an unsigned type and wraps around. clamp_val() then clamps this to 3, setting the leds to max
brightness

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

My reasoning for int was consistency, as asus_hid_event() and
do_kbd_led_set() already do this arithmetic as int, and the comment
on enum led_brightness in leds.h describes it as obsolete.

That said, if you prefer keeping the enum, one could avoid the
wraparound instead with the call

	kbd_led_set_by_kbd(asus, led_value ? led_value - 1 : 0);

and keep the clamp in kbd_led_set_by_kbd() for the upper bound.

Thanks,
Alex

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

[Denis Benato]

On 10/25/26 06:46, alex@bogusta.com wrote:
> Hi Denis, 
> thanks for the review!
>
> On Monday, October 19th, 2026 at 5:52 PM, Denis Benato <denis.benato@linux.dev> wrote:
>
>> Is the change of type really necessary? I think clamp_value does everything required already.
> Hi Denis, thanks for the reply,
>
> clamp_val() without the type change wouldn't fix the problem, as
> at level 0, led_value - 1 is an unsigned type and wraps around. clamp_val() then clamps this to 3, setting the leds to max
> brightness
>
>> Honestly I don't like the change of type... I would prefer something that prevents the
>> bad thing from happening without changing the type.
> My reasoning for int was consistency, as asus_hid_event() and
> do_kbd_led_set() already do this arithmetic as int, and the comment
> on enum led_brightness in leds.h describes it as obsolete.
>
> That said, if you prefer keeping the enum, one could avoid the
> wraparound instead with the call
>
> 	kbd_led_set_by_kbd(asus, led_value ? led_value - 1 : 0);
>
> and keep the clamp in kbd_led_set_by_kbd() for the upper bound.

I see where this is going: either math done on an enum or an int converted to an enum.

I personally like neither, but any of the two will do to get the issue solved, so... If you happen
to make another version please add a comment somewhere to outline it so nobody gets the idea of
making the code looks better at the expense of functionality. Either with that comment or not:

Reviewed-by: Denis Benato <denis.benato@linux.dev>

> Thanks,
> Alex

Best regards,
Denis Benato