On 21/09/24 08:22, Antheas Kapenekakis wrote: > Hi Denis, > thank you for taking the time to test the patch. > Hello Antheas, > First I want to give some context. I have tested variants of this > patch on 6.11rc5 and linux-pm/bleeding-edge on both an Ally X and a > Legion go. I experienced 0 sleep failures on both of them throughout > my whole series of testing, on all variants of the patch. I did not > test 6.11 final. So I hope you did A/B test, as it is not clear from > your email. We have had a lot of issues with new kernels and suspend, > so please make sure to test 6.11 stock. > My tests were conducted on top of the released 6.11.0, as suggested to me by Mario. > The ACPI error in the s2idle report for the _DSM is normal, Asus left > a lid sensor call that goes nowhere below the CSEE call that turns off > the MCU. > Yes, there is a leftover call on lid in the ACPI, however it has been there since the very first release and it has never caused any issue. > I am not that well versed in s2idle traces, but from what I am seeing > is that your controller suspends beautifully, but some suspends fail > due to a PCI error? Then you have "[Errno 16] Device or resource busy" > but no accounting of which device that is. All this patch does is move > CSSE to happen outside the suspend sequence, whereas before it was > done *twice* within the suspend sequence, > Two notes here: - there is no evidence, nor words from ASUS, that indicates calling CSEE twice could be a problem, on the contrary, there is strong evidence that doing it only once causes issues such as what I am having and/or controller returning "every other sleep". - This has never been a problem before and my device has been entering and exiting s2idle just fine for the previous months. > @Mario what are your thoughts? > > The controller is expected to disconnect and reconnect to the device, > we are not trying to avoid that. > The controller is indeed expected to disconnect upon entering s2idle and the fact that I can wake up the device using A,B,X,Y buttons is concerning in this regard: such behavior has never come up in past months. > We plan to do a lot more thorough testing next week on Bazzite (on all > devices), however our kernel maintainer is feeling under the weather > so it will take a couple more days for that to start. > Please, do so before submitting any patch that is meant to change the s2idle path for every existing device. > Also, a small comment on this new firmware. This patch has merit > regardless of what Asus does with the ROG Ally, it is both a feature > and bug fix contribution. I would like to see suspend in Linux become > a lot more modern, as it is a feature we are often asked about. Users > want to be able to download games suspended for example. The Switch > can do it, the Playstation 5 can do it, but uh Linux cannot? It will > not cause regressions regardless of what Asus does, as this is making > Linux mimic Windows more. > This patch might have merit if you properly identify and address regressions you introduced. Windows does not show any sign of spurious wakeup nor aborts while entering modern standby, therefore there is still some difference that must be identified and addressed accordingly if this is the path to be followed going forward. > Then, to move on to your other concern. Hopefully, you conducted these > tests with a stock firmware. I think you did. Secondly, to me, > requiring a firmware update + kernel quirk + software quirk (as > extreme mode will be MCU version dependent) is something that I do not > find to be a very satisfactory solution. In any case, we are happy to > hold this patch out-of-tree for our users with older firmware and then > tack on whatever else is required for the newer firmware. > I conducted these tests with the lastest available firmware that is available to every user: exactly the firmware this patchset is meant to target. - Main Firmware: GA80100.RC71LM.318 - Second Firmware: GA80100.RC71LS.318 On that note I want to add that the path taken for entering s2idle by RC71L and RC72L is different as in the case of RC71L two MCUs are involved, while with what is available to you (RC72L) only one MCU is present and therefore you cannot assume that what works for your device also works on the other. Keeping the firmware updated is something users do regularly on this device, even if it means booting to windows. Beside, as for necessary kernel/software quirks, the new firmware is expected to require none, at least for asus-wmi, so I ask you to leave csee calls where they are now as in the future those will be used only on non-updated firmware. I also did the main testing with the previous 339 BIOS update, but just finished confirming all regressions are still happening with the latest available 341 BIOS update too. > I will post an updated version of the patch later today, although it > is functionally identical. > If it's functionally identical there is no point in it: it cannot be merged with these kind of regressions happening. > Antheas Best regards, Denis Benato
