Re: [PATCH v2 0/5] acpi/x86: s2idle: move Display off/on calls outside suspend (fixes ROG Ally suspend)

["Luke Jones" <luke@ljones.dev>]

On Mon, 23 Sep 2024, at 5:57 PM, Antheas Kapenekakis wrote:
> Hi Derek,
> 
> > I'm going to be somewhat brief here as I don't like repeating myself, you are
> > working from incomplete information and from that you are inferring incorrect
> > assertions. Due to NDA the full slate of information that would clarify this
> > cannot be released here, but I will be clear: I am not sharing my opinion, I
> > am stating facts. What you have described here is a missinterpretation of the
> > symptoms and is not correct. The _DSM call is not relevant to the proper fix,
> > the sequencing you observe is not applicable to Linux, and the sensitivity of
> > the controller is another symptom of the Windows quirk behaving badly in Linux.
> > Furthermore, the RGB "flourish" as you call it works as intended with the new
> > firmware and no kernel changes required.
> 
> Only thing I am seeing here is a bit of heresay. Please get in touch

What Derek is saying here is correct because it is based on data I have shared with him for testing to help ASUS engineers fix the root issue in firmware.

> with Asus, cross your t's and dot your i's, and share what you found
> with the rest of the class.

I have direct contact with engineers in ASUS and I am under NDA (extended to a few others) so certain information can not be shared. The proper fix for Linux is done in the MCU firmware - this is something ASUS engineers who work on the Ally devices have done with our aid in pinpointing the exact cause.

> I am in no rush to see this merge, and as all gaming distributions
> carry custom kernels that update on a weekly cadence and 90% of users
> are on those kernels with the rest being able to figure it out, there
> is no practical reason for this to merge quickly. Ubuntu (+variant)
> users will get this fix after they've thrown their ally away anyway.

Lucky for Ubuntu and variant users, they can simply update their firmware and have suspend/resume/reboot all work fine.

> > What I can provide is information on a test we did that should hopefully
> > elucidate the issue more clearly for you. We included a patch that allowed us
> > to alter the delay in asus-wmi on the fly by writing to an attribute in sysfs.
> > In addition, we pushed the _DSM calls as early as possible in the suspend
> > sequence. We were unable to find a timing for this that would work consistently
> > on different configurations. The same issue exists in your patch set and the
> > testing bears this out with Denis still getting spurious wakes when using it.
> > The problem with your approach is that you aren't listening to us despite our
> > much broader understanding of the issue at hand. If this worked we would have
> > submitted it ourselves nearly three weeks ago.
> 
> Here you assume I did not do the same. Yes, I did. The asus-wmi quirk
> is subject to racing conditions that mean it will never work correctly
> (well; without newer firmware maybe perhaps). This series is not, as
> the calls happen before suspend starts.
> 
> Yes, my V1 of the patch did not include a delay at all. Since the
> original Ally is XInput, one of the MCUs was left on, which caused
> what Denis experienced. It also triggered a restart of the MCU on the
> Ally X after resume. V2 fixes this restart on the Ally X and makes it
> behave completely properly.

With the fixed firmware no delay is required anywhere, no restructuring of suspend is required, no more trying to figure out an optimal ordering is required.

> In fact, after being included in bazzite-testing yesterday, my Ally X
> unit went on a deep slumber tonight with powersave on, and then woke
> up today beautifully, having only lost 1% of battery.

The Ally X is much less sensitive to things than the Ally 1 is and invariably when we thought we'd found a fix, someone with a different kernel config, distro, and compiler (maybe even with LTO) would change the timing of things just enough. We tried pretty much everything you seem to be trying.

> It is not clear if the issue still exists on the original Ally, I
> could not get a clear signal from Denis but do not worry, one of our
> contributors is on it.

Denis was very clear I thought, perhaps you misread? Thank you for your thorough testing Denis.

> > *Our solution doesn't require any kernel changes for newer firmware*, as I
> > already stated the solution from ASUS fixes the root cause of the problem. Your
> > attempt at a solution attempts to outrace the symptoms of it. Please don't
> > mischaracterize my statements.
> 
> I would hope that is not true and your solution completely removes the
> quirk from asus-wmi for newer firmware, as it prolongs the suspend and
> resume sequences noticeably. However, given my and Denis' limited

Yes that excessive delay is awful. It was a misguided attempt to race or delay things much like you are doing right now.

With the release of the new fixed firmware for the MCU imminent, this will be removed and all users should be strongly encouraged to update their firmware to the fixed version from ASUS.

> testing, I doubt the controller is able to suspend quickly enough even
> with new firmware (you can see the RGB cut before it's able to fade).

It isn't about trying to suspend the thing "quickly enough". And I'm hoping that with your analysing of that statement it provides you some insight as to why it's a bad assumption to make and why things are always going to be at risk of breaking with async linux suspend when you get another device like the Ally. At best the issue may be masked only to have seemingly random fails that people can't reproduce easily.

> In any case, this patch series is not expected to conflict with Asus'
> newer firmware and is in fact complementary with it. Both fix the same
> issue in a different way, which means our users will have a great
> experience, squared. This series also greatly improves resume behavior
> on the Ally, which, let me tell you, is blazing fast now.

You are correct in that it won't conflict. That is because the firmware that ASUS engineers who worked with us to fix, fixes the root cause of the issue regardless of the USB state at suspend time - which fully removes the need to make changes to core suspend code, especially in regards to the Ally devices.

It is perhaps best if I refer to what Denis and Derek told you about modern standby: focus on background downloads and proceed in that direction - the firmware really does fix the suspend/resume/reboot issues. You can also drop the asus-wmi patch (in submissions) as that will get dealt with soon enough.

Sincerely,
Luke.