Message-ID: 
 <CAGwozwFBQ4DWS5s-La5f-6H=ZQvQFjU3=7U2RiJStGxO1sM+bQ@mail.gmail.com>
Date: Wed, 15 Oct 2025 17:45:45 +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 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.

Perhaps the driver could be refactored to use rwsem, I am not sure.

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.

Antheas

[1] https://github.com/bazzite-org/patchwork/commits/asusrgb/

> > +};
> > +
> > +struct asus_hid_ref asus_ref = {
>
> Should be static ?
>
> > +     .listeners = LIST_HEAD_INIT(asus_ref.listeners),
> > +     .asus = NULL,
> > +     .lock = __SPIN_LOCK_UNLOCKED(asus_ref.lock),
> > +};
> > +
> > +int asus_hid_register_listener(struct asus_hid_listener *bdev)
> > +{
> > +     unsigned long flags;
> > +     int ret = 0;
> > +
> > +     spin_lock_irqsave(&asus_ref.lock, flags);
> > +     list_add_tail(&bdev->list, &asus_ref.listeners);
> > +     if (asus_ref.asus) {
> > +             if (asus_ref.asus->kbd_led_registered && asus_ref.asus->kbd_led_wk >= 0)
> > +                     bdev->brightness_set(bdev, asus_ref.asus->kbd_led_wk);
> > +
> > +             if (!asus_ref.asus->kbd_led_registered) {
> > +                     ret = led_classdev_register(
> > +                             &asus_ref.asus->platform_device->dev,
> > +                             &asus_ref.asus->kbd_led);
> > +                     if (!ret)
> > +                             asus_ref.asus->kbd_led_registered = true;
>
> I suggest you use guard() for the spinlock and return early where possible.
>
> With guard() in use, you can do normal error handling here with return ret
> immediately.
>
> Please also add a local var for asus_ref.asus.
>
> > +             }
> > +     }
> > +     spin_unlock_irqrestore(&asus_ref.lock, flags);
> > +
> > +     return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(asus_hid_register_listener);
> > +
> > +void asus_hid_unregister_listener(struct asus_hid_listener *bdev)
> > +{
> > +     unsigned long flags;
> > +
> > +     spin_lock_irqsave(&asus_ref.lock, flags);
> > +     list_del(&bdev->list);
> > +     spin_unlock_irqrestore(&asus_ref.lock, flags);
> > +}
> > +EXPORT_SYMBOL_GPL(asus_hid_unregister_listener);
> > +
> >  /*
> >   * These functions actually update the LED's, and are called from a
> >   * workqueue. By doing this as separate work rather than when the LED
> > @@ -1609,6 +1658,7 @@ static int kbd_led_read(struct asus_wmi *asus, int *level, int *env)
> >
> >  static void do_kbd_led_set(struct led_classdev *led_cdev, int value)
> >  {
> > +     struct asus_hid_listener *listener;
> >       struct asus_wmi *asus;
> >       int max_level;
> >
> > @@ -1616,25 +1666,39 @@ static void do_kbd_led_set(struct led_classdev *led_cdev, int value)
> >       max_level = asus->kbd_led.max_brightness;
> >
> >       asus->kbd_led_wk = clamp_val(value, 0, max_level);
> > -     kbd_led_update(asus);
> > +
> > +     if (asus->kbd_led_avail)
> > +             kbd_led_update(asus);
> > +
> > +     list_for_each_entry(listener, &asus_ref.listeners, list)
> > +             listener->brightness_set(listener, asus->kbd_led_wk);
> >  }
> >
> >  static void kbd_led_set(struct led_classdev *led_cdev,
> >                       enum led_brightness value)
> >  {
> > +     unsigned long flags;
> > +
> >       /* Prevent disabling keyboard backlight on module unregister */
> >       if (led_cdev->flags & LED_UNREGISTERING)
> >               return;
> >
> > +     spin_lock_irqsave(&asus_ref.lock, flags);
> >       do_kbd_led_set(led_cdev, value);
> > +     spin_unlock_irqrestore(&asus_ref.lock, flags);
> >  }
> >
> >  static void kbd_led_set_by_kbd(struct asus_wmi *asus, enum led_brightness value)
> >  {
> > -     struct led_classdev *led_cdev = &asus->kbd_led;
> > +     struct led_classdev *led_cdev;
> > +     unsigned long flags;
> > +
> > +     spin_lock_irqsave(&asus_ref.lock, flags);
> > +     led_cdev = &asus->kbd_led;
> >
>
> I'd remove the empty line from the critical section.
>
> I think you should mention the locking in the changelog too as it clearly
> impacts not only the new code.
>
> >       do_kbd_led_set(led_cdev, value);
> >       led_classdev_notify_brightness_hw_changed(led_cdev, asus->kbd_led_wk);
> > +     spin_unlock_irqrestore(&asus_ref.lock, flags);
> >  }
> >
> >  static enum led_brightness kbd_led_get(struct led_classdev *led_cdev)
> > @@ -1644,6 +1708,9 @@ static enum led_brightness kbd_led_get(struct led_classdev *led_cdev)
> >
> >       asus = container_of(led_cdev, struct asus_wmi, kbd_led);
> >
> > +     if (!asus->kbd_led_avail)
> > +             return asus->kbd_led_wk;
> > +
> >       retval = kbd_led_read(asus, &value, NULL);
> >       if (retval < 0)
> >               return retval;
> > @@ -1759,7 +1826,15 @@ static int camera_led_set(struct led_classdev *led_cdev,
> >
> >  static void asus_wmi_led_exit(struct asus_wmi *asus)
> >  {
> > -     led_classdev_unregister(&asus->kbd_led);
> > +     unsigned long flags;
> > +
> > +     spin_lock_irqsave(&asus_ref.lock, flags);
> > +     asus_ref.asus = NULL;
> > +     spin_unlock_irqrestore(&asus_ref.lock, flags);
> > +
> > +     if (asus->kbd_led_registered)
> > +             led_classdev_unregister(&asus->kbd_led);
> > +
> >       led_classdev_unregister(&asus->tpd_led);
> >       led_classdev_unregister(&asus->wlan_led);
> >       led_classdev_unregister(&asus->lightbar_led);
> > @@ -1773,6 +1848,8 @@ static void asus_wmi_led_exit(struct asus_wmi *asus)
> >  static int asus_wmi_led_init(struct asus_wmi *asus)
> >  {
> >       int rv = 0, num_rgb_groups = 0, led_val;
> > +     struct asus_hid_listener *listener;
> > +     unsigned long flags;
> >
> >       if (asus->kbd_rgb_dev)
> >               kbd_rgb_mode_groups[num_rgb_groups++] = &kbd_rgb_mode_group;
> > @@ -1797,23 +1874,38 @@ static int asus_wmi_led_init(struct asus_wmi *asus)
> >                       goto error;
> >       }
> >
> > -     if (!kbd_led_read(asus, &led_val, NULL) && !dmi_check_system(asus_use_hid_led_dmi_ids)) {
> > -             pr_info("using asus-wmi for asus::kbd_backlight\n");
> > +     asus->kbd_led.name = "asus::kbd_backlight";
> > +     asus->kbd_led.flags = LED_BRIGHT_HW_CHANGED;
> > +     asus->kbd_led.brightness_set = kbd_led_set;
> > +     asus->kbd_led.brightness_get = kbd_led_get;
> > +     asus->kbd_led.max_brightness = 3;
> > +     asus->kbd_led_avail = !kbd_led_read(asus, &led_val, NULL);
> > +
> > +     if (asus->kbd_led_avail)
> >               asus->kbd_led_wk = led_val;
> > -             asus->kbd_led.name = "asus::kbd_backlight";
> > -             asus->kbd_led.flags = LED_BRIGHT_HW_CHANGED;
> > -             asus->kbd_led.brightness_set = kbd_led_set;
> > -             asus->kbd_led.brightness_get = kbd_led_get;
> > -             asus->kbd_led.max_brightness = 3;
> > +     else
> > +             asus->kbd_led_wk = -1;
> >
> > -             if (num_rgb_groups != 0)
> > -                     asus->kbd_led.groups = kbd_rgb_mode_groups;
> > +     if (asus->kbd_led_avail && num_rgb_groups != 0)
> > +             asus->kbd_led.groups = kbd_rgb_mode_groups;
>
> Can't this be placed into the block above?
>
> >
> > +     spin_lock_irqsave(&asus_ref.lock, flags);
> > +     if (asus->kbd_led_avail || !list_empty(&asus_ref.listeners)) {
> >               rv = led_classdev_register(&asus->platform_device->dev,
> >                                          &asus->kbd_led);
> > -             if (rv)
> > +             if (rv) {
> > +                     spin_unlock_irqrestore(&asus_ref.lock, flags);
>
> Please use scoped_guard() so you don't need to call unlock yourself.
>
> Unrelated to this patch, I'd also simplify error label by adding return 0
> before it so the code after the label doesn't need if (rv) check.
>
> >                       goto error;
> > +             }
> > +             asus->kbd_led_registered = true;
> > +
> > +             if (asus->kbd_led_wk >= 0) {
> > +                     list_for_each_entry(listener, &asus_ref.listeners, list)
> > +                             listener->brightness_set(listener, asus->kbd_led_wk);
> > +             }
> >       }
> > +     asus_ref.asus = asus;
> > +     spin_unlock_irqrestore(&asus_ref.lock, flags);
> >
> >       if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_WIRELESS_LED)
> >                       && (asus->driver->quirks->wapf > 0)) {
> > diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
> > index 8a515179113d..f13a701f47a8 100644
> > --- a/include/linux/platform_data/x86/asus-wmi.h
> > +++ b/include/linux/platform_data/x86/asus-wmi.h
> > @@ -163,11 +163,19 @@ enum asus_ally_mcu_hack {
> >       ASUS_WMI_ALLY_MCU_HACK_DISABLED,
> >  };
> >
> > +struct asus_hid_listener {
> > +     struct list_head list;
> > +     void (*brightness_set)(struct asus_hid_listener *listener, int brightness);
> > +};
>
> Please add kerneldoc to this struct and the exported functions.
>
> > +
> >  #if IS_REACHABLE(CONFIG_ASUS_WMI)
> >  void set_ally_mcu_hack(enum asus_ally_mcu_hack status);
> >  void set_ally_mcu_powersave(bool enabled);
> >  int asus_wmi_set_devstate(u32 dev_id, u32 ctrl_param, u32 *retval);
> >  int asus_wmi_evaluate_method(u32 method_id, u32 arg0, u32 arg1, u32 *retval);
> > +
> > +int asus_hid_register_listener(struct asus_hid_listener *cdev);
> > +void asus_hid_unregister_listener(struct asus_hid_listener *cdev);
> >  #else
> >  static inline void set_ally_mcu_hack(enum asus_ally_mcu_hack status)
> >  {
> > @@ -184,6 +192,14 @@ static inline int asus_wmi_evaluate_method(u32 method_id, u32 arg0, u32 arg1,
> >  {
> >       return -ENODEV;
> >  }
> > +
> > +static inline int asus_hid_register_listener(struct asus_hid_listener *bdev)
> > +{
> > +     return -ENODEV;
> > +}
> > +static inline void asus_hid_unregister_listener(struct asus_hid_listener *bdev)
> > +{
> > +}
> >  #endif
> >
> >  /* To be used by both hid-asus and asus-wmi to determine which controls kbd_brightness */
> >
>
> --
>  i.
>
>

Powered by blists - more mailing lists