linux-kernel - Re: [PATCH v6 3/7] platform/x86: asus-wmi: Add support for multiple kbd RGB handlers

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: 
 <CAGwozwEo+Mx8fv_ohQQ9Ha6p=bJcJmfj==aRGcgoqQXzXEZmVw@mail.gmail.com>
Date: Thu, 16 Oct 2025 12:23:50 +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,
	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 <hdegoede@...hat.com>,
	Denis Benato <benato.denis96@...il.com>
Subject: Re: [PATCH v6 3/7] platform/x86: asus-wmi: Add support for multiple
 kbd RGB handlers

On Thu, 16 Oct 2025 at 12:19, Ilpo Järvinen
<ilpo.jarvinen@...ux.intel.com> wrote:
>
> On Wed, 15 Oct 2025, Antheas Kapenekakis wrote:
>
> > On Wed, 15 Oct 2025 at 13:59, Ilpo Järvinen
> > <ilpo.jarvinen@...ux.intel.com> wrote:
> > >
> > > On Mon, 13 Oct 2025, Antheas Kapenekakis wrote:
> > >
> > > > Some devices, such as the Z13 have multiple AURA devices connected
> > > > to them by USB. In addition, they might have a WMI interface for
> > > > RGB. In Windows, Armoury Crate exposes a unified brightness slider
> > > > for all of them, with 3 brightness levels.
> > > >
> > > > Therefore, to be synergistic in Linux, and support existing tooling
> > > > such as UPower, allow adding listeners to the RGB device of the WMI
> > > > interface. If WMI does not exist, lazy initialize the interface.
> > > >
> > > > 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            | 118 ++++++++++++++++++---
> > > >  include/linux/platform_data/x86/asus-wmi.h |  16 +++
> > > >  2 files changed, 121 insertions(+), 13 deletions(-)
> > > >
> > > > diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> > > > index e72a2b5d158e..a2a7cd61fd59 100644
> > > > --- a/drivers/platform/x86/asus-wmi.c
> > > > +++ b/drivers/platform/x86/asus-wmi.c
> > > > @@ -258,6 +258,8 @@ struct asus_wmi {
> > > >       int tpd_led_wk;
> > > >       struct led_classdev kbd_led;
> > > >       int kbd_led_wk;
> > > > +     bool kbd_led_avail;
> > > > +     bool kbd_led_registered;
> > > >       struct led_classdev lightbar_led;
> > > >       int lightbar_led_wk;
> > > >       struct led_classdev micmute_led;
> > > > @@ -1530,6 +1532,53 @@ static void asus_wmi_battery_exit(struct asus_wmi *asus)
> > > >
> > > >  /* LEDs ***********************************************************************/
> > > >
> > > > +struct asus_hid_ref {
> > > > +     struct list_head listeners;
> > > > +     struct asus_wmi *asus;
> > > > +     spinlock_t lock;
> > >
> > > Please always document what a lock protects.
> > >
> > > I started wonder why it needs to be spinlock?
> > >
> > > It would seem rwsem is more natural for it as write is only needed at
> > > probe/remove time (if there's no good reason for using a spinlock).
> > >
> > > You're also missing include.
> >
> > I went through the comments. Thanks. The reason that it is a spinlock
> > is that both hid-asus and asus-wmi interact with the primitives to
> > register and unregister listeners, either of which can prompt the
> > creation of the led device which has to be atomic. And they do so from
> > IRQs too.
>
> Please note in the changelog how it can happen from IRQs as I tried but
> couldn't find anything. Admittedly, I didn't try to follow the callchains
> that deeply. The justification should be clear enough to anyone who
> looks this commit later so better have it in the changelog.

The initial versions used a mutex, and we found kernel hangs under
IRQs, so it was converted to a spinlock. I will try to reword.
Specifically, I think the errors came when holding the lock when
changing brightness.

I recall one of them was brightness hotkey (second to last patch)
triggers an IRQ -> event goes to asus-wmi -> calls brightness handler
-> tries to hold lock -> crashes. Lock needs to be held because
hid-asus can choose to unregister a listener.


> > Perhaps the driver could be refactored to use rwsem, I am not sure.
>
> Just leave it spinlock.
>
> > The fixed version can be found here[1]. I will give it 1-2 more days
> > in case someone else wants to chime in and resend.
>
>
>
> --
>  i.