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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 3115 bytes --]

On Wed, 15 Oct 2025, Antheas Kapenekakis wrote:

> On Wed, 15 Oct 2025 at 13:59, Ilpo Järvinen
> <ilpo.jarvinen@linux.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@ljones.dev>
> > > Tested-by: Luke D. Jones <luke@ljones.dev>
> > > Signed-off-by: Antheas Kapenekakis <lkml@antheas.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.