* [PATCH v7 1/9] HID: asus: simplify RGB init sequence
2025-10-18 10:17 [PATCH v7 0/9] HID: asus: Fix ASUS ROG Laptop's Keyboard backlight handling Antheas Kapenekakis
@ 2025-10-18 10:17 ` Antheas Kapenekakis
2025-10-23 17:38 ` Denis Benato
2025-10-18 10:17 ` [PATCH v7 2/9] HID: asus: use same report_id in response Antheas Kapenekakis
` (7 subsequent siblings)
8 siblings, 1 reply; 34+ messages in thread
From: Antheas Kapenekakis @ 2025-10-18 10:17 UTC (permalink / raw)
To: platform-driver-x86, linux-input
Cc: linux-kernel, Jiri Kosina, Benjamin Tissoires, Corentin Chary,
Luke D . Jones, Hans de Goede, Ilpo Järvinen, Denis Benato,
Antheas Kapenekakis
Currently, RGB initialization forks depending on whether a device is
NKEY. Then, NKEY devices are initialized using 0x5a, 0x5d, 0x5e
endpoints, and non-NKEY devices with 0x5a and then a
backlight check, which is omitted for NKEY devices.
Remove the fork, using a common initialization sequence for both,
where they are both only initialized with 0x5a, then checked for
backlight support. This patch should not affect existing functionality.
0x5d and 0x5e endpoint initializations are performed by Windows
userspace programs associated with different usages that reside under
the vendor HID. Specifically, 0x5d is used by Armoury Crate, which
controls RGB and 0x5e by an animation program for certain Asus laptops.
Neither is used currently in the driver.
Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
---
drivers/hid/hid-asus.c | 56 ++++++++++++++----------------------------
1 file changed, 19 insertions(+), 37 deletions(-)
diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index a444d41e53b6..7ea1037c3979 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -638,50 +638,32 @@ static int asus_kbd_register_leds(struct hid_device *hdev)
unsigned char kbd_func;
int ret;
- if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) {
- /* Initialize keyboard */
- ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID);
- if (ret < 0)
- return ret;
-
- /* The LED endpoint is initialised in two HID */
- ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID1);
- if (ret < 0)
- return ret;
-
- ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID2);
- if (ret < 0)
- return ret;
-
- if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) {
- ret = asus_kbd_disable_oobe(hdev);
- if (ret < 0)
- return ret;
- }
-
- if (drvdata->quirks & QUIRK_ROG_ALLY_XPAD) {
- intf = to_usb_interface(hdev->dev.parent);
- udev = interface_to_usbdev(intf);
- validate_mcu_fw_version(hdev,
- le16_to_cpu(udev->descriptor.idProduct));
- }
+ ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID);
+ if (ret < 0)
+ return ret;
- } else {
- /* Initialize keyboard */
- ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID);
- if (ret < 0)
- return ret;
+ /* Get keyboard functions */
+ ret = asus_kbd_get_functions(hdev, &kbd_func, FEATURE_KBD_REPORT_ID);
+ if (ret < 0)
+ return ret;
- /* Get keyboard functions */
- ret = asus_kbd_get_functions(hdev, &kbd_func, FEATURE_KBD_REPORT_ID);
+ if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) {
+ ret = asus_kbd_disable_oobe(hdev);
if (ret < 0)
return ret;
+ }
- /* Check for backlight support */
- if (!(kbd_func & SUPPORT_KBD_BACKLIGHT))
- return -ENODEV;
+ if (drvdata->quirks & QUIRK_ROG_ALLY_XPAD) {
+ intf = to_usb_interface(hdev->dev.parent);
+ udev = interface_to_usbdev(intf);
+ validate_mcu_fw_version(
+ hdev, le16_to_cpu(udev->descriptor.idProduct));
}
+ /* Check for backlight support */
+ if (!(kbd_func & SUPPORT_KBD_BACKLIGHT))
+ return -ENODEV;
+
drvdata->kbd_backlight = devm_kzalloc(&hdev->dev,
sizeof(struct asus_kbd_leds),
GFP_KERNEL);
--
2.51.0
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH v7 1/9] HID: asus: simplify RGB init sequence
2025-10-18 10:17 ` [PATCH v7 1/9] HID: asus: simplify RGB init sequence Antheas Kapenekakis
@ 2025-10-23 17:38 ` Denis Benato
2025-10-23 18:06 ` Antheas Kapenekakis
0 siblings, 1 reply; 34+ messages in thread
From: Denis Benato @ 2025-10-23 17:38 UTC (permalink / raw)
To: Antheas Kapenekakis, platform-driver-x86, linux-input
Cc: linux-kernel, Jiri Kosina, Benjamin Tissoires, Corentin Chary,
Luke D . Jones, Hans de Goede, Ilpo Järvinen
On 10/18/25 12:17, Antheas Kapenekakis wrote:
> Currently, RGB initialization forks depending on whether a device is
> NKEY. Then, NKEY devices are initialized using 0x5a, 0x5d, 0x5e
> endpoints, and non-NKEY devices with 0x5a and then a
> backlight check, which is omitted for NKEY devices.
>
> Remove the fork, using a common initialization sequence for both,
> where they are both only initialized with 0x5a, then checked for
> backlight support. This patch should not affect existing functionality.
>
> 0x5d and 0x5e endpoint initializations are performed by Windows
> userspace programs associated with different usages that reside under
> the vendor HID. Specifically, 0x5d is used by Armoury Crate, which
> controls RGB and 0x5e by an animation program for certain Asus laptops.
> Neither is used currently in the driver.
What benefits do we get from removing the unused initialization?
If this has never caused any troubles I don't see the reason for removing
them. Moreover the lighting protocol is known and I might as well add
support for it in the near future,
> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> ---
> drivers/hid/hid-asus.c | 56 ++++++++++++++----------------------------
> 1 file changed, 19 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index a444d41e53b6..7ea1037c3979 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
> @@ -638,50 +638,32 @@ static int asus_kbd_register_leds(struct hid_device *hdev)
> unsigned char kbd_func;
> int ret;
>
> - if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) {
> - /* Initialize keyboard */
> - ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID);
> - if (ret < 0)
> - return ret;
> -
> - /* The LED endpoint is initialised in two HID */
> - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID1);
> - if (ret < 0)
> - return ret;
> -
> - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID2);
> - if (ret < 0)
> - return ret;
> -
> - if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) {
> - ret = asus_kbd_disable_oobe(hdev);
> - if (ret < 0)
> - return ret;
> - }
> -
> - if (drvdata->quirks & QUIRK_ROG_ALLY_XPAD) {
> - intf = to_usb_interface(hdev->dev.parent);
> - udev = interface_to_usbdev(intf);
> - validate_mcu_fw_version(hdev,
> - le16_to_cpu(udev->descriptor.idProduct));
> - }
> + ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID);
> + if (ret < 0)
> + return ret;
>
> - } else {
> - /* Initialize keyboard */
> - ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID);
> - if (ret < 0)
> - return ret;
> + /* Get keyboard functions */
> + ret = asus_kbd_get_functions(hdev, &kbd_func, FEATURE_KBD_REPORT_ID);
> + if (ret < 0)
> + return ret;
>
> - /* Get keyboard functions */
> - ret = asus_kbd_get_functions(hdev, &kbd_func, FEATURE_KBD_REPORT_ID);
> + if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) {
> + ret = asus_kbd_disable_oobe(hdev);
> if (ret < 0)
> return ret;
> + }
>
> - /* Check for backlight support */
> - if (!(kbd_func & SUPPORT_KBD_BACKLIGHT))
> - return -ENODEV;
> + if (drvdata->quirks & QUIRK_ROG_ALLY_XPAD) {
> + intf = to_usb_interface(hdev->dev.parent);
> + udev = interface_to_usbdev(intf);
> + validate_mcu_fw_version(
> + hdev, le16_to_cpu(udev->descriptor.idProduct));
> }
>
> + /* Check for backlight support */
> + if (!(kbd_func & SUPPORT_KBD_BACKLIGHT))
> + return -ENODEV;
> +
> drvdata->kbd_backlight = devm_kzalloc(&hdev->dev,
> sizeof(struct asus_kbd_leds),
> GFP_KERNEL);
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v7 1/9] HID: asus: simplify RGB init sequence
2025-10-23 17:38 ` Denis Benato
@ 2025-10-23 18:06 ` Antheas Kapenekakis
2025-10-23 20:04 ` Denis Benato
0 siblings, 1 reply; 34+ messages in thread
From: Antheas Kapenekakis @ 2025-10-23 18:06 UTC (permalink / raw)
To: Denis Benato
Cc: platform-driver-x86, linux-input, linux-kernel, Jiri Kosina,
Benjamin Tissoires, Corentin Chary, Luke D . Jones, Hans de Goede,
Ilpo Järvinen
On Thu, 23 Oct 2025 at 19:38, Denis Benato <benato.denis96@gmail.com> wrote:
>
>
> On 10/18/25 12:17, Antheas Kapenekakis wrote:
> > Currently, RGB initialization forks depending on whether a device is
> > NKEY. Then, NKEY devices are initialized using 0x5a, 0x5d, 0x5e
> > endpoints, and non-NKEY devices with 0x5a and then a
> > backlight check, which is omitted for NKEY devices.
> >
> > Remove the fork, using a common initialization sequence for both,
> > where they are both only initialized with 0x5a, then checked for
> > backlight support. This patch should not affect existing functionality.
> >
> > 0x5d and 0x5e endpoint initializations are performed by Windows
> > userspace programs associated with different usages that reside under
> > the vendor HID. Specifically, 0x5d is used by Armoury Crate, which
> > controls RGB and 0x5e by an animation program for certain Asus laptops.
> > Neither is used currently in the driver.
> What benefits do we get from removing the unused initialization?
>
> If this has never caused any troubles I don't see the reason for removing
> them. Moreover the lighting protocol is known and I might as well add
> support for it in the near future,
I already have a patch that adds RGB and delay inits that endpoint. It
got removed to make this easier to merge. See [1].
[1] https://lore.kernel.org/lkml/20250324210151.6042-10-lkml@antheas.dev/
> > Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> > ---
> > drivers/hid/hid-asus.c | 56 ++++++++++++++----------------------------
> > 1 file changed, 19 insertions(+), 37 deletions(-)
> >
> > diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> > index a444d41e53b6..7ea1037c3979 100644
> > --- a/drivers/hid/hid-asus.c
> > +++ b/drivers/hid/hid-asus.c
> > @@ -638,50 +638,32 @@ static int asus_kbd_register_leds(struct hid_device *hdev)
> > unsigned char kbd_func;
> > int ret;
> >
> > - if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) {
> > - /* Initialize keyboard */
> > - ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID);
> > - if (ret < 0)
> > - return ret;
> > -
> > - /* The LED endpoint is initialised in two HID */
> > - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID1);
> > - if (ret < 0)
> > - return ret;
> > -
> > - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID2);
> > - if (ret < 0)
> > - return ret;
> > -
> > - if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) {
> > - ret = asus_kbd_disable_oobe(hdev);
> > - if (ret < 0)
> > - return ret;
> > - }
> > -
> > - if (drvdata->quirks & QUIRK_ROG_ALLY_XPAD) {
> > - intf = to_usb_interface(hdev->dev.parent);
> > - udev = interface_to_usbdev(intf);
> > - validate_mcu_fw_version(hdev,
> > - le16_to_cpu(udev->descriptor.idProduct));
> > - }
> > + ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID);
> > + if (ret < 0)
> > + return ret;
> >
> > - } else {
> > - /* Initialize keyboard */
> > - ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID);
> > - if (ret < 0)
> > - return ret;
> > + /* Get keyboard functions */
> > + ret = asus_kbd_get_functions(hdev, &kbd_func, FEATURE_KBD_REPORT_ID);
> > + if (ret < 0)
> > + return ret;
> >
> > - /* Get keyboard functions */
> > - ret = asus_kbd_get_functions(hdev, &kbd_func, FEATURE_KBD_REPORT_ID);
> > + if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) {
> > + ret = asus_kbd_disable_oobe(hdev);
> > if (ret < 0)
> > return ret;
> > + }
> >
> > - /* Check for backlight support */
> > - if (!(kbd_func & SUPPORT_KBD_BACKLIGHT))
> > - return -ENODEV;
> > + if (drvdata->quirks & QUIRK_ROG_ALLY_XPAD) {
> > + intf = to_usb_interface(hdev->dev.parent);
> > + udev = interface_to_usbdev(intf);
> > + validate_mcu_fw_version(
> > + hdev, le16_to_cpu(udev->descriptor.idProduct));
> > }
> >
> > + /* Check for backlight support */
> > + if (!(kbd_func & SUPPORT_KBD_BACKLIGHT))
> > + return -ENODEV;
> > +
> > drvdata->kbd_backlight = devm_kzalloc(&hdev->dev,
> > sizeof(struct asus_kbd_leds),
> > GFP_KERNEL);
>
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v7 1/9] HID: asus: simplify RGB init sequence
2025-10-23 18:06 ` Antheas Kapenekakis
@ 2025-10-23 20:04 ` Denis Benato
2025-10-23 21:30 ` Antheas Kapenekakis
0 siblings, 1 reply; 34+ messages in thread
From: Denis Benato @ 2025-10-23 20:04 UTC (permalink / raw)
To: Antheas Kapenekakis
Cc: platform-driver-x86, linux-input, linux-kernel, Jiri Kosina,
Benjamin Tissoires, Corentin Chary, Luke D . Jones, Hans de Goede,
Ilpo Järvinen
On 10/23/25 20:06, Antheas Kapenekakis wrote:
> On Thu, 23 Oct 2025 at 19:38, Denis Benato <benato.denis96@gmail.com> wrote:
>>
>> On 10/18/25 12:17, Antheas Kapenekakis wrote:
>>> Currently, RGB initialization forks depending on whether a device is
>>> NKEY. Then, NKEY devices are initialized using 0x5a, 0x5d, 0x5e
>>> endpoints, and non-NKEY devices with 0x5a and then a
>>> backlight check, which is omitted for NKEY devices.
>>>
>>> Remove the fork, using a common initialization sequence for both,
>>> where they are both only initialized with 0x5a, then checked for
>>> backlight support. This patch should not affect existing functionality.
>>>
>>> 0x5d and 0x5e endpoint initializations are performed by Windows
>>> userspace programs associated with different usages that reside under
>>> the vendor HID. Specifically, 0x5d is used by Armoury Crate, which
>>> controls RGB and 0x5e by an animation program for certain Asus laptops.
>>> Neither is used currently in the driver.
>> What benefits do we get from removing the unused initialization?
>>
>> If this has never caused any troubles I don't see the reason for removing
>> them. Moreover the lighting protocol is known and I might as well add
>> support for it in the near future,
> I already have a patch that adds RGB and delay inits that endpoint. It
> got removed to make this easier to merge. See [1].
>
> [1] https://lore.kernel.org/lkml/20250324210151.6042-10-lkml@antheas.dev/
I have to main concerns about this:
1. taking away initialization commands in one patchset to make it
easier to merge another unrelated patch doesn't seem the right thing
to do if the other patch it's not in the same series.
I can see [1] has been removed from the set for a later moment in time,
it's fine if it needs more work, just send something that function in the
same way and do not remove initialization commands when unnecessary,
especially since there will be for sure future development.
2. Your patchset resolves around keyboard backlight control and how
the keyboard device is exposed to userspace: it's fine but I do not see
the point in removing initialization commands that has nothing to do
with the issue we are trying to solve here.
Please leave 0x5E and 0x5D initialization commands where they are now.
>>> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
>>> ---
>>> drivers/hid/hid-asus.c | 56 ++++++++++++++----------------------------
>>> 1 file changed, 19 insertions(+), 37 deletions(-)
>>>
>>> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
>>> index a444d41e53b6..7ea1037c3979 100644
>>> --- a/drivers/hid/hid-asus.c
>>> +++ b/drivers/hid/hid-asus.c
>>> @@ -638,50 +638,32 @@ static int asus_kbd_register_leds(struct hid_device *hdev)
>>> unsigned char kbd_func;
>>> int ret;
>>>
>>> - if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) {
>>> - /* Initialize keyboard */
>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID);
>>> - if (ret < 0)
>>> - return ret;
>>> -
>>> - /* The LED endpoint is initialised in two HID */
>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID1);
>>> - if (ret < 0)
>>> - return ret;
>>> -
>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID2);
>>> - if (ret < 0)
>>> - return ret;
>>> -
>>> - if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) {
>>> - ret = asus_kbd_disable_oobe(hdev);
>>> - if (ret < 0)
>>> - return ret;
>>> - }
>>> -
>>> - if (drvdata->quirks & QUIRK_ROG_ALLY_XPAD) {
>>> - intf = to_usb_interface(hdev->dev.parent);
>>> - udev = interface_to_usbdev(intf);
>>> - validate_mcu_fw_version(hdev,
>>> - le16_to_cpu(udev->descriptor.idProduct));
>>> - }
>>> + ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID);
>>> + if (ret < 0)
>>> + return ret;
>>>
>>> - } else {
>>> - /* Initialize keyboard */
>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID);
>>> - if (ret < 0)
>>> - return ret;
>>> + /* Get keyboard functions */
>>> + ret = asus_kbd_get_functions(hdev, &kbd_func, FEATURE_KBD_REPORT_ID);
>>> + if (ret < 0)
>>> + return ret;
>>>
>>> - /* Get keyboard functions */
>>> - ret = asus_kbd_get_functions(hdev, &kbd_func, FEATURE_KBD_REPORT_ID);
>>> + if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) {
>>> + ret = asus_kbd_disable_oobe(hdev);
>>> if (ret < 0)
>>> return ret;
>>> + }
>>>
>>> - /* Check for backlight support */
>>> - if (!(kbd_func & SUPPORT_KBD_BACKLIGHT))
>>> - return -ENODEV;
>>> + if (drvdata->quirks & QUIRK_ROG_ALLY_XPAD) {
>>> + intf = to_usb_interface(hdev->dev.parent);
>>> + udev = interface_to_usbdev(intf);
>>> + validate_mcu_fw_version(
>>> + hdev, le16_to_cpu(udev->descriptor.idProduct));
>>> }
>>>
>>> + /* Check for backlight support */
>>> + if (!(kbd_func & SUPPORT_KBD_BACKLIGHT))
>>> + return -ENODEV;
>>> +
>>> drvdata->kbd_backlight = devm_kzalloc(&hdev->dev,
>>> sizeof(struct asus_kbd_leds),
>>> GFP_KERNEL);
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v7 1/9] HID: asus: simplify RGB init sequence
2025-10-23 20:04 ` Denis Benato
@ 2025-10-23 21:30 ` Antheas Kapenekakis
2025-10-23 22:53 ` Denis Benato
0 siblings, 1 reply; 34+ messages in thread
From: Antheas Kapenekakis @ 2025-10-23 21:30 UTC (permalink / raw)
To: Denis Benato
Cc: platform-driver-x86, linux-input, linux-kernel, Jiri Kosina,
Benjamin Tissoires, Corentin Chary, Luke D . Jones, Hans de Goede,
Ilpo Järvinen
On Thu, 23 Oct 2025 at 22:05, Denis Benato <benato.denis96@gmail.com> wrote:
>
>
> On 10/23/25 20:06, Antheas Kapenekakis wrote:
> > On Thu, 23 Oct 2025 at 19:38, Denis Benato <benato.denis96@gmail.com> wrote:
> >>
> >> On 10/18/25 12:17, Antheas Kapenekakis wrote:
> >>> Currently, RGB initialization forks depending on whether a device is
> >>> NKEY. Then, NKEY devices are initialized using 0x5a, 0x5d, 0x5e
> >>> endpoints, and non-NKEY devices with 0x5a and then a
> >>> backlight check, which is omitted for NKEY devices.
> >>>
> >>> Remove the fork, using a common initialization sequence for both,
> >>> where they are both only initialized with 0x5a, then checked for
> >>> backlight support. This patch should not affect existing functionality.
> >>>
> >>> 0x5d and 0x5e endpoint initializations are performed by Windows
> >>> userspace programs associated with different usages that reside under
> >>> the vendor HID. Specifically, 0x5d is used by Armoury Crate, which
> >>> controls RGB and 0x5e by an animation program for certain Asus laptops.
> >>> Neither is used currently in the driver.
> >> What benefits do we get from removing the unused initialization?
> >>
> >> If this has never caused any troubles I don't see the reason for removing
> >> them. Moreover the lighting protocol is known and I might as well add
> >> support for it in the near future,
> > I already have a patch that adds RGB and delay inits that endpoint. It
> > got removed to make this easier to merge. See [1].
> >
> > [1] https://lore.kernel.org/lkml/20250324210151.6042-10-lkml@antheas.dev/
> I have to main concerns about this:
>
> 1. taking away initialization commands in one patchset to make it
> easier to merge another unrelated patch doesn't seem the right thing
> to do if the other patch it's not in the same series.
>
> I can see [1] has been removed from the set for a later moment in time,
> it's fine if it needs more work, just send something that function in the
> same way and do not remove initialization commands when unnecessary,
> especially since there will be for sure future development.
The initialization was removed as part of general cleanup. Not to make
it easier to merge the RGB patch. In addition, the RGB patch only runs
the init in a lazy fashion, so if nobody uses the RGB sysfs the init
does not run and the behavior is the same.
> 2. Your patchset resolves around keyboard backlight control and how
> the keyboard device is exposed to userspace: it's fine but I do not see
> the point in removing initialization commands that has nothing to do
> with the issue we are trying to solve here.
>
> Please leave 0x5E and 0x5D initialization commands where they are now.
I mean the second part of the patchset does that. The first part is a
cleanup. What would be the reason for keeping 0x5E and 0x5D? They are
only used when initializing those endpoints to write further commands
to them and for identification. The current driver does not write
commands to those endpoints and identifies itself over 0x5A.
I do get that it is a bit risky as some laptops might be hardcoded to
wait for 0x5D to turn on RGB. Which is why we had the last patch until
V4. But we have yet to find a laptop that has this problem, so I find
it difficult to justify keeping the init.
Do note that you might need to add the 0x5D init to your userspace
program for certain laptops if you haven't already. But that is ok,
since in doing so you are also validating you are speaking to an Asus
device, which is important.
Antheas
> >>> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> >>> ---
> >>> drivers/hid/hid-asus.c | 56 ++++++++++++++----------------------------
> >>> 1 file changed, 19 insertions(+), 37 deletions(-)
> >>>
> >>> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> >>> index a444d41e53b6..7ea1037c3979 100644
> >>> --- a/drivers/hid/hid-asus.c
> >>> +++ b/drivers/hid/hid-asus.c
> >>> @@ -638,50 +638,32 @@ static int asus_kbd_register_leds(struct hid_device *hdev)
> >>> unsigned char kbd_func;
> >>> int ret;
> >>>
> >>> - if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) {
> >>> - /* Initialize keyboard */
> >>> - ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID);
> >>> - if (ret < 0)
> >>> - return ret;
> >>> -
> >>> - /* The LED endpoint is initialised in two HID */
> >>> - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID1);
> >>> - if (ret < 0)
> >>> - return ret;
> >>> -
> >>> - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID2);
> >>> - if (ret < 0)
> >>> - return ret;
> >>> -
> >>> - if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) {
> >>> - ret = asus_kbd_disable_oobe(hdev);
> >>> - if (ret < 0)
> >>> - return ret;
> >>> - }
> >>> -
> >>> - if (drvdata->quirks & QUIRK_ROG_ALLY_XPAD) {
> >>> - intf = to_usb_interface(hdev->dev.parent);
> >>> - udev = interface_to_usbdev(intf);
> >>> - validate_mcu_fw_version(hdev,
> >>> - le16_to_cpu(udev->descriptor.idProduct));
> >>> - }
> >>> + ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID);
> >>> + if (ret < 0)
> >>> + return ret;
> >>>
> >>> - } else {
> >>> - /* Initialize keyboard */
> >>> - ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID);
> >>> - if (ret < 0)
> >>> - return ret;
> >>> + /* Get keyboard functions */
> >>> + ret = asus_kbd_get_functions(hdev, &kbd_func, FEATURE_KBD_REPORT_ID);
> >>> + if (ret < 0)
> >>> + return ret;
> >>>
> >>> - /* Get keyboard functions */
> >>> - ret = asus_kbd_get_functions(hdev, &kbd_func, FEATURE_KBD_REPORT_ID);
> >>> + if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) {
> >>> + ret = asus_kbd_disable_oobe(hdev);
> >>> if (ret < 0)
> >>> return ret;
> >>> + }
> >>>
> >>> - /* Check for backlight support */
> >>> - if (!(kbd_func & SUPPORT_KBD_BACKLIGHT))
> >>> - return -ENODEV;
> >>> + if (drvdata->quirks & QUIRK_ROG_ALLY_XPAD) {
> >>> + intf = to_usb_interface(hdev->dev.parent);
> >>> + udev = interface_to_usbdev(intf);
> >>> + validate_mcu_fw_version(
> >>> + hdev, le16_to_cpu(udev->descriptor.idProduct));
> >>> }
> >>>
> >>> + /* Check for backlight support */
> >>> + if (!(kbd_func & SUPPORT_KBD_BACKLIGHT))
> >>> + return -ENODEV;
> >>> +
> >>> drvdata->kbd_backlight = devm_kzalloc(&hdev->dev,
> >>> sizeof(struct asus_kbd_leds),
> >>> GFP_KERNEL);
>
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v7 1/9] HID: asus: simplify RGB init sequence
2025-10-23 21:30 ` Antheas Kapenekakis
@ 2025-10-23 22:53 ` Denis Benato
2025-10-23 23:25 ` Antheas Kapenekakis
0 siblings, 1 reply; 34+ messages in thread
From: Denis Benato @ 2025-10-23 22:53 UTC (permalink / raw)
To: Antheas Kapenekakis
Cc: platform-driver-x86, linux-input, linux-kernel, Jiri Kosina,
Benjamin Tissoires, Corentin Chary, Luke D . Jones, Hans de Goede,
Ilpo Järvinen
On 10/23/25 23:30, Antheas Kapenekakis wrote:
> On Thu, 23 Oct 2025 at 22:05, Denis Benato <benato.denis96@gmail.com> wrote:
>>
>> On 10/23/25 20:06, Antheas Kapenekakis wrote:
>>> On Thu, 23 Oct 2025 at 19:38, Denis Benato <benato.denis96@gmail.com> wrote:
>>>> On 10/18/25 12:17, Antheas Kapenekakis wrote:
>>>>> Currently, RGB initialization forks depending on whether a device is
>>>>> NKEY. Then, NKEY devices are initialized using 0x5a, 0x5d, 0x5e
>>>>> endpoints, and non-NKEY devices with 0x5a and then a
>>>>> backlight check, which is omitted for NKEY devices.
>>>>>
>>>>> Remove the fork, using a common initialization sequence for both,
>>>>> where they are both only initialized with 0x5a, then checked for
>>>>> backlight support. This patch should not affect existing functionality.
>>>>>
>>>>> 0x5d and 0x5e endpoint initializations are performed by Windows
>>>>> userspace programs associated with different usages that reside under
>>>>> the vendor HID. Specifically, 0x5d is used by Armoury Crate, which
>>>>> controls RGB and 0x5e by an animation program for certain Asus laptops.
>>>>> Neither is used currently in the driver.
>>>> What benefits do we get from removing the unused initialization?
>>>>
>>>> If this has never caused any troubles I don't see the reason for removing
>>>> them. Moreover the lighting protocol is known and I might as well add
>>>> support for it in the near future,
>>> I already have a patch that adds RGB and delay inits that endpoint. It
>>> got removed to make this easier to merge. See [1].
>>>
>>> [1] https://lore.kernel.org/lkml/20250324210151.6042-10-lkml@antheas.dev/
>> I have to main concerns about this:
>>
>> 1. taking away initialization commands in one patchset to make it
>> easier to merge another unrelated patch doesn't seem the right thing
>> to do if the other patch it's not in the same series.
>>
>> I can see [1] has been removed from the set for a later moment in time,
>> it's fine if it needs more work, just send something that function in the
>> same way and do not remove initialization commands when unnecessary,
>> especially since there will be for sure future development.
> The initialization was removed as part of general cleanup. Not to make
> it easier to merge the RGB patch. In addition, the RGB patch only runs
> the init in a lazy fashion, so if nobody uses the RGB sysfs the init
> does not run and the behavior is the same.
There are a few problems here:
1. sope creep: either do a cleanup or solve bugs. The fact that your flow z13
doesn't load hid-asus correctly has nothing to do with the initialization of anime.
The fact that hid-asus is driving leds instead of asus-wmi has nothing to do with
anime matrix initialization either.
2. not sending the initialization can get hardware misbehave because it
is left in an uninitialized state.
3. there are absolutely zero reasons to do that. There are even less reasons
as to do it as part of this patchset.
>> 2. Your patchset resolves around keyboard backlight control and how
>> the keyboard device is exposed to userspace: it's fine but I do not see
>> the point in removing initialization commands that has nothing to do
>> with the issue we are trying to solve here.
>>
>> Please leave 0x5E and 0x5D initialization commands where they are now.
> I mean the second part of the patchset does that. The first part is a
> cleanup. What would be the reason for keeping 0x5E and 0x5D? They are
> only used when initializing those endpoints to write further commands
> to them and for identification. The current driver does not write
> commands to those endpoints and identifies itself over 0x5A.
There are no bugs opened that ties initialization of devices to bugs.
Quite the opposite: I can guarantee you that removing part of the
init will introduce regressions.
The onus is on you to provide strong evidence that the removal is
a necessary act.
Regardless it is not in the scope of this patchset: remove it.
> I do get that it is a bit risky as some laptops might be hardcoded to
> wait for 0x5D to turn on RGB. Which is why we had the last patch until
> V4. But we have yet to find a laptop that has this problem, so I find
> it difficult to justify keeping the init.
Yes it's risky to remove initialization sequences for a device that is
in every modern ASUS laptop and is tied to the EC.
> Do note that you might need to add the 0x5D init to your userspace
> program for certain laptops if you haven't already. But that is ok,
> since in doing so you are also validating you are speaking to an Asus
> device, which is important.
This doesn't make much sense: why would anyone remove
a command from the kernel, that can be very well essential to some models
(sleep can break, for example) just to add it back in a userspace program?
What does it mean I have to validate I am speaking to an asus device?
Software selects devices by known attribute, one of them is the vid:pid....
Beside what does this have to do with the removal of initialization commands
from the kernel?
Even late initializing devices can lead to problems. Windows doesn't do that:
as soon as asus drivers are loaded all relevant initialization sequences are
sent; Windows is the only officially supported OS: do not introduce commands
flow divergence without strong reasons backing it up.
> Antheas
>
Denis
>>>>> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
>>>>> ---
>>>>> drivers/hid/hid-asus.c | 56 ++++++++++++++----------------------------
>>>>> 1 file changed, 19 insertions(+), 37 deletions(-)
>>>>>
>>>>> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
>>>>> index a444d41e53b6..7ea1037c3979 100644
>>>>> --- a/drivers/hid/hid-asus.c
>>>>> +++ b/drivers/hid/hid-asus.c
>>>>> @@ -638,50 +638,32 @@ static int asus_kbd_register_leds(struct hid_device *hdev)
>>>>> unsigned char kbd_func;
>>>>> int ret;
>>>>>
>>>>> - if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) {
>>>>> - /* Initialize keyboard */
>>>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID);
>>>>> - if (ret < 0)
>>>>> - return ret;
>>>>> -
>>>>> - /* The LED endpoint is initialised in two HID */
>>>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID1);
>>>>> - if (ret < 0)
>>>>> - return ret;
>>>>> -
>>>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID2);
>>>>> - if (ret < 0)
>>>>> - return ret;
>>>>> -
>>>>> - if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) {
>>>>> - ret = asus_kbd_disable_oobe(hdev);
>>>>> - if (ret < 0)
>>>>> - return ret;
>>>>> - }
>>>>> -
>>>>> - if (drvdata->quirks & QUIRK_ROG_ALLY_XPAD) {
>>>>> - intf = to_usb_interface(hdev->dev.parent);
>>>>> - udev = interface_to_usbdev(intf);
>>>>> - validate_mcu_fw_version(hdev,
>>>>> - le16_to_cpu(udev->descriptor.idProduct));
>>>>> - }
>>>>> + ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID);
>>>>> + if (ret < 0)
>>>>> + return ret;
>>>>>
>>>>> - } else {
>>>>> - /* Initialize keyboard */
>>>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID);
>>>>> - if (ret < 0)
>>>>> - return ret;
>>>>> + /* Get keyboard functions */
>>>>> + ret = asus_kbd_get_functions(hdev, &kbd_func, FEATURE_KBD_REPORT_ID);
>>>>> + if (ret < 0)
>>>>> + return ret;
>>>>>
>>>>> - /* Get keyboard functions */
>>>>> - ret = asus_kbd_get_functions(hdev, &kbd_func, FEATURE_KBD_REPORT_ID);
>>>>> + if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) {
>>>>> + ret = asus_kbd_disable_oobe(hdev);
>>>>> if (ret < 0)
>>>>> return ret;
>>>>> + }
>>>>>
>>>>> - /* Check for backlight support */
>>>>> - if (!(kbd_func & SUPPORT_KBD_BACKLIGHT))
>>>>> - return -ENODEV;
>>>>> + if (drvdata->quirks & QUIRK_ROG_ALLY_XPAD) {
>>>>> + intf = to_usb_interface(hdev->dev.parent);
>>>>> + udev = interface_to_usbdev(intf);
>>>>> + validate_mcu_fw_version(
>>>>> + hdev, le16_to_cpu(udev->descriptor.idProduct));
>>>>> }
>>>>>
>>>>> + /* Check for backlight support */
>>>>> + if (!(kbd_func & SUPPORT_KBD_BACKLIGHT))
>>>>> + return -ENODEV;
>>>>> +
>>>>> drvdata->kbd_backlight = devm_kzalloc(&hdev->dev,
>>>>> sizeof(struct asus_kbd_leds),
>>>>> GFP_KERNEL);
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v7 1/9] HID: asus: simplify RGB init sequence
2025-10-23 22:53 ` Denis Benato
@ 2025-10-23 23:25 ` Antheas Kapenekakis
2025-10-24 16:20 ` Antheas Kapenekakis
0 siblings, 1 reply; 34+ messages in thread
From: Antheas Kapenekakis @ 2025-10-23 23:25 UTC (permalink / raw)
To: Denis Benato
Cc: platform-driver-x86, linux-input, linux-kernel, Jiri Kosina,
Benjamin Tissoires, Corentin Chary, Luke D . Jones, Hans de Goede,
Ilpo Järvinen
On Fri, 24 Oct 2025 at 00:53, Denis Benato <benato.denis96@gmail.com> wrote:
>
>
> On 10/23/25 23:30, Antheas Kapenekakis wrote:
> > On Thu, 23 Oct 2025 at 22:05, Denis Benato <benato.denis96@gmail.com> wrote:
> >>
> >> On 10/23/25 20:06, Antheas Kapenekakis wrote:
> >>> On Thu, 23 Oct 2025 at 19:38, Denis Benato <benato.denis96@gmail.com> wrote:
> >>>> On 10/18/25 12:17, Antheas Kapenekakis wrote:
> >>>>> Currently, RGB initialization forks depending on whether a device is
> >>>>> NKEY. Then, NKEY devices are initialized using 0x5a, 0x5d, 0x5e
> >>>>> endpoints, and non-NKEY devices with 0x5a and then a
> >>>>> backlight check, which is omitted for NKEY devices.
> >>>>>
> >>>>> Remove the fork, using a common initialization sequence for both,
> >>>>> where they are both only initialized with 0x5a, then checked for
> >>>>> backlight support. This patch should not affect existing functionality.
> >>>>>
> >>>>> 0x5d and 0x5e endpoint initializations are performed by Windows
> >>>>> userspace programs associated with different usages that reside under
> >>>>> the vendor HID. Specifically, 0x5d is used by Armoury Crate, which
> >>>>> controls RGB and 0x5e by an animation program for certain Asus laptops.
> >>>>> Neither is used currently in the driver.
> >>>> What benefits do we get from removing the unused initialization?
> >>>>
> >>>> If this has never caused any troubles I don't see the reason for removing
> >>>> them. Moreover the lighting protocol is known and I might as well add
> >>>> support for it in the near future,
> >>> I already have a patch that adds RGB and delay inits that endpoint. It
> >>> got removed to make this easier to merge. See [1].
> >>>
> >>> [1] https://lore.kernel.org/lkml/20250324210151.6042-10-lkml@antheas.dev/
> >> I have to main concerns about this:
> >>
> >> 1. taking away initialization commands in one patchset to make it
> >> easier to merge another unrelated patch doesn't seem the right thing
> >> to do if the other patch it's not in the same series.
> >>
> >> I can see [1] has been removed from the set for a later moment in time,
> >> it's fine if it needs more work, just send something that function in the
> >> same way and do not remove initialization commands when unnecessary,
> >> especially since there will be for sure future development.
> > The initialization was removed as part of general cleanup. Not to make
> > it easier to merge the RGB patch. In addition, the RGB patch only runs
> > the init in a lazy fashion, so if nobody uses the RGB sysfs the init
> > does not run and the behavior is the same.
> There are a few problems here:
> 1. sope creep: either do a cleanup or solve bugs. The fact that your flow z13
> doesn't load hid-asus correctly has nothing to do with the initialization of anime.
> The fact that hid-asus is driving leds instead of asus-wmi has nothing to do with
> anime matrix initialization either.
> 2. not sending the initialization can get hardware misbehave because it
> is left in an uninitialized state.
> 3. there are absolutely zero reasons to do that. There are even less reasons
> as to do it as part of this patchset.
>
> >> 2. Your patchset resolves around keyboard backlight control and how
> >> the keyboard device is exposed to userspace: it's fine but I do not see
> >> the point in removing initialization commands that has nothing to do
> >> with the issue we are trying to solve here.
> >>
> >> Please leave 0x5E and 0x5D initialization commands where they are now.
> > I mean the second part of the patchset does that. The first part is a
> > cleanup. What would be the reason for keeping 0x5E and 0x5D? They are
> > only used when initializing those endpoints to write further commands
> > to them and for identification. The current driver does not write
> > commands to those endpoints and identifies itself over 0x5A.
> There are no bugs opened that ties initialization of devices to bugs.
> Quite the opposite: I can guarantee you that removing part of the
> init will introduce regressions.
>
> The onus is on you to provide strong evidence that the removal is
> a necessary act.
>
> Regardless it is not in the scope of this patchset: remove it.
> > I do get that it is a bit risky as some laptops might be hardcoded to
> > wait for 0x5D to turn on RGB. Which is why we had the last patch until
> > V4. But we have yet to find a laptop that has this problem, so I find
> > it difficult to justify keeping the init.
> Yes it's risky to remove initialization sequences for a device that is
> in every modern ASUS laptop and is tied to the EC.
> > Do note that you might need to add the 0x5D init to your userspace
> > program for certain laptops if you haven't already. But that is ok,
> > since in doing so you are also validating you are speaking to an Asus
> > device, which is important.
> This doesn't make much sense: why would anyone remove
> a command from the kernel, that can be very well essential to some models
> (sleep can break, for example) just to add it back in a userspace program?
>
> What does it mean I have to validate I am speaking to an asus device?
> Software selects devices by known attribute, one of them is the vid:pid....
> Beside what does this have to do with the removal of initialization commands
> from the kernel?
>
> Even late initializing devices can lead to problems. Windows doesn't do that:
> as soon as asus drivers are loaded all relevant initialization sequences are
> sent; Windows is the only officially supported OS: do not introduce commands
> flow divergence without strong reasons backing it up.
If you think keeping 0x5D init is that important, I can spin patch [1]
into this series. But then this quirk will stay in the kernel forever.
I can even add 0x5E since that does not affect newer devices, which I
care for simplifying the sequence.
Luke said these two pairs are the important ones to keep.
I'm not sure what to do.
Antheas
[1] https://lore.kernel.org/all/20250325184601.10990-12-lkml@antheas.dev/
> > Antheas
> >
> Denis
> >>>>> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> >>>>> ---
> >>>>> drivers/hid/hid-asus.c | 56 ++++++++++++++----------------------------
> >>>>> 1 file changed, 19 insertions(+), 37 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> >>>>> index a444d41e53b6..7ea1037c3979 100644
> >>>>> --- a/drivers/hid/hid-asus.c
> >>>>> +++ b/drivers/hid/hid-asus.c
> >>>>> @@ -638,50 +638,32 @@ static int asus_kbd_register_leds(struct hid_device *hdev)
> >>>>> unsigned char kbd_func;
> >>>>> int ret;
> >>>>>
> >>>>> - if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) {
> >>>>> - /* Initialize keyboard */
> >>>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID);
> >>>>> - if (ret < 0)
> >>>>> - return ret;
> >>>>> -
> >>>>> - /* The LED endpoint is initialised in two HID */
> >>>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID1);
> >>>>> - if (ret < 0)
> >>>>> - return ret;
> >>>>> -
> >>>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID2);
> >>>>> - if (ret < 0)
> >>>>> - return ret;
> >>>>> -
> >>>>> - if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) {
> >>>>> - ret = asus_kbd_disable_oobe(hdev);
> >>>>> - if (ret < 0)
> >>>>> - return ret;
> >>>>> - }
> >>>>> -
> >>>>> - if (drvdata->quirks & QUIRK_ROG_ALLY_XPAD) {
> >>>>> - intf = to_usb_interface(hdev->dev.parent);
> >>>>> - udev = interface_to_usbdev(intf);
> >>>>> - validate_mcu_fw_version(hdev,
> >>>>> - le16_to_cpu(udev->descriptor.idProduct));
> >>>>> - }
> >>>>> + ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID);
> >>>>> + if (ret < 0)
> >>>>> + return ret;
> >>>>>
> >>>>> - } else {
> >>>>> - /* Initialize keyboard */
> >>>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID);
> >>>>> - if (ret < 0)
> >>>>> - return ret;
> >>>>> + /* Get keyboard functions */
> >>>>> + ret = asus_kbd_get_functions(hdev, &kbd_func, FEATURE_KBD_REPORT_ID);
> >>>>> + if (ret < 0)
> >>>>> + return ret;
> >>>>>
> >>>>> - /* Get keyboard functions */
> >>>>> - ret = asus_kbd_get_functions(hdev, &kbd_func, FEATURE_KBD_REPORT_ID);
> >>>>> + if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) {
> >>>>> + ret = asus_kbd_disable_oobe(hdev);
> >>>>> if (ret < 0)
> >>>>> return ret;
> >>>>> + }
> >>>>>
> >>>>> - /* Check for backlight support */
> >>>>> - if (!(kbd_func & SUPPORT_KBD_BACKLIGHT))
> >>>>> - return -ENODEV;
> >>>>> + if (drvdata->quirks & QUIRK_ROG_ALLY_XPAD) {
> >>>>> + intf = to_usb_interface(hdev->dev.parent);
> >>>>> + udev = interface_to_usbdev(intf);
> >>>>> + validate_mcu_fw_version(
> >>>>> + hdev, le16_to_cpu(udev->descriptor.idProduct));
> >>>>> }
> >>>>>
> >>>>> + /* Check for backlight support */
> >>>>> + if (!(kbd_func & SUPPORT_KBD_BACKLIGHT))
> >>>>> + return -ENODEV;
> >>>>> +
> >>>>> drvdata->kbd_backlight = devm_kzalloc(&hdev->dev,
> >>>>> sizeof(struct asus_kbd_leds),
> >>>>> GFP_KERNEL);
>
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v7 1/9] HID: asus: simplify RGB init sequence
2025-10-23 23:25 ` Antheas Kapenekakis
@ 2025-10-24 16:20 ` Antheas Kapenekakis
2025-10-24 18:53 ` Denis Benato
0 siblings, 1 reply; 34+ messages in thread
From: Antheas Kapenekakis @ 2025-10-24 16:20 UTC (permalink / raw)
To: Denis Benato
Cc: platform-driver-x86, linux-input, linux-kernel, Jiri Kosina,
Benjamin Tissoires, Corentin Chary, Luke D . Jones, Hans de Goede,
Ilpo Järvinen
On Fri, 24 Oct 2025 at 01:25, Antheas Kapenekakis <lkml@antheas.dev> wrote:
>
> On Fri, 24 Oct 2025 at 00:53, Denis Benato <benato.denis96@gmail.com> wrote:
> >
> >
> > On 10/23/25 23:30, Antheas Kapenekakis wrote:
> > > On Thu, 23 Oct 2025 at 22:05, Denis Benato <benato.denis96@gmail.com> wrote:
> > >>
> > >> On 10/23/25 20:06, Antheas Kapenekakis wrote:
> > >>> On Thu, 23 Oct 2025 at 19:38, Denis Benato <benato.denis96@gmail.com> wrote:
> > >>>> On 10/18/25 12:17, Antheas Kapenekakis wrote:
> > >>>>> Currently, RGB initialization forks depending on whether a device is
> > >>>>> NKEY. Then, NKEY devices are initialized using 0x5a, 0x5d, 0x5e
> > >>>>> endpoints, and non-NKEY devices with 0x5a and then a
> > >>>>> backlight check, which is omitted for NKEY devices.
> > >>>>>
> > >>>>> Remove the fork, using a common initialization sequence for both,
> > >>>>> where they are both only initialized with 0x5a, then checked for
> > >>>>> backlight support. This patch should not affect existing functionality.
> > >>>>>
> > >>>>> 0x5d and 0x5e endpoint initializations are performed by Windows
> > >>>>> userspace programs associated with different usages that reside under
> > >>>>> the vendor HID. Specifically, 0x5d is used by Armoury Crate, which
> > >>>>> controls RGB and 0x5e by an animation program for certain Asus laptops.
> > >>>>> Neither is used currently in the driver.
> > >>>> What benefits do we get from removing the unused initialization?
> > >>>>
> > >>>> If this has never caused any troubles I don't see the reason for removing
> > >>>> them. Moreover the lighting protocol is known and I might as well add
> > >>>> support for it in the near future,
> > >>> I already have a patch that adds RGB and delay inits that endpoint. It
> > >>> got removed to make this easier to merge. See [1].
> > >>>
> > >>> [1] https://lore.kernel.org/lkml/20250324210151.6042-10-lkml@antheas.dev/
> > >> I have to main concerns about this:
> > >>
> > >> 1. taking away initialization commands in one patchset to make it
> > >> easier to merge another unrelated patch doesn't seem the right thing
> > >> to do if the other patch it's not in the same series.
> > >>
> > >> I can see [1] has been removed from the set for a later moment in time,
> > >> it's fine if it needs more work, just send something that function in the
> > >> same way and do not remove initialization commands when unnecessary,
> > >> especially since there will be for sure future development.
> > > The initialization was removed as part of general cleanup. Not to make
> > > it easier to merge the RGB patch. In addition, the RGB patch only runs
> > > the init in a lazy fashion, so if nobody uses the RGB sysfs the init
> > > does not run and the behavior is the same.
> > There are a few problems here:
> > 1. sope creep: either do a cleanup or solve bugs. The fact that your flow z13
> > doesn't load hid-asus correctly has nothing to do with the initialization of anime.
> > The fact that hid-asus is driving leds instead of asus-wmi has nothing to do with
> > anime matrix initialization either.
> > 2. not sending the initialization can get hardware misbehave because it
> > is left in an uninitialized state.
> > 3. there are absolutely zero reasons to do that. There are even less reasons
> > as to do it as part of this patchset.
> >
> > >> 2. Your patchset resolves around keyboard backlight control and how
> > >> the keyboard device is exposed to userspace: it's fine but I do not see
> > >> the point in removing initialization commands that has nothing to do
> > >> with the issue we are trying to solve here.
> > >>
> > >> Please leave 0x5E and 0x5D initialization commands where they are now.
> > > I mean the second part of the patchset does that. The first part is a
> > > cleanup. What would be the reason for keeping 0x5E and 0x5D? They are
> > > only used when initializing those endpoints to write further commands
> > > to them and for identification. The current driver does not write
> > > commands to those endpoints and identifies itself over 0x5A.
> > There are no bugs opened that ties initialization of devices to bugs.
> > Quite the opposite: I can guarantee you that removing part of the
> > init will introduce regressions.
> >
> > The onus is on you to provide strong evidence that the removal is
> > a necessary act.
> >
> > Regardless it is not in the scope of this patchset: remove it.
> > > I do get that it is a bit risky as some laptops might be hardcoded to
> > > wait for 0x5D to turn on RGB. Which is why we had the last patch until
> > > V4. But we have yet to find a laptop that has this problem, so I find
> > > it difficult to justify keeping the init.
> > Yes it's risky to remove initialization sequences for a device that is
> > in every modern ASUS laptop and is tied to the EC.
> > > Do note that you might need to add the 0x5D init to your userspace
> > > program for certain laptops if you haven't already. But that is ok,
> > > since in doing so you are also validating you are speaking to an Asus
> > > device, which is important.
> > This doesn't make much sense: why would anyone remove
> > a command from the kernel, that can be very well essential to some models
> > (sleep can break, for example) just to add it back in a userspace program?
> >
> > What does it mean I have to validate I am speaking to an asus device?
> > Software selects devices by known attribute, one of them is the vid:pid....
> > Beside what does this have to do with the removal of initialization commands
> > from the kernel?
> >
> > Even late initializing devices can lead to problems. Windows doesn't do that:
> > as soon as asus drivers are loaded all relevant initialization sequences are
> > sent; Windows is the only officially supported OS: do not introduce commands
> > flow divergence without strong reasons backing it up.
>
> If you think keeping 0x5D init is that important, I can spin patch [1]
> into this series. But then this quirk will stay in the kernel forever.
> I can even add 0x5E since that does not affect newer devices, which I
> care for simplifying the sequence.
>
> Luke said these two pairs are the important ones to keep.
>
> I'm not sure what to do.
I was asked by a 2025 Asus Zenbook Duo user to add his IDs in [1]. In
doing so, I updated the rgb and legacy init patches for the new series
and added a quirk for early init of the duo keyboards.
The series is 14 patches long, I don't think my email can take it :(
Should we merge the first part of this series with the legacy init,
then do the backlight refactor, and finally the new Duo stuff + rgb?
Antheas
> Antheas
>
> [1] https://lore.kernel.org/all/20250325184601.10990-12-lkml@antheas.dev/
>
> > > Antheas
> > >
> > Denis
> > >>>>> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> > >>>>> ---
> > >>>>> drivers/hid/hid-asus.c | 56 ++++++++++++++----------------------------
> > >>>>> 1 file changed, 19 insertions(+), 37 deletions(-)
> > >>>>>
> > >>>>> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> > >>>>> index a444d41e53b6..7ea1037c3979 100644
> > >>>>> --- a/drivers/hid/hid-asus.c
> > >>>>> +++ b/drivers/hid/hid-asus.c
> > >>>>> @@ -638,50 +638,32 @@ static int asus_kbd_register_leds(struct hid_device *hdev)
> > >>>>> unsigned char kbd_func;
> > >>>>> int ret;
> > >>>>>
> > >>>>> - if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) {
> > >>>>> - /* Initialize keyboard */
> > >>>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID);
> > >>>>> - if (ret < 0)
> > >>>>> - return ret;
> > >>>>> -
> > >>>>> - /* The LED endpoint is initialised in two HID */
> > >>>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID1);
> > >>>>> - if (ret < 0)
> > >>>>> - return ret;
> > >>>>> -
> > >>>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID2);
> > >>>>> - if (ret < 0)
> > >>>>> - return ret;
> > >>>>> -
> > >>>>> - if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) {
> > >>>>> - ret = asus_kbd_disable_oobe(hdev);
> > >>>>> - if (ret < 0)
> > >>>>> - return ret;
> > >>>>> - }
> > >>>>> -
> > >>>>> - if (drvdata->quirks & QUIRK_ROG_ALLY_XPAD) {
> > >>>>> - intf = to_usb_interface(hdev->dev.parent);
> > >>>>> - udev = interface_to_usbdev(intf);
> > >>>>> - validate_mcu_fw_version(hdev,
> > >>>>> - le16_to_cpu(udev->descriptor.idProduct));
> > >>>>> - }
> > >>>>> + ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID);
> > >>>>> + if (ret < 0)
> > >>>>> + return ret;
> > >>>>>
> > >>>>> - } else {
> > >>>>> - /* Initialize keyboard */
> > >>>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID);
> > >>>>> - if (ret < 0)
> > >>>>> - return ret;
> > >>>>> + /* Get keyboard functions */
> > >>>>> + ret = asus_kbd_get_functions(hdev, &kbd_func, FEATURE_KBD_REPORT_ID);
> > >>>>> + if (ret < 0)
> > >>>>> + return ret;
> > >>>>>
> > >>>>> - /* Get keyboard functions */
> > >>>>> - ret = asus_kbd_get_functions(hdev, &kbd_func, FEATURE_KBD_REPORT_ID);
> > >>>>> + if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) {
> > >>>>> + ret = asus_kbd_disable_oobe(hdev);
> > >>>>> if (ret < 0)
> > >>>>> return ret;
> > >>>>> + }
> > >>>>>
> > >>>>> - /* Check for backlight support */
> > >>>>> - if (!(kbd_func & SUPPORT_KBD_BACKLIGHT))
> > >>>>> - return -ENODEV;
> > >>>>> + if (drvdata->quirks & QUIRK_ROG_ALLY_XPAD) {
> > >>>>> + intf = to_usb_interface(hdev->dev.parent);
> > >>>>> + udev = interface_to_usbdev(intf);
> > >>>>> + validate_mcu_fw_version(
> > >>>>> + hdev, le16_to_cpu(udev->descriptor.idProduct));
> > >>>>> }
> > >>>>>
> > >>>>> + /* Check for backlight support */
> > >>>>> + if (!(kbd_func & SUPPORT_KBD_BACKLIGHT))
> > >>>>> + return -ENODEV;
> > >>>>> +
> > >>>>> drvdata->kbd_backlight = devm_kzalloc(&hdev->dev,
> > >>>>> sizeof(struct asus_kbd_leds),
> > >>>>> GFP_KERNEL);
> >
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v7 1/9] HID: asus: simplify RGB init sequence
2025-10-24 16:20 ` Antheas Kapenekakis
@ 2025-10-24 18:53 ` Denis Benato
2025-10-24 21:20 ` Antheas Kapenekakis
0 siblings, 1 reply; 34+ messages in thread
From: Denis Benato @ 2025-10-24 18:53 UTC (permalink / raw)
To: Antheas Kapenekakis
Cc: platform-driver-x86, linux-input, linux-kernel, Jiri Kosina,
Benjamin Tissoires, Corentin Chary, Luke D . Jones, Hans de Goede,
Ilpo Järvinen
On 10/24/25 18:20, Antheas Kapenekakis wrote:
> On Fri, 24 Oct 2025 at 01:25, Antheas Kapenekakis <lkml@antheas.dev> wrote:
>> On Fri, 24 Oct 2025 at 00:53, Denis Benato <benato.denis96@gmail.com> wrote:
>>>
>>> On 10/23/25 23:30, Antheas Kapenekakis wrote:
>>>> On Thu, 23 Oct 2025 at 22:05, Denis Benato <benato.denis96@gmail.com> wrote:
>>>>> On 10/23/25 20:06, Antheas Kapenekakis wrote:
>>>>>> On Thu, 23 Oct 2025 at 19:38, Denis Benato <benato.denis96@gmail.com> wrote:
>>>>>>> On 10/18/25 12:17, Antheas Kapenekakis wrote:
>>>>>>>> Currently, RGB initialization forks depending on whether a device is
>>>>>>>> NKEY. Then, NKEY devices are initialized using 0x5a, 0x5d, 0x5e
>>>>>>>> endpoints, and non-NKEY devices with 0x5a and then a
>>>>>>>> backlight check, which is omitted for NKEY devices.
>>>>>>>>
>>>>>>>> Remove the fork, using a common initialization sequence for both,
>>>>>>>> where they are both only initialized with 0x5a, then checked for
>>>>>>>> backlight support. This patch should not affect existing functionality.
>>>>>>>>
>>>>>>>> 0x5d and 0x5e endpoint initializations are performed by Windows
>>>>>>>> userspace programs associated with different usages that reside under
>>>>>>>> the vendor HID. Specifically, 0x5d is used by Armoury Crate, which
>>>>>>>> controls RGB and 0x5e by an animation program for certain Asus laptops.
>>>>>>>> Neither is used currently in the driver.
>>>>>>> What benefits do we get from removing the unused initialization?
>>>>>>>
>>>>>>> If this has never caused any troubles I don't see the reason for removing
>>>>>>> them. Moreover the lighting protocol is known and I might as well add
>>>>>>> support for it in the near future,
>>>>>> I already have a patch that adds RGB and delay inits that endpoint. It
>>>>>> got removed to make this easier to merge. See [1].
>>>>>>
>>>>>> [1] https://lore.kernel.org/lkml/20250324210151.6042-10-lkml@antheas.dev/
>>>>> I have to main concerns about this:
>>>>>
>>>>> 1. taking away initialization commands in one patchset to make it
>>>>> easier to merge another unrelated patch doesn't seem the right thing
>>>>> to do if the other patch it's not in the same series.
>>>>>
>>>>> I can see [1] has been removed from the set for a later moment in time,
>>>>> it's fine if it needs more work, just send something that function in the
>>>>> same way and do not remove initialization commands when unnecessary,
>>>>> especially since there will be for sure future development.
>>>> The initialization was removed as part of general cleanup. Not to make
>>>> it easier to merge the RGB patch. In addition, the RGB patch only runs
>>>> the init in a lazy fashion, so if nobody uses the RGB sysfs the init
>>>> does not run and the behavior is the same.
>>> There are a few problems here:
>>> 1. sope creep: either do a cleanup or solve bugs. The fact that your flow z13
>>> doesn't load hid-asus correctly has nothing to do with the initialization of anime.
>>> The fact that hid-asus is driving leds instead of asus-wmi has nothing to do with
>>> anime matrix initialization either.
>>> 2. not sending the initialization can get hardware misbehave because it
>>> is left in an uninitialized state.
>>> 3. there are absolutely zero reasons to do that. There are even less reasons
>>> as to do it as part of this patchset.
>>>
>>>>> 2. Your patchset resolves around keyboard backlight control and how
>>>>> the keyboard device is exposed to userspace: it's fine but I do not see
>>>>> the point in removing initialization commands that has nothing to do
>>>>> with the issue we are trying to solve here.
>>>>>
>>>>> Please leave 0x5E and 0x5D initialization commands where they are now.
>>>> I mean the second part of the patchset does that. The first part is a
>>>> cleanup. What would be the reason for keeping 0x5E and 0x5D? They are
>>>> only used when initializing those endpoints to write further commands
>>>> to them and for identification. The current driver does not write
>>>> commands to those endpoints and identifies itself over 0x5A.
>>> There are no bugs opened that ties initialization of devices to bugs.
>>> Quite the opposite: I can guarantee you that removing part of the
>>> init will introduce regressions.
>>>
>>> The onus is on you to provide strong evidence that the removal is
>>> a necessary act.
>>>
>>> Regardless it is not in the scope of this patchset: remove it.
>>>> I do get that it is a bit risky as some laptops might be hardcoded to
>>>> wait for 0x5D to turn on RGB. Which is why we had the last patch until
>>>> V4. But we have yet to find a laptop that has this problem, so I find
>>>> it difficult to justify keeping the init.
>>> Yes it's risky to remove initialization sequences for a device that is
>>> in every modern ASUS laptop and is tied to the EC.
>>>> Do note that you might need to add the 0x5D init to your userspace
>>>> program for certain laptops if you haven't already. But that is ok,
>>>> since in doing so you are also validating you are speaking to an Asus
>>>> device, which is important.
>>> This doesn't make much sense: why would anyone remove
>>> a command from the kernel, that can be very well essential to some models
>>> (sleep can break, for example) just to add it back in a userspace program?
>>>
>>> What does it mean I have to validate I am speaking to an asus device?
>>> Software selects devices by known attribute, one of them is the vid:pid....
>>> Beside what does this have to do with the removal of initialization commands
>>> from the kernel?
>>>
>>> Even late initializing devices can lead to problems. Windows doesn't do that:
>>> as soon as asus drivers are loaded all relevant initialization sequences are
>>> sent; Windows is the only officially supported OS: do not introduce commands
>>> flow divergence without strong reasons backing it up.
>> If you think keeping 0x5D init is that important, I can spin patch [1]
>> into this series. But then this quirk will stay in the kernel forever.
>> I can even add 0x5E since that does not affect newer devices, which I
>> care for simplifying the sequence.
Fully initializing the device tied to the EC in the same windows does
is not a "quirk". Please stop calling it that.
It will stay on the kernel until we have strong evidence that it is causing
problems, at that point we simply avoid doing it for problematic laptops.
If adding other commands doesn't introduce regressions or are otherwise
easy to bisect and makes more hardware working please do.
>> Luke said these two pairs are the important ones to keep.
>>
>> I'm not sure what to do.
> I was asked by a 2025 Asus Zenbook Duo user to add his IDs in [1]. In
> doing so, I updated the rgb and legacy init patches for the new series
> and added a quirk for early init of the duo keyboards.
I will take a look when I can, but if you haven't removed anything
that shouldn't pose any risk. None that I can think of at the moment anyway.
> The series is 14 patches long, I don't think my email can take it :(
linux.dev accounts for maintainers are provided free of charge
and I had to ask for an account too. I suggest you do the same.
> Should we merge the first part of this series with the legacy init,
> then do the backlight refactor, and finally the new Duo stuff + rgb?
I think so. My only doubt is about the per_app quirk. Other than
that looks good and solves one problem while also better representing
the hardware, so I can't think of any blockers.
> Antheas
>
Thanks,
Denis
>> Antheas
>>
>> [1] https://lore.kernel.org/all/20250325184601.10990-12-lkml@antheas.dev/
>>
>>>> Antheas
>>>>
>>> Denis
>>>>>>>> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
>>>>>>>> ---
>>>>>>>> drivers/hid/hid-asus.c | 56 ++++++++++++++----------------------------
>>>>>>>> 1 file changed, 19 insertions(+), 37 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
>>>>>>>> index a444d41e53b6..7ea1037c3979 100644
>>>>>>>> --- a/drivers/hid/hid-asus.c
>>>>>>>> +++ b/drivers/hid/hid-asus.c
>>>>>>>> @@ -638,50 +638,32 @@ static int asus_kbd_register_leds(struct hid_device *hdev)
>>>>>>>> unsigned char kbd_func;
>>>>>>>> int ret;
>>>>>>>>
>>>>>>>> - if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) {
>>>>>>>> - /* Initialize keyboard */
>>>>>>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID);
>>>>>>>> - if (ret < 0)
>>>>>>>> - return ret;
>>>>>>>> -
>>>>>>>> - /* The LED endpoint is initialised in two HID */
>>>>>>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID1);
>>>>>>>> - if (ret < 0)
>>>>>>>> - return ret;
>>>>>>>> -
>>>>>>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID2);
>>>>>>>> - if (ret < 0)
>>>>>>>> - return ret;
>>>>>>>> -
>>>>>>>> - if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) {
>>>>>>>> - ret = asus_kbd_disable_oobe(hdev);
>>>>>>>> - if (ret < 0)
>>>>>>>> - return ret;
>>>>>>>> - }
>>>>>>>> -
>>>>>>>> - if (drvdata->quirks & QUIRK_ROG_ALLY_XPAD) {
>>>>>>>> - intf = to_usb_interface(hdev->dev.parent);
>>>>>>>> - udev = interface_to_usbdev(intf);
>>>>>>>> - validate_mcu_fw_version(hdev,
>>>>>>>> - le16_to_cpu(udev->descriptor.idProduct));
>>>>>>>> - }
>>>>>>>> + ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID);
>>>>>>>> + if (ret < 0)
>>>>>>>> + return ret;
>>>>>>>>
>>>>>>>> - } else {
>>>>>>>> - /* Initialize keyboard */
>>>>>>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID);
>>>>>>>> - if (ret < 0)
>>>>>>>> - return ret;
>>>>>>>> + /* Get keyboard functions */
>>>>>>>> + ret = asus_kbd_get_functions(hdev, &kbd_func, FEATURE_KBD_REPORT_ID);
>>>>>>>> + if (ret < 0)
>>>>>>>> + return ret;
>>>>>>>>
>>>>>>>> - /* Get keyboard functions */
>>>>>>>> - ret = asus_kbd_get_functions(hdev, &kbd_func, FEATURE_KBD_REPORT_ID);
>>>>>>>> + if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) {
>>>>>>>> + ret = asus_kbd_disable_oobe(hdev);
>>>>>>>> if (ret < 0)
>>>>>>>> return ret;
>>>>>>>> + }
>>>>>>>>
>>>>>>>> - /* Check for backlight support */
>>>>>>>> - if (!(kbd_func & SUPPORT_KBD_BACKLIGHT))
>>>>>>>> - return -ENODEV;
>>>>>>>> + if (drvdata->quirks & QUIRK_ROG_ALLY_XPAD) {
>>>>>>>> + intf = to_usb_interface(hdev->dev.parent);
>>>>>>>> + udev = interface_to_usbdev(intf);
>>>>>>>> + validate_mcu_fw_version(
>>>>>>>> + hdev, le16_to_cpu(udev->descriptor.idProduct));
>>>>>>>> }
>>>>>>>>
>>>>>>>> + /* Check for backlight support */
>>>>>>>> + if (!(kbd_func & SUPPORT_KBD_BACKLIGHT))
>>>>>>>> + return -ENODEV;
>>>>>>>> +
>>>>>>>> drvdata->kbd_backlight = devm_kzalloc(&hdev->dev,
>>>>>>>> sizeof(struct asus_kbd_leds),
>>>>>>>> GFP_KERNEL);
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v7 1/9] HID: asus: simplify RGB init sequence
2025-10-24 18:53 ` Denis Benato
@ 2025-10-24 21:20 ` Antheas Kapenekakis
2025-10-25 1:25 ` Denis Benato
0 siblings, 1 reply; 34+ messages in thread
From: Antheas Kapenekakis @ 2025-10-24 21:20 UTC (permalink / raw)
To: Denis Benato
Cc: platform-driver-x86, linux-input, linux-kernel, Jiri Kosina,
Benjamin Tissoires, Corentin Chary, Luke D . Jones, Hans de Goede,
Ilpo Järvinen
On Fri, 24 Oct 2025 at 20:53, Denis Benato <benato.denis96@gmail.com> wrote:
>
>
> On 10/24/25 18:20, Antheas Kapenekakis wrote:
> > On Fri, 24 Oct 2025 at 01:25, Antheas Kapenekakis <lkml@antheas.dev> wrote:
> >> On Fri, 24 Oct 2025 at 00:53, Denis Benato <benato.denis96@gmail.com> wrote:
> >>>
> >>> On 10/23/25 23:30, Antheas Kapenekakis wrote:
> >>>> On Thu, 23 Oct 2025 at 22:05, Denis Benato <benato.denis96@gmail.com> wrote:
> >>>>> On 10/23/25 20:06, Antheas Kapenekakis wrote:
> >>>>>> On Thu, 23 Oct 2025 at 19:38, Denis Benato <benato.denis96@gmail.com> wrote:
> >>>>>>> On 10/18/25 12:17, Antheas Kapenekakis wrote:
> >>>>>>>> Currently, RGB initialization forks depending on whether a device is
> >>>>>>>> NKEY. Then, NKEY devices are initialized using 0x5a, 0x5d, 0x5e
> >>>>>>>> endpoints, and non-NKEY devices with 0x5a and then a
> >>>>>>>> backlight check, which is omitted for NKEY devices.
> >>>>>>>>
> >>>>>>>> Remove the fork, using a common initialization sequence for both,
> >>>>>>>> where they are both only initialized with 0x5a, then checked for
> >>>>>>>> backlight support. This patch should not affect existing functionality.
> >>>>>>>>
> >>>>>>>> 0x5d and 0x5e endpoint initializations are performed by Windows
> >>>>>>>> userspace programs associated with different usages that reside under
> >>>>>>>> the vendor HID. Specifically, 0x5d is used by Armoury Crate, which
> >>>>>>>> controls RGB and 0x5e by an animation program for certain Asus laptops.
> >>>>>>>> Neither is used currently in the driver.
> >>>>>>> What benefits do we get from removing the unused initialization?
> >>>>>>>
> >>>>>>> If this has never caused any troubles I don't see the reason for removing
> >>>>>>> them. Moreover the lighting protocol is known and I might as well add
> >>>>>>> support for it in the near future,
> >>>>>> I already have a patch that adds RGB and delay inits that endpoint. It
> >>>>>> got removed to make this easier to merge. See [1].
> >>>>>>
> >>>>>> [1] https://lore.kernel.org/lkml/20250324210151.6042-10-lkml@antheas.dev/
> >>>>> I have to main concerns about this:
> >>>>>
> >>>>> 1. taking away initialization commands in one patchset to make it
> >>>>> easier to merge another unrelated patch doesn't seem the right thing
> >>>>> to do if the other patch it's not in the same series.
> >>>>>
> >>>>> I can see [1] has been removed from the set for a later moment in time,
> >>>>> it's fine if it needs more work, just send something that function in the
> >>>>> same way and do not remove initialization commands when unnecessary,
> >>>>> especially since there will be for sure future development.
> >>>> The initialization was removed as part of general cleanup. Not to make
> >>>> it easier to merge the RGB patch. In addition, the RGB patch only runs
> >>>> the init in a lazy fashion, so if nobody uses the RGB sysfs the init
> >>>> does not run and the behavior is the same.
> >>> There are a few problems here:
> >>> 1. sope creep: either do a cleanup or solve bugs. The fact that your flow z13
> >>> doesn't load hid-asus correctly has nothing to do with the initialization of anime.
> >>> The fact that hid-asus is driving leds instead of asus-wmi has nothing to do with
> >>> anime matrix initialization either.
> >>> 2. not sending the initialization can get hardware misbehave because it
> >>> is left in an uninitialized state.
> >>> 3. there are absolutely zero reasons to do that. There are even less reasons
> >>> as to do it as part of this patchset.
> >>>
> >>>>> 2. Your patchset resolves around keyboard backlight control and how
> >>>>> the keyboard device is exposed to userspace: it's fine but I do not see
> >>>>> the point in removing initialization commands that has nothing to do
> >>>>> with the issue we are trying to solve here.
> >>>>>
> >>>>> Please leave 0x5E and 0x5D initialization commands where they are now.
> >>>> I mean the second part of the patchset does that. The first part is a
> >>>> cleanup. What would be the reason for keeping 0x5E and 0x5D? They are
> >>>> only used when initializing those endpoints to write further commands
> >>>> to them and for identification. The current driver does not write
> >>>> commands to those endpoints and identifies itself over 0x5A.
> >>> There are no bugs opened that ties initialization of devices to bugs.
> >>> Quite the opposite: I can guarantee you that removing part of the
> >>> init will introduce regressions.
> >>>
> >>> The onus is on you to provide strong evidence that the removal is
> >>> a necessary act.
> >>>
> >>> Regardless it is not in the scope of this patchset: remove it.
> >>>> I do get that it is a bit risky as some laptops might be hardcoded to
> >>>> wait for 0x5D to turn on RGB. Which is why we had the last patch until
> >>>> V4. But we have yet to find a laptop that has this problem, so I find
> >>>> it difficult to justify keeping the init.
> >>> Yes it's risky to remove initialization sequences for a device that is
> >>> in every modern ASUS laptop and is tied to the EC.
> >>>> Do note that you might need to add the 0x5D init to your userspace
> >>>> program for certain laptops if you haven't already. But that is ok,
> >>>> since in doing so you are also validating you are speaking to an Asus
> >>>> device, which is important.
> >>> This doesn't make much sense: why would anyone remove
> >>> a command from the kernel, that can be very well essential to some models
> >>> (sleep can break, for example) just to add it back in a userspace program?
> >>>
> >>> What does it mean I have to validate I am speaking to an asus device?
> >>> Software selects devices by known attribute, one of them is the vid:pid....
> >>> Beside what does this have to do with the removal of initialization commands
> >>> from the kernel?
> >>>
> >>> Even late initializing devices can lead to problems. Windows doesn't do that:
> >>> as soon as asus drivers are loaded all relevant initialization sequences are
> >>> sent; Windows is the only officially supported OS: do not introduce commands
> >>> flow divergence without strong reasons backing it up.
> >> If you think keeping 0x5D init is that important, I can spin patch [1]
> >> into this series. But then this quirk will stay in the kernel forever.
> >> I can even add 0x5E since that does not affect newer devices, which I
> >> care for simplifying the sequence.
> Fully initializing the device tied to the EC in the same windows does
> is not a "quirk". Please stop calling it that.
>
> It will stay on the kernel until we have strong evidence that it is causing
> problems, at that point we simply avoid doing it for problematic laptops.
>
> If adding other commands doesn't introduce regressions or are otherwise
> easy to bisect and makes more hardware working please do.
It is not an init sequence. It is a handshake with the userspace
program that proves to the program it is talking with a genuine asus
device and to the device with the correct program. For all devices
that I have tested it seems to NOOP.
0x5a is the only one used for a driver and it does brightness control.
0x5d/0x5e are used with userspace Windows programs. 0x5d does RGB.
Moreover, the application 0xff310076 only has a single report ID under
it, 0x5a. 0x5d and 0x5e belong to different hid applications that are
not currently checked by the driver (but when they exist they reside
under the same hid endpoint, with a multi-application collection that
bifurcates in Windows to multiple hid devices).
So it makes sense to remove the redundant handshakes. If some laptops
require 0x5d to enable shortcuts as Luke said, I have a patch that
does that and is straightforward to do. But since the shortcut
response comes from the 0x5a endpoint, I find it unlikely for it to
require a handshake over a different endpoint to init.
> >> Luke said these two pairs are the important ones to keep.
> >>
> >> I'm not sure what to do.
> > I was asked by a 2025 Asus Zenbook Duo user to add his IDs in [1]. In
> > doing so, I updated the rgb and legacy init patches for the new series
> > and added a quirk for early init of the duo keyboards.
> I will take a look when I can, but if you haven't removed anything
> that shouldn't pose any risk. None that I can think of at the moment anyway.
> > The series is 14 patches long, I don't think my email can take it :(
> linux.dev accounts for maintainers are provided free of charge
> and I had to ask for an account too. I suggest you do the same.
> > Should we merge the first part of this series with the legacy init,
> > then do the backlight refactor, and finally the new Duo stuff + rgb?
> I think so. My only doubt is about the per_app quirk. Other than
> that looks good and solves one problem while also better representing
> the hardware, so I can't think of any blockers.
> > Antheas
> >
> Thanks,
> Denis
> >> Antheas
> >>
> >> [1] https://lore.kernel.org/all/20250325184601.10990-12-lkml@antheas.dev/
> >>
> >>>> Antheas
> >>>>
> >>> Denis
> >>>>>>>> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> >>>>>>>> ---
> >>>>>>>> drivers/hid/hid-asus.c | 56 ++++++++++++++----------------------------
> >>>>>>>> 1 file changed, 19 insertions(+), 37 deletions(-)
> >>>>>>>>
> >>>>>>>> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> >>>>>>>> index a444d41e53b6..7ea1037c3979 100644
> >>>>>>>> --- a/drivers/hid/hid-asus.c
> >>>>>>>> +++ b/drivers/hid/hid-asus.c
> >>>>>>>> @@ -638,50 +638,32 @@ static int asus_kbd_register_leds(struct hid_device *hdev)
> >>>>>>>> unsigned char kbd_func;
> >>>>>>>> int ret;
> >>>>>>>>
> >>>>>>>> - if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) {
> >>>>>>>> - /* Initialize keyboard */
> >>>>>>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID);
> >>>>>>>> - if (ret < 0)
> >>>>>>>> - return ret;
> >>>>>>>> -
> >>>>>>>> - /* The LED endpoint is initialised in two HID */
> >>>>>>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID1);
> >>>>>>>> - if (ret < 0)
> >>>>>>>> - return ret;
> >>>>>>>> -
> >>>>>>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID2);
> >>>>>>>> - if (ret < 0)
> >>>>>>>> - return ret;
> >>>>>>>> -
> >>>>>>>> - if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) {
> >>>>>>>> - ret = asus_kbd_disable_oobe(hdev);
> >>>>>>>> - if (ret < 0)
> >>>>>>>> - return ret;
> >>>>>>>> - }
> >>>>>>>> -
> >>>>>>>> - if (drvdata->quirks & QUIRK_ROG_ALLY_XPAD) {
> >>>>>>>> - intf = to_usb_interface(hdev->dev.parent);
> >>>>>>>> - udev = interface_to_usbdev(intf);
> >>>>>>>> - validate_mcu_fw_version(hdev,
> >>>>>>>> - le16_to_cpu(udev->descriptor.idProduct));
> >>>>>>>> - }
> >>>>>>>> + ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID);
> >>>>>>>> + if (ret < 0)
> >>>>>>>> + return ret;
> >>>>>>>>
> >>>>>>>> - } else {
> >>>>>>>> - /* Initialize keyboard */
> >>>>>>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID);
> >>>>>>>> - if (ret < 0)
> >>>>>>>> - return ret;
> >>>>>>>> + /* Get keyboard functions */
> >>>>>>>> + ret = asus_kbd_get_functions(hdev, &kbd_func, FEATURE_KBD_REPORT_ID);
> >>>>>>>> + if (ret < 0)
> >>>>>>>> + return ret;
> >>>>>>>>
> >>>>>>>> - /* Get keyboard functions */
> >>>>>>>> - ret = asus_kbd_get_functions(hdev, &kbd_func, FEATURE_KBD_REPORT_ID);
> >>>>>>>> + if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) {
> >>>>>>>> + ret = asus_kbd_disable_oobe(hdev);
> >>>>>>>> if (ret < 0)
> >>>>>>>> return ret;
> >>>>>>>> + }
> >>>>>>>>
> >>>>>>>> - /* Check for backlight support */
> >>>>>>>> - if (!(kbd_func & SUPPORT_KBD_BACKLIGHT))
> >>>>>>>> - return -ENODEV;
> >>>>>>>> + if (drvdata->quirks & QUIRK_ROG_ALLY_XPAD) {
> >>>>>>>> + intf = to_usb_interface(hdev->dev.parent);
> >>>>>>>> + udev = interface_to_usbdev(intf);
> >>>>>>>> + validate_mcu_fw_version(
> >>>>>>>> + hdev, le16_to_cpu(udev->descriptor.idProduct));
> >>>>>>>> }
> >>>>>>>>
> >>>>>>>> + /* Check for backlight support */
> >>>>>>>> + if (!(kbd_func & SUPPORT_KBD_BACKLIGHT))
> >>>>>>>> + return -ENODEV;
> >>>>>>>> +
> >>>>>>>> drvdata->kbd_backlight = devm_kzalloc(&hdev->dev,
> >>>>>>>> sizeof(struct asus_kbd_leds),
> >>>>>>>> GFP_KERNEL);
>
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v7 1/9] HID: asus: simplify RGB init sequence
2025-10-24 21:20 ` Antheas Kapenekakis
@ 2025-10-25 1:25 ` Denis Benato
2025-10-25 7:20 ` Antheas Kapenekakis
0 siblings, 1 reply; 34+ messages in thread
From: Denis Benato @ 2025-10-25 1:25 UTC (permalink / raw)
To: Antheas Kapenekakis
Cc: platform-driver-x86, linux-input, linux-kernel, Jiri Kosina,
Benjamin Tissoires, Corentin Chary, Luke D . Jones, Hans de Goede,
Ilpo Järvinen
On 10/24/25 23:20, Antheas Kapenekakis wrote:
> On Fri, 24 Oct 2025 at 20:53, Denis Benato <benato.denis96@gmail.com> wrote:
>>
>> On 10/24/25 18:20, Antheas Kapenekakis wrote:
>>> On Fri, 24 Oct 2025 at 01:25, Antheas Kapenekakis <lkml@antheas.dev> wrote:
>>>> On Fri, 24 Oct 2025 at 00:53, Denis Benato <benato.denis96@gmail.com> wrote:
>>>>> On 10/23/25 23:30, Antheas Kapenekakis wrote:
>>>>>> On Thu, 23 Oct 2025 at 22:05, Denis Benato <benato.denis96@gmail.com> wrote:
>>>>>>> On 10/23/25 20:06, Antheas Kapenekakis wrote:
>>>>>>>> On Thu, 23 Oct 2025 at 19:38, Denis Benato <benato.denis96@gmail.com> wrote:
>>>>>>>>> On 10/18/25 12:17, Antheas Kapenekakis wrote:
>>>>>>>>>> Currently, RGB initialization forks depending on whether a device is
>>>>>>>>>> NKEY. Then, NKEY devices are initialized using 0x5a, 0x5d, 0x5e
>>>>>>>>>> endpoints, and non-NKEY devices with 0x5a and then a
>>>>>>>>>> backlight check, which is omitted for NKEY devices.
>>>>>>>>>>
>>>>>>>>>> Remove the fork, using a common initialization sequence for both,
>>>>>>>>>> where they are both only initialized with 0x5a, then checked for
>>>>>>>>>> backlight support. This patch should not affect existing functionality.
>>>>>>>>>>
>>>>>>>>>> 0x5d and 0x5e endpoint initializations are performed by Windows
>>>>>>>>>> userspace programs associated with different usages that reside under
>>>>>>>>>> the vendor HID. Specifically, 0x5d is used by Armoury Crate, which
>>>>>>>>>> controls RGB and 0x5e by an animation program for certain Asus laptops.
>>>>>>>>>> Neither is used currently in the driver.
>>>>>>>>> What benefits do we get from removing the unused initialization?
>>>>>>>>>
>>>>>>>>> If this has never caused any troubles I don't see the reason for removing
>>>>>>>>> them. Moreover the lighting protocol is known and I might as well add
>>>>>>>>> support for it in the near future,
>>>>>>>> I already have a patch that adds RGB and delay inits that endpoint. It
>>>>>>>> got removed to make this easier to merge. See [1].
>>>>>>>>
>>>>>>>> [1] https://lore.kernel.org/lkml/20250324210151.6042-10-lkml@antheas.dev/
>>>>>>> I have to main concerns about this:
>>>>>>>
>>>>>>> 1. taking away initialization commands in one patchset to make it
>>>>>>> easier to merge another unrelated patch doesn't seem the right thing
>>>>>>> to do if the other patch it's not in the same series.
>>>>>>>
>>>>>>> I can see [1] has been removed from the set for a later moment in time,
>>>>>>> it's fine if it needs more work, just send something that function in the
>>>>>>> same way and do not remove initialization commands when unnecessary,
>>>>>>> especially since there will be for sure future development.
>>>>>> The initialization was removed as part of general cleanup. Not to make
>>>>>> it easier to merge the RGB patch. In addition, the RGB patch only runs
>>>>>> the init in a lazy fashion, so if nobody uses the RGB sysfs the init
>>>>>> does not run and the behavior is the same.
>>>>> There are a few problems here:
>>>>> 1. sope creep: either do a cleanup or solve bugs. The fact that your flow z13
>>>>> doesn't load hid-asus correctly has nothing to do with the initialization of anime.
>>>>> The fact that hid-asus is driving leds instead of asus-wmi has nothing to do with
>>>>> anime matrix initialization either.
>>>>> 2. not sending the initialization can get hardware misbehave because it
>>>>> is left in an uninitialized state.
>>>>> 3. there are absolutely zero reasons to do that. There are even less reasons
>>>>> as to do it as part of this patchset.
>>>>>
>>>>>>> 2. Your patchset resolves around keyboard backlight control and how
>>>>>>> the keyboard device is exposed to userspace: it's fine but I do not see
>>>>>>> the point in removing initialization commands that has nothing to do
>>>>>>> with the issue we are trying to solve here.
>>>>>>>
>>>>>>> Please leave 0x5E and 0x5D initialization commands where they are now.
>>>>>> I mean the second part of the patchset does that. The first part is a
>>>>>> cleanup. What would be the reason for keeping 0x5E and 0x5D? They are
>>>>>> only used when initializing those endpoints to write further commands
>>>>>> to them and for identification. The current driver does not write
>>>>>> commands to those endpoints and identifies itself over 0x5A.
>>>>> There are no bugs opened that ties initialization of devices to bugs.
>>>>> Quite the opposite: I can guarantee you that removing part of the
>>>>> init will introduce regressions.
>>>>>
>>>>> The onus is on you to provide strong evidence that the removal is
>>>>> a necessary act.
>>>>>
>>>>> Regardless it is not in the scope of this patchset: remove it.
>>>>>> I do get that it is a bit risky as some laptops might be hardcoded to
>>>>>> wait for 0x5D to turn on RGB. Which is why we had the last patch until
>>>>>> V4. But we have yet to find a laptop that has this problem, so I find
>>>>>> it difficult to justify keeping the init.
>>>>> Yes it's risky to remove initialization sequences for a device that is
>>>>> in every modern ASUS laptop and is tied to the EC.
>>>>>> Do note that you might need to add the 0x5D init to your userspace
>>>>>> program for certain laptops if you haven't already. But that is ok,
>>>>>> since in doing so you are also validating you are speaking to an Asus
>>>>>> device, which is important.
>>>>> This doesn't make much sense: why would anyone remove
>>>>> a command from the kernel, that can be very well essential to some models
>>>>> (sleep can break, for example) just to add it back in a userspace program?
>>>>>
>>>>> What does it mean I have to validate I am speaking to an asus device?
>>>>> Software selects devices by known attribute, one of them is the vid:pid....
>>>>> Beside what does this have to do with the removal of initialization commands
>>>>> from the kernel?
>>>>>
>>>>> Even late initializing devices can lead to problems. Windows doesn't do that:
>>>>> as soon as asus drivers are loaded all relevant initialization sequences are
>>>>> sent; Windows is the only officially supported OS: do not introduce commands
>>>>> flow divergence without strong reasons backing it up.
>>>> If you think keeping 0x5D init is that important, I can spin patch [1]
>>>> into this series. But then this quirk will stay in the kernel forever.
>>>> I can even add 0x5E since that does not affect newer devices, which I
>>>> care for simplifying the sequence.
>> Fully initializing the device tied to the EC in the same windows does
>> is not a "quirk". Please stop calling it that.
>>
>> It will stay on the kernel until we have strong evidence that it is causing
>> problems, at that point we simply avoid doing it for problematic laptops.
>>
>> If adding other commands doesn't introduce regressions or are otherwise
>> easy to bisect and makes more hardware working please do.
> It is not an init sequence. It is a handshake with the userspace
> program that proves to the program it is talking with a genuine asus
> device and to the device with the correct program. For all devices
> that I have tested it seems to NOOP.
The MCU doesn't distinguish between userspace or kernel space:
"it is a handshake" => yeah handshakes are part of initialization procedures.
"with the userspace program [...]" => MCU does not care where data is coming from.
Anyway further discussion is useless. We understood you are against
keeping commands that that you believe are useless, but sometimes
software is like life: you have to accept compromises.
> 0x5a is the only one used for a driver and it does brightness control.
> 0x5d/0x5e are used with userspace Windows programs. 0x5d does RGB.
> Moreover, the application 0xff310076 only has a single report ID under
> it, 0x5a. 0x5d and 0x5e belong to different hid applications that are
> not currently checked by the driver (but when they exist they reside
> under the same hid endpoint, with a multi-application collection that
> bifurcates in Windows to multiple hid devices).
The MCU works as a state machine where the status is updated
on sleep, power on and power off: not sending initialization commands
will confuse (some) hardware. If not now in a few years when some user will
migrate away from whatever debian 12 they are running.
Those commands are not simple commands to "just init the device" as you
are depicting here. Stop doing that.
> So it makes sense to remove the redundant handshakes. If some laptops
> require 0x5d to enable shortcuts as Luke said, I have a patch that
> does that and is straightforward to do. But since the shortcut
> response comes from the 0x5a endpoint, I find it unlikely for it to
> require a handshake over a different endpoint to init.
So you want to remove some code, that has caused no troubles, on
the assumption that such removal won't have any visible consequence,
but you have a patch ready to restore the previous behavior in case
something goes wrong? It doesn't matter if you find it unlikely or not:
either there is a strong reason to remove it or there is not: you finding
such removal "unlikely" to break anything down the line is not a strong
reason to remove it. The fact that you don't like some code is not a strong
reason to remove it either.
Antheas... your flow z13 isn't loading correctly. Focus on the issue
at hands. Please.
>>>> Luke said these two pairs are the important ones to keep.
>>>>
>>>> I'm not sure what to do.
>>> I was asked by a 2025 Asus Zenbook Duo user to add his IDs in [1]. In
>>> doing so, I updated the rgb and legacy init patches for the new series
>>> and added a quirk for early init of the duo keyboards.
>> I will take a look when I can, but if you haven't removed anything
>> that shouldn't pose any risk. None that I can think of at the moment anyway.
>>> The series is 14 patches long, I don't think my email can take it :(
>> linux.dev accounts for maintainers are provided free of charge
>> and I had to ask for an account too. I suggest you do the same.
>>> Should we merge the first part of this series with the legacy init,
>>> then do the backlight refactor, and finally the new Duo stuff + rgb?
>> I think so. My only doubt is about the per_app quirk. Other than
>> that looks good and solves one problem while also better representing
>> the hardware, so I can't think of any blockers.
>>> Antheas
>>>
>> Thanks,
>> Denis
>>>> Antheas
>>>>
>>>> [1] https://lore.kernel.org/all/20250325184601.10990-12-lkml@antheas.dev/
>>>>
>>>>>> Antheas
>>>>>>
>>>>> Denis
>>>>>>>>>> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
>>>>>>>>>> ---
>>>>>>>>>> drivers/hid/hid-asus.c | 56 ++++++++++++++----------------------------
>>>>>>>>>> 1 file changed, 19 insertions(+), 37 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
>>>>>>>>>> index a444d41e53b6..7ea1037c3979 100644
>>>>>>>>>> --- a/drivers/hid/hid-asus.c
>>>>>>>>>> +++ b/drivers/hid/hid-asus.c
>>>>>>>>>> @@ -638,50 +638,32 @@ static int asus_kbd_register_leds(struct hid_device *hdev)
>>>>>>>>>> unsigned char kbd_func;
>>>>>>>>>> int ret;
>>>>>>>>>>
>>>>>>>>>> - if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) {
>>>>>>>>>> - /* Initialize keyboard */
>>>>>>>>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID);
>>>>>>>>>> - if (ret < 0)
>>>>>>>>>> - return ret;
>>>>>>>>>> -
>>>>>>>>>> - /* The LED endpoint is initialised in two HID */
>>>>>>>>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID1);
>>>>>>>>>> - if (ret < 0)
>>>>>>>>>> - return ret;
>>>>>>>>>> -
>>>>>>>>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID2);
>>>>>>>>>> - if (ret < 0)
>>>>>>>>>> - return ret;
>>>>>>>>>> -
>>>>>>>>>> - if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) {
>>>>>>>>>> - ret = asus_kbd_disable_oobe(hdev);
>>>>>>>>>> - if (ret < 0)
>>>>>>>>>> - return ret;
>>>>>>>>>> - }
>>>>>>>>>> -
>>>>>>>>>> - if (drvdata->quirks & QUIRK_ROG_ALLY_XPAD) {
>>>>>>>>>> - intf = to_usb_interface(hdev->dev.parent);
>>>>>>>>>> - udev = interface_to_usbdev(intf);
>>>>>>>>>> - validate_mcu_fw_version(hdev,
>>>>>>>>>> - le16_to_cpu(udev->descriptor.idProduct));
>>>>>>>>>> - }
>>>>>>>>>> + ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID);
>>>>>>>>>> + if (ret < 0)
>>>>>>>>>> + return ret;
>>>>>>>>>>
>>>>>>>>>> - } else {
>>>>>>>>>> - /* Initialize keyboard */
>>>>>>>>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID);
>>>>>>>>>> - if (ret < 0)
>>>>>>>>>> - return ret;
>>>>>>>>>> + /* Get keyboard functions */
>>>>>>>>>> + ret = asus_kbd_get_functions(hdev, &kbd_func, FEATURE_KBD_REPORT_ID);
>>>>>>>>>> + if (ret < 0)
>>>>>>>>>> + return ret;
>>>>>>>>>>
>>>>>>>>>> - /* Get keyboard functions */
>>>>>>>>>> - ret = asus_kbd_get_functions(hdev, &kbd_func, FEATURE_KBD_REPORT_ID);
>>>>>>>>>> + if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) {
>>>>>>>>>> + ret = asus_kbd_disable_oobe(hdev);
>>>>>>>>>> if (ret < 0)
>>>>>>>>>> return ret;
>>>>>>>>>> + }
>>>>>>>>>>
>>>>>>>>>> - /* Check for backlight support */
>>>>>>>>>> - if (!(kbd_func & SUPPORT_KBD_BACKLIGHT))
>>>>>>>>>> - return -ENODEV;
>>>>>>>>>> + if (drvdata->quirks & QUIRK_ROG_ALLY_XPAD) {
>>>>>>>>>> + intf = to_usb_interface(hdev->dev.parent);
>>>>>>>>>> + udev = interface_to_usbdev(intf);
>>>>>>>>>> + validate_mcu_fw_version(
>>>>>>>>>> + hdev, le16_to_cpu(udev->descriptor.idProduct));
>>>>>>>>>> }
>>>>>>>>>>
>>>>>>>>>> + /* Check for backlight support */
>>>>>>>>>> + if (!(kbd_func & SUPPORT_KBD_BACKLIGHT))
>>>>>>>>>> + return -ENODEV;
>>>>>>>>>> +
>>>>>>>>>> drvdata->kbd_backlight = devm_kzalloc(&hdev->dev,
>>>>>>>>>> sizeof(struct asus_kbd_leds),
>>>>>>>>>> GFP_KERNEL);
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v7 1/9] HID: asus: simplify RGB init sequence
2025-10-25 1:25 ` Denis Benato
@ 2025-10-25 7:20 ` Antheas Kapenekakis
0 siblings, 0 replies; 34+ messages in thread
From: Antheas Kapenekakis @ 2025-10-25 7:20 UTC (permalink / raw)
To: Denis Benato
Cc: platform-driver-x86, linux-input, linux-kernel, Jiri Kosina,
Benjamin Tissoires, Corentin Chary, Luke D . Jones, Hans de Goede,
Ilpo Järvinen
On Sat, 25 Oct 2025 at 03:25, Denis Benato <benato.denis96@gmail.com> wrote:
>
>
> On 10/24/25 23:20, Antheas Kapenekakis wrote:
> > On Fri, 24 Oct 2025 at 20:53, Denis Benato <benato.denis96@gmail.com> wrote:
> >>
> >> On 10/24/25 18:20, Antheas Kapenekakis wrote:
> >>> On Fri, 24 Oct 2025 at 01:25, Antheas Kapenekakis <lkml@antheas.dev> wrote:
> >>>> On Fri, 24 Oct 2025 at 00:53, Denis Benato <benato.denis96@gmail.com> wrote:
> >>>>> On 10/23/25 23:30, Antheas Kapenekakis wrote:
> >>>>>> On Thu, 23 Oct 2025 at 22:05, Denis Benato <benato.denis96@gmail.com> wrote:
> >>>>>>> On 10/23/25 20:06, Antheas Kapenekakis wrote:
> >>>>>>>> On Thu, 23 Oct 2025 at 19:38, Denis Benato <benato.denis96@gmail.com> wrote:
> >>>>>>>>> On 10/18/25 12:17, Antheas Kapenekakis wrote:
> >>>>>>>>>> Currently, RGB initialization forks depending on whether a device is
> >>>>>>>>>> NKEY. Then, NKEY devices are initialized using 0x5a, 0x5d, 0x5e
> >>>>>>>>>> endpoints, and non-NKEY devices with 0x5a and then a
> >>>>>>>>>> backlight check, which is omitted for NKEY devices.
> >>>>>>>>>>
> >>>>>>>>>> Remove the fork, using a common initialization sequence for both,
> >>>>>>>>>> where they are both only initialized with 0x5a, then checked for
> >>>>>>>>>> backlight support. This patch should not affect existing functionality.
> >>>>>>>>>>
> >>>>>>>>>> 0x5d and 0x5e endpoint initializations are performed by Windows
> >>>>>>>>>> userspace programs associated with different usages that reside under
> >>>>>>>>>> the vendor HID. Specifically, 0x5d is used by Armoury Crate, which
> >>>>>>>>>> controls RGB and 0x5e by an animation program for certain Asus laptops.
> >>>>>>>>>> Neither is used currently in the driver.
> >>>>>>>>> What benefits do we get from removing the unused initialization?
> >>>>>>>>>
> >>>>>>>>> If this has never caused any troubles I don't see the reason for removing
> >>>>>>>>> them. Moreover the lighting protocol is known and I might as well add
> >>>>>>>>> support for it in the near future,
> >>>>>>>> I already have a patch that adds RGB and delay inits that endpoint. It
> >>>>>>>> got removed to make this easier to merge. See [1].
> >>>>>>>>
> >>>>>>>> [1] https://lore.kernel.org/lkml/20250324210151.6042-10-lkml@antheas.dev/
> >>>>>>> I have to main concerns about this:
> >>>>>>>
> >>>>>>> 1. taking away initialization commands in one patchset to make it
> >>>>>>> easier to merge another unrelated patch doesn't seem the right thing
> >>>>>>> to do if the other patch it's not in the same series.
> >>>>>>>
> >>>>>>> I can see [1] has been removed from the set for a later moment in time,
> >>>>>>> it's fine if it needs more work, just send something that function in the
> >>>>>>> same way and do not remove initialization commands when unnecessary,
> >>>>>>> especially since there will be for sure future development.
> >>>>>> The initialization was removed as part of general cleanup. Not to make
> >>>>>> it easier to merge the RGB patch. In addition, the RGB patch only runs
> >>>>>> the init in a lazy fashion, so if nobody uses the RGB sysfs the init
> >>>>>> does not run and the behavior is the same.
> >>>>> There are a few problems here:
> >>>>> 1. sope creep: either do a cleanup or solve bugs. The fact that your flow z13
> >>>>> doesn't load hid-asus correctly has nothing to do with the initialization of anime.
> >>>>> The fact that hid-asus is driving leds instead of asus-wmi has nothing to do with
> >>>>> anime matrix initialization either.
> >>>>> 2. not sending the initialization can get hardware misbehave because it
> >>>>> is left in an uninitialized state.
> >>>>> 3. there are absolutely zero reasons to do that. There are even less reasons
> >>>>> as to do it as part of this patchset.
> >>>>>
> >>>>>>> 2. Your patchset resolves around keyboard backlight control and how
> >>>>>>> the keyboard device is exposed to userspace: it's fine but I do not see
> >>>>>>> the point in removing initialization commands that has nothing to do
> >>>>>>> with the issue we are trying to solve here.
> >>>>>>>
> >>>>>>> Please leave 0x5E and 0x5D initialization commands where they are now.
> >>>>>> I mean the second part of the patchset does that. The first part is a
> >>>>>> cleanup. What would be the reason for keeping 0x5E and 0x5D? They are
> >>>>>> only used when initializing those endpoints to write further commands
> >>>>>> to them and for identification. The current driver does not write
> >>>>>> commands to those endpoints and identifies itself over 0x5A.
> >>>>> There are no bugs opened that ties initialization of devices to bugs.
> >>>>> Quite the opposite: I can guarantee you that removing part of the
> >>>>> init will introduce regressions.
> >>>>>
> >>>>> The onus is on you to provide strong evidence that the removal is
> >>>>> a necessary act.
> >>>>>
> >>>>> Regardless it is not in the scope of this patchset: remove it.
> >>>>>> I do get that it is a bit risky as some laptops might be hardcoded to
> >>>>>> wait for 0x5D to turn on RGB. Which is why we had the last patch until
> >>>>>> V4. But we have yet to find a laptop that has this problem, so I find
> >>>>>> it difficult to justify keeping the init.
> >>>>> Yes it's risky to remove initialization sequences for a device that is
> >>>>> in every modern ASUS laptop and is tied to the EC.
> >>>>>> Do note that you might need to add the 0x5D init to your userspace
> >>>>>> program for certain laptops if you haven't already. But that is ok,
> >>>>>> since in doing so you are also validating you are speaking to an Asus
> >>>>>> device, which is important.
> >>>>> This doesn't make much sense: why would anyone remove
> >>>>> a command from the kernel, that can be very well essential to some models
> >>>>> (sleep can break, for example) just to add it back in a userspace program?
> >>>>>
> >>>>> What does it mean I have to validate I am speaking to an asus device?
> >>>>> Software selects devices by known attribute, one of them is the vid:pid....
> >>>>> Beside what does this have to do with the removal of initialization commands
> >>>>> from the kernel?
> >>>>>
> >>>>> Even late initializing devices can lead to problems. Windows doesn't do that:
> >>>>> as soon as asus drivers are loaded all relevant initialization sequences are
> >>>>> sent; Windows is the only officially supported OS: do not introduce commands
> >>>>> flow divergence without strong reasons backing it up.
> >>>> If you think keeping 0x5D init is that important, I can spin patch [1]
> >>>> into this series. But then this quirk will stay in the kernel forever.
> >>>> I can even add 0x5E since that does not affect newer devices, which I
> >>>> care for simplifying the sequence.
> >> Fully initializing the device tied to the EC in the same windows does
> >> is not a "quirk". Please stop calling it that.
> >>
> >> It will stay on the kernel until we have strong evidence that it is causing
> >> problems, at that point we simply avoid doing it for problematic laptops.
> >>
> >> If adding other commands doesn't introduce regressions or are otherwise
> >> easy to bisect and makes more hardware working please do.
> > It is not an init sequence. It is a handshake with the userspace
> > program that proves to the program it is talking with a genuine asus
> > device and to the device with the correct program. For all devices
> > that I have tested it seems to NOOP.
> The MCU doesn't distinguish between userspace or kernel space:
> "it is a handshake" => yeah handshakes are part of initialization procedures.
> "with the userspace program [...]" => MCU does not care where data is coming from.
>
> Anyway further discussion is useless. We understood you are against
> keeping commands that that you believe are useless, but sometimes
> software is like life: you have to accept compromises.
> > 0x5a is the only one used for a driver and it does brightness control.
> > 0x5d/0x5e are used with userspace Windows programs. 0x5d does RGB.
> > Moreover, the application 0xff310076 only has a single report ID under
> > it, 0x5a. 0x5d and 0x5e belong to different hid applications that are
> > not currently checked by the driver (but when they exist they reside
> > under the same hid endpoint, with a multi-application collection that
> > bifurcates in Windows to multiple hid devices).
> The MCU works as a state machine where the status is updated
> on sleep, power on and power off: not sending initialization commands
> will confuse (some) hardware. If not now in a few years when some user will
> migrate away from whatever debian 12 they are running.
>
> Those commands are not simple commands to "just init the device" as you
> are depicting here. Stop doing that.
> > So it makes sense to remove the redundant handshakes. If some laptops
> > require 0x5d to enable shortcuts as Luke said, I have a patch that
> > does that and is straightforward to do. But since the shortcut
> > response comes from the 0x5a endpoint, I find it unlikely for it to
> > require a handshake over a different endpoint to init.
> So you want to remove some code, that has caused no troubles, on
> the assumption that such removal won't have any visible consequence,
> but you have a patch ready to restore the previous behavior in case
> something goes wrong? It doesn't matter if you find it unlikely or not:
> either there is a strong reason to remove it or there is not: you finding
> such removal "unlikely" to break anything down the line is not a strong
> reason to remove it. The fact that you don't like some code is not a strong
> reason to remove it either.
>
> Antheas... your flow z13 isn't loading correctly. Focus on the issue
> at hands. Please.
>
If that initialization means that much to you I will re-add the patch
that runs it on old laptops. No need to crash out
> >>>> Luke said these two pairs are the important ones to keep.
> >>>>
> >>>> I'm not sure what to do.
> >>> I was asked by a 2025 Asus Zenbook Duo user to add his IDs in [1]. In
> >>> doing so, I updated the rgb and legacy init patches for the new series
> >>> and added a quirk for early init of the duo keyboards.
> >> I will take a look when I can, but if you haven't removed anything
> >> that shouldn't pose any risk. None that I can think of at the moment anyway.
> >>> The series is 14 patches long, I don't think my email can take it :(
> >> linux.dev accounts for maintainers are provided free of charge
> >> and I had to ask for an account too. I suggest you do the same.
> >>> Should we merge the first part of this series with the legacy init,
> >>> then do the backlight refactor, and finally the new Duo stuff + rgb?
> >> I think so. My only doubt is about the per_app quirk. Other than
> >> that looks good and solves one problem while also better representing
> >> the hardware, so I can't think of any blockers.
> >>> Antheas
> >>>
> >> Thanks,
> >> Denis
> >>>> Antheas
> >>>>
> >>>> [1] https://lore.kernel.org/all/20250325184601.10990-12-lkml@antheas.dev/
> >>>>
> >>>>>> Antheas
> >>>>>>
> >>>>> Denis
> >>>>>>>>>> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> >>>>>>>>>> ---
> >>>>>>>>>> drivers/hid/hid-asus.c | 56 ++++++++++++++----------------------------
> >>>>>>>>>> 1 file changed, 19 insertions(+), 37 deletions(-)
> >>>>>>>>>>
> >>>>>>>>>> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> >>>>>>>>>> index a444d41e53b6..7ea1037c3979 100644
> >>>>>>>>>> --- a/drivers/hid/hid-asus.c
> >>>>>>>>>> +++ b/drivers/hid/hid-asus.c
> >>>>>>>>>> @@ -638,50 +638,32 @@ static int asus_kbd_register_leds(struct hid_device *hdev)
> >>>>>>>>>> unsigned char kbd_func;
> >>>>>>>>>> int ret;
> >>>>>>>>>>
> >>>>>>>>>> - if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) {
> >>>>>>>>>> - /* Initialize keyboard */
> >>>>>>>>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID);
> >>>>>>>>>> - if (ret < 0)
> >>>>>>>>>> - return ret;
> >>>>>>>>>> -
> >>>>>>>>>> - /* The LED endpoint is initialised in two HID */
> >>>>>>>>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID1);
> >>>>>>>>>> - if (ret < 0)
> >>>>>>>>>> - return ret;
> >>>>>>>>>> -
> >>>>>>>>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID2);
> >>>>>>>>>> - if (ret < 0)
> >>>>>>>>>> - return ret;
> >>>>>>>>>> -
> >>>>>>>>>> - if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) {
> >>>>>>>>>> - ret = asus_kbd_disable_oobe(hdev);
> >>>>>>>>>> - if (ret < 0)
> >>>>>>>>>> - return ret;
> >>>>>>>>>> - }
> >>>>>>>>>> -
> >>>>>>>>>> - if (drvdata->quirks & QUIRK_ROG_ALLY_XPAD) {
> >>>>>>>>>> - intf = to_usb_interface(hdev->dev.parent);
> >>>>>>>>>> - udev = interface_to_usbdev(intf);
> >>>>>>>>>> - validate_mcu_fw_version(hdev,
> >>>>>>>>>> - le16_to_cpu(udev->descriptor.idProduct));
> >>>>>>>>>> - }
> >>>>>>>>>> + ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID);
> >>>>>>>>>> + if (ret < 0)
> >>>>>>>>>> + return ret;
> >>>>>>>>>>
> >>>>>>>>>> - } else {
> >>>>>>>>>> - /* Initialize keyboard */
> >>>>>>>>>> - ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID);
> >>>>>>>>>> - if (ret < 0)
> >>>>>>>>>> - return ret;
> >>>>>>>>>> + /* Get keyboard functions */
> >>>>>>>>>> + ret = asus_kbd_get_functions(hdev, &kbd_func, FEATURE_KBD_REPORT_ID);
> >>>>>>>>>> + if (ret < 0)
> >>>>>>>>>> + return ret;
> >>>>>>>>>>
> >>>>>>>>>> - /* Get keyboard functions */
> >>>>>>>>>> - ret = asus_kbd_get_functions(hdev, &kbd_func, FEATURE_KBD_REPORT_ID);
> >>>>>>>>>> + if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) {
> >>>>>>>>>> + ret = asus_kbd_disable_oobe(hdev);
> >>>>>>>>>> if (ret < 0)
> >>>>>>>>>> return ret;
> >>>>>>>>>> + }
> >>>>>>>>>>
> >>>>>>>>>> - /* Check for backlight support */
> >>>>>>>>>> - if (!(kbd_func & SUPPORT_KBD_BACKLIGHT))
> >>>>>>>>>> - return -ENODEV;
> >>>>>>>>>> + if (drvdata->quirks & QUIRK_ROG_ALLY_XPAD) {
> >>>>>>>>>> + intf = to_usb_interface(hdev->dev.parent);
> >>>>>>>>>> + udev = interface_to_usbdev(intf);
> >>>>>>>>>> + validate_mcu_fw_version(
> >>>>>>>>>> + hdev, le16_to_cpu(udev->descriptor.idProduct));
> >>>>>>>>>> }
> >>>>>>>>>>
> >>>>>>>>>> + /* Check for backlight support */
> >>>>>>>>>> + if (!(kbd_func & SUPPORT_KBD_BACKLIGHT))
> >>>>>>>>>> + return -ENODEV;
> >>>>>>>>>> +
> >>>>>>>>>> drvdata->kbd_backlight = devm_kzalloc(&hdev->dev,
> >>>>>>>>>> sizeof(struct asus_kbd_leds),
> >>>>>>>>>> GFP_KERNEL);
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v7 2/9] HID: asus: use same report_id in response
2025-10-18 10:17 [PATCH v7 0/9] HID: asus: Fix ASUS ROG Laptop's Keyboard backlight handling Antheas Kapenekakis
2025-10-18 10:17 ` [PATCH v7 1/9] HID: asus: simplify RGB init sequence Antheas Kapenekakis
@ 2025-10-18 10:17 ` Antheas Kapenekakis
2025-10-18 10:17 ` [PATCH v7 3/9] HID: asus: fortify keyboard handshake Antheas Kapenekakis
` (6 subsequent siblings)
8 siblings, 0 replies; 34+ messages in thread
From: Antheas Kapenekakis @ 2025-10-18 10:17 UTC (permalink / raw)
To: platform-driver-x86, linux-input
Cc: linux-kernel, Jiri Kosina, Benjamin Tissoires, Corentin Chary,
Luke D . Jones, Hans de Goede, Ilpo Järvinen, Denis Benato,
Antheas Kapenekakis
Currently, asus_kbd_get_functions prods the device using feature
report report_id, but then is hardcoded to check the response through
FEATURE_KBD_REPORT_ID. This only works if report_id is that value
(currently true). So, use report_id in the response as well to
maintain functionality if that value changes in the future.
Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
---
drivers/hid/hid-asus.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index 7ea1037c3979..4676b7f20caf 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -422,7 +422,7 @@ static int asus_kbd_get_functions(struct hid_device *hdev,
if (!readbuf)
return -ENOMEM;
- ret = hid_hw_raw_request(hdev, FEATURE_KBD_REPORT_ID, readbuf,
+ ret = hid_hw_raw_request(hdev, report_id, readbuf,
FEATURE_KBD_REPORT_SIZE, HID_FEATURE_REPORT,
HID_REQ_GET_REPORT);
if (ret < 0) {
--
2.51.0
^ permalink raw reply related [flat|nested] 34+ messages in thread* [PATCH v7 3/9] HID: asus: fortify keyboard handshake
2025-10-18 10:17 [PATCH v7 0/9] HID: asus: Fix ASUS ROG Laptop's Keyboard backlight handling Antheas Kapenekakis
2025-10-18 10:17 ` [PATCH v7 1/9] HID: asus: simplify RGB init sequence Antheas Kapenekakis
2025-10-18 10:17 ` [PATCH v7 2/9] HID: asus: use same report_id in response Antheas Kapenekakis
@ 2025-10-18 10:17 ` Antheas Kapenekakis
2025-10-18 10:17 ` [PATCH v7 4/9] HID: asus: prevent binding to all HID devices on ROG Antheas Kapenekakis
` (5 subsequent siblings)
8 siblings, 0 replies; 34+ messages in thread
From: Antheas Kapenekakis @ 2025-10-18 10:17 UTC (permalink / raw)
To: platform-driver-x86, linux-input
Cc: linux-kernel, Jiri Kosina, Benjamin Tissoires, Corentin Chary,
Luke D . Jones, Hans de Goede, Ilpo Järvinen, Denis Benato,
Antheas Kapenekakis
Handshaking with an Asus device involves sending it a feature report
with the string "ASUS Tech.Inc." and then reading it back to verify the
handshake was successful, under the feature ID the interaction will
take place.
Currently, the driver only does the first part. Add the readback to
verify the handshake was successful. As this could cause breakages,
allow the verification to fail with a dmesg error until we verify
all devices work with it (they seem to).
Since the response is more than 16 bytes, increase the buffer size
to 64 as well to avoid overflow errors.
Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
---
drivers/hid/hid-asus.c | 32 +++++++++++++++++++++++++++++---
1 file changed, 29 insertions(+), 3 deletions(-)
diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index 4676b7f20caf..03f0d86936fc 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -48,7 +48,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
#define FEATURE_REPORT_ID 0x0d
#define INPUT_REPORT_ID 0x5d
#define FEATURE_KBD_REPORT_ID 0x5a
-#define FEATURE_KBD_REPORT_SIZE 16
+#define FEATURE_KBD_REPORT_SIZE 64
#define FEATURE_KBD_LED_REPORT_ID1 0x5d
#define FEATURE_KBD_LED_REPORT_ID2 0x5e
@@ -393,14 +393,40 @@ static int asus_kbd_set_report(struct hid_device *hdev, const u8 *buf, size_t bu
static int asus_kbd_init(struct hid_device *hdev, u8 report_id)
{
+ /*
+ * The handshake is first sent as a set_report, then retrieved
+ * from a get_report. They should be equal.
+ */
const u8 buf[] = { report_id, 0x41, 0x53, 0x55, 0x53, 0x20, 0x54,
0x65, 0x63, 0x68, 0x2e, 0x49, 0x6e, 0x63, 0x2e, 0x00 };
+ u8 *readbuf;
int ret;
ret = asus_kbd_set_report(hdev, buf, sizeof(buf));
- if (ret < 0)
- hid_err(hdev, "Asus failed to send init command: %d\n", ret);
+ if (ret < 0) {
+ hid_err(hdev, "Asus failed to send handshake: %d\n", ret);
+ return ret;
+ }
+
+ readbuf = kzalloc(FEATURE_KBD_REPORT_SIZE, GFP_KERNEL);
+ if (!readbuf)
+ return -ENOMEM;
+
+ ret = hid_hw_raw_request(hdev, report_id, readbuf,
+ FEATURE_KBD_REPORT_SIZE, HID_FEATURE_REPORT,
+ HID_REQ_GET_REPORT);
+ if (ret < 0) {
+ hid_err(hdev, "Asus failed to receive handshake ack: %d\n", ret);
+ } else if (memcmp(readbuf, buf, sizeof(buf)) != 0) {
+ hid_warn(hdev, "Asus handshake returned invalid response: %*ph\n",
+ FEATURE_KBD_REPORT_SIZE, readbuf);
+ /*
+ * Do not return error if handshake is wrong until this is
+ * verified to work for all devices.
+ */
+ }
+ kfree(readbuf);
return ret;
}
--
2.51.0
^ permalink raw reply related [flat|nested] 34+ messages in thread* [PATCH v7 4/9] HID: asus: prevent binding to all HID devices on ROG
2025-10-18 10:17 [PATCH v7 0/9] HID: asus: Fix ASUS ROG Laptop's Keyboard backlight handling Antheas Kapenekakis
` (2 preceding siblings ...)
2025-10-18 10:17 ` [PATCH v7 3/9] HID: asus: fortify keyboard handshake Antheas Kapenekakis
@ 2025-10-18 10:17 ` Antheas Kapenekakis
2025-10-23 18:23 ` Denis Benato
2025-10-18 10:17 ` [PATCH v7 5/9] platform/x86: asus-wmi: Add support for multiple kbd led handlers Antheas Kapenekakis
` (4 subsequent siblings)
8 siblings, 1 reply; 34+ messages in thread
From: Antheas Kapenekakis @ 2025-10-18 10:17 UTC (permalink / raw)
To: platform-driver-x86, linux-input
Cc: linux-kernel, Jiri Kosina, Benjamin Tissoires, Corentin Chary,
Luke D . Jones, Hans de Goede, Ilpo Järvinen, Denis Benato,
Antheas Kapenekakis
Currently, when hid-asus is not loaded, NKEY keyboards load as ~6
event devices with a pretty ASUSTEK name. When it loads, it concatenates
all applications per HID endpoint, renames them, and prints errors
when some of them do not have an input device.
Therefore, change probe so that this is no longer the case. Stop
renaming the devices, omit the check for .input which causes errors on
e.g., the Z13 for some hiddev only devices, and add
HID_QUIRK_INPUT_PER_APP so that each application gets its own event.
When this is done, the probes are called multiple times. Due to this,
the rgb check needs to be moved into probe, and the report fixup should
be skipped for non-vendor endpoints (prevents multiple prints).
Reviewed-by: Luke D. Jones <luke@ljones.dev>
Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
---
drivers/hid/hid-asus.c | 59 +++++++++++++++++++++++++++---------------
1 file changed, 38 insertions(+), 21 deletions(-)
diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index 03f0d86936fc..bbbac98f76c6 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -47,6 +47,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
#define T100CHI_MOUSE_REPORT_ID 0x06
#define FEATURE_REPORT_ID 0x0d
#define INPUT_REPORT_ID 0x5d
+#define HID_USAGE_PAGE_VENDOR 0xff310000
#define FEATURE_KBD_REPORT_ID 0x5a
#define FEATURE_KBD_REPORT_SIZE 64
#define FEATURE_KBD_LED_REPORT_ID1 0x5d
@@ -89,6 +90,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
#define QUIRK_ROG_NKEY_KEYBOARD BIT(11)
#define QUIRK_ROG_CLAYMORE_II_KEYBOARD BIT(12)
#define QUIRK_ROG_ALLY_XPAD BIT(13)
+#define QUIRK_SKIP_REPORT_FIXUP BIT(14)
#define I2C_KEYBOARD_QUIRKS (QUIRK_FIX_NOTEBOOK_REPORT | \
QUIRK_NO_INIT_REPORTS | \
@@ -125,7 +127,6 @@ struct asus_drvdata {
struct input_dev *tp_kbd_input;
struct asus_kbd_leds *kbd_backlight;
const struct asus_touchpad_info *tp;
- bool enable_backlight;
struct power_supply *battery;
struct power_supply_desc battery_desc;
int battery_capacity;
@@ -316,7 +317,7 @@ static int asus_e1239t_event(struct asus_drvdata *drvdat, u8 *data, int size)
static int asus_event(struct hid_device *hdev, struct hid_field *field,
struct hid_usage *usage, __s32 value)
{
- if ((usage->hid & HID_USAGE_PAGE) == 0xff310000 &&
+ if ((usage->hid & HID_USAGE_PAGE) == HID_USAGE_PAGE_VENDOR &&
(usage->hid & HID_USAGE) != 0x00 &&
(usage->hid & HID_USAGE) != 0xff && !usage->type) {
hid_warn(hdev, "Unmapped Asus vendor usagepage code 0x%02x\n",
@@ -931,11 +932,6 @@ static int asus_input_configured(struct hid_device *hdev, struct hid_input *hi)
drvdata->input = input;
- if (drvdata->enable_backlight &&
- !asus_kbd_wmi_led_control_present(hdev) &&
- asus_kbd_register_leds(hdev))
- hid_warn(hdev, "Failed to initialize backlight.\n");
-
return 0;
}
@@ -1008,15 +1004,6 @@ static int asus_input_mapping(struct hid_device *hdev,
return -1;
}
- /*
- * Check and enable backlight only on devices with UsagePage ==
- * 0xff31 to avoid initializing the keyboard firmware multiple
- * times on devices with multiple HID descriptors but same
- * PID/VID.
- */
- if (drvdata->quirks & QUIRK_USE_KBD_BACKLIGHT)
- drvdata->enable_backlight = true;
-
set_bit(EV_REP, hi->input->evbit);
return 1;
}
@@ -1133,8 +1120,10 @@ static int __maybe_unused asus_reset_resume(struct hid_device *hdev)
static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
{
- int ret;
+ struct hid_report_enum *rep_enum;
struct asus_drvdata *drvdata;
+ struct hid_report *rep;
+ int ret, is_vendor = 0;
drvdata = devm_kzalloc(&hdev->dev, sizeof(*drvdata), GFP_KERNEL);
if (drvdata == NULL) {
@@ -1218,18 +1207,42 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
return ret;
}
+ /* Check for vendor for RGB init and handle generic devices properly. */
+ rep_enum = &hdev->report_enum[HID_INPUT_REPORT];
+ list_for_each_entry(rep, &rep_enum->report_list, list) {
+ if ((rep->application & HID_USAGE_PAGE) == HID_USAGE_PAGE_VENDOR)
+ is_vendor = true;
+ }
+
+ /*
+ * For ROG keyboards, make them HID/hiddev compliant by creating one
+ * input per application. For interfaces other than the vendor one,
+ * disable report fixups.
+ */
+ if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) {
+ if (!is_vendor)
+ drvdata->quirks |= QUIRK_SKIP_REPORT_FIXUP;
+ hdev->quirks |= HID_QUIRK_INPUT_PER_APP;
+ }
+
ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
if (ret) {
hid_err(hdev, "Asus hw start failed: %d\n", ret);
return ret;
}
+ if (is_vendor && (drvdata->quirks & QUIRK_USE_KBD_BACKLIGHT) &&
+ !asus_kbd_wmi_led_control_present(hdev) &&
+ asus_kbd_register_leds(hdev))
+ hid_warn(hdev, "Failed to initialize backlight.\n");
+
/*
- * Check that input registration succeeded. Checking that
- * HID_CLAIMED_INPUT is set prevents a UAF when all input devices
- * were freed during registration due to no usages being mapped,
- * leaving drvdata->input pointing to freed memory.
+ * For ROG keyboards, skip rename for consistency and ->input check as
+ * some devices do not have inputs.
*/
+ if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD)
+ return 0;
+
if (!drvdata->input || !(hdev->claimed & HID_CLAIMED_INPUT)) {
hid_err(hdev, "Asus input not registered\n");
ret = -ENOMEM;
@@ -1352,6 +1365,10 @@ static const __u8 *asus_report_fixup(struct hid_device *hdev, __u8 *rdesc,
rdesc = new_rdesc;
}
+ /* Vendor fixups should only apply to NKEY vendor devices. */
+ if (drvdata->quirks & QUIRK_SKIP_REPORT_FIXUP)
+ return rdesc;
+
if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD &&
*rsize == 331 && rdesc[190] == 0x85 && rdesc[191] == 0x5a &&
rdesc[204] == 0x95 && rdesc[205] == 0x05) {
--
2.51.0
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH v7 4/9] HID: asus: prevent binding to all HID devices on ROG
2025-10-18 10:17 ` [PATCH v7 4/9] HID: asus: prevent binding to all HID devices on ROG Antheas Kapenekakis
@ 2025-10-23 18:23 ` Denis Benato
2025-10-23 18:27 ` Antheas Kapenekakis
0 siblings, 1 reply; 34+ messages in thread
From: Denis Benato @ 2025-10-23 18:23 UTC (permalink / raw)
To: Antheas Kapenekakis, platform-driver-x86, linux-input
Cc: linux-kernel, Jiri Kosina, Benjamin Tissoires, Corentin Chary,
Luke D . Jones, Hans de Goede, Ilpo Järvinen
On 10/18/25 12:17, Antheas Kapenekakis wrote:
> Currently, when hid-asus is not loaded, NKEY keyboards load as ~6
> event devices with a pretty ASUSTEK name. When it loads, it concatenates
> all applications per HID endpoint, renames them, and prints errors
> when some of them do not have an input device.
>
> Therefore, change probe so that this is no longer the case. Stop
> renaming the devices, omit the check for .input which causes errors on
the devices -> devices
> e.g., the Z13 for some hiddev only devices, and add
> HID_QUIRK_INPUT_PER_APP so that each application gets its own event.
event -> event device (or evdev?)
It is not clear from the message what HID_QUIRK_INPUT_PER_APP has to do with
renaming the devices/having one evdev vs multiple: please make
it explicit in the commit message (and perhaps make explicit if (and how),
in case it could make any difference, how programs might change
theirs behavior as a consequence).
I like the fact that userspace only sees one keyboard for what is,
effectively, one keyboard device.
The code looks good to me: make the commit message more
explanatory and I'll include my reviewed-by.
Thanks,
Denis
> When this is done, the probes are called multiple times. Due to this,
> the rgb check needs to be moved into probe, and the report fixup should
> be skipped for non-vendor endpoints (prevents multiple prints).
>
> Reviewed-by: Luke D. Jones <luke@ljones.dev>
> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> ---
> drivers/hid/hid-asus.c | 59 +++++++++++++++++++++++++++---------------
> 1 file changed, 38 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 03f0d86936fc..bbbac98f76c6 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
> @@ -47,6 +47,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
> #define T100CHI_MOUSE_REPORT_ID 0x06
> #define FEATURE_REPORT_ID 0x0d
> #define INPUT_REPORT_ID 0x5d
> +#define HID_USAGE_PAGE_VENDOR 0xff310000
> #define FEATURE_KBD_REPORT_ID 0x5a
> #define FEATURE_KBD_REPORT_SIZE 64
> #define FEATURE_KBD_LED_REPORT_ID1 0x5d
> @@ -89,6 +90,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
> #define QUIRK_ROG_NKEY_KEYBOARD BIT(11)
> #define QUIRK_ROG_CLAYMORE_II_KEYBOARD BIT(12)
> #define QUIRK_ROG_ALLY_XPAD BIT(13)
> +#define QUIRK_SKIP_REPORT_FIXUP BIT(14)
>
> #define I2C_KEYBOARD_QUIRKS (QUIRK_FIX_NOTEBOOK_REPORT | \
> QUIRK_NO_INIT_REPORTS | \
> @@ -125,7 +127,6 @@ struct asus_drvdata {
> struct input_dev *tp_kbd_input;
> struct asus_kbd_leds *kbd_backlight;
> const struct asus_touchpad_info *tp;
> - bool enable_backlight;
> struct power_supply *battery;
> struct power_supply_desc battery_desc;
> int battery_capacity;
> @@ -316,7 +317,7 @@ static int asus_e1239t_event(struct asus_drvdata *drvdat, u8 *data, int size)
> static int asus_event(struct hid_device *hdev, struct hid_field *field,
> struct hid_usage *usage, __s32 value)
> {
> - if ((usage->hid & HID_USAGE_PAGE) == 0xff310000 &&
> + if ((usage->hid & HID_USAGE_PAGE) == HID_USAGE_PAGE_VENDOR &&
> (usage->hid & HID_USAGE) != 0x00 &&
> (usage->hid & HID_USAGE) != 0xff && !usage->type) {
> hid_warn(hdev, "Unmapped Asus vendor usagepage code 0x%02x\n",
> @@ -931,11 +932,6 @@ static int asus_input_configured(struct hid_device *hdev, struct hid_input *hi)
>
> drvdata->input = input;
>
> - if (drvdata->enable_backlight &&
> - !asus_kbd_wmi_led_control_present(hdev) &&
> - asus_kbd_register_leds(hdev))
> - hid_warn(hdev, "Failed to initialize backlight.\n");
> -
> return 0;
> }
>
> @@ -1008,15 +1004,6 @@ static int asus_input_mapping(struct hid_device *hdev,
> return -1;
> }
>
> - /*
> - * Check and enable backlight only on devices with UsagePage ==
> - * 0xff31 to avoid initializing the keyboard firmware multiple
> - * times on devices with multiple HID descriptors but same
> - * PID/VID.
> - */
> - if (drvdata->quirks & QUIRK_USE_KBD_BACKLIGHT)
> - drvdata->enable_backlight = true;
> -
> set_bit(EV_REP, hi->input->evbit);
> return 1;
> }
> @@ -1133,8 +1120,10 @@ static int __maybe_unused asus_reset_resume(struct hid_device *hdev)
>
> static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
> {
> - int ret;
> + struct hid_report_enum *rep_enum;
> struct asus_drvdata *drvdata;
> + struct hid_report *rep;
> + int ret, is_vendor = 0;
>
> drvdata = devm_kzalloc(&hdev->dev, sizeof(*drvdata), GFP_KERNEL);
> if (drvdata == NULL) {
> @@ -1218,18 +1207,42 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
> return ret;
> }
>
> + /* Check for vendor for RGB init and handle generic devices properly. */
> + rep_enum = &hdev->report_enum[HID_INPUT_REPORT];
> + list_for_each_entry(rep, &rep_enum->report_list, list) {
> + if ((rep->application & HID_USAGE_PAGE) == HID_USAGE_PAGE_VENDOR)
> + is_vendor = true;
> + }
> +
> + /*
> + * For ROG keyboards, make them HID/hiddev compliant by creating one
> + * input per application. For interfaces other than the vendor one,
> + * disable report fixups.
> + */
> + if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) {
> + if (!is_vendor)
> + drvdata->quirks |= QUIRK_SKIP_REPORT_FIXUP;
> + hdev->quirks |= HID_QUIRK_INPUT_PER_APP;
> + }
> +
> ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
> if (ret) {
> hid_err(hdev, "Asus hw start failed: %d\n", ret);
> return ret;
> }
>
> + if (is_vendor && (drvdata->quirks & QUIRK_USE_KBD_BACKLIGHT) &&
> + !asus_kbd_wmi_led_control_present(hdev) &&
> + asus_kbd_register_leds(hdev))
> + hid_warn(hdev, "Failed to initialize backlight.\n");
> +
> /*
> - * Check that input registration succeeded. Checking that
> - * HID_CLAIMED_INPUT is set prevents a UAF when all input devices
> - * were freed during registration due to no usages being mapped,
> - * leaving drvdata->input pointing to freed memory.
> + * For ROG keyboards, skip rename for consistency and ->input check as
> + * some devices do not have inputs.
> */
> + if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD)
> + return 0;
> +
> if (!drvdata->input || !(hdev->claimed & HID_CLAIMED_INPUT)) {
> hid_err(hdev, "Asus input not registered\n");
> ret = -ENOMEM;
> @@ -1352,6 +1365,10 @@ static const __u8 *asus_report_fixup(struct hid_device *hdev, __u8 *rdesc,
> rdesc = new_rdesc;
> }
>
> + /* Vendor fixups should only apply to NKEY vendor devices. */
> + if (drvdata->quirks & QUIRK_SKIP_REPORT_FIXUP)
> + return rdesc;
> +
> if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD &&
> *rsize == 331 && rdesc[190] == 0x85 && rdesc[191] == 0x5a &&
> rdesc[204] == 0x95 && rdesc[205] == 0x05) {
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v7 4/9] HID: asus: prevent binding to all HID devices on ROG
2025-10-23 18:23 ` Denis Benato
@ 2025-10-23 18:27 ` Antheas Kapenekakis
0 siblings, 0 replies; 34+ messages in thread
From: Antheas Kapenekakis @ 2025-10-23 18:27 UTC (permalink / raw)
To: Denis Benato
Cc: platform-driver-x86, linux-input, linux-kernel, Jiri Kosina,
Benjamin Tissoires, Corentin Chary, Luke D . Jones, Hans de Goede,
Ilpo Järvinen
On Thu, 23 Oct 2025 at 20:23, Denis Benato <benato.denis96@gmail.com> wrote:
>
>
> On 10/18/25 12:17, Antheas Kapenekakis wrote:
> > Currently, when hid-asus is not loaded, NKEY keyboards load as ~6
> > event devices with a pretty ASUSTEK name. When it loads, it concatenates
> > all applications per HID endpoint, renames them, and prints errors
> > when some of them do not have an input device.
> >
> > Therefore, change probe so that this is no longer the case. Stop
> > renaming the devices, omit the check for .input which causes errors on
> the devices -> devices
> > e.g., the Z13 for some hiddev only devices, and add
> > HID_QUIRK_INPUT_PER_APP so that each application gets its own event.
>
> event -> event device (or evdev?)
>
> It is not clear from the message what HID_QUIRK_INPUT_PER_APP has to do with
> renaming the devices/having one evdev vs multiple: please make
> it explicit in the commit message (and perhaps make explicit if (and how),
> in case it could make any difference, how programs might change
> theirs behavior as a consequence).
>
> I like the fact that userspace only sees one keyboard for what is,
> effectively, one keyboard device.
>
> The code looks good to me: make the commit message more
> explanatory and I'll include my reviewed-by.
If I respin the series to a v8 I will reword this patch subject.
Antheas
> Thanks,
> Denis
>
> > When this is done, the probes are called multiple times. Due to this,
> > the rgb check needs to be moved into probe, and the report fixup should
> > be skipped for non-vendor endpoints (prevents multiple prints).
> >
> > Reviewed-by: Luke D. Jones <luke@ljones.dev>
> > Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> > ---
> > drivers/hid/hid-asus.c | 59 +++++++++++++++++++++++++++---------------
> > 1 file changed, 38 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> > index 03f0d86936fc..bbbac98f76c6 100644
> > --- a/drivers/hid/hid-asus.c
> > +++ b/drivers/hid/hid-asus.c
> > @@ -47,6 +47,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
> > #define T100CHI_MOUSE_REPORT_ID 0x06
> > #define FEATURE_REPORT_ID 0x0d
> > #define INPUT_REPORT_ID 0x5d
> > +#define HID_USAGE_PAGE_VENDOR 0xff310000
> > #define FEATURE_KBD_REPORT_ID 0x5a
> > #define FEATURE_KBD_REPORT_SIZE 64
> > #define FEATURE_KBD_LED_REPORT_ID1 0x5d
> > @@ -89,6 +90,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
> > #define QUIRK_ROG_NKEY_KEYBOARD BIT(11)
> > #define QUIRK_ROG_CLAYMORE_II_KEYBOARD BIT(12)
> > #define QUIRK_ROG_ALLY_XPAD BIT(13)
> > +#define QUIRK_SKIP_REPORT_FIXUP BIT(14)
> >
> > #define I2C_KEYBOARD_QUIRKS (QUIRK_FIX_NOTEBOOK_REPORT | \
> > QUIRK_NO_INIT_REPORTS | \
> > @@ -125,7 +127,6 @@ struct asus_drvdata {
> > struct input_dev *tp_kbd_input;
> > struct asus_kbd_leds *kbd_backlight;
> > const struct asus_touchpad_info *tp;
> > - bool enable_backlight;
> > struct power_supply *battery;
> > struct power_supply_desc battery_desc;
> > int battery_capacity;
> > @@ -316,7 +317,7 @@ static int asus_e1239t_event(struct asus_drvdata *drvdat, u8 *data, int size)
> > static int asus_event(struct hid_device *hdev, struct hid_field *field,
> > struct hid_usage *usage, __s32 value)
> > {
> > - if ((usage->hid & HID_USAGE_PAGE) == 0xff310000 &&
> > + if ((usage->hid & HID_USAGE_PAGE) == HID_USAGE_PAGE_VENDOR &&
> > (usage->hid & HID_USAGE) != 0x00 &&
> > (usage->hid & HID_USAGE) != 0xff && !usage->type) {
> > hid_warn(hdev, "Unmapped Asus vendor usagepage code 0x%02x\n",
> > @@ -931,11 +932,6 @@ static int asus_input_configured(struct hid_device *hdev, struct hid_input *hi)
> >
> > drvdata->input = input;
> >
> > - if (drvdata->enable_backlight &&
> > - !asus_kbd_wmi_led_control_present(hdev) &&
> > - asus_kbd_register_leds(hdev))
> > - hid_warn(hdev, "Failed to initialize backlight.\n");
> > -
> > return 0;
> > }
> >
> > @@ -1008,15 +1004,6 @@ static int asus_input_mapping(struct hid_device *hdev,
> > return -1;
> > }
> >
> > - /*
> > - * Check and enable backlight only on devices with UsagePage ==
> > - * 0xff31 to avoid initializing the keyboard firmware multiple
> > - * times on devices with multiple HID descriptors but same
> > - * PID/VID.
> > - */
> > - if (drvdata->quirks & QUIRK_USE_KBD_BACKLIGHT)
> > - drvdata->enable_backlight = true;
> > -
> > set_bit(EV_REP, hi->input->evbit);
> > return 1;
> > }
> > @@ -1133,8 +1120,10 @@ static int __maybe_unused asus_reset_resume(struct hid_device *hdev)
> >
> > static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
> > {
> > - int ret;
> > + struct hid_report_enum *rep_enum;
> > struct asus_drvdata *drvdata;
> > + struct hid_report *rep;
> > + int ret, is_vendor = 0;
> >
> > drvdata = devm_kzalloc(&hdev->dev, sizeof(*drvdata), GFP_KERNEL);
> > if (drvdata == NULL) {
> > @@ -1218,18 +1207,42 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
> > return ret;
> > }
> >
> > + /* Check for vendor for RGB init and handle generic devices properly. */
> > + rep_enum = &hdev->report_enum[HID_INPUT_REPORT];
> > + list_for_each_entry(rep, &rep_enum->report_list, list) {
> > + if ((rep->application & HID_USAGE_PAGE) == HID_USAGE_PAGE_VENDOR)
> > + is_vendor = true;
> > + }
> > +
> > + /*
> > + * For ROG keyboards, make them HID/hiddev compliant by creating one
> > + * input per application. For interfaces other than the vendor one,
> > + * disable report fixups.
> > + */
> > + if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) {
> > + if (!is_vendor)
> > + drvdata->quirks |= QUIRK_SKIP_REPORT_FIXUP;
> > + hdev->quirks |= HID_QUIRK_INPUT_PER_APP;
> > + }
> > +
> > ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
> > if (ret) {
> > hid_err(hdev, "Asus hw start failed: %d\n", ret);
> > return ret;
> > }
> >
> > + if (is_vendor && (drvdata->quirks & QUIRK_USE_KBD_BACKLIGHT) &&
> > + !asus_kbd_wmi_led_control_present(hdev) &&
> > + asus_kbd_register_leds(hdev))
> > + hid_warn(hdev, "Failed to initialize backlight.\n");
> > +
> > /*
> > - * Check that input registration succeeded. Checking that
> > - * HID_CLAIMED_INPUT is set prevents a UAF when all input devices
> > - * were freed during registration due to no usages being mapped,
> > - * leaving drvdata->input pointing to freed memory.
> > + * For ROG keyboards, skip rename for consistency and ->input check as
> > + * some devices do not have inputs.
> > */
> > + if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD)
> > + return 0;
> > +
> > if (!drvdata->input || !(hdev->claimed & HID_CLAIMED_INPUT)) {
> > hid_err(hdev, "Asus input not registered\n");
> > ret = -ENOMEM;
> > @@ -1352,6 +1365,10 @@ static const __u8 *asus_report_fixup(struct hid_device *hdev, __u8 *rdesc,
> > rdesc = new_rdesc;
> > }
> >
> > + /* Vendor fixups should only apply to NKEY vendor devices. */
> > + if (drvdata->quirks & QUIRK_SKIP_REPORT_FIXUP)
> > + return rdesc;
> > +
> > if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD &&
> > *rsize == 331 && rdesc[190] == 0x85 && rdesc[191] == 0x5a &&
> > rdesc[204] == 0x95 && rdesc[205] == 0x05) {
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v7 5/9] platform/x86: asus-wmi: Add support for multiple kbd led handlers
2025-10-18 10:17 [PATCH v7 0/9] HID: asus: Fix ASUS ROG Laptop's Keyboard backlight handling Antheas Kapenekakis
` (3 preceding siblings ...)
2025-10-18 10:17 ` [PATCH v7 4/9] HID: asus: prevent binding to all HID devices on ROG Antheas Kapenekakis
@ 2025-10-18 10:17 ` Antheas Kapenekakis
2025-10-22 13:38 ` kernel test robot
2025-10-18 10:17 ` [PATCH v7 6/9] HID: asus: listen to the asus-wmi brightness device instead of creating one Antheas Kapenekakis
` (3 subsequent siblings)
8 siblings, 1 reply; 34+ messages in thread
From: Antheas Kapenekakis @ 2025-10-18 10:17 UTC (permalink / raw)
To: platform-driver-x86, linux-input
Cc: linux-kernel, Jiri Kosina, Benjamin Tissoires, Corentin Chary,
Luke D . Jones, Hans de Goede, Ilpo Järvinen, Denis Benato,
Antheas Kapenekakis
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.
Since both hid-asus and asus-wmi can both interact with the led
objects including from an atomic context, protect the brightness
access with a spinlock and update the values from a workqueue.
Use this workqueue to process WMI keyboard events as well, so they
are processed asynchronously.
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 | 175 ++++++++++++++++++---
include/linux/platform_data/x86/asus-wmi.h | 17 ++
2 files changed, 168 insertions(+), 24 deletions(-)
diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index e72a2b5d158e..aab779142323 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -36,6 +36,7 @@
#include <linux/rfkill.h>
#include <linux/seq_file.h>
#include <linux/slab.h>
+#include <linux/spinlock.h>
#include <linux/types.h>
#include <linux/units.h>
@@ -258,6 +259,9 @@ struct asus_wmi {
int tpd_led_wk;
struct led_classdev kbd_led;
int kbd_led_wk;
+ bool kbd_led_notify;
+ bool kbd_led_avail;
+ bool kbd_led_registered;
struct led_classdev lightbar_led;
int lightbar_led_wk;
struct led_classdev micmute_led;
@@ -266,6 +270,7 @@ struct asus_wmi {
struct work_struct tpd_led_work;
struct work_struct wlan_led_work;
struct work_struct lightbar_led_work;
+ struct work_struct kbd_led_work;
struct asus_rfkill wlan;
struct asus_rfkill bluetooth;
@@ -1530,6 +1535,100 @@ static void asus_wmi_battery_exit(struct asus_wmi *asus)
/* LEDs ***********************************************************************/
+struct asus_hid_ref {
+ struct list_head listeners;
+ struct asus_wmi *asus;
+ /* Protects concurrent access from hid-asus and asus-wmi to leds */
+ spinlock_t lock;
+};
+
+static struct asus_hid_ref asus_ref = {
+ .listeners = LIST_HEAD_INIT(asus_ref.listeners),
+ .asus = NULL,
+ /*
+ * Protects .asus, .asus.kbd_led_{wk,notify}, and .listener refs. Other
+ * asus variables are read-only after .asus is set. Except the led cdev
+ * device if not kbd_led_avail. That becomes read-only after the
+ * first hid-asus listener registers and triggers the work queue. It is
+ * then not referenced again until unregistering, which happens after
+ * .asus ref is dropped. Since .asus needs to be accessed by hid-asus
+ * IRQs to check if forwarding events is possible, a spinlock is used.
+ */
+ .lock = __SPIN_LOCK_UNLOCKED(asus_ref.lock),
+};
+
+/*
+ * Allows registering hid-asus listeners that want to be notified of
+ * keyboard backlight changes.
+ */
+int asus_hid_register_listener(struct asus_hid_listener *bdev)
+{
+ struct asus_wmi *asus;
+
+ guard(spinlock_irqsave)(&asus_ref.lock);
+ list_add_tail(&bdev->list, &asus_ref.listeners);
+ asus = asus_ref.asus;
+ if (asus)
+ queue_work(asus->led_workqueue, &asus->kbd_led_work);
+ return 0;
+}
+EXPORT_SYMBOL_GPL(asus_hid_register_listener);
+
+/*
+ * Allows unregistering hid-asus listeners that were added with
+ * asus_hid_register_listener().
+ */
+void asus_hid_unregister_listener(struct asus_hid_listener *bdev)
+{
+ guard(spinlock_irqsave)(&asus_ref.lock);
+ list_del(&bdev->list);
+}
+EXPORT_SYMBOL_GPL(asus_hid_unregister_listener);
+
+static void do_kbd_led_set(struct led_classdev *led_cdev, int value);
+
+static void kbd_led_update_all(struct work_struct *work)
+{
+ enum led_brightness value;
+ struct asus_wmi *asus;
+ bool registered, notify;
+ int ret;
+
+ asus = container_of(work, struct asus_wmi, kbd_led_work);
+
+ scoped_guard(spinlock_irqsave, &asus_ref.lock) {
+ registered = asus->kbd_led_registered;
+ value = asus->kbd_led_wk;
+ notify = asus->kbd_led_notify;
+ }
+
+ if (!registered) {
+ /*
+ * This workqueue runs under asus-wmi, which means probe has
+ * completed and asus-wmi will keep running until it finishes.
+ * Therefore, we can safely register the LED without holding
+ * a spinlock.
+ */
+ ret = devm_led_classdev_register(&asus->platform_device->dev,
+ &asus->kbd_led);
+ if (!ret) {
+ scoped_guard(spinlock_irqsave, &asus_ref.lock)
+ asus->kbd_led_registered = true;
+ } else {
+ pr_warn("Failed to register keyboard backlight LED: %d\n", ret);
+ return;
+ }
+ }
+
+ if (value >= 0)
+ do_kbd_led_set(&asus->kbd_led, value);
+ if (notify) {
+ scoped_guard(spinlock_irqsave, &asus_ref.lock)
+ asus->kbd_led_notify = false;
+ led_classdev_notify_brightness_hw_changed(&asus->kbd_led, value);
+ }
+}
+
/*
* These functions actually update the LED's, and are called from a
* workqueue. By doing this as separate work rather than when the LED
@@ -1576,7 +1675,8 @@ static void kbd_led_update(struct asus_wmi *asus)
{
int ctrl_param = 0;
- ctrl_param = 0x80 | (asus->kbd_led_wk & 0x7F);
+ scoped_guard(spinlock_irqsave, &asus_ref.lock)
+ ctrl_param = 0x80 | (asus->kbd_led_wk & 0x7F);
asus_wmi_set_devstate(ASUS_WMI_DEVID_KBD_BACKLIGHT, ctrl_param, NULL);
}
@@ -1609,14 +1709,23 @@ 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;
asus = container_of(led_cdev, struct asus_wmi, kbd_led);
max_level = asus->kbd_led.max_brightness;
- asus->kbd_led_wk = clamp_val(value, 0, max_level);
- kbd_led_update(asus);
+ scoped_guard(spinlock_irqsave, &asus_ref.lock)
+ asus->kbd_led_wk = clamp_val(value, 0, max_level);
+
+ if (asus->kbd_led_avail)
+ kbd_led_update(asus);
+
+ scoped_guard(spinlock_irqsave, &asus_ref.lock) {
+ 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,
@@ -1631,10 +1740,11 @@ static void kbd_led_set(struct led_classdev *led_cdev,
static void kbd_led_set_by_kbd(struct asus_wmi *asus, enum led_brightness value)
{
- struct led_classdev *led_cdev = &asus->kbd_led;
-
- do_kbd_led_set(led_cdev, value);
- led_classdev_notify_brightness_hw_changed(led_cdev, asus->kbd_led_wk);
+ scoped_guard(spinlock_irqsave, &asus_ref.lock) {
+ asus->kbd_led_wk = value;
+ asus->kbd_led_notify = true;
+ }
+ queue_work(asus->led_workqueue, &asus->kbd_led_work);
}
static enum led_brightness kbd_led_get(struct led_classdev *led_cdev)
@@ -1644,10 +1754,18 @@ static enum led_brightness kbd_led_get(struct led_classdev *led_cdev)
asus = container_of(led_cdev, struct asus_wmi, kbd_led);
+ scoped_guard(spinlock_irqsave, &asus_ref.lock) {
+ if (!asus->kbd_led_avail)
+ return asus->kbd_led_wk;
+ }
+
retval = kbd_led_read(asus, &value, NULL);
if (retval < 0)
return retval;
+ scoped_guard(spinlock_irqsave, &asus_ref.lock)
+ asus->kbd_led_wk = value;
+
return value;
}
@@ -1759,7 +1877,9 @@ 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);
+ scoped_guard(spinlock_irqsave, &asus_ref.lock)
+ asus_ref.asus = NULL;
+
led_classdev_unregister(&asus->tpd_led);
led_classdev_unregister(&asus->wlan_led);
led_classdev_unregister(&asus->lightbar_led);
@@ -1797,22 +1917,25 @@ 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_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;
+ 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);
+ INIT_WORK(&asus->kbd_led_work, kbd_led_update_all);
+ if (asus->kbd_led_avail) {
+ asus->kbd_led_wk = led_val;
if (num_rgb_groups != 0)
asus->kbd_led.groups = kbd_rgb_mode_groups;
+ } else
+ asus->kbd_led_wk = -1;
- rv = led_classdev_register(&asus->platform_device->dev,
- &asus->kbd_led);
- if (rv)
- goto error;
+ scoped_guard(spinlock_irqsave, &asus_ref.lock) {
+ asus_ref.asus = asus;
+ if (asus->kbd_led_avail || !list_empty(&asus_ref.listeners))
+ queue_work(asus->led_workqueue, &asus->kbd_led_work);
}
if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_WIRELESS_LED)
@@ -4272,6 +4395,7 @@ static int asus_wmi_get_event_code(union acpi_object *obj)
static void asus_wmi_handle_event_code(int code, struct asus_wmi *asus)
{
+ enum led_brightness led_value;
unsigned int key_value = 1;
bool autorelease = 1;
@@ -4288,19 +4412,22 @@ static void asus_wmi_handle_event_code(int code, struct asus_wmi *asus)
return;
}
+ scoped_guard(spinlock_irqsave, &asus_ref.lock)
+ led_value = asus->kbd_led_wk;
+
if (code == NOTIFY_KBD_BRTUP) {
- kbd_led_set_by_kbd(asus, asus->kbd_led_wk + 1);
+ kbd_led_set_by_kbd(asus, led_value + 1);
return;
}
if (code == NOTIFY_KBD_BRTDWN) {
- kbd_led_set_by_kbd(asus, asus->kbd_led_wk - 1);
+ kbd_led_set_by_kbd(asus, led_value - 1);
return;
}
if (code == NOTIFY_KBD_BRTTOGGLE) {
- if (asus->kbd_led_wk == asus->kbd_led.max_brightness)
+ if (led_value == asus->kbd_led.max_brightness)
kbd_led_set_by_kbd(asus, 0);
else
- kbd_led_set_by_kbd(asus, asus->kbd_led_wk + 1);
+ kbd_led_set_by_kbd(asus, led_value + 1);
return;
}
diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
index 8a515179113d..1165039013b1 100644
--- a/include/linux/platform_data/x86/asus-wmi.h
+++ b/include/linux/platform_data/x86/asus-wmi.h
@@ -163,11 +163,20 @@ enum asus_ally_mcu_hack {
ASUS_WMI_ALLY_MCU_HACK_DISABLED,
};
+/* Used to notify hid-asus when asus-wmi changes keyboard backlight */
+struct asus_hid_listener {
+ struct list_head list;
+ void (*brightness_set)(struct asus_hid_listener *listener, int brightness);
+};
+
#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 +193,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 */
--
2.51.0
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH v7 5/9] platform/x86: asus-wmi: Add support for multiple kbd led handlers
2025-10-18 10:17 ` [PATCH v7 5/9] platform/x86: asus-wmi: Add support for multiple kbd led handlers Antheas Kapenekakis
@ 2025-10-22 13:38 ` kernel test robot
2025-10-23 6:56 ` Antheas Kapenekakis
0 siblings, 1 reply; 34+ messages in thread
From: kernel test robot @ 2025-10-22 13:38 UTC (permalink / raw)
To: Antheas Kapenekakis, platform-driver-x86, linux-input
Cc: oe-kbuild-all, linux-kernel, Jiri Kosina, Benjamin Tissoires,
Corentin Chary, Luke D . Jones, Hans de Goede, Ilpo Järvinen,
Denis Benato, Antheas Kapenekakis
Hi Antheas,
kernel test robot noticed the following build warnings:
[auto build test WARNING on 3a8660878839faadb4f1a6dd72c3179c1df56787]
url: https://github.com/intel-lab-lkp/linux/commits/Antheas-Kapenekakis/HID-asus-simplify-RGB-init-sequence/20251018-182410
base: 3a8660878839faadb4f1a6dd72c3179c1df56787
patch link: https://lore.kernel.org/r/20251018101759.4089-6-lkml%40antheas.dev
patch subject: [PATCH v7 5/9] platform/x86: asus-wmi: Add support for multiple kbd led handlers
config: i386-randconfig-141-20251020 (https://download.01.org/0day-ci/archive/20251022/202510222013.EBLC609m-lkp@intel.com/config)
compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202510222013.EBLC609m-lkp@intel.com/
New smatch warnings:
drivers/platform/x86/asus-wmi.c:1623 kbd_led_update_all() warn: always true condition '(value >= 0) => (0-u32max >= 0)'
Old smatch warnings:
drivers/platform/x86/asus-wmi.c:2288 asus_new_rfkill() warn: '*rfkill' is an error pointer or valid
vim +1623 drivers/platform/x86/asus-wmi.c
1589
1590 static void kbd_led_update_all(struct work_struct *work)
1591 {
1592 enum led_brightness value;
1593 struct asus_wmi *asus;
1594 bool registered, notify;
1595 int ret;
1596
1597 asus = container_of(work, struct asus_wmi, kbd_led_work);
1598
1599 scoped_guard(spinlock_irqsave, &asus_ref.lock) {
1600 registered = asus->kbd_led_registered;
1601 value = asus->kbd_led_wk;
1602 notify = asus->kbd_led_notify;
1603 }
1604
1605 if (!registered) {
1606 /*
1607 * This workqueue runs under asus-wmi, which means probe has
1608 * completed and asus-wmi will keep running until it finishes.
1609 * Therefore, we can safely register the LED without holding
1610 * a spinlock.
1611 */
1612 ret = devm_led_classdev_register(&asus->platform_device->dev,
1613 &asus->kbd_led);
1614 if (!ret) {
1615 scoped_guard(spinlock_irqsave, &asus_ref.lock)
1616 asus->kbd_led_registered = true;
1617 } else {
1618 pr_warn("Failed to register keyboard backlight LED: %d\n", ret);
1619 return;
1620 }
1621 }
1622
> 1623 if (value >= 0)
1624 do_kbd_led_set(&asus->kbd_led, value);
1625 if (notify) {
1626 scoped_guard(spinlock_irqsave, &asus_ref.lock)
1627 asus->kbd_led_notify = false;
1628 led_classdev_notify_brightness_hw_changed(&asus->kbd_led, value);
1629 }
1630 }
1631
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v7 5/9] platform/x86: asus-wmi: Add support for multiple kbd led handlers
2025-10-22 13:38 ` kernel test robot
@ 2025-10-23 6:56 ` Antheas Kapenekakis
2025-10-31 8:26 ` Jiri Kosina
0 siblings, 1 reply; 34+ messages in thread
From: Antheas Kapenekakis @ 2025-10-23 6:56 UTC (permalink / raw)
To: kernel test robot
Cc: platform-driver-x86, linux-input, oe-kbuild-all, linux-kernel,
Jiri Kosina, Benjamin Tissoires, Corentin Chary, Luke D . Jones,
Hans de Goede, Ilpo Järvinen, Denis Benato
On Wed, 22 Oct 2025 at 15:38, kernel test robot <lkp@intel.com> wrote:
>
> Hi Antheas,
>
> kernel test robot noticed the following build warnings:
>
> [auto build test WARNING on 3a8660878839faadb4f1a6dd72c3179c1df56787]
>
> url: https://github.com/intel-lab-lkp/linux/commits/Antheas-Kapenekakis/HID-asus-simplify-RGB-init-sequence/20251018-182410
> base: 3a8660878839faadb4f1a6dd72c3179c1df56787
> patch link: https://lore.kernel.org/r/20251018101759.4089-6-lkml%40antheas.dev
> patch subject: [PATCH v7 5/9] platform/x86: asus-wmi: Add support for multiple kbd led handlers
> config: i386-randconfig-141-20251020 (https://download.01.org/0day-ci/archive/20251022/202510222013.EBLC609m-lkp@intel.com/config)
> compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202510222013.EBLC609m-lkp@intel.com/
>
> New smatch warnings:
> drivers/platform/x86/asus-wmi.c:1623 kbd_led_update_all() warn: always true condition '(value >= 0) => (0-u32max >= 0)'
>
> Old smatch warnings:
> drivers/platform/x86/asus-wmi.c:2288 asus_new_rfkill() warn: '*rfkill' is an error pointer or valid
>
> vim +1623 drivers/platform/x86/asus-wmi.c
>
> 1589
> 1590 static void kbd_led_update_all(struct work_struct *work)
> 1591 {
> 1592 enum led_brightness value;
> 1593 struct asus_wmi *asus;
> 1594 bool registered, notify;
> 1595 int ret;
/\ value should have been an int and
placed here. It can take the value -1 hence the check
Are there any other comments on the series?
The only issue I am aware of is that Denis identified a bug in asusd
(asusctl userspace program daemon) in certain Asus G14/G16 laptops
that cause laptop keys to become sticky, I have had users also report
that bug in previous versions of the series. WIthout asusd running,
keyboards work fine incl. with brightness control (did not work
before). Given it will take two months for this to reach mainline, I
think it is a fair amount of time to address the bug.
Antheas
> 1596
> 1597 asus = container_of(work, struct asus_wmi, kbd_led_work);
> 1598
> 1599 scoped_guard(spinlock_irqsave, &asus_ref.lock) {
> 1600 registered = asus->kbd_led_registered;
> 1601 value = asus->kbd_led_wk;
> 1602 notify = asus->kbd_led_notify;
> 1603 }
> 1604
> 1605 if (!registered) {
> 1606 /*
> 1607 * This workqueue runs under asus-wmi, which means probe has
> 1608 * completed and asus-wmi will keep running until it finishes.
> 1609 * Therefore, we can safely register the LED without holding
> 1610 * a spinlock.
> 1611 */
> 1612 ret = devm_led_classdev_register(&asus->platform_device->dev,
> 1613 &asus->kbd_led);
> 1614 if (!ret) {
> 1615 scoped_guard(spinlock_irqsave, &asus_ref.lock)
> 1616 asus->kbd_led_registered = true;
> 1617 } else {
> 1618 pr_warn("Failed to register keyboard backlight LED: %d\n", ret);
> 1619 return;
> 1620 }
> 1621 }
> 1622
> > 1623 if (value >= 0)
> 1624 do_kbd_led_set(&asus->kbd_led, value);
> 1625 if (notify) {
> 1626 scoped_guard(spinlock_irqsave, &asus_ref.lock)
> 1627 asus->kbd_led_notify = false;
> 1628 led_classdev_notify_brightness_hw_changed(&asus->kbd_led, value);
> 1629 }
> 1630 }
> 1631
>
> --
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests/wiki
>
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v7 5/9] platform/x86: asus-wmi: Add support for multiple kbd led handlers
2025-10-23 6:56 ` Antheas Kapenekakis
@ 2025-10-31 8:26 ` Jiri Kosina
2025-10-31 12:13 ` Antheas Kapenekakis
0 siblings, 1 reply; 34+ messages in thread
From: Jiri Kosina @ 2025-10-31 8:26 UTC (permalink / raw)
To: Antheas Kapenekakis
Cc: kernel test robot, platform-driver-x86, linux-input,
oe-kbuild-all, linux-kernel, Benjamin Tissoires, Corentin Chary,
Luke D . Jones, Hans de Goede, Ilpo Järvinen, Denis Benato
On Thu, 23 Oct 2025, Antheas Kapenekakis wrote:
> > 1589
> > 1590 static void kbd_led_update_all(struct work_struct *work)
> > 1591 {
> > 1592 enum led_brightness value;
> > 1593 struct asus_wmi *asus;
> > 1594 bool registered, notify;
> > 1595 int ret;
> /\ value should have been an int and
> placed here. It can take the value -1 hence the check
Thanks, that needs to be fixed before the final merge.
> Are there any other comments on the series?
>
> The only issue I am aware of is that Denis identified a bug in asusd
> (asusctl userspace program daemon) in certain Asus G14/G16 laptops
> that cause laptop keys to become sticky, I have had users also report
> that bug in previous versions of the series. WIthout asusd running,
> keyboards work fine incl. with brightness control (did not work
> before). Given it will take two months for this to reach mainline, I
> think it is a fair amount of time to address the bug.
One thing that is not clear to me about this -- is this causing a visible
user-space behavior regression before vs. after the patchset with asusctl?
If so, I am afraid this needs to be root-caused and fixed before the set
can be considered for inclusion.
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v7 5/9] platform/x86: asus-wmi: Add support for multiple kbd led handlers
2025-10-31 8:26 ` Jiri Kosina
@ 2025-10-31 12:13 ` Antheas Kapenekakis
2025-11-03 4:28 ` Derek J. Clark
0 siblings, 1 reply; 34+ messages in thread
From: Antheas Kapenekakis @ 2025-10-31 12:13 UTC (permalink / raw)
To: Jiri Kosina
Cc: kernel test robot, platform-driver-x86, linux-input,
oe-kbuild-all, linux-kernel, Benjamin Tissoires, Corentin Chary,
Luke D . Jones, Hans de Goede, Ilpo Järvinen, Denis Benato
On Fri, 31 Oct 2025 at 09:27, Jiri Kosina <jikos@kernel.org> wrote:
>
> On Thu, 23 Oct 2025, Antheas Kapenekakis wrote:
>
> > > 1589
> > > 1590 static void kbd_led_update_all(struct work_struct *work)
> > > 1591 {
> > > 1592 enum led_brightness value;
> > > 1593 struct asus_wmi *asus;
> > > 1594 bool registered, notify;
> > > 1595 int ret;
> > /\ value should have been an int and
> > placed here. It can take the value -1 hence the check
>
> Thanks, that needs to be fixed before the final merge.
>
> > Are there any other comments on the series?
> >
> > The only issue I am aware of is that Denis identified a bug in asusd
> > (asusctl userspace program daemon) in certain Asus G14/G16 laptops
> > that cause laptop keys to become sticky, I have had users also report
> > that bug in previous versions of the series. WIthout asusd running,
> > keyboards work fine incl. with brightness control (did not work
> > before). Given it will take two months for this to reach mainline, I
> > think it is a fair amount of time to address the bug.
>
> One thing that is not clear to me about this -- is this causing a visible
> user-space behavior regression before vs. after the patchset with asusctl?
>
> If so, I am afraid this needs to be root-caused and fixed before the set
> can be considered for inclusion.
Commit 591ba2074337 ("HID: asus: prevent binding to all HID devices on
ROG") adds HID_QUIRK_INPUT_PER_APP and the extra devices seem to
confuse asusd. Since the devices are the same as with hid-asus not
loaded, it is specific to that program.
We can delay that patch until Denis who took over maintenance of the
program can have a deeper look. I will still keep the last part of
that patch that skips the input check, because that causes errors in
devices that do not create an input device (e.g., lightbar).
Antheas
> Thanks,
>
> --
> Jiri Kosina
> SUSE Labs
>
>
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v7 5/9] platform/x86: asus-wmi: Add support for multiple kbd led handlers
2025-10-31 12:13 ` Antheas Kapenekakis
@ 2025-11-03 4:28 ` Derek J. Clark
2025-11-03 7:36 ` Antheas Kapenekakis
0 siblings, 1 reply; 34+ messages in thread
From: Derek J. Clark @ 2025-11-03 4:28 UTC (permalink / raw)
To: Antheas Kapenekakis
Cc: Jiri Kosina, Benjamin Tissoires, Corentin Chary, Luke D . Jones,
Hans de Goede, Ilpo Järvinen, Denis Benato,
kernel test robot, platform-driver-x86, linux-input,
oe-kbuild-all, linux-kernel, Derek J. Clark
>On Fri, 31 Oct 2025 at 09:27, Jiri Kosina <jikos@kernel.org> wrote:
>>
>> On Thu, 23 Oct 2025, Antheas Kapenekakis wrote:
>>
>> > > 1589
>> > > 1590 static void kbd_led_update_all(struct work_struct *work)
>> > > 1591 {
>> > > 1592 enum led_brightness value;
>> > > 1593 struct asus_wmi *asus;
>> > > 1594 bool registered, notify;
>> > > 1595 int ret;
>> > /\ value should have been an int and
>> > placed here. It can take the value -1 hence the check
>>
>> Thanks, that needs to be fixed before the final merge.
>>
>> > Are there any other comments on the series?
>> >
>> > The only issue I am aware of is that Denis identified a bug in asusd
>> > (asusctl userspace program daemon) in certain Asus G14/G16 laptops
>> > that cause laptop keys to become sticky, I have had users also report
>> > that bug in previous versions of the series. WIthout asusd running,
>> > keyboards work fine incl. with brightness control (did not work
>> > before). Given it will take two months for this to reach mainline, I
>> > think it is a fair amount of time to address the bug.
>>
>> One thing that is not clear to me about this -- is this causing a visible
>> user-space behavior regression before vs. after the patchset with asusctl?
>>
>> If so, I am afraid this needs to be root-caused and fixed before the set
>> can be considered for inclusion.
>Commit 591ba2074337 ("HID: asus: prevent binding to all HID devices on
>ROG") adds HID_QUIRK_INPUT_PER_APP and the extra devices seem to
>confuse asusd. Since the devices are the same as with hid-asus not
>loaded, it is specific to that program.
>
>
Hi Antheas.
While you have previously expressed to me directly that you wish InputPlumber
didn't exist, it still very much does, in fact, exist. I also know that you are
explicitly aware that InputPlumber is a consumer of this interface, so your
comment that asusctl is the only affected program is something you know to be
false. This is not even the first time you have renamed an input device that
you knew InputPlumber was a consumer of without notifying me[1].
I can't abide you outright lying to the maintainers here and I'm sick and tired
of having to watch your every move on the LKML. Either become a good citizen of
kernel maintenance, or get out of it.
Commit 591ba2074337 ("HID: asus: prevent binding to all HID devices on ROG")
Nacked-By: Derek J. Clark <derekjohn.clark@gmail.com>
- Derek
[1] https://lore.kernel.org/linux-input/Z74vZD7ZtKBTDlwy@google.com/
>We can delay that patch until Denis who took over maintenance of the
>program can have a deeper look. I will still keep the last part of
>that patch that skips the input check, because that causes errors in
>devices that do not create an input device (e.g., lightbar).
>
>Antheas
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v7 5/9] platform/x86: asus-wmi: Add support for multiple kbd led handlers
2025-11-03 4:28 ` Derek J. Clark
@ 2025-11-03 7:36 ` Antheas Kapenekakis
2025-11-03 8:37 ` luke
0 siblings, 1 reply; 34+ messages in thread
From: Antheas Kapenekakis @ 2025-11-03 7:36 UTC (permalink / raw)
To: Derek J. Clark
Cc: Jiri Kosina, Benjamin Tissoires, Corentin Chary, Luke D . Jones,
Hans de Goede, Ilpo Järvinen, Denis Benato,
kernel test robot, platform-driver-x86, linux-input,
oe-kbuild-all, linux-kernel
On Mon, 3 Nov 2025 at 05:29, Derek J. Clark <derekjohn.clark@gmail.com> wrote:
>
> >On Fri, 31 Oct 2025 at 09:27, Jiri Kosina <jikos@kernel.org> wrote:
> >>
> >> On Thu, 23 Oct 2025, Antheas Kapenekakis wrote:
> >>
> >> > > 1589
> >> > > 1590 static void kbd_led_update_all(struct work_struct *work)
> >> > > 1591 {
> >> > > 1592 enum led_brightness value;
> >> > > 1593 struct asus_wmi *asus;
> >> > > 1594 bool registered, notify;
> >> > > 1595 int ret;
> >> > /\ value should have been an int and
> >> > placed here. It can take the value -1 hence the check
> >>
> >> Thanks, that needs to be fixed before the final merge.
> >>
> >> > Are there any other comments on the series?
> >> >
> >> > The only issue I am aware of is that Denis identified a bug in asusd
> >> > (asusctl userspace program daemon) in certain Asus G14/G16 laptops
> >> > that cause laptop keys to become sticky, I have had users also report
> >> > that bug in previous versions of the series. WIthout asusd running,
> >> > keyboards work fine incl. with brightness control (did not work
> >> > before). Given it will take two months for this to reach mainline, I
> >> > think it is a fair amount of time to address the bug.
> >>
> >> One thing that is not clear to me about this -- is this causing a visible
> >> user-space behavior regression before vs. after the patchset with asusctl?
> >>
> >> If so, I am afraid this needs to be root-caused and fixed before the set
> >> can be considered for inclusion.
>
> >Commit 591ba2074337 ("HID: asus: prevent binding to all HID devices on
> >ROG") adds HID_QUIRK_INPUT_PER_APP and the extra devices seem to
> >confuse asusd. Since the devices are the same as with hid-asus not
> >loaded, it is specific to that program.
> >
> >
> Hi Antheas.
>
> While you have previously expressed to me directly that you wish InputPlumber
> didn't exist, it still very much does, in fact, exist. I also know that you are
> explicitly aware that InputPlumber is a consumer of this interface, so your
> comment that asusctl is the only affected program is something you know to be
> false. This is not even the first time you have renamed an input device that
> you knew InputPlumber was a consumer of without notifying me[1].
>
> I can't abide you outright lying to the maintainers here and I'm sick and tired
> of having to watch your every move on the LKML. Either become a good citizen of
> kernel maintenance, or get out of it.
Hi Derek,
I am not aware if your software is affected or not by this series as I
have not received reports about it.
If it is, please add:
"ASUSTeK Computer Inc. N-KEY Device"
As a name match to your software (should only take 5 minutes).
I worked with Luke and Dennis on it for the better part of a year so
hopefully they forwarded to you if it affects you or not.
Your software relies on OOT drivers for most devices incl. the Ally so
I am unsure if it is affected in reality. E.g., it would not be
affected in SteamOS and CachyOS. In the future, it would be good to
avoid name matches for your software as it makes it very fragile,
which is a discussion we have had before. I do not think device evdev
names constitute an ABI technically.
Best,
Antheas
> Commit 591ba2074337 ("HID: asus: prevent binding to all HID devices on ROG")
> Nacked-By: Derek J. Clark <derekjohn.clark@gmail.com>
>
> - Derek
>
> [1] https://lore.kernel.org/linux-input/Z74vZD7ZtKBTDlwy@google.com/
>
> >We can delay that patch until Denis who took over maintenance of the
> >program can have a deeper look. I will still keep the last part of
> >that patch that skips the input check, because that causes errors in
> >devices that do not create an input device (e.g., lightbar).
> >
> >Antheas
>
>
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v7 5/9] platform/x86: asus-wmi: Add support for multiple kbd led handlers
2025-11-03 7:36 ` Antheas Kapenekakis
@ 2025-11-03 8:37 ` luke
2025-11-03 8:48 ` Antheas Kapenekakis
0 siblings, 1 reply; 34+ messages in thread
From: luke @ 2025-11-03 8:37 UTC (permalink / raw)
To: Antheas Kapenekakis
Cc: Derek J. Clark, Jiri Kosina, Benjamin Tissoires, Corentin Chary,
Hans de Goede, Ilpo Järvinen, Denis Benato,
kernel test robot, platform-driver-x86, linux-input,
oe-kbuild-all, linux-kernel
> On 3 Nov 2025, at 20:36, Antheas Kapenekakis <lkml@antheas.dev> wrote:
>
> On Mon, 3 Nov 2025 at 05:29, Derek J. Clark <derekjohn.clark@gmail.com> wrote:
>>
>>> On Fri, 31 Oct 2025 at 09:27, Jiri Kosina <jikos@kernel.org> wrote:
>>>>
>>>> On Thu, 23 Oct 2025, Antheas Kapenekakis wrote:
>>>>
>>>>>> 1589
>>>>>> 1590 static void kbd_led_update_all(struct work_struct *work)
>>>>>> 1591 {
>>>>>> 1592 enum led_brightness value;
>>>>>> 1593 struct asus_wmi *asus;
>>>>>> 1594 bool registered, notify;
>>>>>> 1595 int ret;
>>>>> /\ value should have been an int and
>>>>> placed here. It can take the value -1 hence the check
>>>>
>>>> Thanks, that needs to be fixed before the final merge.
>>>>
>>>>> Are there any other comments on the series?
>>>>>
>>>>> The only issue I am aware of is that Denis identified a bug in asusd
>>>>> (asusctl userspace program daemon) in certain Asus G14/G16 laptops
>>>>> that cause laptop keys to become sticky, I have had users also report
>>>>> that bug in previous versions of the series. WIthout asusd running,
>>>>> keyboards work fine incl. with brightness control (did not work
>>>>> before). Given it will take two months for this to reach mainline, I
>>>>> think it is a fair amount of time to address the bug.
>>>>
>>>> One thing that is not clear to me about this -- is this causing a visible
>>>> user-space behavior regression before vs. after the patchset with asusctl?
>>>>
>>>> If so, I am afraid this needs to be root-caused and fixed before the set
>>>> can be considered for inclusion.
>>
>>> Commit 591ba2074337 ("HID: asus: prevent binding to all HID devices on
>>> ROG") adds HID_QUIRK_INPUT_PER_APP and the extra devices seem to
>>> confuse asusd. Since the devices are the same as with hid-asus not
>>> loaded, it is specific to that program.
>>>
>>>
>> Hi Antheas.
>>
>> While you have previously expressed to me directly that you wish InputPlumber
>> didn't exist, it still very much does, in fact, exist. I also know that you are
>> explicitly aware that InputPlumber is a consumer of this interface, so your
>> comment that asusctl is the only affected program is something you know to be
>> false. This is not even the first time you have renamed an input device that
>> you knew InputPlumber was a consumer of without notifying me[1].
>>
>> I can't abide you outright lying to the maintainers here and I'm sick and tired
>> of having to watch your every move on the LKML. Either become a good citizen of
>> kernel maintenance, or get out of it.
>
> Hi Derek,
> I am not aware if your software is affected or not by this series as I
> have not received reports about it.
>
> If it is, please add:
> "ASUSTeK Computer Inc. N-KEY Device"
>
> As a name match to your software (should only take 5 minutes).
>
> I worked with Luke and Dennis on it for the better part of a year so
> hopefully they forwarded to you if it affects you or not.
>
> Your software relies on OOT drivers for most devices incl. the Ally so
> I am unsure if it is affected in reality. E.g., it would not be
> affected in SteamOS and CachyOS. In the future, it would be good to
> avoid name matches for your software as it makes it very fragile,
> which is a discussion we have had before. I do not think device evdev
> names constitute an ABI technically.
Taking no sides here.
An unfortunate reality is that there is stuff out there that does use name matches (and yes I agree they shouldn’t because it is *not* an ABI and many many devices have had name changes over the decades).
While name strings aren't a formal ABI, when a change affects known downstream users, a heads-up helps the ecosystem adapt smoothly—even if the technical stance is that they shouldn't rely on names.
In general it also needs to be justified such as:
- "Matches updated product branding"
- "Current string is misleading (says 'mouse' but it's a keyboard)"
- "Fixing spelling error"
- "Aligning with USB-IF device class names"
I always advocated use of evdev libraries to discover devices rather than the shortcut of name matching as it is much more powerful and reliable (which is how asusctl does dynamic add/remove of LED dev dbus interfaces). It’s much better to use evdev with vid/pid, device sub/classes, into descriptors and so on - you can be as open or restrictive as required by use case. This particular issue illustrates why this approach is preferable.
If the name change is a result of something I said or missed then I apologise to both Derek and yourself. Likely I missed it as I’ve never relied on name strings for userspace.
Regarding the OOT ally drivers I started, these will of course get upstreamed in the future (by Denis in this case when he can). They are getting very heavily battled tested in the mean time. Please do contribute to them if there is anything required to be addressed or chat to Denis, after all they are made only to benefit all users (there is no *race to be first* here.
As I no longer work on Asus laptops/handhelds and don't have hardware left to test with, I can't contribute further to this discussion. Best of luck resolving this.
I'm out.
Luke.
>
> Best,
> Antheas
>
>> Commit 591ba2074337 ("HID: asus: prevent binding to all HID devices on ROG")
>> Nacked-By: Derek J. Clark <derekjohn.clark@gmail.com>
>>
>> - Derek
>>
>> [1] https://lore.kernel.org/linux-input/Z74vZD7ZtKBTDlwy@google.com/
>>
>>> We can delay that patch until Denis who took over maintenance of the
>>> program can have a deeper look. I will still keep the last part of
>>> that patch that skips the input check, because that causes errors in
>>> devices that do not create an input device (e.g., lightbar).
>>>
>>> Antheas
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v7 5/9] platform/x86: asus-wmi: Add support for multiple kbd led handlers
2025-11-03 8:37 ` luke
@ 2025-11-03 8:48 ` Antheas Kapenekakis
2025-11-03 9:05 ` luke
0 siblings, 1 reply; 34+ messages in thread
From: Antheas Kapenekakis @ 2025-11-03 8:48 UTC (permalink / raw)
To: luke
Cc: Derek J. Clark, Jiri Kosina, Benjamin Tissoires, Corentin Chary,
Hans de Goede, Ilpo Järvinen, Denis Benato,
kernel test robot, platform-driver-x86, linux-input,
oe-kbuild-all, linux-kernel
On Mon, 3 Nov 2025 at 09:38, <luke@ljones.dev> wrote:
>
>
> > On 3 Nov 2025, at 20:36, Antheas Kapenekakis <lkml@antheas.dev> wrote:
> >
> > On Mon, 3 Nov 2025 at 05:29, Derek J. Clark <derekjohn.clark@gmail.com> wrote:
> >>
> >>> On Fri, 31 Oct 2025 at 09:27, Jiri Kosina <jikos@kernel.org> wrote:
> >>>>
> >>>> On Thu, 23 Oct 2025, Antheas Kapenekakis wrote:
> >>>>
> >>>>>> 1589
> >>>>>> 1590 static void kbd_led_update_all(struct work_struct *work)
> >>>>>> 1591 {
> >>>>>> 1592 enum led_brightness value;
> >>>>>> 1593 struct asus_wmi *asus;
> >>>>>> 1594 bool registered, notify;
> >>>>>> 1595 int ret;
> >>>>> /\ value should have been an int and
> >>>>> placed here. It can take the value -1 hence the check
> >>>>
> >>>> Thanks, that needs to be fixed before the final merge.
> >>>>
> >>>>> Are there any other comments on the series?
> >>>>>
> >>>>> The only issue I am aware of is that Denis identified a bug in asusd
> >>>>> (asusctl userspace program daemon) in certain Asus G14/G16 laptops
> >>>>> that cause laptop keys to become sticky, I have had users also report
> >>>>> that bug in previous versions of the series. WIthout asusd running,
> >>>>> keyboards work fine incl. with brightness control (did not work
> >>>>> before). Given it will take two months for this to reach mainline, I
> >>>>> think it is a fair amount of time to address the bug.
> >>>>
> >>>> One thing that is not clear to me about this -- is this causing a visible
> >>>> user-space behavior regression before vs. after the patchset with asusctl?
> >>>>
> >>>> If so, I am afraid this needs to be root-caused and fixed before the set
> >>>> can be considered for inclusion.
> >>
> >>> Commit 591ba2074337 ("HID: asus: prevent binding to all HID devices on
> >>> ROG") adds HID_QUIRK_INPUT_PER_APP and the extra devices seem to
> >>> confuse asusd. Since the devices are the same as with hid-asus not
> >>> loaded, it is specific to that program.
> >>>
> >>>
> >> Hi Antheas.
> >>
> >> While you have previously expressed to me directly that you wish InputPlumber
> >> didn't exist, it still very much does, in fact, exist. I also know that you are
> >> explicitly aware that InputPlumber is a consumer of this interface, so your
> >> comment that asusctl is the only affected program is something you know to be
> >> false. This is not even the first time you have renamed an input device that
> >> you knew InputPlumber was a consumer of without notifying me[1].
> >>
> >> I can't abide you outright lying to the maintainers here and I'm sick and tired
> >> of having to watch your every move on the LKML. Either become a good citizen of
> >> kernel maintenance, or get out of it.
> >
> > Hi Derek,
> > I am not aware if your software is affected or not by this series as I
> > have not received reports about it.
> >
> > If it is, please add:
> > "ASUSTeK Computer Inc. N-KEY Device"
> >
> > As a name match to your software (should only take 5 minutes).
> >
> > I worked with Luke and Dennis on it for the better part of a year so
> > hopefully they forwarded to you if it affects you or not.
> >
> > Your software relies on OOT drivers for most devices incl. the Ally so
> > I am unsure if it is affected in reality. E.g., it would not be
> > affected in SteamOS and CachyOS. In the future, it would be good to
> > avoid name matches for your software as it makes it very fragile,
> > which is a discussion we have had before. I do not think device evdev
> > names constitute an ABI technically.
>
> Taking no sides here.
>
> An unfortunate reality is that there is stuff out there that does use name matches (and yes I agree they shouldn’t because it is *not* an ABI and many many devices have had name changes over the decades).
>
> While name strings aren't a formal ABI, when a change affects known downstream users, a heads-up helps the ecosystem adapt smoothly—even if the technical stance is that they shouldn't rely on names.
>
> In general it also needs to be justified such as:
> - "Matches updated product branding"
> - "Current string is misleading (says 'mouse' but it's a keyboard)"
> - "Fixing spelling error"
> - "Aligning with USB-IF device class names"
>
> I always advocated use of evdev libraries to discover devices rather than the shortcut of name matching as it is much more powerful and reliable (which is how asusctl does dynamic add/remove of LED dev dbus interfaces). It’s much better to use evdev with vid/pid, device sub/classes, into descriptors and so on - you can be as open or restrictive as required by use case. This particular issue illustrates why this approach is preferable.
>
> If the name change is a result of something I said or missed then I apologise to both Derek and yourself. Likely I missed it as I’ve never relied on name strings for userspace.
>
> Regarding the OOT ally drivers I started, these will of course get upstreamed in the future (by Denis in this case when he can). They are getting very heavily battled tested in the mean time. Please do contribute to them if there is anything required to be addressed or chat to Denis, after all they are made only to benefit all users (there is no *race to be first* here.
>
> As I no longer work on Asus laptops/handhelds and don't have hardware left to test with, I can't contribute further to this discussion. Best of luck resolving this.
>
> I'm out.
> Luke.
Hi Luke,
good luck to your future endeavors.
The OOT reference was not to disparage the drivers, just to note that
in kernels that use those, hid-asus is stubbed for the Ally so there
is no change there.
As for reasons, see below.
> - "Matches updated product branding"
Reason for [1]
> - "Aligning with USB-IF device class names"
Reason for the change in this patch
If you would like me to stop cc'ing you in future asus changes let me
know. I am preparing some additional patches for the Duo class of
laptops.
Best,
Antheas
> >
> > Best,
> > Antheas
> >
> >> Commit 591ba2074337 ("HID: asus: prevent binding to all HID devices on ROG")
> >> Nacked-By: Derek J. Clark <derekjohn.clark@gmail.com>
> >>
> >> - Derek
> >>
> >> [1] https://lore.kernel.org/linux-input/Z74vZD7ZtKBTDlwy@google.com/
> >>
> >>> We can delay that patch until Denis who took over maintenance of the
> >>> program can have a deeper look. I will still keep the last part of
> >>> that patch that skips the input check, because that causes errors in
> >>> devices that do not create an input device (e.g., lightbar).
> >>>
> >>> Antheas
>
>
>
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v7 5/9] platform/x86: asus-wmi: Add support for multiple kbd led handlers
2025-11-03 8:48 ` Antheas Kapenekakis
@ 2025-11-03 9:05 ` luke
2025-11-03 9:15 ` Antheas Kapenekakis
0 siblings, 1 reply; 34+ messages in thread
From: luke @ 2025-11-03 9:05 UTC (permalink / raw)
To: Antheas Kapenekakis
Cc: Derek J. Clark, Jiri Kosina, Benjamin Tissoires, Corentin Chary,
Hans de Goede, Ilpo Järvinen, Denis Benato,
kernel test robot, platform-driver-x86, linux-input,
oe-kbuild-all, linux-kernel
> On 3 Nov 2025, at 21:48, Antheas Kapenekakis <lkml@antheas.dev> wrote:
>
> On Mon, 3 Nov 2025 at 09:38, <luke@ljones.dev> wrote:
>>
>>
>>> On 3 Nov 2025, at 20:36, Antheas Kapenekakis <lkml@antheas.dev> wrote:
>>>
>>> On Mon, 3 Nov 2025 at 05:29, Derek J. Clark <derekjohn.clark@gmail.com> wrote:
>>>>
>>>>> On Fri, 31 Oct 2025 at 09:27, Jiri Kosina <jikos@kernel.org> wrote:
>>>>>>
>>>>>> On Thu, 23 Oct 2025, Antheas Kapenekakis wrote:
>>>>>>
>>>>>>>> 1589
>>>>>>>> 1590 static void kbd_led_update_all(struct work_struct *work)
>>>>>>>> 1591 {
>>>>>>>> 1592 enum led_brightness value;
>>>>>>>> 1593 struct asus_wmi *asus;
>>>>>>>> 1594 bool registered, notify;
>>>>>>>> 1595 int ret;
>>>>>>> /\ value should have been an int and
>>>>>>> placed here. It can take the value -1 hence the check
>>>>>>
>>>>>> Thanks, that needs to be fixed before the final merge.
>>>>>>
>>>>>>> Are there any other comments on the series?
>>>>>>>
>>>>>>> The only issue I am aware of is that Denis identified a bug in asusd
>>>>>>> (asusctl userspace program daemon) in certain Asus G14/G16 laptops
>>>>>>> that cause laptop keys to become sticky, I have had users also report
>>>>>>> that bug in previous versions of the series. WIthout asusd running,
>>>>>>> keyboards work fine incl. with brightness control (did not work
>>>>>>> before). Given it will take two months for this to reach mainline, I
>>>>>>> think it is a fair amount of time to address the bug.
>>>>>>
>>>>>> One thing that is not clear to me about this -- is this causing a visible
>>>>>> user-space behavior regression before vs. after the patchset with asusctl?
>>>>>>
>>>>>> If so, I am afraid this needs to be root-caused and fixed before the set
>>>>>> can be considered for inclusion.
>>>>
>>>>> Commit 591ba2074337 ("HID: asus: prevent binding to all HID devices on
>>>>> ROG") adds HID_QUIRK_INPUT_PER_APP and the extra devices seem to
>>>>> confuse asusd. Since the devices are the same as with hid-asus not
>>>>> loaded, it is specific to that program.
>>>>>
>>>>>
>>>> Hi Antheas.
>>>>
>>>> While you have previously expressed to me directly that you wish InputPlumber
>>>> didn't exist, it still very much does, in fact, exist. I also know that you are
>>>> explicitly aware that InputPlumber is a consumer of this interface, so your
>>>> comment that asusctl is the only affected program is something you know to be
>>>> false. This is not even the first time you have renamed an input device that
>>>> you knew InputPlumber was a consumer of without notifying me[1].
>>>>
>>>> I can't abide you outright lying to the maintainers here and I'm sick and tired
>>>> of having to watch your every move on the LKML. Either become a good citizen of
>>>> kernel maintenance, or get out of it.
>>>
>>> Hi Derek,
>>> I am not aware if your software is affected or not by this series as I
>>> have not received reports about it.
>>>
>>> If it is, please add:
>>> "ASUSTeK Computer Inc. N-KEY Device"
>>>
>>> As a name match to your software (should only take 5 minutes).
>>>
>>> I worked with Luke and Dennis on it for the better part of a year so
>>> hopefully they forwarded to you if it affects you or not.
>>>
>>> Your software relies on OOT drivers for most devices incl. the Ally so
>>> I am unsure if it is affected in reality. E.g., it would not be
>>> affected in SteamOS and CachyOS. In the future, it would be good to
>>> avoid name matches for your software as it makes it very fragile,
>>> which is a discussion we have had before. I do not think device evdev
>>> names constitute an ABI technically.
>>
>> Taking no sides here.
>>
>> An unfortunate reality is that there is stuff out there that does use name matches (and yes I agree they shouldn’t because it is *not* an ABI and many many devices have had name changes over the decades).
>>
>> While name strings aren't a formal ABI, when a change affects known downstream users, a heads-up helps the ecosystem adapt smoothly—even if the technical stance is that they shouldn't rely on names.
>>
>> In general it also needs to be justified such as:
>> - "Matches updated product branding"
>> - "Current string is misleading (says 'mouse' but it's a keyboard)"
>> - "Fixing spelling error"
>> - "Aligning with USB-IF device class names"
>>
>> I always advocated use of evdev libraries to discover devices rather than the shortcut of name matching as it is much more powerful and reliable (which is how asusctl does dynamic add/remove of LED dev dbus interfaces). It’s much better to use evdev with vid/pid, device sub/classes, into descriptors and so on - you can be as open or restrictive as required by use case. This particular issue illustrates why this approach is preferable.
>>
>> If the name change is a result of something I said or missed then I apologise to both Derek and yourself. Likely I missed it as I’ve never relied on name strings for userspace.
>>
>> Regarding the OOT ally drivers I started, these will of course get upstreamed in the future (by Denis in this case when he can). They are getting very heavily battled tested in the mean time. Please do contribute to them if there is anything required to be addressed or chat to Denis, after all they are made only to benefit all users (there is no *race to be first* here.
>>
>> As I no longer work on Asus laptops/handhelds and don't have hardware left to test with, I can't contribute further to this discussion. Best of luck resolving this.
>>
>> I'm out.
>> Luke.
>
> Hi Luke,
> good luck to your future endeavors.
>
> The OOT reference was not to disparage the drivers, just to note that
> in kernels that use those, hid-asus is stubbed for the Ally so there
> is no change there.
>
> As for reasons, see below.
>
>> - "Matches updated product branding"
>
> Reason for [1]
>
>> - "Aligning with USB-IF device class names"
>
> Reason for the change in this patch
Ack. Thank you, appreciate the clarification, this is the appropriate reason. Might be worth mentioning in change cover letter or commit message (if not already, sorry if I missed).
>
> If you would like me to stop cc'ing you in future asus changes let me
> know. I am preparing some additional patches for the Duo class of
> laptops.
Leave me in for the moment. I’ll submit a patch to remove myself from maintainers once Denis is comfortable in the role.
Regarding Duo, I assume you mean the larger type with two full-size screens and not the older models with a thin screen above the keyboard?
There was an issue with the older ones which I could not solve until I had the physical hardware to test due to it being difficult for testers to describe the exact behaviour. The way the power-off and brightness control works is funky. I submitted the patch here https://lore.kernel.org/all/20250525204214.104030-1-luke@ljones.dev/
Hopefully that’s of some help to you. It’s very unlikely I will resubmit that as a split series.
If you need anything from me related to above or otherwise, do please reach out off-chain to avoid us further hitting the CC.
>
> Best,
> Antheas
>
>>>
>>> Best,
>>> Antheas
>>>
>>>> Commit 591ba2074337 ("HID: asus: prevent binding to all HID devices on ROG")
>>>> Nacked-By: Derek J. Clark <derekjohn.clark@gmail.com>
>>>>
>>>> - Derek
>>>>
>>>> [1] https://lore.kernel.org/linux-input/Z74vZD7ZtKBTDlwy@google.com/
>>>>
>>>>> We can delay that patch until Denis who took over maintenance of the
>>>>> program can have a deeper look. I will still keep the last part of
>>>>> that patch that skips the input check, because that causes errors in
>>>>> devices that do not create an input device (e.g., lightbar).
>>>>>
>>>>> Antheas
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v7 5/9] platform/x86: asus-wmi: Add support for multiple kbd led handlers
2025-11-03 9:05 ` luke
@ 2025-11-03 9:15 ` Antheas Kapenekakis
0 siblings, 0 replies; 34+ messages in thread
From: Antheas Kapenekakis @ 2025-11-03 9:15 UTC (permalink / raw)
To: luke
Cc: Derek J. Clark, Jiri Kosina, Benjamin Tissoires, Corentin Chary,
Ilpo Järvinen, Denis Benato, kernel test robot,
platform-driver-x86, linux-input, oe-kbuild-all, linux-kernel
On Mon, 3 Nov 2025 at 10:06, <luke@ljones.dev> wrote:
>
>
>
> > On 3 Nov 2025, at 21:48, Antheas Kapenekakis <lkml@antheas.dev> wrote:
> >
> > On Mon, 3 Nov 2025 at 09:38, <luke@ljones.dev> wrote:
> >>
> >>
> >>> On 3 Nov 2025, at 20:36, Antheas Kapenekakis <lkml@antheas.dev> wrote:
> >>>
> >>> On Mon, 3 Nov 2025 at 05:29, Derek J. Clark <derekjohn.clark@gmail.com> wrote:
> >>>>
> >>>>> On Fri, 31 Oct 2025 at 09:27, Jiri Kosina <jikos@kernel.org> wrote:
> >>>>>>
> >>>>>> On Thu, 23 Oct 2025, Antheas Kapenekakis wrote:
> >>>>>>
> >>>>>>>> 1589
> >>>>>>>> 1590 static void kbd_led_update_all(struct work_struct *work)
> >>>>>>>> 1591 {
> >>>>>>>> 1592 enum led_brightness value;
> >>>>>>>> 1593 struct asus_wmi *asus;
> >>>>>>>> 1594 bool registered, notify;
> >>>>>>>> 1595 int ret;
> >>>>>>> /\ value should have been an int and
> >>>>>>> placed here. It can take the value -1 hence the check
> >>>>>>
> >>>>>> Thanks, that needs to be fixed before the final merge.
> >>>>>>
> >>>>>>> Are there any other comments on the series?
> >>>>>>>
> >>>>>>> The only issue I am aware of is that Denis identified a bug in asusd
> >>>>>>> (asusctl userspace program daemon) in certain Asus G14/G16 laptops
> >>>>>>> that cause laptop keys to become sticky, I have had users also report
> >>>>>>> that bug in previous versions of the series. WIthout asusd running,
> >>>>>>> keyboards work fine incl. with brightness control (did not work
> >>>>>>> before). Given it will take two months for this to reach mainline, I
> >>>>>>> think it is a fair amount of time to address the bug.
> >>>>>>
> >>>>>> One thing that is not clear to me about this -- is this causing a visible
> >>>>>> user-space behavior regression before vs. after the patchset with asusctl?
> >>>>>>
> >>>>>> If so, I am afraid this needs to be root-caused and fixed before the set
> >>>>>> can be considered for inclusion.
> >>>>
> >>>>> Commit 591ba2074337 ("HID: asus: prevent binding to all HID devices on
> >>>>> ROG") adds HID_QUIRK_INPUT_PER_APP and the extra devices seem to
> >>>>> confuse asusd. Since the devices are the same as with hid-asus not
> >>>>> loaded, it is specific to that program.
> >>>>>
> >>>>>
> >>>> Hi Antheas.
> >>>>
> >>>> While you have previously expressed to me directly that you wish InputPlumber
> >>>> didn't exist, it still very much does, in fact, exist. I also know that you are
> >>>> explicitly aware that InputPlumber is a consumer of this interface, so your
> >>>> comment that asusctl is the only affected program is something you know to be
> >>>> false. This is not even the first time you have renamed an input device that
> >>>> you knew InputPlumber was a consumer of without notifying me[1].
> >>>>
> >>>> I can't abide you outright lying to the maintainers here and I'm sick and tired
> >>>> of having to watch your every move on the LKML. Either become a good citizen of
> >>>> kernel maintenance, or get out of it.
> >>>
> >>> Hi Derek,
> >>> I am not aware if your software is affected or not by this series as I
> >>> have not received reports about it.
> >>>
> >>> If it is, please add:
> >>> "ASUSTeK Computer Inc. N-KEY Device"
> >>>
> >>> As a name match to your software (should only take 5 minutes).
> >>>
> >>> I worked with Luke and Dennis on it for the better part of a year so
> >>> hopefully they forwarded to you if it affects you or not.
> >>>
> >>> Your software relies on OOT drivers for most devices incl. the Ally so
> >>> I am unsure if it is affected in reality. E.g., it would not be
> >>> affected in SteamOS and CachyOS. In the future, it would be good to
> >>> avoid name matches for your software as it makes it very fragile,
> >>> which is a discussion we have had before. I do not think device evdev
> >>> names constitute an ABI technically.
> >>
> >> Taking no sides here.
> >>
> >> An unfortunate reality is that there is stuff out there that does use name matches (and yes I agree they shouldn’t because it is *not* an ABI and many many devices have had name changes over the decades).
> >>
> >> While name strings aren't a formal ABI, when a change affects known downstream users, a heads-up helps the ecosystem adapt smoothly—even if the technical stance is that they shouldn't rely on names.
> >>
> >> In general it also needs to be justified such as:
> >> - "Matches updated product branding"
> >> - "Current string is misleading (says 'mouse' but it's a keyboard)"
> >> - "Fixing spelling error"
> >> - "Aligning with USB-IF device class names"
> >>
> >> I always advocated use of evdev libraries to discover devices rather than the shortcut of name matching as it is much more powerful and reliable (which is how asusctl does dynamic add/remove of LED dev dbus interfaces). It’s much better to use evdev with vid/pid, device sub/classes, into descriptors and so on - you can be as open or restrictive as required by use case. This particular issue illustrates why this approach is preferable.
> >>
> >> If the name change is a result of something I said or missed then I apologise to both Derek and yourself. Likely I missed it as I’ve never relied on name strings for userspace.
> >>
> >> Regarding the OOT ally drivers I started, these will of course get upstreamed in the future (by Denis in this case when he can). They are getting very heavily battled tested in the mean time. Please do contribute to them if there is anything required to be addressed or chat to Denis, after all they are made only to benefit all users (there is no *race to be first* here.
> >>
> >> As I no longer work on Asus laptops/handhelds and don't have hardware left to test with, I can't contribute further to this discussion. Best of luck resolving this.
> >>
> >> I'm out.
> >> Luke.
> >
> > Hi Luke,
> > good luck to your future endeavors.
> >
> > The OOT reference was not to disparage the drivers, just to note that
> > in kernels that use those, hid-asus is stubbed for the Ally so there
> > is no change there.
> >
> > As for reasons, see below.
> >
> >> - "Matches updated product branding"
> >
> > Reason for [1]
> >
> >> - "Aligning with USB-IF device class names"
> >
> > Reason for the change in this patch
>
> Ack. Thank you, appreciate the clarification, this is the appropriate reason. Might be worth mentioning in change cover letter or commit message (if not already, sorry if I missed).
>
> >
> > If you would like me to stop cc'ing you in future asus changes let me
> > know. I am preparing some additional patches for the Duo class of
> > laptops.
>
> Leave me in for the moment. I’ll submit a patch to remove myself from maintainers once Denis is comfortable in the role.
>
> Regarding Duo, I assume you mean the larger type with two full-size screens and not the older models with a thin screen above the keyboard?
> There was an issue with the older ones which I could not solve until I had the physical hardware to test due to it being difficult for testers to describe the exact behaviour. The way the power-off and brightness control works is funky. I submitted the patch here https://lore.kernel.org/all/20250525204214.104030-1-luke@ljones.dev/
>
> Hopefully that’s of some help to you. It’s very unlikely I will resubmit that as a split series.
>
> If you need anything from me related to above or otherwise, do please reach out off-chain to avoid us further hitting the CC.
Yes, the two screen type. I am focusing on the keyboard for now. I am
waiting for a user to test the changes. This series helps with
maintaining keyboard brightness on that. Hopefully it will work as the
keyboard transitions from USB to bluetooth.
If there are issues with the screen brightness, I will rereview your
patch and if it is needed adapt it and coby you.
Best,
Antheas
> >
> > Best,
> > Antheas
> >
> >>>
> >>> Best,
> >>> Antheas
> >>>
> >>>> Commit 591ba2074337 ("HID: asus: prevent binding to all HID devices on ROG")
> >>>> Nacked-By: Derek J. Clark <derekjohn.clark@gmail.com>
> >>>>
> >>>> - Derek
> >>>>
> >>>> [1] https://lore.kernel.org/linux-input/Z74vZD7ZtKBTDlwy@google.com/
> >>>>
> >>>>> We can delay that patch until Denis who took over maintenance of the
> >>>>> program can have a deeper look. I will still keep the last part of
> >>>>> that patch that skips the input check, because that causes errors in
> >>>>> devices that do not create an input device (e.g., lightbar).
> >>>>>
> >>>>> Antheas
>
>
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v7 6/9] HID: asus: listen to the asus-wmi brightness device instead of creating one
2025-10-18 10:17 [PATCH v7 0/9] HID: asus: Fix ASUS ROG Laptop's Keyboard backlight handling Antheas Kapenekakis
` (4 preceding siblings ...)
2025-10-18 10:17 ` [PATCH v7 5/9] platform/x86: asus-wmi: Add support for multiple kbd led handlers Antheas Kapenekakis
@ 2025-10-18 10:17 ` Antheas Kapenekakis
2025-10-18 10:17 ` [PATCH v7 7/9] platform/x86: asus-wmi: remove unused keyboard backlight quirk Antheas Kapenekakis
` (2 subsequent siblings)
8 siblings, 0 replies; 34+ messages in thread
From: Antheas Kapenekakis @ 2025-10-18 10:17 UTC (permalink / raw)
To: platform-driver-x86, linux-input
Cc: linux-kernel, Jiri Kosina, Benjamin Tissoires, Corentin Chary,
Luke D . Jones, Hans de Goede, Ilpo Järvinen, Denis Benato,
Antheas Kapenekakis
Some ROG laptops expose multiple interfaces for controlling the
keyboard/RGB brightness. This creates a name conflict under
asus::kbd_brightness, where the second device ends up being
named asus::kbd_brightness_1 and they are both broken.
Therefore, register a listener to the asus-wmi brightness device
instead of creating a new one.
Reviewed-by: Luke D. Jones <luke@ljones.dev>
Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
---
drivers/hid/hid-asus.c | 64 +++++++-----------------------------------
1 file changed, 10 insertions(+), 54 deletions(-)
diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index bbbac98f76c6..96cff7690987 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -102,7 +102,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
#define TRKID_SGN ((TRKID_MAX + 1) >> 1)
struct asus_kbd_leds {
- struct led_classdev cdev;
+ struct asus_hid_listener listener;
struct hid_device *hdev;
struct work_struct work;
unsigned int brightness;
@@ -494,11 +494,11 @@ static void asus_schedule_work(struct asus_kbd_leds *led)
spin_unlock_irqrestore(&led->lock, flags);
}
-static void asus_kbd_backlight_set(struct led_classdev *led_cdev,
- enum led_brightness brightness)
+static void asus_kbd_backlight_set(struct asus_hid_listener *listener,
+ int brightness)
{
- struct asus_kbd_leds *led = container_of(led_cdev, struct asus_kbd_leds,
- cdev);
+ struct asus_kbd_leds *led = container_of(listener, struct asus_kbd_leds,
+ listener);
unsigned long flags;
spin_lock_irqsave(&led->lock, flags);
@@ -508,20 +508,6 @@ static void asus_kbd_backlight_set(struct led_classdev *led_cdev,
asus_schedule_work(led);
}
-static enum led_brightness asus_kbd_backlight_get(struct led_classdev *led_cdev)
-{
- struct asus_kbd_leds *led = container_of(led_cdev, struct asus_kbd_leds,
- cdev);
- enum led_brightness brightness;
- unsigned long flags;
-
- spin_lock_irqsave(&led->lock, flags);
- brightness = led->brightness;
- spin_unlock_irqrestore(&led->lock, flags);
-
- return brightness;
-}
-
static void asus_kbd_backlight_work(struct work_struct *work)
{
struct asus_kbd_leds *led = container_of(work, struct asus_kbd_leds, work);
@@ -538,34 +524,6 @@ static void asus_kbd_backlight_work(struct work_struct *work)
hid_err(led->hdev, "Asus failed to set keyboard backlight: %d\n", ret);
}
-/* WMI-based keyboard backlight LED control (via asus-wmi driver) takes
- * precedence. We only activate HID-based backlight control when the
- * WMI control is not available.
- */
-static bool asus_kbd_wmi_led_control_present(struct hid_device *hdev)
-{
- struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
- u32 value;
- int ret;
-
- if (!IS_ENABLED(CONFIG_ASUS_WMI))
- return false;
-
- if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD &&
- dmi_check_system(asus_use_hid_led_dmi_ids)) {
- hid_info(hdev, "using HID for asus::kbd_backlight\n");
- return false;
- }
-
- ret = asus_wmi_evaluate_method(ASUS_WMI_METHODID_DSTS,
- ASUS_WMI_DEVID_KBD_BACKLIGHT, 0, &value);
- hid_dbg(hdev, "WMI backlight check: rc %d value %x", ret, value);
- if (ret)
- return false;
-
- return !!(value & ASUS_WMI_DSTS_PRESENCE_BIT);
-}
-
/*
* We don't care about any other part of the string except the version section.
* Example strings: FGA80100.RC72LA.312_T01, FGA80100.RC71LS.318_T01
@@ -700,14 +658,11 @@ static int asus_kbd_register_leds(struct hid_device *hdev)
drvdata->kbd_backlight->removed = false;
drvdata->kbd_backlight->brightness = 0;
drvdata->kbd_backlight->hdev = hdev;
- drvdata->kbd_backlight->cdev.name = "asus::kbd_backlight";
- drvdata->kbd_backlight->cdev.max_brightness = 3;
- drvdata->kbd_backlight->cdev.brightness_set = asus_kbd_backlight_set;
- drvdata->kbd_backlight->cdev.brightness_get = asus_kbd_backlight_get;
+ drvdata->kbd_backlight->listener.brightness_set = asus_kbd_backlight_set;
INIT_WORK(&drvdata->kbd_backlight->work, asus_kbd_backlight_work);
spin_lock_init(&drvdata->kbd_backlight->lock);
- ret = devm_led_classdev_register(&hdev->dev, &drvdata->kbd_backlight->cdev);
+ ret = asus_hid_register_listener(&drvdata->kbd_backlight->listener);
if (ret < 0) {
/* No need to have this still around */
devm_kfree(&hdev->dev, drvdata->kbd_backlight);
@@ -1096,7 +1051,7 @@ static int __maybe_unused asus_resume(struct hid_device *hdev) {
if (drvdata->kbd_backlight) {
const u8 buf[] = { FEATURE_KBD_REPORT_ID, 0xba, 0xc5, 0xc4,
- drvdata->kbd_backlight->cdev.brightness };
+ drvdata->kbd_backlight->brightness };
ret = asus_kbd_set_report(hdev, buf, sizeof(buf));
if (ret < 0) {
hid_err(hdev, "Asus failed to set keyboard backlight: %d\n", ret);
@@ -1232,7 +1187,6 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
}
if (is_vendor && (drvdata->quirks & QUIRK_USE_KBD_BACKLIGHT) &&
- !asus_kbd_wmi_led_control_present(hdev) &&
asus_kbd_register_leds(hdev))
hid_warn(hdev, "Failed to initialize backlight.\n");
@@ -1273,6 +1227,8 @@ static void asus_remove(struct hid_device *hdev)
unsigned long flags;
if (drvdata->kbd_backlight) {
+ asus_hid_unregister_listener(&drvdata->kbd_backlight->listener);
+
spin_lock_irqsave(&drvdata->kbd_backlight->lock, flags);
drvdata->kbd_backlight->removed = true;
spin_unlock_irqrestore(&drvdata->kbd_backlight->lock, flags);
--
2.51.0
^ permalink raw reply related [flat|nested] 34+ messages in thread* [PATCH v7 7/9] platform/x86: asus-wmi: remove unused keyboard backlight quirk
2025-10-18 10:17 [PATCH v7 0/9] HID: asus: Fix ASUS ROG Laptop's Keyboard backlight handling Antheas Kapenekakis
` (5 preceding siblings ...)
2025-10-18 10:17 ` [PATCH v7 6/9] HID: asus: listen to the asus-wmi brightness device instead of creating one Antheas Kapenekakis
@ 2025-10-18 10:17 ` Antheas Kapenekakis
2025-10-18 10:17 ` [PATCH v7 8/9] platform/x86: asus-wmi: add keyboard brightness event handler Antheas Kapenekakis
2025-10-18 10:17 ` [PATCH v7 9/9] HID: asus: add support for the asus-wmi brightness handler Antheas Kapenekakis
8 siblings, 0 replies; 34+ messages in thread
From: Antheas Kapenekakis @ 2025-10-18 10:17 UTC (permalink / raw)
To: platform-driver-x86, linux-input
Cc: linux-kernel, Jiri Kosina, Benjamin Tissoires, Corentin Chary,
Luke D . Jones, Hans de Goede, Ilpo Järvinen, Denis Benato,
Antheas Kapenekakis
The quirk for selecting whether keyboard backlight should be controlled
by HID or WMI is not needed anymore, so remove it.
Reviewed-by: Luke D. Jones <luke@ljones.dev>
Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
---
include/linux/platform_data/x86/asus-wmi.h | 40 ----------------------
1 file changed, 40 deletions(-)
diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
index 1165039013b1..d8c5269854b0 100644
--- a/include/linux/platform_data/x86/asus-wmi.h
+++ b/include/linux/platform_data/x86/asus-wmi.h
@@ -203,44 +203,4 @@ 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 */
-static const struct dmi_system_id asus_use_hid_led_dmi_ids[] = {
- {
- .matches = {
- DMI_MATCH(DMI_PRODUCT_FAMILY, "ROG Zephyrus"),
- },
- },
- {
- .matches = {
- DMI_MATCH(DMI_PRODUCT_FAMILY, "ROG Strix"),
- },
- },
- {
- .matches = {
- DMI_MATCH(DMI_PRODUCT_FAMILY, "ROG Flow"),
- },
- },
- {
- .matches = {
- DMI_MATCH(DMI_PRODUCT_FAMILY, "ProArt P16"),
- },
- },
- {
- .matches = {
- DMI_MATCH(DMI_BOARD_NAME, "GA403U"),
- },
- },
- {
- .matches = {
- DMI_MATCH(DMI_BOARD_NAME, "GU605M"),
- },
- },
- {
- .matches = {
- DMI_MATCH(DMI_BOARD_NAME, "RC71L"),
- },
- },
- { },
-};
-
#endif /* __PLATFORM_DATA_X86_ASUS_WMI_H */
--
2.51.0
^ permalink raw reply related [flat|nested] 34+ messages in thread* [PATCH v7 8/9] platform/x86: asus-wmi: add keyboard brightness event handler
2025-10-18 10:17 [PATCH v7 0/9] HID: asus: Fix ASUS ROG Laptop's Keyboard backlight handling Antheas Kapenekakis
` (6 preceding siblings ...)
2025-10-18 10:17 ` [PATCH v7 7/9] platform/x86: asus-wmi: remove unused keyboard backlight quirk Antheas Kapenekakis
@ 2025-10-18 10:17 ` Antheas Kapenekakis
2025-10-18 10:17 ` [PATCH v7 9/9] HID: asus: add support for the asus-wmi brightness handler Antheas Kapenekakis
8 siblings, 0 replies; 34+ messages in thread
From: Antheas Kapenekakis @ 2025-10-18 10:17 UTC (permalink / raw)
To: platform-driver-x86, linux-input
Cc: linux-kernel, Jiri Kosina, Benjamin Tissoires, Corentin Chary,
Luke D . Jones, Hans de Goede, Ilpo Järvinen, Denis Benato,
Antheas Kapenekakis
The keyboard brightness control of Asus WMI keyboards is handled in
kernel, which leads to the shortcut going from brightness 0, to 1,
to 2, and 3.
However, for HID keyboards it is exposed as a key and handled by the
user's desktop environment. For the toggle button, this means that
brightness control becomes on/off. In addition, in the absence of a
DE, the keyboard brightness does not work.
Therefore, expose an event handler for the keyboard brightness control
which can then be used by hid-asus. Since this handler is called from
an interrupt context, defer the actual work to a workqueue.
In the process, introduce ASUS_EV_MAX_BRIGHTNESS to hold the constant
for maximum brightness since it is shared between hid-asus/asus-wmi.
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 | 46 +++++++++++++++++++---
include/linux/platform_data/x86/asus-wmi.h | 13 ++++++
2 files changed, 54 insertions(+), 5 deletions(-)
diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index aab779142323..f229a50bd69e 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -1629,6 +1629,44 @@ static void kbd_led_update_all(struct work_struct *work)
}
}
+/*
+ * This function is called from hid-asus to inform asus-wmi of brightness
+ * changes initiated by the keyboard backlight keys.
+ */
+int asus_hid_event(enum asus_hid_event event)
+{
+ struct asus_wmi *asus;
+ int brightness;
+
+ guard(spinlock_irqsave)(&asus_ref.lock);
+ asus = asus_ref.asus;
+ if (!asus || !asus->kbd_led_registered)
+ return -EBUSY;
+
+ brightness = asus->kbd_led_wk;
+
+ switch (event) {
+ case ASUS_EV_BRTUP:
+ brightness += 1;
+ break;
+ case ASUS_EV_BRTDOWN:
+ brightness -= 1;
+ break;
+ case ASUS_EV_BRTTOGGLE:
+ if (brightness >= ASUS_EV_MAX_BRIGHTNESS)
+ brightness = 0;
+ else
+ brightness += 1;
+ break;
+ }
+
+ asus->kbd_led_wk = clamp_val(brightness, 0, ASUS_EV_MAX_BRIGHTNESS);
+ asus->kbd_led_notify = true;
+ queue_work(asus->led_workqueue, &asus->kbd_led_work);
+ return 0;
+}
+EXPORT_SYMBOL_GPL(asus_hid_event);
+
/*
* These functions actually update the LED's, and are called from a
* workqueue. By doing this as separate work rather than when the LED
@@ -1711,13 +1749,11 @@ 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;
asus = container_of(led_cdev, struct asus_wmi, kbd_led);
- max_level = asus->kbd_led.max_brightness;
scoped_guard(spinlock_irqsave, &asus_ref.lock)
- asus->kbd_led_wk = clamp_val(value, 0, max_level);
+ asus->kbd_led_wk = clamp_val(value, 0, ASUS_EV_MAX_BRIGHTNESS);
if (asus->kbd_led_avail)
kbd_led_update(asus);
@@ -1921,7 +1957,7 @@ static int asus_wmi_led_init(struct asus_wmi *asus)
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.max_brightness = ASUS_EV_MAX_BRIGHTNESS;
asus->kbd_led_avail = !kbd_led_read(asus, &led_val, NULL);
INIT_WORK(&asus->kbd_led_work, kbd_led_update_all);
@@ -4424,7 +4460,7 @@ static void asus_wmi_handle_event_code(int code, struct asus_wmi *asus)
return;
}
if (code == NOTIFY_KBD_BRTTOGGLE) {
- if (led_value == asus->kbd_led.max_brightness)
+ if (led_value == ASUS_EV_MAX_BRIGHTNESS)
kbd_led_set_by_kbd(asus, 0);
else
kbd_led_set_by_kbd(asus, led_value + 1);
diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
index d8c5269854b0..3f679598b629 100644
--- a/include/linux/platform_data/x86/asus-wmi.h
+++ b/include/linux/platform_data/x86/asus-wmi.h
@@ -169,6 +169,14 @@ struct asus_hid_listener {
void (*brightness_set)(struct asus_hid_listener *listener, int brightness);
};
+enum asus_hid_event {
+ ASUS_EV_BRTUP,
+ ASUS_EV_BRTDOWN,
+ ASUS_EV_BRTTOGGLE,
+};
+
+#define ASUS_EV_MAX_BRIGHTNESS 3
+
#if IS_REACHABLE(CONFIG_ASUS_WMI)
void set_ally_mcu_hack(enum asus_ally_mcu_hack status);
void set_ally_mcu_powersave(bool enabled);
@@ -177,6 +185,7 @@ 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);
+int asus_hid_event(enum asus_hid_event event);
#else
static inline void set_ally_mcu_hack(enum asus_ally_mcu_hack status)
{
@@ -201,6 +210,10 @@ static inline int asus_hid_register_listener(struct asus_hid_listener *bdev)
static inline void asus_hid_unregister_listener(struct asus_hid_listener *bdev)
{
}
+static inline int asus_hid_event(enum asus_hid_event event)
+{
+ return -ENODEV;
+}
#endif
#endif /* __PLATFORM_DATA_X86_ASUS_WMI_H */
--
2.51.0
^ permalink raw reply related [flat|nested] 34+ messages in thread* [PATCH v7 9/9] HID: asus: add support for the asus-wmi brightness handler
2025-10-18 10:17 [PATCH v7 0/9] HID: asus: Fix ASUS ROG Laptop's Keyboard backlight handling Antheas Kapenekakis
` (7 preceding siblings ...)
2025-10-18 10:17 ` [PATCH v7 8/9] platform/x86: asus-wmi: add keyboard brightness event handler Antheas Kapenekakis
@ 2025-10-18 10:17 ` Antheas Kapenekakis
8 siblings, 0 replies; 34+ messages in thread
From: Antheas Kapenekakis @ 2025-10-18 10:17 UTC (permalink / raw)
To: platform-driver-x86, linux-input
Cc: linux-kernel, Jiri Kosina, Benjamin Tissoires, Corentin Chary,
Luke D . Jones, Hans de Goede, Ilpo Järvinen, Denis Benato,
Antheas Kapenekakis
If the asus-wmi brightness handler is available, send the
keyboard brightness events to it instead of passing them
to userspace. If it is not, fall back to sending them to it.
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/hid/hid-asus.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index 96cff7690987..c2c25825cb42 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -324,6 +324,17 @@ static int asus_event(struct hid_device *hdev, struct hid_field *field,
usage->hid & HID_USAGE);
}
+ if (usage->type == EV_KEY && value) {
+ switch (usage->code) {
+ case KEY_KBDILLUMUP:
+ return !asus_hid_event(ASUS_EV_BRTUP);
+ case KEY_KBDILLUMDOWN:
+ return !asus_hid_event(ASUS_EV_BRTDOWN);
+ case KEY_KBDILLUMTOGGLE:
+ return !asus_hid_event(ASUS_EV_BRTTOGGLE);
+ }
+ }
+
return 0;
}
--
2.51.0
^ permalink raw reply related [flat|nested] 34+ messages in thread