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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 4515 bytes --]

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@gmx.de>
> > > Tested-by: Mark Pearson <mpearson-lenovo@squebb.ca>
> > > Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
> > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.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?

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