Re: [PATCH v9 18/22] ACPI: platform_profile: Check all profile handler to calculate next

[Mario Limonciello]

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.

If this has been done they're "off the beating path" of a predfined
profile because they're picking and choosing individual knobs.

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