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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <71b14dc3-77e1-4fd7-b576-821e3a41ba19@kernel.org>
Date: Tue, 4 Mar 2025 08:52:06 -0600
From: Mario Limonciello <superm1@...nel.org>
To: "Rafael J. Wysocki" <rafael@...nel.org>
Cc: Antheas Kapenekakis <lkml@...heas.dev>, Kurt Borja <kuurtb@...il.com>,
 Shyam Sundar S K <Shyam-sundar.S-k@....com>,
 Hans de Goede <hdegoede@...hat.com>,
 Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
 "Luke D . Jones" <luke@...nes.dev>, Mark Pearson
 <mpearson-lenovo@...ebb.ca>,
 "open list:AMD PMF DRIVER" <platform-driver-x86@...r.kernel.org>,
 open list <linux-kernel@...r.kernel.org>,
 "open list:ACPI" <linux-acpi@...r.kernel.org>,
 "Derek J . Clark" <derekjohn.clark@...il.com>, me@...egospodneti.ch,
 Denis Benato <benato.denis96@...il.com>,
 Mario Limonciello <mario.limonciello@....com>
Subject: Re: [PATCH v2 1/1] ACPI: platform_profile: Treat quiet and low power
 the same

On 3/4/2025 08:08, Rafael J. Wysocki wrote:
> On Tue, Mar 4, 2025 at 1:49 PM Mario Limonciello <superm1@...nel.org> wrote:
>>
>> On 3/4/25 02:38, Antheas Kapenekakis wrote:
>>> On Tue, 4 Mar 2025 at 07:48, Mario Limonciello <superm1@...nel.org> wrote:
>>>>
>>>> From: Mario Limonciello <mario.limonciello@....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@...heas.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@....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.