| 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: <CAGwozwFDeneALZ_-kOXQ70kg3OQ5BK8ANJrj+32hLHK_PMqVNQ@mail.gmail.com>
Date: Sat, 22 Mar 2025 09:12:50 +0100
From: Antheas Kapenekakis <lkml@...heas.dev>
To: "Luke D. Jones" <luke@...nes.dev>
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>,
Hans de Goede <hdegoede@...hat.com>, Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Subject: Re: [PATCH 08/11] platform/x86: asus-wmi: add keyboard brightness
event handler
On Sat, 22 Mar 2025 at 05:31, Luke D. Jones <luke@...nes.dev> wrote:
>
> On 21/03/25 11:09, Antheas Kapenekakis wrote:
> > Currenlty, the keyboard brightness control of Asus WMI keyboards is
> > 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.
> >
> > Signed-off-by: Antheas Kapenekakis <lkml@...heas.dev>
> > ---
> > drivers/platform/x86/asus-wmi.c | 38 ++++++++++++++++++++++
> > include/linux/platform_data/x86/asus-wmi.h | 11 +++++++
> > 2 files changed, 49 insertions(+)
> >
> > diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> > index 21e034be71b2f..45999dda9e7ed 100644
> > --- a/drivers/platform/x86/asus-wmi.c
> > +++ b/drivers/platform/x86/asus-wmi.c
> > @@ -1529,6 +1529,44 @@ void asus_brt_unregister_listener(struct asus_brt_listener *bdev)
> > }
> > EXPORT_SYMBOL_GPL(asus_brt_unregister_listener);
> >
> > +static void do_kbd_led_set(struct led_classdev *led_cdev, int value);
> > +
> > +int asus_brt_event(enum asus_brt_event event)
> > +{
> > + int brightness;
> > +
> > + mutex_lock(&asus_brt_lock);
> > + if (!asus_brt_ref || !asus_brt_ref->kbd_led_registered) {
> > + mutex_unlock(&asus_brt_lock);
> > + return -EBUSY;
> > + }
> > + brightness = asus_brt_ref->kbd_led_wk;
> > +
> > + switch (event) {
> > + case ASUS_BRT_UP:
> > + brightness += 1;
> > + break;
> > + case ASUS_BRT_DOWN:
> > + brightness -= 1;
> > + break;
> > + case ASUS_BRT_TOGGLE:
> > + if (brightness >= 3)
> > + brightness = 0;
> > + else
> > + brightness += 1;
> > + break;
> > + }
> > +
> > + do_kbd_led_set(&asus_brt_ref->kbd_led, brightness);
> > + led_classdev_notify_brightness_hw_changed(&asus_brt_ref->kbd_led,
> > + asus_brt_ref->kbd_led_wk);
> > +
> > + mutex_unlock(&asus_brt_lock);
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(asus_brt_event);
> > +
>
> I quick test on 6.14-rc7 gives me this:
>
> [ 288.039255] BUG: sleeping function called from invalid context at
> kernel/locking/mutex.c:258
> [ 288.039262] in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 0,
> name: swapper/17
> [ 288.039263] preempt_count: 101, expected: 0
> [ 288.039264] RCU nest depth: 0, expected: 0
> [ 288.039266] CPU: 17 UID: 0 PID: 0 Comm: swapper/17 Tainted: G
> W 6.14.0-rc7+ #164
> [ 288.039268] Tainted: [W]=WARN
> [ 288.039269] Hardware name: ASUSTeK COMPUTER INC. ROG Zephyrus M16
> GU604VY_GU604VY_00130747B/GU604VY, BIOS GU604VY.313 08/13/2024
> [ 288.039270] Call Trace:
> [ 288.039272] <IRQ>
> [ 288.039273] dump_stack_lvl+0x5d/0x80
> [ 288.039277] __might_resched.cold+0xba/0xc9
> [ 288.039282] mutex_lock+0x19/0x40
> [ 288.039285] asus_brt_event+0x13/0xb0 [asus_wmi]
> [ 288.039292] asus_event+0x91/0xa0 [hid_asus]
> [ 288.039295] hid_process_event+0xae/0x120
> [ 288.039298] hid_input_array_field+0x132/0x180
> [ 288.039300] hid_report_raw_event+0x360/0x4e0
> [ 288.039302] __hid_input_report.constprop.0+0xf1/0x180
> [ 288.039304] hid_irq_in+0x17f/0x1b0
> [ 288.039306] __usb_hcd_giveback_urb+0x98/0x110
> [ 288.039308] usb_giveback_urb_bh+0xbd/0x150
> [ 288.039310] process_one_work+0x171/0x290
> [ 288.039312] bh_worker+0x1ac/0x210
> [ 288.039314] tasklet_hi_action+0xe/0x30
> [ 288.039315] handle_softirqs+0xdb/0x1f0
> [ 288.039317] __irq_exit_rcu+0xc2/0xe0
> [ 288.039318] common_interrupt+0x85/0xa0
> [ 288.039320] </IRQ>
> [ 288.039320] <TASK>
> [ 288.039321] asm_common_interrupt+0x26/0x40
> [ 288.039323] RIP: 0010:cpuidle_enter_state+0xb9/0x2c0
> [ 288.039325] Code: 40 0f 84 1b 01 00 00 e8 35 e8 13 ff e8 40 f2 ff ff
> 31 ff 49 89 c5 e8 c6 f0 12 ff 45 84 f6 0f 85 f2 00 00 00 fb 0f 1f 44 00
> 00 <45> 85 ff 0f 88 cf 00 00 00 49 63 f7 48 8d 04 76 48 8d 04 86 49 8d
> [ 288.039326] RSP: 0018:ffffc90000253e90 EFLAGS: 00000246
> [ 288.039328] RAX: ffff888890680000 RBX: 0000000000000003 RCX:
> 0000000000000000
> [ 288.039329] RDX: 00000043107862af RSI: fffffffd616e8210 RDI:
> 0000000000000000
> [ 288.039329] RBP: ffff8888906ba370 R08: 0000000000000000 R09:
> 0000000000000007
> [ 288.039330] R10: ffff88888ffa6098 R11: 0000000000000008 R12:
> ffffffff82fd4140
> [ 288.039331] R13: 00000043107862af R14: 0000000000000000 R15:
> 0000000000000003
> [ 288.039332] cpuidle_enter+0x28/0x40
> [ 288.039334] do_idle+0x1a8/0x200
> [ 288.039336] cpu_startup_entry+0x24/0x30
> [ 288.039337] start_secondary+0x11e/0x140
> [ 288.039340] common_startup_64+0x13e/0x141
> [ 288.039342] </TASK>
>
> I think you need to swap the mutex to a spin_lock_irqsave and associated
> spin_unlock_irqrestore plus DEFINE_SPINLOCK(asus_brt_lock).
>
> I'm out of time tonight but I'll apply the above to your patches and
> report back tomorrow if you don't get to it before I do.
>
> It might be worth checking any other mutex uses in the LED path too.
Thank you for catching that, I will replace the mutex with a spinlock.
Might have to do with the WMI method being active as I got no such
issue in my device.
I guess I will try to do a v3 today as that will hold back our kernel too.
> Cheers,
> Luke.
>
> > /*
> > * These functions actually update the LED's, and are called from a
> > * workqueue. By doing this as separate work rather than when the LED
> > diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
> > index add04524031d8..2ac7912d8acd3 100644
> > --- a/include/linux/platform_data/x86/asus-wmi.h
> > +++ b/include/linux/platform_data/x86/asus-wmi.h
> > @@ -162,11 +162,18 @@ struct asus_brt_listener {
> > void (*notify)(struct asus_brt_listener *listener, int brightness);
> > };
> >
> > +enum asus_brt_event {
> > + ASUS_BRT_UP,
> > + ASUS_BRT_DOWN,
> > + ASUS_BRT_TOGGLE,
> > +};
> > +
> > #if IS_REACHABLE(CONFIG_ASUS_WMI)
> > int asus_wmi_evaluate_method(u32 method_id, u32 arg0, u32 arg1, u32 *retval);
> >
> > int asus_brt_register_listener(struct asus_brt_listener *cdev);
> > void asus_brt_unregister_listener(struct asus_brt_listener *cdev);
> > +int asus_brt_event(enum asus_brt_event event);
> > #else
> > static inline int asus_wmi_evaluate_method(u32 method_id, u32 arg0, u32 arg1,
> > u32 *retval)
> > @@ -181,6 +188,10 @@ static inline int asus_brt_register_listener(struct asus_brt_listener *bdev)
> > static inline void asus_brt_unregister_listener(struct asus_brt_listener *bdev)
> > {
> > }
> > +static inline int asus_brt_event(enum asus_brt_event event)
> > +{
> > + return -ENODEV;
> > +}
> > #endif
> >
> > #endif /* __PLATFORM_DATA_X86_ASUS_WMI_H */
>
Powered by blists - more mailing lists