Message-ID: <29e6cea2-b315-49f1-8956-c70f123289de@kernel.org>
Date: Mon, 7 Oct 2024 20:53:24 -0500
From: Mario Limonciello <superm1@...nel.org>
To: Luke Jones <luke@...nes.dev>, Hans de Goede <hdegoede@...hat.com>,
 linux-kernel@...r.kernel.org
Cc: platform-driver-x86@...r.kernel.org,
 Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
 corentin.chary@...il.com
Subject: Re: [PATCH 0/3] platfom/x86: asus-wmi: revert ROG Ally quirks

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

Powered by blists - more mailing lists