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@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > ---
> > 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 },
>
