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

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

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.

-- 
 i.

> +		next = find_next_bit_wrap(choices,
> +					  PLATFORM_PROFILE_LAST,
>  					  profile + 1);
>  
> -		if (WARN_ON(next == PLATFORM_PROFILE_LAST))
> -			return -EINVAL;
> +		err = class_for_each_device(&platform_profile_class, NULL, &next,
> +					    _store_and_notify);
>  
> -		err = cur_profile->profile_set(cur_profile, next);
>  		if (err)
>  			return err;
>  	}
>