| 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:
<CAGwozwH71LDiBFM-Ro9UpZNy29C6HHwCNZjwCS3F1hMfuUXZ4g@mail.gmail.com>
Date: Sat, 17 Jan 2026 17:10:06 +0100
From: Antheas Kapenekakis <lkml@...heas.dev>
To: Denis Benato <denis.benato@...ux.dev>
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 <hansg@...nel.org>,
Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Subject: Re: [PATCH v11 02/11] HID: asus: initialize additional endpoints only
for legacy devices
On Sat, 17 Jan 2026 at 16:13, Denis Benato <denis.benato@...ux.dev> wrote:
>
>
> On 1/17/26 16:07, Antheas Kapenekakis wrote:
> > On Sat, 17 Jan 2026 at 14:51, Denis Benato <denis.benato@...ux.dev> wrote:
> >>
> >> On 1/17/26 00:10, Antheas Kapenekakis wrote:
> >>> On Fri, 16 Jan 2026 at 21:44, Denis Benato <denis.benato@...ux.dev> wrote:
> >>>> On 1/16/26 14:31, Antheas Kapenekakis wrote:
> >>>>
> >>>>> Currently, ID1/ID2 initializations are performed for all NKEY devices.
> >>>>> However, ID1 initializations are only required for RGB control and are
> >>>>> only supported for RGB capable devices. ID2 initializations are only
> >>>>> required for initializing the Anime display endpoint which is only
> >>>>> supported on devices with an Anime display. Both of these
> >>>>> initializations are out of scope for this driver (this is a brightness
> >>>>> control and keyboard shortcut driver) and they should not be performed
> >>>>> for devices that do not support them in any case.
> >>>>>
> >>>>> At the same time, there are older NKEY devices that have only been
> >>>>> tested with these initializations in the kernel and it is not possible
> >>>>> to recheck them. There is a possibility that especially with the ID1
> >>>>> initialization, certain laptop models might have their shortcuts stop
> >>>>> working (currently unproven).
> >>>>>
> >>>>> For an abundance of caution, only initialize ID1/ID2 for those older
> >>>>> NKEY devices by introducing a quirk for them and replacing the NKEY
> >>>>> quirk in the block that performs the inits with that.
> >>>>>
> >>>>> In addition, as these initializations might not be supported by the
> >>>>> affected devices, change the function to not bail if they fail.
> >>>>>
> >>>>> Acked-by: Benjamin Tissoires <bentiss@...nel.org>
> >>>>> Signed-off-by: Antheas Kapenekakis <lkml@...heas.dev>
> >>>>> ---
> >>>>> drivers/hid/hid-asus.c | 16 ++++++----------
> >>>>> 1 file changed, 6 insertions(+), 10 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> >>>>> index 323e6302bac5..dc7af12cf31a 100644
> >>>>> --- a/drivers/hid/hid-asus.c
> >>>>> +++ b/drivers/hid/hid-asus.c
> >>>>> @@ -90,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_ROG_NKEY_LEGACY BIT(14)
> >>>> These past days I have taken a look at new 2025 models and they do make use of ID2,
> >>>> and won't do harm sending ID1 either. I think you can safely remove the if and send regardless.
> >>> Hi Denis,
> >>> it is not the responsibility of this driver. ID2 is used by Anime
> >>> models. It is a concession to make sure that we do not cause a
> >>> regression that will cause warnings for a lot of users.
> >> Who decided it is a concession?
> > I would rather remove the extra calls unless they are shown to be
> > needed, which they might be for these PIDs.
> They are needed on older laptop and to not regress userspace.
>
> You just named _LEGACY an usb pid that is not legacy.
> > The quirk is named legacy because we can't retest these devices. If we
> > can, then we could remove the quirk and the inits if not needed.
> We can't retest every device, and that pid is used in pre-2021 models,
> and these are the unknown, I am criticizing the name of the quirk here,
> not what it does.
If you can test whether your device needs them that would be great.
> I am also questioning if the quirk is even needed since sending
> those commands to (at least) recent hardware that doesn't use
> those endpoints carries no downsides, while removing them
> surely does.
We have not found a device yet that needs them. I do not want to keep
sending unneeded commands. It could cause obscure bugs or interfere
with userspace software such as the one you maintain. So at least for
new hardware that is possible to test we should remove them.
Antheas
> > Antheas
> >
> >> Anyway I will move relevant code tied to these two to this driver,
> >> so it doesn't make sense to remove them anyway.
> >>>> At least 2023 models like mine that don't support ID2 will simply reply with 0xFF 0xFF and the rest 0x00.
> >>>> No consequences.
> >>> In your laptop. In the other user's laptop, the get feature report fails
> >> for the response to be a failure (as it is supposed to be in mine and other models)
> >> and to cause problems are two different things. Here I am saying that the hardware
> >> correctly reports "unsupported" and nothing bad happens (if you ignore the return value).
> >>>> Regardless the name is wrong: mine is a 2023 rog strix with
> >>>> ID 0b05:19b6ASUSTek Computer, Inc. N-KEY Device
> >>>> and surely isn't legacy.
> >>> Sure, can you try removing the if block?
> >> I have asked to distribute a kernel that init ID1 and ID2 regardless
> >> of that quirk. We will soon know if it causes problems or not.
> >>> If it works in your laptop, that is one less reason to keep it for 19b6
> >> If it works in my laptop one more reason not to exclude code that
> >> works and haven't caused any problem ever.
> >>> Antheas
> >>>
> >>>>> #define I2C_KEYBOARD_QUIRKS (QUIRK_FIX_NOTEBOOK_REPORT | \
> >>>>> QUIRK_NO_INIT_REPORTS | \
> >>>>> @@ -652,14 +653,9 @@ static int asus_kbd_register_leds(struct hid_device *hdev)
> >>>>> if (!(kbd_func & SUPPORT_KBD_BACKLIGHT))
> >>>>> return -ENODEV;
> >>>>>
> >>>>> - if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) {
> >>>>> - 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 (drvdata->quirks & QUIRK_ROG_NKEY_LEGACY) {
> >>>>> + asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID1);
> >>>>> + asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID2);
> >>>>> }
> >>>>>
> >>>>> if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) {
> >>>>> @@ -1376,10 +1372,10 @@ static const struct hid_device_id asus_devices[] = {
> >>>>> QUIRK_USE_KBD_BACKLIGHT },
> >>>>> { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
> >>>>> USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD),
> >>>>> - QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
> >>>>> + QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_NKEY_LEGACY },
> >>>>> { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
> >>>>> USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD2),
> >>>>> - QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
> >>>>> + QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_NKEY_LEGACY },
> >>>>> { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
> >>>>> USB_DEVICE_ID_ASUSTEK_ROG_Z13_LIGHTBAR),
> >>>>> QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
> >>> On Fri, 16 Jan 2026 at 21:44, Denis Benato <denis.benato@...ux.dev> wrote:
> >>>> On 1/16/26 14:31, Antheas Kapenekakis wrote:
> >>>>
> >>>>> Currently, ID1/ID2 initializations are performed for all NKEY devices.
> >>>>> However, ID1 initializations are only required for RGB control and are
> >>>>> only supported for RGB capable devices. ID2 initializations are only
> >>>>> required for initializing the Anime display endpoint which is only
> >>>>> supported on devices with an Anime display. Both of these
> >>>>> initializations are out of scope for this driver (this is a brightness
> >>>>> control and keyboard shortcut driver) and they should not be performed
> >>>>> for devices that do not support them in any case.
> >>>>>
> >>>>> At the same time, there are older NKEY devices that have only been
> >>>>> tested with these initializations in the kernel and it is not possible
> >>>>> to recheck them. There is a possibility that especially with the ID1
> >>>>> initialization, certain laptop models might have their shortcuts stop
> >>>>> working (currently unproven).
> >>>>>
> >>>>> For an abundance of caution, only initialize ID1/ID2 for those older
> >>>>> NKEY devices by introducing a quirk for them and replacing the NKEY
> >>>>> quirk in the block that performs the inits with that.
> >>>>>
> >>>>> In addition, as these initializations might not be supported by the
> >>>>> affected devices, change the function to not bail if they fail.
> >>>>>
> >>>>> Acked-by: Benjamin Tissoires <bentiss@...nel.org>
> >>>>> Signed-off-by: Antheas Kapenekakis <lkml@...heas.dev>
> >>>>> ---
> >>>>> drivers/hid/hid-asus.c | 16 ++++++----------
> >>>>> 1 file changed, 6 insertions(+), 10 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> >>>>> index 323e6302bac5..dc7af12cf31a 100644
> >>>>> --- a/drivers/hid/hid-asus.c
> >>>>> +++ b/drivers/hid/hid-asus.c
> >>>>> @@ -90,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_ROG_NKEY_LEGACY BIT(14)
> >>>> These past days I have taken a look at new 2025 models and they do make use of ID2,
> >>>> and won't do harm sending ID1 either. I think you can safely remove the if and send regardless.
> >>>>
> >>>> At least 2023 models like mine that don't support ID2 will simply reply with 0xFF 0xFF and the rest 0x00.
> >>>> No consequences.
> >>>>
> >>>> Regardless the name is wrong: mine is a 2023 rog strix with
> >>>> ID 0b05:19b6ASUSTek Computer, Inc. N-KEY Device
> >>>> and surely isn't legacy.
> >>>>> #define I2C_KEYBOARD_QUIRKS (QUIRK_FIX_NOTEBOOK_REPORT | \
> >>>>> QUIRK_NO_INIT_REPORTS | \
> >>>>> @@ -652,14 +653,9 @@ static int asus_kbd_register_leds(struct hid_device *hdev)
> >>>>> if (!(kbd_func & SUPPORT_KBD_BACKLIGHT))
> >>>>> return -ENODEV;
> >>>>>
> >>>>> - if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) {
> >>>>> - 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 (drvdata->quirks & QUIRK_ROG_NKEY_LEGACY) {
> >>>>> + asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID1);
> >>>>> + asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID2);
> >>>>> }
> >>>>>
> >>>>> if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) {
> >>>>> @@ -1376,10 +1372,10 @@ static const struct hid_device_id asus_devices[] = {
> >>>>> QUIRK_USE_KBD_BACKLIGHT },
> >>>>> { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
> >>>>> USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD),
> >>>>> - QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
> >>>>> + QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_NKEY_LEGACY },
> >>>>> { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
> >>>>> USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD2),
> >>>>> - QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
> >>>>> + QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_NKEY_LEGACY },
> >>>>> { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
> >>>>> USB_DEVICE_ID_ASUSTEK_ROG_Z13_LIGHTBAR),
> >>>>> QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
>
Powered by blists - more mailing lists