[PATCH v2 0/1] platform/x86: asus-wmi: disable USB0 hub on ROG Ally before suspend

[PATCH v2 0/1] platform/x86: asus-wmi: disable USB0 hub on ROG Ally before suspend

[Luke D. Jones]

This is a fix for the ROG Ally not being able to use the N-Key device after a suspend/resume cycle.

The root of the issue is that ASUS changed the MCU firmware to dfisconnect the USB0 hub when the
screen is switched off during the s2idle suspend path. I tried many many different tactics to try
and get this s2idle part to work but it seems there are races between this and other subsystems.

What has so far been reliable and consistent is a manual call to the CSEE method that is called in
ACPI by the Microsoft DSM screen off path followed by a short sleep in asus-wmi. The PM prepare
hook looks to be the earliest possible place. A sleep that is too long ends up with USB subsystem
registering a disconnect, and thus on resume the device paths change. Too short and it is unreliable.

Some discussion regarding this mess is at https://gitlab.freedesktop.org/drm/amd/-/issues/2719#note_2181402

Changelog:
- v2:
  - Emit dev_err(), but don't return error codes
  - Add check for MCU power save mode being enabled and disable if active
  - General cleanup and rename some vars/funcs
- v1: https://lore.kernel.org/all/20231124082749.23353-1-luke@ljones.dev/

Luke D. Jones (1):
  platform/x86: asus-wmi: disable USB0 hub on ROG Ally before suspend

 drivers/platform/x86/asus-wmi.c            | 50 ++++++++++++++++++++++
 include/linux/platform_data/x86/asus-wmi.h |  3 ++
 2 files changed, 53 insertions(+)

-- 
2.43.0

[PATCH v2 1/1] platform/x86: asus-wmi: disable USB0 hub on ROG Ally before suspend

[Luke D. Jones]

ASUS have worked around an issue in XInput where it doesn't support USB
selective suspend, which causes suspend issues in Windows. They worked
around this by adjusting the MCU firmware to disable the USB0 hub when
the screen is switched off during the Microsoft DSM suspend path in ACPI.

The issue we have with this however is one of timing - the call the tells
the MCU to this isn't able to complete before suspend is done so we call
this in a prepare() and add a small msleep() to ensure it is done. This
must be done before the screen is switched off to prevent a variety of
possible races.

Further to this the MCU powersave option must also be disabled as it can
cause a number of issues such as:
- unreliable resume connection of N-Key
- complete loss of N-Key if the power is plugged in while suspended
Disabling the powersave option prevents this.

Without this the MCU is unable to initialise itself correctly on resume.

Signed-off-by: Luke D. Jones <luke@ljones.dev>
---
 drivers/platform/x86/asus-wmi.c            | 50 ++++++++++++++++++++++
 include/linux/platform_data/x86/asus-wmi.h |  3 ++
 2 files changed, 53 insertions(+)

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 6a79f16233ab..4ba33dfebfd4 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -16,6 +16,7 @@
 #include <linux/acpi.h>
 #include <linux/backlight.h>
 #include <linux/debugfs.h>
+#include <linux/delay.h>
 #include <linux/dmi.h>
 #include <linux/fb.h>
 #include <linux/hwmon.h>
@@ -132,6 +133,11 @@ module_param(fnlock_default, bool, 0444);
 #define ASUS_SCREENPAD_BRIGHT_MAX 255
 #define ASUS_SCREENPAD_BRIGHT_DEFAULT 60
 
+/* 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 300
+
 static const char * const ashs_ids[] = { "ATK4001", "ATK4002", NULL };
 
 static int throttle_thermal_policy_write(struct asus_wmi *);
@@ -300,6 +306,9 @@ struct asus_wmi {
 
 	bool fnlock_locked;
 
+	/* The ROG Ally device requires the MCU USB device be disconnected before suspend */
+	bool ally_mcu_usb_switch;
+
 	struct asus_wmi_debug debug;
 
 	struct asus_wmi_driver *driver;
@@ -4488,6 +4497,8 @@ static int asus_wmi_add(struct platform_device *pdev)
 	asus->nv_temp_tgt_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_NV_THERM_TARGET);
 	asus->panel_overdrive_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_PANEL_OD);
 	asus->mini_led_mode_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_MINI_LED_MODE);
+	asus->ally_mcu_usb_switch = acpi_has_method(NULL, ASUS_USB0_PWR_EC0_CSEE)
+						&& dmi_match(DMI_BOARD_NAME, "RC71L");
 
 	err = fan_boost_mode_check_present(asus);
 	if (err)
@@ -4654,6 +4665,43 @@ static int asus_hotk_resume(struct device *device)
 		asus_wmi_fnlock_update(asus);
 
 	asus_wmi_tablet_mode_get_state(asus);
+
+	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) {
+		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);
+	int result, err;
+
+	if (asus->ally_mcu_usb_switch) {
+		/* When powersave is enabled it causes many issues with resume of USB hub */
+		result = asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_MCU_POWERSAVE);
+		if (result == 1) {
+			dev_warn(device, "MCU powersave enabled, disabling to prevent resume issues");
+			err = asus_wmi_set_devstate(ASUS_WMI_DEVID_MCU_POWERSAVE, 0, &result);
+			if (err || result != 1)
+				dev_err(device, "Failed to set MCU powersave mode: %d\n", err);
+		}
+		/* 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;
 }
 
@@ -4701,6 +4749,8 @@ 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,
 };
 
 /* Registration ***************************************************************/
diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
index 63e630276499..ab1c7deff118 100644
--- a/include/linux/platform_data/x86/asus-wmi.h
+++ b/include/linux/platform_data/x86/asus-wmi.h
@@ -114,6 +114,9 @@
 /* Charging mode - 1=Barrel, 2=USB */
 #define ASUS_WMI_DEVID_CHARGE_MODE	0x0012006C
 
+/* MCU powersave mode */
+#define ASUS_WMI_DEVID_MCU_POWERSAVE   0x001200E2
+
 /* epu is connected? 1 == true */
 #define ASUS_WMI_DEVID_EGPU_CONNECTED	0x00090018
 /* egpu on/off */
-- 
2.43.0

Re: [PATCH v2 0/1] platform/x86: asus-wmi: disable USB0 hub on ROG Ally before suspend

[Luke Jones]

On Mon, Nov 27 2023 at 12:05:20 PM +13:00:00, Luke D. Jones 
<luke@ljones.dev> wrote:
> This is a fix for the ROG Ally not being able to use the N-Key device 
> after a suspend/resume cycle.
> 
> The root of the issue is that ASUS changed the MCU firmware to 
> dfisconnect the USB0 hub when the
> screen is switched off during the s2idle suspend path. I tried many 
> many different tactics to try
> and get this s2idle part to work but it seems there are races between 
> this and other subsystems.
> 
> What has so far been reliable and consistent is a manual call to the 
> CSEE method that is called in
> ACPI by the Microsoft DSM screen off path followed by a short sleep 
> in asus-wmi. The PM prepare
> hook looks to be the earliest possible place. A sleep that is too 
> long ends up with USB subsystem
> registering a disconnect, and thus on resume the device paths change. 
> Too short and it is unreliable.
> 
> Some discussion regarding this mess is at 
> https://gitlab.freedesktop.org/drm/amd/-/issues/2719#note_2181402
> 
> Changelog:
> - v2:
>   - Emit dev_err(), but don't return error codes
>   - Add check for MCU power save mode being enabled and disable if 
> active
>   - General cleanup and rename some vars/funcs
> - v1: 
> https://lore.kernel.org/all/20231124082749.23353-1-luke@ljones.dev/
> 
> Luke D. Jones (1):
>   platform/x86: asus-wmi: disable USB0 hub on ROG Ally before suspend
> 
>  drivers/platform/x86/asus-wmi.c            | 50 
> ++++++++++++++++++++++
>  include/linux/platform_data/x86/asus-wmi.h |  3 ++
>  2 files changed, 53 insertions(+)
> 
> --
> 2.43.0
> 

Hans I omitted your review tag as I made more changes than requested.

Re: [PATCH v2 1/1] platform/x86: asus-wmi: disable USB0 hub on ROG Ally before suspend

[Philip Müller]

On 27.11.23 06:05, Luke D. Jones wrote:
> ASUS have worked around an issue in XInput where it doesn't support USB
> selective suspend, which causes suspend issues in Windows. They worked
> around this by adjusting the MCU firmware to disable the USB0 hub when
> the screen is switched off during the Microsoft DSM suspend path in ACPI.
> 
> The issue we have with this however is one of timing - the call the tells
> the MCU to this isn't able to complete before suspend is done so we call
> this in a prepare() and add a small msleep() to ensure it is done. This
> must be done before the screen is switched off to prevent a variety of
> possible races.
> 
> Further to this the MCU powersave option must also be disabled as it can
> cause a number of issues such as:
> - unreliable resume connection of N-Key
> - complete loss of N-Key if the power is plugged in while suspended
> Disabling the powersave option prevents this.
> 
> Without this the MCU is unable to initialise itself correctly on resume.
> 
> Signed-off-by: Luke D. Jones <luke@ljones.dev>
> ---
>   drivers/platform/x86/asus-wmi.c            | 50 ++++++++++++++++++++++
>   include/linux/platform_data/x86/asus-wmi.h |  3 ++
>   2 files changed, 53 insertions(+)
> 
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index 6a79f16233ab..4ba33dfebfd4 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -16,6 +16,7 @@
>   #include <linux/acpi.h>
>   #include <linux/backlight.h>
>   #include <linux/debugfs.h>
> +#include <linux/delay.h>
>   #include <linux/dmi.h>
>   #include <linux/fb.h>
>   #include <linux/hwmon.h>
> @@ -132,6 +133,11 @@ module_param(fnlock_default, bool, 0444);
>   #define ASUS_SCREENPAD_BRIGHT_MAX 255
>   #define ASUS_SCREENPAD_BRIGHT_DEFAULT 60
>   
> +/* 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 300
> +
>   static const char * const ashs_ids[] = { "ATK4001", "ATK4002", NULL };
>   
>   static int throttle_thermal_policy_write(struct asus_wmi *);
> @@ -300,6 +306,9 @@ struct asus_wmi {
>   
>   	bool fnlock_locked;
>   
> +	/* The ROG Ally device requires the MCU USB device be disconnected before suspend */
> +	bool ally_mcu_usb_switch;
> +
>   	struct asus_wmi_debug debug;
>   
>   	struct asus_wmi_driver *driver;
> @@ -4488,6 +4497,8 @@ static int asus_wmi_add(struct platform_device *pdev)
>   	asus->nv_temp_tgt_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_NV_THERM_TARGET);
>   	asus->panel_overdrive_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_PANEL_OD);
>   	asus->mini_led_mode_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_MINI_LED_MODE);
> +	asus->ally_mcu_usb_switch = acpi_has_method(NULL, ASUS_USB0_PWR_EC0_CSEE)
> +						&& dmi_match(DMI_BOARD_NAME, "RC71L");
>   
>   	err = fan_boost_mode_check_present(asus);
>   	if (err)
> @@ -4654,6 +4665,43 @@ static int asus_hotk_resume(struct device *device)
>   		asus_wmi_fnlock_update(asus);
>   
>   	asus_wmi_tablet_mode_get_state(asus);
> +
> +	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) {
> +		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);
> +	int result, err;
> +
> +	if (asus->ally_mcu_usb_switch) {
> +		/* When powersave is enabled it causes many issues with resume of USB hub */
> +		result = asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_MCU_POWERSAVE);
> +		if (result == 1) {
> +			dev_warn(device, "MCU powersave enabled, disabling to prevent resume issues");
> +			err = asus_wmi_set_devstate(ASUS_WMI_DEVID_MCU_POWERSAVE, 0, &result);
> +			if (err || result != 1)
> +				dev_err(device, "Failed to set MCU powersave mode: %d\n", err);
> +		}
> +		/* 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;
>   }
>   
> @@ -4701,6 +4749,8 @@ 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,
>   };
>   
>   /* Registration ***************************************************************/
> diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
> index 63e630276499..ab1c7deff118 100644
> --- a/include/linux/platform_data/x86/asus-wmi.h
> +++ b/include/linux/platform_data/x86/asus-wmi.h
> @@ -114,6 +114,9 @@
>   /* Charging mode - 1=Barrel, 2=USB */
>   #define ASUS_WMI_DEVID_CHARGE_MODE	0x0012006C
>   
> +/* MCU powersave mode */
> +#define ASUS_WMI_DEVID_MCU_POWERSAVE   0x001200E2
> +
>   /* epu is connected? 1 == true */
>   #define ASUS_WMI_DEVID_EGPU_CONNECTED	0x00090018
>   /* egpu on/off */

Tested-by: Philip Mueller <philm@manjaro.org>

Re: [PATCH v2 1/1] platform/x86: asus-wmi: disable USB0 hub on ROG Ally before suspend

[Hans de Goede]

Hi,

On 11/27/23 00:05, Luke D. Jones wrote:
> ASUS have worked around an issue in XInput where it doesn't support USB
> selective suspend, which causes suspend issues in Windows. They worked
> around this by adjusting the MCU firmware to disable the USB0 hub when
> the screen is switched off during the Microsoft DSM suspend path in ACPI.
> 
> The issue we have with this however is one of timing - the call the tells
> the MCU to this isn't able to complete before suspend is done so we call
> this in a prepare() and add a small msleep() to ensure it is done. This
> must be done before the screen is switched off to prevent a variety of
> possible races.
> 
> Further to this the MCU powersave option must also be disabled as it can
> cause a number of issues such as:
> - unreliable resume connection of N-Key
> - complete loss of N-Key if the power is plugged in while suspended
> Disabling the powersave option prevents this.
> 
> Without this the MCU is unable to initialise itself correctly on resume.
> 
> Signed-off-by: Luke D. Jones <luke@ljones.dev>

Thanks, patch looks good to me, except that all the new lines
seem to use 4 spaces rather then a tab char as indent.

With that fixed you can add my:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

to the next version.

Regards,

Hans


> ---
>  drivers/platform/x86/asus-wmi.c            | 50 ++++++++++++++++++++++
>  include/linux/platform_data/x86/asus-wmi.h |  3 ++
>  2 files changed, 53 insertions(+)
> 
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index 6a79f16233ab..4ba33dfebfd4 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -16,6 +16,7 @@
>  #include <linux/acpi.h>
>  #include <linux/backlight.h>
>  #include <linux/debugfs.h>
> +#include <linux/delay.h>
>  #include <linux/dmi.h>
>  #include <linux/fb.h>
>  #include <linux/hwmon.h>
> @@ -132,6 +133,11 @@ module_param(fnlock_default, bool, 0444);
>  #define ASUS_SCREENPAD_BRIGHT_MAX 255
>  #define ASUS_SCREENPAD_BRIGHT_DEFAULT 60
>  
> +/* 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 300
> +
>  static const char * const ashs_ids[] = { "ATK4001", "ATK4002", NULL };
>  
>  static int throttle_thermal_policy_write(struct asus_wmi *);
> @@ -300,6 +306,9 @@ struct asus_wmi {
>  
>  	bool fnlock_locked;
>  
> +	/* The ROG Ally device requires the MCU USB device be disconnected before suspend */
> +	bool ally_mcu_usb_switch;
> +
>  	struct asus_wmi_debug debug;
>  
>  	struct asus_wmi_driver *driver;
> @@ -4488,6 +4497,8 @@ static int asus_wmi_add(struct platform_device *pdev)
>  	asus->nv_temp_tgt_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_NV_THERM_TARGET);
>  	asus->panel_overdrive_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_PANEL_OD);
>  	asus->mini_led_mode_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_MINI_LED_MODE);
> +	asus->ally_mcu_usb_switch = acpi_has_method(NULL, ASUS_USB0_PWR_EC0_CSEE)
> +						&& dmi_match(DMI_BOARD_NAME, "RC71L");
>  
>  	err = fan_boost_mode_check_present(asus);
>  	if (err)
> @@ -4654,6 +4665,43 @@ static int asus_hotk_resume(struct device *device)
>  		asus_wmi_fnlock_update(asus);
>  
>  	asus_wmi_tablet_mode_get_state(asus);
> +
> +	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) {
> +		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);
> +	int result, err;
> +
> +	if (asus->ally_mcu_usb_switch) {
> +		/* When powersave is enabled it causes many issues with resume of USB hub */
> +		result = asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_MCU_POWERSAVE);
> +		if (result == 1) {
> +			dev_warn(device, "MCU powersave enabled, disabling to prevent resume issues");
> +			err = asus_wmi_set_devstate(ASUS_WMI_DEVID_MCU_POWERSAVE, 0, &result);
> +			if (err || result != 1)
> +				dev_err(device, "Failed to set MCU powersave mode: %d\n", err);
> +		}
> +		/* 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;
>  }
>  
> @@ -4701,6 +4749,8 @@ 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,
>  };
>  
>  /* Registration ***************************************************************/
> diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
> index 63e630276499..ab1c7deff118 100644
> --- a/include/linux/platform_data/x86/asus-wmi.h
> +++ b/include/linux/platform_data/x86/asus-wmi.h
> @@ -114,6 +114,9 @@
>  /* Charging mode - 1=Barrel, 2=USB */
>  #define ASUS_WMI_DEVID_CHARGE_MODE	0x0012006C
>  
> +/* MCU powersave mode */
> +#define ASUS_WMI_DEVID_MCU_POWERSAVE   0x001200E2
> +
>  /* epu is connected? 1 == true */
>  #define ASUS_WMI_DEVID_EGPU_CONNECTED	0x00090018
>  /* egpu on/off */

Re: [PATCH v2 1/1] platform/x86: asus-wmi: disable USB0 hub on ROG Ally before suspend

[Mario Limonciello]

On 11/26/2023 17:05, Luke D. Jones wrote:
> ASUS have worked around an issue in XInput where it doesn't support USB
> selective suspend, which causes suspend issues in Windows. They worked
> around this by adjusting the MCU firmware to disable the USB0 hub when
> the screen is switched off during the Microsoft DSM suspend path in ACPI.
> 
> The issue we have with this however is one of timing - the call the tells
> the MCU to this isn't able to complete before suspend is done so we call
> this in a prepare() and add a small msleep() to ensure it is done. This
> must be done before the screen is switched off to prevent a variety of
> possible races.

Right now the way that Linux handles the LPS0 calls is that they're all 
back to back.  Luke did try to inject a delay after the LPS0 calls were 
done but before it went to sleep but this wasn't sufficient.

Another "potential" way to solve this problem from Linux may be to 
actually glue the LPS0 screen off call to when DRM actually has eDP 
turned off.

Making such a change would essentially push back the "screen off" LPS0 
command to when the user has run 'systemctl suspend' (or an action that 
did this) because the compositor usually turns it off with DPMS at this 
time.

This is a much bigger change though and *much more ripe for breakage*.

So I think in may be worth leaving a TODO comment to look into doing 
that in the future.

If that ever happens; it's possible that this change could be reverted too.

> 
> Further to this the MCU powersave option must also be disabled as it can
> cause a number of issues such as:
> - unreliable resume connection of N-Key
> - complete loss of N-Key if the power is plugged in while suspended
> Disabling the powersave option prevents this.
> 
> Without this the MCU is unable to initialise itself correctly on resume.

initialize

> 
> Signed-off-by: Luke D. Jones <luke@ljones.dev>

I think it would be good to add a Closes: tag to the AMD Gitlab issue 
that this was discussed within as well as any other public references 
you know about.

Additionally as Phoenix APU support goes back as far as kernel 6.1 and 
this is well contained to only run on the ROG I suggest to CC stable so 
that people can use the ROG on that LTS kernel or later.

> ---
>   drivers/platform/x86/asus-wmi.c            | 50 ++++++++++++++++++++++
>   include/linux/platform_data/x86/asus-wmi.h |  3 ++
>   2 files changed, 53 insertions(+)
> 
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index 6a79f16233ab..4ba33dfebfd4 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -16,6 +16,7 @@
>   #include <linux/acpi.h>
>   #include <linux/backlight.h>
>   #include <linux/debugfs.h>
> +#include <linux/delay.h>
>   #include <linux/dmi.h>
>   #include <linux/fb.h>
>   #include <linux/hwmon.h>
> @@ -132,6 +133,11 @@ module_param(fnlock_default, bool, 0444);
>   #define ASUS_SCREENPAD_BRIGHT_MAX 255
>   #define ASUS_SCREENPAD_BRIGHT_DEFAULT 60
>   
> +/* 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 300
> +
>   static const char * const ashs_ids[] = { "ATK4001", "ATK4002", NULL };
>   
>   static int throttle_thermal_policy_write(struct asus_wmi *);
> @@ -300,6 +306,9 @@ struct asus_wmi {
>   
>   	bool fnlock_locked;
>   
> +	/* The ROG Ally device requires the MCU USB device be disconnected before suspend */
> +	bool ally_mcu_usb_switch;
> +
>   	struct asus_wmi_debug debug;
>   
>   	struct asus_wmi_driver *driver;
> @@ -4488,6 +4497,8 @@ static int asus_wmi_add(struct platform_device *pdev)
>   	asus->nv_temp_tgt_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_NV_THERM_TARGET);
>   	asus->panel_overdrive_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_PANEL_OD);
>   	asus->mini_led_mode_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_MINI_LED_MODE);
> +	asus->ally_mcu_usb_switch = acpi_has_method(NULL, ASUS_USB0_PWR_EC0_CSEE)
> +						&& dmi_match(DMI_BOARD_NAME, "RC71L");
>   
>   	err = fan_boost_mode_check_present(asus);
>   	if (err)
> @@ -4654,6 +4665,43 @@ static int asus_hotk_resume(struct device *device)
>   		asus_wmi_fnlock_update(asus);
>   
>   	asus_wmi_tablet_mode_get_state(asus);
> +
> +	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) {
> +		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);
> +	int result, err;
> +
> +	if (asus->ally_mcu_usb_switch) {
> +		/* When powersave is enabled it causes many issues with resume of USB hub */
> +		result = asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_MCU_POWERSAVE);
> +		if (result == 1) {
> +			dev_warn(device, "MCU powersave enabled, disabling to prevent resume issues");
> +			err = asus_wmi_set_devstate(ASUS_WMI_DEVID_MCU_POWERSAVE, 0, &result);
> +			if (err || result != 1)
> +				dev_err(device, "Failed to set MCU powersave mode: %d\n", err);
> +		}
> +		/* 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;
>   }
>   
> @@ -4701,6 +4749,8 @@ 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,

Have you experimented with only using the prepare() call or only the 
resume_early() call?  Are both really needed?

>   };
>   
>   /* Registration ***************************************************************/
> diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
> index 63e630276499..ab1c7deff118 100644
> --- a/include/linux/platform_data/x86/asus-wmi.h
> +++ b/include/linux/platform_data/x86/asus-wmi.h
> @@ -114,6 +114,9 @@
>   /* Charging mode - 1=Barrel, 2=USB */
>   #define ASUS_WMI_DEVID_CHARGE_MODE	0x0012006C
>   
> +/* MCU powersave mode */
> +#define ASUS_WMI_DEVID_MCU_POWERSAVE   0x001200E2
> +
>   /* epu is connected? 1 == true */
>   #define ASUS_WMI_DEVID_EGPU_CONNECTED	0x00090018
>   /* egpu on/off */

Re: [PATCH v2 1/1] platform/x86: asus-wmi: disable USB0 hub on ROG Ally before suspend

[Luke Jones]

On Mon, Nov 27 2023 at 09:53:13 AM +01:00:00, Hans de Goede 
<hdegoede@redhat.com> wrote:
> Hi,
> 
> On 11/27/23 00:05, Luke D. Jones wrote:
>>  ASUS have worked around an issue in XInput where it doesn't support 
>> USB
>>  selective suspend, which causes suspend issues in Windows. They 
>> worked
>>  around this by adjusting the MCU firmware to disable the USB0 hub 
>> when
>>  the screen is switched off during the Microsoft DSM suspend path in 
>> ACPI.
>> 
>>  The issue we have with this however is one of timing - the call the 
>> tells
>>  the MCU to this isn't able to complete before suspend is done so we 
>> call
>>  this in a prepare() and add a small msleep() to ensure it is done. 
>> This
>>  must be done before the screen is switched off to prevent a variety 
>> of
>>  possible races.
>> 
>>  Further to this the MCU powersave option must also be disabled as 
>> it can
>>  cause a number of issues such as:
>>  - unreliable resume connection of N-Key
>>  - complete loss of N-Key if the power is plugged in while suspended
>>  Disabling the powersave option prevents this.
>> 
>>  Without this the MCU is unable to initialise itself correctly on 
>> resume.
>> 
>>  Signed-off-by: Luke D. Jones <luke@ljones.dev>
> 
> Thanks, patch looks good to me, except that all the new lines
> seem to use 4 spaces rather then a tab char as indent.

Apologies for the previous HTML email.
I must be going mad... are you sure? I've checked the patch file I 
submitted. Run checkpatch on it. Checked my email copy, and checked in 
lore... I can't see where space chars are?

> 
> With that fixed you can add my:
> 
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> 
> to the next version.
> 
> Regards,
> 
> Hans
> 
> 
>>  ---
>>   drivers/platform/x86/asus-wmi.c            | 50 
>> ++++++++++++++++++++++
>>   include/linux/platform_data/x86/asus-wmi.h |  3 ++
>>   2 files changed, 53 insertions(+)
>> 
>>  diff --git a/drivers/platform/x86/asus-wmi.c 
>> b/drivers/platform/x86/asus-wmi.c
>>  index 6a79f16233ab..4ba33dfebfd4 100644
>>  --- a/drivers/platform/x86/asus-wmi.c
>>  +++ b/drivers/platform/x86/asus-wmi.c
>>  @@ -16,6 +16,7 @@
>>   #include <linux/acpi.h>
>>   #include <linux/backlight.h>
>>   #include <linux/debugfs.h>
>>  +#include <linux/delay.h>
>>   #include <linux/dmi.h>
>>   #include <linux/fb.h>
>>   #include <linux/hwmon.h>
>>  @@ -132,6 +133,11 @@ module_param(fnlock_default, bool, 0444);
>>   #define ASUS_SCREENPAD_BRIGHT_MAX 255
>>   #define ASUS_SCREENPAD_BRIGHT_DEFAULT 60
>> 
>>  +/* 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 300
>>  +
>>   static const char * const ashs_ids[] = { "ATK4001", "ATK4002", 
>> NULL };
>> 
>>   static int throttle_thermal_policy_write(struct asus_wmi *);
>>  @@ -300,6 +306,9 @@ struct asus_wmi {
>> 
>>   	bool fnlock_locked;
>> 
>>  +	/* The ROG Ally device requires the MCU USB device be 
>> disconnected before suspend */
>>  +	bool ally_mcu_usb_switch;
>>  +
>>   	struct asus_wmi_debug debug;
>> 
>>   	struct asus_wmi_driver *driver;
>>  @@ -4488,6 +4497,8 @@ static int asus_wmi_add(struct 
>> platform_device *pdev)
>>   	asus->nv_temp_tgt_available = asus_wmi_dev_is_present(asus, 
>> ASUS_WMI_DEVID_NV_THERM_TARGET);
>>   	asus->panel_overdrive_available = asus_wmi_dev_is_present(asus, 
>> ASUS_WMI_DEVID_PANEL_OD);
>>   	asus->mini_led_mode_available = asus_wmi_dev_is_present(asus, 
>> ASUS_WMI_DEVID_MINI_LED_MODE);
>>  +	asus->ally_mcu_usb_switch = acpi_has_method(NULL, 
>> ASUS_USB0_PWR_EC0_CSEE)
>>  +						&& dmi_match(DMI_BOARD_NAME, "RC71L");
>> 
>>   	err = fan_boost_mode_check_present(asus);
>>   	if (err)
>>  @@ -4654,6 +4665,43 @@ static int asus_hotk_resume(struct device 
>> *device)
>>   		asus_wmi_fnlock_update(asus);
>> 
>>   	asus_wmi_tablet_mode_get_state(asus);
>>  +
>>  +	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) {
>>  +		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);
>>  +	int result, err;
>>  +
>>  +	if (asus->ally_mcu_usb_switch) {
>>  +		/* When powersave is enabled it causes many issues with resume 
>> of USB hub */
>>  +		result = asus_wmi_get_devstate_simple(asus, 
>> ASUS_WMI_DEVID_MCU_POWERSAVE);
>>  +		if (result == 1) {
>>  +			dev_warn(device, "MCU powersave enabled, disabling to prevent 
>> resume issues");
>>  +			err = asus_wmi_set_devstate(ASUS_WMI_DEVID_MCU_POWERSAVE, 0, 
>> &result);
>>  +			if (err || result != 1)
>>  +				dev_err(device, "Failed to set MCU powersave mode: %d\n", err);
>>  +		}
>>  +		/* 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;
>>   }
>> 
>>  @@ -4701,6 +4749,8 @@ 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,
>>   };
>> 
>>   /* Registration 
>> ***************************************************************/
>>  diff --git a/include/linux/platform_data/x86/asus-wmi.h 
>> b/include/linux/platform_data/x86/asus-wmi.h
>>  index 63e630276499..ab1c7deff118 100644
>>  --- a/include/linux/platform_data/x86/asus-wmi.h
>>  +++ b/include/linux/platform_data/x86/asus-wmi.h
>>  @@ -114,6 +114,9 @@
>>   /* Charging mode - 1=Barrel, 2=USB */
>>   #define ASUS_WMI_DEVID_CHARGE_MODE	0x0012006C
>> 
>>  +/* MCU powersave mode */
>>  +#define ASUS_WMI_DEVID_MCU_POWERSAVE   0x001200E2
>>  +
>>   /* epu is connected? 1 == true */
>>   #define ASUS_WMI_DEVID_EGPU_CONNECTED	0x00090018
>>   /* egpu on/off */
> 

Re: [PATCH v2 1/1] platform/x86: asus-wmi: disable USB0 hub on ROG Ally before suspend

[Luke Jones]

On Mon, Nov 27 2023 at 02:14:23 PM -06:00:00, Mario Limonciello 
<mario.limonciello@amd.com> wrote:
> On 11/26/2023 17:05, Luke D. Jones wrote:
>> ASUS have worked around an issue in XInput where it doesn't support 
>> USB
>> selective suspend, which causes suspend issues in Windows. They 
>> worked
>> around this by adjusting the MCU firmware to disable the USB0 hub 
>> when
>> the screen is switched off during the Microsoft DSM suspend path in 
>> ACPI.
>> 
>> The issue we have with this however is one of timing - the call the 
>> tells
>> the MCU to this isn't able to complete before suspend is done so we 
>> call
>> this in a prepare() and add a small msleep() to ensure it is done. 
>> This
>> must be done before the screen is switched off to prevent a variety 
>> of
>> possible races.
> 
> Right now the way that Linux handles the LPS0 calls is that they're 
> all back to back.  Luke did try to inject a delay after the LPS0 
> calls were done but before it went to sleep but this wasn't 
> sufficient.
> 
> Another "potential" way to solve this problem from Linux may be to 
> actually glue the LPS0 screen off call to when DRM actually has eDP 
> turned off.
> 
> Making such a change would essentially push back the "screen off" 
> LPS0 command to when the user has run 'systemctl suspend' (or an 
> action that did this) because the compositor usually turns it off 
> with DPMS at this time.

I would be willing to test this if you want some concrete data. See my 
big block of text below.

> 
> This is a much bigger change though and *much more ripe for breakage*.
> 
> So I think in may be worth leaving a TODO comment to look into doing 
> that in the future.
Do you mean add the TODO to a line in this patch?

> 
> If that ever happens; it's possible that this change could be 
> reverted too.
> 
>> 
>> Further to this the MCU powersave option must also be disabled as it 
>> can
>> cause a number of issues such as:
>> - unreliable resume connection of N-Key
>> - complete loss of N-Key if the power is plugged in while suspended
>> Disabling the powersave option prevents this.
>> 
>> Without this the MCU is unable to initialise itself correctly on 
>> resume.
> 
> initialize

Are we forced to use USA spelling? I'm from NZ
"initialise is predominantly used in British English (used in UK/AU/NZ) 
( en-GB )"

> 
>> 
>> Signed-off-by: Luke D. Jones <luke@ljones.dev>
> 
> I think it would be good to add a Closes: tag to the AMD Gitlab issue 
> that this was discussed within as well as any other public references 
> you know about.
> 
> Additionally as Phoenix APU support goes back as far as kernel 6.1 
> and this is well contained to only run on the ROG I suggest to CC 
> stable so that people can use the ROG on that LTS kernel or later.
> 
>> ---
>> -SNIP-
>>   \x7f@@ -4701,6 +4749,8 @@ 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,
> 
> Have you experimented with only using the prepare() call or only the 
> resume_early() call?  Are both really needed?

I have yes. Although the device comes back eventually in resume after 
only a prepare call it's not preferable as it tends to change the 
device path. With resume_early we can get the device replugged super 
early (before anything notices it's gone in fact).

This whole thing is a bit of a mess. It ends up being a race between 
various things to prevent a HUB0 disconnect being registered by the 
xhci subsystem, and adding the device back before the xhci subsystem 
gets control.

If I add a sleep longer than 1300ms in prepare then the xhci subsys 
registers a disconnect of the USB0 hub. If the sleep is under 250ms it 
isn't quite enough for the MCU to do its thing, and on battery it seems 
worse.

I have asked the ASUS guys I'm in contact with for something to disable 
this MCU behaviour since it is purely a workaround for a broken Windows 
thing :( They are open to something, maybe an OS detect in ACPI or a 
WMI method addition similar to the MCU powersave method, from what I'm 
told it would require an MCU firmware update along with BIOS update. If 
this eventuates I'll submit an additional patch to check and set that 
plus disable this.

I may possibly write a new version of this patch as we've seen that 
enabling powersave reduces suspend power use by at least half. And 
looking through my DSDT dumps, there are a few laptops with the same 
feature as Ally. The patch for powersave being enabled requires also AC 
power state on suspend change detection, and a later forced reset in 
late resume (and the device paths change regardless when powersave is 
on).

When I look at it objectively, the device path changing should be a 
non-issue really as it is fully handled by USB subsystem and behaves 
exactly like what it is - a USB hub disconnect. It's just that some 
userspace apps don't expect this. I will experiment some more.

Regards,
Luke.

> 
>>   };
>>   \x7f  /* Registration 
>> ***************************************************************/
>> diff --git a/include/linux/platform_data/x86/asus-wmi.h 
>> b/include/linux/platform_data/x86/asus-wmi.h
>> index 63e630276499..ab1c7deff118 100644
>> --- a/include/linux/platform_data/x86/asus-wmi.h
>> +++ b/include/linux/platform_data/x86/asus-wmi.h
>> @@ -114,6 +114,9 @@
>>   /* Charging mode - 1=Barrel, 2=USB */
>>   #define ASUS_WMI_DEVID_CHARGE_MODE	0x0012006C
>>   \x7f+/* MCU powersave mode */
>> +#define ASUS_WMI_DEVID_MCU_POWERSAVE   0x001200E2
>> +
>>   /* epu is connected? 1 == true */
>>   #define ASUS_WMI_DEVID_EGPU_CONNECTED	0x00090018
>>   /* egpu on/off */
> 

Re: [PATCH v2 1/1] platform/x86: asus-wmi: disable USB0 hub on ROG Ally before suspend

[Mario Limonciello]

On 11/27/2023 14:46, Luke Jones wrote:
> 
> 
> On Mon, Nov 27 2023 at 02:14:23 PM -06:00:00, Mario Limonciello 
> <mario.limonciello@amd.com> wrote:
>> On 11/26/2023 17:05, Luke D. Jones wrote:
>>> ASUS have worked around an issue in XInput where it doesn't support USB
>>> selective suspend, which causes suspend issues in Windows. They worked
>>> around this by adjusting the MCU firmware to disable the USB0 hub when
>>> the screen is switched off during the Microsoft DSM suspend path in 
>>> ACPI.
>>>
>>> The issue we have with this however is one of timing - the call the 
>>> tells
>>> the MCU to this isn't able to complete before suspend is done so we call
>>> this in a prepare() and add a small msleep() to ensure it is done. This
>>> must be done before the screen is switched off to prevent a variety of
>>> possible races.
>>
>> Right now the way that Linux handles the LPS0 calls is that they're 
>> all back to back.  Luke did try to inject a delay after the LPS0 calls 
>> were done but before it went to sleep but this wasn't sufficient.
>>
>> Another "potential" way to solve this problem from Linux may be to 
>> actually glue the LPS0 screen off call to when DRM actually has eDP 
>> turned off.
>>
>> Making such a change would essentially push back the "screen off" LPS0 
>> command to when the user has run 'systemctl suspend' (or an action 
>> that did this) because the compositor usually turns it off with DPMS 
>> at this time.
> 
> I would be willing to test this if you want some concrete data. 

It would require some cross subsystem plumbing to evaluate feasibility.
I don't currently have any plans to do it.

I think your patch makes sense; I just want to make it known that 
"might" clean this up if it ever happens.

> See my 
> big block of text below.
> 
>>
>> This is a much bigger change though and *much more ripe for breakage*.
>>
>> So I think in may be worth leaving a TODO comment to look into doing 
>> that in the future.
> Do you mean add the TODO to a line in this patch?

Yeah.  In case someone ever does it (me or otherwise) I think it would 
be good to have some reference in the comments that the commit 'might' 
be possible to revert.

> 
>>
>> If that ever happens; it's possible that this change could be reverted 
>> too.
>>
>>>
>>> Further to this the MCU powersave option must also be disabled as it can
>>> cause a number of issues such as:
>>> - unreliable resume connection of N-Key
>>> - complete loss of N-Key if the power is plugged in while suspended
>>> Disabling the powersave option prevents this.
>>>
>>> Without this the MCU is unable to initialise itself correctly on resume.
>>
>> initialize
> 
> Are we forced to use USA spelling? I'm from NZ
> "initialise is predominantly used in British English (used in UK/AU/NZ) 
> ( en-GB )"
> 

Ah I didn't realize it's an acceptable spelling for en-GB, and thought 
it was just a typo; sorry.

>>
>>>
>>> Signed-off-by: Luke D. Jones <luke@ljones.dev>
>>
>> I think it would be good to add a Closes: tag to the AMD Gitlab issue 
>> that this was discussed within as well as any other public references 
>> you know about.
>>
>> Additionally as Phoenix APU support goes back as far as kernel 6.1 and 
>> this is well contained to only run on the ROG I suggest to CC stable 
>> so that people can use the ROG on that LTS kernel or later.
>>
>>> ---
>>> -SNIP-
>>>   \x7f@@ -4701,6 +4749,8 @@ 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,
>>
>> Have you experimented with only using the prepare() call or only the 
>> resume_early() call?  Are both really needed?
> 
> I have yes. Although the device comes back eventually in resume after 
> only a prepare call it's not preferable as it tends to change the device 
> path. With resume_early we can get the device replugged super early 
> (before anything notices it's gone in fact).
> 
> This whole thing is a bit of a mess. It ends up being a race between 
> various things to prevent a HUB0 disconnect being registered by the xhci 
> subsystem, and adding the device back before the xhci subsystem gets 
> control.
> 
> If I add a sleep longer than 1300ms in prepare then the xhci subsys 
> registers a disconnect of the USB0 hub. If the sleep is under 250ms it 
> isn't quite enough for the MCU to do its thing, and on battery it seems 
> worse.
> 
> I have asked the ASUS guys I'm in contact with for something to disable 
> this MCU behaviour since it is purely a workaround for a broken Windows 
> thing :( They are open to something, maybe an OS detect in ACPI or a WMI 
> method addition similar to the MCU powersave method, from what I'm told 
> it would require an MCU firmware update along with BIOS update. If this 
> eventuates I'll submit an additional patch to check and set that plus 
> disable this.

Don't let them do an OS detection in ACPI, it's going to be too painful.
I would instead suggest that they can have a bit that you can program in 
via ACPI or WMI from the ASUS WMI driver that says to skip the MCU 
disconnect behavior.

> 
> I may possibly write a new version of this patch as we've seen that 
> enabling powersave reduces suspend power use by at least half. And 
> looking through my DSDT dumps, there are a few laptops with the same 
> feature as Ally. The patch for powersave being enabled requires also AC 
> power state on suspend change detection, and a later forced reset in 
> late resume (and the device paths change regardless when powersave is on).
> 
> When I look at it objectively, the device path changing should be a 
> non-issue really as it is fully handled by USB subsystem and behaves 
> exactly like what it is - a USB hub disconnect. It's just that some 
> userspace apps don't expect this. I will experiment some more.
> 
> Regards,
> Luke.
> 

As another experiment - what happens if you "comment out" the LPS0 calls 
that do this problematic stuff?

It's important to make sure the callback to amd-pmc stays in place, but 
if you just skip those ACPI ones does it still get to the deepest state 
and are there other problems?

>>
>>>   };
>>>   \x7f  /* Registration 
>>> ***************************************************************/
>>> diff --git a/include/linux/platform_data/x86/asus-wmi.h 
>>> b/include/linux/platform_data/x86/asus-wmi.h
>>> index 63e630276499..ab1c7deff118 100644
>>> --- a/include/linux/platform_data/x86/asus-wmi.h
>>> +++ b/include/linux/platform_data/x86/asus-wmi.h
>>> @@ -114,6 +114,9 @@
>>>   /* Charging mode - 1=Barrel, 2=USB */
>>>   #define ASUS_WMI_DEVID_CHARGE_MODE    0x0012006C
>>>   \x7f+/* MCU powersave mode */
>>> +#define ASUS_WMI_DEVID_MCU_POWERSAVE   0x001200E2
>>> +
>>>   /* epu is connected? 1 == true */
>>>   #define ASUS_WMI_DEVID_EGPU_CONNECTED    0x00090018
>>>   /* egpu on/off */
>>
> 
> 

Re: [PATCH v2 1/1] platform/x86: asus-wmi: disable USB0 hub on ROG Ally before suspend

[Armin Wolf]

Am 27.11.23 um 21:55 schrieb Mario Limonciello:

> On 11/27/2023 14:46, Luke Jones wrote:
>>
>>
>> On Mon, Nov 27 2023 at 02:14:23 PM -06:00:00, Mario Limonciello
>> <mario.limonciello@amd.com> wrote:
>>> On 11/26/2023 17:05, Luke D. Jones wrote:
>>>> ASUS have worked around an issue in XInput where it doesn't support
>>>> USB
>>>> selective suspend, which causes suspend issues in Windows. They worked
>>>> around this by adjusting the MCU firmware to disable the USB0 hub when
>>>> the screen is switched off during the Microsoft DSM suspend path in
>>>> ACPI.
>>>>
>>>> The issue we have with this however is one of timing - the call the
>>>> tells
>>>> the MCU to this isn't able to complete before suspend is done so we
>>>> call
>>>> this in a prepare() and add a small msleep() to ensure it is done.
>>>> This
>>>> must be done before the screen is switched off to prevent a variety of
>>>> possible races.
>>>
>>> Right now the way that Linux handles the LPS0 calls is that they're
>>> all back to back.  Luke did try to inject a delay after the LPS0
>>> calls were done but before it went to sleep but this wasn't sufficient.
>>>
>>> Another "potential" way to solve this problem from Linux may be to
>>> actually glue the LPS0 screen off call to when DRM actually has eDP
>>> turned off.
>>>
>>> Making such a change would essentially push back the "screen off"
>>> LPS0 command to when the user has run 'systemctl suspend' (or an
>>> action that did this) because the compositor usually turns it off
>>> with DPMS at this time.
>>
>> I would be willing to test this if you want some concrete data.
>
> It would require some cross subsystem plumbing to evaluate feasibility.
> I don't currently have any plans to do it.
>
> I think your patch makes sense; I just want to make it known that
> "might" clean this up if it ever happens.
>
>> See my big block of text below.
>>
>>>
>>> This is a much bigger change though and *much more ripe for breakage*.
>>>
>>> So I think in may be worth leaving a TODO comment to look into doing
>>> that in the future.
>> Do you mean add the TODO to a line in this patch?
>
> Yeah.  In case someone ever does it (me or otherwise) I think it would
> be good to have some reference in the comments that the commit 'might'
> be possible to revert.
>
>>
>>>
>>> If that ever happens; it's possible that this change could be
>>> reverted too.
>>>
>>>>
>>>> Further to this the MCU powersave option must also be disabled as
>>>> it can
>>>> cause a number of issues such as:
>>>> - unreliable resume connection of N-Key
>>>> - complete loss of N-Key if the power is plugged in while suspended
>>>> Disabling the powersave option prevents this.
>>>>
>>>> Without this the MCU is unable to initialise itself correctly on
>>>> resume.
>>>
>>> initialize
>>
>> Are we forced to use USA spelling? I'm from NZ
>> "initialise is predominantly used in British English (used in
>> UK/AU/NZ) ( en-GB )"
>>
>
> Ah I didn't realize it's an acceptable spelling for en-GB, and thought
> it was just a typo; sorry.
>
>>>
>>>>
>>>> Signed-off-by: Luke D. Jones <luke@ljones.dev>
>>>
>>> I think it would be good to add a Closes: tag to the AMD Gitlab
>>> issue that this was discussed within as well as any other public
>>> references you know about.
>>>
>>> Additionally as Phoenix APU support goes back as far as kernel 6.1
>>> and this is well contained to only run on the ROG I suggest to CC
>>> stable so that people can use the ROG on that LTS kernel or later.
>>>
>>>> ---
>>>> -SNIP-
>>>>   \x7f@@ -4701,6 +4749,8 @@ 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,
>>>
>>> Have you experimented with only using the prepare() call or only the
>>> resume_early() call?  Are both really needed?
>>
>> I have yes. Although the device comes back eventually in resume after
>> only a prepare call it's not preferable as it tends to change the
>> device path. With resume_early we can get the device replugged super
>> early (before anything notices it's gone in fact).
>>
>> This whole thing is a bit of a mess. It ends up being a race between
>> various things to prevent a HUB0 disconnect being registered by the
>> xhci subsystem, and adding the device back before the xhci subsystem
>> gets control.
>>
>> If I add a sleep longer than 1300ms in prepare then the xhci subsys
>> registers a disconnect of the USB0 hub. If the sleep is under 250ms
>> it isn't quite enough for the MCU to do its thing, and on battery it
>> seems worse.
>>
>> I have asked the ASUS guys I'm in contact with for something to
>> disable this MCU behaviour since it is purely a workaround for a
>> broken Windows thing :( They are open to something, maybe an OS
>> detect in ACPI or a WMI method addition similar to the MCU powersave
>> method, from what I'm told it would require an MCU firmware update
>> along with BIOS update. If this eventuates I'll submit an additional
>> patch to check and set that plus disable this.
>
> Don't let them do an OS detection in ACPI, it's going to be too painful.
> I would instead suggest that they can have a bit that you can program
> in via ACPI or WMI from the ASUS WMI driver that says to skip the MCU
> disconnect behavior.
>
I totally agree, we do not need another _OSI(Linux) type of problem.
Maybe those guys at Asus could just implement a ACPI _DSM for the USB controller in question which allows for disabling this workaround.
This would be preferable to an additional WMI method, since the notebook would otherwise depend on the asus-wmi driver to suspend properly.
With the ACPI _DSM, the USB controller driver can disable the workaround as soon as the USB controller probes.

Armin Wolf

>>
>> I may possibly write a new version of this patch as we've seen that
>> enabling powersave reduces suspend power use by at least half. And
>> looking through my DSDT dumps, there are a few laptops with the same
>> feature as Ally. The patch for powersave being enabled requires also
>> AC power state on suspend change detection, and a later forced reset
>> in late resume (and the device paths change regardless when powersave
>> is on).
>>
>> When I look at it objectively, the device path changing should be a
>> non-issue really as it is fully handled by USB subsystem and behaves
>> exactly like what it is - a USB hub disconnect. It's just that some
>> userspace apps don't expect this. I will experiment some more.
>>
>> Regards,
>> Luke.
>>
>
> As another experiment - what happens if you "comment out" the LPS0
> calls that do this problematic stuff?
>
> It's important to make sure the callback to amd-pmc stays in place,
> but if you just skip those ACPI ones does it still get to the deepest
> state and are there other problems?
>
>>>
>>>>   };
>>>>   \x7f  /* Registration
>>>> ***************************************************************/
>>>> diff --git a/include/linux/platform_data/x86/asus-wmi.h
>>>> b/include/linux/platform_data/x86/asus-wmi.h
>>>> index 63e630276499..ab1c7deff118 100644
>>>> --- a/include/linux/platform_data/x86/asus-wmi.h
>>>> +++ b/include/linux/platform_data/x86/asus-wmi.h
>>>> @@ -114,6 +114,9 @@
>>>>   /* Charging mode - 1=Barrel, 2=USB */
>>>>   #define ASUS_WMI_DEVID_CHARGE_MODE    0x0012006C
>>>>   \x7f+/* MCU powersave mode */
>>>> +#define ASUS_WMI_DEVID_MCU_POWERSAVE   0x001200E2
>>>> +
>>>>   /* epu is connected? 1 == true */
>>>>   #define ASUS_WMI_DEVID_EGPU_CONNECTED    0x00090018
>>>>   /* egpu on/off */
>>>
>>
>>
>
>

Re: [PATCH v2 1/1] platform/x86: asus-wmi: disable USB0 hub on ROG Ally before suspend

[Hans de Goede]

Hi,

On 11/27/23 21:17, Luke Jones wrote:
> 
> 
> On Mon, Nov 27 2023 at 09:53:13 AM +01:00:00, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>> On 11/27/23 00:05, Luke D. Jones wrote:
>>>  ASUS have worked around an issue in XInput where it doesn't support USB
>>>  selective suspend, which causes suspend issues in Windows. They worked
>>>  around this by adjusting the MCU firmware to disable the USB0 hub when
>>>  the screen is switched off during the Microsoft DSM suspend path in ACPI.
>>>
>>>  The issue we have with this however is one of timing - the call the tells
>>>  the MCU to this isn't able to complete before suspend is done so we call
>>>  this in a prepare() and add a small msleep() to ensure it is done. This
>>>  must be done before the screen is switched off to prevent a variety of
>>>  possible races.
>>>
>>>  Further to this the MCU powersave option must also be disabled as it can
>>>  cause a number of issues such as:
>>>  - unreliable resume connection of N-Key
>>>  - complete loss of N-Key if the power is plugged in while suspended
>>>  Disabling the powersave option prevents this.
>>>
>>>  Without this the MCU is unable to initialise itself correctly on resume.
>>>
>>>  Signed-off-by: Luke D. Jones <luke@ljones.dev>
>>
>> Thanks, patch looks good to me, except that all the new lines
>> seem to use 4 spaces rather then a tab char as indent.
> 
> Apologies for the previous HTML email.
> I must be going mad... are you sure? I've checked the patch file I submitted. Run checkpatch on it. Checked my email copy, and checked in lore... I can't see where space chars are?

So I just checked the copy in patchwork:

https://patchwork.kernel.org/project/platform-driver-x86/patch/20231126230521.125708-2-luke@ljones.dev/

and you are rights, no 4 spaces there.

Where as if you look further down in this reply, where the original
patch is quoted the 4 spaces are right there, so now I'm wondering
if maybe my mail client introduced the problem when I was replying ?

(replies to other patches preserve the tabs just fine).

So this is weird, but lets just forget about it, just some weird
glitch ...

Regards,

Hans




> 
>>
>> With that fixed you can add my:
>>
>> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
>>
>> to the next version.
>>
>> Regards,
>>
>> Hans
>>
>>
>>>  ---
>>>   drivers/platform/x86/asus-wmi.c            | 50 ++++++++++++++++++++++
>>>   include/linux/platform_data/x86/asus-wmi.h |  3 ++
>>>   2 files changed, 53 insertions(+)
>>>
>>>  diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
>>>  index 6a79f16233ab..4ba33dfebfd4 100644
>>>  --- a/drivers/platform/x86/asus-wmi.c
>>>  +++ b/drivers/platform/x86/asus-wmi.c
>>>  @@ -16,6 +16,7 @@
>>>   #include <linux/acpi.h>
>>>   #include <linux/backlight.h>
>>>   #include <linux/debugfs.h>
>>>  +#include <linux/delay.h>
>>>   #include <linux/dmi.h>
>>>   #include <linux/fb.h>
>>>   #include <linux/hwmon.h>
>>>  @@ -132,6 +133,11 @@ module_param(fnlock_default, bool, 0444);
>>>   #define ASUS_SCREENPAD_BRIGHT_MAX 255
>>>   #define ASUS_SCREENPAD_BRIGHT_DEFAULT 60
>>>
>>>  +/* 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 300
>>>  +
>>>   static const char * const ashs_ids[] = { "ATK4001", "ATK4002", NULL };
>>>
>>>   static int throttle_thermal_policy_write(struct asus_wmi *);
>>>  @@ -300,6 +306,9 @@ struct asus_wmi {
>>>
>>>       bool fnlock_locked;
>>>
>>>  +    /* The ROG Ally device requires the MCU USB device be disconnected before suspend */
>>>  +    bool ally_mcu_usb_switch;
>>>  +
>>>       struct asus_wmi_debug debug;
>>>
>>>       struct asus_wmi_driver *driver;
>>>  @@ -4488,6 +4497,8 @@ static int asus_wmi_add(struct platform_device *pdev)
>>>       asus->nv_temp_tgt_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_NV_THERM_TARGET);
>>>       asus->panel_overdrive_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_PANEL_OD);
>>>       asus->mini_led_mode_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_MINI_LED_MODE);
>>>  +    asus->ally_mcu_usb_switch = acpi_has_method(NULL, ASUS_USB0_PWR_EC0_CSEE)
>>>  +                        && dmi_match(DMI_BOARD_NAME, "RC71L");
>>>
>>>       err = fan_boost_mode_check_present(asus);
>>>       if (err)
>>>  @@ -4654,6 +4665,43 @@ static int asus_hotk_resume(struct device *device)
>>>           asus_wmi_fnlock_update(asus);
>>>
>>>       asus_wmi_tablet_mode_get_state(asus);
>>>  +
>>>  +    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) {
>>>  +        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);
>>>  +    int result, err;
>>>  +
>>>  +    if (asus->ally_mcu_usb_switch) {
>>>  +        /* When powersave is enabled it causes many issues with resume of USB hub */
>>>  +        result = asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_MCU_POWERSAVE);
>>>  +        if (result == 1) {
>>>  +            dev_warn(device, "MCU powersave enabled, disabling to prevent resume issues");
>>>  +            err = asus_wmi_set_devstate(ASUS_WMI_DEVID_MCU_POWERSAVE, 0, &result);
>>>  +            if (err || result != 1)
>>>  +                dev_err(device, "Failed to set MCU powersave mode: %d\n", err);
>>>  +        }
>>>  +        /* 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;
>>>   }
>>>
>>>  @@ -4701,6 +4749,8 @@ 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,
>>>   };
>>>
>>>   /* Registration ***************************************************************/
>>>  diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
>>>  index 63e630276499..ab1c7deff118 100644
>>>  --- a/include/linux/platform_data/x86/asus-wmi.h
>>>  +++ b/include/linux/platform_data/x86/asus-wmi.h
>>>  @@ -114,6 +114,9 @@
>>>   /* Charging mode - 1=Barrel, 2=USB */
>>>   #define ASUS_WMI_DEVID_CHARGE_MODE    0x0012006C
>>>
>>>  +/* MCU powersave mode */
>>>  +#define ASUS_WMI_DEVID_MCU_POWERSAVE   0x001200E2
>>>  +
>>>   /* epu is connected? 1 == true */
>>>   #define ASUS_WMI_DEVID_EGPU_CONNECTED    0x00090018
>>>   /* egpu on/off */
>>
> 
> 

Re: [PATCH v2 1/1] platform/x86: asus-wmi: disable USB0 hub on ROG Ally before suspend

[Luke Jones]

On Mon, Nov 27 2023 at 10:42:48 PM +01:00:00, Armin Wolf 
<W_Armin@gmx.de> wrote:
> Am 27.11.23 um 21:55 schrieb Mario Limonciello:
>> On 11/27/2023 14:46, Luke Jones wrote:
>>> 
>>> 
>>>  On Mon, Nov 27 2023 at 02:14:23 PM -06:00:00, Mario Limonciello 
>>> <mario.limonciello@amd.com> wrote:
>>>> On 11/26/2023 17:05, Luke D. Jones wrote:
>>>>> ASUS have worked around an issue in XInput where it doesn't 
>>>>> support USB
>>>>>  selective suspend, which causes suspend issues in Windows. They 
>>>>> worked
>>>>>  around this by adjusting the MCU firmware to disable the USB0 
>>>>> hub when
>>>>>  the screen is switched off during the Microsoft DSM suspend path 
>>>>> in ACPI.
>>>>> 
>>>>>  The issue we have with this however is one of timing - the call 
>>>>> the tells
>>>>>  the MCU to this isn't able to complete before suspend is done so 
>>>>> we call
>>>>>  this in a prepare() and add a small msleep() to ensure it is 
>>>>> done. This
>>>>>  must be done before the screen is switched off to prevent a 
>>>>> variety of
>>>>>  possible races.
>>>> 
>>>>  Right now the way that Linux handles the LPS0 calls is that 
>>>> they're all back to back.  Luke did try to inject a delay after 
>>>> the LPS0 calls were done but before it went to sleep but this 
>>>> wasn't sufficient.
>>>> 
>>>>  Another "potential" way to solve this problem from Linux may be 
>>>> to actually glue the LPS0 screen off call to when DRM actually has 
>>>> eDP turned off.
>>>> 
>>>>  Making such a change would essentially push back the "screen off" 
>>>> LPS0 command to when the user has run 'systemctl suspend' (or an 
>>>> action that did this) because the compositor usually turns it off 
>>>> with DPMS at this time.
>>> 
>>>  I would be willing to test this if you want some concrete data.
>> 
>>  It would require some cross subsystem plumbing to evaluate 
>> feasibility.
>>  I don't currently have any plans to do it.
>> 
>>  I think your patch makes sense; I just want to make it known that 
>> "might" clean this up if it ever happens.
>> 
>>> See my big block of text below.
>>> 
>>>> 
>>>>  This is a much bigger change though and *much more ripe for 
>>>> breakage*.
>>>> 
>>>>  So I think in may be worth leaving a TODO comment to look into 
>>>> doing that in the future.
>>>  Do you mean add the TODO to a line in this patch?
>> 
>>  Yeah.  In case someone ever does it (me or otherwise) I think it 
>> would be good to have some reference in the comments that the commit 
>> 'might' be possible to revert.
>> 
>>> 
>>>> 
>>>>  If that ever happens; it's possible that this change could be 
>>>> reverted too.
>>>> 
>>>>> 
>>>>>  Further to this the MCU powersave option must also be disabled 
>>>>> as it can
>>>>>  cause a number of issues such as:
>>>>>  - unreliable resume connection of N-Key
>>>>>  - complete loss of N-Key if the power is plugged in while 
>>>>> suspended
>>>>>  Disabling the powersave option prevents this.
>>>>> 
>>>>>  Without this the MCU is unable to initialise itself correctly on 
>>>>> resume.
>>>> 
>>>>  initialize
>>> 
>>>  Are we forced to use USA spelling? I'm from NZ
>>>  "initialise is predominantly used in British English (used in 
>>> UK/AU/NZ) ( en-GB )"
>>> 
>> 
>>  Ah I didn't realize it's an acceptable spelling for en-GB, and 
>> thought it was just a typo; sorry.
>> 
>>>> 
>>>>> 
>>>>>  Signed-off-by: Luke D. Jones <luke@ljones.dev>
>>>> 
>>>>  I think it would be good to add a Closes: tag to the AMD Gitlab 
>>>> issue that this was discussed within as well as any other public 
>>>> references you know about.
>>>> 
>>>>  Additionally as Phoenix APU support goes back as far as kernel 
>>>> 6.1 and this is well contained to only run on the ROG I suggest to 
>>>> CC stable so that people can use the ROG on that LTS kernel or 
>>>> later.
>>>> 
>>>>> ---
>>>>>  -SNIP-
>>>>>    \x7f@@ -4701,6 +4749,8 @@ 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,
>>>> 
>>>>  Have you experimented with only using the prepare() call or only 
>>>> the resume_early() call?  Are both really needed?
>>> 
>>>  I have yes. Although the device comes back eventually in resume 
>>> after only a prepare call it's not preferable as it tends to change 
>>> the device path. With resume_early we can get the device replugged 
>>> super early (before anything notices it's gone in fact).
>>> 
>>>  This whole thing is a bit of a mess. It ends up being a race 
>>> between various things to prevent a HUB0 disconnect being 
>>> registered by the xhci subsystem, and adding the device back before 
>>> the xhci subsystem gets control.
>>> 
>>>  If I add a sleep longer than 1300ms in prepare then the xhci 
>>> subsys registers a disconnect of the USB0 hub. If the sleep is 
>>> under 250ms it isn't quite enough for the MCU to do its thing, and 
>>> on battery it seems worse.
>>> 
>>>  I have asked the ASUS guys I'm in contact with for something to 
>>> disable this MCU behaviour since it is purely a workaround for a 
>>> broken Windows thing :( They are open to something, maybe an OS 
>>> detect in ACPI or a WMI method addition similar to the MCU 
>>> powersave method, from what I'm told it would require an MCU 
>>> firmware update along with BIOS update. If this eventuates I'll 
>>> submit an additional patch to check and set that plus disable this.
>> 
>>  Don't let them do an OS detection in ACPI, it's going to be too 
>> painful.
>>  I would instead suggest that they can have a bit that you can 
>> program in via ACPI or WMI from the ASUS WMI driver that says to 
>> skip the MCU disconnect behavior.
>> 
> I totally agree, we do not need another _OSI(Linux) type of problem.
> Maybe those guys at Asus could just implement a ACPI _DSM for the USB 
> controller in question which allows for disabling this workaround.
> This would be preferable to an additional WMI method, since the 
> notebook would otherwise depend on the asus-wmi driver to suspend 
> properly.
> With the ACPI _DSM, the USB controller driver can disable the 
> workaround as soon as the USB controller probes.

Would you be so kind as to explain what this means? My knowledge of 
ACPI is paper thin and generally revolves just around the ASUS WMI 
part. From what i can find the XHC0 (the hub the MCU is attached to) 
doesn't have any current _DSM. I understand it means Device Specific 
Method, so I guess you mean adding a method to be used only if that HUB 
is there and implements it?

The ROG Ally depends on the asus-wmi driver regardless, without it, it 
is barely functional.

>>> 
>>>  I may possibly write a new version of this patch as we've seen 
>>> that enabling powersave reduces suspend power use by at least half. 
>>> And looking through my DSDT dumps, there are a few laptops with the 
>>> same feature as Ally. The patch for powersave being enabled 
>>> requires also AC power state on suspend change detection, and a 
>>> later forced reset in late resume (and the device paths change 
>>> regardless when powersave is on).
>>> 
>>>  When I look at it objectively, the device path changing should be 
>>> a non-issue really as it is fully handled by USB subsystem and 
>>> behaves exactly like what it is - a USB hub disconnect. It's just 
>>> that some userspace apps don't expect this. I will experiment some 
>>> more.
>>> 
>>>  Regards,
>>>  Luke.
>>> 
>> 
>>  As another experiment - what happens if you "comment out" the LPS0 
>> calls that do this problematic stuff?
>> 
>>  It's important to make sure the callback to amd-pmc stays in place, 
>> but if you just skip those ACPI ones does it still get to the 
>> deepest state and are there other problems?
>> 
>>>> 
>>>>>   };
>>>>>    \x7f  /* Registration 
>>>>> ***************************************************************/
>>>>>  diff --git a/include/linux/platform_data/x86/asus-wmi.h 
>>>>> b/include/linux/platform_data/x86/asus-wmi.h
>>>>>  index 63e630276499..ab1c7deff118 100644
>>>>>  --- a/include/linux/platform_data/x86/asus-wmi.h
>>>>>  +++ b/include/linux/platform_data/x86/asus-wmi.h
>>>>>  @@ -114,6 +114,9 @@
>>>>>    /* Charging mode - 1=Barrel, 2=USB */
>>>>>    #define ASUS_WMI_DEVID_CHARGE_MODE    0x0012006C
>>>>>    \x7f+/* MCU powersave mode */
>>>>>  +#define ASUS_WMI_DEVID_MCU_POWERSAVE   0x001200E2
>>>>>  +
>>>>>    /* epu is connected? 1 == true */
>>>>>    #define ASUS_WMI_DEVID_EGPU_CONNECTED    0x00090018
>>>>>    /* egpu on/off */
>>>> 
>>> 
>>> 
>> 
>> 

Re: [PATCH v2 1/1] platform/x86: asus-wmi: disable USB0 hub on ROG Ally before suspend

[Luke Jones]

On Mon, Nov 27 2023 at 11:39:09 PM +01:00:00, Hans de Goede 
<hdegoede@redhat.com> wrote:
> Hi,
> 
> On 11/27/23 21:17, Luke Jones wrote:
>> 
>> 
>>  On Mon, Nov 27 2023 at 09:53:13 AM +01:00:00, Hans de Goede 
>> <hdegoede@redhat.com> wrote:
>>>  Hi,
>>> 
>>>  On 11/27/23 00:05, Luke D. Jones wrote:
>>>>   ASUS have worked around an issue in XInput where it doesn't 
>>>> support USB
>>>>   selective suspend, which causes suspend issues in Windows. They 
>>>> worked
>>>>   around this by adjusting the MCU firmware to disable the USB0 
>>>> hub when
>>>>   the screen is switched off during the Microsoft DSM suspend path 
>>>> in ACPI.
>>>> 
>>>>   The issue we have with this however is one of timing - the call 
>>>> the tells
>>>>   the MCU to this isn't able to complete before suspend is done so 
>>>> we call
>>>>   this in a prepare() and add a small msleep() to ensure it is 
>>>> done. This
>>>>   must be done before the screen is switched off to prevent a 
>>>> variety of
>>>>   possible races.
>>>> 
>>>>   Further to this the MCU powersave option must also be disabled 
>>>> as it can
>>>>   cause a number of issues such as:
>>>>   - unreliable resume connection of N-Key
>>>>   - complete loss of N-Key if the power is plugged in while 
>>>> suspended
>>>>   Disabling the powersave option prevents this.
>>>> 
>>>>   Without this the MCU is unable to initialise itself correctly on 
>>>> resume.
>>>> 
>>>>   Signed-off-by: Luke D. Jones <luke@ljones.dev>
>>> 
>>>  Thanks, patch looks good to me, except that all the new lines
>>>  seem to use 4 spaces rather then a tab char as indent.
>> 
>>  Apologies for the previous HTML email.
>>  I must be going mad... are you sure? I've checked the patch file I 
>> submitted. Run checkpatch on it. Checked my email copy, and checked 
>> in lore... I can't see where space chars are?
> 
> So I just checked the copy in patchwork:
> 
> https://patchwork.kernel.org/project/platform-driver-x86/patch/20231126230521.125708-2-luke@ljones.dev/
> 
> and you are rights, no 4 spaces there.
> 
> Where as if you look further down in this reply, where the original
> patch is quoted the 4 spaces are right there, so now I'm wondering
> if maybe my mail client introduced the problem when I was replying ?
> 
> (replies to other patches preserve the tabs just fine).
> 
> So this is weird, but lets just forget about it, just some weird
> glitch ...
> 

Sun flares and cosmic particles.

I'm comfortable with this being merged and doing through stable also. I 
did many many more tests this morning, along with a half dozen other 
people and this solution appears to be the only reliable option. When 
ASUS provide a way to turn off the MCU unplug feature I will update 
with a new patch (and add the TODO).

> 
> 
> 
>> 
>>> 
>>>  With that fixed you can add my:
>>> 
>>>  Reviewed-by: Hans de Goede <hdegoede@redhat.com>
>>> 
>>>  to the next version.
>>> 
>>>  Regards,
>>> 
>>>  Hans
>>> 
>>> 
>>>>   ---
>>>>    drivers/platform/x86/asus-wmi.c            | 50 
>>>> ++++++++++++++++++++++
>>>>    include/linux/platform_data/x86/asus-wmi.h |  3 ++
>>>>    2 files changed, 53 insertions(+)
>>>> 
>>>>   diff --git a/drivers/platform/x86/asus-wmi.c 
>>>> b/drivers/platform/x86/asus-wmi.c
>>>>   index 6a79f16233ab..4ba33dfebfd4 100644
>>>>   --- a/drivers/platform/x86/asus-wmi.c
>>>>   +++ b/drivers/platform/x86/asus-wmi.c
>>>>   @@ -16,6 +16,7 @@
>>>>    #include <linux/acpi.h>
>>>>    #include <linux/backlight.h>
>>>>    #include <linux/debugfs.h>
>>>>   +#include <linux/delay.h>
>>>>    #include <linux/dmi.h>
>>>>    #include <linux/fb.h>
>>>>    #include <linux/hwmon.h>
>>>>   @@ -132,6 +133,11 @@ module_param(fnlock_default, bool, 0444);
>>>>    #define ASUS_SCREENPAD_BRIGHT_MAX 255
>>>>    #define ASUS_SCREENPAD_BRIGHT_DEFAULT 60
>>>> 
>>>>   +/* 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 300
>>>>   +
>>>>    static const char * const ashs_ids[] = { "ATK4001", "ATK4002", 
>>>> NULL };
>>>> 
>>>>    static int throttle_thermal_policy_write(struct asus_wmi *);
>>>>   @@ -300,6 +306,9 @@ struct asus_wmi {
>>>> 
>>>>        bool fnlock_locked;
>>>> 
>>>>   +    /* The ROG Ally device requires the MCU USB device be 
>>>> disconnected before suspend */
>>>>   +    bool ally_mcu_usb_switch;
>>>>   +
>>>>        struct asus_wmi_debug debug;
>>>> 
>>>>        struct asus_wmi_driver *driver;
>>>>   @@ -4488,6 +4497,8 @@ static int asus_wmi_add(struct 
>>>> platform_device *pdev)
>>>>        asus->nv_temp_tgt_available = asus_wmi_dev_is_present(asus, 
>>>> ASUS_WMI_DEVID_NV_THERM_TARGET);
>>>>        asus->panel_overdrive_available = 
>>>> asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_PANEL_OD);
>>>>        asus->mini_led_mode_available = 
>>>> asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_MINI_LED_MODE);
>>>>   +    asus->ally_mcu_usb_switch = acpi_has_method(NULL, 
>>>> ASUS_USB0_PWR_EC0_CSEE)
>>>>   +                        && dmi_match(DMI_BOARD_NAME, "RC71L");
>>>> 
>>>>        err = fan_boost_mode_check_present(asus);
>>>>        if (err)
>>>>   @@ -4654,6 +4665,43 @@ static int asus_hotk_resume(struct device 
>>>> *device)
>>>>            asus_wmi_fnlock_update(asus);
>>>> 
>>>>        asus_wmi_tablet_mode_get_state(asus);
>>>>   +
>>>>   +    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) {
>>>>   +        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);
>>>>   +    int result, err;
>>>>   +
>>>>   +    if (asus->ally_mcu_usb_switch) {
>>>>   +        /* When powersave is enabled it causes many issues with 
>>>> resume of USB hub */
>>>>   +        result = asus_wmi_get_devstate_simple(asus, 
>>>> ASUS_WMI_DEVID_MCU_POWERSAVE);
>>>>   +        if (result == 1) {
>>>>   +            dev_warn(device, "MCU powersave enabled, disabling 
>>>> to prevent resume issues");
>>>>   +            err = 
>>>> asus_wmi_set_devstate(ASUS_WMI_DEVID_MCU_POWERSAVE, 0, &result);
>>>>   +            if (err || result != 1)
>>>>   +                dev_err(device, "Failed to set MCU powersave 
>>>> mode: %d\n", err);
>>>>   +        }
>>>>   +        /* 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;
>>>>    }
>>>> 
>>>>   @@ -4701,6 +4749,8 @@ 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,
>>>>    };
>>>> 
>>>>    /* Registration 
>>>> ***************************************************************/
>>>>   diff --git a/include/linux/platform_data/x86/asus-wmi.h 
>>>> b/include/linux/platform_data/x86/asus-wmi.h
>>>>   index 63e630276499..ab1c7deff118 100644
>>>>   --- a/include/linux/platform_data/x86/asus-wmi.h
>>>>   +++ b/include/linux/platform_data/x86/asus-wmi.h
>>>>   @@ -114,6 +114,9 @@
>>>>    /* Charging mode - 1=Barrel, 2=USB */
>>>>    #define ASUS_WMI_DEVID_CHARGE_MODE    0x0012006C
>>>> 
>>>>   +/* MCU powersave mode */
>>>>   +#define ASUS_WMI_DEVID_MCU_POWERSAVE   0x001200E2
>>>>   +
>>>>    /* epu is connected? 1 == true */
>>>>    #define ASUS_WMI_DEVID_EGPU_CONNECTED    0x00090018
>>>>    /* egpu on/off */
>>> 
>> 
>> 
> 

Re: [PATCH v2 1/1] platform/x86: asus-wmi: disable USB0 hub on ROG Ally before suspend

[Armin Wolf]

Am 28.11.23 um 01:54 schrieb Luke Jones:

>
>
> On Mon, Nov 27 2023 at 10:42:48 PM +01:00:00, Armin Wolf
> <W_Armin@gmx.de> wrote:
>> Am 27.11.23 um 21:55 schrieb Mario Limonciello:
>>> On 11/27/2023 14:46, Luke Jones wrote:
>>>>
>>>>
>>>>  On Mon, Nov 27 2023 at 02:14:23 PM -06:00:00, Mario Limonciello
>>>> <mario.limonciello@amd.com> wrote:
>>>>> On 11/26/2023 17:05, Luke D. Jones wrote:
>>>>>> ASUS have worked around an issue in XInput where it doesn't
>>>>>> support USB
>>>>>>  selective suspend, which causes suspend issues in Windows. They
>>>>>> worked
>>>>>>  around this by adjusting the MCU firmware to disable the USB0
>>>>>> hub when
>>>>>>  the screen is switched off during the Microsoft DSM suspend path
>>>>>> in ACPI.
>>>>>>
>>>>>>  The issue we have with this however is one of timing - the call
>>>>>> the tells
>>>>>>  the MCU to this isn't able to complete before suspend is done so
>>>>>> we call
>>>>>>  this in a prepare() and add a small msleep() to ensure it is
>>>>>> done. This
>>>>>>  must be done before the screen is switched off to prevent a
>>>>>> variety of
>>>>>>  possible races.
>>>>>
>>>>>  Right now the way that Linux handles the LPS0 calls is that
>>>>> they're all back to back.  Luke did try to inject a delay after
>>>>> the LPS0 calls were done but before it went to sleep but this
>>>>> wasn't sufficient.
>>>>>
>>>>>  Another "potential" way to solve this problem from Linux may be
>>>>> to actually glue the LPS0 screen off call to when DRM actually has
>>>>> eDP turned off.
>>>>>
>>>>>  Making such a change would essentially push back the "screen off"
>>>>> LPS0 command to when the user has run 'systemctl suspend' (or an
>>>>> action that did this) because the compositor usually turns it off
>>>>> with DPMS at this time.
>>>>
>>>>  I would be willing to test this if you want some concrete data.
>>>
>>>  It would require some cross subsystem plumbing to evaluate
>>> feasibility.
>>>  I don't currently have any plans to do it.
>>>
>>>  I think your patch makes sense; I just want to make it known that
>>> "might" clean this up if it ever happens.
>>>
>>>> See my big block of text below.
>>>>
>>>>>
>>>>>  This is a much bigger change though and *much more ripe for
>>>>> breakage*.
>>>>>
>>>>>  So I think in may be worth leaving a TODO comment to look into
>>>>> doing that in the future.
>>>>  Do you mean add the TODO to a line in this patch?
>>>
>>>  Yeah.  In case someone ever does it (me or otherwise) I think it
>>> would be good to have some reference in the comments that the commit
>>> 'might' be possible to revert.
>>>
>>>>
>>>>>
>>>>>  If that ever happens; it's possible that this change could be
>>>>> reverted too.
>>>>>
>>>>>>
>>>>>>  Further to this the MCU powersave option must also be disabled
>>>>>> as it can
>>>>>>  cause a number of issues such as:
>>>>>>  - unreliable resume connection of N-Key
>>>>>>  - complete loss of N-Key if the power is plugged in while suspended
>>>>>>  Disabling the powersave option prevents this.
>>>>>>
>>>>>>  Without this the MCU is unable to initialise itself correctly on
>>>>>> resume.
>>>>>
>>>>>  initialize
>>>>
>>>>  Are we forced to use USA spelling? I'm from NZ
>>>>  "initialise is predominantly used in British English (used in
>>>> UK/AU/NZ) ( en-GB )"
>>>>
>>>
>>>  Ah I didn't realize it's an acceptable spelling for en-GB, and
>>> thought it was just a typo; sorry.
>>>
>>>>>
>>>>>>
>>>>>>  Signed-off-by: Luke D. Jones <luke@ljones.dev>
>>>>>
>>>>>  I think it would be good to add a Closes: tag to the AMD Gitlab
>>>>> issue that this was discussed within as well as any other public
>>>>> references you know about.
>>>>>
>>>>>  Additionally as Phoenix APU support goes back as far as kernel
>>>>> 6.1 and this is well contained to only run on the ROG I suggest to
>>>>> CC stable so that people can use the ROG on that LTS kernel or later.
>>>>>
>>>>>> ---
>>>>>>  -SNIP-
>>>>>>    \x7f@@ -4701,6 +4749,8 @@ 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,
>>>>>
>>>>>  Have you experimented with only using the prepare() call or only
>>>>> the resume_early() call?  Are both really needed?
>>>>
>>>>  I have yes. Although the device comes back eventually in resume
>>>> after only a prepare call it's not preferable as it tends to change
>>>> the device path. With resume_early we can get the device replugged
>>>> super early (before anything notices it's gone in fact).
>>>>
>>>>  This whole thing is a bit of a mess. It ends up being a race
>>>> between various things to prevent a HUB0 disconnect being
>>>> registered by the xhci subsystem, and adding the device back before
>>>> the xhci subsystem gets control.
>>>>
>>>>  If I add a sleep longer than 1300ms in prepare then the xhci
>>>> subsys registers a disconnect of the USB0 hub. If the sleep is
>>>> under 250ms it isn't quite enough for the MCU to do its thing, and
>>>> on battery it seems worse.
>>>>
>>>>  I have asked the ASUS guys I'm in contact with for something to
>>>> disable this MCU behaviour since it is purely a workaround for a
>>>> broken Windows thing :( They are open to something, maybe an OS
>>>> detect in ACPI or a WMI method addition similar to the MCU
>>>> powersave method, from what I'm told it would require an MCU
>>>> firmware update along with BIOS update. If this eventuates I'll
>>>> submit an additional patch to check and set that plus disable this.
>>>
>>>  Don't let them do an OS detection in ACPI, it's going to be too
>>> painful.
>>>  I would instead suggest that they can have a bit that you can
>>> program in via ACPI or WMI from the ASUS WMI driver that says to
>>> skip the MCU disconnect behavior.
>>>
>> I totally agree, we do not need another _OSI(Linux) type of problem.
>> Maybe those guys at Asus could just implement a ACPI _DSM for the USB
>> controller in question which allows for disabling this workaround.
>> This would be preferable to an additional WMI method, since the
>> notebook would otherwise depend on the asus-wmi driver to suspend
>> properly.
>> With the ACPI _DSM, the USB controller driver can disable the
>> workaround as soon as the USB controller probes.
>
> Would you be so kind as to explain what this means? My knowledge of
> ACPI is paper thin and generally revolves just around the ASUS WMI
> part. From what i can find the XHC0 (the hub the MCU is attached to)
> doesn't have any current _DSM. I understand it means Device Specific
> Method, so I guess you mean adding a method to be used only if that
> HUB is there and implements it?
>
> The ROG Ally depends on the asus-wmi driver regardless, without it, it
> is barely functional.
>
An ACPI _DSM method is a "standardized" way for manufacturers to add device specific methods to ACPI devices, see
https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/09_ACPI-Defined_Devices_and_Device-Specific_Objects/ACPIdefined_Devices_and_DeviceSpecificObjects.html for details.

Implementing via a ACPI _DSM would contain the necessary logic for USB suspend inside the USB hub driver, instead of forcing asus-wmi and the USB hub driver
to collaborate on USB suspend. The USB hub driver would discover this device specific method during probe and disable the workaround, leaving no chance for
race conditions between asus-wmi and the USB hub driver to develop.

Of course if the ROG Ally depends on asus-wmi regardless, then Asus could as well use the WMI interface for this. But IMHO standard ACPI interfaces should be
preferred to custom ones like WMI.

Armin Wolf

>>>>
>>>>  I may possibly write a new version of this patch as we've seen
>>>> that enabling powersave reduces suspend power use by at least half.
>>>> And looking through my DSDT dumps, there are a few laptops with the
>>>> same feature as Ally. The patch for powersave being enabled
>>>> requires also AC power state on suspend change detection, and a
>>>> later forced reset in late resume (and the device paths change
>>>> regardless when powersave is on).
>>>>
>>>>  When I look at it objectively, the device path changing should be
>>>> a non-issue really as it is fully handled by USB subsystem and
>>>> behaves exactly like what it is - a USB hub disconnect. It's just
>>>> that some userspace apps don't expect this. I will experiment some
>>>> more.
>>>>
>>>>  Regards,
>>>>  Luke.
>>>>
>>>
>>>  As another experiment - what happens if you "comment out" the LPS0
>>> calls that do this problematic stuff?
>>>
>>>  It's important to make sure the callback to amd-pmc stays in place,
>>> but if you just skip those ACPI ones does it still get to the
>>> deepest state and are there other problems?
>>>
>>>>>
>>>>>>   };
>>>>>>    \x7f  /* Registration
>>>>>> ***************************************************************/
>>>>>>  diff --git a/include/linux/platform_data/x86/asus-wmi.h
>>>>>> b/include/linux/platform_data/x86/asus-wmi.h
>>>>>>  index 63e630276499..ab1c7deff118 100644
>>>>>>  --- a/include/linux/platform_data/x86/asus-wmi.h
>>>>>>  +++ b/include/linux/platform_data/x86/asus-wmi.h
>>>>>>  @@ -114,6 +114,9 @@
>>>>>>    /* Charging mode - 1=Barrel, 2=USB */
>>>>>>    #define ASUS_WMI_DEVID_CHARGE_MODE    0x0012006C
>>>>>>    \x7f+/* MCU powersave mode */
>>>>>>  +#define ASUS_WMI_DEVID_MCU_POWERSAVE   0x001200E2
>>>>>>  +
>>>>>>    /* epu is connected? 1 == true */
>>>>>>    #define ASUS_WMI_DEVID_EGPU_CONNECTED    0x00090018
>>>>>>    /* egpu on/off */
>>>>>
>>>>
>>>>
>>>
>>>
>
>
>

Re: [PATCH v2 1/1] platform/x86: asus-wmi: disable USB0 hub on ROG Ally before suspend

[Luke Jones]

On Tue, Nov 28 2023 at 02:16:53 AM +01:00:00, Armin Wolf 
<W_Armin@gmx.de> wrote:
> Am 28.11.23 um 01:54 schrieb Luke Jones:
> 
>> 
>> 
>> On Mon, Nov 27 2023 at 10:42:48 PM +01:00:00, Armin Wolf
>> <W_Armin@gmx.de> wrote:
>>> Am 27.11.23 um 21:55 schrieb Mario Limonciello:
>>>> On 11/27/2023 14:46, Luke Jones wrote:
>>>>> 
>>>>> 
>>>>>  On Mon, Nov 27 2023 at 02:14:23 PM -06:00:00, Mario Limonciello
>>>>> <mario.limonciello@amd.com> wrote:
>>>>>> On 11/26/2023 17:05, Luke D. Jones wrote:
>>>>>>> ASUS have worked around an issue in XInput where it doesn't
>>>>>>> support USB
>>>>>>>  selective suspend, which causes suspend issues in Windows. They
>>>>>>> worked
>>>>>>>  around this by adjusting the MCU firmware to disable the USB0
>>>>>>> hub when
>>>>>>>  the screen is switched off during the Microsoft DSM suspend 
>>>>>>> path
>>>>>>> in ACPI.
>>>>>>> 
>>>>>>>  The issue we have with this however is one of timing - the call
>>>>>>> the tells
>>>>>>>  the MCU to this isn't able to complete before suspend is done 
>>>>>>> so
>>>>>>> we call
>>>>>>>  this in a prepare() and add a small msleep() to ensure it is
>>>>>>> done. This
>>>>>>>  must be done before the screen is switched off to prevent a
>>>>>>> variety of
>>>>>>>  possible races.
>>>>>> 
>>>>>>  Right now the way that Linux handles the LPS0 calls is that
>>>>>> they're all back to back.  Luke did try to inject a delay after
>>>>>> the LPS0 calls were done but before it went to sleep but this
>>>>>> wasn't sufficient.
>>>>>> 
>>>>>>  Another "potential" way to solve this problem from Linux may be
>>>>>> to actually glue the LPS0 screen off call to when DRM actually 
>>>>>> has
>>>>>> eDP turned off.
>>>>>> 
>>>>>>  Making such a change would essentially push back the "screen 
>>>>>> off"
>>>>>> LPS0 command to when the user has run 'systemctl suspend' (or an
>>>>>> action that did this) because the compositor usually turns it off
>>>>>> with DPMS at this time.
>>>>> 
>>>>>  I would be willing to test this if you want some concrete data.
>>>> 
>>>>  It would require some cross subsystem plumbing to evaluate
>>>> feasibility.
>>>>  I don't currently have any plans to do it.
>>>> 
>>>>  I think your patch makes sense; I just want to make it known that
>>>> "might" clean this up if it ever happens.
>>>> 
>>>>> See my big block of text below.
>>>>> 
>>>>>> 
>>>>>>  This is a much bigger change though and *much more ripe for
>>>>>> breakage*.
>>>>>> 
>>>>>>  So I think in may be worth leaving a TODO comment to look into
>>>>>> doing that in the future.
>>>>>  Do you mean add the TODO to a line in this patch?
>>>> 
>>>>  Yeah.  In case someone ever does it (me or otherwise) I think it
>>>> would be good to have some reference in the comments that the 
>>>> commit
>>>> 'might' be possible to revert.
>>>> 
>>>>> 
>>>>>> 
>>>>>>  If that ever happens; it's possible that this change could be
>>>>>> reverted too.
>>>>>> 
>>>>>>> 
>>>>>>>  Further to this the MCU powersave option must also be disabled
>>>>>>> as it can
>>>>>>>  cause a number of issues such as:
>>>>>>>  - unreliable resume connection of N-Key
>>>>>>>  - complete loss of N-Key if the power is plugged in while 
>>>>>>> suspended
>>>>>>>  Disabling the powersave option prevents this.
>>>>>>> 
>>>>>>>  Without this the MCU is unable to initialise itself correctly 
>>>>>>> on
>>>>>>> resume.
>>>>>> 
>>>>>>  initialize
>>>>> 
>>>>>  Are we forced to use USA spelling? I'm from NZ
>>>>>  "initialise is predominantly used in British English (used in
>>>>> UK/AU/NZ) ( en-GB )"
>>>>> 
>>>> 
>>>>  Ah I didn't realize it's an acceptable spelling for en-GB, and
>>>> thought it was just a typo; sorry.
>>>> 
>>>>>> 
>>>>>>> 
>>>>>>>  Signed-off-by: Luke D. Jones <luke@ljones.dev>
>>>>>> 
>>>>>>  I think it would be good to add a Closes: tag to the AMD Gitlab
>>>>>> issue that this was discussed within as well as any other public
>>>>>> references you know about.
>>>>>> 
>>>>>>  Additionally as Phoenix APU support goes back as far as kernel
>>>>>> 6.1 and this is well contained to only run on the ROG I suggest 
>>>>>> to
>>>>>> CC stable so that people can use the ROG on that LTS kernel or 
>>>>>> later.
>>>>>> 
>>>>>>> ---
>>>>>>>  -SNIP-
>>>>>>>    \x7f@@ -4701,6 +4749,8 @@ 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,
>>>>>> 
>>>>>>  Have you experimented with only using the prepare() call or only
>>>>>> the resume_early() call?  Are both really needed?
>>>>> 
>>>>>  I have yes. Although the device comes back eventually in resume
>>>>> after only a prepare call it's not preferable as it tends to 
>>>>> change
>>>>> the device path. With resume_early we can get the device replugged
>>>>> super early (before anything notices it's gone in fact).
>>>>> 
>>>>>  This whole thing is a bit of a mess. It ends up being a race
>>>>> between various things to prevent a HUB0 disconnect being
>>>>> registered by the xhci subsystem, and adding the device back 
>>>>> before
>>>>> the xhci subsystem gets control.
>>>>> 
>>>>>  If I add a sleep longer than 1300ms in prepare then the xhci
>>>>> subsys registers a disconnect of the USB0 hub. If the sleep is
>>>>> under 250ms it isn't quite enough for the MCU to do its thing, and
>>>>> on battery it seems worse.
>>>>> 
>>>>>  I have asked the ASUS guys I'm in contact with for something to
>>>>> disable this MCU behaviour since it is purely a workaround for a
>>>>> broken Windows thing :( They are open to something, maybe an OS
>>>>> detect in ACPI or a WMI method addition similar to the MCU
>>>>> powersave method, from what I'm told it would require an MCU
>>>>> firmware update along with BIOS update. If this eventuates I'll
>>>>> submit an additional patch to check and set that plus disable 
>>>>> this.
>>>> 
>>>>  Don't let them do an OS detection in ACPI, it's going to be too
>>>> painful.
>>>>  I would instead suggest that they can have a bit that you can
>>>> program in via ACPI or WMI from the ASUS WMI driver that says to
>>>> skip the MCU disconnect behavior.
>>>> 
>>> I totally agree, we do not need another _OSI(Linux) type of problem.
>>> Maybe those guys at Asus could just implement a ACPI _DSM for the 
>>> USB
>>> controller in question which allows for disabling this workaround.
>>> This would be preferable to an additional WMI method, since the
>>> notebook would otherwise depend on the asus-wmi driver to suspend
>>> properly.
>>> With the ACPI _DSM, the USB controller driver can disable the
>>> workaround as soon as the USB controller probes.
>> 
>> Would you be so kind as to explain what this means? My knowledge of
>> ACPI is paper thin and generally revolves just around the ASUS WMI
>> part. From what i can find the XHC0 (the hub the MCU is attached to)
>> doesn't have any current _DSM. I understand it means Device Specific
>> Method, so I guess you mean adding a method to be used only if that
>> HUB is there and implements it?
>> 
>> The ROG Ally depends on the asus-wmi driver regardless, without it, 
>> it
>> is barely functional.
>> 
> An ACPI _DSM method is a "standardized" way for manufacturers to add 
> device specific methods to ACPI devices, see
> https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/09_ACPI-Defined_Devices_and_Device-Specific_Objects/ACPIdefined_Devices_and_DeviceSpecificObjects.html 
> for details.
> 
> Implementing via a ACPI _DSM would contain the necessary logic for 
> USB suspend inside the USB hub driver, instead of forcing asus-wmi 
> and the USB hub driver
> to collaborate on USB suspend. The USB hub driver would discover this 
> device specific method during probe and disable the workaround, 
> leaving no chance for
> race conditions between asus-wmi and the USB hub driver to develop.
> 
> Of course if the ROG Ally depends on asus-wmi regardless, then Asus 
> could as well use the WMI interface for this. But IMHO standard ACPI 
> interfaces should be
> preferred to custom ones like WMI.

Okay i understand now, thank you. I'll make a note of it in my next 
response to ASUS.

> 
>>>>> 
>>>>>  I may possibly write a new version of this patch as we've seen
>>>>> that enabling powersave reduces suspend power use by at least 
>>>>> half.
>>>>> And looking through my DSDT dumps, there are a few laptops with 
>>>>> the
>>>>> same feature as Ally. The patch for powersave being enabled
>>>>> requires also AC power state on suspend change detection, and a
>>>>> later forced reset in late resume (and the device paths change
>>>>> regardless when powersave is on).
>>>>> 
>>>>>  When I look at it objectively, the device path changing should be
>>>>> a non-issue really as it is fully handled by USB subsystem and
>>>>> behaves exactly like what it is - a USB hub disconnect. It's just
>>>>> that some userspace apps don't expect this. I will experiment some
>>>>> more.
>>>>> 
>>>>>  Regards,
>>>>>  Luke.
>>>>> 
>>>> 
>>>>  As another experiment - what happens if you "comment out" the LPS0
>>>> calls that do this problematic stuff?
>>>> 
>>>>  It's important to make sure the callback to amd-pmc stays in 
>>>> place,
>>>> but if you just skip those ACPI ones does it still get to the
>>>> deepest state and are there other problems?
>>>> 
>>>>>> 
>>>>>>>   };
>>>>>>>    \x7f  /* Registration
>>>>>>> ***************************************************************/
>>>>>>>  diff --git a/include/linux/platform_data/x86/asus-wmi.h
>>>>>>> b/include/linux/platform_data/x86/asus-wmi.h
>>>>>>>  index 63e630276499..ab1c7deff118 100644
>>>>>>>  --- a/include/linux/platform_data/x86/asus-wmi.h
>>>>>>>  +++ b/include/linux/platform_data/x86/asus-wmi.h
>>>>>>>  @@ -114,6 +114,9 @@
>>>>>>>    /* Charging mode - 1=Barrel, 2=USB */
>>>>>>>    #define ASUS_WMI_DEVID_CHARGE_MODE    0x0012006C
>>>>>>>    \x7f+/* MCU powersave mode */
>>>>>>>  +#define ASUS_WMI_DEVID_MCU_POWERSAVE   0x001200E2
>>>>>>>  +
>>>>>>>    /* epu is connected? 1 == true */
>>>>>>>    #define ASUS_WMI_DEVID_EGPU_CONNECTED    0x00090018
>>>>>>>    /* egpu on/off */
>>>>>> 
>>>>> 
>>>>> 
>>>> 
>>>> 
>> 
>> 
>> 

Re: [PATCH v2 1/1] platform/x86: asus-wmi: disable USB0 hub on ROG Ally before suspend

[Mario Limonciello]

On 11/27/2023 19:20, Luke Jones wrote:
> 
> 
> Okay i understand now, thank you. I'll make a note of it in my next 
> response to ASUS.
> 
Another thought here - could they be doing this MCU on/off control as 
part of the ACPI power resource on and off methods for the USB 
controller and embed any required waits there instead?

I would think this could then be "invisible" to any drivers on any OS.

Re: [PATCH v2 0/1] platform/x86: asus-wmi: disable USB0 hub on ROG Ally before suspend

[Ilpo Järvinen]

On Mon, 27 Nov 2023 12:05:20 +1300, Luke D. Jones wrote:

> This is a fix for the ROG Ally not being able to use the N-Key device after a suspend/resume cycle.
> 
> The root of the issue is that ASUS changed the MCU firmware to dfisconnect the USB0 hub when the
> screen is switched off during the s2idle suspend path. I tried many many different tactics to try
> and get this s2idle part to work but it seems there are races between this and other subsystems.
> 
> What has so far been reliable and consistent is a manual call to the CSEE method that is called in
> ACPI by the Microsoft DSM screen off path followed by a short sleep in asus-wmi. The PM prepare
> hook looks to be the earliest possible place. A sleep that is too long ends up with USB subsystem
> registering a disconnect, and thus on resume the device paths change. Too short and it is unreliable.
> 
> [...]


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

The list of commits applied:
[1/1] platform/x86: asus-wmi: disable USB0 hub on ROG Ally before suspend
      commit: e0894ff038d86f30614ec16ec26dacb88c8d2bd4

--
 i.