| 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: <D87J6E7DFLS0.1BY00BAZFWEH7@gmail.com>
Date: Tue, 04 Mar 2025 08:53:59 -0500
From: "Kurt Borja" <kuurtb@...il.com>
To: "Antheas Kapenekakis" <lkml@...heas.dev>
Cc: "Mario Limonciello" <superm1@...nel.org>, "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 Tue Mar 4, 2025 at 8:32 AM -05, Antheas Kapenekakis wrote:
> On Tue, 4 Mar 2025 at 14:28, Kurt Borja <kuurtb@...il.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@...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?
>>
>> 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
>> >>>
>>
Powered by blists - more mailing lists