[PATCH 0/3] ACPI: platform_profile: fix legacy sysfs with multiple handlers

[PATCH 0/3] ACPI: platform_profile: fix legacy sysfs with multiple handlers

[Antheas Kapenekakis]

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

Antheas Kapenekakis (3):
  ACPI: platform_profile: Add support for secondary handlers
  ACPI: platform_profile: add all options to amd-pmf as a secondary
    handler
  ACPI: platform_profile: Do not hide options missing in secondary
    handlers

 drivers/acpi/platform_profile.c    | 57 +++++++++++++++++++++++++-----
 drivers/platform/x86/amd/pmf/spc.c |  3 ++
 drivers/platform/x86/amd/pmf/sps.c |  8 +++++
 include/linux/platform_profile.h   |  7 ++++
 4 files changed, 67 insertions(+), 8 deletions(-)

-- 
2.48.1

[PATCH 1/3] ACPI: platform_profile: Add support for secondary handlers

[Antheas Kapenekakis]

Currently, only the subset of supported profiles are exposed when
using platform profile. This is a big problem when e.g., asus-wmi
and amd-pmf are loaded together, as they have conflicting low
power options. This causes ppd and tuned to miss the low power
option increasing power consumption.

Therefore, add the secondary variable. Profile handlers with it should
accept all profile options always. Only when they are the sole
handler, their probe function will be used to expose available profiles.

Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
---
 drivers/platform/x86/amd/pmf/sps.c | 1 +
 include/linux/platform_profile.h   | 7 +++++++
 2 files changed, 8 insertions(+)

diff --git a/drivers/platform/x86/amd/pmf/sps.c b/drivers/platform/x86/amd/pmf/sps.c
index e6cf0b22dac3..424b146d3574 100644
--- a/drivers/platform/x86/amd/pmf/sps.c
+++ b/drivers/platform/x86/amd/pmf/sps.c
@@ -397,6 +397,7 @@ static int amd_pmf_profile_probe(void *drvdata, unsigned long *choices)
 }
 
 static const struct platform_profile_ops amd_pmf_profile_ops = {
+	.secondary = true,
 	.probe = amd_pmf_profile_probe,
 	.profile_get = amd_pmf_profile_get,
 	.profile_set = amd_pmf_profile_set,
diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h
index 8ab5b0e8eb2c..fef5f2404e9f 100644
--- a/include/linux/platform_profile.h
+++ b/include/linux/platform_profile.h
@@ -30,6 +30,12 @@ enum platform_profile_option {
 
 /**
  * struct platform_profile_ops - platform profile operations
+ * @secondary: Set the platform handler as a secondary. Secondary handlers
+ *		 should be able to accept all profile options. This way, they do
+ *		 not obscure the primary handler's profile options (such as amdpmf
+ *		 having cool and asus-wmi having quiet). When a secondary handler
+ *		 is the only handler available, its probe function is still used
+ *		 to avoid showing all profiles.
  * @probe: Callback to setup choices available to the new class device. These
  *	   choices will only be enforced when setting a new profile, not when
  *	   getting the current one.
@@ -39,6 +45,7 @@ enum platform_profile_option {
  *		 profile in sysfs.
  */
 struct platform_profile_ops {
+	bool secondary;
 	int (*probe)(void *drvdata, unsigned long *choices);
 	int (*profile_get)(struct device *dev, enum platform_profile_option *profile);
 	int (*profile_set)(struct device *dev, enum platform_profile_option profile);
-- 
2.48.1

[PATCH 2/3] ACPI: platform_profile: add all options to amd-pmf as a secondary handler

[Antheas Kapenekakis]

As a secondary handler, the amd-pmf module should be able to accept
all platform profiles. In the case of a custom profile, it should NOOP
without an error to allow primary handlers to receive a custom profile.
The sysfs endpoint will still report custom, after all.

Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
---
 drivers/platform/x86/amd/pmf/spc.c | 3 +++
 drivers/platform/x86/amd/pmf/sps.c | 7 +++++++
 2 files changed, 10 insertions(+)

diff --git a/drivers/platform/x86/amd/pmf/spc.c b/drivers/platform/x86/amd/pmf/spc.c
index f34f3130c330..99c48378f943 100644
--- a/drivers/platform/x86/amd/pmf/spc.c
+++ b/drivers/platform/x86/amd/pmf/spc.c
@@ -219,12 +219,15 @@ static int amd_pmf_get_slider_info(struct amd_pmf_dev *dev, struct ta_pmf_enact_
 
 	switch (dev->current_profile) {
 	case PLATFORM_PROFILE_PERFORMANCE:
+	case PLATFORM_PROFILE_BALANCED_PERFORMANCE:
 		val = TA_BEST_PERFORMANCE;
 		break;
 	case PLATFORM_PROFILE_BALANCED:
 		val = TA_BETTER_PERFORMANCE;
 		break;
 	case PLATFORM_PROFILE_LOW_POWER:
+	case PLATFORM_PROFILE_COOL:
+	case PLATFORM_PROFILE_QUIET:
 		val = TA_BEST_BATTERY;
 		break;
 	default:
diff --git a/drivers/platform/x86/amd/pmf/sps.c b/drivers/platform/x86/amd/pmf/sps.c
index 424b146d3574..f284ebc495ee 100644
--- a/drivers/platform/x86/amd/pmf/sps.c
+++ b/drivers/platform/x86/amd/pmf/sps.c
@@ -297,12 +297,15 @@ int amd_pmf_get_pprof_modes(struct amd_pmf_dev *pmf)
 
 	switch (pmf->current_profile) {
 	case PLATFORM_PROFILE_PERFORMANCE:
+	case PLATFORM_PROFILE_BALANCED_PERFORMANCE:
 		mode = POWER_MODE_PERFORMANCE;
 		break;
 	case PLATFORM_PROFILE_BALANCED:
 		mode = POWER_MODE_BALANCED_POWER;
 		break;
 	case PLATFORM_PROFILE_LOW_POWER:
+	case PLATFORM_PROFILE_COOL:
+	case PLATFORM_PROFILE_QUIET:
 		mode = POWER_MODE_POWER_SAVER;
 		break;
 	default:
@@ -369,6 +372,10 @@ static int amd_pmf_profile_set(struct device *dev,
 	struct amd_pmf_dev *pmf = dev_get_drvdata(dev);
 	int ret = 0;
 
+	/* If the profile is custom, bail without an error. */
+	if (profile == PLATFORM_PROFILE_CUSTOM)
+		return 0;
+
 	pmf->current_profile = profile;
 
 	/* Notify EC about the slider position change */
-- 
2.48.1

[PATCH 3/3] ACPI: platform_profile: Do not hide options missing in secondary handlers

[Antheas Kapenekakis]

In the legacy endpoint, previously, only the options common to all
handlers were exposed. This causes an issue when the primary handler
of the device has its options hidden, as it results in a performance
degradation.

Therefore, this commit introduces the concept of secondary handlers.
These are handlers that are able to accept all options and do not
partake in the process of selecting which profiles are visible.

These handlers still have a probe function which is used for their
endpoint and their endpoint works normally and will block other
options. However, when called from the legacy endpoint, all options
will be sent to them. It is the expectation that secondary handlers
will pick the closest profile they have to what was sent.

In the absence of a primary handler, the options shown in the legacy
endpoint will be the union of all options of all secondary handlers.

Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
---
 drivers/acpi/platform_profile.c | 57 ++++++++++++++++++++++++++++-----
 1 file changed, 49 insertions(+), 8 deletions(-)

diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
index 2ad53cc6aae5..55e8bb6adf8e 100644
--- a/drivers/acpi/platform_profile.c
+++ b/drivers/acpi/platform_profile.c
@@ -63,17 +63,18 @@ static ssize_t _commmon_choices_show(unsigned long *choices, char *buf)
  * _store_class_profile - Set the profile for a class device
  * @dev: The class device
  * @data: The profile to set
+ * @enforce_valid: For secondary handlers, enforce that the profile is valid
  *
  * Return: 0 on success, -errno on failure
  */
-static int _store_class_profile(struct device *dev, void *data)
+static int _store_class_profile(struct device *dev, void *data, bool enforce_valid)
 {
 	struct platform_profile_handler *handler;
 	int *bit = (int *)data;
 
 	lockdep_assert_held(&profile_lock);
 	handler = to_pprof_handler(dev);
-	if (!test_bit(*bit, handler->choices))
+	if ((enforce_valid || !handler->ops->secondary) && !test_bit(*bit, handler->choices))
 		return -EOPNOTSUPP;
 
 	return handler->ops->profile_set(dev, *bit);
@@ -204,7 +205,7 @@ static ssize_t profile_store(struct device *dev,
 		return -EINVAL;
 
 	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
-		ret = _store_class_profile(dev, &index);
+		ret = _store_class_profile(dev, &index, true);
 		if (ret)
 			return ret;
 	}
@@ -243,21 +244,37 @@ static const struct class platform_profile_class = {
  *
  * Return: 0 on success, -errno on failure
  */
-static int _aggregate_choices(struct device *dev, void *data)
+static int _aggregate_choices(struct device *dev, void *data, bool secondary)
 {
 	struct platform_profile_handler *handler;
 	unsigned long *aggregate = data;
 
 	lockdep_assert_held(&profile_lock);
 	handler = to_pprof_handler(dev);
+
+	if (handler->ops->secondary != secondary)
+		return 0;
+
 	if (test_bit(PLATFORM_PROFILE_LAST, aggregate))
 		bitmap_copy(aggregate, handler->choices, PLATFORM_PROFILE_LAST);
+	else if (handler->ops->secondary)
+		bitmap_or(aggregate, handler->choices, aggregate, PLATFORM_PROFILE_LAST);
 	else
 		bitmap_and(aggregate, handler->choices, aggregate, PLATFORM_PROFILE_LAST);
 
 	return 0;
 }
 
+static int _aggregate_choices_primary(struct device *dev, void *data)
+{
+	return _aggregate_choices(dev, data, false);
+}
+
+static int _aggregate_choices_secondary(struct device *dev, void *data)
+{
+	return _aggregate_choices(dev, data, true);
+}
+
 /**
  * platform_profile_choices_show - Show the available profile choices for legacy sysfs interface
  * @dev: The device
@@ -276,9 +293,16 @@ static ssize_t platform_profile_choices_show(struct device *dev,
 	set_bit(PLATFORM_PROFILE_LAST, aggregate);
 	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
 		err = class_for_each_device(&platform_profile_class, NULL,
-					    aggregate, _aggregate_choices);
+					    aggregate, _aggregate_choices_primary);
 		if (err)
 			return err;
+
+		if (test_bit(PLATFORM_PROFILE_LAST, aggregate)) {
+			err = class_for_each_device(&platform_profile_class, NULL,
+							aggregate, _aggregate_choices_secondary);
+			if (err)
+				return err;
+		}
 	}
 
 	/* no profile handler registered any more */
@@ -325,7 +349,7 @@ static int _store_and_notify(struct device *dev, void *data)
 	enum platform_profile_option *profile = data;
 	int err;
 
-	err = _store_class_profile(dev, profile);
+	err = _store_class_profile(dev, profile, false);
 	if (err)
 		return err;
 	return _notify_class_profile(dev, NULL);
@@ -384,9 +408,18 @@ static ssize_t platform_profile_store(struct device *dev,
 	set_bit(PLATFORM_PROFILE_LAST, choices);
 	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
 		ret = class_for_each_device(&platform_profile_class, NULL,
-					    choices, _aggregate_choices);
+					    choices, _aggregate_choices_primary);
 		if (ret)
 			return ret;
+
+		if (test_bit(PLATFORM_PROFILE_LAST, choices)) {
+			ret = class_for_each_device(
+				&platform_profile_class, NULL, choices,
+				_aggregate_choices_secondary);
+			if (ret)
+				return ret;
+		}
+
 		if (!test_bit(i, choices))
 			return -EOPNOTSUPP;
 
@@ -470,10 +503,18 @@ int platform_profile_cycle(void)
 			return -EINVAL;
 
 		err = class_for_each_device(&platform_profile_class, NULL,
-					    choices, _aggregate_choices);
+					    choices, _aggregate_choices_primary);
 		if (err)
 			return err;
 
+		if (test_bit(PLATFORM_PROFILE_LAST, choices)) {
+			err = class_for_each_device(
+				&platform_profile_class, NULL, choices,
+				_aggregate_choices_secondary);
+			if (err)
+				return err;
+		}
+
 		/* never iterate into a custom if all drivers supported it */
 		clear_bit(PLATFORM_PROFILE_CUSTOM, choices);
 
-- 
2.48.1

Re: [PATCH 0/3] ACPI: platform_profile: fix legacy sysfs with multiple handlers

[Mario Limonciello]

On 2/24/2025 13:50, 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?).

To me this raises a fundamental question.  What is really different 
between quiet and low-power?  quiet just affects fan curves and 
low-power affects power?

One could argue that changing power will indirectly affect fan performance.

Because it makes me wonder if we really should just make them permanent 
aliases.

> 
> 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
> 
> Antheas Kapenekakis (3):
>    ACPI: platform_profile: Add support for secondary handlers
>    ACPI: platform_profile: add all options to amd-pmf as a secondary
>      handler
>    ACPI: platform_profile: Do not hide options missing in secondary
>      handlers
> 
>   drivers/acpi/platform_profile.c    | 57 +++++++++++++++++++++++++-----
>   drivers/platform/x86/amd/pmf/spc.c |  3 ++
>   drivers/platform/x86/amd/pmf/sps.c |  8 +++++
>   include/linux/platform_profile.h   |  7 ++++
>   4 files changed, 67 insertions(+), 8 deletions(-)
> 

Re: [PATCH 0/3] ACPI: platform_profile: fix legacy sysfs with multiple handlers

[Antheas Kapenekakis]

That is a very good question, but a summary search along the drivers
reveals that some laptops have both low-power and quiet, so it is not
that clear cut.

If you ask me, three is all you need + custom

Antheas

Re: [PATCH 0/3] ACPI: platform_profile: fix legacy sysfs with multiple handlers

[Mark Pearson]

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

Re: [PATCH 0/3] ACPI: platform_profile: fix legacy sysfs with multiple handlers

[Antheas Kapenekakis]

Hi Mark,
My primary focus with this patch series is a bug fix. I imported
Mario's series into our Bazzite 6.13 kernel, only to find it broke
power handling on asus laptops, and it will also do the same on both
legion gos, once a driver exists for those.

And 6.14rc4 is the same. This needs to be fixed before it ships.

This was my attempt at it. I considered other options, like making
amd-pmf implement all profiles. But this seems like too dirty for me.
So I settled at this.

The primary TDP handler of a device is the WMI handler. When that
exists, if one of its options is hidden, that is a regression. It does
not matter the option. If we want amd-pmf to be able to load as a
secondary handler (where the point of that has not been proven to me),
then it (or any other secondary handler) cannot obscure the options of
the primary platform. So it either has to implement all of them, or do
something like this, where it is in between.

Antheas

Re: [PATCH 0/3] ACPI: platform_profile: fix legacy sysfs with multiple handlers

[Luke Jones]

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.

Re: [PATCH 0/3] ACPI: platform_profile: fix legacy sysfs with multiple handlers

[Antheas Kapenekakis]

I will tell you that compared with other manufacturers, when asus says
quiet, they mean quiet and not low power

Z13's quiet mode is 40W, not very low-power if you ask me. Ally X uses
13w+boost, a source of many complaints. Other manufacturers use around
8W for low power modes.

In any case, any rename might break user scripts and there are
actually 3 types of low power profiles:
PLATFORM_PROFILE_COOL
PLATFORM_PROFILE_QUIET
PLATFORM_PROFILE_LOW_POWER

Then, there is also:
PLATFORM_PROFILE_BALANCED_PERFORMANCE

Some ACER laptops implement many of those

Obscuring any of those is not ideal.


Antheas

Re: [PATCH 0/3] ACPI: platform_profile: fix legacy sysfs with multiple handlers

[Armin Wolf]

Am 24.02.25 um 22:51 schrieb Luke Jones:

> 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.

I agree with you here, changing asus-wmi to using "low-power" would be an
appropriate quick fix here.

But i think the real solution to this is to extend tuned and platform-profiles-daemon
so that they use the new platform-profile class interface when available. This would
prevent such issues in the future.

Maybe someone can open an issue for both projects to notify them that an improved API
for controlling the platform-profiles is available?

Thanks,
Armin Wolf

Re: [PATCH 0/3] ACPI: platform_profile: fix legacy sysfs with multiple handlers

[Armin Wolf]

Am 24.02.25 um 22:58 schrieb Antheas Kapenekakis:

> I will tell you that compared with other manufacturers, when asus says
> quiet, they mean quiet and not low power
>
> Z13's quiet mode is 40W, not very low-power if you ask me. Ally X uses
> 13w+boost, a source of many complaints. Other manufacturers use around
> 8W for low power modes.
>
> In any case, any rename might break user scripts and there are
> actually 3 types of low power profiles:
> PLATFORM_PROFILE_COOL
> PLATFORM_PROFILE_QUIET
> PLATFORM_PROFILE_LOW_POWER
>
> Then, there is also:
> PLATFORM_PROFILE_BALANCED_PERFORMANCE
>
> Some ACER laptops implement many of those
>
> Obscuring any of those is not ideal.
>
>
> Antheas
>
I see. Maybe extending userspace to support the new platform-profile class interface
could solve this issue without breaking already existing scripts.

However i do not know the amount of work which needs to be done to achieve this.

Thanks,
Armin Wolf

Re: [PATCH 0/3] ACPI: platform_profile: fix legacy sysfs with multiple handlers

[Antheas Kapenekakis]

> I see. Maybe extending userspace to support the new platform-profile class interface
> could solve this issue without breaking already existing scripts.
>
> However i do not know the amount of work which needs to be done to achieve this.

The patch series I proposed here restores the behavior of the legacy
API 1-1 with 6.13, ensuring that there are no userspace breakages and
allows for maintaining the legacy interface's usefulness in the
future. Without it, once more power profile handlers are added, it
will begin to regress even more. We are also not sure it is just Asus,
it is very likely there will be other breakages.

Currently, platform profile handlers are split in half between
low-power and quiet, and another third has additional options.

The legacy interface is still good for most cases, there is no need to
start moving non-specialized power managers such as ppd to it just
yet.

Antheas

Re: [PATCH 0/3] ACPI: platform_profile: fix legacy sysfs with multiple handlers

[Antheas Kapenekakis]

Yes, making asus-wmi use low-power is indeed the easiest solution, but
if I thought it was good enough, I would have done that already as a
downstream consumer of the kernel.

I just want to be done with this once and for all, so I spent an extra
hour today solving this in a cleaner way.

Antheas

Re: [PATCH 0/3] ACPI: platform_profile: fix legacy sysfs with multiple handlers

[Luke Jones]

On Mon, 2025-02-24 at 22:58 +0100, Antheas Kapenekakis wrote:
> I will tell you that compared with other manufacturers, when asus
> says
> quiet, they mean quiet and not low power
> 
> Z13's quiet mode is 40W, not very low-power if you ask me. Ally X
> uses
> 13w+boost, a source of many complaints. Other manufacturers use
> around
> 8W for low power modes.
> 
> In any case, any rename might break user scripts and there are
> actually 3 types of low power profiles:
> PLATFORM_PROFILE_COOL
> PLATFORM_PROFILE_QUIET
> PLATFORM_PROFILE_LOW_POWER
> 
> Then, there is also:
> PLATFORM_PROFILE_BALANCED_PERFORMANCE
> 
> Some ACER laptops implement many of those
> 
> Obscuring any of those is not ideal.
> 

In the context of their other modes it is low power. If there is
confusion over what this is meant to mean for all vendors then it
should be clarified in the relevant documentation.

low-power		Low power consumption
cool			Cooler operation
quiet			Quieter operation
balanced		Balance between low power consumption
			and performance
balanced-performance	Balance between performance and low
			power consumption with a slight bias
			towards performance
performance		High performance operation
custom			Driver defined custom profile


"Low power consumption" compared to?

If these "scripts" use `platform_profile_choices` to get their
selections and verify they are available then there should be zero
breakage. If they don't then they should be updated to be correct.

In any case I am in the process of finalising an update to use the new
platform_profile API including "custom". Please don't begin trying to
break things just to be "first". My work has been ongoing for this in
my spare time for months.

Re: [PATCH 0/3] ACPI: platform_profile: fix legacy sysfs with multiple handlers

[Antheas Kapenekakis]

> If these "scripts" use `platform_profile_choices` to get their
> selections and verify they are available then there should be zero
> breakage. If they don't then they should be updated to be correct.

Yeah, if any Asus users wrote scripts for their laptops to e.g., "echo
quiet | sudo tee /sys/firmware/acpi/platform_profile" or used TLP let
them spend a few days finding out why kernel 6.14 does not work. They
should have written a 300 line bash script instead.

> In any case I am in the process of finalising an update to use the new
> platform_profile API including "custom". Please don't begin trying to
> break things just to be "first". My work has been ongoing for this

 drivers/acpi/platform_profile.c    | 57 +++++++++++++++++++++++++-----
 drivers/platform/x86/amd/pmf/spc.c |  3 ++
 drivers/platform/x86/amd/pmf/sps.c |  8 +++++
 include/linux/platform_profile.h   |  7 ++++

I do not see the name Asus here. This is a compatibility patch. You
should try it before commenting on it further. Looking at my ACPI
database, there are at least a few Ayaneos, GPDs, and Legion laptops
that have the ACPI bindings for pmf, this is not an Asus issue.

By the way, I have merged your patch series on Bazzite (well... a
cleaned up version that does not happen to crash your own software...)
and it happens to work fine with this patch (I know you said platform
profiles are not in yet). I still use the asus-wmi APIs personally.

sudo fwupdmgr get-bios-setting
Authenticating…          [ -                                     ]
ppt_pl3_fppt:
  Setting type:         Integer
  Current Value:        80
  Description:          Set the CPU slow package limit
  Read Only:            False
  Minimum value:        5
...

> in my spare time for months.

Let me comment on this a bit further. Hobbies are good to have and it
is nice you found one you like. However, a lot of people are spending
a lot of money on their Asus laptops and are actually starting to
depend on Linux. If they cannot depend on you or your hobby for
support, you should at least make sure to not interfere with parallel
efforts for that support, if not try to be synergistic.

I did not make this patch to one up you or rush it. This issue is a
blocker for deploying our 6.13 kernel. Since this kernel needs to work
for the Z13 and pmf quirks are dead ends now (I also got annoyed by
asus users complaining about their fan curves being wrong because pmf
blew up), I pulled in Mario's platform profile series early, only to
find this issue. Botching the asus-wmi platform handler did not meet
my standards, so I had to make this series. Also, since I could not
pull in Kurk's series, and his changes were extensive, I had to make
this series twice, and test it twice.

Good news is this series works and the kernel is on its way to be
deployed in a few days. Flatpak fix came in clutch today too with
6.13.4.

Antheas

Re: [PATCH 0/3] ACPI: platform_profile: fix legacy sysfs with multiple handlers

[Armin Wolf]

Am 25.02.25 um 03:26 schrieb Antheas Kapenekakis:

>> If these "scripts" use `platform_profile_choices` to get their
>> selections and verify they are available then there should be zero
>> breakage. If they don't then they should be updated to be correct.
> Yeah, if any Asus users wrote scripts for their laptops to e.g., "echo
> quiet | sudo tee /sys/firmware/acpi/platform_profile" or used TLP let
> them spend a few days finding out why kernel 6.14 does not work. They
> should have written a 300 line bash script instead.

Hi,

using "echo quiet | sudo tee /sys/firmware/acpi/platform_profile" is quite
brittle, as some hardware will populate the available profiles dynamically.

Still breaking userspace is indeed not an option here, so we have to think
of something else.

>> In any case I am in the process of finalising an update to use the new
>> platform_profile API including "custom". Please don't begin trying to
>> break things just to be "first". My work has been ongoing for this
>   drivers/acpi/platform_profile.c    | 57 +++++++++++++++++++++++++-----
>   drivers/platform/x86/amd/pmf/spc.c |  3 ++
>   drivers/platform/x86/amd/pmf/sps.c |  8 +++++
>   include/linux/platform_profile.h   |  7 ++++
>
> I do not see the name Asus here. This is a compatibility patch. You
> should try it before commenting on it further. Looking at my ACPI
> database, there are at least a few Ayaneos, GPDs, and Legion laptops
> that have the ACPI bindings for pmf, this is not an Asus issue.
>
> By the way, I have merged your patch series on Bazzite (well... a
> cleaned up version that does not happen to crash your own software...)
> and it happens to work fine with this patch (I know you said platform
> profiles are not in yet). I still use the asus-wmi APIs personally.
>
> sudo fwupdmgr get-bios-setting
> Authenticating…          [ -                                     ]
> ppt_pl3_fppt:
>    Setting type:         Integer
>    Current Value:        80
>    Description:          Set the CPU slow package limit
>    Read Only:            False
>    Minimum value:        5
> ...
>
>> in my spare time for months.
> Let me comment on this a bit further. Hobbies are good to have and it
> is nice you found one you like. However, a lot of people are spending
> a lot of money on their Asus laptops and are actually starting to
> depend on Linux. If they cannot depend on you or your hobby for
> support, you should at least make sure to not interfere with parallel
> efforts for that support, if not try to be synergistic.

This whole driver was likely written by someone as a hobby, so you already
depend on a hobby here.

That being said, i agree that fixes have a priority over new features, and
i think everyone agrees on that.

> I did not make this patch to one up you or rush it. This issue is a
> blocker for deploying our 6.13 kernel. Since this kernel needs to work
> for the Z13 and pmf quirks are dead ends now (I also got annoyed by
> asus users complaining about their fan curves being wrong because pmf
> blew up), I pulled in Mario's platform profile series early, only to
> find this issue. Botching the asus-wmi platform handler did not meet
> my standards, so I had to make this series. Also, since I could not
> pull in Kurk's series, and his changes were extensive, I had to make
> this series twice, and test it twice.
>
> Good news is this series works and the kernel is on its way to be
> deployed in a few days. Flatpak fix came in clutch today too with
> 6.13.4.
>
> Antheas
>
Maybe the current strategy of the legacy platform-profile interface can be extended
without introducing the "secondary handler" concept.

The current strategy only advertises platform profiles supported by all handlers, and
as you pointed out this causes problems for users on certain devices.

I was thinking that be can change this strategy to advertise all platform profiles supported
by at least one handler can then do something like this:

  - handler 1: supports low_power, balanced and performance

  - handler 2: supports quiet, balanced and balanced-performance

-> legacy interface advertises low_power, quiet, balanced, balanced-performance and performance

When setting low_power, the closes equivalent is picked for handlers which do not support low_power:

  - handler 1: setting low_power

  - handler 2: setting quiet

When setting quiet, the same happens:

  - handler 1: setting balanced

  - handler 2: setting quiet

Basically all profiles get treated like a range:

low_power <- lower end of the performance range
cool,
quiet,
balanced,
balanced-performance,
performance <- upper end of the performance range

The only problem will be that getting the current platform profile would be more difficult, as
the legacy handler has to determine the lowest currently selected platform profile.

Would this approach be OK?

Thanks,
Armin Wolf

Re: [PATCH 0/3] ACPI: platform_profile: fix legacy sysfs with multiple handlers

[Antheas Kapenekakis]

On Tue, 25 Feb 2025 at 16:56, Armin Wolf <W_Armin@gmx.de> wrote:
>
> Am 25.02.25 um 03:26 schrieb Antheas Kapenekakis:
>
> >> If these "scripts" use `platform_profile_choices` to get their
> >> selections and verify they are available then there should be zero
> >> breakage. If they don't then they should be updated to be correct.
> > Yeah, if any Asus users wrote scripts for their laptops to e.g., "echo
> > quiet | sudo tee /sys/firmware/acpi/platform_profile" or used TLP let
> > them spend a few days finding out why kernel 6.14 does not work. They
> > should have written a 300 line bash script instead.
>
> Hi,
>
> using "echo quiet | sudo tee /sys/firmware/acpi/platform_profile" is quite
> brittle, as some hardware will populate the available profiles dynamically.
>
> Still breaking userspace is indeed not an option here, so we have to think
> of something else.

e.g., for example I had a tdp handler for the ally and had to
transplant it to thermal_throttle_profile when the breakage with
amd_pmf started happening.

> snip
>
> This whole driver was likely written by someone as a hobby, so you already
> depend on a hobby here.
>
> That being said, i agree that fixes have a priority over new features, and
> i think everyone agrees on that.

Indeed.In which case, saying people should use your hobby code perhaps
is a bit overreaching. Haha.

> Maybe the current strategy of the legacy platform-profile interface can be extended
> without introducing the "secondary handler" concept.
>
> The current strategy only advertises platform profiles supported by all handlers, and
> as you pointed out this causes problems for users on certain devices.
>
> I was thinking that be can change this strategy to advertise all platform profiles supported
> by at least one handler can then do something like this:
>
>   - handler 1: supports low_power, balanced and performance
>
>   - handler 2: supports quiet, balanced and balanced-performance
>
> -> legacy interface advertises low_power, quiet, balanced, balanced-performance and performance
>
> When setting low_power, the closes equivalent is picked for handlers which do not support low_power:
>
>   - handler 1: setting low_power
>
>   - handler 2: setting quiet
>
> When setting quiet, the same happens:
>
>   - handler 1: setting balanced
>
>   - handler 2: setting quiet
>
> Basically all profiles get treated like a range:
>
> low_power <- lower end of the performance range
> cool,
> quiet,
> balanced,
> balanced-performance,
> performance <- upper end of the performance range
>
> The only problem will be that getting the current platform profile would be more difficult, as
> the legacy handler has to determine the lowest currently selected platform profile.
>
> Would this approach be OK?
>
> Thanks,
> Armin Wolf
>

So the way this patch series is designed is that the new
/sys/class/platform-profileX works exactly the same.

Then, when asus-wmi is loaded, regardless of whether amd-pmf is
loaded, you get quiet, balanced, and performance. Like it was before.
Setting it to quiet makes amd-pmf use its low power setting. And it
will work the same with all WMI drivers, regardless of whether they
use cool, low-power, quiet, or balanced-power

When asus-wmi is unloaded, you get low-power, balanced, and
performance. As you would with amd-pmf on its own. 0 ABI changes.

Series is small enough so that if you don't like it, it is easy to
refactor out during the 6.15 merge window.

$ cat /sys/firmware/acpi/platform_profile_choices
quiet balanced performance
$ sudo rmmod asus-nb-wmi
$ cat /sys/firmware/acpi/platform_profile_choices
low-power balanced performance

Antheas

Re: [PATCH 0/3] ACPI: platform_profile: fix legacy sysfs with multiple handlers

[Rafael J. Wysocki]

On Tue, Feb 25, 2025 at 7:07 AM Antheas Kapenekakis <lkml@antheas.dev> wrote:
>
> Yes, making asus-wmi use low-power is indeed the easiest solution, but
> if I thought it was good enough, I would have done that already as a
> downstream consumer of the kernel.
>
> I just want to be done with this once and for all, so I spent an extra
> hour today solving this in a cleaner way.

What about adding "quiet" as a "hidden choice" to amd-pmf such that it
would allow the test_bit(*bit, handler->choices) check in
_store_class_profile() to pass, but it would not cause this "choice"
to become visible in the new I/F (or when amd-pmf becomes the only
platform-profile driver) and it would be aliased to "low-power"
internally?

Re: [PATCH 0/3] ACPI: platform_profile: fix legacy sysfs with multiple handlers

[Antheas Kapenekakis]

This is what this patch series essentially does. It makes amd-pmf
accept all choices but only show its own in its own handler and when
it is the only option

On Tue, 25 Feb 2025 at 21:22, Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Tue, Feb 25, 2025 at 7:07 AM Antheas Kapenekakis <lkml@antheas.dev> wrote:
> >
> > Yes, making asus-wmi use low-power is indeed the easiest solution, but
> > if I thought it was good enough, I would have done that already as a
> > downstream consumer of the kernel.
> >
> > I just want to be done with this once and for all, so I spent an extra
> > hour today solving this in a cleaner way.
>
> What about adding "quiet" as a "hidden choice" to amd-pmf such that it
> would allow the test_bit(*bit, handler->choices) check in
> _store_class_profile() to pass, but it would not cause this "choice"
> to become visible in the new I/F (or when amd-pmf becomes the only
> platform-profile driver) and it would be aliased to "low-power"
> internally?

Re: [PATCH 0/3] ACPI: platform_profile: fix legacy sysfs with multiple handlers

[Rafael J. Wysocki]

Top-posting not welcome.

On Wed, Feb 26, 2025 at 8:52 PM Antheas Kapenekakis <lkml@antheas.dev> wrote:
> >
> > What about adding "quiet" as a "hidden choice" to amd-pmf such that it
> > would allow the test_bit(*bit, handler->choices) check in
> > _store_class_profile() to pass, but it would not cause this "choice"
> > to become visible in the new I/F (or when amd-pmf becomes the only
> > platform-profile driver) and it would be aliased to "low-power"
> > internally?
>
> This is what this patch series essentially does. It makes amd-pmf
> accept all choices but only show its own in its own handler and when
> it is the only option

No, it does more than this.  For instance, it is not necessary to do
anything about PLATFORM_PROFILE_BALANCED_PERFORMANCE in it.

The structure of it is questionable either.  It really should be two
patches, one modifying the ACPI platform-profile driver and the other
changing amd-pmf on top of this.

Moreover, I'm not entirely convinced that the "secondary" driver
concept is needed to address the problem at hand.

Re: [PATCH 0/3] ACPI: platform_profile: fix legacy sysfs with multiple handlers

[Antheas Kapenekakis]

On Wed, 26 Feb 2025 at 21:04, Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> Top-posting not welcome.

?

> On Wed, Feb 26, 2025 at 8:52 PM Antheas Kapenekakis <lkml@antheas.dev> wrote:
> > >
> > > What about adding "quiet" as a "hidden choice" to amd-pmf such that it
> > > would allow the test_bit(*bit, handler->choices) check in
> > > _store_class_profile() to pass, but it would not cause this "choice"
> > > to become visible in the new I/F (or when amd-pmf becomes the only
> > > platform-profile driver) and it would be aliased to "low-power"
> > > internally?
> >
> > This is what this patch series essentially does. It makes amd-pmf
> > accept all choices but only show its own in its own handler and when
> > it is the only option
>
> No, it does more than this.

I would say functionality-wise no. The patch could be minified further.

>  For instance, it is not necessary to do
> anything about PLATFORM_PROFILE_BALANCED_PERFORMANCE in it.

I do not see a difference between QUIET and BALANCED_PERFORMANCE, any
driver occluding either causes the same issue. Severity is debatably
lower on BP though.

> The structure of it is questionable either.  It really should be two
> patches, one modifying the ACPI platform-profile driver and the other
> changing amd-pmf on top of this.

Ack. I can spin it up as 2 patches.

> Moreover, I'm not entirely convinced that the "secondary" driver
> concept is needed to address the problem at hand.

Any suggestions on that front would be welcome. This is just the way I
came up with doing it.

Best,
Antheas

Re: [PATCH 3/3] ACPI: platform_profile: Do not hide options missing in secondary handlers

[kernel test robot]

Hi Antheas,

kernel test robot noticed the following build warnings:

[auto build test WARNING on linus/master]
[also build test WARNING on v6.14-rc4 next-20250227]
[cannot apply to rafael-pm/linux-next rafael-pm/bleeding-edge]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Antheas-Kapenekakis/ACPI-platform_profile-Add-support-for-secondary-handlers/20250225-035455
base:   linus/master
patch link:    https://lore.kernel.org/r/20250224195059.10185-4-lkml%40antheas.dev
patch subject: [PATCH 3/3] ACPI: platform_profile: Do not hide options missing in secondary handlers
config: i386-buildonly-randconfig-003-20250227 (https://download.01.org/0day-ci/archive/20250228/202502280150.DkqQsO8C-lkp@intel.com/config)
compiler: clang version 19.1.7 (https://github.com/llvm/llvm-project cd708029e0b2869e80abe31ddb175f7c35361f90)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250228/202502280150.DkqQsO8C-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202502280150.DkqQsO8C-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/acpi/platform_profile.c:248: warning: Function parameter or struct member 'secondary' not described in '_aggregate_choices'


vim +248 drivers/acpi/platform_profile.c

77be5cacb2c2d8 Mario Limonciello   2024-12-05  239  
06ec24388f1de6 Mario Limonciello   2024-12-05  240  /**
06ec24388f1de6 Mario Limonciello   2024-12-05  241   * _aggregate_choices - Aggregate the available profile choices
06ec24388f1de6 Mario Limonciello   2024-12-05  242   * @dev: The device
06ec24388f1de6 Mario Limonciello   2024-12-05  243   * @data: The available profile choices
06ec24388f1de6 Mario Limonciello   2024-12-05  244   *
06ec24388f1de6 Mario Limonciello   2024-12-05  245   * Return: 0 on success, -errno on failure
06ec24388f1de6 Mario Limonciello   2024-12-05  246   */
724b0dfe3faddb Antheas Kapenekakis 2025-02-24  247  static int _aggregate_choices(struct device *dev, void *data, bool secondary)
06ec24388f1de6 Mario Limonciello   2024-12-05 @248  {
06ec24388f1de6 Mario Limonciello   2024-12-05  249  	struct platform_profile_handler *handler;
06ec24388f1de6 Mario Limonciello   2024-12-05  250  	unsigned long *aggregate = data;
06ec24388f1de6 Mario Limonciello   2024-12-05  251  
06ec24388f1de6 Mario Limonciello   2024-12-05  252  	lockdep_assert_held(&profile_lock);
d960f14800b581 Kurt Borja          2025-01-15  253  	handler = to_pprof_handler(dev);
724b0dfe3faddb Antheas Kapenekakis 2025-02-24  254  
724b0dfe3faddb Antheas Kapenekakis 2025-02-24  255  	if (handler->ops->secondary != secondary)
724b0dfe3faddb Antheas Kapenekakis 2025-02-24  256  		return 0;
724b0dfe3faddb Antheas Kapenekakis 2025-02-24  257  
06ec24388f1de6 Mario Limonciello   2024-12-05  258  	if (test_bit(PLATFORM_PROFILE_LAST, aggregate))
06ec24388f1de6 Mario Limonciello   2024-12-05  259  		bitmap_copy(aggregate, handler->choices, PLATFORM_PROFILE_LAST);
724b0dfe3faddb Antheas Kapenekakis 2025-02-24  260  	else if (handler->ops->secondary)
724b0dfe3faddb Antheas Kapenekakis 2025-02-24  261  		bitmap_or(aggregate, handler->choices, aggregate, PLATFORM_PROFILE_LAST);
06ec24388f1de6 Mario Limonciello   2024-12-05  262  	else
06ec24388f1de6 Mario Limonciello   2024-12-05  263  		bitmap_and(aggregate, handler->choices, aggregate, PLATFORM_PROFILE_LAST);
06ec24388f1de6 Mario Limonciello   2024-12-05  264  
06ec24388f1de6 Mario Limonciello   2024-12-05  265  	return 0;
06ec24388f1de6 Mario Limonciello   2024-12-05  266  }
06ec24388f1de6 Mario Limonciello   2024-12-05  267  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki