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

Antheas Kapenekakis
2024-09-23T13:12:09Z

Hi Luke,

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

Would be nice to back it up. Indeed, right now on the X i am facing an
issue that Windows also has (see below), so it is not like Asus do not
have their work cut out for them. I would have to test the new
firmware and get back to you.

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

The proper fix for _Asus_. I need to support a lot of manufacturers
and a lot of them use these _DSMs, so we need to get a better ordering
here.

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

Uh, Denis is never very clear unfortunately. From what I understood he
hit the reboot bug once. But I could not get a feel for what his issue
was. I know on V1, the XInput controller did not suspend properly.

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

If you look into my patch series, you will see that unlike the
variants you tried, there is no async within it. The calls and the
delay are inserted before beginning the suspend sequence (which is
async). I think this is a good compromise and can be blended with
userspace going down, so it will not increase suspend delay in the
long run for devices that need a quirk. For the Ally X, even without a
delay, the controller always comes back online. Seems that it is the
case on the original Ally as well. With a caveat.

That caveat has to do with the delay before suspend. If it is too
little, the OG Ally XInput MCU does not turn off and the Ally X MCU
loses its state. For the OG Ally, I guess Denis informed you about it.
For the Ally X, this manifests itself as the MCU rebooting once when
it receives the first command after suspend. When powersave is off,
this happens when there is no delay, when it is on, it always happens.

I can reproduce this bug on windows very easily right now too.

It is a small nit that can go unnoticed though. So hopefully you can
test the new firmware (with whatever kernel you want) and report back.

Hopefully you are right and it was a timing check and that timing
check also fixes this.

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

Indeed. I am the one with the problem here. I have too many Ally users
and for me to require them to update their firmware right now (for
basic functionality) is a non-starter. Which is why I was happy kind
of keeping this patch to myself.

I personally do not have a problem with making the next variant of
this patch an RFC. But currently I think it has a good scope, so I
would like to ask what other kernel maintainers think.
Also, after we do a bit more testing on the OG Ally, if everything
goes well, my version of the quirk is expected to have better behavior
than the asus-wmi, so I do not see a reason in refactoring that,
unless that refactoring ends in it completely removed. There is still
the issue of old users in this case though.

Antheas