On Sat, 22 Mar 2025 at 10:34, Luke D. Jones <luke@xxxxxxxxxx> wrote:
>
> On 22/03/25 22:13, Antheas Kapenekakis wrote:
> > On Sat, 22 Mar 2025 at 10:06, Luke D. Jones <luke@xxxxxxxxxx> wrote:
> >>
> >> On 22/03/25 21:12, Antheas Kapenekakis wrote:
> >>> On Sat, 22 Mar 2025 at 05:31, Luke D. Jones <luke@xxxxxxxxxx> 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@xxxxxxxxxxx>
> >>>>> ---
> >>>>> 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.
> >>
> >> This might highlight the HID + WMI issue I was talking about in the
> >> other replies and why i think the quirk table is still required.. Or
> >> perhaps an alternative would be to have HID block the WMI method for the
> >> 0x19b6 PID? There are approximately 30 laptops I know of with both
> >> methods available.
> >>
> >> I just checked the DSDT dump for Ally again and it looks like those are
> >> all good too. You might have lucked out and ended up with all devices
> >> with no WMI keyboard brightness :D
> >
> > Well we for sure will need to test a device that has both. The intent
> > of this series is to align both the WMI and HID, which is why it is
> > placed in WMI. If it NOOPs it should be ok.
> >
> > However if the set noops and the get returns dummy data that might be an issue.
>
> Unfortunately I don't recall the exact device now sorry. I though it was
> the GU605, but that like the GA605, ProArt, Ally, and Z13 all missing
> the WMI method, so those are fine.
>
> This is an sample of some of the other laptops:
>
> GL553VE.dsl
> 37871: If ((IIA0 == 0x00050021))
> 37872- {
> 37873- If (GLKB (One))
> 37874- {
> 37875- Local0 <<= 0x08
> 37876- Local0 += GLKB (0x02)
> --
> 38581: If ((IIA0 == 0x00050021))
> 38582- {
> 38583- SLKB (IIA1)
> 38584- Return (One)
> 38585- }
> 38586-
>
> GU603Z-3.10.dsl
> 90702: If ((IIA0 == 0x00050021))
> 90703- {
> 90704- Return (0xFFFFFFFE)
> 90705- }
> 90706-
> 90707- If ((IIA0 == 0x00100051))
> --
> 91125: If ((IIA0 == 0x00050021))
> 91126- {
> 91127- ^^PC00.LPCB.EC0.SLKB (IIA1)
> 91128- Return (One)
> 91129- }
> 91130-
>
> G713Q.dsl
> 8454: If ((IIA0 == 0x00050021))
> 8455- {
> 8456- Return (Zero)
> 8457- }
> 8458-
> 8459- If ((IIA0 == 0x00050031))
> --
> 9092: If ((IIA0 == 0x00050021))
> 9093- {
> 9094- Return (Zero)
> 9095- }
>
> H7606WI.dsl
> 9881: If ((IIA0 == 0x00050021))
> 9882- {
> 9883- Local0 = Zero
> 9884- Local0 = ^^PCI0.SBRG.EC0.KBLL /*
> \_SB_.PCI0.SBRG.EC0_.KBLL */
> 9885- Local0 |= 0x00350000
> 9886- Return (Local0)
> --
> 10663: If ((IIA0 == 0x00050021))
> 10664- {
> 10665- Local0 = Zero
> 10666- Local0 = (IIA1 & 0x83)
> 10667- ^^PCI0.SBRG.EC0.KBLL = Local0
> 10668- Return (One)
>
> GU603-b310-dsdt.dsl
> 91115: If ((IIA0 == 0x00050011))
> 91116- {
> 91117- If ((IIA1 == 0x02))
> 91118- {
> 91119- ^^PC00.LPCB.EC0.BLCT = One
> 91120- }
> 91121-
> 91122- Return (One)
> 91123- }
>
> GU502GU.dsl
> 59909: If ((IIA0 == 0x00050011))
> 59910- {
> 59911- If ((IIA1 == 0x02))
> 59912- {
> 59913- ^^PCI0.LPCB.EC0.BLCT = One
> 59914- }
> 59915-
> 59916- Return (One)
> 59917- }
>
> Hopefully that's enough data-points to work with? Let me know if there's
> anything else i can provide or clarify.
>
> >>>
> >>> 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 */
> >>>>
> >>
>
SBRG.EC0.KBLL being used should mean that the brightness is saved.
Since it is used as a backlight reference if it is stuck that would be
a problem. Other than that it should be OK.
The quirk also contains some laptop IDs, testing one of those should be enough.
Antheas
