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] [thread-next>] [day] [month] [year] [list]

Message-ID: <027706ba7b19a4eefb51aeddb5d10cc235751780.camel@ljones.dev>
Date: Wed, 26 Feb 2025 16:08:15 +1300
From: Luke Jones <luke@...nes.dev>
To: Antheas Kapenekakis <lkml@...heas.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

On Tue, 2025-02-25 at 15:15 +0100, Antheas Kapenekakis wrote:
> Hi Luke,
> setting MCU powersave too close to the boot-up sequence can cause the
> controller of the original Ally to malfunction.

Read the code. It doesn't, as it is set by hid driver. Zero issues in
all our testing.

> Which is why you created
> this little init sequence in which you call CSEE manually. In
> addition,

No it wasn't.

> MCU powersave (which is called Extreme Standby in Windows and you
> named
> incorrectly in asus-wmi), causes a very large 8 second delay in the

That is a UI label. ASUS call it Mcu Powersave in internal emails to
me. It is also called *MCUPowerSaving in their source code.

> resume
> process. It should under no circumstance be set enabled by default.
> 
> Users that want it can enable it manually. Following, distributions
> that
> want it and lack the appropriate configuration interface can use a
> udev
> rule with an 8 second delay to enable it, and then, the udev rule
> should
> first check if mcu_powersave is disabled before enabling it. This is
> because writing to it even with the same value causes an issue
> regardless.
> 

No it does not.

> Therefore, please remove both parts of it from the second patch in
> your
> series and produce a v2, which contains no hints of CSEE. When you
> do:
> 
> Suggested-by: Antheas Kapenekakis <lkml@...heas.dev>
> 

No. You've suggested changes with zero testing, simply in an attempt to
get a tag.

> Following, when you do finish the new Asus Armoury patch series,
> please
> rename MCU powersave to extreme standby, and add a suggested-by in
> the
> appropriate patch. Since to avoid user confusion, the names should
> match
> their windows branding.
> 

No. See first response.

> During our testing of the controller, we found that the lack of a
> delay
> causes the RGB of both the Ally and the Ally X to malfunction, so

This is to do with your own code in userspace. 

> this is
> a small nack for me (the old quirk is preferable in that regard). But
> then
> again, this patch series is not getting anywhere close to our users
> even
> if it is accepted, so you can do as you wish (given appropriate
> attribution).
> 

Your message attempts to frame this as a personal matter ("small nack
for me," "you can do as you wish") rather than providing substantive
technical feedback appropriate for LKML.

To be clear: This patch is submitted for mainline Linux kernel
consideration, not your downstream project. Your NACK lacks technical
merit relevant to mainline development, as you yourself acknowledge
these changes would not affect your users.

As I am the maintainer on this driver I will proceed with the
submission process as there are no valid technical objections to
address. Further non-technical commentary on this thread is
unnecessary.

Cheers,
Luke.