| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID:
<CAGwozwFbQWyuQB6EwLMLon5muff2WudR+oVL62DqP_MXGW+p-Q@mail.gmail.com>
Date: Thu, 23 Oct 2025 23:30:08 +0200
From: Antheas Kapenekakis <lkml@...heas.dev>
To: Denis Benato <benato.denis96@...il.com>
Cc: platform-driver-x86@...r.kernel.org, linux-input@...r.kernel.org,
linux-kernel@...r.kernel.org, Jiri Kosina <jikos@...nel.org>,
Benjamin Tissoires <bentiss@...nel.org>,
Corentin Chary <corentin.chary@...il.com>,
"Luke D . Jones" <luke@...nes.dev>, Hans de Goede <hdegoede@...hat.com>,
Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Subject: Re: [PATCH v7 1/9] HID: asus: simplify RGB init sequence
On Thu, 23 Oct 2025 at 22:05, Denis Benato <benato.denis96@...il.com> wrote:
>
>
> On 10/23/25 20:06, Antheas Kapenekakis wrote:
> > On Thu, 23 Oct 2025 at 19:38, Denis Benato <benato.denis96@...il.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@...heas.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);
>
Powered by blists - more mailing lists