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@xxxxxx>
Tested-by: Mark Pearson <mpearson-lenovo@xxxxxxxxx>
Reviewed-by: Mark Pearson <mpearson-lenovo@xxxxxxxxx>
Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>
---
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.
