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

[PATCH v2 0/2] 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?) and only
get worse in 2025, as more laptops start to integrate amd-pmf.

The problem stems from the fact that wmi handlers use different
profiles than amd-pmf, which can load alongside them, and block their
choices. This patch series is a mitigation of this issue, by making pmf
accept all profiles through the legacy sysfs, and by making it a secondary
handler.

While we can argue about whether the secondary handler concept is
necessary, alternatives such as renaming profiles in current drivers will
break existing scripts that are tested for a particular manufacturer, and
allowing amd-pmf override options may cause unforseen regressions in
other wmi drivers.

+CC Luke

Changelog since V1:
    - merge patches 1 and 3 as per Rafael
    - simplify secondary comment about secondary and make it last

Behavior with this patch applied and asus-wmi, amd-pmf
(maintains interop with 6.13):

$ 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 (2):
  ACPI: platform_profile: Add handlers that do not occlude power options
  ACPI: platform_profile: make amd-pmf a secondary handler

 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   |  5 +++
 4 files changed, 65 insertions(+), 8 deletions(-)

-- 
2.48.1

[PATCH v2 1/2] ACPI: platform_profile: Add handlers that do not occlude power options

[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. In 2025, this is only expected
to increase, as more laptops ship with amd-pmf notifiers and this
starts to become an issue in more manufacturers.

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 +++++++++++++++++++++++++++-----
 include/linux/platform_profile.h |  5 +++
 2 files changed, 54 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);
 
diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h
index 8ab5b0e8eb2c..3593d3311a89 100644
--- a/include/linux/platform_profile.h
+++ b/include/linux/platform_profile.h
@@ -37,11 +37,16 @@ enum platform_profile_option {
  *		 profile in sysfs.
  * @profile_set: Callback that will be called when storing a new platform
  *		 profile in sysfs.
+ * @secondary: Set the platform handler as a secondary. Secondary handlers
+ *		 accept all profile options and are not considered in choosing
+ *		 which options to show under the legacy sysfs. This way, they do
+ *		 not occlude the primary handler's choices.
  */
 struct platform_profile_ops {
 	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);
+	bool secondary;
 };
 
 struct device *platform_profile_register(struct device *dev, const char *name,
-- 
2.48.1

[PATCH v2 2/2] ACPI: platform_profile: make amd-pmf a secondary handler

[Antheas Kapenekakis]

Since amd-pmf is expected to run alongside other platform handlers, it
should be able to accept all platform profiles. Therefore, mark it as
secondary and in the case of a custom profile, make it 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 | 8 ++++++++
 2 files changed, 11 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 e6cf0b22dac3..a2a8511768ce 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 */
@@ -400,6 +407,7 @@ static const struct platform_profile_ops amd_pmf_profile_ops = {
 	.probe = amd_pmf_profile_probe,
 	.profile_get = amd_pmf_profile_get,
 	.profile_set = amd_pmf_profile_set,
+	.secondary = true,
 };
 
 int amd_pmf_init_sps(struct amd_pmf_dev *dev)
-- 
2.48.1

Re: [PATCH v2 2/2] ACPI: platform_profile: make amd-pmf a secondary handler

[Mario Limonciello]

On 2/27/2025 09:36, Antheas Kapenekakis wrote:
> Since amd-pmf is expected to run alongside other platform handlers, it
> should be able to accept all platform profiles. Therefore, mark it as
> secondary and in the case of a custom profile, make it 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 | 8 ++++++++
>   2 files changed, 11 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;

I would really prefer we do the absolute bare minimum to help this issue 
on ASUS (just special case quiet) and leave adding compat for other 
profiles for other development.

The reason for this is that if you look at power_modes_v2 there are 
actually 4 'possible' modes for v2 platforms.  So there is a bit of 
nuance involved and it's really not a 'bug fix' anymore by doing so much 
at once.

>   		break;
>   	default:
> diff --git a/drivers/platform/x86/amd/pmf/sps.c b/drivers/platform/x86/amd/pmf/sps.c
> index e6cf0b22dac3..a2a8511768ce 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;
> +

The legacy interface doesn't support writing custom.

https://github.com/torvalds/linux/blob/v6.14-rc3/drivers/acpi/platform_profile.c#L382

IoW this is dead code.

>   	pmf->current_profile = profile;
>   
>   	/* Notify EC about the slider position change */
> @@ -400,6 +407,7 @@ static const struct platform_profile_ops amd_pmf_profile_ops = {
>   	.probe = amd_pmf_profile_probe,
>   	.profile_get = amd_pmf_profile_get,
>   	.profile_set = amd_pmf_profile_set,
> +	.secondary = true,

I really don't understand the need for declaring primary / secondary. 
It really doesn't matter which driver can do it.  This same problem 
could happen in any direction.

As a different suggestion; how about a new "generic" callback for
'compatibility' profiles?

Right now the .probe() callback amd_pmf_get_pprof_modes() will set bits 
for visible profiles.

How about an optional .compat_profiles() for the hidden one(s)?  This 
would allow any driver to implement them.

>   };
>   
>   int amd_pmf_init_sps(struct amd_pmf_dev *dev)

Re: [PATCH v2 2/2] ACPI: platform_profile: make amd-pmf a secondary handler

[Antheas Kapenekakis]

On Thu, 27 Feb 2025 at 17:46, Mario Limonciello
<mario.limonciello@amd.com> wrote:
>
> On 2/27/2025 09:36, Antheas Kapenekakis wrote:
> > Since amd-pmf is expected to run alongside other platform handlers, it
> > should be able to accept all platform profiles. Therefore, mark it as
> > secondary and in the case of a custom profile, make it 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 | 8 ++++++++
> >   2 files changed, 11 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;
>
> I would really prefer we do the absolute bare minimum to help this issue
> on ASUS (just special case quiet) and leave adding compat for other
> profiles for other development.

I cannot risk other drivers having their options disabled. This can
have carry-on effects in other drivers too.

Including in the legion v3 driver, in which you will end up disabling
balanced-performance. Since Derek posted the v3 for that today.

> The reason for this is that if you look at power_modes_v2 there are
> actually 4 'possible' modes for v2 platforms.  So there is a bit of
> nuance involved and it's really not a 'bug fix' anymore by doing so much
> at once.
>
> >               break;
> >       default:
> > diff --git a/drivers/platform/x86/amd/pmf/sps.c b/drivers/platform/x86/amd/pmf/sps.c
> > index e6cf0b22dac3..a2a8511768ce 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;
> > +
>
> The legacy interface doesn't support writing custom.
>
> https://github.com/torvalds/linux/blob/v6.14-rc3/drivers/acpi/platform_profile.c#L382
>
> IoW this is dead code.

Noted.

> >       pmf->current_profile = profile;
> >
> >       /* Notify EC about the slider position change */
> > @@ -400,6 +407,7 @@ static const struct platform_profile_ops amd_pmf_profile_ops = {
> >       .probe = amd_pmf_profile_probe,
> >       .profile_get = amd_pmf_profile_get,
> >       .profile_set = amd_pmf_profile_set,
> > +     .secondary = true,
>
> I really don't understand the need for declaring primary / secondary.
> It really doesn't matter which driver can do it.  This same problem
> could happen in any direction.

No. amd-pmf is responsible here. That's why you made the multiple
platform profile series after all. Other WMI drivers never load
together. To maintain existing compatibility, those drivers need to
still show the same options under the legacy endpoint.

> As a different suggestion; how about a new "generic" callback for
> 'compatibility' profiles?
>
> Right now the .probe() callback amd_pmf_get_pprof_modes() will set bits
> for visible profiles.
>
> How about an optional .compat_profiles() for the hidden one(s)?  This
> would allow any driver to implement them.

amd-pmf cannot obscure any settings of the primary platform, so even
in this case it ends up implementing all of them, and compat profiles
becomes equivalent to .secondary with more steps (incl. a probe).

> >   };
> >
> >   int amd_pmf_init_sps(struct amd_pmf_dev *dev)
>

Re: [PATCH v2 2/2] ACPI: platform_profile: make amd-pmf a secondary handler

[Mario Limonciello]

On 2/27/2025 11:04, Antheas Kapenekakis wrote:
> On Thu, 27 Feb 2025 at 17:46, Mario Limonciello
> <mario.limonciello@amd.com> wrote:
>>
>> On 2/27/2025 09:36, Antheas Kapenekakis wrote:
>>> Since amd-pmf is expected to run alongside other platform handlers, it
>>> should be able to accept all platform profiles. Therefore, mark it as
>>> secondary and in the case of a custom profile, make it 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 | 8 ++++++++
>>>    2 files changed, 11 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;
>>
>> I would really prefer we do the absolute bare minimum to help this issue
>> on ASUS (just special case quiet) and leave adding compat for other
>> profiles for other development.
> 
> I cannot risk other drivers having their options disabled. This can
> have carry-on effects in other drivers too.
> 
> Including in the legion v3 driver, in which you will end up disabling
> balanced-performance. Since Derek posted the v3 for that today.
> 

Sure - but let's handle that separately from this bug fix.  That driver 
will be targeted to 6.15 or later.

We need to be cognizant about what can go into 6.14 needs to be bug 
fixes for drivers in tree.

>> The reason for this is that if you look at power_modes_v2 there are
>> actually 4 'possible' modes for v2 platforms.  So there is a bit of
>> nuance involved and it's really not a 'bug fix' anymore by doing so much
>> at once.
>>
>>>                break;
>>>        default:
>>> diff --git a/drivers/platform/x86/amd/pmf/sps.c b/drivers/platform/x86/amd/pmf/sps.c
>>> index e6cf0b22dac3..a2a8511768ce 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;
>>> +
>>
>> The legacy interface doesn't support writing custom.
>>
>> https://github.com/torvalds/linux/blob/v6.14-rc3/drivers/acpi/platform_profile.c#L382
>>
>> IoW this is dead code.
> 
> Noted.
> 
>>>        pmf->current_profile = profile;
>>>
>>>        /* Notify EC about the slider position change */
>>> @@ -400,6 +407,7 @@ static const struct platform_profile_ops amd_pmf_profile_ops = {
>>>        .probe = amd_pmf_profile_probe,
>>>        .profile_get = amd_pmf_profile_get,
>>>        .profile_set = amd_pmf_profile_set,
>>> +     .secondary = true,
>>
>> I really don't understand the need for declaring primary / secondary.
>> It really doesn't matter which driver can do it.  This same problem
>> could happen in any direction.
> 
> No. amd-pmf is responsible here. That's why you made the multiple
> platform profile series after all. Other WMI drivers never load
> together. To maintain existing compatibility, those drivers need to
> still show the same options under the legacy endpoint.

My point is mostly hypothetical right now because the realistic 
combinations right now are amd-pmf + other driver.

> 
>> As a different suggestion; how about a new "generic" callback for
>> 'compatibility' profiles?
>>
>> Right now the .probe() callback amd_pmf_get_pprof_modes() will set bits
>> for visible profiles.
>>
>> How about an optional .compat_profiles() for the hidden one(s)?  This
>> would allow any driver to implement them.
> 
> amd-pmf cannot obscure any settings of the primary platform, so even
> in this case it ends up implementing all of them, and compat profiles
> becomes equivalent to .secondary with more steps (incl. a probe).
> 
>>>    };
>>>
>>>    int amd_pmf_init_sps(struct amd_pmf_dev *dev)
>>

Re: [PATCH v2 2/2] ACPI: platform_profile: make amd-pmf a secondary handler

[Antheas Kapenekakis]

On Thu, 27 Feb 2025 at 18:10, Mario Limonciello
<mario.limonciello@amd.com> wrote:
>
> On 2/27/2025 11:04, Antheas Kapenekakis wrote:
> > On Thu, 27 Feb 2025 at 17:46, Mario Limonciello
> > <mario.limonciello@amd.com> wrote:
> >>
> >> On 2/27/2025 09:36, Antheas Kapenekakis wrote:
> >>> Since amd-pmf is expected to run alongside other platform handlers, it
> >>> should be able to accept all platform profiles. Therefore, mark it as
> >>> secondary and in the case of a custom profile, make it 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 | 8 ++++++++
> >>>    2 files changed, 11 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;
> >>
> >> I would really prefer we do the absolute bare minimum to help this issue
> >> on ASUS (just special case quiet) and leave adding compat for other
> >> profiles for other development.
> >
> > I cannot risk other drivers having their options disabled. This can
> > have carry-on effects in other drivers too.
> >
> > Including in the legion v3 driver, in which you will end up disabling
> > balanced-performance. Since Derek posted the v3 for that today.
> >
>
> Sure - but let's handle that separately from this bug fix.  That driver
> will be targeted to 6.15 or later.
>
> We need to be cognizant about what can go into 6.14 needs to be bug
> fixes for drivers in tree.

For me to consider this problem resolved, I need a mitigation that
matches the behavior of this patch series 1-1.

If you have a better suggestion, I can implement it and test it real quick.

If this issue is not fully resolved, it will cause a lot of downstream
issues that will result in the legacy interface becoming unusable.

Acer and alienware implement balanced performance too. In the current tree.

> >> The reason for this is that if you look at power_modes_v2 there are
> >> actually 4 'possible' modes for v2 platforms.  So there is a bit of
> >> nuance involved and it's really not a 'bug fix' anymore by doing so much
> >> at once.
> >>
> >>>                break;
> >>>        default:
> >>> diff --git a/drivers/platform/x86/amd/pmf/sps.c b/drivers/platform/x86/amd/pmf/sps.c
> >>> index e6cf0b22dac3..a2a8511768ce 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;
> >>> +
> >>
> >> The legacy interface doesn't support writing custom.
> >>
> >> https://github.com/torvalds/linux/blob/v6.14-rc3/drivers/acpi/platform_profile.c#L382
> >>
> >> IoW this is dead code.
> >
> > Noted.
> >
> >>>        pmf->current_profile = profile;
> >>>
> >>>        /* Notify EC about the slider position change */
> >>> @@ -400,6 +407,7 @@ static const struct platform_profile_ops amd_pmf_profile_ops = {
> >>>        .probe = amd_pmf_profile_probe,
> >>>        .profile_get = amd_pmf_profile_get,
> >>>        .profile_set = amd_pmf_profile_set,
> >>> +     .secondary = true,
> >>
> >> I really don't understand the need for declaring primary / secondary.
> >> It really doesn't matter which driver can do it.  This same problem
> >> could happen in any direction.
> >
> > No. amd-pmf is responsible here. That's why you made the multiple
> > platform profile series after all. Other WMI drivers never load
> > together. To maintain existing compatibility, those drivers need to
> > still show the same options under the legacy endpoint.
>
> My point is mostly hypothetical right now because the realistic
> combinations right now are amd-pmf + other driver.
>
> >
> >> As a different suggestion; how about a new "generic" callback for
> >> 'compatibility' profiles?
> >>
> >> Right now the .probe() callback amd_pmf_get_pprof_modes() will set bits
> >> for visible profiles.
> >>
> >> How about an optional .compat_profiles() for the hidden one(s)?  This
> >> would allow any driver to implement them.
> >
> > amd-pmf cannot obscure any settings of the primary platform, so even
> > in this case it ends up implementing all of them, and compat profiles
> > becomes equivalent to .secondary with more steps (incl. a probe).
> >
> >>>    };
> >>>
> >>>    int amd_pmf_init_sps(struct amd_pmf_dev *dev)
> >>
>

Re: [PATCH v2 2/2] ACPI: platform_profile: make amd-pmf a secondary handler

[Mario Limonciello]

On 2/27/2025 11:18, Antheas Kapenekakis wrote:
> On Thu, 27 Feb 2025 at 18:10, Mario Limonciello
> <mario.limonciello@amd.com> wrote:
>>
>> On 2/27/2025 11:04, Antheas Kapenekakis wrote:
>>> On Thu, 27 Feb 2025 at 17:46, Mario Limonciello
>>> <mario.limonciello@amd.com> wrote:
>>>>
>>>> On 2/27/2025 09:36, Antheas Kapenekakis wrote:
>>>>> Since amd-pmf is expected to run alongside other platform handlers, it
>>>>> should be able to accept all platform profiles. Therefore, mark it as
>>>>> secondary and in the case of a custom profile, make it 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 | 8 ++++++++
>>>>>     2 files changed, 11 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;
>>>>
>>>> I would really prefer we do the absolute bare minimum to help this issue
>>>> on ASUS (just special case quiet) and leave adding compat for other
>>>> profiles for other development.
>>>
>>> I cannot risk other drivers having their options disabled. This can
>>> have carry-on effects in other drivers too.
>>>
>>> Including in the legion v3 driver, in which you will end up disabling
>>> balanced-performance. Since Derek posted the v3 for that today.
>>>
>>
>> Sure - but let's handle that separately from this bug fix.  That driver
>> will be targeted to 6.15 or later.
>>
>> We need to be cognizant about what can go into 6.14 needs to be bug
>> fixes for drivers in tree.
> 
> For me to consider this problem resolved, I need a mitigation that
> matches the behavior of this patch series 1-1.
> 
> If you have a better suggestion, I can implement it and test it real quick.

I think just covering the QUIET == LOW_POWER is the important one for now.

> 
> If this issue is not fully resolved, it will cause a lot of downstream
> issues that will result in the legacy interface becoming unusable.
> 
> Acer and alienware implement balanced performance too. In the current tree.

But do Acer and Alienware have designs that amd-pmf will bind at the 
same time?

I'm not so sure.

> 
>>>> The reason for this is that if you look at power_modes_v2 there are
>>>> actually 4 'possible' modes for v2 platforms.  So there is a bit of
>>>> nuance involved and it's really not a 'bug fix' anymore by doing so much
>>>> at once.
>>>>
>>>>>                 break;
>>>>>         default:
>>>>> diff --git a/drivers/platform/x86/amd/pmf/sps.c b/drivers/platform/x86/amd/pmf/sps.c
>>>>> index e6cf0b22dac3..a2a8511768ce 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;
>>>>> +
>>>>
>>>> The legacy interface doesn't support writing custom.
>>>>
>>>> https://github.com/torvalds/linux/blob/v6.14-rc3/drivers/acpi/platform_profile.c#L382
>>>>
>>>> IoW this is dead code.
>>>
>>> Noted.
>>>
>>>>>         pmf->current_profile = profile;
>>>>>
>>>>>         /* Notify EC about the slider position change */
>>>>> @@ -400,6 +407,7 @@ static const struct platform_profile_ops amd_pmf_profile_ops = {
>>>>>         .probe = amd_pmf_profile_probe,
>>>>>         .profile_get = amd_pmf_profile_get,
>>>>>         .profile_set = amd_pmf_profile_set,
>>>>> +     .secondary = true,
>>>>
>>>> I really don't understand the need for declaring primary / secondary.
>>>> It really doesn't matter which driver can do it.  This same problem
>>>> could happen in any direction.
>>>
>>> No. amd-pmf is responsible here. That's why you made the multiple
>>> platform profile series after all. Other WMI drivers never load
>>> together. To maintain existing compatibility, those drivers need to
>>> still show the same options under the legacy endpoint.
>>
>> My point is mostly hypothetical right now because the realistic
>> combinations right now are amd-pmf + other driver.
>>
>>>
>>>> As a different suggestion; how about a new "generic" callback for
>>>> 'compatibility' profiles?
>>>>
>>>> Right now the .probe() callback amd_pmf_get_pprof_modes() will set bits
>>>> for visible profiles.
>>>>
>>>> How about an optional .compat_profiles() for the hidden one(s)?  This
>>>> would allow any driver to implement them.
>>>
>>> amd-pmf cannot obscure any settings of the primary platform, so even
>>> in this case it ends up implementing all of them, and compat profiles
>>> becomes equivalent to .secondary with more steps (incl. a probe).
>>>
>>>>>     };
>>>>>
>>>>>     int amd_pmf_init_sps(struct amd_pmf_dev *dev)
>>>>
>>

Re: [PATCH v2 2/2] ACPI: platform_profile: make amd-pmf a secondary handler

[Antheas Kapenekakis]

On Thu, 27 Feb 2025 at 18:24, Mario Limonciello
<mario.limonciello@amd.com> wrote:
>
> On 2/27/2025 11:18, Antheas Kapenekakis wrote:
> > On Thu, 27 Feb 2025 at 18:10, Mario Limonciello
> > <mario.limonciello@amd.com> wrote:
> >>
> >> On 2/27/2025 11:04, Antheas Kapenekakis wrote:
> >>> On Thu, 27 Feb 2025 at 17:46, Mario Limonciello
> >>> <mario.limonciello@amd.com> wrote:
> >>>>
> >>>> On 2/27/2025 09:36, Antheas Kapenekakis wrote:
> >>>>> Since amd-pmf is expected to run alongside other platform handlers, it
> >>>>> should be able to accept all platform profiles. Therefore, mark it as
> >>>>> secondary and in the case of a custom profile, make it 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 | 8 ++++++++
> >>>>>     2 files changed, 11 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;
> >>>>
> >>>> I would really prefer we do the absolute bare minimum to help this issue
> >>>> on ASUS (just special case quiet) and leave adding compat for other
> >>>> profiles for other development.
> >>>
> >>> I cannot risk other drivers having their options disabled. This can
> >>> have carry-on effects in other drivers too.
> >>>
> >>> Including in the legion v3 driver, in which you will end up disabling
> >>> balanced-performance. Since Derek posted the v3 for that today.
> >>>
> >>
> >> Sure - but let's handle that separately from this bug fix.  That driver
> >> will be targeted to 6.15 or later.
> >>
> >> We need to be cognizant about what can go into 6.14 needs to be bug
> >> fixes for drivers in tree.
> >
> > For me to consider this problem resolved, I need a mitigation that
> > matches the behavior of this patch series 1-1.
> >
> > If you have a better suggestion, I can implement it and test it real quick.
>
> I think just covering the QUIET == LOW_POWER is the important one for now.

Sure, how do we do that? You want to make amd-pmf accept both just for
6.14? I would be ok with that.

> >
> > If this issue is not fully resolved, it will cause a lot of downstream
> > issues that will result in the legacy interface becoming unusable.
> >
> > Acer and alienware implement balanced performance too. In the current tree.
>
> But do Acer and Alienware have designs that amd-pmf will bind at the
> same time?
>
> I'm not so sure.

From a quick google search, Acer Swift Edge 16 - 8840U. But we do not
have a lot of acer users I'd say.

> >
> snip
> >>
>