Re: [PATCH v3 01/10] HID: asus: refactor init sequence per spec

["Luke D. Jones" <luke@ljones.dev>]

On 23/03/25 12:53, Antheas Kapenekakis wrote:
> On Sun, 23 Mar 2025 at 00:29, Luke D. Jones <luke@ljones.dev> wrote:
>>
>> On 23/03/25 12:05, Antheas Kapenekakis wrote:
>>> On Sat, 22 Mar 2025 at 23:01, Luke D. Jones <luke@ljones.dev> wrote:
>>>>
>>>> On 22/03/25 23:27, 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 brightness 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.
>>>>>
>>>>> Then, refactor asus_kbd_get_functions() to use the same ID it is called
>>>>> with, instead of hardcoding it to 0x5a so that it may be used for 0x0d
>>>>> in the future.
>>>>>
>>>>> 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..8d4df1b6f143b 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
>>>>>
>>>>> @@ -388,14 +388,41 @@ 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)
>>>>>     {
>>>>> -     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.
>>>>> +      */
>>>>
>>>> This entire comment is not required, especially not the last paragraph.
>>>>    From what I've seen in .dll reversing attempts there's no real
>>>> distinction from driver/app and it's simply an init/enable sequence
>>>> common to almost every ITE MCU that ASUS have used (slash, anime, Ally).
>>>>
>>>> Please remove.
>>>
>>> It is a context comment but can be removed.
>>>
>>>>> +     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_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;
>>>>>     }
>>>>>
>>>>> @@ -417,7 +444,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) {
>>>>> @@ -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, FEATURE_KBD_REPORT_ID);
>>>>> +     if (ret < 0)
>>>>> +             return ret;
>>>>>
>>>>
>>>> I don't have any non-ROG equipment to test. There have been some cases
>>>> of Vivobook using the same MCU, but these otherwise used something else.
>>>> And the oldest hardware I have uses a 0x1866, which uses the same init
>>>> sequence but works with both 0x5A and 0x5D report ID for init (on same
>>>> endpoint). There are instances of other laptops that accept both 0x5A
>>>> and 0x5D, and some that have only 0x5D.
>>>
>>> The driver sets the brightness using 0x5a and accepts keyboard
>>> shortcuts only from 0x5a. Therefore it needs to init only with 0x5a.
>>>
>>> How would those laptops regress and in what way?
>>>
>>
>> The media keys fail to work (vol, mic, rog). Can you please accept that
>> I do know some laptops used only 0x5D, and these are older models,
>> around 5+ years. The only thing I have to go on is my memory
>> unfortunately, and I've been trying to find the concrete examples.
> 
> I just looked at the history. Yeah it seems you added ID1 in 2020 with
> some other commands. But on the same commit you blocked 0x5d and 0x5e,
> so it means those keyboards use 0x5a to send keyboard events.
> 
> Nevertheless, it is not worth looking up or risking regressions for
> old hardware. I will readd 0x5d, 0x5e for
> USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD and
> USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD2, since those are the ones you
> added the inits with.
> 

Thank you. Please know that I'm not trying to be a pain, I only don't 
want to take that risk when I know things were working for all before 
now, and that I know at least some laptops required 0x5D historically. I 
do know you can drop 0x5E here.

I saw we have:
#define INPUT_REPORT_ID 0x5d
#define FEATURE_KBD_LED_REPORT_ID1 0x5d

Maybe unifying to just one of these can be added as a cleanup?

Cheers,
Luke.

> But I will still keep them off for the Z13 and Ally.
> 
> By the way,
> 
> Antheas
> 
>>>> I think you will need to change this to try both 0x5A and 0x5D sorry.
>>>> The older models using 0x1854, 0x1869, 0x1866 PID may regress and
>>>> although I'm reasonably confident there won't be issues due to age of
>>>> those, it's not a risk I'm willing to take, I've spent all morning
>>>> trawling through archives of info and I can't actually verify this other
>>>> than from my memory.
>>>
>>> For devices that support RGB, only when RGB is set, 0x5D is initialized too.
>>
>> Sure. But as I've said above.. Please add both to init. It's only done
>> once, and it doesn't hurt anything plus doesn't risk regressing older
>> hardware.
>>
>> If I can get the proper evidence that only 0x5A is required I'm happy to
>> use only that, but until then I don't want that risk. And it's only a
>> small thing here.
>>
>> Cheers,
>> Luke.
>>
>>>
>>>> I mentioned 0x5E being used for some of the oddball devices like slash
>>>> and anime, don't worry about that one, it's a bridge that can be crossed
>>>> at a later time. But it does illustrate that other report ID have been
>>>> used for init.
>>>>
>>>> Otherwise the cleanup is good.
>>>>
>>>> No other comments required and I'll sign off with the above corrections.
>>>>
>>>> Cheers,
>>>> Luke
>>>>
>>>>> -             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);
>>>>
>>