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?
> 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.
