lkml.org 
[lkml]   [2023]   [Nov]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v2 1/1] platform/x86: asus-wmi: disable USB0 hub on ROG Ally before suspend
From
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-
>>>   @@ -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?

>>
>>>   };
>>>     /* 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 */
>>
>
>

\
 
 \ /
  Last update: 2023-11-27 21:57    [W:0.085 / U:0.744 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site