Re: [PATCH 1/2] hid-asus: check ROG Ally MCU version and warn

[Luke Jones <luke@ljones.dev>]

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