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: 4253 bytes --]

On Thu, 16 Oct 2025, Antheas Kapenekakis wrote:

> On Thu, 16 Oct 2025 at 12:19, Ilpo Järvinen
> <ilpo.jarvinen@linux.intel.com> wrote:
> >
> > 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.
> 
> 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.

If you're not certain and want an easy way to obtain a stacktrace to 
confirm, you can likely do something like:

	WARN_ON_ONCE(in_atomic());

...at where you are taking the lock.

-- 
 i.

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