| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID:
<CAGwozwFb9HsT4DCDvASdyxBjO6rJoODbBchXt1GXmRD7gqZ1nA@mail.gmail.com>
Date: Wed, 15 Oct 2025 17:38:10 +0200
From: Antheas Kapenekakis <lkml@...heas.dev>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: platform-driver-x86@...r.kernel.org, linux-input@...r.kernel.org,
linux-kernel@...r.kernel.org, Jiri Kosina <jikos@...nel.org>,
Benjamin Tissoires <bentiss@...nel.org>,
Corentin Chary <corentin.chary@...il.com>,
"Luke D . Jones" <luke@...nes.dev>, Hans de Goede <hdegoede@...hat.com>,
Denis Benato <benato.denis96@...il.com>
Subject: Re: [PATCH v6 6/7] platform/x86: asus-wmi: add keyboard brightness
event handler
On Wed, 15 Oct 2025 at 14:19, Ilpo Järvinen
<ilpo.jarvinen@...ux.intel.com> wrote:
>
> On Mon, 13 Oct 2025, Antheas Kapenekakis wrote:
>
> > Currenlty, the keyboard brightness control of Asus WMI keyboards is
>
> There's a typo here but preferrably avoid "currently" altogether where
> possible.
>
> > handled in the kernel, which leads to the shortcut going from
> > brightness 0, to 1, to 2, and 3.
> >
> > However, for HID keyboards it is exposed as a key and handled by the
> > user's desktop environment. For the toggle button, this means that
> > brightness control becomes on/off. In addition, in the absence of a
> > DE, the keyboard brightness does not work.
> >
> > Therefore, expose an event handler for the keyboard brightness control
> > which can then be used by hid-asus.
> >
> > Reviewed-by: Luke D. Jones <luke@...nes.dev>
> > Tested-by: Luke D. Jones <luke@...nes.dev>
> > Signed-off-by: Antheas Kapenekakis <lkml@...heas.dev>
> > ---
> > drivers/platform/x86/asus-wmi.c | 41 +++++++++++++++++++++-
> > include/linux/platform_data/x86/asus-wmi.h | 13 +++++++
> > 2 files changed, 53 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> > index a2a7cd61fd59..58407a3b6d41 100644
> > --- a/drivers/platform/x86/asus-wmi.c
> > +++ b/drivers/platform/x86/asus-wmi.c
> > @@ -1579,6 +1579,45 @@ void asus_hid_unregister_listener(struct asus_hid_listener *bdev)
> > }
> > EXPORT_SYMBOL_GPL(asus_hid_unregister_listener);
> >
> > +static void do_kbd_led_set(struct led_classdev *led_cdev, int value);
> > +
> > +int asus_hid_event(enum asus_hid_event event)
> > +{
> > + unsigned long flags;
> > + int brightness;
> > +
> > + spin_lock_irqsave(&asus_ref.lock, flags);
> > + if (!asus_ref.asus || !asus_ref.asus->kbd_led_registered) {
>
> Please add a local variable for asus_ref.asus. Check other
> patches/functions too if its use is repeated in some function many times,
> the local var seems to be in order.
>
> > + spin_unlock_irqrestore(&asus_ref.lock, flags);
>
> Use guard() instead.
>
> > + return -EBUSY;
> > + }
> > + brightness = asus_ref.asus->kbd_led_wk;
> > +
> > + switch (event) {
> > + case ASUS_EV_BRTUP:
> > + brightness += 1;
> > + break;
> > + case ASUS_EV_BRTDOWN:
> > + brightness -= 1;
> > + break;
> > + case ASUS_EV_BRTTOGGLE:
> > + if (brightness >= ASUS_EV_MAX_BRIGHTNESS)
> > + brightness = 0;
> > + else
> > + brightness += 1;
> > + break;
> > + }
> > +
> > + do_kbd_led_set(&asus_ref.asus->kbd_led, brightness);
> > + led_classdev_notify_brightness_hw_changed(&asus_ref.asus->kbd_led,
> > + asus_ref.asus->kbd_led_wk);
> > +
> > + spin_unlock_irqrestore(&asus_ref.lock, flags);
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(asus_hid_event);
> > +
> > /*
> > * These functions actually update the LED's, and are called from a
> > * workqueue. By doing this as separate work rather than when the LED
> > @@ -1878,7 +1917,7 @@ static int asus_wmi_led_init(struct asus_wmi *asus)
> > asus->kbd_led.flags = LED_BRIGHT_HW_CHANGED;
> > asus->kbd_led.brightness_set = kbd_led_set;
> > asus->kbd_led.brightness_get = kbd_led_get;
> > - asus->kbd_led.max_brightness = 3;
> > + asus->kbd_led.max_brightness = ASUS_EV_MAX_BRIGHTNESS;
> > asus->kbd_led_avail = !kbd_led_read(asus, &led_val, NULL);
> >
> > if (asus->kbd_led_avail)
> > diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
> > index 1f85d76387a8..e78e0fbccede 100644
> > --- a/include/linux/platform_data/x86/asus-wmi.h
> > +++ b/include/linux/platform_data/x86/asus-wmi.h
> > @@ -168,6 +168,14 @@ struct asus_hid_listener {
> > void (*brightness_set)(struct asus_hid_listener *listener, int brightness);
> > };
> >
> > +enum asus_hid_event {
> > + ASUS_EV_BRTUP,
> > + ASUS_EV_BRTDOWN,
> > + ASUS_EV_BRTTOGGLE,
>
> Where does "BRT" come from. To me it doesn't associate with brightness
> (might be due to me being non-native). If there's a good reason why it's
> that way, fine but otherwise I suggest changing it so that it becomes
> easier to understand.
>
> It's not a big problem as is because the context in the code above allows
> decrypting the meaning but without the other names, I'd have been totally
> lost what it means.
Comes from e9809c0b9670 ("asus-wmi: add keyboard backlight support")
I matched it to the driver, other alternative is KBDILLUM. I will keep
it as BRT for now to match the current driver.
> > +};
> > +
> > +#define ASUS_EV_MAX_BRIGHTNESS 3
> > +
> > #if IS_REACHABLE(CONFIG_ASUS_WMI)
> > void set_ally_mcu_hack(enum asus_ally_mcu_hack status);
> > void set_ally_mcu_powersave(bool enabled);
> > @@ -176,6 +184,7 @@ int asus_wmi_evaluate_method(u32 method_id, u32 arg0, u32 arg1, u32 *retval);
> >
> > int asus_hid_register_listener(struct asus_hid_listener *cdev);
> > void asus_hid_unregister_listener(struct asus_hid_listener *cdev);
> > +int asus_hid_event(enum asus_hid_event event);
> > #else
> > static inline void set_ally_mcu_hack(enum asus_ally_mcu_hack status)
> > {
> > @@ -200,6 +209,10 @@ static inline int asus_hid_register_listener(struct asus_hid_listener *bdev)
> > static inline void asus_hid_unregister_listener(struct asus_hid_listener *bdev)
> > {
> > }
> > +static inline int asus_hid_event(enum asus_hid_event event)
> > +{
> > + return -ENODEV;
> > +}
> > #endif
> >
> > #endif /* __PLATFORM_DATA_X86_ASUS_WMI_H */
> >
>
> --
> i.
>
>
Powered by blists - more mailing lists