| Message ID | 20250319191320.10092-2-lkml@antheas.dev (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | HID: asus: hid-asus and asus-wmi backlight unification, Z13 QOL improvements | expand |
On 20/03/25 08:13, Antheas Kapenekakis wrote: > Currently, asus_kbd_init() uses a reverse engineered init sequence > from Windows, which contains the handshakes from multiple programs. > Keep the main one, which is 0x5a (meant for drivers). 0x5A is also used for Ally setup commands, used from userspace in Windows. Only a nit but I don't think stating it's only for drivers is accurate but then again asus kind of blurs the line a bit. > In addition, perform a get_response and check if the response is the > same. To avoid regressions, print an error if the response does not > match instead of rejecting device. > > Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev> > --- > drivers/hid/hid-asus.c | 82 +++++++++++++++++++++++------------------- > 1 file changed, 46 insertions(+), 36 deletions(-) > > diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c > index 46e3e42f9eb5f..aa4a481dc4f27 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 > > @@ -386,16 +386,43 @@ static int asus_kbd_set_report(struct hid_device *hdev, const u8 *buf, size_t bu > return ret; > } > > -static int asus_kbd_init(struct hid_device *hdev, u8 report_id) > +static int asus_kbd_init(struct hid_device *hdev) > { > - const u8 buf[] = { report_id, 0x41, 0x53, 0x55, 0x53, 0x20, 0x54, > - 0x65, 0x63, 0x68, 0x2e, 0x49, 0x6e, 0x63, 0x2e, 0x00 }; > + /* > + * Asus handshake identifying us as a driver (0x5A) > + * 0x5A then ASCII for "ASUS Tech.Inc." > + * 0x5D is for userspace Windows applications. 0x5D is the report ID used for commands such as RGB modes. Probably don't need to mention it here, and only where it is used. > + * The handshake is first sent as a set_report, then retrieved > + * from a get_report to verify the response. > + */ > + const u8 buf[] = { FEATURE_KBD_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, FEATURE_KBD_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_err(hdev, "Asus handshake returned invalid response: %*ph\n", > + FEATURE_KBD_REPORT_SIZE, readbuf); > + // Do not return error if handshake is wrong to avoid regressions I'll have to test this on the oldest model I have. Hopefully it's a non-issue and this can return error instead. Side-note: I notice you're using a msleep to try and work around an issue in a later patch - it might be worth trying replacing that with a retry/count loop with an inner of small msleep + a call to this init, see if it still responds to this during that critical period. > + } > + > + kfree(readbuf); > return ret; > } > > @@ -540,42 +567,25 @@ 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; Ah, I recall now. Some devices like the Slash or AniMe Matrix required the 0x5E and 0x5D report ID (device dependent) however these are currently being done via userspace due to not being HID devices. There *are* some older laptops still in use that require init on 0x5E or 0x5D for RGB to be usable, from memory. It's been over 5 years so I'll pull out the laptop I have with 0x1866 PID MCU and see if that is actually true and not just my imagination. > + ret = asus_kbd_init(hdev); > + if (ret < 0) > + return ret; > > - if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) { > - ret = asus_kbd_disable_oobe(hdev); > - 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; > } > > + /* 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); I've left only small comments on a few patches for now. I'll review in full after I get testing done on a variety of devices whcih I'm aiming for this weekend. Overall impression so far is everything looks good and this is a nice improvement. Thank you for taking the time to implement it. Cheers, Luke.
On Thu, 20 Mar 2025 at 08:19, Luke D. Jones <luke@ljones.dev> wrote: > > > On 20/03/25 08:13, Antheas Kapenekakis wrote: > > Currently, asus_kbd_init() uses a reverse engineered init sequence > > from Windows, which contains the handshakes from multiple programs. > > Keep the main one, which is 0x5a (meant for drivers). > > 0x5A is also used for Ally setup commands, used from userspace in > Windows. Only a nit but I don't think stating it's only for drivers is > accurate but then again asus kind of blurs the line a bit. ROG devices contain a HID USB endpoint that exposes multiple applications. On my Z13, that is 4 hiddev devices. However, we only care about two. Those are: Application / Report ID: 0xff310076 / 0x5a meant for Asus drivers 0xff310079 / 0x5d meant for Asus applications Both require the same handshake when they start. Well, in theory. But as you say in some of the Anime stuff requires it in practice too. The handshake is set_report 0x5X + "Asus...", then get_report with the same ID which should return the asus string. In hiddraw, they appear under the same endpoint, both on the Ally and the Z13. But in hiddev (with hid-asus disabled or with this series), they appear as separate. I cannot comment on the Aura protocol, because I don't know, but for the basic sticky RGB mode that supports set and apply, they _should_ behave identically. I use 0x5d in my userspace software for everything now [1]. Previously, I used 0x5a but I am not a driver. They do behave identically on the Ally X and the Z13 2025 though. I do not know about 0x5e. Perhaps Asus made a special endpoint for their Anime creation app. > > In addition, perform a get_response and check if the response is the > > same. To avoid regressions, print an error if the response does not > > match instead of rejecting device. > > > > Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev> > > --- > > drivers/hid/hid-asus.c | 82 +++++++++++++++++++++++------------------- > > 1 file changed, 46 insertions(+), 36 deletions(-) > > > > diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c > > index 46e3e42f9eb5f..aa4a481dc4f27 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 > > > > @@ -386,16 +386,43 @@ static int asus_kbd_set_report(struct hid_device *hdev, const u8 *buf, size_t bu > > return ret; > > } > > > > -static int asus_kbd_init(struct hid_device *hdev, u8 report_id) > > +static int asus_kbd_init(struct hid_device *hdev) > > { > > - const u8 buf[] = { report_id, 0x41, 0x53, 0x55, 0x53, 0x20, 0x54, > > - 0x65, 0x63, 0x68, 0x2e, 0x49, 0x6e, 0x63, 0x2e, 0x00 }; > > + /* > > + * Asus handshake identifying us as a driver (0x5A) > > + * 0x5A then ASCII for "ASUS Tech.Inc." > > + * 0x5D is for userspace Windows applications. > > 0x5D is the report ID used for commands such as RGB modes. Probably > don't need to mention it here, and only where it is used. Yep, see above. Not required for basic RGB. Maybe it is for Aura, but I'd leave that to userspace. > > + * The handshake is first sent as a set_report, then retrieved > > + * from a get_report to verify the response. > > + */ > > + const u8 buf[] = { FEATURE_KBD_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, FEATURE_KBD_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_err(hdev, "Asus handshake returned invalid response: %*ph\n", > > + FEATURE_KBD_REPORT_SIZE, readbuf); > > + // Do not return error if handshake is wrong to avoid regressions > > I'll have to test this on the oldest model I have. Hopefully it's a > non-issue and this can return error instead. > > Side-note: I notice you're using a msleep to try and work around an > issue in a later patch - it might be worth trying replacing that with a > retry/count loop with an inner of small msleep + a call to this init, > see if it still responds to this during that critical period. The call did not fail. I was thinking it was because the device needs some time to warm up (it happens with certain devices). Turns out it was hid-multitouch not attaching. > > + } > > + > > + kfree(readbuf); > > return ret; > > } > > > > @@ -540,42 +567,25 @@ 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; > > Ah, I recall now. Some devices like the Slash or AniMe Matrix required > the 0x5E and 0x5D report ID (device dependent) however these are > currently being done via userspace due to not being HID devices. > > There *are* some older laptops still in use that require init on 0x5E or > 0x5D for RGB to be usable, from memory. It's been over 5 years so I'll > pull out the laptop I have with 0x1866 PID MCU and see if that is > actually true and not just my imagination. Hopefully you handshake with these devices over userspace, so they will not be affected. > > + ret = asus_kbd_init(hdev); > > + if (ret < 0) > > + return ret; > > > > - if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) { > > - ret = asus_kbd_disable_oobe(hdev); > > - 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; > > } > > > > + /* 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); > > I've left only small comments on a few patches for now. I'll review in > full after I get testing done on a variety of devices whcih I'm aiming > for this weekend. Overall impression so far is everything looks good and > this is a nice improvement. Thank you for taking the time to implement it. > > Cheers, > Luke. I'll try to have V2 out today. I finished it yesterday and fixed all the lockups and the hid-multitouch issue. Just needs a good lookthrough. Perhaps I will also do a small multi-intensity endpoint that works with KDE and only applies the colors when asked. This way our programs are not affected and normal laptop users get basic RGB OOTB. If I do that, I will make the quirk for the Ally in a separate patch, so that you can nack it if you'd rather introduce RGB support with your driver, so that it does not need to be reverted. Antheas [1] https://github.com/hhd-dev/hhd/blob/d3bbd7fa25fe9a4838896a2c5cfda460abe48dc6/src/hhd/device/rog_ally/const.py#L5-L8
On Thu, 20 Mar 2025 at 10:50, Antheas Kapenekakis <lkml@antheas.dev> wrote: > > On Thu, 20 Mar 2025 at 08:19, Luke D. Jones <luke@ljones.dev> wrote: > > > > > > On 20/03/25 08:13, Antheas Kapenekakis wrote: > > > Currently, asus_kbd_init() uses a reverse engineered init sequence > > > from Windows, which contains the handshakes from multiple programs. > > > Keep the main one, which is 0x5a (meant for drivers). > > > > 0x5A is also used for Ally setup commands, used from userspace in > > Windows. Only a nit but I don't think stating it's only for drivers is > > accurate but then again asus kind of blurs the line a bit. > > ROG devices contain a HID USB endpoint that exposes multiple > applications. On my Z13, that is 4 hiddev devices. > > However, we only care about two. Those are: > > Application / Report ID: > 0xff310076 / 0x5a meant for Asus drivers > 0xff310079 / 0x5d meant for Asus applications > > Both require the same handshake when they start. Well, in theory. But > as you say in some of the Anime stuff requires it in practice too. > > The handshake is set_report 0x5X + "Asus...", then get_report with the > same ID which should return the asus string. I might have misread here and only 0x5d can be used for RGB. Actually I probably misread. Let me get back to you on that. But brightness is both 0x5d and 0x5a for sure. If I do RGB I will defer initializing 0x5d until userspace tries to write the color. Antheas > In hiddraw, they appear under the same endpoint, both on the Ally and > the Z13. But in hiddev (with hid-asus disabled or with this series), > they appear as separate. > > I cannot comment on the Aura protocol, because I don't know, but for > the basic sticky RGB mode that supports set and apply, they _should_ > behave identically. I use 0x5d in my userspace software for everything > now [1]. Previously, I used 0x5a but I am not a driver. > > They do behave identically on the Ally X and the Z13 2025 though. > > I do not know about 0x5e. Perhaps Asus made a special endpoint for > their Anime creation app. > > > > In addition, perform a get_response and check if the response is the > > > same. To avoid regressions, print an error if the response does not > > > match instead of rejecting device. > > > > > > Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev> > > > --- > > > drivers/hid/hid-asus.c | 82 +++++++++++++++++++++++------------------- > > > 1 file changed, 46 insertions(+), 36 deletions(-) > > > > > > diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c > > > index 46e3e42f9eb5f..aa4a481dc4f27 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 > > > > > > @@ -386,16 +386,43 @@ static int asus_kbd_set_report(struct hid_device *hdev, const u8 *buf, size_t bu > > > return ret; > > > } > > > > > > -static int asus_kbd_init(struct hid_device *hdev, u8 report_id) > > > +static int asus_kbd_init(struct hid_device *hdev) > > > { > > > - const u8 buf[] = { report_id, 0x41, 0x53, 0x55, 0x53, 0x20, 0x54, > > > - 0x65, 0x63, 0x68, 0x2e, 0x49, 0x6e, 0x63, 0x2e, 0x00 }; > > > + /* > > > + * Asus handshake identifying us as a driver (0x5A) > > > + * 0x5A then ASCII for "ASUS Tech.Inc." > > > + * 0x5D is for userspace Windows applications. > > > > 0x5D is the report ID used for commands such as RGB modes. Probably > > don't need to mention it here, and only where it is used. > > Yep, see above. Not required for basic RGB. Maybe it is for Aura, but > I'd leave that to userspace. > > > > + * The handshake is first sent as a set_report, then retrieved > > > + * from a get_report to verify the response. > > > + */ > > > + const u8 buf[] = { FEATURE_KBD_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, FEATURE_KBD_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_err(hdev, "Asus handshake returned invalid response: %*ph\n", > > > + FEATURE_KBD_REPORT_SIZE, readbuf); > > > + // Do not return error if handshake is wrong to avoid regressions > > > > I'll have to test this on the oldest model I have. Hopefully it's a > > non-issue and this can return error instead. > > > > Side-note: I notice you're using a msleep to try and work around an > > issue in a later patch - it might be worth trying replacing that with a > > retry/count loop with an inner of small msleep + a call to this init, > > see if it still responds to this during that critical period. > > The call did not fail. I was thinking it was because the device needs > some time to warm up (it happens with certain devices). > > Turns out it was hid-multitouch not attaching. > > > > + } > > > + > > > + kfree(readbuf); > > > return ret; > > > } > > > > > > @@ -540,42 +567,25 @@ 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; > > > > Ah, I recall now. Some devices like the Slash or AniMe Matrix required > > the 0x5E and 0x5D report ID (device dependent) however these are > > currently being done via userspace due to not being HID devices. > > > > There *are* some older laptops still in use that require init on 0x5E or > > 0x5D for RGB to be usable, from memory. It's been over 5 years so I'll > > pull out the laptop I have with 0x1866 PID MCU and see if that is > > actually true and not just my imagination. > > Hopefully you handshake with these devices over userspace, so they > will not be affected. > > > > + ret = asus_kbd_init(hdev); > > > + if (ret < 0) > > > + return ret; > > > > > > - if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) { > > > - ret = asus_kbd_disable_oobe(hdev); > > > - 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; > > > } > > > > > > + /* 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); > > > > I've left only small comments on a few patches for now. I'll review in > > full after I get testing done on a variety of devices whcih I'm aiming > > for this weekend. Overall impression so far is everything looks good and > > this is a nice improvement. Thank you for taking the time to implement it. > > > > Cheers, > > Luke. > > I'll try to have V2 out today. I finished it yesterday and fixed all > the lockups and the hid-multitouch issue. Just needs a good > lookthrough. > > Perhaps I will also do a small multi-intensity endpoint that works > with KDE and only applies the colors when asked. This way our programs > are not affected and normal laptop users get basic RGB OOTB. > > If I do that, I will make the quirk for the Ally in a separate patch, > so that you can nack it if you'd rather introduce RGB support with > your driver, so that it does not need to be reverted. > > Antheas > > [1] https://github.com/hhd-dev/hhd/blob/d3bbd7fa25fe9a4838896a2c5cfda460abe48dc6/src/hhd/device/rog_ally/const.py#L5-L8
On 20/03/25 22:50, Antheas Kapenekakis wrote: > On Thu, 20 Mar 2025 at 08:19, Luke D. Jones <luke@ljones.dev> wrote: >> >> >> On 20/03/25 08:13, Antheas Kapenekakis wrote: >>> Currently, asus_kbd_init() uses a reverse engineered init sequence >>> from Windows, which contains the handshakes from multiple programs. >>> Keep the main one, which is 0x5a (meant for drivers). >> >> 0x5A is also used for Ally setup commands, used from userspace in >> Windows. Only a nit but I don't think stating it's only for drivers is >> accurate but then again asus kind of blurs the line a bit. > > ROG devices contain a HID USB endpoint that exposes multiple > applications. On my Z13, that is 4 hiddev devices. > > However, we only care about two. Those are: > > Application / Report ID: > 0xff310076 / 0x5a meant for Asus drivers > 0xff310079 / 0x5d meant for Asus applications I'm curious how you determined that. From what I've seen asus are consistent until they're not - typically it's when they have a weird device like the Slash or Anime. The slash on the G16 uses the same MCU and endpoint as the keyboard, but with a new report ID, while the slash on a G14 is a separate MCU (or at least device exposed by MCU) with own endpoints and hid report. It's not important here yet, I'm just trying to add context and provide some extra information for you. > Both require the same handshake when they start. Well, in theory. But > as you say in some of the Anime stuff requires it in practice too. Yeah as above, the G14 slash needs it, so does the older Anime dispaly. But the G16 slash being on the same MCU doesn't. In any case those gadgets are being handled in userspace just fine regardless of the driver state. Still, not really relevant here and I'm just adding context. > The handshake is set_report 0x5X + "Asus...", then get_report with the > same ID which should return the asus string. > > In hiddraw, they appear under the same endpoint, both on the Ally and > the Z13. But in hiddev (with hid-asus disabled or with this series), > they appear as separate. > > I cannot comment on the Aura protocol, because I don't know, but for > the basic sticky RGB mode that supports set and apply, they _should_ > behave identically. I use 0x5d in my userspace software for everything > now [1]. Previously, I used 0x5a but I am not a driver. > > They do behave identically on the Ally X and the Z13 2025 though. It is something I do have to test, and I'll add your v2 to kernels to get some feedback from my discord group too. I know for sure that there were some laptops that didn't accept 0x5D for brightness - I'm sorry I'm so vague on this part, I really don't remember the details and I lost notes so it will need to be tested. > I do not know about 0x5e. Perhaps Asus made a special endpoint for > their Anime creation app. It is only for that yes. Cheers, Luke. >>> In addition, perform a get_response and check if the response is the >>> same. To avoid regressions, print an error if the response does not >>> match instead of rejecting device. >>> >>> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev> >>> --- >>> drivers/hid/hid-asus.c | 82 +++++++++++++++++++++++------------------- >>> 1 file changed, 46 insertions(+), 36 deletions(-) >>> >>> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c >>> index 46e3e42f9eb5f..aa4a481dc4f27 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 >>> >>> @@ -386,16 +386,43 @@ static int asus_kbd_set_report(struct hid_device *hdev, const u8 *buf, size_t bu >>> return ret; >>> } >>> >>> -static int asus_kbd_init(struct hid_device *hdev, u8 report_id) >>> +static int asus_kbd_init(struct hid_device *hdev) >>> { >>> - const u8 buf[] = { report_id, 0x41, 0x53, 0x55, 0x53, 0x20, 0x54, >>> - 0x65, 0x63, 0x68, 0x2e, 0x49, 0x6e, 0x63, 0x2e, 0x00 }; >>> + /* >>> + * Asus handshake identifying us as a driver (0x5A) >>> + * 0x5A then ASCII for "ASUS Tech.Inc." >>> + * 0x5D is for userspace Windows applications. >> >> 0x5D is the report ID used for commands such as RGB modes. Probably >> don't need to mention it here, and only where it is used. > > Yep, see above. Not required for basic RGB. Maybe it is for Aura, but > I'd leave that to userspace. > >>> + * The handshake is first sent as a set_report, then retrieved >>> + * from a get_report to verify the response. >>> + */ >>> + const u8 buf[] = { FEATURE_KBD_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, FEATURE_KBD_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_err(hdev, "Asus handshake returned invalid response: %*ph\n", >>> + FEATURE_KBD_REPORT_SIZE, readbuf); >>> + // Do not return error if handshake is wrong to avoid regressions >> >> I'll have to test this on the oldest model I have. Hopefully it's a >> non-issue and this can return error instead. >> >> Side-note: I notice you're using a msleep to try and work around an >> issue in a later patch - it might be worth trying replacing that with a >> retry/count loop with an inner of small msleep + a call to this init, >> see if it still responds to this during that critical period. > > The call did not fail. I was thinking it was because the device needs > some time to warm up (it happens with certain devices). > > Turns out it was hid-multitouch not attaching. > >>> + } >>> + >>> + kfree(readbuf); >>> return ret; >>> } >>> >>> @@ -540,42 +567,25 @@ 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; >> >> Ah, I recall now. Some devices like the Slash or AniMe Matrix required >> the 0x5E and 0x5D report ID (device dependent) however these are >> currently being done via userspace due to not being HID devices. >> >> There *are* some older laptops still in use that require init on 0x5E or >> 0x5D for RGB to be usable, from memory. It's been over 5 years so I'll >> pull out the laptop I have with 0x1866 PID MCU and see if that is >> actually true and not just my imagination. > > Hopefully you handshake with these devices over userspace, so they > will not be affected. > >>> + ret = asus_kbd_init(hdev); >>> + if (ret < 0) >>> + return ret; >>> >>> - if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) { >>> - ret = asus_kbd_disable_oobe(hdev); >>> - 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; >>> } >>> >>> + /* 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); >> >> I've left only small comments on a few patches for now. I'll review in >> full after I get testing done on a variety of devices whcih I'm aiming >> for this weekend. Overall impression so far is everything looks good and >> this is a nice improvement. Thank you for taking the time to implement it. >> >> Cheers, >> Luke. > > I'll try to have V2 out today. I finished it yesterday and fixed all > the lockups and the hid-multitouch issue. Just needs a good > lookthrough. > > Perhaps I will also do a small multi-intensity endpoint that works > with KDE and only applies the colors when asked. This way our programs > are not affected and normal laptop users get basic RGB OOTB. > > If I do that, I will make the quirk for the Ally in a separate patch, > so that you can nack it if you'd rather introduce RGB support with > your driver, so that it does not need to be reverted. > > Antheas > > [1] https://github.com/hhd-dev/hhd/blob/d3bbd7fa25fe9a4838896a2c5cfda460abe48dc6/src/hhd/device/rog_ally/const.py#L5-L8
On Thu, 20 Mar 2025 at 22:01, Luke D. Jones <luke@ljones.dev> wrote: > > On 20/03/25 22:50, Antheas Kapenekakis wrote: > > On Thu, 20 Mar 2025 at 08:19, Luke D. Jones <luke@ljones.dev> wrote: > >> > >> > >> On 20/03/25 08:13, Antheas Kapenekakis wrote: > >>> Currently, asus_kbd_init() uses a reverse engineered init sequence > >>> from Windows, which contains the handshakes from multiple programs. > >>> Keep the main one, which is 0x5a (meant for drivers). > >> > >> 0x5A is also used for Ally setup commands, used from userspace in > >> Windows. Only a nit but I don't think stating it's only for drivers is > >> accurate but then again asus kind of blurs the line a bit. > > > > ROG devices contain a HID USB endpoint that exposes multiple > > applications. On my Z13, that is 4 hiddev devices. > > > > However, we only care about two. Those are: > > > > Application / Report ID: > > 0xff310076 / 0x5a meant for Asus drivers > > 0xff310079 / 0x5d meant for Asus applications > > I'm curious how you determined that. From what I've seen asus are > consistent until they're not - typically it's when they have a weird > device like the Slash or Anime. The slash on the G16 uses the same MCU > and endpoint as the keyboard, but with a new report ID, while the slash > on a G14 is a separate MCU (or at least device exposed by MCU) with own > endpoints and hid report. > > It's not important here yet, I'm just trying to add context and provide > some extra information for you. > > > Both require the same handshake when they start. Well, in theory. But > > as you say in some of the Anime stuff requires it in practice too. > > Yeah as above, the G14 slash needs it, so does the older Anime dispaly. > But the G16 slash being on the same MCU doesn't. In any case those > gadgets are being handled in userspace just fine regardless of the > driver state. Still, not really relevant here and I'm just adding context. > > > The handshake is set_report 0x5X + "Asus...", then get_report with the > > same ID which should return the asus string. > > > > In hiddraw, they appear under the same endpoint, both on the Ally and > > the Z13. But in hiddev (with hid-asus disabled or with this series), > > they appear as separate. > > > > I cannot comment on the Aura protocol, because I don't know, but for > > the basic sticky RGB mode that supports set and apply, they _should_ > > behave identically. I use 0x5d in my userspace software for everything > > now [1]. Previously, I used 0x5a but I am not a driver. > > > > They do behave identically on the Ally X and the Z13 2025 though. > > It is something I do have to test, and I'll add your v2 to kernels to > get some feedback from my discord group too. I know for sure that there > were some laptops that didn't accept 0x5D for brightness - I'm sorry I'm > so vague on this part, I really don't remember the details and I lost > notes so it will need to be tested. Well, different ODMs different years stuff changes, theory is theory. I looked it up and I still use 0x5a for the Ally for RGB. I am almost done with an initial version of the RGB patch. KDE seems to not be cooperating and setting the brightness to 0 though. I would not say it is ready to consume yet. The rest of the series as a V2 should work well though. Should post it in around an hour. Antheas > > I do not know about 0x5e. Perhaps Asus made a special endpoint for > > their Anime creation app. > > It is only for that yes. > > Cheers, > Luke. > > >>> In addition, perform a get_response and check if the response is the > >>> same. To avoid regressions, print an error if the response does not > >>> match instead of rejecting device. > >>> > >>> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev> > >>> --- > >>> drivers/hid/hid-asus.c | 82 +++++++++++++++++++++++------------------- > >>> 1 file changed, 46 insertions(+), 36 deletions(-) > >>> > >>> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c > >>> index 46e3e42f9eb5f..aa4a481dc4f27 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 > >>> > >>> @@ -386,16 +386,43 @@ static int asus_kbd_set_report(struct hid_device *hdev, const u8 *buf, size_t bu > >>> return ret; > >>> } > >>> > >>> -static int asus_kbd_init(struct hid_device *hdev, u8 report_id) > >>> +static int asus_kbd_init(struct hid_device *hdev) > >>> { > >>> - const u8 buf[] = { report_id, 0x41, 0x53, 0x55, 0x53, 0x20, 0x54, > >>> - 0x65, 0x63, 0x68, 0x2e, 0x49, 0x6e, 0x63, 0x2e, 0x00 }; > >>> + /* > >>> + * Asus handshake identifying us as a driver (0x5A) > >>> + * 0x5A then ASCII for "ASUS Tech.Inc." > >>> + * 0x5D is for userspace Windows applications. > >> > >> 0x5D is the report ID used for commands such as RGB modes. Probably > >> don't need to mention it here, and only where it is used. > > > > Yep, see above. Not required for basic RGB. Maybe it is for Aura, but > > I'd leave that to userspace. > > > >>> + * The handshake is first sent as a set_report, then retrieved > >>> + * from a get_report to verify the response. > >>> + */ > >>> + const u8 buf[] = { FEATURE_KBD_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, FEATURE_KBD_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_err(hdev, "Asus handshake returned invalid response: %*ph\n", > >>> + FEATURE_KBD_REPORT_SIZE, readbuf); > >>> + // Do not return error if handshake is wrong to avoid regressions > >> > >> I'll have to test this on the oldest model I have. Hopefully it's a > >> non-issue and this can return error instead. > >> > >> Side-note: I notice you're using a msleep to try and work around an > >> issue in a later patch - it might be worth trying replacing that with a > >> retry/count loop with an inner of small msleep + a call to this init, > >> see if it still responds to this during that critical period. > > > > The call did not fail. I was thinking it was because the device needs > > some time to warm up (it happens with certain devices). > > > > Turns out it was hid-multitouch not attaching. > > > >>> + } > >>> + > >>> + kfree(readbuf); > >>> return ret; > >>> } > >>> > >>> @@ -540,42 +567,25 @@ 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; > >> > >> Ah, I recall now. Some devices like the Slash or AniMe Matrix required > >> the 0x5E and 0x5D report ID (device dependent) however these are > >> currently being done via userspace due to not being HID devices. > >> > >> There *are* some older laptops still in use that require init on 0x5E or > >> 0x5D for RGB to be usable, from memory. It's been over 5 years so I'll > >> pull out the laptop I have with 0x1866 PID MCU and see if that is > >> actually true and not just my imagination. > > > > Hopefully you handshake with these devices over userspace, so they > > will not be affected. > > > >>> + ret = asus_kbd_init(hdev); > >>> + if (ret < 0) > >>> + return ret; > >>> > >>> - if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) { > >>> - ret = asus_kbd_disable_oobe(hdev); > >>> - 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; > >>> } > >>> > >>> + /* 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); > >> > >> I've left only small comments on a few patches for now. I'll review in > >> full after I get testing done on a variety of devices whcih I'm aiming > >> for this weekend. Overall impression so far is everything looks good and > >> this is a nice improvement. Thank you for taking the time to implement it. > >> > >> Cheers, > >> Luke. > > > > I'll try to have V2 out today. I finished it yesterday and fixed all > > the lockups and the hid-multitouch issue. Just needs a good > > lookthrough. > > > > Perhaps I will also do a small multi-intensity endpoint that works > > with KDE and only applies the colors when asked. This way our programs > > are not affected and normal laptop users get basic RGB OOTB. > > > > If I do that, I will make the quirk for the Ally in a separate patch, > > so that you can nack it if you'd rather introduce RGB support with > > your driver, so that it does not need to be reverted. > > > > Antheas > > > > [1] https://github.com/hhd-dev/hhd/blob/d3bbd7fa25fe9a4838896a2c5cfda460abe48dc6/src/hhd/device/rog_ally/const.py#L5-L8 >
diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c index 46e3e42f9eb5f..aa4a481dc4f27 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 @@ -386,16 +386,43 @@ static int asus_kbd_set_report(struct hid_device *hdev, const u8 *buf, size_t bu return ret; } -static int asus_kbd_init(struct hid_device *hdev, u8 report_id) +static int asus_kbd_init(struct hid_device *hdev) { - const u8 buf[] = { report_id, 0x41, 0x53, 0x55, 0x53, 0x20, 0x54, - 0x65, 0x63, 0x68, 0x2e, 0x49, 0x6e, 0x63, 0x2e, 0x00 }; + /* + * Asus handshake identifying us as a driver (0x5A) + * 0x5A then ASCII for "ASUS Tech.Inc." + * 0x5D is for userspace Windows applications. + * + * The handshake is first sent as a set_report, then retrieved + * from a get_report to verify the response. + */ + const u8 buf[] = { FEATURE_KBD_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, FEATURE_KBD_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_err(hdev, "Asus handshake returned invalid response: %*ph\n", + FEATURE_KBD_REPORT_SIZE, readbuf); + // Do not return error if handshake is wrong to avoid regressions + } + + kfree(readbuf); return ret; } @@ -540,42 +567,25 @@ 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; + ret = asus_kbd_init(hdev); + if (ret < 0) + return ret; - if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) { - ret = asus_kbd_disable_oobe(hdev); - 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; } + /* 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);
Currently, asus_kbd_init() uses a reverse engineered init sequence from Windows, which contains the handshakes from multiple programs. Keep the main one, which is 0x5a (meant for drivers). In addition, perform a get_response and check if the response is the same. To avoid regressions, print an error if the response does not match instead of rejecting device. Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev> --- drivers/hid/hid-asus.c | 82 +++++++++++++++++++++++------------------- 1 file changed, 46 insertions(+), 36 deletions(-)