Re: [PATCH v3 04/10] platform/x86: asus-wmi: Add support for multiple kbd RGB handlers

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 24-Mar-25 13:29, Antheas Kapenekakis wrote:
> On Mon, 24 Mar 2025 at 12:31, Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi Antheas,
>>
>> Note not a full review, just taking a generic look at the new API
>> between asus-wmi and asus-hid.
>>
>> On 22-Mar-25 11:27, 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.
>>>
>>> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
>>> ---
>>>  drivers/platform/x86/asus-wmi.c            | 113 ++++++++++++++++++---
>>>  include/linux/platform_data/x86/asus-wmi.h |  16 +++
>>>  2 files changed, 117 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
>>> index 38ef778e8c19b..95ef9b1d321bb 100644
>>> --- a/drivers/platform/x86/asus-wmi.c
>>> +++ b/drivers/platform/x86/asus-wmi.c
>>> @@ -254,6 +254,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;
>>> @@ -1487,6 +1489,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;
>>> +};
>>> +
>>> +struct asus_hid_ref asus_ref = {
>>> +     .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;
>>> +             }
>>> +     }
>>> +     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
>>> @@ -1566,6 +1615,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;
>>>
>>> @@ -1573,25 +1623,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;
>>>
>>>       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)
>>> @@ -1601,6 +1665,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;
>>> @@ -1716,7 +1783,14 @@ 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;
>>> +     if (asus->kbd_led_registered)
>>> +             led_classdev_unregister(&asus->kbd_led);
>>> +     spin_unlock_irqrestore(&asus_ref.lock, flags);
>>> +
>>>       led_classdev_unregister(&asus->tpd_led);
>>>       led_classdev_unregister(&asus->wlan_led);
>>>       led_classdev_unregister(&asus->lightbar_led);
>>> @@ -1730,6 +1804,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;
>>> +     unsigned long flags;
>>> +     bool has_listeners;
>>>
>>>       if (asus->kbd_rgb_dev)
>>>               kbd_rgb_mode_groups[num_rgb_groups++] = &kbd_rgb_mode_group;
>>> @@ -1754,24 +1830,37 @@ 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 (asus->kbd_led_avail && num_rgb_groups != 0)
>>> +             asus->kbd_led.groups = kbd_rgb_mode_groups;
>>>
>>> -             if (num_rgb_groups != 0)
>>> -                     asus->kbd_led.groups = kbd_rgb_mode_groups;
>>> +     spin_lock_irqsave(&asus_ref.lock, flags);
>>> +     has_listeners = !list_empty(&asus_ref.listeners);
>>> +     spin_unlock_irqrestore(&asus_ref.lock, flags);
>>
>> It seems to me that you should also call brightness_set()
>> on all the kbds already in the list so that their brightness
>> gets synced with the wmi kbd-backlight brightness when
>> the wmi driver loads later then the hid driver ?
> 
> You raise a good point here. Let me think about it.
> 
> But yes, if we do a notify when kbd_led_wk != -1 if the hid device
> connects after, we should do it for the before case too.
> 
>>>
>>> +     if (asus->kbd_led_avail || has_listeners) {
>>>               rv = led_classdev_register(&asus->platform_device->dev,
>>>                                          &asus->kbd_led);
>>>               if (rv)
>>>                       goto error;
>>> +             asus->kbd_led_registered = true;
>>>       }
>>>
>>> +     spin_lock_irqsave(&asus_ref.lock, flags);
>>> +     asus_ref.asus = asus;
>>
>> There is race here where a hid keyboard might show up between
>> the 2 places you take the lock, in that case if there is
>> no wmi kbd-backlight then you will not register the led_classdev
>> when asus_hid_register_listener() gets called between the unlock
>> and the lock...  I'm not sure what the best way is to fix this.
> 
> Thanks for catching that, probably a recheck:
> 
> has_listeners = !list_empty(&asus_ref.listeners);
> if (has_listeners) goto register;

I just noticed that in asus_hid_register_listener()
you keep the lock locked while calling led_classdev_register()
so that apparently is ok to do. In which case you can just
hold the lock over the led_classdev_register() call here too.

Regards,

Hans




>>> +     spin_unlock_irqrestore(&asus_ref.lock, flags);
>>> +
>>>       if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_WIRELESS_LED)
>>>                       && (asus->driver->quirks->wapf > 0)) {
>>>               INIT_WORK(&asus->wlan_led_work, wlan_led_update);
>>> diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
>>> index 783e2a336861b..ec8b0c585a63f 100644
>>> --- a/include/linux/platform_data/x86/asus-wmi.h
>>> +++ b/include/linux/platform_data/x86/asus-wmi.h
>>> @@ -157,14 +157,30 @@
>>>  #define ASUS_WMI_DSTS_MAX_BRIGTH_MASK        0x0000FF00
>>>  #define ASUS_WMI_DSTS_LIGHTBAR_MASK  0x0000000F
>>>
>>> +struct asus_hid_listener {
>>> +     struct list_head list;
>>> +     void (*brightness_set)(struct asus_hid_listener *listener, int brightness);
>>> +};
>>> +
>>>  #if IS_REACHABLE(CONFIG_ASUS_WMI)
>>>  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 int asus_wmi_evaluate_method(u32 method_id, u32 arg0, u32 arg1,
>>>                                          u32 *retval)
>>>  {
>>>       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 */
>>
>