[PATCH v3 0/2] hid-asus: asus-wmi: refactor Ally suspend/resume

[PATCH v3 0/2] hid-asus: asus-wmi: refactor Ally suspend/resume

[Luke Jones]

This short series refactors the Ally suspend/resume functionality in the
asus-wmi driver along with adding support for ROG Ally MCU version checking.

The version checking is then used to toggle the use of older CSEE call hacks
that were initially used to combat Ally suspend/wake issues arising from the MCU
not clearing a particular flag on resume. ASUS have since corrected this
especially for Linux in newer firmware versions.

- hid-asus requests the MCU version and displays a warning if the version is
  older than the one that fixes the issue.
- hid-asus awill also toggle the CSEE hack off, and mcu_powersave to on if the
version is high enough.

*Note: In review it was requested by Mario that I try strsep() for parsing
the version. I did try this and a few variations but the result was much
more code due to having to check more edge cases due to the input being
raw bytes. In the end the cleaned up while loop proved more robust.

- Changelog:
  + V2: https://lore.kernel.org/platform-driver-x86/20250226010129.32043-1-luke@ljones.dev/T/#t
    - Adjust warning message to explicitly mention suspend issues
    - Use switch/case block to set min_version
      - Set min_version to 0 by default and toggle hacks off
  + V3
    - Remove noise (excess pr_info)
    - Use kstrtoint, not kstrtolong
    - Use __free(kfree) for allocated mem and drop goto + logging
    - Use print_hex_dump() to show failed data after pr_err in mcu_request_version()
    - Use pr_debug in set_ally_mcu_hack() and set_ally_mcu_powersave() plus
      correct the message.

Luke D. Jones (2):
  hid-asus: check ROG Ally MCU version and warn
  platform/x86: asus-wmi: Refactor Ally suspend/resume

 drivers/hid/hid-asus.c                     | 111 +++++++++++++++++-
 drivers/platform/x86/asus-wmi.c            | 130 ++++++++++++++-------
 include/linux/platform_data/x86/asus-wmi.h |  15 +++
 3 files changed, 215 insertions(+), 41 deletions(-)

-- 
2.48.1

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

[Luke Jones]

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 | 107 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 105 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index 46e3e42f9eb5..599c836507ff 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,99 @@ 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
+ * The bytes "5a 05 03 31 00 1a 13" and possibly more come before the version
+ * string, and there may be additional bytes after the version string such as
+ * "75 00 74 00 65 00" or a postfix such as "_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, version;
+	char buf[4];
+
+	dots = 0;
+	while (p < end && dots < 2) {
+		if (*p++ == '.')
+			dots++;
+	}
+
+	if (dots != 2 || p >= end || (p + 3) >= end)
+		return -EINVAL;
+
+	memcpy(buf, p, 3);
+	buf[3] = '\0';
+
+	err = kstrtoint(buf, 10, &version);
+	if (err || version < 0)
+		return -EINVAL;
+
+	return version;
+}
+
+static int mcu_request_version(struct hid_device *hdev)
+{
+	u8 *response __free(kfree) = kzalloc(ROG_ALLY_REPORT_SIZE, GFP_KERNEL);
+	const u8 request[] = { 0x5a, 0x05, 0x03, 0x31, 0x00, 0x20 };
+	int ret;
+
+	if (!response)
+		return -ENOMEM;
+
+	ret = asus_kbd_set_report(hdev, request, sizeof(request));
+	if (ret < 0)
+		return ret;
+
+	ret = hid_hw_raw_request(hdev, FEATURE_REPORT_ID, response,
+				ROG_ALLY_REPORT_SIZE, HID_FEATURE_REPORT,
+				HID_REQ_GET_REPORT);
+	if (ret < 0)
+		return ret;
+
+	ret = mcu_parse_version_string(response, ROG_ALLY_REPORT_SIZE);
+	if (ret < 0) {
+		pr_err("Failed to parse MCU version: %d\n", ret);
+		print_hex_dump(KERN_ERR, "MCU: ", DUMP_PREFIX_NONE,
+			      16, 1, response, ROG_ALLY_REPORT_SIZE, false);
+	}
+
+	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;
+	}
+
+	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 +655,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 +1383,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.1

[PATCH v3 2/2] platform/x86: asus-wmi: Refactor Ally suspend/resume

[Luke Jones]

From: "Luke D. Jones" <luke@ljones.dev>

Adjust how the CSEE direct call hack is used.

The results of months of testing combined with help from ASUS to
determine the actual cause of suspend issues has resulted in this
refactoring which immensely improves the reliability for devices which
do not have the following minimum MCU FW version:
- ROG Ally X: 313
- ROG Ally 1: 319

For MCU FW versions that match the minimum or above the CSEE hack is
disabled and mcu_powersave set to on by default as there are no
negatives beyond a slightly slower device reinitialization due to the
MCU being powered off.

As this is set only at module load time, it is still possible for
mcu_powersave sysfs attributes to change it at runtime if so desired.

Signed-off-by: Luke D. Jones <luke@ljones.dev>
---
 drivers/hid/hid-asus.c                     |   4 +
 drivers/platform/x86/asus-wmi.c            | 130 ++++++++++++++-------
 include/linux/platform_data/x86/asus-wmi.h |  15 +++
 3 files changed, 110 insertions(+), 39 deletions(-)

diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index 599c836507ff..66bae5cea4f9 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -624,6 +624,9 @@ static void validate_mcu_fw_version(struct hid_device *hdev, int idProduct)
 		hid_warn(hdev,
 			"The MCU firmware version must be %d or greater to avoid issues with suspend.\n",
 			min_version);
+	} else {
+		set_ally_mcu_hack(false);
+		set_ally_mcu_powersave(true);
 	}
 }
 
@@ -1430,4 +1433,5 @@ static struct hid_driver asus_driver = {
 };
 module_hid_driver(asus_driver);
 
+MODULE_IMPORT_NS("ASUS_WMI");
 MODULE_LICENSE("GPL");
diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 38ef778e8c19..10936a091c42 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -142,16 +142,20 @@ module_param(fnlock_default, bool, 0444);
 #define ASUS_MINI_LED_2024_STRONG	0x01
 #define ASUS_MINI_LED_2024_OFF		0x02
 
-/* Controls the power state of the USB0 hub on ROG Ally which input is on */
 #define ASUS_USB0_PWR_EC0_CSEE "\\_SB.PCI0.SBRG.EC0.CSEE"
-/* 300ms so far seems to produce a reliable result on AC and battery */
-#define ASUS_USB0_PWR_EC0_CSEE_WAIT 1500
+/*
+ * The period required to wait after screen off/on/s2idle.check in MS.
+ * Time here greatly impacts the wake behaviour. Used in suspend/wake.
+ */
+#define ASUS_USB0_PWR_EC0_CSEE_WAIT	600
+#define ASUS_USB0_PWR_EC0_CSEE_OFF	0xB7
+#define ASUS_USB0_PWR_EC0_CSEE_ON	0xB8
 
 static const char * const ashs_ids[] = { "ATK4001", "ATK4002", NULL };
 
 static int throttle_thermal_policy_write(struct asus_wmi *);
 
-static const struct dmi_system_id asus_ally_mcu_quirk[] = {
+static const struct dmi_system_id asus_rog_ally_device[] = {
 	{
 		.matches = {
 			DMI_MATCH(DMI_BOARD_NAME, "RC71L"),
@@ -274,9 +278,6 @@ struct asus_wmi {
 	u32 tablet_switch_dev_id;
 	bool tablet_switch_inverted;
 
-	/* The ROG Ally device requires the MCU USB device be disconnected before suspend */
-	bool ally_mcu_usb_switch;
-
 	enum fan_type fan_type;
 	enum fan_type gpu_fan_type;
 	enum fan_type mid_fan_type;
@@ -335,6 +336,9 @@ struct asus_wmi {
 	struct asus_wmi_driver *driver;
 };
 
+/* Global to allow setting externally without requiring driver data */
+static bool use_ally_mcu_hack;
+
 /* WMI ************************************************************************/
 
 static int asus_wmi_evaluate_method3(u32 method_id,
@@ -549,7 +553,7 @@ static int asus_wmi_get_devstate(struct asus_wmi *asus, u32 dev_id, u32 *retval)
 	return 0;
 }
 
-static int asus_wmi_set_devstate(u32 dev_id, u32 ctrl_param,
+int asus_wmi_set_devstate(u32 dev_id, u32 ctrl_param,
 				 u32 *retval)
 {
 	return asus_wmi_evaluate_method(ASUS_WMI_METHODID_DEVS, dev_id,
@@ -1343,6 +1347,44 @@ static ssize_t nv_temp_target_show(struct device *dev,
 static DEVICE_ATTR_RW(nv_temp_target);
 
 /* Ally MCU Powersave ********************************************************/
+
+/*
+ * The HID driver needs to check MCU version and set this to false if the MCU FW
+ * version is >= the minimum requirements. New FW do not need the hacks.
+ */
+void set_ally_mcu_hack(bool enabled)
+{
+	use_ally_mcu_hack = enabled;
+	pr_debug("%s Ally MCU suspend quirk\n",
+		 enabled ? "Enabled" : "Disabled");
+}
+EXPORT_SYMBOL_NS_GPL(set_ally_mcu_hack, "ASUS_WMI");
+
+/*
+ * mcu_powersave should be enabled always, as it is fixed in MCU FW versions:
+ * - v313 for Ally X
+ * - v319 for Ally 1
+ * The HID driver checks MCU versions and so should set this if requirements match
+ */
+void set_ally_mcu_powersave(bool enabled)
+{
+	int result, err;
+
+	err = asus_wmi_set_devstate(ASUS_WMI_DEVID_MCU_POWERSAVE, enabled, &result);
+	if (err) {
+		pr_warn("Failed to set MCU powersave: %d\n", err);
+		return;
+	}
+	if (result > 1) {
+		pr_warn("Failed to set MCU powersave (result): 0x%x\n", result);
+		return;
+	}
+
+	pr_debug("%s MCU Powersave\n",
+		 enabled ? "Enabled" : "Disabled");
+}
+EXPORT_SYMBOL_NS_GPL(set_ally_mcu_powersave, "ASUS_WMI");
+
 static ssize_t mcu_powersave_show(struct device *dev,
 				   struct device_attribute *attr, char *buf)
 {
@@ -4711,6 +4753,18 @@ static int asus_wmi_add(struct platform_device *pdev)
 	if (err)
 		goto fail_platform;
 
+	use_ally_mcu_hack = acpi_has_method(NULL, ASUS_USB0_PWR_EC0_CSEE)
+				&& dmi_check_system(asus_rog_ally_device);
+	if (use_ally_mcu_hack && dmi_match(DMI_BOARD_NAME, "RC71")) {
+		/*
+		 * These steps ensure the device is in a valid good state, this is
+		 * especially important for the Ally 1 after a reboot.
+		 */
+		acpi_execute_simple_method(NULL, ASUS_USB0_PWR_EC0_CSEE,
+					   ASUS_USB0_PWR_EC0_CSEE_ON);
+		msleep(ASUS_USB0_PWR_EC0_CSEE_WAIT);
+	}
+
 	/* ensure defaults for tunables */
 	asus->ppt_pl2_sppt = 5;
 	asus->ppt_pl1_spl = 5;
@@ -4723,8 +4777,6 @@ static int asus_wmi_add(struct platform_device *pdev)
 	asus->egpu_enable_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_EGPU);
 	asus->dgpu_disable_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_DGPU);
 	asus->kbd_rgb_state_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_TUF_RGB_STATE);
-	asus->ally_mcu_usb_switch = acpi_has_method(NULL, ASUS_USB0_PWR_EC0_CSEE)
-						&& dmi_check_system(asus_ally_mcu_quirk);
 
 	if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_MINI_LED_MODE))
 		asus->mini_led_dev_id = ASUS_WMI_DEVID_MINI_LED_MODE;
@@ -4910,34 +4962,6 @@ static int asus_hotk_resume(struct device *device)
 	return 0;
 }
 
-static int asus_hotk_resume_early(struct device *device)
-{
-	struct asus_wmi *asus = dev_get_drvdata(device);
-
-	if (asus->ally_mcu_usb_switch) {
-		/* sleep required to prevent USB0 being yanked then reappearing rapidly */
-		if (ACPI_FAILURE(acpi_execute_simple_method(NULL, ASUS_USB0_PWR_EC0_CSEE, 0xB8)))
-			dev_err(device, "ROG Ally MCU failed to connect USB dev\n");
-		else
-			msleep(ASUS_USB0_PWR_EC0_CSEE_WAIT);
-	}
-	return 0;
-}
-
-static int asus_hotk_prepare(struct device *device)
-{
-	struct asus_wmi *asus = dev_get_drvdata(device);
-
-	if (asus->ally_mcu_usb_switch) {
-		/* sleep required to ensure USB0 is disabled before sleep continues */
-		if (ACPI_FAILURE(acpi_execute_simple_method(NULL, ASUS_USB0_PWR_EC0_CSEE, 0xB7)))
-			dev_err(device, "ROG Ally MCU failed to disconnect USB dev\n");
-		else
-			msleep(ASUS_USB0_PWR_EC0_CSEE_WAIT);
-	}
-	return 0;
-}
-
 static int asus_hotk_restore(struct device *device)
 {
 	struct asus_wmi *asus = dev_get_drvdata(device);
@@ -4978,11 +5002,34 @@ static int asus_hotk_restore(struct device *device)
 	return 0;
 }
 
+static void asus_ally_s2idle_restore(void)
+{
+	if (use_ally_mcu_hack) {
+		acpi_execute_simple_method(NULL, ASUS_USB0_PWR_EC0_CSEE,
+					   ASUS_USB0_PWR_EC0_CSEE_ON);
+		msleep(ASUS_USB0_PWR_EC0_CSEE_WAIT);
+	}
+}
+
+static int asus_hotk_prepare(struct device *device)
+{
+	if (use_ally_mcu_hack) {
+		acpi_execute_simple_method(NULL, ASUS_USB0_PWR_EC0_CSEE,
+					   ASUS_USB0_PWR_EC0_CSEE_OFF);
+		msleep(ASUS_USB0_PWR_EC0_CSEE_WAIT);
+	}
+	return 0;
+}
+
+/* Use only for Ally devices due to the wake_on_ac */
+static struct acpi_s2idle_dev_ops asus_ally_s2idle_dev_ops = {
+	.restore = asus_ally_s2idle_restore,
+};
+
 static const struct dev_pm_ops asus_pm_ops = {
 	.thaw = asus_hotk_thaw,
 	.restore = asus_hotk_restore,
 	.resume = asus_hotk_resume,
-	.resume_early = asus_hotk_resume_early,
 	.prepare = asus_hotk_prepare,
 };
 
@@ -5010,6 +5057,10 @@ static int asus_wmi_probe(struct platform_device *pdev)
 			return ret;
 	}
 
+	ret = acpi_register_lps0_dev(&asus_ally_s2idle_dev_ops);
+	if (ret)
+		pr_warn("failed to register LPS0 sleep handler in asus-wmi\n");
+
 	return asus_wmi_add(pdev);
 }
 
@@ -5042,6 +5093,7 @@ EXPORT_SYMBOL_GPL(asus_wmi_register_driver);
 
 void asus_wmi_unregister_driver(struct asus_wmi_driver *driver)
 {
+	acpi_unregister_lps0_dev(&asus_ally_s2idle_dev_ops);
 	platform_device_unregister(driver->platform_device);
 	platform_driver_unregister(&driver->platform_driver);
 	used = false;
diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
index 783e2a336861..a32cb8865b2f 100644
--- a/include/linux/platform_data/x86/asus-wmi.h
+++ b/include/linux/platform_data/x86/asus-wmi.h
@@ -158,8 +158,23 @@
 #define ASUS_WMI_DSTS_LIGHTBAR_MASK	0x0000000F
 
 #if IS_REACHABLE(CONFIG_ASUS_WMI)
+void set_ally_mcu_hack(bool enabled);
+void set_ally_mcu_powersave(bool enabled);
+int asus_wmi_set_devstate(u32 dev_id, u32 ctrl_param, u32 *retval);
 int asus_wmi_evaluate_method(u32 method_id, u32 arg0, u32 arg1, u32 *retval);
 #else
+static inline void set_ally_mcu_hack(bool enabled)
+{
+	return -ENODEV;
+}
+static inline void set_ally_mcu_powersave(bool enabled)
+{
+	return -ENODEV;
+}
+static inline int asus_wmi_set_devstate(u32 dev_id, u32 ctrl_param, u32 *retval)
+{
+	return -ENODEV;
+}
 static inline int asus_wmi_evaluate_method(u32 method_id, u32 arg0, u32 arg1,
 					   u32 *retval)
 {
-- 
2.48.1

Re: [PATCH v3 2/2] platform/x86: asus-wmi: Refactor Ally suspend/resume

[kernel test robot]

Hi Luke,

kernel test robot noticed the following build errors:

[auto build test ERROR on hid/for-next]
[also build test ERROR on linus/master v6.14-rc5 next-20250307]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Luke-Jones/hid-asus-check-ROG-Ally-MCU-version-and-warn/20250227-165941
base:   https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git for-next
patch link:    https://lore.kernel.org/r/20250227085817.1007697-3-luke%40ljones.dev
patch subject: [PATCH v3 2/2] platform/x86: asus-wmi: Refactor Ally suspend/resume
config: mips-allyesconfig (https://download.01.org/0day-ci/archive/20250308/202503081003.gNlKWt4M-lkp@intel.com/config)
compiler: mips-linux-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250308/202503081003.gNlKWt4M-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202503081003.gNlKWt4M-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from drivers/hid/hid-asus.c:29:
   include/linux/platform_data/x86/asus-wmi.h: In function 'set_ally_mcu_hack':
>> include/linux/platform_data/x86/asus-wmi.h:168:16: error: 'return' with a value, in function returning void [-Wreturn-mismatch]
     168 |         return -ENODEV;
         |                ^
   include/linux/platform_data/x86/asus-wmi.h:166:20: note: declared here
     166 | static inline void set_ally_mcu_hack(bool enabled)
         |                    ^~~~~~~~~~~~~~~~~
   include/linux/platform_data/x86/asus-wmi.h: In function 'set_ally_mcu_powersave':
   include/linux/platform_data/x86/asus-wmi.h:172:16: error: 'return' with a value, in function returning void [-Wreturn-mismatch]
     172 |         return -ENODEV;
         |                ^
   include/linux/platform_data/x86/asus-wmi.h:170:20: note: declared here
     170 | static inline void set_ally_mcu_powersave(bool enabled)
         |                    ^~~~~~~~~~~~~~~~~~~~~~


vim +/return +168 include/linux/platform_data/x86/asus-wmi.h

   159	
   160	#if IS_REACHABLE(CONFIG_ASUS_WMI)
   161	void set_ally_mcu_hack(bool enabled);
   162	void set_ally_mcu_powersave(bool enabled);
   163	int asus_wmi_set_devstate(u32 dev_id, u32 ctrl_param, u32 *retval);
   164	int asus_wmi_evaluate_method(u32 method_id, u32 arg0, u32 arg1, u32 *retval);
   165	#else
   166	static inline void set_ally_mcu_hack(bool enabled)
   167	{
 > 168		return -ENODEV;
   169	}
   170	static inline void set_ally_mcu_powersave(bool enabled)
   171	{
   172		return -ENODEV;
   173	}
   174	static inline int asus_wmi_set_devstate(u32 dev_id, u32 ctrl_param, u32 *retval)
   175	{
   176		return -ENODEV;
   177	}
   178	static inline int asus_wmi_evaluate_method(u32 method_id, u32 arg0, u32 arg1,
   179						   u32 *retval)
   180	{
   181		return -ENODEV;
   182	}
   183	#endif
   184	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v3 2/2] platform/x86: asus-wmi: Refactor Ally suspend/resume

[Luke Jones]

On Sat, 8 Mar 2025, at 3:55 PM, kernel test robot wrote:
> Hi Luke,
>
> kernel test robot noticed the following build errors:
>
> [auto build test ERROR on hid/for-next]
> [also build test ERROR on linus/master v6.14-rc5 next-20250307]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>
> url:    
> https://github.com/intel-lab-lkp/linux/commits/Luke-Jones/hid-asus-check-ROG-Ally-MCU-version-and-warn/20250227-165941
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git 
> for-next
> patch link:    
> https://lore.kernel.org/r/20250227085817.1007697-3-luke%40ljones.dev
> patch subject: [PATCH v3 2/2] platform/x86: asus-wmi: Refactor Ally 
> suspend/resume
> config: mips-allyesconfig 
> (https://download.01.org/0day-ci/archive/20250308/202503081003.gNlKWt4M-lkp@intel.com/config)
> compiler: mips-linux-gcc (GCC) 14.2.0
> reproduce (this is a W=1 build): 
> (https://download.01.org/0day-ci/archive/20250308/202503081003.gNlKWt4M-lkp@intel.com/reproduce)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new 
> version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: 
> https://lore.kernel.org/oe-kbuild-all/202503081003.gNlKWt4M-lkp@intel.com/
>
> All errors (new ones prefixed by >>):
>
>    In file included from drivers/hid/hid-asus.c:29:
>    include/linux/platform_data/x86/asus-wmi.h: In function 'set_ally_mcu_hack':
>>> include/linux/platform_data/x86/asus-wmi.h:168:16: error: 'return' with a value, in function returning void [-Wreturn-mismatch]
>      168 |         return -ENODEV;
>          |                ^
>    include/linux/platform_data/x86/asus-wmi.h:166:20: note: declared 
> here
>      166 | static inline void set_ally_mcu_hack(bool enabled)
>          |                    ^~~~~~~~~~~~~~~~~
>    include/linux/platform_data/x86/asus-wmi.h: In function 
> 'set_ally_mcu_powersave':
>    include/linux/platform_data/x86/asus-wmi.h:172:16: error: 'return' 
> with a value, in function returning void [-Wreturn-mismatch]
>      172 |         return -ENODEV;
>          |                ^
>    include/linux/platform_data/x86/asus-wmi.h:170:20: note: declared 
> here
>      170 | static inline void set_ally_mcu_powersave(bool enabled)
>          |                    ^~~~~~~~~~~~~~~~~~~~~~

*Groaning sounds*.
Ilpo/Hans, you guys want me to fix this and submit new series or can this be corrected when pulled in?
There's two more of the same below this.

>
> vim +/return +168 include/linux/platform_data/x86/asus-wmi.h
>
>    159	
>    160	#if IS_REACHABLE(CONFIG_ASUS_WMI)
>    161	void set_ally_mcu_hack(bool enabled);
>    162	void set_ally_mcu_powersave(bool enabled);
>    163	int asus_wmi_set_devstate(u32 dev_id, u32 ctrl_param, u32 
> *retval);
>    164	int asus_wmi_evaluate_method(u32 method_id, u32 arg0, u32 arg1, 
> u32 *retval);
>    165	#else
>    166	static inline void set_ally_mcu_hack(bool enabled)
>    167	{
>  > 168		return -ENODEV;
>    169	}
>    170	static inline void set_ally_mcu_powersave(bool enabled)
>    171	{
>    172		return -ENODEV;
>    173	}
>    174	static inline int asus_wmi_set_devstate(u32 dev_id, u32 
> ctrl_param, u32 *retval)
>    175	{
>    176		return -ENODEV;
>    177	}
>    178	static inline int asus_wmi_evaluate_method(u32 method_id, u32 
> arg0, u32 arg1,
>    179						   u32 *retval)
>    180	{
>    181		return -ENODEV;
>    182	}
>    183	#endif
>    184	
>
> -- 
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests/wiki

Re: [PATCH v3 0/2] hid-asus: asus-wmi: refactor Ally suspend/resume

[Mario Limonciello]

On 2/27/2025 02:58, Luke Jones wrote:
> This short series refactors the Ally suspend/resume functionality in the
> asus-wmi driver along with adding support for ROG Ally MCU version checking.
> 
> The version checking is then used to toggle the use of older CSEE call hacks
> that were initially used to combat Ally suspend/wake issues arising from the MCU
> not clearing a particular flag on resume. ASUS have since corrected this
> especially for Linux in newer firmware versions.
> 
> - hid-asus requests the MCU version and displays a warning if the version is
>    older than the one that fixes the issue.
> - hid-asus awill also toggle the CSEE hack off, and mcu_powersave to on if the
> version is high enough.
> 
> *Note: In review it was requested by Mario that I try strsep() for parsing
> the version. I did try this and a few variations but the result was much
> more code due to having to check more edge cases due to the input being
> raw bytes. In the end the cleaned up while loop proved more robust.
> 
> - Changelog:
>    + V2: https://lore.kernel.org/platform-driver-x86/20250226010129.32043-1-luke@ljones.dev/T/#t
>      - Adjust warning message to explicitly mention suspend issues
>      - Use switch/case block to set min_version
>        - Set min_version to 0 by default and toggle hacks off
>    + V3
>      - Remove noise (excess pr_info)
>      - Use kstrtoint, not kstrtolong
>      - Use __free(kfree) for allocated mem and drop goto + logging
>      - Use print_hex_dump() to show failed data after pr_err in mcu_request_version()
>      - Use pr_debug in set_ally_mcu_hack() and set_ally_mcu_powersave() plus
>        correct the message.
> 
> Luke D. Jones (2):
>    hid-asus: check ROG Ally MCU version and warn
>    platform/x86: asus-wmi: Refactor Ally suspend/resume
> 
>   drivers/hid/hid-asus.c                     | 111 +++++++++++++++++-
>   drivers/platform/x86/asus-wmi.c            | 130 ++++++++++++++-------
>   include/linux/platform_data/x86/asus-wmi.h |  15 +++
>   3 files changed, 215 insertions(+), 41 deletions(-)
> 

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>

Re: [PATCH v3 0/2] hid-asus: asus-wmi: refactor Ally suspend/resume

[Jiri Kosina]

On Thu, 27 Feb 2025, Luke Jones wrote:

> This short series refactors the Ally suspend/resume functionality in the
> asus-wmi driver along with adding support for ROG Ally MCU version checking.
> 
> The version checking is then used to toggle the use of older CSEE call hacks
> that were initially used to combat Ally suspend/wake issues arising from the MCU
> not clearing a particular flag on resume. ASUS have since corrected this
> especially for Linux in newer firmware versions.
> 
> - hid-asus requests the MCU version and displays a warning if the version is
>   older than the one that fixes the issue.
> - hid-asus awill also toggle the CSEE hack off, and mcu_powersave to on if the
> version is high enough.
> 
> *Note: In review it was requested by Mario that I try strsep() for parsing
> the version. I did try this and a few variations but the result was much
> more code due to having to check more edge cases due to the input being
> raw bytes. In the end the cleaned up while loop proved more robust.
> 
> - Changelog:
>   + V2: https://lore.kernel.org/platform-driver-x86/20250226010129.32043-1-luke@ljones.dev/T/#t
>     - Adjust warning message to explicitly mention suspend issues
>     - Use switch/case block to set min_version
>       - Set min_version to 0 by default and toggle hacks off
>   + V3
>     - Remove noise (excess pr_info)
>     - Use kstrtoint, not kstrtolong
>     - Use __free(kfree) for allocated mem and drop goto + logging
>     - Use print_hex_dump() to show failed data after pr_err in mcu_request_version()
>     - Use pr_debug in set_ally_mcu_hack() and set_ally_mcu_powersave() plus
>       correct the message.
> 
> Luke D. Jones (2):
>   hid-asus: check ROG Ally MCU version and warn
>   platform/x86: asus-wmi: Refactor Ally suspend/resume
> 
>  drivers/hid/hid-asus.c                     | 111 +++++++++++++++++-
>  drivers/platform/x86/asus-wmi.c            | 130 ++++++++++++++-------
>  include/linux/platform_data/x86/asus-wmi.h |  15 +++
>  3 files changed, 215 insertions(+), 41 deletions(-)

Hans, are you OK taking both patches through your tree?

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v3 0/2] hid-asus: asus-wmi: refactor Ally suspend/resume

[Ilpo Järvinen]

On Tue, 4 Mar 2025, Jiri Kosina wrote:
> On Thu, 27 Feb 2025, Luke Jones wrote:
> 
> > This short series refactors the Ally suspend/resume functionality in the
> > asus-wmi driver along with adding support for ROG Ally MCU version checking.
> > 
> > The version checking is then used to toggle the use of older CSEE call hacks
> > that were initially used to combat Ally suspend/wake issues arising from the MCU
> > not clearing a particular flag on resume. ASUS have since corrected this
> > especially for Linux in newer firmware versions.
> > 
> > - hid-asus requests the MCU version and displays a warning if the version is
> >   older than the one that fixes the issue.
> > - hid-asus awill also toggle the CSEE hack off, and mcu_powersave to on if the
> > version is high enough.
> > 
> > *Note: In review it was requested by Mario that I try strsep() for parsing
> > the version. I did try this and a few variations but the result was much
> > more code due to having to check more edge cases due to the input being
> > raw bytes. In the end the cleaned up while loop proved more robust.
> > 
> > - Changelog:
> >   + V2: https://lore.kernel.org/platform-driver-x86/20250226010129.32043-1-luke@ljones.dev/T/#t
> >     - Adjust warning message to explicitly mention suspend issues
> >     - Use switch/case block to set min_version
> >       - Set min_version to 0 by default and toggle hacks off
> >   + V3
> >     - Remove noise (excess pr_info)
> >     - Use kstrtoint, not kstrtolong
> >     - Use __free(kfree) for allocated mem and drop goto + logging
> >     - Use print_hex_dump() to show failed data after pr_err in mcu_request_version()
> >     - Use pr_debug in set_ally_mcu_hack() and set_ally_mcu_powersave() plus
> >       correct the message.
> > 
> > Luke D. Jones (2):
> >   hid-asus: check ROG Ally MCU version and warn
> >   platform/x86: asus-wmi: Refactor Ally suspend/resume
> > 
> >  drivers/hid/hid-asus.c                     | 111 +++++++++++++++++-
> >  drivers/platform/x86/asus-wmi.c            | 130 ++++++++++++++-------
> >  include/linux/platform_data/x86/asus-wmi.h |  15 +++
> >  3 files changed, 215 insertions(+), 41 deletions(-)
> 
> Hans, are you OK taking both patches through your tree?

Yes, I can take them both.

-- 
 i.

[PATCH v3 0/2] hid-asus: asus-wmi: refactor Ally suspend/resume

[Luke Jones]

This short series refactors the Ally suspend/resume functionality in the
asus-wmi driver along with adding support for ROG Ally MCU version checking.

The version checking is then used to toggle the use of older CSEE call hacks
that were initially used to combat Ally suspend/wake issues arising from the MCU
not clearing a particular flag on resume. ASUS have since corrected this
especially for Linux in newer firmware versions.

- hid-asus requests the MCU version and displays a warning if the version is
  older than the one that fixes the issue.
- hid-asus awill also toggle the CSEE hack off, and mcu_powersave to on if the
version is high enough.

- Changelog:
  + V2:
    - Adjust warning message to explicitly mention suspend issues
    - Use switch/case block to set min_version
      - Set min_version to 0 by default and toggle hacks off
  + V3:
    - Fix errors picked up by test bot: incorrect return in the else block
      of `#if IS_REACHABLE(CONFIG_ASUS_WMI)`:
      - set_ally_mcu_hack()
      - set_ally_mcu_powersave()

Luke D. Jones (2):
  hid-asus: check ROG Ally MCU version and warn
  platform/x86: asus-wmi: Refactor Ally suspend/resume

 drivers/hid/hid-asus.c                     | 111 +++++++++++++++++-
 drivers/platform/x86/asus-wmi.c            | 130 ++++++++++++++-------
 include/linux/platform_data/x86/asus-wmi.h |  13 +++
 3 files changed, 213 insertions(+), 41 deletions(-)

-- 
2.49.0

Re: [PATCH v3 0/2] hid-asus: asus-wmi: refactor Ally suspend/resume

[Luke D. Jones]

On 21/03/25 16:51, Luke Jones wrote:
> This short series refactors the Ally suspend/resume functionality in the
> asus-wmi driver along with adding support for ROG Ally MCU version checking.
> 
> The version checking is then used to toggle the use of older CSEE call hacks
> that were initially used to combat Ally suspend/wake issues arising from the MCU
> not clearing a particular flag on resume. ASUS have since corrected this
> especially for Linux in newer firmware versions.
> 
> - hid-asus requests the MCU version and displays a warning if the version is
>    older than the one that fixes the issue.
> - hid-asus awill also toggle the CSEE hack off, and mcu_powersave to on if the
> version is high enough.

It is *strongly* preferred to remove the hack completely, but this may 
take some time after this series lands for it to be viable without 
regressing users.

I thought I had added to above to cover-letter but did not, sorry all.

The aim is to remove the hack at some stage this year.

> - Changelog:
>    + V2:
>      - Adjust warning message to explicitly mention suspend issues
>      - Use switch/case block to set min_version
>        - Set min_version to 0 by default and toggle hacks off
>    + V3:
>      - Fix errors picked up by test bot: incorrect return in the else block
>        of `#if IS_REACHABLE(CONFIG_ASUS_WMI)`:
>        - set_ally_mcu_hack()
>        - set_ally_mcu_powersave()
> 
> Luke D. Jones (2):
>    hid-asus: check ROG Ally MCU version and warn
>    platform/x86: asus-wmi: Refactor Ally suspend/resume
> 
>   drivers/hid/hid-asus.c                     | 111 +++++++++++++++++-
>   drivers/platform/x86/asus-wmi.c            | 130 ++++++++++++++-------
>   include/linux/platform_data/x86/asus-wmi.h |  13 +++
>   3 files changed, 213 insertions(+), 41 deletions(-)
> 

Re: [PATCH v3 0/2] hid-asus: asus-wmi: refactor Ally suspend/resume

[Antheas Kapenekakis]

On Fri, 21 Mar 2025 at 04:51, Luke Jones <luke@ljones.dev> wrote:
>
> This short series refactors the Ally suspend/resume functionality in the
> asus-wmi driver along with adding support for ROG Ally MCU version checking.
>
> The version checking is then used to toggle the use of older CSEE call hacks
> that were initially used to combat Ally suspend/wake issues arising from the MCU
> not clearing a particular flag on resume. ASUS have since corrected this
> especially for Linux in newer firmware versions.
>
> - hid-asus requests the MCU version and displays a warning if the version is
>   older than the one that fixes the issue.
> - hid-asus awill also toggle the CSEE hack off, and mcu_powersave to on if the
> version is high enough.
>
> - Changelog:
>   + V2:
>     - Adjust warning message to explicitly mention suspend issues
>     - Use switch/case block to set min_version
>       - Set min_version to 0 by default and toggle hacks off
>   + V3:
>     - Fix errors picked up by test bot: incorrect return in the else block
>       of `#if IS_REACHABLE(CONFIG_ASUS_WMI)`:
>       - set_ally_mcu_hack()
>       - set_ally_mcu_powersave()
>
> Luke D. Jones (2):
>   hid-asus: check ROG Ally MCU version and warn
>   platform/x86: asus-wmi: Refactor Ally suspend/resume
>
>  drivers/hid/hid-asus.c                     | 111 +++++++++++++++++-
>  drivers/platform/x86/asus-wmi.c            | 130 ++++++++++++++-------
>  include/linux/platform_data/x86/asus-wmi.h |  13 +++
>  3 files changed, 213 insertions(+), 41 deletions(-)
>
> --
> 2.49.0
>

Since I have to also test my series on my ally and booted a dev
environment, I am also giving this a go. I'll post some results in a
bit

Antheas