[PATCH 0/3] platfom/x86: asus-wmi: revert ROG Ally quirks

[PATCH 0/3] platfom/x86: asus-wmi: revert ROG Ally quirks

[Luke D. Jones]

The ASUS ROG Ally (and Ally X) quirks that I added over the last year
are not required. I worked with ASUS to pinpoint the exact cause of
the original issue (MCU USB dev missing every second resume) and the
result is a new MCU firmware which will be released on approx 16/10/24.

All users should update to MCU FW as soon as released to:
- Ally 1: v319
- Ally X: v313

Luke D. Jones (3):
  Revert "platform/x86: asus-wmi: ROG Ally increase wait time, allow MCU
    powersave"
  Revert "platform/x86: asus-wmi: disable USB0 hub on ROG Ally before
    suspend"
  platfom/x86: asus-wmi: cleanup after Ally quirk reverts

 drivers/platform/x86/asus-wmi.c | 39 +--------------------------------
 1 file changed, 1 insertion(+), 38 deletions(-)

-- 
2.46.1

[PATCH 1/3] Revert "platform/x86: asus-wmi: ROG Ally increase wait time, allow MCU powersave"

[Luke D. Jones]

This reverts commit 7e7a5dee49732ed01a3a17c9a3edf027fb9457fe.

This is part of some hacks that are no-longer required when users have
updated their MCU firmware to the latest version:
- Ally 1: v319
- Ally X: v313

Signed-off-by: Luke D. Jones <luke@ljones.dev>
---
 drivers/platform/x86/asus-wmi.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 08861792bddd..8daefd9a0d94 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -146,7 +146,7 @@ module_param(fnlock_default, bool, 0444);
 /* Controls the power state of the USB0 hub on ROG Ally which input is on */
 #define ASUS_USB0_PWR_EC0_CSEE "\\_SB.PCI0.SBRG.EC0.CSEE"
 /* 300ms so far seems to produce a reliable result on AC and battery */
-#define ASUS_USB0_PWR_EC0_CSEE_WAIT 1500
+#define ASUS_USB0_PWR_EC0_CSEE_WAIT 300
 
 static const char * const ashs_ids[] = { "ATK4001", "ATK4002", NULL };
 
@@ -4971,7 +4971,6 @@ static int asus_hotk_resume_early(struct device *device)
 	struct asus_wmi *asus = dev_get_drvdata(device);
 
 	if (asus->ally_mcu_usb_switch) {
-		/* sleep required to prevent USB0 being yanked then reappearing rapidly */
 		if (ACPI_FAILURE(acpi_execute_simple_method(NULL, ASUS_USB0_PWR_EC0_CSEE, 0xB8)))
 			dev_err(device, "ROG Ally MCU failed to connect USB dev\n");
 		else
@@ -4983,8 +4982,17 @@ static int asus_hotk_resume_early(struct device *device)
 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");
-- 
2.46.1

[PATCH 2/3] Revert "platform/x86: asus-wmi: disable USB0 hub on ROG Ally before suspend"

[Luke D. Jones]

This reverts commit e0894ff038d86f30614ec16ec26dacb88c8d2bd4.

This is part of some hacks that are no-longer required when users have
updated their MCU firmware to the latest version:
- Ally 1: v319
- Ally X: v313

Signed-off-by: Luke D. Jones <luke@ljones.dev>
---
 drivers/platform/x86/asus-wmi.c | 40 ---------------------------------
 1 file changed, 40 deletions(-)

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 8daefd9a0d94..27b9fa2c29ae 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -16,7 +16,6 @@
 #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>
@@ -4962,43 +4961,6 @@ 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;
 }
 
@@ -5046,8 +5008,6 @@ 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 ***************************************************************/
-- 
2.46.1

[PATCH 3/3] platfom/x86: asus-wmi: cleanup after Ally quirk reverts

[Luke D. Jones]

Cleanup some leftover code after reverting:
- 7e7a5dee49732ed01a3a17c9a3edf027fb9457fe
- e0894ff038d86f30614ec16ec26dacb88c8d2bd4

This is part of some hacks that are no-longer required when users have
updated their MCU firmware to the latest version:
- Ally 1: v319
- Ally X: v313

Signed-off-by: Luke D. Jones <luke@ljones.dev>
---
 drivers/platform/x86/asus-wmi.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 27b9fa2c29ae..2c4094004549 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -274,9 +274,6 @@ struct asus_wmi {
 	u32 tablet_switch_dev_id;
 	bool tablet_switch_inverted;
 
-	/* The ROG Ally device requires the MCU USB device be disconnected before suspend */
-	bool ally_mcu_usb_switch;
-
 	enum fan_type fan_type;
 	enum fan_type gpu_fan_type;
 	enum fan_type mid_fan_type;
@@ -4773,8 +4770,6 @@ static int asus_wmi_add(struct platform_device *pdev)
 	asus->egpu_enable_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_EGPU);
 	asus->dgpu_disable_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_DGPU);
 	asus->kbd_rgb_state_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_TUF_RGB_STATE);
-	asus->ally_mcu_usb_switch = acpi_has_method(NULL, ASUS_USB0_PWR_EC0_CSEE)
-						&& dmi_check_system(asus_ally_mcu_quirk);
 
 	if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_MINI_LED_MODE))
 		asus->mini_led_dev_id = ASUS_WMI_DEVID_MINI_LED_MODE;
-- 
2.46.1

Re: [PATCH 0/3] platfom/x86: asus-wmi: revert ROG Ally quirks

[Mario Limonciello]

On 9/26/2024 04:53, Luke D. Jones wrote:
> The ASUS ROG Ally (and Ally X) quirks that I added over the last year
> are not required. I worked with ASUS to pinpoint the exact cause of
> the original issue (MCU USB dev missing every second resume) and the
> result is a new MCU firmware which will be released on approx 16/10/24.
> 
> All users should update to MCU FW as soon as released to:
> - Ally 1: v319
> - Ally X: v313
> 
> Luke D. Jones (3):
>    Revert "platform/x86: asus-wmi: ROG Ally increase wait time, allow MCU
>      powersave"
>    Revert "platform/x86: asus-wmi: disable USB0 hub on ROG Ally before
>      suspend"
>    platfom/x86: asus-wmi: cleanup after Ally quirk reverts
> 
>   drivers/platform/x86/asus-wmi.c | 39 +--------------------------------
>   1 file changed, 1 insertion(+), 38 deletions(-)
> 

This series looks good to me, but I would suggest that you also in the 
appropriate HID driver that communicates with the MCU to show a warning 
or notice if the version is below the required version.

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

Re: [PATCH 3/3] platfom/x86: asus-wmi: cleanup after Ally quirk reverts

[kernel test robot]

Hi Luke,

kernel test robot noticed the following build warnings:

[auto build test WARNING on amd-pstate/bleeding-edge]
[also build test WARNING on linus/master v6.11 next-20240927]
[cannot apply to amd-pstate/linux-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Luke-D-Jones/Revert-platform-x86-asus-wmi-ROG-Ally-increase-wait-time-allow-MCU-powersave/20240926-175641
base:   https://git.kernel.org/pub/scm/linux/kernel/git/superm1/linux.git bleeding-edge
patch link:    https://lore.kernel.org/r/20240926095344.1291013-4-luke%40ljones.dev
patch subject: [PATCH 3/3] platfom/x86: asus-wmi: cleanup after Ally quirk reverts
config: x86_64-rhel-8.3-func (https://download.01.org/0day-ci/archive/20240929/202409290438.IYVkP4i0-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240929/202409290438.IYVkP4i0-lkp@intel.com/reproduce)

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

All warnings (new ones prefixed by >>):

>> drivers/platform/x86/asus-wmi.c:148:35: warning: 'asus_ally_mcu_quirk' defined but not used [-Wunused-const-variable=]
     148 | static const struct dmi_system_id asus_ally_mcu_quirk[] = {
         |                                   ^~~~~~~~~~~~~~~~~~~


vim +/asus_ally_mcu_quirk +148 drivers/platform/x86/asus-wmi.c

0f0ac158d28ff7 Luke D. Jones 2021-10-24  147  
d2dfed310aae07 Luke D. Jones 2024-08-06 @148  static const struct dmi_system_id asus_ally_mcu_quirk[] = {
d2dfed310aae07 Luke D. Jones 2024-08-06  149  	{
d2dfed310aae07 Luke D. Jones 2024-08-06  150  		.matches = {
d2dfed310aae07 Luke D. Jones 2024-08-06  151  			DMI_MATCH(DMI_BOARD_NAME, "RC71L"),
d2dfed310aae07 Luke D. Jones 2024-08-06  152  		},
d2dfed310aae07 Luke D. Jones 2024-08-06  153  	},
d2dfed310aae07 Luke D. Jones 2024-08-06  154  	{
d2dfed310aae07 Luke D. Jones 2024-08-06  155  		.matches = {
d2dfed310aae07 Luke D. Jones 2024-08-06  156  			DMI_MATCH(DMI_BOARD_NAME, "RC72L"),
d2dfed310aae07 Luke D. Jones 2024-08-06  157  		},
d2dfed310aae07 Luke D. Jones 2024-08-06  158  	},
d2dfed310aae07 Luke D. Jones 2024-08-06  159  	{ },
d2dfed310aae07 Luke D. Jones 2024-08-06  160  };
d2dfed310aae07 Luke D. Jones 2024-08-06  161  

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

Re: [PATCH 0/3] platfom/x86: asus-wmi: revert ROG Ally quirks

[Luke Jones]

On Fri, 27 Sep 2024, at 1:36 AM, Mario Limonciello wrote:
> On 9/26/2024 04:53, Luke D. Jones wrote:
>> The ASUS ROG Ally (and Ally X) quirks that I added over the last year
>> are not required. I worked with ASUS to pinpoint the exact cause of
>> the original issue (MCU USB dev missing every second resume) and the
>> result is a new MCU firmware which will be released on approx 16/10/24.
>> 
>> All users should update to MCU FW as soon as released to:
>> - Ally 1: v319
>> - Ally X: v313
>> 
>> Luke D. Jones (3):
>>    Revert "platform/x86: asus-wmi: ROG Ally increase wait time, allow MCU
>>      powersave"
>>    Revert "platform/x86: asus-wmi: disable USB0 hub on ROG Ally before
>>      suspend"
>>    platfom/x86: asus-wmi: cleanup after Ally quirk reverts
>> 
>>   drivers/platform/x86/asus-wmi.c | 39 +--------------------------------
>>   1 file changed, 1 insertion(+), 38 deletions(-)
>> 
>
> This series looks good to me, but I would suggest that you also in the 
> appropriate HID driver that communicates with the MCU to show a warning 
> or notice if the version is below the required version.
>
> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>

I have something that works, but it's in the hid-asus-ally driver. Since that might take a while to prepare I think I'll bring it over to this series soon.

Re: [PATCH 0/3] platfom/x86: asus-wmi: revert ROG Ally quirks

[Hans de Goede]

Hi Luke,

On 26-Sep-24 11:53 AM, Luke D. Jones wrote:
> The ASUS ROG Ally (and Ally X) quirks that I added over the last year
> are not required. I worked with ASUS to pinpoint the exact cause of
> the original issue (MCU USB dev missing every second resume) and the
> result is a new MCU firmware which will be released on approx 16/10/24.

First of all let me say that it is great that you have gotten Asus
to come up with a fixed firmware, thank you.

With that said I believe that it is way too early to revert these quirks,
users are usually not great at installing BIOS updates and that assumes
this will be handled as part of a BIOS update, if it requires running
a separate tool then the chances of users not installing the update
will likely be even worse.

So IMHO for now we should keep these quirks around to avoid regressions
for users who don't have the MCU update.

Related, have you seen this series:

https://lore.kernel.org/platform-driver-x86/20240922172258.48435-1-lkml@antheas.dev/

that seems to fix the same issue ?

And it does so in another, arguably better way.

Although unfortunately as patch 3/5 shows just calling the global
"display off" callback before suspending devices is not enough
fixing things still requires inserting a sleep using a DMI quirk :|

Still that series including the DMI quirk might be a cleaner way
to deal with this and if that is merged then dropping the quirks
from asus-wmi makes sense.

Regards,

Hans




> All users should update to MCU FW as soon as released to:
> - Ally 1: v319
> - Ally X: v313
> 
> Luke D. Jones (3):
>   Revert "platform/x86: asus-wmi: ROG Ally increase wait time, allow MCU
>     powersave"
>   Revert "platform/x86: asus-wmi: disable USB0 hub on ROG Ally before
>     suspend"
>   platfom/x86: asus-wmi: cleanup after Ally quirk reverts
> 
>  drivers/platform/x86/asus-wmi.c | 39 +--------------------------------
>  1 file changed, 1 insertion(+), 38 deletions(-)
> 

Re: [PATCH 0/3] platfom/x86: asus-wmi: revert ROG Ally quirks

[Luke Jones]

Hi Hans,

On Sun, 6 Oct 2024, at 3:37 AM, Hans de Goede wrote:
> Hi Luke,
>
> On 26-Sep-24 11:53 AM, Luke D. Jones wrote:
>> The ASUS ROG Ally (and Ally X) quirks that I added over the last year
>> are not required. I worked with ASUS to pinpoint the exact cause of
>> the original issue (MCU USB dev missing every second resume) and the
>> result is a new MCU firmware which will be released on approx 16/10/24.
>
> First of all let me say that it is great that you have gotten Asus
> to come up with a fixed firmware, thank you.
>
> With that said I believe that it is way too early to revert these quirks,
> users are usually not great at installing BIOS updates and that assumes
> this will be handled as part of a BIOS update, if it requires running
> a separate tool then the chances of users not installing the update
> will likely be even worse.
>
> So IMHO for now we should keep these quirks around to avoid regressions
> for users who don't have the MCU update.

I wasn't sure how best to handle it, mostly the intention was to publicise things. In any case the quirks don't affect the new FW update at all and most folks won't ever notice.

> Related, have you seen this series:
>
> https://lore.kernel.org/platform-driver-x86/20240922172258.48435-1-lkml@antheas.dev/
>
> that seems to fix the same issue ?

The history of that is here https://lore.kernel.org/linux-pm/20240919171952.403745-1-lkml@antheas.dev/#t

> And it does so in another, arguably better way.

It is a variation of the many many things I've tried while building a comprehensive set of data for ASUS to work with. You can achieve a similar thing with only s2idle_pm callbacks and Mario's patches to export the DSM screen-off as an external symbol. Better is subjective since it still fails to fix the initial reason this work ever started - fixing the Ally - unless delays are added.

> Although unfortunately as patch 3/5 shows just calling the global
> "display off" callback before suspending devices is not enough
> fixing things still requires inserting a sleep using a DMI quirk :|

This is because the issue can only be fully fixed in FW. What is happening here is just another variation of the quirk and the things I mentioned above. It gets worse with different compiler such as clang, or different kernel config, or even distro. The cause of issues is that a particular signal the MCU is waiting on may not occur and that becomes wildly unpredictable depending on kernel config, compiler etc.

Even Windows can have the issue we have here.

> Still that series including the DMI quirk might be a cleaner way
> to deal with this and if that is merged then dropping the quirks
> from asus-wmi makes sense.

All of this is fully negated by the coming firmware. Having said that, *if* there are any issues with these patches then those issues will never come to light with the new MCU FW either as it fixes the root cause of the issues seen.

The mentioned patches achieve a similar result to using Mario's s2idle callback patches and using those in s2idle_pm_ops. But as seen above, the timing issue becomes apparent - and this is fixed only by using fixed FW.

Kind regards,
Luke.

> Regards,
>
> Hans
>
>
>
>
>> All users should update to MCU FW as soon as released to:
>> - Ally 1: v319
>> - Ally X: v313
>> 
>> Luke D. Jones (3):
>>   Revert "platform/x86: asus-wmi: ROG Ally increase wait time, allow MCU
>>     powersave"
>>   Revert "platform/x86: asus-wmi: disable USB0 hub on ROG Ally before
>>     suspend"
>>   platfom/x86: asus-wmi: cleanup after Ally quirk reverts
>> 
>>  drivers/platform/x86/asus-wmi.c | 39 +--------------------------------
>>  1 file changed, 1 insertion(+), 38 deletions(-)
>>

Re: [PATCH 0/3] platfom/x86: asus-wmi: revert ROG Ally quirks

[Hans de Goede]

Hi Luke,

On 5-Oct-24 9:48 PM, Luke Jones wrote:
> Hi Hans,
> 
> On Sun, 6 Oct 2024, at 3:37 AM, Hans de Goede wrote:
>> Hi Luke,
>>
>> On 26-Sep-24 11:53 AM, Luke D. Jones wrote:
>>> The ASUS ROG Ally (and Ally X) quirks that I added over the last year
>>> are not required. I worked with ASUS to pinpoint the exact cause of
>>> the original issue (MCU USB dev missing every second resume) and the
>>> result is a new MCU firmware which will be released on approx 16/10/24.
>>
>> First of all let me say that it is great that you have gotten Asus
>> to come up with a fixed firmware, thank you.
>>
>> With that said I believe that it is way too early to revert these quirks,
>> users are usually not great at installing BIOS updates and that assumes
>> this will be handled as part of a BIOS update, if it requires running
>> a separate tool then the chances of users not installing the update
>> will likely be even worse.
>>
>> So IMHO for now we should keep these quirks around to avoid regressions
>> for users who don't have the MCU update.
> 
> I wasn't sure how best to handle it, mostly the intention was to publicise things. In any case the quirks don't affect the new FW update at all and most folks won't ever notice.

I think we can look at dropping the quirks in maybe a year from now
or some such. Doing it right now feels like a bit to quick after
the fw fix.

And as mentioned elsewhere in the thread, if possible it would be
good if some other driver. e.g. hid-asus could check the FW version
and log a warning if the old version is still found.

>> Related, have you seen this series:
>>
>> https://lore.kernel.org/platform-driver-x86/20240922172258.48435-1-lkml@antheas.dev/
>>
>> that seems to fix the same issue ?
> 
> The history of that is here https://lore.kernel.org/linux-pm/20240919171952.403745-1-lkml@antheas.dev/#t
> 
>> And it does so in another, arguably better way.
> 
> It is a variation of the many many things I've tried while building a comprehensive set of data for ASUS to work with. You can achieve a similar thing with only s2idle_pm callbacks and Mario's patches to export the DSM screen-off as an external symbol. Better is subjective since it still fails to fix the initial reason this work ever started - fixing the Ally - unless delays are added.
> 
>> Although unfortunately as patch 3/5 shows just calling the global
>> "display off" callback before suspending devices is not enough
>> fixing things still requires inserting a sleep using a DMI quirk :|
> 
> This is because the issue can only be fully fixed in FW. What is happening here is just another variation of the quirk and the things I mentioned above. It gets worse with different compiler such as clang, or different kernel config, or even distro. The cause of issues is that a particular signal the MCU is waiting on may not occur and that becomes wildly unpredictable depending on kernel config, compiler etc.
> 
> Even Windows can have the issue we have here.
> 
>> Still that series including the DMI quirk might be a cleaner way
>> to deal with this and if that is merged then dropping the quirks
>> from asus-wmi makes sense.
> 
> All of this is fully negated by the coming firmware. Having said that, *if* there are any issues with these patches then those issues will never come to light with the new MCU FW either as it fixes the root cause of the issues seen.

That sounds great, once more thank you for working with Asus to
properly fix this.

> The mentioned patches achieve a similar result to using Mario's s2idle callback patches and using those in s2idle_pm_ops. But as seen above, the timing issue becomes apparent - and this is fixed only by using fixed FW.

Right. As I mentioned already in the other thread I am having second
doubts about moving the LPS0 display power off call to before devices
are suspended, doing so would mean that the display might still be on
when that call is made and that call could disable power-resources which
are necessary for the display causing issues when the display driver's
suspend method runs.

So I think that we need something closer to Mario's original POC from:

https://git.kernel.org/pub/scm/linux/kernel/git/superm1/linux.git/log/?h=superm1/dsm-screen-on-off

if we want to make the suspend order more like Windows and make
the LPS0 display off call when the last display is turned off.

And as you have explained making the suspend order more like Windows
is unrelated to the real cause for the ROG Ally MCU suspend issue,
so lets continue any discussion about suspend ordering in the other
thread.

Regards,

Hans

Re: [PATCH 0/3] platfom/x86: asus-wmi: revert ROG Ally quirks

[Luke Jones]

On Sun, 6 Oct 2024, at 8:48 AM, Luke Jones wrote:
> Hi Hans,
>
> On Sun, 6 Oct 2024, at 3:37 AM, Hans de Goede wrote:
>> Hi Luke,
>>
>> On 26-Sep-24 11:53 AM, Luke D. Jones wrote:
>>> The ASUS ROG Ally (and Ally X) quirks that I added over the last year
>>> are not required. I worked with ASUS to pinpoint the exact cause of
>>> the original issue (MCU USB dev missing every second resume) and the
>>> result is a new MCU firmware which will be released on approx 16/10/24.
>>
>> First of all let me say that it is great that you have gotten Asus
>> to come up with a fixed firmware, thank you.
>>
>> With that said I believe that it is way too early to revert these quirks,
>> users are usually not great at installing BIOS updates and that assumes
>> this will be handled as part of a BIOS update, if it requires running
>> a separate tool then the chances of users not installing the update
>> will likely be even worse.
>>
>> So IMHO for now we should keep these quirks around to avoid regressions
>> for users who don't have the MCU update.
>
> I wasn't sure how best to handle it, mostly the intention was to 
> publicise things. In any case the quirks don't affect the new FW update 
> at all and most folks won't ever notice.
>
>> Related, have you seen this series:
>>
>> https://lore.kernel.org/platform-driver-x86/20240922172258.48435-1-lkml@antheas.dev/
>>
>> that seems to fix the same issue ?
>
> The history of that is here 
> https://lore.kernel.org/linux-pm/20240919171952.403745-1-lkml@antheas.dev/#t
>
>> And it does so in another, arguably better way.
>
> It is a variation of the many many things I've tried while building a 
> comprehensive set of data for ASUS to work with. You can achieve a 
> similar thing with only s2idle_pm callbacks and Mario's patches to 
> export the DSM screen-off as an external symbol. Better is subjective 
> since it still fails to fix the initial reason this work ever started - 
> fixing the Ally - unless delays are added.
>
>> Although unfortunately as patch 3/5 shows just calling the global
>> "display off" callback before suspending devices is not enough
>> fixing things still requires inserting a sleep using a DMI quirk :|
>
> This is because the issue can only be fully fixed in FW. What is 
> happening here is just another variation of the quirk and the things I 
> mentioned above. It gets worse with different compiler such as clang, 
> or different kernel config, or even distro. The cause of issues is that 
> a particular signal the MCU is waiting on may not occur and that 
> becomes wildly unpredictable depending on kernel config, compiler etc.
>
> Even Windows can have the issue we have here.
>
>> Still that series including the DMI quirk might be a cleaner way
>> to deal with this and if that is merged then dropping the quirks
>> from asus-wmi makes sense.
>
> All of this is fully negated by the coming firmware. Having said that, 
> *if* there are any issues with these patches then those issues will 
> never come to light with the new MCU FW either as it fixes the root 
> cause of the issues seen.
>
> The mentioned patches achieve a similar result to using Mario's s2idle 
> callback patches and using those in s2idle_pm_ops. But as seen above, 
> the timing issue becomes apparent - and this is fixed only by using 
> fixed FW.

Hi Mario,

I am now wondering if there is some merit still in upstreaming your original series plus the asus-wmi patch based on those. The benefit for asus-wmi is that it is cleaned up a lot, and the delay can be reduced.

I can likely achieve the same thing using the CSEE calls manually but shifting to the s2idle_pm callback for suspend since the main thing is just ensuring device recovery with no regard for powersave.

The main reason for this consideration now is due to requiring some form of it to remain for a while as Hans requests. I already know from testing that there is zero negative effect on the coming MCU update and most users won't notice at all, but I would very much like to improve the current hack.

One other consideration though is that I've found it pretty difficult to get some kind of safe HID request done through asus-wmi to fetch the MCU FW version and disable this quirk based on that, but I could possibly go in the other direction and have the hid-asus-* drivers find the asus-ally driver data and flip a switch to on/off these quirks. That would apply only for suspend/resume and not the reboot fix though.


> Kind regards,
> Luke.
>
>> Regards,
>>
>> Hans
>>
>>
>>
>>
>>> All users should update to MCU FW as soon as released to:
>>> - Ally 1: v319
>>> - Ally X: v313
>>> 
>>> Luke D. Jones (3):
>>>   Revert "platform/x86: asus-wmi: ROG Ally increase wait time, allow MCU
>>>     powersave"
>>>   Revert "platform/x86: asus-wmi: disable USB0 hub on ROG Ally before
>>>     suspend"
>>>   platfom/x86: asus-wmi: cleanup after Ally quirk reverts
>>> 
>>>  drivers/platform/x86/asus-wmi.c | 39 +--------------------------------
>>>  1 file changed, 1 insertion(+), 38 deletions(-)
>>>

Re: [PATCH 0/3] platfom/x86: asus-wmi: revert ROG Ally quirks

[Mario Limonciello]

On 10/7/2024 19:03, Luke Jones wrote:
> 
> 
> On Sun, 6 Oct 2024, at 8:48 AM, Luke Jones wrote:
>> Hi Hans,
>>
>> On Sun, 6 Oct 2024, at 3:37 AM, Hans de Goede wrote:
>>> Hi Luke,
>>>
>>> On 26-Sep-24 11:53 AM, Luke D. Jones wrote:
>>>> The ASUS ROG Ally (and Ally X) quirks that I added over the last year
>>>> are not required. I worked with ASUS to pinpoint the exact cause of
>>>> the original issue (MCU USB dev missing every second resume) and the
>>>> result is a new MCU firmware which will be released on approx 16/10/24.
>>>
>>> First of all let me say that it is great that you have gotten Asus
>>> to come up with a fixed firmware, thank you.
>>>
>>> With that said I believe that it is way too early to revert these quirks,
>>> users are usually not great at installing BIOS updates and that assumes
>>> this will be handled as part of a BIOS update, if it requires running
>>> a separate tool then the chances of users not installing the update
>>> will likely be even worse.
>>>
>>> So IMHO for now we should keep these quirks around to avoid regressions
>>> for users who don't have the MCU update.
>>
>> I wasn't sure how best to handle it, mostly the intention was to
>> publicise things. In any case the quirks don't affect the new FW update
>> at all and most folks won't ever notice.
>>
>>> Related, have you seen this series:
>>>
>>> https://lore.kernel.org/platform-driver-x86/20240922172258.48435-1-lkml@antheas.dev/
>>>
>>> that seems to fix the same issue ?
>>
>> The history of that is here
>> https://lore.kernel.org/linux-pm/20240919171952.403745-1-lkml@antheas.dev/#t
>>
>>> And it does so in another, arguably better way.
>>
>> It is a variation of the many many things I've tried while building a
>> comprehensive set of data for ASUS to work with. You can achieve a
>> similar thing with only s2idle_pm callbacks and Mario's patches to
>> export the DSM screen-off as an external symbol. Better is subjective
>> since it still fails to fix the initial reason this work ever started -
>> fixing the Ally - unless delays are added.
>>
>>> Although unfortunately as patch 3/5 shows just calling the global
>>> "display off" callback before suspending devices is not enough
>>> fixing things still requires inserting a sleep using a DMI quirk :|
>>
>> This is because the issue can only be fully fixed in FW. What is
>> happening here is just another variation of the quirk and the things I
>> mentioned above. It gets worse with different compiler such as clang,
>> or different kernel config, or even distro. The cause of issues is that
>> a particular signal the MCU is waiting on may not occur and that
>> becomes wildly unpredictable depending on kernel config, compiler etc.
>>
>> Even Windows can have the issue we have here.
>>
>>> Still that series including the DMI quirk might be a cleaner way
>>> to deal with this and if that is merged then dropping the quirks
>>> from asus-wmi makes sense.
>>
>> All of this is fully negated by the coming firmware. Having said that,
>> *if* there are any issues with these patches then those issues will
>> never come to light with the new MCU FW either as it fixes the root
>> cause of the issues seen.
>>
>> The mentioned patches achieve a similar result to using Mario's s2idle
>> callback patches and using those in s2idle_pm_ops. But as seen above,
>> the timing issue becomes apparent - and this is fixed only by using
>> fixed FW.
> 
> Hi Mario,
> 
> I am now wondering if there is some merit still in upstreaming your original series plus the asus-wmi patch based on those. The benefit for asus-wmi is that it is cleaned up a lot, and the delay can be reduced.
> 
> I can likely achieve the same thing using the CSEE calls manually but shifting to the s2idle_pm callback for suspend since the main thing is just ensuring device recovery with no regard for powersave.
> 
> The main reason for this consideration now is due to requiring some form of it to remain for a while as Hans requests. I already know from testing that there is zero negative effect on the coming MCU update and most users won't notice at all, but I would very much like to improve the current hack.
> 

I'm totally fine with that.  As you have the asus-wmi patch based on 
those that worked best would you mind dropping it all together as a 
series to all 3 mailing lists (linux-pm/dri-devel/platform-x86?

I double checked and my 3 patches apply cleanly still on 6.12-rc2.

> One other consideration though is that I've found it pretty difficult to get some kind of safe HID request done through asus-wmi to fetch the MCU FW version and disable this quirk based on that, but I could possibly go in the other direction and have the hid-asus-* drivers find the asus-ally driver data and flip a switch to on/off these quirks. That would apply only for suspend/resume and not the reboot fix though.

I think once the hid-asus drivers land you can export a symbol that 
provides that information of the firmware version.  asus-wmi can have a 
dependency on those drivers and call that symbol and then make decisions 
on the quirks based on it.

> 
> 
>> Kind regards,
>> Luke.
>>
>>> Regards,
>>>
>>> Hans
>>>
>>>
>>>
>>>
>>>> All users should update to MCU FW as soon as released to:
>>>> - Ally 1: v319
>>>> - Ally X: v313
>>>>
>>>> Luke D. Jones (3):
>>>>    Revert "platform/x86: asus-wmi: ROG Ally increase wait time, allow MCU
>>>>      powersave"
>>>>    Revert "platform/x86: asus-wmi: disable USB0 hub on ROG Ally before
>>>>      suspend"
>>>>    platfom/x86: asus-wmi: cleanup after Ally quirk reverts
>>>>
>>>>   drivers/platform/x86/asus-wmi.c | 39 +--------------------------------
>>>>   1 file changed, 1 insertion(+), 38 deletions(-)
>>>>
>