| 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: <64613914-1759-4008-8e56-ae220c0171fc@amd.com>
Date: Tue, 25 Feb 2025 23:00:32 -0800
From: Mario Limonciello <mario.limonciello@....com>
To: Luke Jones <luke@...nes.dev>, linux-kernel@...r.kernel.org
Cc: hdegoede@...hat.com, ilpo.jarvinen@...ux.intel.com,
platform-driver-x86@...r.kernel.org, linux-input@...r.kernel.org,
bentiss@...nel.org, jikos@...nel.org
Subject: Re: [PATCH v2 1/2] hid-asus: check ROG Ally MCU version and warn
On 2/25/2025 17:01, Luke Jones wrote:
> From: "Luke D. Jones" <luke@...nes.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@...nes.dev>
> ---
> drivers/hid/hid-asus.c | 103 ++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 101 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 46e3e42f9eb5..3cec622b6e68 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,95 @@ 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++;
> + }
Did you miss my comment about using strsep() instead?
> +
> + if (dots != 2 || p >= end)
> + return -EINVAL;
> +
> + err = kstrtol((const char *)p, 10, &version);
It seems a bit odd to me to convert to long only to then convert again
to an int (for the return code).
Would it make more sense to jump right to an integer immediately?
Sorry I missed this the first time.
> + 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;
If you're spinning away, maybe worth using a __free() macro to avoid a
manual kfree.
You could also drop the goto statements then and return ret for the
failures. Although admittedly you'll lose your error message for the
asus_kbd_set_report() and hid_hw_raw_request().
tedly > + 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, version;
> +
> + version = mcu_request_version(hdev);
> + if (version < 0)
> + return;
> +
> + switch (idProduct) {
> + case USB_DEVICE_ID_ASUSTEK_ROG_NKEY_ALLY:
> + min_version = ROG_ALLY_MIN_MCU;
> + break;
> + case USB_DEVICE_ID_ASUSTEK_ROG_NKEY_ALLY_X:
> + min_version = ROG_ALLY_X_MIN_MCU;
> + break;
> + default:
> + min_version = 0;
> + }
> +
> + 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 to avoid issues with suspend.\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 +651,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 +1379,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 },
Powered by blists - more mailing lists