| 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: <5dmvgp4srucat7mfc5dalklsmujuldlbfe3jawi4uharmku4ab@yb44yaf5v4x4>
Date: Tue, 2 Dec 2025 12:26:59 +0100
From: Benjamin Tissoires <bentiss@...nel.org>
To: Antheas Kapenekakis <lkml@...heas.dev>
Cc: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
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>,
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 Dec 01 2025, Antheas Kapenekakis wrote:
> 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?
Sorry for the current situation. We were following the series without
much involvment as there were still changes requested here and there.
Anyway, we (Jiri and I) are fine for this series to be merged entirely
through the drivers-platform-x86 tree. The only HID changes are solely
located into hid-asus.c, so we can punt to you the patches for this
cycle if there is anything happening until this gets merge into Linus
tree and we can go back to normal.
Cheers,
Benjamin
>
> > --
> > 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