Re: [PATCH v2 1/1] ACPI: platform_profile: Treat quiet and low power the same

["Derek J. Clark" <derekjohn.clark@gmail.com>]

On March 3, 2025 10:47:45 PM PST, Mario Limonciello <superm1@kernel.org> wrote:
>From: Mario Limonciello <mario.limonciello@amd.com>
>
>When two drivers don't support all the same profiles the legacy interface
>only exports the common profiles.
>
>This causes problems for cases where one driver uses low-power but another
>uses quiet because the result is that neither is exported to sysfs.
>ion yet
>If one platform profile handler supports quiet and the other
>supports low power treat them as the same for the purpose of
>the sysfs interface.

Hi Mario,

I haven't tested this version yet but from an initial glance I do have some concerns. In v1 there was handling of balanaced_perfomance, and that isn't present here, which would affect my in progress driver. This also doesn't cover the cool -> low_power option (though I'm not sure where/if that is an actual concern in any drivers at the moment). I'm concerned that if we take the v2 approach that we'll eventually be aliasing a majority of the profiles, further adding ambiguity on what each one actually means. 

In my driver balanced_perfomance is closer to amd_pmf's performance, if shown, whereas in others it might be closer to balanced. Since that is essentially implementation specific I currently am doubtful there is a clean universal approach to aliasing.

The real issue appears to me at that the enabled profiles need to be context aware. Because of that I think something closer to v1 and the hidden options method provides a better way to implement those aliases within any specific driver, allowing the maintainers to determine the "best alias" so to speak. If we put the control into the "primary" driver of how those aliases work and somehow provide context to amd_pmf of the "best match", we can then allow amd_pmf to present all options when more than one low end profile is valid, or only the matching ones if they are just aliased.

- Derek

>Fixes: 688834743d67 ("ACPI: platform_profile: Allow multiple handlers")
>Reported-by: Antheas Kapenekakis <lkml@antheas.dev>
>Closes: https://lore.kernel.org/platform-driver-x86/e64b771e-3255-42ad-9257-5b8fc6c24ac9@gmx.de/T/#mc068042dd29df36c16c8af92664860fc4763974b
>Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>---
> drivers/acpi/platform_profile.c | 38 ++++++++++++++++++++++++++++++---
> 1 file changed, 35 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
>index 2ad53cc6aae53..d9a7cc5891734 100644
>--- a/drivers/acpi/platform_profile.c
>+++ b/drivers/acpi/platform_profile.c
>@@ -73,8 +73,20 @@ static int _store_class_profile(struct device *dev, void *data)
> 
> 	lockdep_assert_held(&profile_lock);
> 	handler = to_pprof_handler(dev);
>-	if (!test_bit(*bit, handler->choices))
>-		return -EOPNOTSUPP;
>+	if (!test_bit(*bit, handler->choices)) {
>+		switch (*bit) {
>+		case PLATFORM_PROFILE_QUIET:
>+			*bit = PLATFORM_PROFILE_LOW_POWER;
>+			break;
>+		case PLATFORM_PROFILE_LOW_POWER:
>+			*bit = PLATFORM_PROFILE_QUIET;
>+			break;
>+		default:
>+			return -EOPNOTSUPP;
>+		}
>+		if (!test_bit(*bit, handler->choices))
>+			return -EOPNOTSUPP;
>+	}
> 
> 	return handler->ops->profile_set(dev, *bit);
> }
>@@ -252,8 +264,16 @@ static int _aggregate_choices(struct device *dev, void *data)
> 	handler = to_pprof_handler(dev);
> 	if (test_bit(PLATFORM_PROFILE_LAST, aggregate))
> 		bitmap_copy(aggregate, handler->choices, PLATFORM_PROFILE_LAST);
>-	else
>+	else {
>+		/* treat quiet and low power the same for aggregation purposes */
>+		if (test_bit(PLATFORM_PROFILE_QUIET, handler->choices) &&
>+		    test_bit(PLATFORM_PROFILE_LOW_POWER, aggregate))
>+			set_bit(PLATFORM_PROFILE_QUIET, aggregate);
>+		else if (test_bit(PLATFORM_PROFILE_LOW_POWER, handler->choices) &&
>+			 test_bit(PLATFORM_PROFILE_QUIET, aggregate))
>+			set_bit(PLATFORM_PROFILE_LOW_POWER, aggregate);
> 		bitmap_and(aggregate, handler->choices, aggregate, PLATFORM_PROFILE_LAST);
>+	}
> 
> 	return 0;
> }
>@@ -305,6 +325,13 @@ static int _aggregate_profiles(struct device *dev, void *data)
> 	if (err)
> 		return err;
> 
>+	/* treat low-power and quiet as the same */
>+	if ((*profile == PLATFORM_PROFILE_LOW_POWER &&
>+	     val == PLATFORM_PROFILE_QUIET) ||
>+	    (*profile == PLATFORM_PROFILE_QUIET &&
>+	     val == PLATFORM_PROFILE_LOW_POWER))
>+		*profile = val;
>+
> 	if (*profile != PLATFORM_PROFILE_LAST && *profile != val)
> 		*profile = PLATFORM_PROFILE_CUSTOM;
> 	else
>@@ -531,6 +558,11 @@ struct device *platform_profile_register(struct device *dev, const char *name,
> 		dev_err(dev, "Failed to register platform_profile class device with empty choices\n");
> 		return ERR_PTR(-EINVAL);
> 	}
>+	if (test_bit(PLATFORM_PROFILE_QUIET, pprof->choices) &&
>+	    test_bit(PLATFORM_PROFILE_LOW_POWER, pprof->choices)) {
>+		dev_err(dev, "Failed to register platform_profile class device with both quiet and low-power\n");
>+		return ERR_PTR(-EINVAL);
>+	}
> 
> 	guard(mutex)(&profile_lock);
> 

- Derek