| 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
| ||
|
Message-ID: <23d6c735-e94f-4d43-87b0-ff119941fcac@kernel.org>
Date: Tue, 4 Mar 2025 06:49:00 -0600
From: Mario Limonciello <superm1@...nel.org>
To: Antheas Kapenekakis <lkml@...heas.dev>, Kurt Borja <kuurtb@...il.com>
Cc: Shyam Sundar S K <Shyam-sundar.S-k@....com>,
"Rafael J . Wysocki" <rafael@...nel.org>, 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/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.
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
>>
Powered by blists - more mailing lists