From: "Luke D. Jones" <luke@ljones.dev>
ASUS have fixed suspend issues arising from a flag not being cleared in
the MCU FW in both the ROG Ally 1 and the ROG Ally X.
Implement a check and a warning to encourage users to update the FW to
a minimum supported version.
Signed-off-by: Luke D. Jones <luke@ljones.dev>
---
drivers/hid/hid-asus.c | 97 +++++++++++++++++++++++++++++++++++++++++-
1 file changed, 95 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index 46e3e42f9eb5..e1e60b80115a 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -52,6 +52,10 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
#define FEATURE_KBD_LED_REPORT_ID1 0x5d
#define FEATURE_KBD_LED_REPORT_ID2 0x5e
+#define ROG_ALLY_REPORT_SIZE 64
+#define ROG_ALLY_X_MIN_MCU 313
+#define ROG_ALLY_MIN_MCU 319
+
#define SUPPORT_KBD_BACKLIGHT BIT(0)
#define MAX_TOUCH_MAJOR 8
@@ -84,6 +88,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
#define QUIRK_MEDION_E1239T BIT(10)
#define QUIRK_ROG_NKEY_KEYBOARD BIT(11)
#define QUIRK_ROG_CLAYMORE_II_KEYBOARD BIT(12)
+#define QUIRK_ROG_ALLY_XPAD BIT(13)
#define I2C_KEYBOARD_QUIRKS (QUIRK_FIX_NOTEBOOK_REPORT | \
QUIRK_NO_INIT_REPORTS | \
@@ -534,9 +539,89 @@ static bool asus_kbd_wmi_led_control_present(struct hid_device *hdev)
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
+ */
+static int mcu_parse_version_string(const u8 *response, size_t response_size)
+{
+ const u8 *end = response + response_size;
+ const u8 *p = response;
+ int dots, err;
+ long version;
+
+ dots = 0;
+ while (p < end && dots < 2) {
+ if (*p++ == '.')
+ dots++;
+ }
+
+ if (dots != 2 || p >= end)
+ return -EINVAL;
+
+ err = kstrtol((const char *)p, 10, &version);
+ if (err || version < 0)
+ return -EINVAL;
+
+ return version;
+}
+
+static int mcu_request_version(struct hid_device *hdev)
+{
+ const u8 request[] = { 0x5a, 0x05, 0x03, 0x31, 0x00, 0x20 };
+ u8 *response;
+ int ret;
+
+ response = kzalloc(ROG_ALLY_REPORT_SIZE, GFP_KERNEL);
+ if (!response)
+ return -ENOMEM;
+
+ ret = asus_kbd_set_report(hdev, request, sizeof(request));
+ if (ret < 0)
+ goto out;
+
+ ret = hid_hw_raw_request(hdev, FEATURE_REPORT_ID, response,
+ ROG_ALLY_REPORT_SIZE, HID_FEATURE_REPORT,
+ HID_REQ_GET_REPORT);
+ if (ret < 0)
+ goto out;
+
+ ret = mcu_parse_version_string(response, ROG_ALLY_REPORT_SIZE);
+
+out:
+ if (ret < 0)
+ hid_err(hdev, "Failed to get MCU version: %d, response: %*ph\n",
+ ret, ROG_ALLY_REPORT_SIZE, response);
+ kfree(response);
+ return ret;
+}
+
+static void validate_mcu_fw_version(struct hid_device *hdev, int idProduct)
+{
+ int min_version = ROG_ALLY_X_MIN_MCU;
+ int version;
+
+ version = mcu_request_version(hdev);
+ if (version < 0)
+ return;
+
+ if (idProduct == USB_DEVICE_ID_ASUSTEK_ROG_NKEY_ALLY)
+ min_version = ROG_ALLY_MIN_MCU;
+
+ hid_info(hdev, "Ally device MCU version: %d\n", version);
+ if (version < min_version) {
+ hid_warn(hdev,
+ "The MCU firmware version must be %d or greater\n"
+ "Please update your MCU with official ASUS firmware release\n",
+ min_version);
+ }
+}
+
static int asus_kbd_register_leds(struct hid_device *hdev)
{
struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
+ struct usb_interface *intf;
+ struct usb_device *udev;
unsigned char kbd_func;
int ret;
@@ -560,6 +645,14 @@ static int asus_kbd_register_leds(struct hid_device *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));
+ }
+
} else {
/* Initialize keyboard */
ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID);
@@ -1280,10 +1373,10 @@ static const struct hid_device_id asus_devices[] = {
QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
{ HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
USB_DEVICE_ID_ASUSTEK_ROG_NKEY_ALLY),
- QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
+ QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_ALLY_XPAD},
{ HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
USB_DEVICE_ID_ASUSTEK_ROG_NKEY_ALLY_X),
- QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
+ QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_ALLY_XPAD },
{ HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
USB_DEVICE_ID_ASUSTEK_ROG_CLAYMORE_II_KEYBOARD),
QUIRK_ROG_CLAYMORE_II_KEYBOARD },
--
2.48.1On 2/25/2025 00:17, Luke Jones wrote:
> From: "Luke D. Jones" <luke@ljones.dev>
>
> ASUS have fixed suspend issues arising from a flag not being cleared in
> the MCU FW in both the ROG Ally 1 and the ROG Ally X.
>
> Implement a check and a warning to encourage users to update the FW to
> a minimum supported version.
>
> Signed-off-by: Luke D. Jones <luke@ljones.dev>
> ---
> drivers/hid/hid-asus.c | 97 +++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 95 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 46e3e42f9eb5..e1e60b80115a 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
> @@ -52,6 +52,10 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
> #define FEATURE_KBD_LED_REPORT_ID1 0x5d
> #define FEATURE_KBD_LED_REPORT_ID2 0x5e
>
> +#define ROG_ALLY_REPORT_SIZE 64
> +#define ROG_ALLY_X_MIN_MCU 313
> +#define ROG_ALLY_MIN_MCU 319
> +
> #define SUPPORT_KBD_BACKLIGHT BIT(0)
>
> #define MAX_TOUCH_MAJOR 8
> @@ -84,6 +88,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
> #define QUIRK_MEDION_E1239T BIT(10)
> #define QUIRK_ROG_NKEY_KEYBOARD BIT(11)
> #define QUIRK_ROG_CLAYMORE_II_KEYBOARD BIT(12)
> +#define QUIRK_ROG_ALLY_XPAD BIT(13)
>
> #define I2C_KEYBOARD_QUIRKS (QUIRK_FIX_NOTEBOOK_REPORT | \
> QUIRK_NO_INIT_REPORTS | \
> @@ -534,9 +539,89 @@ static bool asus_kbd_wmi_led_control_present(struct hid_device *hdev)
> 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
> + */
> +static int mcu_parse_version_string(const u8 *response, size_t response_size)
> +{
> + const u8 *end = response + response_size;
> + const u8 *p = response;
> + int dots, err;
> + long version;
> +
> + dots = 0;
> + while (p < end && dots < 2) {
> + if (*p++ == '.')
> + dots++;
> + }
> +
I think it would be good to use strsep() here.
> + if (dots != 2 || p >= end)
> + return -EINVAL;
> +
> + err = kstrtol((const char *)p, 10, &version);
> + if (err || version < 0)
> + return -EINVAL;
> +
> + return version;
> +}
> +
> +static int mcu_request_version(struct hid_device *hdev)
> +{
> + const u8 request[] = { 0x5a, 0x05, 0x03, 0x31, 0x00, 0x20 };
> + u8 *response;
> + int ret;
> +
> + response = kzalloc(ROG_ALLY_REPORT_SIZE, GFP_KERNEL);
> + if (!response)
> + return -ENOMEM;
> +
> + ret = asus_kbd_set_report(hdev, request, sizeof(request));
> + if (ret < 0)
> + goto out;
> +
> + ret = hid_hw_raw_request(hdev, FEATURE_REPORT_ID, response,
> + ROG_ALLY_REPORT_SIZE, HID_FEATURE_REPORT,
> + HID_REQ_GET_REPORT);
> + if (ret < 0)
> + goto out;
> +
> + ret = mcu_parse_version_string(response, ROG_ALLY_REPORT_SIZE);
> +
> +out:
> + if (ret < 0)
> + hid_err(hdev, "Failed to get MCU version: %d, response: %*ph\n",
> + ret, ROG_ALLY_REPORT_SIZE, response);
> + kfree(response);
> + return ret;
> +}
> +
> +static void validate_mcu_fw_version(struct hid_device *hdev, int idProduct)
> +{
> + int min_version = ROG_ALLY_X_MIN_MCU;
> + int version;
> +
> + version = mcu_request_version(hdev);
> + if (version < 0)
> + return;
> +
> + if (idProduct == USB_DEVICE_ID_ASUSTEK_ROG_NKEY_ALLY)
> + min_version = ROG_ALLY_MIN_MCU;
> +
> + hid_info(hdev, "Ally device MCU version: %d\n", version);
> + if (version < min_version) {
> + hid_warn(hdev,
> + "The MCU firmware version must be %d or greater\n"
What do you think about:
"The MCU firmware version must be %d or greater to avoid issues with
suspend.\n"
> + "Please update your MCU with official ASUS firmware release\n",
> + min_version);
> + }
> +}
Thinking forward to any future hypothetical devices that don't have a
min MCU version type of bug I have a suggestion that you put the
min_version into a lookup method of some sort.
So the flow can be something like this:
static void validate_mcu_fw_version(struct hid_device *hdev, int idProduct)
{
int min_version = get_min_version(idProduct);
if (!min_version)
return;
.
.
.
}
Or you can do a switch/case instead of get_min_version().
static void validate_mcu_fw_version(struct hid_device *hdev, int idProduct)
{
int min_version;
switch(idProduct) {
case USB_DEVICE_ID_ASUSTEK_ROG_NKEY_ALLY:
min_version = ROG_ALLY_MIN_MCU;
break;
case USB_DEVICE_ID_ASUSTEK_ROG_NKEY_ALLYX:
min_version = ROG_ALLYX_MIN_MCU;
break;
default:
return;
}
.
.
.
}
That way you have really straight forward logic that
validate_mcu_version only runs on devices that you specify.
> +
> static int asus_kbd_register_leds(struct hid_device *hdev)
> {
> struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
> + struct usb_interface *intf;
> + struct usb_device *udev;
> unsigned char kbd_func;
> int ret;
>
> @@ -560,6 +645,14 @@ static int asus_kbd_register_leds(struct hid_device *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));
> + }
> +
> } else {
> /* Initialize keyboard */
> ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID);
> @@ -1280,10 +1373,10 @@ static const struct hid_device_id asus_devices[] = {
> QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
> { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
> USB_DEVICE_ID_ASUSTEK_ROG_NKEY_ALLY),
> - QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
> + QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_ALLY_XPAD},
> { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
> USB_DEVICE_ID_ASUSTEK_ROG_NKEY_ALLY_X),
> - QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
> + QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_ALLY_XPAD },
> { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
> USB_DEVICE_ID_ASUSTEK_ROG_CLAYMORE_II_KEYBOARD),
> QUIRK_ROG_CLAYMORE_II_KEYBOARD },
On Tue, 2025-02-25 at 07:16 -0800, Mario Limonciello wrote:
> On 2/25/2025 00:17, Luke Jones wrote:
> > From: "Luke D. Jones" <luke@ljones.dev>
> >
> > ASUS have fixed suspend issues arising from a flag not being
> > cleared in
> > the MCU FW in both the ROG Ally 1 and the ROG Ally X.
> >
> > Implement a check and a warning to encourage users to update the FW
> > to
> > a minimum supported version.
> >
> > Signed-off-by: Luke D. Jones <luke@ljones.dev>
> > ---
> > drivers/hid/hid-asus.c | 97
> > +++++++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 95 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> > index 46e3e42f9eb5..e1e60b80115a 100644
> > --- a/drivers/hid/hid-asus.c
> > +++ b/drivers/hid/hid-asus.c
> > @@ -52,6 +52,10 @@ MODULE_DESCRIPTION("Asus HID Keyboard and
> > TouchPad");
> > #define FEATURE_KBD_LED_REPORT_ID1 0x5d
> > #define FEATURE_KBD_LED_REPORT_ID2 0x5e
> >
> > +#define ROG_ALLY_REPORT_SIZE 64
> > +#define ROG_ALLY_X_MIN_MCU 313
> > +#define ROG_ALLY_MIN_MCU 319
> > +
> > #define SUPPORT_KBD_BACKLIGHT BIT(0)
> >
> > #define MAX_TOUCH_MAJOR 8
> > @@ -84,6 +88,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and
> > TouchPad");
> > #define QUIRK_MEDION_E1239T BIT(10)
> > #define QUIRK_ROG_NKEY_KEYBOARD BIT(11)
> > #define QUIRK_ROG_CLAYMORE_II_KEYBOARD BIT(12)
> > +#define QUIRK_ROG_ALLY_XPAD BIT(13)
> >
> > #define
> > I2C_KEYBOARD_QUIRKS (QUIRK_FIX_NOTEBOOK_REPORT | \
> >
> > QUIRK_NO_INIT_REPORTS | \
> > @@ -534,9 +539,89 @@ static bool
> > asus_kbd_wmi_led_control_present(struct hid_device *hdev)
> > 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
> > + */
> > +static int mcu_parse_version_string(const u8 *response, size_t
> > response_size)
> > +{
> > + const u8 *end = response + response_size;
> > + const u8 *p = response;
> > + int dots, err;
> > + long version;
> > +
> > + dots = 0;
> > + while (p < end && dots < 2) {
> > + if (*p++ == '.')
> > + dots++;
> > + }
> > +
>
> I think it would be good to use strsep() here.
>
> > + if (dots != 2 || p >= end)
> > + return -EINVAL;
> > +
> > + err = kstrtol((const char *)p, 10, &version);
> > + if (err || version < 0)
> > + return -EINVAL;
> > +
> > + return version;
> > +}
> > +
> > +static int mcu_request_version(struct hid_device *hdev)
> > +{
> > + const u8 request[] = { 0x5a, 0x05, 0x03, 0x31, 0x00, 0x20
> > };
> > + u8 *response;
> > + int ret;
> > +
> > + response = kzalloc(ROG_ALLY_REPORT_SIZE, GFP_KERNEL);
> > + if (!response)
> > + return -ENOMEM;
> > +
> > + ret = asus_kbd_set_report(hdev, request, sizeof(request));
> > + if (ret < 0)
> > + goto out;
> > +
> > + ret = hid_hw_raw_request(hdev, FEATURE_REPORT_ID,
> > response,
> > + ROG_ALLY_REPORT_SIZE,
> > HID_FEATURE_REPORT,
> > + HID_REQ_GET_REPORT);
> > + if (ret < 0)
> > + goto out;
> > +
> > + ret = mcu_parse_version_string(response,
> > ROG_ALLY_REPORT_SIZE);
> > +
> > +out:
> > + if (ret < 0)
> > + hid_err(hdev, "Failed to get MCU version: %d,
> > response: %*ph\n",
> > + ret, ROG_ALLY_REPORT_SIZE,
> > response);
> > + kfree(response);
> > + return ret;
> > +}
> > +
> > +static void validate_mcu_fw_version(struct hid_device *hdev, int
> > idProduct)
> > +{
> > + int min_version = ROG_ALLY_X_MIN_MCU;
> > + int version;
> > +
> > + version = mcu_request_version(hdev);
> > + if (version < 0)
> > + return;
> > +
> > + if (idProduct == USB_DEVICE_ID_ASUSTEK_ROG_NKEY_ALLY)
> > + min_version = ROG_ALLY_MIN_MCU;
> > +
> > + hid_info(hdev, "Ally device MCU version: %d\n", version);
> > + if (version < min_version) {
> > + hid_warn(hdev,
> > + "The MCU firmware version must be %d or
> > greater\n"
>
> What do you think about:
>
> "The MCU firmware version must be %d or greater to avoid issues with
> suspend.\n"
>
I wasn't sure if I should be as explicit. But since it is directly
related I guess that should be fine. Will do for V2.
> > + "Please update your MCU with official
> > ASUS firmware release\n",
> > + min_version);
> > + }
> > +}
>
> Thinking forward to any future hypothetical devices that don't have a
> min MCU version type of bug I have a suggestion that you put the
> min_version into a lookup method of some sort.
>
> So the flow can be something like this:
>
> static void validate_mcu_fw_version(struct hid_device *hdev, int
> idProduct)
> {
>
> int min_version = get_min_version(idProduct);
>
> if (!min_version)
> return;
> .
> .
> .
> }
>
> Or you can do a switch/case instead of get_min_version().
>
> static void validate_mcu_fw_version(struct hid_device *hdev, int
> idProduct)
> {
>
> int min_version;
>
> switch(idProduct) {
> case USB_DEVICE_ID_ASUSTEK_ROG_NKEY_ALLY:
> min_version = ROG_ALLY_MIN_MCU;
> break;
> case USB_DEVICE_ID_ASUSTEK_ROG_NKEY_ALLYX:
> min_version = ROG_ALLYX_MIN_MCU;
> break;
> default:
> return;
> }
>
> .
> .
> .
> }
>
> That way you have really straight forward logic that
> validate_mcu_version only runs on devices that you specify.
>
I actually had a switch/case to start with. Not sure why I changed it
now. I'll go back to that as it's very clear.
> > +
> > static int asus_kbd_register_leds(struct hid_device *hdev)
> > {
> > struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
> > + struct usb_interface *intf;
> > + struct usb_device *udev;
> > unsigned char kbd_func;
> > int ret;
> >
> > @@ -560,6 +645,14 @@ static int asus_kbd_register_leds(struct
> > hid_device *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));
> > + }
> > +
> > } else {
> > /* Initialize keyboard */
> > ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID);
> > @@ -1280,10 +1373,10 @@ static const struct hid_device_id
> > asus_devices[] = {
> > QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
> > { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
> > USB_DEVICE_ID_ASUSTEK_ROG_NKEY_ALLY),
> > - QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
> > + QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD |
> > QUIRK_ROG_ALLY_XPAD},
> > { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
> > USB_DEVICE_ID_ASUSTEK_ROG_NKEY_ALLY_X),
> > - QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
> > + QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD |
> > QUIRK_ROG_ALLY_XPAD },
> > { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
> > USB_DEVICE_ID_ASUSTEK_ROG_CLAYMORE_II_KEYBOARD),
> > QUIRK_ROG_CLAYMORE_II_KEYBOARD },
>
© 2016 - 2026 Red Hat, Inc.