[PATCH v2 0/1] Add quiet/low power compat code

[PATCH v2 0/1] Add quiet/low power compat code

[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

[PATCH v2 1/1] ACPI: platform_profile: Treat quiet and low power the same

[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

Re: [PATCH v2 1/1] ACPI: platform_profile: Treat quiet and low power the same

[Antheas Kapenekakis]

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
>

Re: [PATCH v2 1/1] ACPI: platform_profile: Treat quiet and low power the same

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

Re: [PATCH v2 1/1] ACPI: platform_profile: Treat quiet and low power the same

[Antheas Kapenekakis]

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

Re: [PATCH v2 1/1] ACPI: platform_profile: Treat quiet and low power the same

[Kurt Borja]

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

Re: [PATCH v2 1/1] ACPI: platform_profile: Treat quiet and low power the same

[Antheas Kapenekakis]

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

Re: [PATCH v2 1/1] ACPI: platform_profile: Treat quiet and low power the same

[Derek J. Clark]

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

Re: [PATCH v2 1/1] ACPI: platform_profile: Treat quiet and low power the same

[Kurt Borja]

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

Re: [PATCH v2 1/1] ACPI: platform_profile: Treat quiet and low power the same

[Antheas Kapenekakis]

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

Re: [PATCH v2 1/1] ACPI: platform_profile: Treat quiet and low power the same

[Rafael J. Wysocki]

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?

Re: [PATCH v2 1/1] ACPI: platform_profile: Treat quiet and low power the same

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

Re: [PATCH v2 1/1] ACPI: platform_profile: Treat quiet and low power the same

[Rafael J. Wysocki]

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?

Re: [PATCH v2 1/1] ACPI: platform_profile: Treat quiet and low power the same

[Ilpo Järvinen]

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

Re: [PATCH v2 1/1] ACPI: platform_profile: Treat quiet and low power the same

[Derek J. Clark]

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