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@xxxxxxxxxxx/
> > >
> > > that seems to fix the same issue ?
> >
> > The history of that is here
> > https://lore.kernel.org/linux-pm/20240919171952.403745-1-lkml@xxxxxxxxxxx/#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(-)