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

[PATCH v4 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.
  + V4
    - Change use_ally_mcu_hack var to enum to track init state and
      prevent a race condition

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            | 133 +++++++++++++++------
 include/linux/platform_data/x86/asus-wmi.h |  19 +++
 3 files changed, 222 insertions(+), 41 deletions(-)

-- 
2.49.0

[PATCH v4 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.49.0

[PATCH v4 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            | 133 +++++++++++++++------
 include/linux/platform_data/x86/asus-wmi.h |  19 +++
 3 files changed, 117 insertions(+), 39 deletions(-)

diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index 599c836507ff..4b45e31f0bab 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(ASUS_WMI_ALLY_MCU_HACK_DISABLED);
+		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..27f11643a00d 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 enum asus_ally_mcu_hack use_ally_mcu_hack = ASUS_WMI_ALLY_MCU_HACK_INIT;
+
 /* 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(enum asus_ally_mcu_hack status)
+{
+	use_ally_mcu_hack = status;
+	pr_debug("%s Ally MCU suspend quirk\n",
+		 status == ASUS_WMI_ALLY_MCU_HACK_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,21 @@ static int asus_wmi_add(struct platform_device *pdev)
 	if (err)
 		goto fail_platform;
 
+	if (use_ally_mcu_hack == ASUS_WMI_ALLY_MCU_HACK_INIT) {
+		if (acpi_has_method(NULL, ASUS_USB0_PWR_EC0_CSEE)
+					&& dmi_check_system(asus_rog_ally_device))
+			use_ally_mcu_hack = ASUS_WMI_ALLY_MCU_HACK_ENABLED;
+		if (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 +4780,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 +4965,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 +5005,34 @@ static int asus_hotk_restore(struct device *device)
 	return 0;
 }
 
+static void asus_ally_s2idle_restore(void)
+{
+	if (use_ally_mcu_hack == ASUS_WMI_ALLY_MCU_HACK_ENABLED) {
+		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 == ASUS_WMI_ALLY_MCU_HACK_ENABLED) {
+		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 +5060,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 +5096,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..8a515179113d 100644
--- a/include/linux/platform_data/x86/asus-wmi.h
+++ b/include/linux/platform_data/x86/asus-wmi.h
@@ -157,9 +157,28 @@
 #define ASUS_WMI_DSTS_MAX_BRIGTH_MASK	0x0000FF00
 #define ASUS_WMI_DSTS_LIGHTBAR_MASK	0x0000000F
 
+enum asus_ally_mcu_hack {
+	ASUS_WMI_ALLY_MCU_HACK_INIT,
+	ASUS_WMI_ALLY_MCU_HACK_ENABLED,
+	ASUS_WMI_ALLY_MCU_HACK_DISABLED,
+};
+
 #if IS_REACHABLE(CONFIG_ASUS_WMI)
+void set_ally_mcu_hack(enum asus_ally_mcu_hack status);
+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(enum asus_ally_mcu_hack status)
+{
+}
+static inline void set_ally_mcu_powersave(bool enabled)
+{
+}
+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.49.0

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

[Antheas Kapenekakis]

On Sun, 23 Mar 2025 at 03:34, 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.
>
> *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

How did the testing go with this one, especially with mcu_powersave 0?

>     - 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.
>   + V4
>     - Change use_ally_mcu_hack var to enum to track init state and
>       prevent a race condition
>
> 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            | 133 +++++++++++++++------
>  include/linux/platform_data/x86/asus-wmi.h |  19 +++
>  3 files changed, 222 insertions(+), 41 deletions(-)
>
> --
> 2.49.0
>

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

[Luke D. Jones]

On 24/03/25 00:41, Antheas Kapenekakis wrote:
> On Sun, 23 Mar 2025 at 03:34, 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.
>>
>> *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
> 
> How did the testing go with this one, especially with mcu_powersave 0?

Appears to be good. Checked a few reboots with powersave off - it is 
setting on as I expect every time. Did modules unload/load also. And 
tested with it set off after boot plus suspend resumes.

Very much hope this is the end of that particular saga, and with 
bazzites help we can hopefully get everyone on November MCU FW or later, 
then finally remove the hack completely this year.

A small side note - I expect ASUS to fully reuse the X hardware, or at 
least the bios/acpi/mcu-fw for that new windows handheld they've doing, 
so fingers crossed that they actually do, and there will be nomore 
suspend issues with current kernels plus this patch.

Cheers,
Luke.

>>      - 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.
>>    + V4
>>      - Change use_ally_mcu_hack var to enum to track init state and
>>        prevent a race condition
>>
>> 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            | 133 +++++++++++++++------
>>   include/linux/platform_data/x86/asus-wmi.h |  19 +++
>>   3 files changed, 222 insertions(+), 41 deletions(-)
>>
>> --
>> 2.49.0
>>

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

[Antheas Kapenekakis]

On Mon, 24 Mar 2025 at 02:41, Luke D. Jones <luke@ljones.dev> wrote:
>
> On 24/03/25 00:41, Antheas Kapenekakis wrote:
> > On Sun, 23 Mar 2025 at 03:34, 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.
> >>
> >> *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
> >
> > How did the testing go with this one, especially with mcu_powersave 0?
>
> Appears to be good. Checked a few reboots with powersave off - it is
> setting on as I expect every time. Did modules unload/load also. And
> tested with it set off after boot plus suspend resumes.

Did you test suspends with mcu_powersave to 0 and rgb on? I had a few
issues you can reference the previous version for and I want to see if
you have them.

Even with powersave set to 1, the RGB does not fade anymore without the quirk

Antheas

> Very much hope this is the end of that particular saga, and with
> bazzites help we can hopefully get everyone on November MCU FW or later,
> then finally remove the hack completely this year.
>
> A small side note - I expect ASUS to fully reuse the X hardware, or at
> least the bios/acpi/mcu-fw for that new windows handheld they've doing,
> so fingers crossed that they actually do, and there will be nomore
> suspend issues with current kernels plus this patch.
>
> Cheers,
> Luke.
>
> >>      - 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.
> >>    + V4
> >>      - Change use_ally_mcu_hack var to enum to track init state and
> >>        prevent a race condition
> >>
> >> 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            | 133 +++++++++++++++------
> >>   include/linux/platform_data/x86/asus-wmi.h |  19 +++
> >>   3 files changed, 222 insertions(+), 41 deletions(-)
> >>
> >> --
> >> 2.49.0
> >>
>

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

[Luke D. Jones]

On 24/03/25 21:11, Antheas Kapenekakis wrote:
> On Mon, 24 Mar 2025 at 02:41, Luke D. Jones <luke@ljones.dev> wrote:
>>
>> On 24/03/25 00:41, Antheas Kapenekakis wrote:
>>> On Sun, 23 Mar 2025 at 03:34, 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.
>>>>
>>>> *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
>>>
>>> How did the testing go with this one, especially with mcu_powersave 0?
>>
>> Appears to be good. Checked a few reboots with powersave off - it is
>> setting on as I expect every time. Did modules unload/load also. And
>> tested with it set off after boot plus suspend resumes.
> 
> Did you test suspends with mcu_powersave to 0 and rgb on? I had a few
> issues you can reference the previous version for and I want to see if
> you have them.
> 
> Even with powersave set to 1, the RGB does not fade anymore without the quirk

Yes I tested every scenario I could think of. I don't think the fade is 
something to worry about - seems like it happening at all previously was 
just due to suspend being held up for a bit longer and now that the hack 
is disabled for new FW, it relies fully on Linux suspend (async? 
Honestly it's never been fully clear how async it really is).

I'd rather the faster suspend/resume. And so far I've heard no 
complaints (although my userbase is smaller than bazzites).

Cheers,
Luke.

> Antheas
> 
>> Very much hope this is the end of that particular saga, and with
>> bazzites help we can hopefully get everyone on November MCU FW or later,
>> then finally remove the hack completely this year.
>>
>> A small side note - I expect ASUS to fully reuse the X hardware, or at
>> least the bios/acpi/mcu-fw for that new windows handheld they've doing,
>> so fingers crossed that they actually do, and there will be nomore
>> suspend issues with current kernels plus this patch.
>>
>> Cheers,
>> Luke.
>>
>>>>       - 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.
>>>>     + V4
>>>>       - Change use_ally_mcu_hack var to enum to track init state and
>>>>         prevent a race condition
>>>>
>>>> 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            | 133 +++++++++++++++------
>>>>    include/linux/platform_data/x86/asus-wmi.h |  19 +++
>>>>    3 files changed, 222 insertions(+), 41 deletions(-)
>>>>
>>>> --
>>>> 2.49.0
>>>>
>>

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

[Ilpo Järvinen]

On Sun, 23 Mar 2025, Luke Jones wrote:

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

This series update lost Mario's rev-bys (given in replies to v3). Was that 
intentional??

-- 
 i.

> ---
>  drivers/hid/hid-asus.c                     |   4 +
>  drivers/platform/x86/asus-wmi.c            | 133 +++++++++++++++------
>  include/linux/platform_data/x86/asus-wmi.h |  19 +++
>  3 files changed, 117 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 599c836507ff..4b45e31f0bab 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(ASUS_WMI_ALLY_MCU_HACK_DISABLED);
> +		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..27f11643a00d 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 enum asus_ally_mcu_hack use_ally_mcu_hack = ASUS_WMI_ALLY_MCU_HACK_INIT;
> +
>  /* 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(enum asus_ally_mcu_hack status)
> +{
> +	use_ally_mcu_hack = status;
> +	pr_debug("%s Ally MCU suspend quirk\n",
> +		 status == ASUS_WMI_ALLY_MCU_HACK_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,21 @@ static int asus_wmi_add(struct platform_device *pdev)
>  	if (err)
>  		goto fail_platform;
>  
> +	if (use_ally_mcu_hack == ASUS_WMI_ALLY_MCU_HACK_INIT) {
> +		if (acpi_has_method(NULL, ASUS_USB0_PWR_EC0_CSEE)
> +					&& dmi_check_system(asus_rog_ally_device))
> +			use_ally_mcu_hack = ASUS_WMI_ALLY_MCU_HACK_ENABLED;
> +		if (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 +4780,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 +4965,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 +5005,34 @@ static int asus_hotk_restore(struct device *device)
>  	return 0;
>  }
>  
> +static void asus_ally_s2idle_restore(void)
> +{
> +	if (use_ally_mcu_hack == ASUS_WMI_ALLY_MCU_HACK_ENABLED) {
> +		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 == ASUS_WMI_ALLY_MCU_HACK_ENABLED) {
> +		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 +5060,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 +5096,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..8a515179113d 100644
> --- a/include/linux/platform_data/x86/asus-wmi.h
> +++ b/include/linux/platform_data/x86/asus-wmi.h
> @@ -157,9 +157,28 @@
>  #define ASUS_WMI_DSTS_MAX_BRIGTH_MASK	0x0000FF00
>  #define ASUS_WMI_DSTS_LIGHTBAR_MASK	0x0000000F
>  
> +enum asus_ally_mcu_hack {
> +	ASUS_WMI_ALLY_MCU_HACK_INIT,
> +	ASUS_WMI_ALLY_MCU_HACK_ENABLED,
> +	ASUS_WMI_ALLY_MCU_HACK_DISABLED,
> +};
> +
>  #if IS_REACHABLE(CONFIG_ASUS_WMI)
> +void set_ally_mcu_hack(enum asus_ally_mcu_hack status);
> +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(enum asus_ally_mcu_hack status)
> +{
> +}
> +static inline void set_ally_mcu_powersave(bool enabled)
> +{
> +}
> +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)
>  {
> 

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

[Antheas Kapenekakis]

On Mon, 24 Mar 2025 at 11:34, Luke D. Jones <luke@ljones.dev> wrote:
>
> On 24/03/25 21:11, Antheas Kapenekakis wrote:
> > On Mon, 24 Mar 2025 at 02:41, Luke D. Jones <luke@ljones.dev> wrote:
> >>
> >> On 24/03/25 00:41, Antheas Kapenekakis wrote:
> >>> On Sun, 23 Mar 2025 at 03:34, 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.
> >>>>
> >>>> *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
> >>>
> >>> How did the testing go with this one, especially with mcu_powersave 0?
> >>
> >> Appears to be good. Checked a few reboots with powersave off - it is
> >> setting on as I expect every time. Did modules unload/load also. And
> >> tested with it set off after boot plus suspend resumes.
> >
> > Did you test suspends with mcu_powersave to 0 and rgb on? I had a few
> > issues you can reference the previous version for and I want to see if
> > you have them.
> >
> > Even with powersave set to 1, the RGB does not fade anymore without the quirk
>
> Yes I tested every scenario I could think of. I don't think the fade is
> something to worry about

From my testing, I got it to flash random colors and not disconnect
properly, so if you really want to remove it, you should make sure to
test the version that disables the quirk properly first.

> seems like it happening at all previously was
> just due to suspend being held up for a bit longer and now that the hack

Yes, because Windows does not enter s0i3 instantly, so some devices,
like the Ally units, like the Go S, rely on that for different
purposes. 500ms is perfectly fine for both, and since it happens
during suspend and not resume, provided that the screen has been
turned off, it is transparent. (The Go S gets an APU hang due to very
aggressive TDP tuning; a delay after the sleep entry call and
userspace suspend lets the VRMs cool off a bit)

> is disabled for new FW, it relies fully on Linux suspend (async?
> Honestly it's never been fully clear how async it really is).

The call is at the wrong place unfortunately. That's about it

> I'd rather the faster suspend/resume. And so far I've heard no
> complaints (although my userbase is smaller than bazzites).

Have you deployed the V4 though? Because the behavior with the quirk
is fine. WIthout, it is soso.

> Cheers,
> Luke.
>
> > Antheas
> >
> >> Very much hope this is the end of that particular saga, and with
> >> bazzites help we can hopefully get everyone on November MCU FW or later,
> >> then finally remove the hack completely this year.
> >>
> >> A small side note - I expect ASUS to fully reuse the X hardware, or at
> >> least the bios/acpi/mcu-fw for that new windows handheld they've doing,
> >> so fingers crossed that they actually do, and there will be nomore
> >> suspend issues with current kernels plus this patch.
> >>
> >> Cheers,
> >> Luke.
> >>
> >>>>       - 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.
> >>>>     + V4
> >>>>       - Change use_ally_mcu_hack var to enum to track init state and
> >>>>         prevent a race condition
> >>>>
> >>>> 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            | 133 +++++++++++++++------
> >>>>    include/linux/platform_data/x86/asus-wmi.h |  19 +++
> >>>>    3 files changed, 222 insertions(+), 41 deletions(-)
> >>>>
> >>>> --
> >>>> 2.49.0
> >>>>
> >>
>

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

[Luke D. Jones]

On 25/03/25 01:10, Ilpo Järvinen wrote:
> On Sun, 23 Mar 2025, Luke Jones wrote:
> 
>> 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>
> 
> This series update lost Mario's rev-bys (given in replies to v3). Was that
> intentional??

No it wasn't. I must have messed up with git.

@ Mario would you mind adding again? There are very few changes.

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

[Luke D. Jones]

On 25/03/25 03:46, Antheas Kapenekakis wrote:
> On Mon, 24 Mar 2025 at 11:34, Luke D. Jones <luke@ljones.dev> wrote:
>>
>> On 24/03/25 21:11, Antheas Kapenekakis wrote:
>>> On Mon, 24 Mar 2025 at 02:41, Luke D. Jones <luke@ljones.dev> wrote:
>>>>
>>>> On 24/03/25 00:41, Antheas Kapenekakis wrote:
>>>>> On Sun, 23 Mar 2025 at 03:34, 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.
>>>>>>
>>>>>> *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
>>>>>
>>>>> How did the testing go with this one, especially with mcu_powersave 0?
>>>>
>>>> Appears to be good. Checked a few reboots with powersave off - it is
>>>> setting on as I expect every time. Did modules unload/load also. And
>>>> tested with it set off after boot plus suspend resumes.
>>>
>>> Did you test suspends with mcu_powersave to 0 and rgb on? I had a few
>>> issues you can reference the previous version for and I want to see if
>>> you have them.
>>>
>>> Even with powersave set to 1, the RGB does not fade anymore without the quirk
>>
>> Yes I tested every scenario I could think of. I don't think the fade is
>> something to worry about
> 
>  From my testing, I got it to flash random colors and not disconnect
> properly, so if you really want to remove it, you should make sure to
> test the version that disables the quirk properly first.

Like I said I have. This "random colours" sounds like a userspace issue 
such as the solid/static colour not having set/apply/save after it. 
There is zero in this patch to cause anything like that.

1` hour later: I'm currently doing a very heavy refactor of the 
hid-asus-ally driver and it looks like I've solved both the fade (a very 
noticeable improvement), and the random colour issue. I'll do my best to 
get this done by the middle of weekend (I'm UTC+12) so you can have a 
test - to save you some time I'll have your patch series on top with 
conflicts solved.

>> seems like it happening at all previously was
>> just due to suspend being held up for a bit longer and now that the hack
> 
> Yes, because Windows does not enter s0i3 instantly, so some devices,
> like the Ally units, like the Go S, rely on that for different
> purposes. 500ms is perfectly fine for both, and since it happens
> during suspend and not resume, provided that the screen has been
> turned off, it is transparent. (The Go S gets an APU hang due to very
> aggressive TDP tuning; a delay after the sleep entry call and
> userspace suspend lets the VRMs cool off a bit)
> 
>> is disabled for new FW, it relies fully on Linux suspend (async?
>> Honestly it's never been fully clear how async it really is).
> 
> The call is at the wrong place unfortunately. That's about it
> 
>> I'd rather the faster suspend/resume. And so far I've heard no
>> complaints (although my userbase is smaller than bazzites).
> 
> Have you deployed the V4 though? Because the behavior with the quirk
> is fine. WIthout, it is soso.

I have. I agree it was... meh. But the hid-asus-ally rewrite is solving 
a lot of issues now, and I'm thoroughly testing every scenario and 
applying a lot of lessons learned.

To be clear: Right now this current series is good. The issues 
encountered are solved in hid-asus-ally, so I'm comfortable with merging 
this upstream and I'll try crack on with the new driver.

>> Cheers,
>> Luke.
>>
>>> Antheas
>>>
>>>> Very much hope this is the end of that particular saga, and with
>>>> bazzites help we can hopefully get everyone on November MCU FW or later,
>>>> then finally remove the hack completely this year.
>>>>
>>>> A small side note - I expect ASUS to fully reuse the X hardware, or at
>>>> least the bios/acpi/mcu-fw for that new windows handheld they've doing,
>>>> so fingers crossed that they actually do, and there will be nomore
>>>> suspend issues with current kernels plus this patch.
>>>>
>>>> Cheers,
>>>> Luke.
>>>>
>>>>>>        - 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.
>>>>>>      + V4
>>>>>>        - Change use_ally_mcu_hack var to enum to track init state and
>>>>>>          prevent a race condition
>>>>>>
>>>>>> 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            | 133 +++++++++++++++------
>>>>>>     include/linux/platform_data/x86/asus-wmi.h |  19 +++
>>>>>>     3 files changed, 222 insertions(+), 41 deletions(-)
>>>>>>
>>>>>> --
>>>>>> 2.49.0
>>>>>>
>>>>
>>

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

[Antheas Kapenekakis]

On Tue, 25 Mar 2025 at 00:44, Luke D. Jones <luke@ljones.dev> wrote:
>
> On 25/03/25 03:46, Antheas Kapenekakis wrote:
> > On Mon, 24 Mar 2025 at 11:34, Luke D. Jones <luke@ljones.dev> wrote:
> >>
> >> On 24/03/25 21:11, Antheas Kapenekakis wrote:
> >>> On Mon, 24 Mar 2025 at 02:41, Luke D. Jones <luke@ljones.dev> wrote:
> >>>>
> >>>> On 24/03/25 00:41, Antheas Kapenekakis wrote:
> >>>>> On Sun, 23 Mar 2025 at 03:34, 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.
> >>>>>>
> >>>>>> *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
> >>>>>
> >>>>> How did the testing go with this one, especially with mcu_powersave 0?
> >>>>
> >>>> Appears to be good. Checked a few reboots with powersave off - it is
> >>>> setting on as I expect every time. Did modules unload/load also. And
> >>>> tested with it set off after boot plus suspend resumes.
> >>>
> >>> Did you test suspends with mcu_powersave to 0 and rgb on? I had a few
> >>> issues you can reference the previous version for and I want to see if
> >>> you have them.
> >>>
> >>> Even with powersave set to 1, the RGB does not fade anymore without the quirk
> >>
> >> Yes I tested every scenario I could think of. I don't think the fade is
> >> something to worry about
> >
> >  From my testing, I got it to flash random colors and not disconnect
> > properly, so if you really want to remove it, you should make sure to
> > test the version that disables the quirk properly first.
>
> Like I said I have. This "random colours" sounds like a userspace issue
> such as the solid/static colour not having set/apply/save after it.
> There is zero in this patch to cause anything like that.

No userspace software was running at the time and hhd will always do
b3/b4/b5 at least once. It looks like a firmware bug that is triggered
because of the reorder.

> 1` hour later: I'm currently doing a very heavy refactor of the
> hid-asus-ally driver and it looks like I've solved both the fade (a very
> noticeable improvement), and the random colour issue. I'll do my best to
> get this done by the middle of weekend (I'm UTC+12) so you can have a
> test - to save you some time I'll have your patch series on top with
> conflicts solved.
>
> >> seems like it happening at all previously was
> >> just due to suspend being held up for a bit longer and now that the hack
> >
> > Yes, because Windows does not enter s0i3 instantly, so some devices,
> > like the Ally units, like the Go S, rely on that for different
> > purposes. 500ms is perfectly fine for both, and since it happens
> > during suspend and not resume, provided that the screen has been
> > turned off, it is transparent. (The Go S gets an APU hang due to very
> > aggressive TDP tuning; a delay after the sleep entry call and
> > userspace suspend lets the VRMs cool off a bit)
> >
> >> is disabled for new FW, it relies fully on Linux suspend (async?
> >> Honestly it's never been fully clear how async it really is).
> >
> > The call is at the wrong place unfortunately. That's about it
> >
> >> I'd rather the faster suspend/resume. And so far I've heard no
> >> complaints (although my userbase is smaller than bazzites).
> >
> > Have you deployed the V4 though? Because the behavior with the quirk
> > is fine. WIthout, it is soso.
>
> I have. I agree it was... meh. But the hid-asus-ally rewrite is solving
> a lot of issues now, and I'm thoroughly testing every scenario and
> applying a lot of lessons learned.
>
> To be clear: Right now this current series is good. The issues
> encountered are solved in hid-asus-ally, so I'm comfortable with merging
> this upstream and I'll try crack on with the new driver.
>
> >> Cheers,
> >> Luke.
> >>
> >>> Antheas
> >>>
> >>>> Very much hope this is the end of that particular saga, and with
> >>>> bazzites help we can hopefully get everyone on November MCU FW or later,
> >>>> then finally remove the hack completely this year.
> >>>>
> >>>> A small side note - I expect ASUS to fully reuse the X hardware, or at
> >>>> least the bios/acpi/mcu-fw for that new windows handheld they've doing,
> >>>> so fingers crossed that they actually do, and there will be nomore
> >>>> suspend issues with current kernels plus this patch.
> >>>>
> >>>> Cheers,
> >>>> Luke.
> >>>>
> >>>>>>        - 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.
> >>>>>>      + V4
> >>>>>>        - Change use_ally_mcu_hack var to enum to track init state and
> >>>>>>          prevent a race condition
> >>>>>>
> >>>>>> 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            | 133 +++++++++++++++------
> >>>>>>     include/linux/platform_data/x86/asus-wmi.h |  19 +++
> >>>>>>     3 files changed, 222 insertions(+), 41 deletions(-)
> >>>>>>
> >>>>>> --
> >>>>>> 2.49.0
> >>>>>>
> >>>>
> >>
>

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

[Mario Limonciello]

On 3/22/2025 21:34, 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>
Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>   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 },

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

[Mario Limonciello]

On 3/22/2025 21:34, Luke Jones wrote:
> 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>
Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>   drivers/hid/hid-asus.c                     |   4 +
>   drivers/platform/x86/asus-wmi.c            | 133 +++++++++++++++------
>   include/linux/platform_data/x86/asus-wmi.h |  19 +++
>   3 files changed, 117 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 599c836507ff..4b45e31f0bab 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(ASUS_WMI_ALLY_MCU_HACK_DISABLED);
> +		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..27f11643a00d 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 enum asus_ally_mcu_hack use_ally_mcu_hack = ASUS_WMI_ALLY_MCU_HACK_INIT;
> +
>   /* 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(enum asus_ally_mcu_hack status)
> +{
> +	use_ally_mcu_hack = status;
> +	pr_debug("%s Ally MCU suspend quirk\n",
> +		 status == ASUS_WMI_ALLY_MCU_HACK_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,21 @@ static int asus_wmi_add(struct platform_device *pdev)
>   	if (err)
>   		goto fail_platform;
>   
> +	if (use_ally_mcu_hack == ASUS_WMI_ALLY_MCU_HACK_INIT) {
> +		if (acpi_has_method(NULL, ASUS_USB0_PWR_EC0_CSEE)
> +					&& dmi_check_system(asus_rog_ally_device))
> +			use_ally_mcu_hack = ASUS_WMI_ALLY_MCU_HACK_ENABLED;
> +		if (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 +4780,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 +4965,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 +5005,34 @@ static int asus_hotk_restore(struct device *device)
>   	return 0;
>   }
>   
> +static void asus_ally_s2idle_restore(void)
> +{
> +	if (use_ally_mcu_hack == ASUS_WMI_ALLY_MCU_HACK_ENABLED) {
> +		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 == ASUS_WMI_ALLY_MCU_HACK_ENABLED) {
> +		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 +5060,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 +5096,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..8a515179113d 100644
> --- a/include/linux/platform_data/x86/asus-wmi.h
> +++ b/include/linux/platform_data/x86/asus-wmi.h
> @@ -157,9 +157,28 @@
>   #define ASUS_WMI_DSTS_MAX_BRIGTH_MASK	0x0000FF00
>   #define ASUS_WMI_DSTS_LIGHTBAR_MASK	0x0000000F
>   
> +enum asus_ally_mcu_hack {
> +	ASUS_WMI_ALLY_MCU_HACK_INIT,
> +	ASUS_WMI_ALLY_MCU_HACK_ENABLED,
> +	ASUS_WMI_ALLY_MCU_HACK_DISABLED,
> +};
> +
>   #if IS_REACHABLE(CONFIG_ASUS_WMI)
> +void set_ally_mcu_hack(enum asus_ally_mcu_hack status);
> +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(enum asus_ally_mcu_hack status)
> +{
> +}
> +static inline void set_ally_mcu_powersave(bool enabled)
> +{
> +}
> +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)
>   {

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

[Ilpo Järvinen]

On Sun, 23 Mar 2025 15:34:19 +1300, 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.
> 
> [...]


Thank you for your contribution, it has been applied to my local
review-ilpo-next branch. Note it will show up in the public
platform-drivers-x86/review-ilpo-next branch only once I've pushed my
local branch there, which might take a while.

The list of commits applied:
[1/2] hid-asus: check ROG Ally MCU version and warn
      commit: 6b4585948d1ed226f194195180be09fa24b760ec
[2/2] platform/x86: asus-wmi: Refactor Ally suspend/resume
      commit: c26e7d5d2d89af198b43e4c57b1aff5289f65450

--
 i.

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

[Jiri Slaby]

On 23. 03. 25, 3:34, Luke Jones wrote:
> 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
...
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
...
> @@ -5010,6 +5060,10 @@ static int asus_wmi_probe(struct platform_device *pdev)
>   			return ret;
>   	}
>   
> +	ret = acpi_register_lps0_dev(&asus_ally_s2idle_dev_ops);

This likely causes crashes:
https://bugzilla.suse.com/show_bug.cgi?id=1246924

 > asus-nb-wmi asus-nb-wmi: Detected ATK, not ASUSWMI, use DSTS
 > list_add double add: new=ffffffffc2094460, prev=ffffffffafa5cd20, 
next=ffffffffc2094460.
 > ------------[ cut here ]------------
 > kernel BUG at lib/list_debug.c:35!
...
 > acpi_register_lps0_dev.cold (include/linux/list.h:88 
include/linux/list.h:150 include/linux/list.h:169 
drivers/acpi/x86/s2idle.c:663)
 > asus_wmi_probe (drivers/platform/x86/asus-wmi.c:5045 (discriminator 
1) drivers/platform/x86/asus-wmi.c:5089 (discriminator 1)) asus_wmi
 > platform_probe (drivers/base/platform.c:1404)
 > really_probe (drivers/base/dd.c:579)

It looks like acpi_register_lps0_dev() is called twice with those ops...

Should the above be registered only if it is this RC71 device?

thanks,
-- 
js
suse labs