linux-kernel - Re: [PATCH 2/2] platform/x86: asus-wmi: Refactor Ally suspend/resume

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: 
 <CAGwozwGwjM7hGOm+6iDHg7Q1StbEYRm=cU-8Cx4v==V1d9W4=w@mail.gmail.com>
Date: Wed, 26 Feb 2025 10:17:06 +0100
From: Antheas Kapenekakis <lkml@...heas.dev>
To: Luke Jones <luke@...nes.dev>
Cc: bentiss@...nel.org, hdegoede@...hat.com, ilpo.jarvinen@...ux.intel.com,
	jikos@...nel.org, linux-input@...r.kernel.org, linux-kernel@...r.kernel.org,
	platform-driver-x86@...r.kernel.org
Subject: Re: [PATCH 2/2] platform/x86: asus-wmi: Refactor Ally suspend/resume

Hi Luke,
using your maintainer status and your NDA creds to veto my comments
will not go well for you if you end up breaking the driver.

Yes, I have done extensive testing with setting the powersave
parameter. I have supported it since September. It is very easy to
break the controller if you start writing to it randomly. In fact,
when I pushed it out [1], it never got out of testing because I got
three user reports about it so I had to make a revised version [2].
Notice there is a day difference between the versions. We never needed
a boot workaround for the original ally, and if you ask me, the reason
your driver here includes that is because in all your testing you set
the powersave parameter.

When it comes to RGB, without a quirk it looks to me as if the MCU
hardlocks and stops accepting RGB commands. I am pretty sure I wrote
to it manually, but excuse me if I am wrong as there have been 5
months since I did the testing for it.

In your new armoury driver, you tried to make internal names
accessible to users via the BIOS interface to make it more user
friendly. In this case, you need to use the names Asus uses in
Windows, otherwise users will get confused. Failing that, asus-wmi is
a perfectly fine driver as it is, with mcu_powersave as an internal
parameter.

As far as I am concerned, the whole Ally issue boils down to the
Modern Standby firmware notifications in the Linux kernel being in the
wrong place. You can see it in the Microsoft documentation [3]. The
Display Off call should happen when the display turns off, not after
the kernel has suspended. This change will need to eventually happen,
as Asus is not the only handheld manufacturer doing this. All
manufacturers turn off their xpad controller via those notifications,
including Lenovo, GPD, OneXPlayer, and MSI. Fortunately, this issue
seems to only affect asus in a major way, although I suspect it causes
a quality degradation in other manufacturers as well.

Given that I do have a custom kernel now and i can rewrite it as I see
fit, I am giving you carte blanche when it comes to the mainline
kernel. As far as I am concerned, my Ally 1st gen/X integration
finished last November. So I will not be revisiting it or doing any
additional testing to validate any new claims or changes. I will just
be removing them and pointing users to my kernel.

Best,
Antheas

[1] https://github.com/hhd-dev/adjustor/releases/tag/v3.5.0
[2] https://github.com/hhd-dev/adjustor/releases/tag/v3.5.1
[3] https://learn.microsoft.com/en-us/windows-hardware/design/device-experiences/modern-standby-firmware-notifications