Message-ID: <633bbd2d5469db5595f66c9eb6ea3172ab7c56b7.camel@ljones.dev>
Date: Tue, 25 Feb 2025 10:51:09 +1300
From: Luke Jones <luke@...nes.dev>
To: Mark Pearson <mpearson-lenovo@...ebb.ca>, Antheas Kapenekakis
	 <lkml@...heas.dev>, "Limonciello, Mario" <mario.limonciello@....com>
Cc: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>, Len
 Brown <lenb@...nel.org>, "linux-acpi@...r.kernel.org"
 <linux-acpi@...r.kernel.org>, 	linux-kernel@...r.kernel.org,
 "platform-driver-x86@...r.kernel.org"	
 <platform-driver-x86@...r.kernel.org>, "Rafael J. Wysocki"
 <rafael@...nel.org>,  Hans de Goede <hdegoede@...hat.com>,
 me@...egospodneti.ch
Subject: Re: [PATCH 0/3] ACPI: platform_profile: fix legacy sysfs with
 multiple handlers

On Mon, 2025-02-24 at 15:52 -0500, Mark Pearson wrote:
> Hi Antheas,
> 
> On Mon, Feb 24, 2025, at 2:50 PM, Antheas Kapenekakis wrote:
> > On the Asus Z13 (2025), a device that would need the amd-pmf quirk
> > that
> > was removed on the platform_profile refactor, we see the following
> > output
> > from the sysfs platform profile:
> > 
> > $ cat /sys/firmware/acpi/platform_profile_choices
> > balanced performance
> > 
> > I.e., the quiet profile is missing. Which is a major regression in
> > terms of
> > power efficiency and affects both tuned, and ppd (it also affected
> > my
> > software but I fixed that on Saturday). This would affect any
> > laptop that
> > loads both amd-pmf and asus-wmi (around 15 models give or take?).
> > 
> > The problem stems from the fact that asus-wmi uses quiet, and amd-
> > pmf uses
> > low-power. While it is not clear to me what the amd-pmf module is
> > supposed
> > to do here, and perhaps some autodetection should be done and make
> > it bail,
> > if we assume it should be kept, then there is a small refactor that
> > is
> > needed to maintain the existing ABI interface.
> > 
> > This is the subject of this patch series.
> > 
> > Essentially, we introduce the concept of a "secondary" handler.
> > Secondary
> > handlers work exactly the same, except for the fact they are able
> > to
> > receive all profile names through the sysfs interface. The
> > expectation
> > here would be that the handlers choose the closest appropriate
> > profile
> > they have, and this is what I did for the amd-pmf handler.
> > 
> > In their own platform_profile namespace, these handlers still work
> > normally
> > and only accept the profiles from their probe functions, with -
> > ENOSUP for
> > the rest.
> > 
> > In the absence of a primary handler, the options of all secondary
> > handlers
> > are unioned in the legacy sysfs, which prevents them from hiding
> > each
> > other's options.
> > 
> > With this patch series applied, the sysfs interface will look like
> > this:
> > 
> > $ cat /sys/firmware/acpi/platform_profile_choices
> > quiet balanced performance
> > 
> > And writing quiet to it results in the profile being applied to
> > both
> > platform profile handlers.
> > 
> > $ echo low-power > /sys/firmware/acpi/platform_profile
> > bash: echo: write error: Operation not supported
> > $ echo quiet > /sys/firmware/acpi/platform_profile
> > $ cat /sys/class/platform-profile/platform-profile-*/{name,profile}
> > asus-wmi
> > amd-pmf
> > quiet
> > quiet
> > 
> > Agreed ABI still works:
> > $ echo quiet > /sys/class/platform-profile/platform-profile-
> > 0/profile
> > $ echo quiet > /sys/class/platform-profile/platform-profile-
> > 1/profile
> > bash: echo: write error: Operation not supported
> > $ echo low-power > /sys/class/platform-profile/platform-profile-
> > 0/profile
> > bash: echo: write error: Operation not supported
> > $ echo low-power > /sys/class/platform-profile/platform-profile-
> > 1/profile
> > 
> 
> I understand where you're coming from with this implementation but my
> concern is this is making profiles more complicated - and they're
> already becoming hard to understand (and debug) for users.
> 
> I'm not a huge fan of multiple profile handlers, but can see why some
> people might want them and that they're a valid tool to have
> (especially given some of the limitations of what platform vendors
> themselves implement).
> 
> In patch #3 it states that 'It is the expectation that secondary
> handlers will pick the closest profile they have to what was sent'.
> I'm not convinced that is true, or desired.
> 
> e.g. Quiet and low-power are different things and can have different
> implementations. One is giving you as much power as possible with the
> fans running below a certain audible level; and one is giving you a
> system with as low-power consumption as possible, but still be
> usable. They're admittedly not very different in practice - but they
> can be different.
> 
> Would it be better here to ask AMD to implement a quiet profile
> (maybe it can be based on low-power, at least initially)?
> I think that would solve the ASUS issue and not introduce another
> layer of complexity.
> 
> Mark

Hi Mark,

I've supported over 80 different ASUS laptops in the last 6 years or
so, I can offer some insight.

Across the entire range (TUF, ROG, Vivobook, Zen) which implements some
form of "thermal throttle" as it is called in asus-wmi (which is what
is used by platform_profile) the difference between low-power and quiet
is very much nil - the "quiet" profile is only a name, and the TDP is
limited along with fans to match - so the result is "low-power".

As Mario suggests in his reply perhaps an alias would be best, or, as I
was going to do, simply rename the "quiet" profile in asus-wmi to "low-
power" as I already did but have not submitted yet due to a large train
of patches in progress. It's a single line change and nullifies the
entire issue and this series.

In any case asus handling of platform profile is something I have been
steadily working on for the last few months for both laptops and
handhelds and I will have a new patch series coming soon (version 7 of
previously submitted dealing with this).

This submitted series is a NACK from me.

Cheers,
Luke.

Powered by blists - more mailing lists