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

[Mario Limonciello <superm1@kernel.org>]

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