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: <cf824f48-58b4-2400-9acf-796bb76d6b28@linux.intel.com>
Date: Thu, 16 Oct 2025 13:18:53 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Antheas Kapenekakis <lkml@...heas.dev>
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 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.

> 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.