* [PATCH v2 0/1] Add quiet/low power compat code
@ 2025-03-04 6:47 Mario Limonciello
2025-03-04 6:47 ` [PATCH v2 1/1] ACPI: platform_profile: Treat quiet and low power the same Mario Limonciello
0 siblings, 1 reply; 15+ messages in thread
From: Mario Limonciello @ 2025-03-04 6:47 UTC (permalink / raw)
To: Shyam Sundar S K, Rafael J . Wysocki, Hans de Goede,
Ilpo Järvinen, Luke D . Jones, Mark Pearson
Cc: open list:AMD PMF DRIVER, open list, open list:ACPI,
Derek J . Clark, Antheas Kapenekakis, me, Denis Benato,
Mario Limonciello
From: Mario Limonciello <mario.limonciello@amd.com>
When two drivers provide platform profile handlers but use different
strings to mean (essentially) the same thing the legacy interface won't
export them because it only shows profiles common to multiple drivers.
This causes an unexpected behavior to people who have upgraded from an
earlier kernel because if multiple drivers have bound platform profile
handlers they might not be able to access profiles they were expecting.
Introduce a compatibility mode to the core that when one driver supports
quiet and the other supports low power that allows both to enter the
appropriate mode.
There have been some other attempts at solving this issue in other ways.
This serves as an alternative to those attempts.
v1 -> v2:
* Drop hidden choices.
* Just add compatibility to the core for low power and quiet modes
Link: https://lore.kernel.org/platform-driver-x86/e64b771e-3255-42ad-9257-5b8fc6c24ac9@gmx.de/T/#t
Link: https://lore.kernel.org/platform-driver-x86/CAGwozwF-WVEgiAbWbRCiUaXf=BVa3KqmMJfs06trdMQHpTGmjQ@mail.gmail.com/T/#m2f3929e2d4f73cc0eedd14738170dad45232fd18
Link: https://lore.kernel.org/platform-driver-x86/20250228170155.2623386-1-superm1@kernel.org/
Cc: Antheas Kapenekakis <lkml@antheas.dev>
Cc: "Luke D. Jones" <luke@ljones.dev>
Mario Limonciello (1):
ACPI: platform_profile: Treat quiet and low power the same
drivers/acpi/platform_profile.c | 26 +++++++++++++++++++++++---
1 file changed, 23 insertions(+), 3 deletions(-)
--
2.43.0
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 1/1] ACPI: platform_profile: Treat quiet and low power the same
2025-03-04 6:47 [PATCH v2 0/1] Add quiet/low power compat code Mario Limonciello
@ 2025-03-04 6:47 ` Mario Limonciello
2025-03-04 8:38 ` Antheas Kapenekakis
2025-03-04 15:24 ` Derek J. Clark
0 siblings, 2 replies; 15+ messages in thread
From: Mario Limonciello @ 2025-03-04 6:47 UTC (permalink / raw)
To: Shyam Sundar S K, Rafael J . Wysocki, Hans de Goede,
Ilpo Järvinen, Luke D . Jones, Mark Pearson
Cc: open list:AMD PMF DRIVER, open list, open list:ACPI,
Derek J . Clark, Antheas Kapenekakis, me, Denis Benato,
Mario Limonciello
From: Mario Limonciello <mario.limonciello@amd.com>
When two drivers don't support all the same profiles the legacy interface
only exports the common profiles.
This causes problems for cases where one driver uses low-power but another
uses quiet because the result is that neither is exported to sysfs.
If one platform profile handler supports quiet and the other
supports low power treat them as the same for the purpose of
the sysfs interface.
Fixes: 688834743d67 ("ACPI: platform_profile: Allow multiple handlers")
Reported-by: Antheas Kapenekakis <lkml@antheas.dev>
Closes: https://lore.kernel.org/platform-driver-x86/e64b771e-3255-42ad-9257-5b8fc6c24ac9@gmx.de/T/#mc068042dd29df36c16c8af92664860fc4763974b
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
drivers/acpi/platform_profile.c | 38 ++++++++++++++++++++++++++++++---
1 file changed, 35 insertions(+), 3 deletions(-)
diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
index 2ad53cc6aae53..d9a7cc5891734 100644
--- a/drivers/acpi/platform_profile.c
+++ b/drivers/acpi/platform_profile.c
@@ -73,8 +73,20 @@ static int _store_class_profile(struct device *dev, void *data)
lockdep_assert_held(&profile_lock);
handler = to_pprof_handler(dev);
- if (!test_bit(*bit, handler->choices))
- return -EOPNOTSUPP;
+ if (!test_bit(*bit, handler->choices)) {
+ switch (*bit) {
+ case PLATFORM_PROFILE_QUIET:
+ *bit = PLATFORM_PROFILE_LOW_POWER;
+ break;
+ case PLATFORM_PROFILE_LOW_POWER:
+ *bit = PLATFORM_PROFILE_QUIET;
+ break;
+ default:
+ return -EOPNOTSUPP;
+ }
+ if (!test_bit(*bit, handler->choices))
+ return -EOPNOTSUPP;
+ }
return handler->ops->profile_set(dev, *bit);
}
@@ -252,8 +264,16 @@ static int _aggregate_choices(struct device *dev, void *data)
handler = to_pprof_handler(dev);
if (test_bit(PLATFORM_PROFILE_LAST, aggregate))
bitmap_copy(aggregate, handler->choices, PLATFORM_PROFILE_LAST);
- else
+ else {
+ /* treat quiet and low power the same for aggregation purposes */
+ if (test_bit(PLATFORM_PROFILE_QUIET, handler->choices) &&
+ test_bit(PLATFORM_PROFILE_LOW_POWER, aggregate))
+ set_bit(PLATFORM_PROFILE_QUIET, aggregate);
+ else if (test_bit(PLATFORM_PROFILE_LOW_POWER, handler->choices) &&
+ test_bit(PLATFORM_PROFILE_QUIET, aggregate))
+ set_bit(PLATFORM_PROFILE_LOW_POWER, aggregate);
bitmap_and(aggregate, handler->choices, aggregate, PLATFORM_PROFILE_LAST);
+ }
return 0;
}
@@ -305,6 +325,13 @@ static int _aggregate_profiles(struct device *dev, void *data)
if (err)
return err;
+ /* treat low-power and quiet as the same */
+ if ((*profile == PLATFORM_PROFILE_LOW_POWER &&
+ val == PLATFORM_PROFILE_QUIET) ||
+ (*profile == PLATFORM_PROFILE_QUIET &&
+ val == PLATFORM_PROFILE_LOW_POWER))
+ *profile = val;
+
if (*profile != PLATFORM_PROFILE_LAST && *profile != val)
*profile = PLATFORM_PROFILE_CUSTOM;
else
@@ -531,6 +558,11 @@ struct device *platform_profile_register(struct device *dev, const char *name,
dev_err(dev, "Failed to register platform_profile class device with empty choices\n");
return ERR_PTR(-EINVAL);
}
+ if (test_bit(PLATFORM_PROFILE_QUIET, pprof->choices) &&
+ test_bit(PLATFORM_PROFILE_LOW_POWER, pprof->choices)) {
+ dev_err(dev, "Failed to register platform_profile class device with both quiet and low-power\n");
+ return ERR_PTR(-EINVAL);
+ }
guard(mutex)(&profile_lock);
--
2.43.0
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/1] ACPI: platform_profile: Treat quiet and low power the same
2025-03-04 6:47 ` [PATCH v2 1/1] ACPI: platform_profile: Treat quiet and low power the same Mario Limonciello
@ 2025-03-04 8:38 ` Antheas Kapenekakis
2025-03-04 12:49 ` Mario Limonciello
2025-03-04 15:24 ` Derek J. Clark
1 sibling, 1 reply; 15+ messages in thread
From: Antheas Kapenekakis @ 2025-03-04 8:38 UTC (permalink / raw)
To: Mario Limonciello
Cc: Shyam Sundar S K, Rafael J . Wysocki, Hans de Goede,
Ilpo Järvinen, Luke D . Jones, Mark Pearson,
open list:AMD PMF DRIVER, open list, open list:ACPI,
Derek J . Clark, me, Denis Benato, Mario Limonciello
On Tue, 4 Mar 2025 at 07:48, Mario Limonciello <superm1@kernel.org> wrote:
>
> From: Mario Limonciello <mario.limonciello@amd.com>
>
> When two drivers don't support all the same profiles the legacy interface
> only exports the common profiles.
>
> This causes problems for cases where one driver uses low-power but another
> uses quiet because the result is that neither is exported to sysfs.
>
> If one platform profile handler supports quiet and the other
> supports low power treat them as the same for the purpose of
> the sysfs interface.
>
> Fixes: 688834743d67 ("ACPI: platform_profile: Allow multiple handlers")
> Reported-by: Antheas Kapenekakis <lkml@antheas.dev>
> Closes: https://lore.kernel.org/platform-driver-x86/e64b771e-3255-42ad-9257-5b8fc6c24ac9@gmx.de/T/#mc068042dd29df36c16c8af92664860fc4763974b
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> drivers/acpi/platform_profile.c | 38 ++++++++++++++++++++++++++++++---
> 1 file changed, 35 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
> index 2ad53cc6aae53..d9a7cc5891734 100644
> --- a/drivers/acpi/platform_profile.c
> +++ b/drivers/acpi/platform_profile.c
> @@ -73,8 +73,20 @@ static int _store_class_profile(struct device *dev, void *data)
>
> lockdep_assert_held(&profile_lock);
> handler = to_pprof_handler(dev);
> - if (!test_bit(*bit, handler->choices))
> - return -EOPNOTSUPP;
> + if (!test_bit(*bit, handler->choices)) {
> + switch (*bit) {
> + case PLATFORM_PROFILE_QUIET:
> + *bit = PLATFORM_PROFILE_LOW_POWER;
> + break;
> + case PLATFORM_PROFILE_LOW_POWER:
> + *bit = PLATFORM_PROFILE_QUIET;
> + break;
> + default:
> + return -EOPNOTSUPP;
> + }
> + if (!test_bit(*bit, handler->choices))
> + return -EOPNOTSUPP;
> + }
>
> return handler->ops->profile_set(dev, *bit);
> }
> @@ -252,8 +264,16 @@ static int _aggregate_choices(struct device *dev, void *data)
> handler = to_pprof_handler(dev);
> if (test_bit(PLATFORM_PROFILE_LAST, aggregate))
> bitmap_copy(aggregate, handler->choices, PLATFORM_PROFILE_LAST);
> - else
> + else {
> + /* treat quiet and low power the same for aggregation purposes */
> + if (test_bit(PLATFORM_PROFILE_QUIET, handler->choices) &&
> + test_bit(PLATFORM_PROFILE_LOW_POWER, aggregate))
> + set_bit(PLATFORM_PROFILE_QUIET, aggregate);
> + else if (test_bit(PLATFORM_PROFILE_LOW_POWER, handler->choices) &&
> + test_bit(PLATFORM_PROFILE_QUIET, aggregate))
> + set_bit(PLATFORM_PROFILE_LOW_POWER, aggregate);
> bitmap_and(aggregate, handler->choices, aggregate, PLATFORM_PROFILE_LAST);
> + }
So you end up showing both? If that's the case, isn't it equivalent to
just make amd-pmf show both quiet and low-power?
I guess it is not ideal for framework devices. But if asus devices end
up showing both, then it should be ok for framework devices to show
both.
I like the behavior of the V1 personally.
> return 0;
> }
> @@ -305,6 +325,13 @@ static int _aggregate_profiles(struct device *dev, void *data)
> if (err)
> return err;
>
> + /* treat low-power and quiet as the same */
> + if ((*profile == PLATFORM_PROFILE_LOW_POWER &&
> + val == PLATFORM_PROFILE_QUIET) ||
> + (*profile == PLATFORM_PROFILE_QUIET &&
> + val == PLATFORM_PROFILE_LOW_POWER))
> + *profile = val;
> +
> if (*profile != PLATFORM_PROFILE_LAST && *profile != val)
> *profile = PLATFORM_PROFILE_CUSTOM;
> else
> @@ -531,6 +558,11 @@ struct device *platform_profile_register(struct device *dev, const char *name,
> dev_err(dev, "Failed to register platform_profile class device with empty choices\n");
> return ERR_PTR(-EINVAL);
> }
> + if (test_bit(PLATFORM_PROFILE_QUIET, pprof->choices) &&
> + test_bit(PLATFORM_PROFILE_LOW_POWER, pprof->choices)) {
> + dev_err(dev, "Failed to register platform_profile class device with both quiet and low-power\n");
> + return ERR_PTR(-EINVAL);
> + }
Can you avoid failing here? It caused a lot of issues in the past (the
WMI driver bails). a dev_err should be enough. Since you do not fail
maybe it can be increased to dev_crit.
There is at least one driver that implements both currently, and a fix
would have to precede this patch.
>
> guard(mutex)(&profile_lock);
>
> --
> 2.43.0
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/1] ACPI: platform_profile: Treat quiet and low power the same
2025-03-04 8:38 ` Antheas Kapenekakis
@ 2025-03-04 12:49 ` Mario Limonciello
2025-03-04 13:06 ` Antheas Kapenekakis
` (2 more replies)
0 siblings, 3 replies; 15+ messages in thread
From: Mario Limonciello @ 2025-03-04 12:49 UTC (permalink / raw)
To: Antheas Kapenekakis, Kurt Borja
Cc: Shyam Sundar S K, Rafael J . Wysocki, Hans de Goede,
Ilpo Järvinen, Luke D . Jones, Mark Pearson,
open list:AMD PMF DRIVER, open list, open list:ACPI,
Derek J . Clark, me, Denis Benato, Mario Limonciello
On 3/4/25 02:38, Antheas Kapenekakis wrote:
> On Tue, 4 Mar 2025 at 07:48, Mario Limonciello <superm1@kernel.org> wrote:
>>
>> From: Mario Limonciello <mario.limonciello@amd.com>
>>
>> When two drivers don't support all the same profiles the legacy interface
>> only exports the common profiles.
>>
>> This causes problems for cases where one driver uses low-power but another
>> uses quiet because the result is that neither is exported to sysfs.
>>
>> If one platform profile handler supports quiet and the other
>> supports low power treat them as the same for the purpose of
>> the sysfs interface.
>>
>> Fixes: 688834743d67 ("ACPI: platform_profile: Allow multiple handlers")
>> Reported-by: Antheas Kapenekakis <lkml@antheas.dev>
>> Closes: https://lore.kernel.org/platform-driver-x86/e64b771e-3255-42ad-9257-5b8fc6c24ac9@gmx.de/T/#mc068042dd29df36c16c8af92664860fc4763974b
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>> drivers/acpi/platform_profile.c | 38 ++++++++++++++++++++++++++++++---
>> 1 file changed, 35 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
>> index 2ad53cc6aae53..d9a7cc5891734 100644
>> --- a/drivers/acpi/platform_profile.c
>> +++ b/drivers/acpi/platform_profile.c
>> @@ -73,8 +73,20 @@ static int _store_class_profile(struct device *dev, void *data)
>>
>> lockdep_assert_held(&profile_lock);
>> handler = to_pprof_handler(dev);
>> - if (!test_bit(*bit, handler->choices))
>> - return -EOPNOTSUPP;
>> + if (!test_bit(*bit, handler->choices)) {
>> + switch (*bit) {
>> + case PLATFORM_PROFILE_QUIET:
>> + *bit = PLATFORM_PROFILE_LOW_POWER;
>> + break;
>> + case PLATFORM_PROFILE_LOW_POWER:
>> + *bit = PLATFORM_PROFILE_QUIET;
>> + break;
>> + default:
>> + return -EOPNOTSUPP;
>> + }
>> + if (!test_bit(*bit, handler->choices))
>> + return -EOPNOTSUPP;
>> + }
>>
>> return handler->ops->profile_set(dev, *bit);
>> }
>> @@ -252,8 +264,16 @@ static int _aggregate_choices(struct device *dev, void *data)
>> handler = to_pprof_handler(dev);
>> if (test_bit(PLATFORM_PROFILE_LAST, aggregate))
>> bitmap_copy(aggregate, handler->choices, PLATFORM_PROFILE_LAST);
>> - else
>> + else {
>> + /* treat quiet and low power the same for aggregation purposes */
>> + if (test_bit(PLATFORM_PROFILE_QUIET, handler->choices) &&
>> + test_bit(PLATFORM_PROFILE_LOW_POWER, aggregate))
>> + set_bit(PLATFORM_PROFILE_QUIET, aggregate);
>> + else if (test_bit(PLATFORM_PROFILE_LOW_POWER, handler->choices) &&
>> + test_bit(PLATFORM_PROFILE_QUIET, aggregate))
>> + set_bit(PLATFORM_PROFILE_LOW_POWER, aggregate);
>> bitmap_and(aggregate, handler->choices, aggregate, PLATFORM_PROFILE_LAST);
>> + }
>
> So you end up showing both? If that's the case, isn't it equivalent to
> just make amd-pmf show both quiet and low-power?
>
> I guess it is not ideal for framework devices. But if asus devices end
> up showing both, then it should be ok for framework devices to show
> both.
>
> I like the behavior of the V1 personally.
No; this doesn't cause it to show both. It only causes one to show up.
I confirmed it with a contrived situation on my laptop that forced
multiple profile handlers that supported a mix.
# cat /sys/firmware/acpi/platform_profile*
low-power
low-power balanced performance
# cat /sys/class/platform-profile/platform-profile-*/profile
quiet
quiet
quiet
quiet
quiet
quiet
quiet
quiet
quiet
quiet
quiet
quiet
quiet
quiet
quiet
quiet
quiet
quiet
quiet
quiet
quiet
quiet
quiet
quiet
low-power
>
>> return 0;
>> }
>> @@ -305,6 +325,13 @@ static int _aggregate_profiles(struct device *dev, void *data)
>> if (err)
>> return err;
>>
>> + /* treat low-power and quiet as the same */
>> + if ((*profile == PLATFORM_PROFILE_LOW_POWER &&
>> + val == PLATFORM_PROFILE_QUIET) ||
>> + (*profile == PLATFORM_PROFILE_QUIET &&
>> + val == PLATFORM_PROFILE_LOW_POWER))
>> + *profile = val;
>> +
>> if (*profile != PLATFORM_PROFILE_LAST && *profile != val)
>> *profile = PLATFORM_PROFILE_CUSTOM;
>> else
>> @@ -531,6 +558,11 @@ struct device *platform_profile_register(struct device *dev, const char *name,
>> dev_err(dev, "Failed to register platform_profile class device with empty choices\n");
>> return ERR_PTR(-EINVAL);
>> }
>> + if (test_bit(PLATFORM_PROFILE_QUIET, pprof->choices) &&
>> + test_bit(PLATFORM_PROFILE_LOW_POWER, pprof->choices)) {
>> + dev_err(dev, "Failed to register platform_profile class device with both quiet and low-power\n");
>> + return ERR_PTR(-EINVAL);
>> + }
>
> Can you avoid failing here? It caused a lot of issues in the past (the
> WMI driver bails). a dev_err should be enough. Since you do not fail
> maybe it can be increased to dev_crit.
>
> There is at least one driver that implements both currently, and a fix
> would have to precede this patch.
Oh, acer-wmi? Kurt; can you please comment? Are both simultaneous?
>
>>
>> guard(mutex)(&profile_lock);
>>
>> --
>> 2.43.0
>>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/1] ACPI: platform_profile: Treat quiet and low power the same
2025-03-04 12:49 ` Mario Limonciello
@ 2025-03-04 13:06 ` Antheas Kapenekakis
2025-03-04 13:26 ` Kurt Borja
2025-03-04 14:08 ` Rafael J. Wysocki
2 siblings, 0 replies; 15+ messages in thread
From: Antheas Kapenekakis @ 2025-03-04 13:06 UTC (permalink / raw)
To: Mario Limonciello
Cc: Kurt Borja, Shyam Sundar S K, Rafael J . Wysocki, Hans de Goede,
Ilpo Järvinen, Luke D . Jones, Mark Pearson,
open list:AMD PMF DRIVER, open list, open list:ACPI,
Derek J . Clark, me, Denis Benato, Mario Limonciello
On Tue, 4 Mar 2025 at 13:49, Mario Limonciello <superm1@kernel.org> wrote:
>
>
>
> On 3/4/25 02:38, Antheas Kapenekakis wrote:
> > On Tue, 4 Mar 2025 at 07:48, Mario Limonciello <superm1@kernel.org> wrote:
> >>
> >> From: Mario Limonciello <mario.limonciello@amd.com>
> >>
> >> When two drivers don't support all the same profiles the legacy interface
> >> only exports the common profiles.
> >>
> >> This causes problems for cases where one driver uses low-power but another
> >> uses quiet because the result is that neither is exported to sysfs.
> >>
> >> If one platform profile handler supports quiet and the other
> >> supports low power treat them as the same for the purpose of
> >> the sysfs interface.
> >>
> >> Fixes: 688834743d67 ("ACPI: platform_profile: Allow multiple handlers")
> >> Reported-by: Antheas Kapenekakis <lkml@antheas.dev>
> >> Closes: https://lore.kernel.org/platform-driver-x86/e64b771e-3255-42ad-9257-5b8fc6c24ac9@gmx.de/T/#mc068042dd29df36c16c8af92664860fc4763974b
> >> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> >> ---
> >> drivers/acpi/platform_profile.c | 38 ++++++++++++++++++++++++++++++---
> >> 1 file changed, 35 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
> >> index 2ad53cc6aae53..d9a7cc5891734 100644
> >> --- a/drivers/acpi/platform_profile.c
> >> +++ b/drivers/acpi/platform_profile.c
> >> @@ -73,8 +73,20 @@ static int _store_class_profile(struct device *dev, void *data)
> >>
> >> lockdep_assert_held(&profile_lock);
> >> handler = to_pprof_handler(dev);
> >> - if (!test_bit(*bit, handler->choices))
> >> - return -EOPNOTSUPP;
> >> + if (!test_bit(*bit, handler->choices)) {
> >> + switch (*bit) {
> >> + case PLATFORM_PROFILE_QUIET:
> >> + *bit = PLATFORM_PROFILE_LOW_POWER;
> >> + break;
> >> + case PLATFORM_PROFILE_LOW_POWER:
> >> + *bit = PLATFORM_PROFILE_QUIET;
> >> + break;
> >> + default:
> >> + return -EOPNOTSUPP;
> >> + }
> >> + if (!test_bit(*bit, handler->choices))
> >> + return -EOPNOTSUPP;
> >> + }
> >>
> >> return handler->ops->profile_set(dev, *bit);
> >> }
> >> @@ -252,8 +264,16 @@ static int _aggregate_choices(struct device *dev, void *data)
> >> handler = to_pprof_handler(dev);
> >> if (test_bit(PLATFORM_PROFILE_LAST, aggregate))
> >> bitmap_copy(aggregate, handler->choices, PLATFORM_PROFILE_LAST);
> >> - else
> >> + else {
> >> + /* treat quiet and low power the same for aggregation purposes */
> >> + if (test_bit(PLATFORM_PROFILE_QUIET, handler->choices) &&
> >> + test_bit(PLATFORM_PROFILE_LOW_POWER, aggregate))
> >> + set_bit(PLATFORM_PROFILE_QUIET, aggregate);
> >> + else if (test_bit(PLATFORM_PROFILE_LOW_POWER, handler->choices) &&
> >> + test_bit(PLATFORM_PROFILE_QUIET, aggregate))
> >> + set_bit(PLATFORM_PROFILE_LOW_POWER, aggregate);
> >> bitmap_and(aggregate, handler->choices, aggregate, PLATFORM_PROFILE_LAST);
> >> + }
> >
> > So you end up showing both? If that's the case, isn't it equivalent to
> > just make amd-pmf show both quiet and low-power?
> >
> > I guess it is not ideal for framework devices. But if asus devices end
> > up showing both, then it should be ok for framework devices to show
> > both.
> >
> > I like the behavior of the V1 personally.
>
> No; this doesn't cause it to show both. It only causes one to show up.
> I confirmed it with a contrived situation on my laptop that forced
> multiple profile handlers that supported a mix.
If you can somehow force it to show the same option every time with a
tie breaker against amd-pmf it should be good enough. Still does not
solve balanced-power so unlike V1 it is not a permanent fix. Hidden
options was a nice tiebreaker imo.
>
> # cat /sys/firmware/acpi/platform_profile*
> low-power
> low-power balanced performance
>
> # cat /sys/class/platform-profile/platform-profile-*/profile
> quiet
> quiet
> quiet
> quiet
> quiet
> quiet
> quiet
> quiet
> quiet
> quiet
> quiet
> quiet
> quiet
> quiet
> quiet
> quiet
> quiet
> quiet
> quiet
> quiet
> quiet
> quiet
> quiet
> quiet
> low-power
>
> >
> >> return 0;
> >> }
> >> @@ -305,6 +325,13 @@ static int _aggregate_profiles(struct device *dev, void *data)
> >> if (err)
> >> return err;
> >>
> >> + /* treat low-power and quiet as the same */
> >> + if ((*profile == PLATFORM_PROFILE_LOW_POWER &&
> >> + val == PLATFORM_PROFILE_QUIET) ||
> >> + (*profile == PLATFORM_PROFILE_QUIET &&
> >> + val == PLATFORM_PROFILE_LOW_POWER))
> >> + *profile = val;
> >> +
> >> if (*profile != PLATFORM_PROFILE_LAST && *profile != val)
> >> *profile = PLATFORM_PROFILE_CUSTOM;
> >> else
> >> @@ -531,6 +558,11 @@ struct device *platform_profile_register(struct device *dev, const char *name,
> >> dev_err(dev, "Failed to register platform_profile class device with empty choices\n");
> >> return ERR_PTR(-EINVAL);
> >> }
> >> + if (test_bit(PLATFORM_PROFILE_QUIET, pprof->choices) &&
> >> + test_bit(PLATFORM_PROFILE_LOW_POWER, pprof->choices)) {
> >> + dev_err(dev, "Failed to register platform_profile class device with both quiet and low-power\n");
> >> + return ERR_PTR(-EINVAL);
> >> + }
> >
> > Can you avoid failing here? It caused a lot of issues in the past (the
> > WMI driver bails). a dev_err should be enough. Since you do not fail
> > maybe it can be increased to dev_crit.
> >
> > There is at least one driver that implements both currently, and a fix
> > would have to precede this patch.
>
> Oh, acer-wmi? Kurt; can you please comment? Are both simultaneous?
I do not have access to my kernel tree but when looking at it I
remember an if block that did a set_bit on both for certain laptops in
one of the drivers. Unsure if it was acer. But it was not ambiguous.
> >
> >>
> >> guard(mutex)(&profile_lock);
> >>
> >> --
> >> 2.43.0
> >>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/1] ACPI: platform_profile: Treat quiet and low power the same
2025-03-04 12:49 ` Mario Limonciello
2025-03-04 13:06 ` Antheas Kapenekakis
@ 2025-03-04 13:26 ` Kurt Borja
2025-03-04 13:32 ` Antheas Kapenekakis
2025-03-04 14:08 ` Rafael J. Wysocki
2 siblings, 1 reply; 15+ messages in thread
From: Kurt Borja @ 2025-03-04 13:26 UTC (permalink / raw)
To: Mario Limonciello, Antheas Kapenekakis
Cc: Shyam Sundar S K, Rafael J . Wysocki, Hans de Goede,
Ilpo Järvinen, Luke D . Jones, Mark Pearson,
open list:AMD PMF DRIVER, open list, open list:ACPI,
Derek J . Clark, me, Denis Benato, Mario Limonciello
Hi all,
On Tue Mar 4, 2025 at 7:49 AM -05, Mario Limonciello wrote:
>
>
> On 3/4/25 02:38, Antheas Kapenekakis wrote:
>> On Tue, 4 Mar 2025 at 07:48, Mario Limonciello <superm1@kernel.org> wrote:
>>>
>>> From: Mario Limonciello <mario.limonciello@amd.com>
>>>
>>> When two drivers don't support all the same profiles the legacy interface
>>> only exports the common profiles.
>>>
>>> This causes problems for cases where one driver uses low-power but another
>>> uses quiet because the result is that neither is exported to sysfs.
>>>
>>> If one platform profile handler supports quiet and the other
>>> supports low power treat them as the same for the purpose of
>>> the sysfs interface.
>>>
>>> Fixes: 688834743d67 ("ACPI: platform_profile: Allow multiple handlers")
>>> Reported-by: Antheas Kapenekakis <lkml@antheas.dev>
>>> Closes: https://lore.kernel.org/platform-driver-x86/e64b771e-3255-42ad-9257-5b8fc6c24ac9@gmx.de/T/#mc068042dd29df36c16c8af92664860fc4763974b
>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>> ---
>>> drivers/acpi/platform_profile.c | 38 ++++++++++++++++++++++++++++++---
>>> 1 file changed, 35 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
>>> index 2ad53cc6aae53..d9a7cc5891734 100644
>>> --- a/drivers/acpi/platform_profile.c
>>> +++ b/drivers/acpi/platform_profile.c
>>> @@ -73,8 +73,20 @@ static int _store_class_profile(struct device *dev, void *data)
>>>
>>> lockdep_assert_held(&profile_lock);
>>> handler = to_pprof_handler(dev);
>>> - if (!test_bit(*bit, handler->choices))
>>> - return -EOPNOTSUPP;
>>> + if (!test_bit(*bit, handler->choices)) {
>>> + switch (*bit) {
>>> + case PLATFORM_PROFILE_QUIET:
>>> + *bit = PLATFORM_PROFILE_LOW_POWER;
>>> + break;
>>> + case PLATFORM_PROFILE_LOW_POWER:
>>> + *bit = PLATFORM_PROFILE_QUIET;
>>> + break;
>>> + default:
>>> + return -EOPNOTSUPP;
>>> + }
>>> + if (!test_bit(*bit, handler->choices))
>>> + return -EOPNOTSUPP;
>>> + }
>>>
>>> return handler->ops->profile_set(dev, *bit);
>>> }
>>> @@ -252,8 +264,16 @@ static int _aggregate_choices(struct device *dev, void *data)
>>> handler = to_pprof_handler(dev);
>>> if (test_bit(PLATFORM_PROFILE_LAST, aggregate))
>>> bitmap_copy(aggregate, handler->choices, PLATFORM_PROFILE_LAST);
>>> - else
>>> + else {
>>> + /* treat quiet and low power the same for aggregation purposes */
>>> + if (test_bit(PLATFORM_PROFILE_QUIET, handler->choices) &&
>>> + test_bit(PLATFORM_PROFILE_LOW_POWER, aggregate))
>>> + set_bit(PLATFORM_PROFILE_QUIET, aggregate);
>>> + else if (test_bit(PLATFORM_PROFILE_LOW_POWER, handler->choices) &&
>>> + test_bit(PLATFORM_PROFILE_QUIET, aggregate))
>>> + set_bit(PLATFORM_PROFILE_LOW_POWER, aggregate);
>>> bitmap_and(aggregate, handler->choices, aggregate, PLATFORM_PROFILE_LAST);
>>> + }
>>
>> So you end up showing both? If that's the case, isn't it equivalent to
>> just make amd-pmf show both quiet and low-power?
>>
>> I guess it is not ideal for framework devices. But if asus devices end
>> up showing both, then it should be ok for framework devices to show
>> both.
>>
>> I like the behavior of the V1 personally.
>
> No; this doesn't cause it to show both. It only causes one to show up.
> I confirmed it with a contrived situation on my laptop that forced
> multiple profile handlers that supported a mix.
>
>
> # cat /sys/firmware/acpi/platform_profile*
> low-power
> low-power balanced performance
>
> # cat /sys/class/platform-profile/platform-profile-*/profile
> quiet
> quiet
> quiet
> quiet
> quiet
> quiet
> quiet
> quiet
> quiet
> quiet
> quiet
> quiet
> quiet
> quiet
> quiet
> quiet
> quiet
> quiet
> quiet
> quiet
> quiet
> quiet
> quiet
> quiet
> low-power
>
>>
>>> return 0;
>>> }
>>> @@ -305,6 +325,13 @@ static int _aggregate_profiles(struct device *dev, void *data)
>>> if (err)
>>> return err;
>>>
>>> + /* treat low-power and quiet as the same */
>>> + if ((*profile == PLATFORM_PROFILE_LOW_POWER &&
>>> + val == PLATFORM_PROFILE_QUIET) ||
>>> + (*profile == PLATFORM_PROFILE_QUIET &&
>>> + val == PLATFORM_PROFILE_LOW_POWER))
>>> + *profile = val;
>>> +
>>> if (*profile != PLATFORM_PROFILE_LAST && *profile != val)
>>> *profile = PLATFORM_PROFILE_CUSTOM;
>>> else
>>> @@ -531,6 +558,11 @@ struct device *platform_profile_register(struct device *dev, const char *name,
>>> dev_err(dev, "Failed to register platform_profile class device with empty choices\n");
>>> return ERR_PTR(-EINVAL);
>>> }
>>> + if (test_bit(PLATFORM_PROFILE_QUIET, pprof->choices) &&
>>> + test_bit(PLATFORM_PROFILE_LOW_POWER, pprof->choices)) {
>>> + dev_err(dev, "Failed to register platform_profile class device with both quiet and low-power\n");
>>> + return ERR_PTR(-EINVAL);
>>> + }
>>
>> Can you avoid failing here? It caused a lot of issues in the past (the
>> WMI driver bails). a dev_err should be enough. Since you do not fail
>> maybe it can be increased to dev_crit.
>>
>> There is at least one driver that implements both currently, and a fix
>> would have to precede this patch.
>
> Oh, acer-wmi? Kurt; can you please comment? Are both simultaneous?
There are a few laptops supported by alienware-wmi that definitely have
both (including mine). The acer-wmi and the samsung-galaxybook drivers
also probe for available choices dynamically, so some of those devices
may be affected by this too.
So yes, we shouldn't fail registration here.
Anyway, I like this approach more than v1. What do you think about
constraining this fix to the legacy interface?
--
~ Kurt
>
>>
>>>
>>> guard(mutex)(&profile_lock);
>>>
>>> --
>>> 2.43.0
>>>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/1] ACPI: platform_profile: Treat quiet and low power the same
2025-03-04 13:26 ` Kurt Borja
@ 2025-03-04 13:32 ` Antheas Kapenekakis
2025-03-04 13:53 ` Derek J. Clark
2025-03-04 13:53 ` Kurt Borja
0 siblings, 2 replies; 15+ messages in thread
From: Antheas Kapenekakis @ 2025-03-04 13:32 UTC (permalink / raw)
To: Kurt Borja
Cc: Mario Limonciello, Shyam Sundar S K, Rafael J . Wysocki,
Hans de Goede, Ilpo Järvinen, Luke D . Jones, Mark Pearson,
open list:AMD PMF DRIVER, open list, open list:ACPI,
Derek J . Clark, me, Denis Benato, Mario Limonciello
On Tue, 4 Mar 2025 at 14:28, Kurt Borja <kuurtb@gmail.com> wrote:
>
> Hi all,
>
> On Tue Mar 4, 2025 at 7:49 AM -05, Mario Limonciello wrote:
> >
> >
> > On 3/4/25 02:38, Antheas Kapenekakis wrote:
> >> On Tue, 4 Mar 2025 at 07:48, Mario Limonciello <superm1@kernel.org> wrote:
> >>>
> >>> From: Mario Limonciello <mario.limonciello@amd.com>
> >>>
> >>> When two drivers don't support all the same profiles the legacy interface
> >>> only exports the common profiles.
> >>>
> >>> This causes problems for cases where one driver uses low-power but another
> >>> uses quiet because the result is that neither is exported to sysfs.
> >>>
> >>> If one platform profile handler supports quiet and the other
> >>> supports low power treat them as the same for the purpose of
> >>> the sysfs interface.
> >>>
> >>> Fixes: 688834743d67 ("ACPI: platform_profile: Allow multiple handlers")
> >>> Reported-by: Antheas Kapenekakis <lkml@antheas.dev>
> >>> Closes: https://lore.kernel.org/platform-driver-x86/e64b771e-3255-42ad-9257-5b8fc6c24ac9@gmx.de/T/#mc068042dd29df36c16c8af92664860fc4763974b
> >>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> >>> ---
> >>> drivers/acpi/platform_profile.c | 38 ++++++++++++++++++++++++++++++---
> >>> 1 file changed, 35 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
> >>> index 2ad53cc6aae53..d9a7cc5891734 100644
> >>> --- a/drivers/acpi/platform_profile.c
> >>> +++ b/drivers/acpi/platform_profile.c
> >>> @@ -73,8 +73,20 @@ static int _store_class_profile(struct device *dev, void *data)
> >>>
> >>> lockdep_assert_held(&profile_lock);
> >>> handler = to_pprof_handler(dev);
> >>> - if (!test_bit(*bit, handler->choices))
> >>> - return -EOPNOTSUPP;
> >>> + if (!test_bit(*bit, handler->choices)) {
> >>> + switch (*bit) {
> >>> + case PLATFORM_PROFILE_QUIET:
> >>> + *bit = PLATFORM_PROFILE_LOW_POWER;
> >>> + break;
> >>> + case PLATFORM_PROFILE_LOW_POWER:
> >>> + *bit = PLATFORM_PROFILE_QUIET;
> >>> + break;
> >>> + default:
> >>> + return -EOPNOTSUPP;
> >>> + }
> >>> + if (!test_bit(*bit, handler->choices))
> >>> + return -EOPNOTSUPP;
> >>> + }
> >>>
> >>> return handler->ops->profile_set(dev, *bit);
> >>> }
> >>> @@ -252,8 +264,16 @@ static int _aggregate_choices(struct device *dev, void *data)
> >>> handler = to_pprof_handler(dev);
> >>> if (test_bit(PLATFORM_PROFILE_LAST, aggregate))
> >>> bitmap_copy(aggregate, handler->choices, PLATFORM_PROFILE_LAST);
> >>> - else
> >>> + else {
> >>> + /* treat quiet and low power the same for aggregation purposes */
> >>> + if (test_bit(PLATFORM_PROFILE_QUIET, handler->choices) &&
> >>> + test_bit(PLATFORM_PROFILE_LOW_POWER, aggregate))
> >>> + set_bit(PLATFORM_PROFILE_QUIET, aggregate);
> >>> + else if (test_bit(PLATFORM_PROFILE_LOW_POWER, handler->choices) &&
> >>> + test_bit(PLATFORM_PROFILE_QUIET, aggregate))
> >>> + set_bit(PLATFORM_PROFILE_LOW_POWER, aggregate);
> >>> bitmap_and(aggregate, handler->choices, aggregate, PLATFORM_PROFILE_LAST);
> >>> + }
> >>
> >> So you end up showing both? If that's the case, isn't it equivalent to
> >> just make amd-pmf show both quiet and low-power?
> >>
> >> I guess it is not ideal for framework devices. But if asus devices end
> >> up showing both, then it should be ok for framework devices to show
> >> both.
> >>
> >> I like the behavior of the V1 personally.
> >
> > No; this doesn't cause it to show both. It only causes one to show up.
> > I confirmed it with a contrived situation on my laptop that forced
> > multiple profile handlers that supported a mix.
> >
> >
> > # cat /sys/firmware/acpi/platform_profile*
> > low-power
> > low-power balanced performance
> >
> > # cat /sys/class/platform-profile/platform-profile-*/profile
> > quiet
> > quiet
> > quiet
> > quiet
> > quiet
> > quiet
> > quiet
> > quiet
> > quiet
> > quiet
> > quiet
> > quiet
> > quiet
> > quiet
> > quiet
> > quiet
> > quiet
> > quiet
> > quiet
> > quiet
> > quiet
> > quiet
> > quiet
> > quiet
> > low-power
> >
> >>
> >>> return 0;
> >>> }
> >>> @@ -305,6 +325,13 @@ static int _aggregate_profiles(struct device *dev, void *data)
> >>> if (err)
> >>> return err;
> >>>
> >>> + /* treat low-power and quiet as the same */
> >>> + if ((*profile == PLATFORM_PROFILE_LOW_POWER &&
> >>> + val == PLATFORM_PROFILE_QUIET) ||
> >>> + (*profile == PLATFORM_PROFILE_QUIET &&
> >>> + val == PLATFORM_PROFILE_LOW_POWER))
> >>> + *profile = val;
> >>> +
> >>> if (*profile != PLATFORM_PROFILE_LAST && *profile != val)
> >>> *profile = PLATFORM_PROFILE_CUSTOM;
> >>> else
> >>> @@ -531,6 +558,11 @@ struct device *platform_profile_register(struct device *dev, const char *name,
> >>> dev_err(dev, "Failed to register platform_profile class device with empty choices\n");
> >>> return ERR_PTR(-EINVAL);
> >>> }
> >>> + if (test_bit(PLATFORM_PROFILE_QUIET, pprof->choices) &&
> >>> + test_bit(PLATFORM_PROFILE_LOW_POWER, pprof->choices)) {
> >>> + dev_err(dev, "Failed to register platform_profile class device with both quiet and low-power\n");
> >>> + return ERR_PTR(-EINVAL);
> >>> + }
> >>
> >> Can you avoid failing here? It caused a lot of issues in the past (the
> >> WMI driver bails). a dev_err should be enough. Since you do not fail
> >> maybe it can be increased to dev_crit.
> >>
> >> There is at least one driver that implements both currently, and a fix
> >> would have to precede this patch.
> >
> > Oh, acer-wmi? Kurt; can you please comment? Are both simultaneous?
>
> There are a few laptops supported by alienware-wmi that definitely have
> both (including mine). The acer-wmi and the samsung-galaxybook drivers
> also probe for available choices dynamically, so some of those devices
> may be affected by this too.
>
> So yes, we shouldn't fail registration here.
>
> Anyway, I like this approach more than v1. What do you think about
> constraining this fix to the legacy interface?
AFAIK new interface is ok and should not be modified. None of the
previous solutions touched it (well, changing quiet to low-power did).
But I still expect the legacy interface to work the same way on 6.14.
What happens if there is one handler that does low-power and one that
does quiet? Is one choice preferred? And then are writes accepted in
both?
I cannot have the same device requiring low-power and quiet depending
on kernel version or boot. I do tdp controls per manufacturer.
> --
> ~ Kurt
>
> >
> >>
> >>>
> >>> guard(mutex)(&profile_lock);
> >>>
> >>> --
> >>> 2.43.0
> >>>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/1] ACPI: platform_profile: Treat quiet and low power the same
2025-03-04 13:32 ` Antheas Kapenekakis
@ 2025-03-04 13:53 ` Derek J. Clark
2025-03-04 13:53 ` Kurt Borja
1 sibling, 0 replies; 15+ messages in thread
From: Derek J. Clark @ 2025-03-04 13:53 UTC (permalink / raw)
To: Antheas Kapenekakis, Kurt Borja
Cc: Mario Limonciello, Shyam Sundar S K, Rafael J . Wysocki,
Hans de Goede, Ilpo Järvinen, Luke D . Jones, Mark Pearson,
open list:AMD PMF DRIVER, open list, open list:ACPI, me,
Denis Benato, Mario Limonciello
On March 4, 2025 5:32:50 AM PST, Antheas Kapenekakis <lkml@antheas.dev> wrote:
>On Tue, 4 Mar 2025 at 14:28, Kurt Borja <kuurtb@gmail.com> wrote:
>>
>> Hi all,
>>
>> On Tue Mar 4, 2025 at 7:49 AM -05, Mario Limonciello wrote:
>> >
>> >
>> > On 3/4/25 02:38, Antheas Kapenekakis wrote:
>> >> On Tue, 4 Mar 2025 at 07:48, Mario Limonciello <superm1@kernel.org> wrote:
>> >>>
>> >>> From: Mario Limonciello <mario.limonciello@amd.com>
>> >>>
>> >>> When two drivers don't support all the same profiles the legacy interface
>> >>> only exports the common profiles.
>> >>>
>> >>> This causes problems for cases where one driver uses low-power but another
>> >>> uses quiet because the result is that neither is exported to sysfs.
>> >>>
>> >>> If one platform profile handler supports quiet and the other
>> >>> supports low power treat them as the same for the purpose of
>> >>> the sysfs interface.
>> >>>
>> >>> Fixes: 688834743d67 ("ACPI: platform_profile: Allow multiple handlers")
>> >>> Reported-by: Antheas Kapenekakis <lkml@antheas.dev>
>> >>> Closes: https://lore.kernel.org/platform-driver-x86/e64b771e-3255-42ad-9257-5b8fc6c24ac9@gmx.de/T/#mc068042dd29df36c16c8af92664860fc4763974b
>> >>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> >>> ---
>> >>> drivers/acpi/platform_profile.c | 38 ++++++++++++++++++++++++++++++---
>> >>> 1 file changed, 35 insertions(+), 3 deletions(-)
>> >>>
>> >>> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
>> >>> index 2ad53cc6aae53..d9a7cc5891734 100644
>> >>> --- a/drivers/acpi/platform_profile.c
>> >>> +++ b/drivers/acpi/platform_profile.c
>> >>> @@ -73,8 +73,20 @@ static int _store_class_profile(struct device *dev, void *data)
>> >>>
>> >>> lockdep_assert_held(&profile_lock);
>> >>> handler = to_pprof_handler(dev);
>> >>> - if (!test_bit(*bit, handler->choices))
>> >>> - return -EOPNOTSUPP;
>> >>> + if (!test_bit(*bit, handler->choices)) {
>> >>> + switch (*bit) {
>> >>> + case PLATFORM_PROFILE_QUIET:
>> >>> + *bit = PLATFORM_PROFILE_LOW_POWER;
>> >>> + break;
>> >>> + case PLATFORM_PROFILE_LOW_POWER:
>> >>> + *bit = PLATFORM_PROFILE_QUIET;
>> >>> + break;
>> >>> + default:
>> >>> + return -EOPNOTSUPP;
>> >>> + }
>> >>> + if (!test_bit(*bit, handler->choices))
>> >>> + return -EOPNOTSUPP;
>> >>> + }
>> >>>
>> >>> return handler->ops->profile_set(dev, *bit);
>> >>> }
>> >>> @@ -252,8 +264,16 @@ static int _aggregate_choices(struct device *dev, void *data)
>> >>> handler = to_pprof_handler(dev);
>> >>> if (test_bit(PLATFORM_PROFILE_LAST, aggregate))
>> >>> bitmap_copy(aggregate, handler->choices, PLATFORM_PROFILE_LAST);
>> >>> - else
>> >>> + else {
>> >>> + /* treat quiet and low power the same for aggregation purposes */
>> >>> + if (test_bit(PLATFORM_PROFILE_QUIET, handler->choices) &&
>> >>> + test_bit(PLATFORM_PROFILE_LOW_POWER, aggregate))
>> >>> + set_bit(PLATFORM_PROFILE_QUIET, aggregate);
>> >>> + else if (test_bit(PLATFORM_PROFILE_LOW_POWER, handler->choices) &&
>> >>> + test_bit(PLATFORM_PROFILE_QUIET, aggregate))
>> >>> + set_bit(PLATFORM_PROFILE_LOW_POWER, aggregate);
>> >>> bitmap_and(aggregate, handler->choices, aggregate, PLATFORM_PROFILE_LAST);
>> >>> + }
>> >>
>> >> So you end up showing both? If that's the case, isn't it equivalent to
>> >> just make amd-pmf show both quiet and low-power?
>> >>
>> >> I guess it is not ideal for framework devices. But if asus devices end
>> >> up showing both, then it should be ok for framework devices to show
>> >> both.
>> >>
>> >> I like the behavior of the V1 personally.
>> >
>> > No; this doesn't cause it to show both. It only causes one to show up.
>> > I confirmed it with a contrived situation on my laptop that forced
>> > multiple profile handlers that supported a mix.
>> >
>> >
>> > # cat /sys/firmware/acpi/platform_profile*
>> > low-power
>> > low-power balanced performance
>> >
>> > # cat /sys/class/platform-profile/platform-profile-*/profile
>> > quiet
>> > quiet
>> > quiet
>> > quiet
>> > quiet
>> > quiet
>> > quiet
>> > quiet
>> > quiet
>> > quiet
>> > quiet
>> > quiet
>> > quiet
>> > quiet
>> > quiet
>> > quiet
>> > quiet
>> > quiet
>> > quiet
>> > quiet
>> > quiet
>> > quiet
>> > quiet
>> > quiet
>> > low-power
>> >
>> >>
>> >>> return 0;
>> >>> }
>> >>> @@ -305,6 +325,13 @@ static int _aggregate_profiles(struct device *dev, void *data)
>> >>> if (err)
>> >>> return err;
>> >>>
>> >>> + /* treat low-power and quiet as the same */
>> >>> + if ((*profile == PLATFORM_PROFILE_LOW_POWER &&
>> >>> + val == PLATFORM_PROFILE_QUIET) ||
>> >>> + (*profile == PLATFORM_PROFILE_QUIET &&
>> >>> + val == PLATFORM_PROFILE_LOW_POWER))
>> >>> + *profile = val;
>> >>> +
>> >>> if (*profile != PLATFORM_PROFILE_LAST && *profile != val)
>> >>> *profile = PLATFORM_PROFILE_CUSTOM;
>> >>> else
>> >>> @@ -531,6 +558,11 @@ struct device *platform_profile_register(struct device *dev, const char *name,
>> >>> dev_err(dev, "Failed to register platform_profile class device with empty choices\n");
>> >>> return ERR_PTR(-EINVAL);
>> >>> }
>> >>> + if (test_bit(PLATFORM_PROFILE_QUIET, pprof->choices) &&
>> >>> + test_bit(PLATFORM_PROFILE_LOW_POWER, pprof->choices)) {
>> >>> + dev_err(dev, "Failed to register platform_profile class device with both quiet and low-power\n");
>> >>> + return ERR_PTR(-EINVAL);
>> >>> + }
>> >>
>> >> Can you avoid failing here? It caused a lot of issues in the past (the
>> >> WMI driver bails). a dev_err should be enough. Since you do not fail
>> >> maybe it can be increased to dev_crit.
>> >>
>> >> There is at least one driver that implements both currently, and a fix
>> >> would have to precede this patch.
>> >
>> > Oh, acer-wmi? Kurt; can you please comment? Are both simultaneous?
>>
>> There are a few laptops supported by alienware-wmi that definitely have
>> both (including mine). The acer-wmi and the samsung-galaxybook drivers
>> also probe for available choices dynamically, so some of those devices
>> may be affected by this too.
>>
>> So yes, we shouldn't fail registration here.
>>
>> Anyway, I like this approach more than v1. What do you think about
>> constraining this fix to the legacy interface?
>
>AFAIK new interface is ok and should not be modified. None of the
>previous solutions touched it (well, changing quiet to low-power did).
>But I still expect the legacy interface to work the same way on 6.14.
>
>What happens if there is one handler that does low-power and one that
>does quiet? Is one choice preferred? And then are writes accepted in
>both?
>
>I cannot have the same device requiring low-power and quiet depending
>on kernel version or boot. I do tdp controls per manufacturer.
>
I agree that isn't ideal, but I see no reason why you can't index the _choices at startup to cover that in a generic way for all manufacturers. They present in performance order as text, specifically ensuring dynamic loading isn't difficult.
>> --
>> ~ Kurt
>>
>> >
>> >>
>> >>>
>> >>> guard(mutex)(&profile_lock);
>> >>>
>> >>> --
>> >>> 2.43.0
>> >>>
>>
- Derek
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/1] ACPI: platform_profile: Treat quiet and low power the same
2025-03-04 13:32 ` Antheas Kapenekakis
2025-03-04 13:53 ` Derek J. Clark
@ 2025-03-04 13:53 ` Kurt Borja
2025-03-04 14:02 ` Antheas Kapenekakis
1 sibling, 1 reply; 15+ messages in thread
From: Kurt Borja @ 2025-03-04 13:53 UTC (permalink / raw)
To: Antheas Kapenekakis
Cc: Mario Limonciello, Shyam Sundar S K, Rafael J . Wysocki,
Hans de Goede, Ilpo Järvinen, Luke D . Jones, Mark Pearson,
open list:AMD PMF DRIVER, open list, open list:ACPI,
Derek J . Clark, me, Denis Benato, Mario Limonciello
On Tue Mar 4, 2025 at 8:32 AM -05, Antheas Kapenekakis wrote:
> On Tue, 4 Mar 2025 at 14:28, Kurt Borja <kuurtb@gmail.com> wrote:
>>
>> Hi all,
>>
>> On Tue Mar 4, 2025 at 7:49 AM -05, Mario Limonciello wrote:
>> >
>> >
>> > On 3/4/25 02:38, Antheas Kapenekakis wrote:
>> >> On Tue, 4 Mar 2025 at 07:48, Mario Limonciello <superm1@kernel.org> wrote:
>> >>>
>> >>> From: Mario Limonciello <mario.limonciello@amd.com>
>> >>>
>> >>> When two drivers don't support all the same profiles the legacy interface
>> >>> only exports the common profiles.
>> >>>
>> >>> This causes problems for cases where one driver uses low-power but another
>> >>> uses quiet because the result is that neither is exported to sysfs.
>> >>>
>> >>> If one platform profile handler supports quiet and the other
>> >>> supports low power treat them as the same for the purpose of
>> >>> the sysfs interface.
>> >>>
>> >>> Fixes: 688834743d67 ("ACPI: platform_profile: Allow multiple handlers")
>> >>> Reported-by: Antheas Kapenekakis <lkml@antheas.dev>
>> >>> Closes: https://lore.kernel.org/platform-driver-x86/e64b771e-3255-42ad-9257-5b8fc6c24ac9@gmx.de/T/#mc068042dd29df36c16c8af92664860fc4763974b
>> >>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> >>> ---
>> >>> drivers/acpi/platform_profile.c | 38 ++++++++++++++++++++++++++++++---
>> >>> 1 file changed, 35 insertions(+), 3 deletions(-)
>> >>>
>> >>> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
>> >>> index 2ad53cc6aae53..d9a7cc5891734 100644
>> >>> --- a/drivers/acpi/platform_profile.c
>> >>> +++ b/drivers/acpi/platform_profile.c
>> >>> @@ -73,8 +73,20 @@ static int _store_class_profile(struct device *dev, void *data)
>> >>>
>> >>> lockdep_assert_held(&profile_lock);
>> >>> handler = to_pprof_handler(dev);
>> >>> - if (!test_bit(*bit, handler->choices))
>> >>> - return -EOPNOTSUPP;
>> >>> + if (!test_bit(*bit, handler->choices)) {
>> >>> + switch (*bit) {
>> >>> + case PLATFORM_PROFILE_QUIET:
>> >>> + *bit = PLATFORM_PROFILE_LOW_POWER;
>> >>> + break;
>> >>> + case PLATFORM_PROFILE_LOW_POWER:
>> >>> + *bit = PLATFORM_PROFILE_QUIET;
>> >>> + break;
>> >>> + default:
>> >>> + return -EOPNOTSUPP;
>> >>> + }
>> >>> + if (!test_bit(*bit, handler->choices))
>> >>> + return -EOPNOTSUPP;
>> >>> + }
>> >>>
>> >>> return handler->ops->profile_set(dev, *bit);
>> >>> }
>> >>> @@ -252,8 +264,16 @@ static int _aggregate_choices(struct device *dev, void *data)
>> >>> handler = to_pprof_handler(dev);
>> >>> if (test_bit(PLATFORM_PROFILE_LAST, aggregate))
>> >>> bitmap_copy(aggregate, handler->choices, PLATFORM_PROFILE_LAST);
>> >>> - else
>> >>> + else {
>> >>> + /* treat quiet and low power the same for aggregation purposes */
>> >>> + if (test_bit(PLATFORM_PROFILE_QUIET, handler->choices) &&
>> >>> + test_bit(PLATFORM_PROFILE_LOW_POWER, aggregate))
>> >>> + set_bit(PLATFORM_PROFILE_QUIET, aggregate);
>> >>> + else if (test_bit(PLATFORM_PROFILE_LOW_POWER, handler->choices) &&
>> >>> + test_bit(PLATFORM_PROFILE_QUIET, aggregate))
>> >>> + set_bit(PLATFORM_PROFILE_LOW_POWER, aggregate);
>> >>> bitmap_and(aggregate, handler->choices, aggregate, PLATFORM_PROFILE_LAST);
>> >>> + }
>> >>
>> >> So you end up showing both? If that's the case, isn't it equivalent to
>> >> just make amd-pmf show both quiet and low-power?
>> >>
>> >> I guess it is not ideal for framework devices. But if asus devices end
>> >> up showing both, then it should be ok for framework devices to show
>> >> both.
>> >>
>> >> I like the behavior of the V1 personally.
>> >
>> > No; this doesn't cause it to show both. It only causes one to show up.
>> > I confirmed it with a contrived situation on my laptop that forced
>> > multiple profile handlers that supported a mix.
>> >
>> >
>> > # cat /sys/firmware/acpi/platform_profile*
>> > low-power
>> > low-power balanced performance
>> >
>> > # cat /sys/class/platform-profile/platform-profile-*/profile
>> > quiet
>> > quiet
>> > quiet
>> > quiet
>> > quiet
>> > quiet
>> > quiet
>> > quiet
>> > quiet
>> > quiet
>> > quiet
>> > quiet
>> > quiet
>> > quiet
>> > quiet
>> > quiet
>> > quiet
>> > quiet
>> > quiet
>> > quiet
>> > quiet
>> > quiet
>> > quiet
>> > quiet
>> > low-power
>> >
>> >>
>> >>> return 0;
>> >>> }
>> >>> @@ -305,6 +325,13 @@ static int _aggregate_profiles(struct device *dev, void *data)
>> >>> if (err)
>> >>> return err;
>> >>>
>> >>> + /* treat low-power and quiet as the same */
>> >>> + if ((*profile == PLATFORM_PROFILE_LOW_POWER &&
>> >>> + val == PLATFORM_PROFILE_QUIET) ||
>> >>> + (*profile == PLATFORM_PROFILE_QUIET &&
>> >>> + val == PLATFORM_PROFILE_LOW_POWER))
>> >>> + *profile = val;
>> >>> +
>> >>> if (*profile != PLATFORM_PROFILE_LAST && *profile != val)
>> >>> *profile = PLATFORM_PROFILE_CUSTOM;
>> >>> else
>> >>> @@ -531,6 +558,11 @@ struct device *platform_profile_register(struct device *dev, const char *name,
>> >>> dev_err(dev, "Failed to register platform_profile class device with empty choices\n");
>> >>> return ERR_PTR(-EINVAL);
>> >>> }
>> >>> + if (test_bit(PLATFORM_PROFILE_QUIET, pprof->choices) &&
>> >>> + test_bit(PLATFORM_PROFILE_LOW_POWER, pprof->choices)) {
>> >>> + dev_err(dev, "Failed to register platform_profile class device with both quiet and low-power\n");
>> >>> + return ERR_PTR(-EINVAL);
>> >>> + }
>> >>
>> >> Can you avoid failing here? It caused a lot of issues in the past (the
>> >> WMI driver bails). a dev_err should be enough. Since you do not fail
>> >> maybe it can be increased to dev_crit.
>> >>
>> >> There is at least one driver that implements both currently, and a fix
>> >> would have to precede this patch.
>> >
>> > Oh, acer-wmi? Kurt; can you please comment? Are both simultaneous?
>>
>> There are a few laptops supported by alienware-wmi that definitely have
>> both (including mine). The acer-wmi and the samsung-galaxybook drivers
>> also probe for available choices dynamically, so some of those devices
>> may be affected by this too.
>>
>> So yes, we shouldn't fail registration here.
>>
>> Anyway, I like this approach more than v1. What do you think about
>> constraining this fix to the legacy interface?
>
> AFAIK new interface is ok and should not be modified. None of the
> previous solutions touched it (well, changing quiet to low-power did).
> But I still expect the legacy interface to work the same way on 6.14.
This patch also permanently alias quiet and low-power for the new
interface, if either one is not available.
>
> What happens if there is one handler that does low-power and one that
> does quiet? Is one choice preferred? And then are writes accepted in
> both?
>
> I cannot have the same device requiring low-power and quiet depending
> on kernel version or boot. I do tdp controls per manufacturer.
I'm not sure what you mean here.
--
~ Kurt
>
>> --
>> ~ Kurt
>>
>> >
>> >>
>> >>>
>> >>> guard(mutex)(&profile_lock);
>> >>>
>> >>> --
>> >>> 2.43.0
>> >>>
>>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/1] ACPI: platform_profile: Treat quiet and low power the same
2025-03-04 13:53 ` Kurt Borja
@ 2025-03-04 14:02 ` Antheas Kapenekakis
0 siblings, 0 replies; 15+ messages in thread
From: Antheas Kapenekakis @ 2025-03-04 14:02 UTC (permalink / raw)
To: Kurt Borja
Cc: Mario Limonciello, Shyam Sundar S K, Rafael J . Wysocki,
Hans de Goede, Ilpo Järvinen, Luke D . Jones, Mark Pearson,
open list:AMD PMF DRIVER, open list, open list:ACPI,
Derek J . Clark, me, Denis Benato, Mario Limonciello
On Tue, 4 Mar 2025 at 14:55, Kurt Borja <kuurtb@gmail.com> wrote:
>
> On Tue Mar 4, 2025 at 8:32 AM -05, Antheas Kapenekakis wrote:
> > On Tue, 4 Mar 2025 at 14:28, Kurt Borja <kuurtb@gmail.com> wrote:
> >>
> >> Hi all,
> >>
> >> On Tue Mar 4, 2025 at 7:49 AM -05, Mario Limonciello wrote:
> >> >
> >> >
> >> > On 3/4/25 02:38, Antheas Kapenekakis wrote:
> >> >> On Tue, 4 Mar 2025 at 07:48, Mario Limonciello <superm1@kernel.org> wrote:
> >> >>>
> >> >>> From: Mario Limonciello <mario.limonciello@amd.com>
> >> >>>
> >> >>> When two drivers don't support all the same profiles the legacy interface
> >> >>> only exports the common profiles.
> >> >>>
> >> >>> This causes problems for cases where one driver uses low-power but another
> >> >>> uses quiet because the result is that neither is exported to sysfs.
> >> >>>
> >> >>> If one platform profile handler supports quiet and the other
> >> >>> supports low power treat them as the same for the purpose of
> >> >>> the sysfs interface.
> >> >>>
> >> >>> Fixes: 688834743d67 ("ACPI: platform_profile: Allow multiple handlers")
> >> >>> Reported-by: Antheas Kapenekakis <lkml@antheas.dev>
> >> >>> Closes: https://lore.kernel.org/platform-driver-x86/e64b771e-3255-42ad-9257-5b8fc6c24ac9@gmx.de/T/#mc068042dd29df36c16c8af92664860fc4763974b
> >> >>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> >> >>> ---
> >> >>> drivers/acpi/platform_profile.c | 38 ++++++++++++++++++++++++++++++---
> >> >>> 1 file changed, 35 insertions(+), 3 deletions(-)
> >> >>>
> >> >>> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
> >> >>> index 2ad53cc6aae53..d9a7cc5891734 100644
> >> >>> --- a/drivers/acpi/platform_profile.c
> >> >>> +++ b/drivers/acpi/platform_profile.c
> >> >>> @@ -73,8 +73,20 @@ static int _store_class_profile(struct device *dev, void *data)
> >> >>>
> >> >>> lockdep_assert_held(&profile_lock);
> >> >>> handler = to_pprof_handler(dev);
> >> >>> - if (!test_bit(*bit, handler->choices))
> >> >>> - return -EOPNOTSUPP;
> >> >>> + if (!test_bit(*bit, handler->choices)) {
> >> >>> + switch (*bit) {
> >> >>> + case PLATFORM_PROFILE_QUIET:
> >> >>> + *bit = PLATFORM_PROFILE_LOW_POWER;
> >> >>> + break;
> >> >>> + case PLATFORM_PROFILE_LOW_POWER:
> >> >>> + *bit = PLATFORM_PROFILE_QUIET;
> >> >>> + break;
> >> >>> + default:
> >> >>> + return -EOPNOTSUPP;
> >> >>> + }
> >> >>> + if (!test_bit(*bit, handler->choices))
> >> >>> + return -EOPNOTSUPP;
> >> >>> + }
> >> >>>
> >> >>> return handler->ops->profile_set(dev, *bit);
> >> >>> }
> >> >>> @@ -252,8 +264,16 @@ static int _aggregate_choices(struct device *dev, void *data)
> >> >>> handler = to_pprof_handler(dev);
> >> >>> if (test_bit(PLATFORM_PROFILE_LAST, aggregate))
> >> >>> bitmap_copy(aggregate, handler->choices, PLATFORM_PROFILE_LAST);
> >> >>> - else
> >> >>> + else {
> >> >>> + /* treat quiet and low power the same for aggregation purposes */
> >> >>> + if (test_bit(PLATFORM_PROFILE_QUIET, handler->choices) &&
> >> >>> + test_bit(PLATFORM_PROFILE_LOW_POWER, aggregate))
> >> >>> + set_bit(PLATFORM_PROFILE_QUIET, aggregate);
> >> >>> + else if (test_bit(PLATFORM_PROFILE_LOW_POWER, handler->choices) &&
> >> >>> + test_bit(PLATFORM_PROFILE_QUIET, aggregate))
> >> >>> + set_bit(PLATFORM_PROFILE_LOW_POWER, aggregate);
> >> >>> bitmap_and(aggregate, handler->choices, aggregate, PLATFORM_PROFILE_LAST);
> >> >>> + }
> >> >>
> >> >> So you end up showing both? If that's the case, isn't it equivalent to
> >> >> just make amd-pmf show both quiet and low-power?
> >> >>
> >> >> I guess it is not ideal for framework devices. But if asus devices end
> >> >> up showing both, then it should be ok for framework devices to show
> >> >> both.
> >> >>
> >> >> I like the behavior of the V1 personally.
> >> >
> >> > No; this doesn't cause it to show both. It only causes one to show up.
> >> > I confirmed it with a contrived situation on my laptop that forced
> >> > multiple profile handlers that supported a mix.
> >> >
> >> >
> >> > # cat /sys/firmware/acpi/platform_profile*
> >> > low-power
> >> > low-power balanced performance
> >> >
> >> > # cat /sys/class/platform-profile/platform-profile-*/profile
> >> > quiet
> >> > quiet
> >> > quiet
> >> > quiet
> >> > quiet
> >> > quiet
> >> > quiet
> >> > quiet
> >> > quiet
> >> > quiet
> >> > quiet
> >> > quiet
> >> > quiet
> >> > quiet
> >> > quiet
> >> > quiet
> >> > quiet
> >> > quiet
> >> > quiet
> >> > quiet
> >> > quiet
> >> > quiet
> >> > quiet
> >> > quiet
> >> > low-power
> >> >
> >> >>
> >> >>> return 0;
> >> >>> }
> >> >>> @@ -305,6 +325,13 @@ static int _aggregate_profiles(struct device *dev, void *data)
> >> >>> if (err)
> >> >>> return err;
> >> >>>
> >> >>> + /* treat low-power and quiet as the same */
> >> >>> + if ((*profile == PLATFORM_PROFILE_LOW_POWER &&
> >> >>> + val == PLATFORM_PROFILE_QUIET) ||
> >> >>> + (*profile == PLATFORM_PROFILE_QUIET &&
> >> >>> + val == PLATFORM_PROFILE_LOW_POWER))
> >> >>> + *profile = val;
> >> >>> +
> >> >>> if (*profile != PLATFORM_PROFILE_LAST && *profile != val)
> >> >>> *profile = PLATFORM_PROFILE_CUSTOM;
> >> >>> else
> >> >>> @@ -531,6 +558,11 @@ struct device *platform_profile_register(struct device *dev, const char *name,
> >> >>> dev_err(dev, "Failed to register platform_profile class device with empty choices\n");
> >> >>> return ERR_PTR(-EINVAL);
> >> >>> }
> >> >>> + if (test_bit(PLATFORM_PROFILE_QUIET, pprof->choices) &&
> >> >>> + test_bit(PLATFORM_PROFILE_LOW_POWER, pprof->choices)) {
> >> >>> + dev_err(dev, "Failed to register platform_profile class device with both quiet and low-power\n");
> >> >>> + return ERR_PTR(-EINVAL);
> >> >>> + }
> >> >>
> >> >> Can you avoid failing here? It caused a lot of issues in the past (the
> >> >> WMI driver bails). a dev_err should be enough. Since you do not fail
> >> >> maybe it can be increased to dev_crit.
> >> >>
> >> >> There is at least one driver that implements both currently, and a fix
> >> >> would have to precede this patch.
> >> >
> >> > Oh, acer-wmi? Kurt; can you please comment? Are both simultaneous?
> >>
> >> There are a few laptops supported by alienware-wmi that definitely have
> >> both (including mine). The acer-wmi and the samsung-galaxybook drivers
> >> also probe for available choices dynamically, so some of those devices
> >> may be affected by this too.
> >>
> >> So yes, we shouldn't fail registration here.
> >>
> >> Anyway, I like this approach more than v1. What do you think about
> >> constraining this fix to the legacy interface?
> >
> > AFAIK new interface is ok and should not be modified. None of the
> > previous solutions touched it (well, changing quiet to low-power did).
> > But I still expect the legacy interface to work the same way on 6.14.
>
> This patch also permanently alias quiet and low-power for the new
> interface, if either one is not available.
Mmm, aliasing it as a hidden option is more of a side effect. I guess
if people start relying on that it might become problematic to revert
though.
> >
> > What happens if there is one handler that does low-power and one that
> > does quiet? Is one choice preferred? And then are writes accepted in
> > both?
> >
> > I cannot have the same device requiring low-power and quiet depending
> > on kernel version or boot. I do tdp controls per manufacturer.
>
> I'm not sure what you mean here.
You have an Asus Z13, in 6.13 it reports low-power, in 6.14 it reports
quiet. This patch series fixes writing blindly to it I would say, not
so much reading from it. Although it is unclear who that would affect.
I think reading will become a bigger problem in the future, as
Legion/Thinkpad devices can change their platform profile via user
action, and I would expect ppd/tuned to respond to that. They do not
currently. By the point they do, they can use the modern ABI though,
and bind bidirectionality to the /name attribute of platform profiles.
> --
> ~ Kurt
>
> >
> >> --
> >> ~ Kurt
> >>
> >> >
> >> >>
> >> >>>
> >> >>> guard(mutex)(&profile_lock);
> >> >>>
> >> >>> --
> >> >>> 2.43.0
> >> >>>
> >>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/1] ACPI: platform_profile: Treat quiet and low power the same
2025-03-04 12:49 ` Mario Limonciello
2025-03-04 13:06 ` Antheas Kapenekakis
2025-03-04 13:26 ` Kurt Borja
@ 2025-03-04 14:08 ` Rafael J. Wysocki
2025-03-04 14:52 ` Mario Limonciello
2 siblings, 1 reply; 15+ messages in thread
From: Rafael J. Wysocki @ 2025-03-04 14:08 UTC (permalink / raw)
To: Mario Limonciello
Cc: Antheas Kapenekakis, Kurt Borja, Shyam Sundar S K,
Rafael J . Wysocki, Hans de Goede, Ilpo Järvinen,
Luke D . Jones, Mark Pearson, open list:AMD PMF DRIVER, open list,
open list:ACPI, Derek J . Clark, me, Denis Benato,
Mario Limonciello
On Tue, Mar 4, 2025 at 1:49 PM Mario Limonciello <superm1@kernel.org> wrote:
>
> On 3/4/25 02:38, Antheas Kapenekakis wrote:
> > On Tue, 4 Mar 2025 at 07:48, Mario Limonciello <superm1@kernel.org> wrote:
> >>
> >> From: Mario Limonciello <mario.limonciello@amd.com>
> >>
> >> When two drivers don't support all the same profiles the legacy interface
> >> only exports the common profiles.
> >>
> >> This causes problems for cases where one driver uses low-power but another
> >> uses quiet because the result is that neither is exported to sysfs.
> >>
> >> If one platform profile handler supports quiet and the other
> >> supports low power treat them as the same for the purpose of
> >> the sysfs interface.
> >>
> >> Fixes: 688834743d67 ("ACPI: platform_profile: Allow multiple handlers")
> >> Reported-by: Antheas Kapenekakis <lkml@antheas.dev>
> >> Closes: https://lore.kernel.org/platform-driver-x86/e64b771e-3255-42ad-9257-5b8fc6c24ac9@gmx.de/T/#mc068042dd29df36c16c8af92664860fc4763974b
> >> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> >> ---
> >> drivers/acpi/platform_profile.c | 38 ++++++++++++++++++++++++++++++---
> >> 1 file changed, 35 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
> >> index 2ad53cc6aae53..d9a7cc5891734 100644
> >> --- a/drivers/acpi/platform_profile.c
> >> +++ b/drivers/acpi/platform_profile.c
> >> @@ -73,8 +73,20 @@ static int _store_class_profile(struct device *dev, void *data)
> >>
> >> lockdep_assert_held(&profile_lock);
> >> handler = to_pprof_handler(dev);
> >> - if (!test_bit(*bit, handler->choices))
> >> - return -EOPNOTSUPP;
> >> + if (!test_bit(*bit, handler->choices)) {
> >> + switch (*bit) {
> >> + case PLATFORM_PROFILE_QUIET:
> >> + *bit = PLATFORM_PROFILE_LOW_POWER;
> >> + break;
> >> + case PLATFORM_PROFILE_LOW_POWER:
> >> + *bit = PLATFORM_PROFILE_QUIET;
> >> + break;
> >> + default:
> >> + return -EOPNOTSUPP;
> >> + }
> >> + if (!test_bit(*bit, handler->choices))
> >> + return -EOPNOTSUPP;
> >> + }
> >>
> >> return handler->ops->profile_set(dev, *bit);
> >> }
> >> @@ -252,8 +264,16 @@ static int _aggregate_choices(struct device *dev, void *data)
> >> handler = to_pprof_handler(dev);
> >> if (test_bit(PLATFORM_PROFILE_LAST, aggregate))
> >> bitmap_copy(aggregate, handler->choices, PLATFORM_PROFILE_LAST);
> >> - else
> >> + else {
> >> + /* treat quiet and low power the same for aggregation purposes */
> >> + if (test_bit(PLATFORM_PROFILE_QUIET, handler->choices) &&
> >> + test_bit(PLATFORM_PROFILE_LOW_POWER, aggregate))
> >> + set_bit(PLATFORM_PROFILE_QUIET, aggregate);
> >> + else if (test_bit(PLATFORM_PROFILE_LOW_POWER, handler->choices) &&
> >> + test_bit(PLATFORM_PROFILE_QUIET, aggregate))
> >> + set_bit(PLATFORM_PROFILE_LOW_POWER, aggregate);
> >> bitmap_and(aggregate, handler->choices, aggregate, PLATFORM_PROFILE_LAST);
> >> + }
> >
> > So you end up showing both? If that's the case, isn't it equivalent to
> > just make amd-pmf show both quiet and low-power?
> >
> > I guess it is not ideal for framework devices. But if asus devices end
> > up showing both, then it should be ok for framework devices to show
> > both.
> >
> > I like the behavior of the V1 personally.
>
> No; this doesn't cause it to show both. It only causes one to show up.
Which may not be the one that was shown before IIUC and that's not good.
What actually is the problem with the previous version?
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/1] ACPI: platform_profile: Treat quiet and low power the same
2025-03-04 14:08 ` Rafael J. Wysocki
@ 2025-03-04 14:52 ` Mario Limonciello
2025-03-04 14:58 ` Rafael J. Wysocki
0 siblings, 1 reply; 15+ messages in thread
From: Mario Limonciello @ 2025-03-04 14:52 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Antheas Kapenekakis, Kurt Borja, Shyam Sundar S K, Hans de Goede,
Ilpo Järvinen, Luke D . Jones, Mark Pearson,
open list:AMD PMF DRIVER, open list, open list:ACPI,
Derek J . Clark, me, Denis Benato, Mario Limonciello
On 3/4/2025 08:08, Rafael J. Wysocki wrote:
> On Tue, Mar 4, 2025 at 1:49 PM Mario Limonciello <superm1@kernel.org> wrote:
>>
>> On 3/4/25 02:38, Antheas Kapenekakis wrote:
>>> On Tue, 4 Mar 2025 at 07:48, Mario Limonciello <superm1@kernel.org> wrote:
>>>>
>>>> From: Mario Limonciello <mario.limonciello@amd.com>
>>>>
>>>> When two drivers don't support all the same profiles the legacy interface
>>>> only exports the common profiles.
>>>>
>>>> This causes problems for cases where one driver uses low-power but another
>>>> uses quiet because the result is that neither is exported to sysfs.
>>>>
>>>> If one platform profile handler supports quiet and the other
>>>> supports low power treat them as the same for the purpose of
>>>> the sysfs interface.
>>>>
>>>> Fixes: 688834743d67 ("ACPI: platform_profile: Allow multiple handlers")
>>>> Reported-by: Antheas Kapenekakis <lkml@antheas.dev>
>>>> Closes: https://lore.kernel.org/platform-driver-x86/e64b771e-3255-42ad-9257-5b8fc6c24ac9@gmx.de/T/#mc068042dd29df36c16c8af92664860fc4763974b
>>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>>> ---
>>>> drivers/acpi/platform_profile.c | 38 ++++++++++++++++++++++++++++++---
>>>> 1 file changed, 35 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
>>>> index 2ad53cc6aae53..d9a7cc5891734 100644
>>>> --- a/drivers/acpi/platform_profile.c
>>>> +++ b/drivers/acpi/platform_profile.c
>>>> @@ -73,8 +73,20 @@ static int _store_class_profile(struct device *dev, void *data)
>>>>
>>>> lockdep_assert_held(&profile_lock);
>>>> handler = to_pprof_handler(dev);
>>>> - if (!test_bit(*bit, handler->choices))
>>>> - return -EOPNOTSUPP;
>>>> + if (!test_bit(*bit, handler->choices)) {
>>>> + switch (*bit) {
>>>> + case PLATFORM_PROFILE_QUIET:
>>>> + *bit = PLATFORM_PROFILE_LOW_POWER;
>>>> + break;
>>>> + case PLATFORM_PROFILE_LOW_POWER:
>>>> + *bit = PLATFORM_PROFILE_QUIET;
>>>> + break;
>>>> + default:
>>>> + return -EOPNOTSUPP;
>>>> + }
>>>> + if (!test_bit(*bit, handler->choices))
>>>> + return -EOPNOTSUPP;
>>>> + }
>>>>
>>>> return handler->ops->profile_set(dev, *bit);
>>>> }
>>>> @@ -252,8 +264,16 @@ static int _aggregate_choices(struct device *dev, void *data)
>>>> handler = to_pprof_handler(dev);
>>>> if (test_bit(PLATFORM_PROFILE_LAST, aggregate))
>>>> bitmap_copy(aggregate, handler->choices, PLATFORM_PROFILE_LAST);
>>>> - else
>>>> + else {
>>>> + /* treat quiet and low power the same for aggregation purposes */
>>>> + if (test_bit(PLATFORM_PROFILE_QUIET, handler->choices) &&
>>>> + test_bit(PLATFORM_PROFILE_LOW_POWER, aggregate))
>>>> + set_bit(PLATFORM_PROFILE_QUIET, aggregate);
>>>> + else if (test_bit(PLATFORM_PROFILE_LOW_POWER, handler->choices) &&
>>>> + test_bit(PLATFORM_PROFILE_QUIET, aggregate))
>>>> + set_bit(PLATFORM_PROFILE_LOW_POWER, aggregate);
>>>> bitmap_and(aggregate, handler->choices, aggregate, PLATFORM_PROFILE_LAST);
>>>> + }
>>>
>>> So you end up showing both? If that's the case, isn't it equivalent to
>>> just make amd-pmf show both quiet and low-power?
>>>
>>> I guess it is not ideal for framework devices. But if asus devices end
>>> up showing both, then it should be ok for framework devices to show
>>> both.
>>>
>>> I like the behavior of the V1 personally.
>>
>> No; this doesn't cause it to show both. It only causes one to show up.
>
> Which may not be the one that was shown before IIUC and that's not good.
>
> What actually is the problem with the previous version?
Functionally? Nothing. This was to demonstrate the other way to do it
that I preferred and get feedback on it as an alternative.
If you and Ilpo are happy with v1 that's totally fine and we can go with
that.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/1] ACPI: platform_profile: Treat quiet and low power the same
2025-03-04 14:52 ` Mario Limonciello
@ 2025-03-04 14:58 ` Rafael J. Wysocki
2025-03-04 16:23 ` Ilpo Järvinen
0 siblings, 1 reply; 15+ messages in thread
From: Rafael J. Wysocki @ 2025-03-04 14:58 UTC (permalink / raw)
To: Mario Limonciello, Ilpo Järvinen
Cc: Antheas Kapenekakis, Kurt Borja, Shyam Sundar S K, Hans de Goede,
Luke D . Jones, Mark Pearson, open list:AMD PMF DRIVER, open list,
open list:ACPI, Derek J . Clark, me, Denis Benato,
Mario Limonciello
On Tue, Mar 4, 2025 at 3:52 PM Mario Limonciello <superm1@kernel.org> wrote:
>
> On 3/4/2025 08:08, Rafael J. Wysocki wrote:
> > On Tue, Mar 4, 2025 at 1:49 PM Mario Limonciello <superm1@kernel.org> wrote:
> >>
> >> On 3/4/25 02:38, Antheas Kapenekakis wrote:
> >>> On Tue, 4 Mar 2025 at 07:48, Mario Limonciello <superm1@kernel.org> wrote:
> >>>>
> >>>> From: Mario Limonciello <mario.limonciello@amd.com>
> >>>>
> >>>> When two drivers don't support all the same profiles the legacy interface
> >>>> only exports the common profiles.
> >>>>
> >>>> This causes problems for cases where one driver uses low-power but another
> >>>> uses quiet because the result is that neither is exported to sysfs.
> >>>>
> >>>> If one platform profile handler supports quiet and the other
> >>>> supports low power treat them as the same for the purpose of
> >>>> the sysfs interface.
> >>>>
> >>>> Fixes: 688834743d67 ("ACPI: platform_profile: Allow multiple handlers")
> >>>> Reported-by: Antheas Kapenekakis <lkml@antheas.dev>
> >>>> Closes: https://lore.kernel.org/platform-driver-x86/e64b771e-3255-42ad-9257-5b8fc6c24ac9@gmx.de/T/#mc068042dd29df36c16c8af92664860fc4763974b
> >>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> >>>> ---
> >>>> drivers/acpi/platform_profile.c | 38 ++++++++++++++++++++++++++++++---
> >>>> 1 file changed, 35 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
> >>>> index 2ad53cc6aae53..d9a7cc5891734 100644
> >>>> --- a/drivers/acpi/platform_profile.c
> >>>> +++ b/drivers/acpi/platform_profile.c
> >>>> @@ -73,8 +73,20 @@ static int _store_class_profile(struct device *dev, void *data)
> >>>>
> >>>> lockdep_assert_held(&profile_lock);
> >>>> handler = to_pprof_handler(dev);
> >>>> - if (!test_bit(*bit, handler->choices))
> >>>> - return -EOPNOTSUPP;
> >>>> + if (!test_bit(*bit, handler->choices)) {
> >>>> + switch (*bit) {
> >>>> + case PLATFORM_PROFILE_QUIET:
> >>>> + *bit = PLATFORM_PROFILE_LOW_POWER;
> >>>> + break;
> >>>> + case PLATFORM_PROFILE_LOW_POWER:
> >>>> + *bit = PLATFORM_PROFILE_QUIET;
> >>>> + break;
> >>>> + default:
> >>>> + return -EOPNOTSUPP;
> >>>> + }
> >>>> + if (!test_bit(*bit, handler->choices))
> >>>> + return -EOPNOTSUPP;
> >>>> + }
> >>>>
> >>>> return handler->ops->profile_set(dev, *bit);
> >>>> }
> >>>> @@ -252,8 +264,16 @@ static int _aggregate_choices(struct device *dev, void *data)
> >>>> handler = to_pprof_handler(dev);
> >>>> if (test_bit(PLATFORM_PROFILE_LAST, aggregate))
> >>>> bitmap_copy(aggregate, handler->choices, PLATFORM_PROFILE_LAST);
> >>>> - else
> >>>> + else {
> >>>> + /* treat quiet and low power the same for aggregation purposes */
> >>>> + if (test_bit(PLATFORM_PROFILE_QUIET, handler->choices) &&
> >>>> + test_bit(PLATFORM_PROFILE_LOW_POWER, aggregate))
> >>>> + set_bit(PLATFORM_PROFILE_QUIET, aggregate);
> >>>> + else if (test_bit(PLATFORM_PROFILE_LOW_POWER, handler->choices) &&
> >>>> + test_bit(PLATFORM_PROFILE_QUIET, aggregate))
> >>>> + set_bit(PLATFORM_PROFILE_LOW_POWER, aggregate);
> >>>> bitmap_and(aggregate, handler->choices, aggregate, PLATFORM_PROFILE_LAST);
> >>>> + }
> >>>
> >>> So you end up showing both? If that's the case, isn't it equivalent to
> >>> just make amd-pmf show both quiet and low-power?
> >>>
> >>> I guess it is not ideal for framework devices. But if asus devices end
> >>> up showing both, then it should be ok for framework devices to show
> >>> both.
> >>>
> >>> I like the behavior of the V1 personally.
> >>
> >> No; this doesn't cause it to show both. It only causes one to show up.
> >
> > Which may not be the one that was shown before IIUC and that's not good.
> >
> > What actually is the problem with the previous version?
>
> Functionally? Nothing. This was to demonstrate the other way to do it
> that I preferred and get feedback on it as an alternative.
>
> If you and Ilpo are happy with v1 that's totally fine and we can go with
> that.
I'd prefer to go for the v1 at this point because it fixes a
regression affecting user space that needs to be addressed before the
6.14 release (and there is not too much time left) and it has been
checked on the affected systems.
Ilpo, do you agree?
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/1] ACPI: platform_profile: Treat quiet and low power the same
2025-03-04 6:47 ` [PATCH v2 1/1] ACPI: platform_profile: Treat quiet and low power the same Mario Limonciello
2025-03-04 8:38 ` Antheas Kapenekakis
@ 2025-03-04 15:24 ` Derek J. Clark
1 sibling, 0 replies; 15+ messages in thread
From: Derek J. Clark @ 2025-03-04 15:24 UTC (permalink / raw)
To: Mario Limonciello, Shyam Sundar S K, Rafael J . Wysocki,
Hans de Goede, Ilpo Järvinen, Luke D . Jones, Mark Pearson
Cc: platform-driver-x86, linux-kernel, linux-acpi,
Antheas Kapenekakis, me, Denis Benato, Mario Limonciello
On March 3, 2025 10:47:45 PM PST, Mario Limonciello <superm1@kernel.org> wrote:
>From: Mario Limonciello <mario.limonciello@amd.com>
>
>When two drivers don't support all the same profiles the legacy interface
>only exports the common profiles.
>
>This causes problems for cases where one driver uses low-power but another
>uses quiet because the result is that neither is exported to sysfs.
>ion yet
>If one platform profile handler supports quiet and the other
>supports low power treat them as the same for the purpose of
>the sysfs interface.
Hi Mario,
I haven't tested this version yet but from an initial glance I do have some concerns. In v1 there was handling of balanaced_perfomance, and that isn't present here, which would affect my in progress driver. This also doesn't cover the cool -> low_power option (though I'm not sure where/if that is an actual concern in any drivers at the moment). I'm concerned that if we take the v2 approach that we'll eventually be aliasing a majority of the profiles, further adding ambiguity on what each one actually means.
In my driver balanced_perfomance is closer to amd_pmf's performance, if shown, whereas in others it might be closer to balanced. Since that is essentially implementation specific I currently am doubtful there is a clean universal approach to aliasing.
The real issue appears to me at that the enabled profiles need to be context aware. Because of that I think something closer to v1 and the hidden options method provides a better way to implement those aliases within any specific driver, allowing the maintainers to determine the "best alias" so to speak. If we put the control into the "primary" driver of how those aliases work and somehow provide context to amd_pmf of the "best match", we can then allow amd_pmf to present all options when more than one low end profile is valid, or only the matching ones if they are just aliased.
- Derek
>Fixes: 688834743d67 ("ACPI: platform_profile: Allow multiple handlers")
>Reported-by: Antheas Kapenekakis <lkml@antheas.dev>
>Closes: https://lore.kernel.org/platform-driver-x86/e64b771e-3255-42ad-9257-5b8fc6c24ac9@gmx.de/T/#mc068042dd29df36c16c8af92664860fc4763974b
>Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>---
> drivers/acpi/platform_profile.c | 38 ++++++++++++++++++++++++++++++---
> 1 file changed, 35 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
>index 2ad53cc6aae53..d9a7cc5891734 100644
>--- a/drivers/acpi/platform_profile.c
>+++ b/drivers/acpi/platform_profile.c
>@@ -73,8 +73,20 @@ static int _store_class_profile(struct device *dev, void *data)
>
> lockdep_assert_held(&profile_lock);
> handler = to_pprof_handler(dev);
>- if (!test_bit(*bit, handler->choices))
>- return -EOPNOTSUPP;
>+ if (!test_bit(*bit, handler->choices)) {
>+ switch (*bit) {
>+ case PLATFORM_PROFILE_QUIET:
>+ *bit = PLATFORM_PROFILE_LOW_POWER;
>+ break;
>+ case PLATFORM_PROFILE_LOW_POWER:
>+ *bit = PLATFORM_PROFILE_QUIET;
>+ break;
>+ default:
>+ return -EOPNOTSUPP;
>+ }
>+ if (!test_bit(*bit, handler->choices))
>+ return -EOPNOTSUPP;
>+ }
>
> return handler->ops->profile_set(dev, *bit);
> }
>@@ -252,8 +264,16 @@ static int _aggregate_choices(struct device *dev, void *data)
> handler = to_pprof_handler(dev);
> if (test_bit(PLATFORM_PROFILE_LAST, aggregate))
> bitmap_copy(aggregate, handler->choices, PLATFORM_PROFILE_LAST);
>- else
>+ else {
>+ /* treat quiet and low power the same for aggregation purposes */
>+ if (test_bit(PLATFORM_PROFILE_QUIET, handler->choices) &&
>+ test_bit(PLATFORM_PROFILE_LOW_POWER, aggregate))
>+ set_bit(PLATFORM_PROFILE_QUIET, aggregate);
>+ else if (test_bit(PLATFORM_PROFILE_LOW_POWER, handler->choices) &&
>+ test_bit(PLATFORM_PROFILE_QUIET, aggregate))
>+ set_bit(PLATFORM_PROFILE_LOW_POWER, aggregate);
> bitmap_and(aggregate, handler->choices, aggregate, PLATFORM_PROFILE_LAST);
>+ }
>
> return 0;
> }
>@@ -305,6 +325,13 @@ static int _aggregate_profiles(struct device *dev, void *data)
> if (err)
> return err;
>
>+ /* treat low-power and quiet as the same */
>+ if ((*profile == PLATFORM_PROFILE_LOW_POWER &&
>+ val == PLATFORM_PROFILE_QUIET) ||
>+ (*profile == PLATFORM_PROFILE_QUIET &&
>+ val == PLATFORM_PROFILE_LOW_POWER))
>+ *profile = val;
>+
> if (*profile != PLATFORM_PROFILE_LAST && *profile != val)
> *profile = PLATFORM_PROFILE_CUSTOM;
> else
>@@ -531,6 +558,11 @@ struct device *platform_profile_register(struct device *dev, const char *name,
> dev_err(dev, "Failed to register platform_profile class device with empty choices\n");
> return ERR_PTR(-EINVAL);
> }
>+ if (test_bit(PLATFORM_PROFILE_QUIET, pprof->choices) &&
>+ test_bit(PLATFORM_PROFILE_LOW_POWER, pprof->choices)) {
>+ dev_err(dev, "Failed to register platform_profile class device with both quiet and low-power\n");
>+ return ERR_PTR(-EINVAL);
>+ }
>
> guard(mutex)(&profile_lock);
>
- Derek
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/1] ACPI: platform_profile: Treat quiet and low power the same
2025-03-04 14:58 ` Rafael J. Wysocki
@ 2025-03-04 16:23 ` Ilpo Järvinen
0 siblings, 0 replies; 15+ messages in thread
From: Ilpo Järvinen @ 2025-03-04 16:23 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Mario Limonciello, Antheas Kapenekakis, Kurt Borja,
Shyam Sundar S K, Hans de Goede, Luke D . Jones, Mark Pearson,
open list:AMD PMF DRIVER, open list, open list:ACPI,
Derek J . Clark, me, Denis Benato, Mario Limonciello
[-- Attachment #1: Type: text/plain, Size: 5294 bytes --]
On Tue, 4 Mar 2025, Rafael J. Wysocki wrote:
> On Tue, Mar 4, 2025 at 3:52 PM Mario Limonciello <superm1@kernel.org> wrote:
> >
> > On 3/4/2025 08:08, Rafael J. Wysocki wrote:
> > > On Tue, Mar 4, 2025 at 1:49 PM Mario Limonciello <superm1@kernel.org> wrote:
> > >>
> > >> On 3/4/25 02:38, Antheas Kapenekakis wrote:
> > >>> On Tue, 4 Mar 2025 at 07:48, Mario Limonciello <superm1@kernel.org> wrote:
> > >>>>
> > >>>> From: Mario Limonciello <mario.limonciello@amd.com>
> > >>>>
> > >>>> When two drivers don't support all the same profiles the legacy interface
> > >>>> only exports the common profiles.
> > >>>>
> > >>>> This causes problems for cases where one driver uses low-power but another
> > >>>> uses quiet because the result is that neither is exported to sysfs.
> > >>>>
> > >>>> If one platform profile handler supports quiet and the other
> > >>>> supports low power treat them as the same for the purpose of
> > >>>> the sysfs interface.
> > >>>>
> > >>>> Fixes: 688834743d67 ("ACPI: platform_profile: Allow multiple handlers")
> > >>>> Reported-by: Antheas Kapenekakis <lkml@antheas.dev>
> > >>>> Closes: https://lore.kernel.org/platform-driver-x86/e64b771e-3255-42ad-9257-5b8fc6c24ac9@gmx.de/T/#mc068042dd29df36c16c8af92664860fc4763974b
> > >>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> > >>>> ---
> > >>>> drivers/acpi/platform_profile.c | 38 ++++++++++++++++++++++++++++++---
> > >>>> 1 file changed, 35 insertions(+), 3 deletions(-)
> > >>>>
> > >>>> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
> > >>>> index 2ad53cc6aae53..d9a7cc5891734 100644
> > >>>> --- a/drivers/acpi/platform_profile.c
> > >>>> +++ b/drivers/acpi/platform_profile.c
> > >>>> @@ -73,8 +73,20 @@ static int _store_class_profile(struct device *dev, void *data)
> > >>>>
> > >>>> lockdep_assert_held(&profile_lock);
> > >>>> handler = to_pprof_handler(dev);
> > >>>> - if (!test_bit(*bit, handler->choices))
> > >>>> - return -EOPNOTSUPP;
> > >>>> + if (!test_bit(*bit, handler->choices)) {
> > >>>> + switch (*bit) {
> > >>>> + case PLATFORM_PROFILE_QUIET:
> > >>>> + *bit = PLATFORM_PROFILE_LOW_POWER;
> > >>>> + break;
> > >>>> + case PLATFORM_PROFILE_LOW_POWER:
> > >>>> + *bit = PLATFORM_PROFILE_QUIET;
> > >>>> + break;
> > >>>> + default:
> > >>>> + return -EOPNOTSUPP;
> > >>>> + }
> > >>>> + if (!test_bit(*bit, handler->choices))
> > >>>> + return -EOPNOTSUPP;
> > >>>> + }
> > >>>>
> > >>>> return handler->ops->profile_set(dev, *bit);
> > >>>> }
> > >>>> @@ -252,8 +264,16 @@ static int _aggregate_choices(struct device *dev, void *data)
> > >>>> handler = to_pprof_handler(dev);
> > >>>> if (test_bit(PLATFORM_PROFILE_LAST, aggregate))
> > >>>> bitmap_copy(aggregate, handler->choices, PLATFORM_PROFILE_LAST);
> > >>>> - else
> > >>>> + else {
> > >>>> + /* treat quiet and low power the same for aggregation purposes */
> > >>>> + if (test_bit(PLATFORM_PROFILE_QUIET, handler->choices) &&
> > >>>> + test_bit(PLATFORM_PROFILE_LOW_POWER, aggregate))
> > >>>> + set_bit(PLATFORM_PROFILE_QUIET, aggregate);
> > >>>> + else if (test_bit(PLATFORM_PROFILE_LOW_POWER, handler->choices) &&
> > >>>> + test_bit(PLATFORM_PROFILE_QUIET, aggregate))
> > >>>> + set_bit(PLATFORM_PROFILE_LOW_POWER, aggregate);
> > >>>> bitmap_and(aggregate, handler->choices, aggregate, PLATFORM_PROFILE_LAST);
> > >>>> + }
> > >>>
> > >>> So you end up showing both? If that's the case, isn't it equivalent to
> > >>> just make amd-pmf show both quiet and low-power?
> > >>>
> > >>> I guess it is not ideal for framework devices. But if asus devices end
> > >>> up showing both, then it should be ok for framework devices to show
> > >>> both.
> > >>>
> > >>> I like the behavior of the V1 personally.
> > >>
> > >> No; this doesn't cause it to show both. It only causes one to show up.
> > >
> > > Which may not be the one that was shown before IIUC and that's not good.
> > >
> > > What actually is the problem with the previous version?
> >
> > Functionally? Nothing. This was to demonstrate the other way to do it
> > that I preferred and get feedback on it as an alternative.
> >
> > If you and Ilpo are happy with v1 that's totally fine and we can go with
> > that.
>
> I'd prefer to go for the v1 at this point because it fixes a
> regression affecting user space that needs to be addressed before the
> 6.14 release (and there is not too much time left) and it has been
> checked on the affected systems.
>
> Ilpo, do you agree?
>
Yes, I'm fine with that.
I would have acked those patches earlier but noticed they'd managed to in
the meantime come up yet another version of the fix so I waited some more.
I've added my ack there now.
--
i.
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2025-03-04 16:24 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-03-04 6:47 [PATCH v2 0/1] Add quiet/low power compat code Mario Limonciello
2025-03-04 6:47 ` [PATCH v2 1/1] ACPI: platform_profile: Treat quiet and low power the same Mario Limonciello
2025-03-04 8:38 ` Antheas Kapenekakis
2025-03-04 12:49 ` Mario Limonciello
2025-03-04 13:06 ` Antheas Kapenekakis
2025-03-04 13:26 ` Kurt Borja
2025-03-04 13:32 ` Antheas Kapenekakis
2025-03-04 13:53 ` Derek J. Clark
2025-03-04 13:53 ` Kurt Borja
2025-03-04 14:02 ` Antheas Kapenekakis
2025-03-04 14:08 ` Rafael J. Wysocki
2025-03-04 14:52 ` Mario Limonciello
2025-03-04 14:58 ` Rafael J. Wysocki
2025-03-04 16:23 ` Ilpo Järvinen
2025-03-04 15:24 ` Derek J. Clark
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox