| 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: <cb5d824f-a146-402a-b489-0d777f267677@amd.com>
Date: Thu, 5 Dec 2024 09:35:53 -0600
From: Mario Limonciello <mario.limonciello@....com>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: Hans de Goede <hdegoede@...hat.com>,
"Rafael J . Wysocki" <rafael@...nel.org>, Len Brown <lenb@...nel.org>,
Maximilian Luz <luzmaximilian@...il.com>, Lee Chun-Yi <jlee@...e.com>,
Shyam Sundar S K <Shyam-sundar.S-k@....com>,
Corentin Chary <corentin.chary@...il.com>, "Luke D . Jones"
<luke@...nes.dev>, Ike Panhc <ike.pan@...onical.com>,
Henrique de Moraes Holschuh <hmh@....eng.br>,
Alexis Belmonte <alexbelm48@...il.com>,
Uwe Kleine-König <u.kleine-koenig@...gutronix.de>,
Ai Chao <aichao@...inos.cn>, Gergo Koteles <soyer@....hu>,
open list <linux-kernel@...r.kernel.org>,
"open list:ACPI" <linux-acpi@...r.kernel.org>,
"open list:MICROSOFT SURFACE PLATFORM PROFILE DRIVER"
<platform-driver-x86@...r.kernel.org>,
"open list:THINKPAD ACPI EXTRAS DRIVER"
<ibm-acpi-devel@...ts.sourceforge.net>,
Mark Pearson <mpearson-lenovo@...ebb.ca>,
Matthew Schwartz <matthew.schwartz@...ux.dev>, Armin Wolf <W_Armin@....de>
Subject: Re: [PATCH v9 18/22] ACPI: platform_profile: Check all profile
handler to calculate next
On 12/5/2024 09:22, Ilpo Järvinen wrote:
> On Thu, 5 Dec 2024, Mario Limonciello wrote:
>
>> On 12/5/2024 08:22, Ilpo Järvinen wrote:
>>> On Sun, 1 Dec 2024, Mario Limonciello wrote:
>>>
>>>> As multiple platform profile handlers might not all support the same
>>>> profile, cycling to the next profile could have a different result
>>>> depending on what handler are registered.
>>>>
>>>> Check what is active and supported by all handlers to decide what
>>>> to do.
>>>>
>>>> Reviewed-by: Armin Wolf <W_Armin@....de>
>>>> Tested-by: Mark Pearson <mpearson-lenovo@...ebb.ca>
>>>> Reviewed-by: Mark Pearson <mpearson-lenovo@...ebb.ca>
>>>> Signed-off-by: Mario Limonciello <mario.limonciello@....com>
>>>> ---
>>>> drivers/acpi/platform_profile.c | 30 +++++++++++++++++++++---------
>>>> 1 file changed, 21 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/drivers/acpi/platform_profile.c
>>>> b/drivers/acpi/platform_profile.c
>>>> index d5f0679d59d50..16746d9b9aa7c 100644
>>>> --- a/drivers/acpi/platform_profile.c
>>>> +++ b/drivers/acpi/platform_profile.c
>>>> @@ -407,25 +407,37 @@ EXPORT_SYMBOL_GPL(platform_profile_notify);
>>>> int platform_profile_cycle(void)
>>>> {
>>>> - enum platform_profile_option profile;
>>>> - enum platform_profile_option next;
>>>> + enum platform_profile_option next = PLATFORM_PROFILE_LAST;
>>>> + enum platform_profile_option profile = PLATFORM_PROFILE_LAST;
>>>> + unsigned long choices[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
>>>> int err;
>>>> + set_bit(PLATFORM_PROFILE_LAST, choices);
>>>> scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
>>>> - if (!cur_profile)
>>>> - return -ENODEV;
>>>> + err = class_for_each_device(&platform_profile_class, NULL,
>>>> + &profile, _aggregate_profiles);
>>>> + if (err)
>>>> + return err;
>>>> - err = cur_profile->profile_get(cur_profile, &profile);
>>>> + if (profile == PLATFORM_PROFILE_CUSTOM ||
>>>> + profile == PLATFORM_PROFILE_LAST)
>>>> + return -EINVAL;
>>>> +
>>>> + err = class_for_each_device(&platform_profile_class, NULL,
>>>> + choices, _aggregate_choices);
>>>> if (err)
>>>> return err;
>>>> - next = find_next_bit_wrap(cur_profile->choices,
>>>> PLATFORM_PROFILE_LAST,
>>>> + /* never iterate into a custom if all drivers supported it */
>>>> + clear_bit(PLATFORM_PROFILE_CUSTOM, choices);
>>>
>>> I'm confused by the comment. I was under impression the custom "profile"
>>> is just a framework construct when the _framework_ couldn't find a
>>> consistent profile? How can a driver decide to "support it"? It sounds
>>> like a driver overstepping its intended domain of operation.
>>>
>>> If the intention really is for the driver to "support" or "not support"
>>> custom profile, then you should adjust the commit message of the patch
>>> which introduced it.
>>>
>>
>> Yes I had envisioned that a driver could potentially set custom as well.
>>
>> This idea was introduced by my RFC series that precluded doing the
>> multiple driver handlers.
>>
>> The basic idea is that some drivers (for example asus-wmi and asus-armoury)
>> have the ability for the user to change a sysfs file that represents sPPT or
>> fPPT directly.
>
> I recall that series.
>
>> If this has been done they're "off the beating path" of a predfined
>> profile because they're picking and choosing individual knobs.
>
> The user would still not set it to "custom" nor driver "support" it,
> right? But it's a consequence of tuning those other knobs? Or do you mean
> user would first have to set "custom" and tuning the knobs is blocked
> otherwise?
I think the driver would have to "support" it. But in terms of a user
having to set "custom" and blocking the knobs until they do I think we
can go back and forth on. I don't feel strongly on how the semantics
would work.
>
>> So if a user touches those a driver could set profile as "custom" and if a
>> user chooses the platform profile the driver will override all of those and
>> report a pre-defined profile.
>>
>> So, yes I had that all in my mind but as you point out I definitely forgot to
>> mention it in the commit messages.
>>
>> Do you agree with it? If so, I'll amend the next version where applicable
>> (probably the patch that introduces custom and the documentation patch).
>
> I'm a little worried about overloading the meaning from mere profile
> disagreement to truly off the charted waters travel. Albeit, I suppose
> that overloading is just between global "custom" vs per-driver "custom",
> the latter would never be "custom" in case of mere profile disagreement,
> if I've understood everything correctly?
>
I personally see both as the same. I think we're in agreement on
multi-driver handler and why custom makes sense.
But think about the common case of "one driver handler". For the
purpose of this conversation let's say it's a system that supports
asus-armory and not amd-pmf and that asus-armory supports "custom".
If the user enabled custom ('either' directly or by writing a file that
set it) I think it's best that the "global" platform profile advertises
it too.
Specifically I think about how it translates over into the power slider
in GNOME/KDE. I don't think it's right this slider should show
power-saver if someone manually tuned sPPT up to a giant value.
However if the global platform profile advertises "custom", then the
slider behavior could show an overlay string for "custom", "undefined",
a "!" or something like that.
Powered by blists - more mailing lists