| 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:
<CAGwozwGquTikG3-7oJxQWDt6a836FU-Lk+M3JtM6efaTCc0Vng@mail.gmail.com>
Date: Mon, 1 Dec 2025 12:44:18 +0100
From: Antheas Kapenekakis <lkml@...heas.dev>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: Denis Benato <benato.denis96@...il.com>,
platform-driver-x86@...r.kernel.org,
linux-input@...r.kernel.org, LKML <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 <hansg@...nel.org>
Subject: Re: [PATCH v10 10/11] platform/x86: asus-wmi: add keyboard brightness
event handler
On Mon, 1 Dec 2025 at 10:52, Ilpo Järvinen
<ilpo.jarvinen@...ux.intel.com> wrote:
>
> On Mon, 1 Dec 2025, Antheas Kapenekakis wrote:
> > On Wed, 26 Nov 2025 at 21:39, Antheas Kapenekakis <lkml@...heas.dev> wrote:
> > > On Wed, 26 Nov 2025 at 21:23, Denis Benato <benato.denis96@...il.com> wrote:
> > > > On 11/22/25 12:00, Antheas Kapenekakis wrote:
> > > > > The keyboard brightness control of Asus WMI keyboards is handled in
> > > > > 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. Since this handler is called from
> > > > > an interrupt context, defer the actual work to a workqueue.
> > > > >
> > > > > In the process, introduce ASUS_EV_MAX_BRIGHTNESS to hold the constant
> > > > > for maximum brightness since it is shared between hid-asus/asus-wmi.
> > > > >
> > > > > 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 | 46 +++++++++++++++++++---
> > > > > include/linux/platform_data/x86/asus-wmi.h | 13 ++++++
> > > > > 2 files changed, 54 insertions(+), 5 deletions(-)
> > > > >
> > > > > diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> > > > > index 84cde34ab6a8..a69464e45ca4 100644
> > > > > --- a/drivers/platform/x86/asus-wmi.c
> > > > > +++ b/drivers/platform/x86/asus-wmi.c
> > > > > @@ -1719,6 +1719,44 @@ static void kbd_led_update_all(struct work_struct *work)
> > > > > }
> > > > > }
> > > > >
> > > > > +/*
> > > > > + * This function is called from hid-asus to inform asus-wmi of brightness
> > > > > + * changes initiated by the keyboard backlight keys.
> > > > > + */
> > > > > +int asus_hid_event(enum asus_hid_event event)
> > > > > +{
> > > > > + struct asus_wmi *asus;
> > > > > + int brightness;
> > > > > +
> > > > > + guard(spinlock_irqsave)(&asus_ref.lock);
> > > > > + asus = asus_ref.asus;
> > > > > + if (!asus || !asus->kbd_led_registered)
> > > > > + return -EBUSY;
> > > > > +
> > > > > + brightness = 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;
> > > > > + }
> > > > > +
> > > > > + asus->kbd_led_wk = clamp_val(brightness, 0, ASUS_EV_MAX_BRIGHTNESS);
> > > > > + asus->kbd_led_notify = true;
> > > > > + queue_work(asus->led_workqueue, &asus->kbd_led_work);
> > > > > + return 0;
> > > > > +}
> > > > > +EXPORT_SYMBOL_GPL(asus_hid_event);
> > > > > +
> > > > > /*
> > > > > * These functions actually update the LED's, and are called from a
> > > > LEDs as opposed to LED's?
> > >
> > > I agree with you, but the author of that line probably wouldn't -
> > > depends on author dialect and both are usually correct.
> > >
> > > When making acronyms plural, adding a ' is usually accepted as
> > > correct. But this is not added as part of this series, so you can do a
> > > reword commit if you wish
> > >
> > > Antheas
> >
> > Hm, perhaps this was not clear but the part you commented on was not
> > part of the patch/this series. Do you want to finish reviewing this
> > patch and add a rev-by?
> >
> > @Ilpo: with 6.18 releasing yesterday, what is the status on this? is
> > it for 6.20? Hans commented on patch 5
>
> It will have to wait to the 6.20 cycle as it's cross-subsystem series so
> there are other maintainers involved than just me.
Unfortunate. Jiri is cc'd on this series and we had around 2 weeks
where the code was frozen. Was it my responsibility to ask for acks so
it can go through x86?
> --
> i.
>
> > Thanks,
> > Antheas
> >
> > > > > * workqueue. By doing this as separate work rather than when the LED
> > > > > @@ -1801,13 +1839,11 @@ static void do_kbd_led_set(struct led_classdev *led_cdev, int value)
> > > > > {
> > > > > struct asus_hid_listener *listener;
> > > > > struct asus_wmi *asus;
> > > > > - int max_level;
> > > > >
> > > > > asus = container_of(led_cdev, struct asus_wmi, kbd_led);
> > > > > - max_level = asus->kbd_led.max_brightness;
> > > > >
> > > > > scoped_guard(spinlock_irqsave, &asus_ref.lock)
> > > > > - asus->kbd_led_wk = clamp_val(value, 0, max_level);
> > > > > + asus->kbd_led_wk = clamp_val(value, 0, ASUS_EV_MAX_BRIGHTNESS);
> > > > >
> > > > > if (asus->kbd_led_avail)
> > > > > kbd_led_update(asus);
> > > > > @@ -2011,7 +2047,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);
> > > > > INIT_WORK(&asus->kbd_led_work, kbd_led_update_all);
> > > > >
> > > > > @@ -4530,7 +4566,7 @@ static void asus_wmi_handle_event_code(int code, struct asus_wmi *asus)
> > > > > return;
> > > > > }
> > > > > if (code == NOTIFY_KBD_BRTTOGGLE) {
> > > > > - if (led_value == asus->kbd_led.max_brightness)
> > > > > + if (led_value == ASUS_EV_MAX_BRIGHTNESS)
> > > > > kbd_led_set_by_kbd(asus, 0);
> > > > > else
> > > > > kbd_led_set_by_kbd(asus, led_value + 1);
> > > > > diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
> > > > > index d347cffd05d5..7b872b5d0960 100644
> > > > > --- a/include/linux/platform_data/x86/asus-wmi.h
> > > > > +++ b/include/linux/platform_data/x86/asus-wmi.h
> > > > > @@ -178,6 +178,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,
> > > > > +};
> > > > > +
> > > > > +#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);
> > > > > @@ -186,6 +194,7 @@ int asus_wmi_set_devstate(u32 dev_id, u32 ctrl_param, u32 *retval);
> > > > > 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)
> > > > > {
> > > > > @@ -213,6 +222,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 */
> > > >
> >
>
Powered by blists - more mailing lists