[PATCH 0/3] Add support for hidden choices to platform_profile

[PATCH 0/3] Add support for hidden choices to platform_profile

[Mario Limonciello]

From: Mario Limonciello <mario.limonciello@amd.com>

When two drivers provide platform profile handlers but use different
strings to mean (essentially) the same thing the legacy interface won't
export them because it only shows profiles common to multiple drivers.

This causes an unexpected behavior to people who have upgraded from an
earlier kernel because if multiple drivers have bound platform profile
handlers they might not be able to access profiles they were expecting.

Introduce a concept of a "hidden choice" that drivers can register and
the platform profile handler code will utilize when using the legacy
interface.

There have been some other attempts at solving this issue in other ways.
This serves as an alternative to those attempts.

Link: https://lore.kernel.org/platform-driver-x86/e64b771e-3255-42ad-9257-5b8fc6c24ac9@gmx.de/T/#t
Link: https://lore.kernel.org/platform-driver-x86/CAGwozwF-WVEgiAbWbRCiUaXf=BVa3KqmMJfs06trdMQHpTGmjQ@mail.gmail.com/T/#m2f3929e2d4f73cc0eedd14738170dad45232fd18
Cc: Antheas Kapenekakis <lkml@antheas.dev>
Cc: "Luke D. Jones" <luke@ljones.dev>

Mario Limonciello (3):
  ACPI: platform_profile: Add support for hidden choices
  platform/x86/amd: pmf: Add 'quiet' to hidden choices
  platform/x86/amd: pmf: Add balanced-performance to hidden choices

 drivers/acpi/platform_profile.c    | 94 +++++++++++++++++++++++-------
 drivers/platform/x86/amd/pmf/sps.c | 11 ++++
 include/linux/platform_profile.h   |  3 +
 3 files changed, 87 insertions(+), 21 deletions(-)

-- 
2.43.0

[PATCH 1/3] ACPI: platform_profile: Add support for hidden choices

[Mario Limonciello]

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.

To allow two drivers to disagree, add support for "hidden choices".
Hidden choices are platform profiles that a driver supports to be
compatible with the platform profile of another driver.

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>
---
Cc: "Luke D. Jones" <luke@ljones.dev>
 drivers/acpi/platform_profile.c  | 94 +++++++++++++++++++++++++-------
 include/linux/platform_profile.h |  3 +
 2 files changed, 76 insertions(+), 21 deletions(-)

diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
index 2ad53cc6aae53..ef9444482db19 100644
--- a/drivers/acpi/platform_profile.c
+++ b/drivers/acpi/platform_profile.c
@@ -21,9 +21,15 @@ struct platform_profile_handler {
 	struct device dev;
 	int minor;
 	unsigned long choices[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
+	unsigned long hidden_choices[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
 	const struct platform_profile_ops *ops;
 };
 
+struct aggregate_choices_data {
+	unsigned long aggregate[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
+	int count;
+};
+
 static const char * const profile_names[] = {
 	[PLATFORM_PROFILE_LOW_POWER] = "low-power",
 	[PLATFORM_PROFILE_COOL] = "cool",
@@ -73,7 +79,7 @@ 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))
+	if (!test_bit(*bit, handler->choices) && !test_bit(*bit, handler->hidden_choices))
 		return -EOPNOTSUPP;
 
 	return handler->ops->profile_set(dev, *bit);
@@ -239,21 +245,44 @@ static const struct class platform_profile_class = {
 /**
  * _aggregate_choices - Aggregate the available profile choices
  * @dev: The device
- * @data: The available profile choices
+ * @arg: struct aggregate_choices_data
  *
  * Return: 0 on success, -errno on failure
  */
-static int _aggregate_choices(struct device *dev, void *data)
+static int _aggregate_choices(struct device *dev, void *arg)
 {
+	unsigned long tmp[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
+	struct aggregate_choices_data *data = arg;
 	struct platform_profile_handler *handler;
-	unsigned long *aggregate = data;
 
 	lockdep_assert_held(&profile_lock);
 	handler = to_pprof_handler(dev);
-	if (test_bit(PLATFORM_PROFILE_LAST, aggregate))
-		bitmap_copy(aggregate, handler->choices, PLATFORM_PROFILE_LAST);
+	bitmap_or(tmp, handler->choices, handler->hidden_choices, PLATFORM_PROFILE_LAST);
+	if (test_bit(PLATFORM_PROFILE_LAST, data->aggregate))
+		bitmap_copy(data->aggregate, tmp, PLATFORM_PROFILE_LAST);
 	else
-		bitmap_and(aggregate, handler->choices, aggregate, PLATFORM_PROFILE_LAST);
+		bitmap_and(data->aggregate, tmp, data->aggregate, PLATFORM_PROFILE_LAST);
+	data->count++;
+
+	return 0;
+}
+
+/**
+ * _remove_hidden_choices - Remove hidden choices from aggregate data
+ * @dev: The device
+ * @arg: struct aggregate_choices_data
+ *
+ * Return: 0 on success, -errno on failure
+ */
+static int _remove_hidden_choices(struct device *dev, void *arg)
+{
+	struct aggregate_choices_data *data = arg;
+	struct platform_profile_handler *handler;
+
+	lockdep_assert_held(&profile_lock);
+	handler = to_pprof_handler(dev);
+	bitmap_andnot(data->aggregate, handler->choices,
+		      handler->hidden_choices, PLATFORM_PROFILE_LAST);
 
 	return 0;
 }
@@ -270,22 +299,31 @@ static ssize_t platform_profile_choices_show(struct device *dev,
 					     struct device_attribute *attr,
 					     char *buf)
 {
-	unsigned long aggregate[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
+	struct aggregate_choices_data data = {
+		.aggregate = { [0 ... BITS_TO_LONGS(PLATFORM_PROFILE_LAST) - 1] = ~0UL },
+		.count = 0,
+	};
 	int err;
 
-	set_bit(PLATFORM_PROFILE_LAST, aggregate);
+	set_bit(PLATFORM_PROFILE_LAST, data.aggregate);
 	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
 		err = class_for_each_device(&platform_profile_class, NULL,
-					    aggregate, _aggregate_choices);
+					    &data, _aggregate_choices);
 		if (err)
 			return err;
+		if (data.count == 1) {
+			err = class_for_each_device(&platform_profile_class, NULL,
+						    &data, _remove_hidden_choices);
+			if (err)
+				return err;
+		}
 	}
 
 	/* no profile handler registered any more */
-	if (bitmap_empty(aggregate, PLATFORM_PROFILE_LAST))
+	if (bitmap_empty(data.aggregate, PLATFORM_PROFILE_LAST))
 		return -EINVAL;
 
-	return _commmon_choices_show(aggregate, buf);
+	return _commmon_choices_show(data.aggregate, buf);
 }
 
 /**
@@ -373,7 +411,10 @@ static ssize_t platform_profile_store(struct device *dev,
 				      struct device_attribute *attr,
 				      const char *buf, size_t count)
 {
-	unsigned long choices[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
+	struct aggregate_choices_data data = {
+		.aggregate = { [0 ... BITS_TO_LONGS(PLATFORM_PROFILE_LAST) - 1] = ~0UL },
+		.count = 0,
+	};
 	int ret;
 	int i;
 
@@ -381,13 +422,13 @@ static ssize_t platform_profile_store(struct device *dev,
 	i = sysfs_match_string(profile_names, buf);
 	if (i < 0 || i == PLATFORM_PROFILE_CUSTOM)
 		return -EINVAL;
-	set_bit(PLATFORM_PROFILE_LAST, choices);
+	set_bit(PLATFORM_PROFILE_LAST, data.aggregate);
 	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
 		ret = class_for_each_device(&platform_profile_class, NULL,
-					    choices, _aggregate_choices);
+					    &data, _aggregate_choices);
 		if (ret)
 			return ret;
-		if (!test_bit(i, choices))
+		if (!test_bit(i, data.aggregate))
 			return -EOPNOTSUPP;
 
 		ret = class_for_each_device(&platform_profile_class, NULL, &i,
@@ -453,12 +494,15 @@ EXPORT_SYMBOL_GPL(platform_profile_notify);
  */
 int platform_profile_cycle(void)
 {
+	struct aggregate_choices_data data = {
+		.aggregate = { [0 ... BITS_TO_LONGS(PLATFORM_PROFILE_LAST) - 1] = ~0UL },
+		.count = 0,
+	};
 	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);
+	set_bit(PLATFORM_PROFILE_LAST, data.aggregate);
 	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
 		err = class_for_each_device(&platform_profile_class, NULL,
 					    &profile, _aggregate_profiles);
@@ -470,14 +514,14 @@ int platform_profile_cycle(void)
 			return -EINVAL;
 
 		err = class_for_each_device(&platform_profile_class, NULL,
-					    choices, _aggregate_choices);
+					    &data, _aggregate_choices);
 		if (err)
 			return err;
 
 		/* never iterate into a custom if all drivers supported it */
-		clear_bit(PLATFORM_PROFILE_CUSTOM, choices);
+		clear_bit(PLATFORM_PROFILE_CUSTOM, data.aggregate);
 
-		next = find_next_bit_wrap(choices,
+		next = find_next_bit_wrap(data.aggregate,
 					  PLATFORM_PROFILE_LAST,
 					  profile + 1);
 
@@ -532,6 +576,14 @@ struct device *platform_profile_register(struct device *dev, const char *name,
 		return ERR_PTR(-EINVAL);
 	}
 
+	if (ops->hidden_choices) {
+		err = ops->hidden_choices(drvdata, pprof->hidden_choices);
+		if (err) {
+			dev_err(dev, "platform_profile hidden_choices failed\n");
+			return ERR_PTR(err);
+		}
+	}
+
 	guard(mutex)(&profile_lock);
 
 	/* create class interface for individual handler */
diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h
index 8ab5b0e8eb2c1..8c9df7dadd5d3 100644
--- a/include/linux/platform_profile.h
+++ b/include/linux/platform_profile.h
@@ -33,6 +33,8 @@ enum platform_profile_option {
  * @probe: Callback to setup choices available to the new class device. These
  *	   choices will only be enforced when setting a new profile, not when
  *	   getting the current one.
+ * @hidden_choices: Callback to setup choices that are not visible to the user
+ *		    but can be set by the driver.
  * @profile_get: Callback that will be called when showing the current platform
  *		 profile in sysfs.
  * @profile_set: Callback that will be called when storing a new platform
@@ -40,6 +42,7 @@ enum platform_profile_option {
  */
 struct platform_profile_ops {
 	int (*probe)(void *drvdata, unsigned long *choices);
+	int (*hidden_choices)(void *drvdata, unsigned long *choices);
 	int (*profile_get)(struct device *dev, enum platform_profile_option *profile);
 	int (*profile_set)(struct device *dev, enum platform_profile_option profile);
 };
-- 
2.43.0

Re: [PATCH 1/3] ACPI: platform_profile: Add support for hidden choices

[Antheas Kapenekakis]

LGTM. Although patch is a bit more complicated.

I do not have time to test this today. I can try tomorrow.

On Fri, 28 Feb 2025 at 18:02, 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.
>
> To allow two drivers to disagree, add support for "hidden choices".
> Hidden choices are platform profiles that a driver supports to be
> compatible with the platform profile of another driver.
>
> 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>
> ---
> Cc: "Luke D. Jones" <luke@ljones.dev>
>  drivers/acpi/platform_profile.c  | 94 +++++++++++++++++++++++++-------
>  include/linux/platform_profile.h |  3 +
>  2 files changed, 76 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
> index 2ad53cc6aae53..ef9444482db19 100644
> --- a/drivers/acpi/platform_profile.c
> +++ b/drivers/acpi/platform_profile.c
> @@ -21,9 +21,15 @@ struct platform_profile_handler {
>         struct device dev;
>         int minor;
>         unsigned long choices[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
> +       unsigned long hidden_choices[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
>         const struct platform_profile_ops *ops;
>  };
>
> +struct aggregate_choices_data {
> +       unsigned long aggregate[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
> +       int count;
> +};
> +
>  static const char * const profile_names[] = {
>         [PLATFORM_PROFILE_LOW_POWER] = "low-power",
>         [PLATFORM_PROFILE_COOL] = "cool",
> @@ -73,7 +79,7 @@ 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))
> +       if (!test_bit(*bit, handler->choices) && !test_bit(*bit, handler->hidden_choices))
>                 return -EOPNOTSUPP;
>
>         return handler->ops->profile_set(dev, *bit);
> @@ -239,21 +245,44 @@ static const struct class platform_profile_class = {
>  /**
>   * _aggregate_choices - Aggregate the available profile choices
>   * @dev: The device
> - * @data: The available profile choices
> + * @arg: struct aggregate_choices_data
>   *
>   * Return: 0 on success, -errno on failure
>   */
> -static int _aggregate_choices(struct device *dev, void *data)
> +static int _aggregate_choices(struct device *dev, void *arg)
>  {
> +       unsigned long tmp[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
> +       struct aggregate_choices_data *data = arg;
>         struct platform_profile_handler *handler;
> -       unsigned long *aggregate = data;
>
>         lockdep_assert_held(&profile_lock);
>         handler = to_pprof_handler(dev);
> -       if (test_bit(PLATFORM_PROFILE_LAST, aggregate))
> -               bitmap_copy(aggregate, handler->choices, PLATFORM_PROFILE_LAST);
> +       bitmap_or(tmp, handler->choices, handler->hidden_choices, PLATFORM_PROFILE_LAST);
> +       if (test_bit(PLATFORM_PROFILE_LAST, data->aggregate))
> +               bitmap_copy(data->aggregate, tmp, PLATFORM_PROFILE_LAST);
>         else
> -               bitmap_and(aggregate, handler->choices, aggregate, PLATFORM_PROFILE_LAST);
> +               bitmap_and(data->aggregate, tmp, data->aggregate, PLATFORM_PROFILE_LAST);
> +       data->count++;
> +
> +       return 0;
> +}
> +
> +/**
> + * _remove_hidden_choices - Remove hidden choices from aggregate data
> + * @dev: The device
> + * @arg: struct aggregate_choices_data
> + *
> + * Return: 0 on success, -errno on failure
> + */
> +static int _remove_hidden_choices(struct device *dev, void *arg)
> +{
> +       struct aggregate_choices_data *data = arg;
> +       struct platform_profile_handler *handler;
> +
> +       lockdep_assert_held(&profile_lock);
> +       handler = to_pprof_handler(dev);
> +       bitmap_andnot(data->aggregate, handler->choices,
> +                     handler->hidden_choices, PLATFORM_PROFILE_LAST);
>
>         return 0;
>  }
> @@ -270,22 +299,31 @@ static ssize_t platform_profile_choices_show(struct device *dev,
>                                              struct device_attribute *attr,
>                                              char *buf)
>  {
> -       unsigned long aggregate[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
> +       struct aggregate_choices_data data = {
> +               .aggregate = { [0 ... BITS_TO_LONGS(PLATFORM_PROFILE_LAST) - 1] = ~0UL },
> +               .count = 0,
> +       };
>         int err;
>
> -       set_bit(PLATFORM_PROFILE_LAST, aggregate);
> +       set_bit(PLATFORM_PROFILE_LAST, data.aggregate);
>         scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
>                 err = class_for_each_device(&platform_profile_class, NULL,
> -                                           aggregate, _aggregate_choices);
> +                                           &data, _aggregate_choices);
>                 if (err)
>                         return err;
> +               if (data.count == 1) {
> +                       err = class_for_each_device(&platform_profile_class, NULL,
> +                                                   &data, _remove_hidden_choices);
> +                       if (err)
> +                               return err;
> +               }
>         }
>
>         /* no profile handler registered any more */
> -       if (bitmap_empty(aggregate, PLATFORM_PROFILE_LAST))
> +       if (bitmap_empty(data.aggregate, PLATFORM_PROFILE_LAST))
>                 return -EINVAL;
>
> -       return _commmon_choices_show(aggregate, buf);
> +       return _commmon_choices_show(data.aggregate, buf);
>  }
>
>  /**
> @@ -373,7 +411,10 @@ static ssize_t platform_profile_store(struct device *dev,
>                                       struct device_attribute *attr,
>                                       const char *buf, size_t count)
>  {
> -       unsigned long choices[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
> +       struct aggregate_choices_data data = {
> +               .aggregate = { [0 ... BITS_TO_LONGS(PLATFORM_PROFILE_LAST) - 1] = ~0UL },
> +               .count = 0,
> +       };
>         int ret;
>         int i;
>
> @@ -381,13 +422,13 @@ static ssize_t platform_profile_store(struct device *dev,
>         i = sysfs_match_string(profile_names, buf);
>         if (i < 0 || i == PLATFORM_PROFILE_CUSTOM)
>                 return -EINVAL;
> -       set_bit(PLATFORM_PROFILE_LAST, choices);
> +       set_bit(PLATFORM_PROFILE_LAST, data.aggregate);
>         scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
>                 ret = class_for_each_device(&platform_profile_class, NULL,
> -                                           choices, _aggregate_choices);
> +                                           &data, _aggregate_choices);
>                 if (ret)
>                         return ret;
> -               if (!test_bit(i, choices))
> +               if (!test_bit(i, data.aggregate))
>                         return -EOPNOTSUPP;
>
>                 ret = class_for_each_device(&platform_profile_class, NULL, &i,
> @@ -453,12 +494,15 @@ EXPORT_SYMBOL_GPL(platform_profile_notify);
>   */
>  int platform_profile_cycle(void)
>  {
> +       struct aggregate_choices_data data = {
> +               .aggregate = { [0 ... BITS_TO_LONGS(PLATFORM_PROFILE_LAST) - 1] = ~0UL },
> +               .count = 0,
> +       };
>         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);
> +       set_bit(PLATFORM_PROFILE_LAST, data.aggregate);
>         scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
>                 err = class_for_each_device(&platform_profile_class, NULL,
>                                             &profile, _aggregate_profiles);
> @@ -470,14 +514,14 @@ int platform_profile_cycle(void)
>                         return -EINVAL;
>
>                 err = class_for_each_device(&platform_profile_class, NULL,
> -                                           choices, _aggregate_choices);
> +                                           &data, _aggregate_choices);
>                 if (err)
>                         return err;
>
>                 /* never iterate into a custom if all drivers supported it */
> -               clear_bit(PLATFORM_PROFILE_CUSTOM, choices);
> +               clear_bit(PLATFORM_PROFILE_CUSTOM, data.aggregate);
>
> -               next = find_next_bit_wrap(choices,
> +               next = find_next_bit_wrap(data.aggregate,
>                                           PLATFORM_PROFILE_LAST,
>                                           profile + 1);
>
> @@ -532,6 +576,14 @@ struct device *platform_profile_register(struct device *dev, const char *name,
>                 return ERR_PTR(-EINVAL);
>         }
>
> +       if (ops->hidden_choices) {
> +               err = ops->hidden_choices(drvdata, pprof->hidden_choices);
> +               if (err) {
> +                       dev_err(dev, "platform_profile hidden_choices failed\n");
> +                       return ERR_PTR(err);
> +               }
> +       }
> +
>         guard(mutex)(&profile_lock);
>
>         /* create class interface for individual handler */
> diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h
> index 8ab5b0e8eb2c1..8c9df7dadd5d3 100644
> --- a/include/linux/platform_profile.h
> +++ b/include/linux/platform_profile.h
> @@ -33,6 +33,8 @@ enum platform_profile_option {
>   * @probe: Callback to setup choices available to the new class device. These
>   *        choices will only be enforced when setting a new profile, not when
>   *        getting the current one.
> + * @hidden_choices: Callback to setup choices that are not visible to the user
> + *                 but can be set by the driver.
>   * @profile_get: Callback that will be called when showing the current platform
>   *              profile in sysfs.
>   * @profile_set: Callback that will be called when storing a new platform
> @@ -40,6 +42,7 @@ enum platform_profile_option {
>   */
>  struct platform_profile_ops {
>         int (*probe)(void *drvdata, unsigned long *choices);
> +       int (*hidden_choices)(void *drvdata, unsigned long *choices);
>         int (*profile_get)(struct device *dev, enum platform_profile_option *profile);
>         int (*profile_set)(struct device *dev, enum platform_profile_option profile);
>  };
> --
> 2.43.0
>

Re: [PATCH 1/3] ACPI: platform_profile: Add support for hidden choices

[Kurt Borja]

Hi Mario,

On Fri Feb 28, 2025 at 12:01 PM -05, Mario Limonciello 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.
>
> To allow two drivers to disagree, add support for "hidden choices".
> Hidden choices are platform profiles that a driver supports to be
> compatible with the platform profile of another driver.
>
> 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>
> ---
> Cc: "Luke D. Jones" <luke@ljones.dev>
>  drivers/acpi/platform_profile.c  | 94 +++++++++++++++++++++++++-------
>  include/linux/platform_profile.h |  3 +
>  2 files changed, 76 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
> index 2ad53cc6aae53..ef9444482db19 100644
> --- a/drivers/acpi/platform_profile.c
> +++ b/drivers/acpi/platform_profile.c
> @@ -21,9 +21,15 @@ struct platform_profile_handler {
>  	struct device dev;
>  	int minor;
>  	unsigned long choices[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
> +	unsigned long hidden_choices[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
>  	const struct platform_profile_ops *ops;
>  };
>  
> +struct aggregate_choices_data {
> +	unsigned long aggregate[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
> +	int count;
> +};
> +
>  static const char * const profile_names[] = {
>  	[PLATFORM_PROFILE_LOW_POWER] = "low-power",
>  	[PLATFORM_PROFILE_COOL] = "cool",
> @@ -73,7 +79,7 @@ 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))
> +	if (!test_bit(*bit, handler->choices) && !test_bit(*bit, handler->hidden_choices))
>  		return -EOPNOTSUPP;
>  
>  	return handler->ops->profile_set(dev, *bit);
> @@ -239,21 +245,44 @@ static const struct class platform_profile_class = {
>  /**
>   * _aggregate_choices - Aggregate the available profile choices
>   * @dev: The device
> - * @data: The available profile choices
> + * @arg: struct aggregate_choices_data
>   *
>   * Return: 0 on success, -errno on failure
>   */
> -static int _aggregate_choices(struct device *dev, void *data)
> +static int _aggregate_choices(struct device *dev, void *arg)
>  {
> +	unsigned long tmp[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
> +	struct aggregate_choices_data *data = arg;
>  	struct platform_profile_handler *handler;
> -	unsigned long *aggregate = data;
>  
>  	lockdep_assert_held(&profile_lock);
>  	handler = to_pprof_handler(dev);
> -	if (test_bit(PLATFORM_PROFILE_LAST, aggregate))
> -		bitmap_copy(aggregate, handler->choices, PLATFORM_PROFILE_LAST);
> +	bitmap_or(tmp, handler->choices, handler->hidden_choices, PLATFORM_PROFILE_LAST);
> +	if (test_bit(PLATFORM_PROFILE_LAST, data->aggregate))
> +		bitmap_copy(data->aggregate, tmp, PLATFORM_PROFILE_LAST);
>  	else
> -		bitmap_and(aggregate, handler->choices, aggregate, PLATFORM_PROFILE_LAST);
> +		bitmap_and(data->aggregate, tmp, data->aggregate, PLATFORM_PROFILE_LAST);
> +	data->count++;
> +
> +	return 0;
> +}
> +
> +/**
> + * _remove_hidden_choices - Remove hidden choices from aggregate data
> + * @dev: The device
> + * @arg: struct aggregate_choices_data
> + *
> + * Return: 0 on success, -errno on failure
> + */
> +static int _remove_hidden_choices(struct device *dev, void *arg)
> +{
> +	struct aggregate_choices_data *data = arg;
> +	struct platform_profile_handler *handler;
> +
> +	lockdep_assert_held(&profile_lock);
> +	handler = to_pprof_handler(dev);
> +	bitmap_andnot(data->aggregate, handler->choices,
> +		      handler->hidden_choices, PLATFORM_PROFILE_LAST);
>  
>  	return 0;
>  }
> @@ -270,22 +299,31 @@ static ssize_t platform_profile_choices_show(struct device *dev,
>  					     struct device_attribute *attr,
>  					     char *buf)
>  {
> -	unsigned long aggregate[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
> +	struct aggregate_choices_data data = {
> +		.aggregate = { [0 ... BITS_TO_LONGS(PLATFORM_PROFILE_LAST) - 1] = ~0UL },
> +		.count = 0,
> +	};
>  	int err;
>  
> -	set_bit(PLATFORM_PROFILE_LAST, aggregate);
> +	set_bit(PLATFORM_PROFILE_LAST, data.aggregate);
>  	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
>  		err = class_for_each_device(&platform_profile_class, NULL,
> -					    aggregate, _aggregate_choices);
> +					    &data, _aggregate_choices);
>  		if (err)
>  			return err;
> +		if (data.count == 1) {
> +			err = class_for_each_device(&platform_profile_class, NULL,
> +						    &data, _remove_hidden_choices);
> +			if (err)
> +				return err;
> +		}
>  	}
>  
>  	/* no profile handler registered any more */
> -	if (bitmap_empty(aggregate, PLATFORM_PROFILE_LAST))
> +	if (bitmap_empty(data.aggregate, PLATFORM_PROFILE_LAST))
>  		return -EINVAL;
>  
> -	return _commmon_choices_show(aggregate, buf);
> +	return _commmon_choices_show(data.aggregate, buf);
>  }
>  
>  /**
> @@ -373,7 +411,10 @@ static ssize_t platform_profile_store(struct device *dev,
>  				      struct device_attribute *attr,
>  				      const char *buf, size_t count)
>  {
> -	unsigned long choices[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
> +	struct aggregate_choices_data data = {
> +		.aggregate = { [0 ... BITS_TO_LONGS(PLATFORM_PROFILE_LAST) - 1] = ~0UL },
> +		.count = 0,
> +	};
>  	int ret;
>  	int i;
>  
> @@ -381,13 +422,13 @@ static ssize_t platform_profile_store(struct device *dev,
>  	i = sysfs_match_string(profile_names, buf);
>  	if (i < 0 || i == PLATFORM_PROFILE_CUSTOM)
>  		return -EINVAL;
> -	set_bit(PLATFORM_PROFILE_LAST, choices);
> +	set_bit(PLATFORM_PROFILE_LAST, data.aggregate);
>  	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
>  		ret = class_for_each_device(&platform_profile_class, NULL,
> -					    choices, _aggregate_choices);
> +					    &data, _aggregate_choices);
>  		if (ret)
>  			return ret;
> -		if (!test_bit(i, choices))
> +		if (!test_bit(i, data.aggregate))
>  			return -EOPNOTSUPP;
>  
>  		ret = class_for_each_device(&platform_profile_class, NULL, &i,
> @@ -453,12 +494,15 @@ EXPORT_SYMBOL_GPL(platform_profile_notify);
>   */
>  int platform_profile_cycle(void)
>  {
> +	struct aggregate_choices_data data = {
> +		.aggregate = { [0 ... BITS_TO_LONGS(PLATFORM_PROFILE_LAST) - 1] = ~0UL },
> +		.count = 0,
> +	};
>  	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);
> +	set_bit(PLATFORM_PROFILE_LAST, data.aggregate);
>  	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
>  		err = class_for_each_device(&platform_profile_class, NULL,
>  					    &profile, _aggregate_profiles);
> @@ -470,14 +514,14 @@ int platform_profile_cycle(void)
>  			return -EINVAL;
>  
>  		err = class_for_each_device(&platform_profile_class, NULL,
> -					    choices, _aggregate_choices);
> +					    &data, _aggregate_choices);
>  		if (err)
>  			return err;
>  
>  		/* never iterate into a custom if all drivers supported it */
> -		clear_bit(PLATFORM_PROFILE_CUSTOM, choices);
> +		clear_bit(PLATFORM_PROFILE_CUSTOM, data.aggregate);
>  
> -		next = find_next_bit_wrap(choices,
> +		next = find_next_bit_wrap(data.aggregate,
>  					  PLATFORM_PROFILE_LAST,
>  					  profile + 1);
>  
> @@ -532,6 +576,14 @@ struct device *platform_profile_register(struct device *dev, const char *name,
>  		return ERR_PTR(-EINVAL);
>  	}
>  
> +	if (ops->hidden_choices) {
> +		err = ops->hidden_choices(drvdata, pprof->hidden_choices);
> +		if (err) {
> +			dev_err(dev, "platform_profile hidden_choices failed\n");
> +			return ERR_PTR(err);
> +		}
> +	}
> +
>  	guard(mutex)(&profile_lock);
>  
>  	/* create class interface for individual handler */
> diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h
> index 8ab5b0e8eb2c1..8c9df7dadd5d3 100644
> --- a/include/linux/platform_profile.h
> +++ b/include/linux/platform_profile.h
> @@ -33,6 +33,8 @@ enum platform_profile_option {
>   * @probe: Callback to setup choices available to the new class device. These
>   *	   choices will only be enforced when setting a new profile, not when
>   *	   getting the current one.
> + * @hidden_choices: Callback to setup choices that are not visible to the user
> + *		    but can be set by the driver.
>   * @profile_get: Callback that will be called when showing the current platform
>   *		 profile in sysfs.
>   * @profile_set: Callback that will be called when storing a new platform
> @@ -40,6 +42,7 @@ enum platform_profile_option {
>   */
>  struct platform_profile_ops {
>  	int (*probe)(void *drvdata, unsigned long *choices);
> +	int (*hidden_choices)(void *drvdata, unsigned long *choices);
>  	int (*profile_get)(struct device *dev, enum platform_profile_option *profile);
>  	int (*profile_set)(struct device *dev, enum platform_profile_option profile);
>  };

This approach works really well for the PMF driver because the
profile_get callback retrieves the raw profile that the profile_set
callback cached. However this is not the case for quite a few drivers,
which usually just retrieve the current profile from WMI for example.

This means that writing a profile to the legacy platform_profile
attribute, which a driver has selected as a "hidden choice" may result
in the operation succeeding, but if the user were to immediately read
from platform_profile it would display "custom", because the profiles
for different handlers may be unsynchronized.

This makes me wonder if the added complexity this patch brings, is
really worth it.

IMHO we should do what Armin suggested in the patch proposed by Antheas.
In fact, I would suggest an even simpler version:

  1. The legacy platform_profile_choices should aggregate `choices`
     with bitmap_or instead of bitmap_and. i.e. It should display all
     available choices
  2. When writing a profile to the legacy platform_profile, if a handler
     doesn't support it, we simply ignore it without failing and
     continue to the next

I believe this works well with power-profiles-daemon, but I'm not
entirely sure. Maybe you know more about it.

This of course has the problem that profiles would be unsync and
platform_profile might display "custom" immediately after setting a
profile, but this patch has the same "issue".

For me this "custom" issue, is not really an issue. The legacy interface
should be deprecated in favor of the class interface, and new/old
user-space tools should use/migrate to that instead.

Let me know what you think!

-- 
 ~ Kurt

Re: [PATCH 1/3] ACPI: platform_profile: Add support for hidden choices

[Mario Limonciello]

On 2/28/2025 16:08, Kurt Borja wrote:
> Hi Mario,
> 
> On Fri Feb 28, 2025 at 12:01 PM -05, Mario Limonciello 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.
>>
>> To allow two drivers to disagree, add support for "hidden choices".
>> Hidden choices are platform profiles that a driver supports to be
>> compatible with the platform profile of another driver.
>>
>> 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>
>> ---
>> Cc: "Luke D. Jones" <luke@ljones.dev>
>>   drivers/acpi/platform_profile.c  | 94 +++++++++++++++++++++++++-------
>>   include/linux/platform_profile.h |  3 +
>>   2 files changed, 76 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
>> index 2ad53cc6aae53..ef9444482db19 100644
>> --- a/drivers/acpi/platform_profile.c
>> +++ b/drivers/acpi/platform_profile.c
>> @@ -21,9 +21,15 @@ struct platform_profile_handler {
>>   	struct device dev;
>>   	int minor;
>>   	unsigned long choices[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
>> +	unsigned long hidden_choices[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
>>   	const struct platform_profile_ops *ops;
>>   };
>>   
>> +struct aggregate_choices_data {
>> +	unsigned long aggregate[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
>> +	int count;
>> +};
>> +
>>   static const char * const profile_names[] = {
>>   	[PLATFORM_PROFILE_LOW_POWER] = "low-power",
>>   	[PLATFORM_PROFILE_COOL] = "cool",
>> @@ -73,7 +79,7 @@ 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))
>> +	if (!test_bit(*bit, handler->choices) && !test_bit(*bit, handler->hidden_choices))
>>   		return -EOPNOTSUPP;
>>   
>>   	return handler->ops->profile_set(dev, *bit);
>> @@ -239,21 +245,44 @@ static const struct class platform_profile_class = {
>>   /**
>>    * _aggregate_choices - Aggregate the available profile choices
>>    * @dev: The device
>> - * @data: The available profile choices
>> + * @arg: struct aggregate_choices_data
>>    *
>>    * Return: 0 on success, -errno on failure
>>    */
>> -static int _aggregate_choices(struct device *dev, void *data)
>> +static int _aggregate_choices(struct device *dev, void *arg)
>>   {
>> +	unsigned long tmp[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
>> +	struct aggregate_choices_data *data = arg;
>>   	struct platform_profile_handler *handler;
>> -	unsigned long *aggregate = data;
>>   
>>   	lockdep_assert_held(&profile_lock);
>>   	handler = to_pprof_handler(dev);
>> -	if (test_bit(PLATFORM_PROFILE_LAST, aggregate))
>> -		bitmap_copy(aggregate, handler->choices, PLATFORM_PROFILE_LAST);
>> +	bitmap_or(tmp, handler->choices, handler->hidden_choices, PLATFORM_PROFILE_LAST);
>> +	if (test_bit(PLATFORM_PROFILE_LAST, data->aggregate))
>> +		bitmap_copy(data->aggregate, tmp, PLATFORM_PROFILE_LAST);
>>   	else
>> -		bitmap_and(aggregate, handler->choices, aggregate, PLATFORM_PROFILE_LAST);
>> +		bitmap_and(data->aggregate, tmp, data->aggregate, PLATFORM_PROFILE_LAST);
>> +	data->count++;
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * _remove_hidden_choices - Remove hidden choices from aggregate data
>> + * @dev: The device
>> + * @arg: struct aggregate_choices_data
>> + *
>> + * Return: 0 on success, -errno on failure
>> + */
>> +static int _remove_hidden_choices(struct device *dev, void *arg)
>> +{
>> +	struct aggregate_choices_data *data = arg;
>> +	struct platform_profile_handler *handler;
>> +
>> +	lockdep_assert_held(&profile_lock);
>> +	handler = to_pprof_handler(dev);
>> +	bitmap_andnot(data->aggregate, handler->choices,
>> +		      handler->hidden_choices, PLATFORM_PROFILE_LAST);
>>   
>>   	return 0;
>>   }
>> @@ -270,22 +299,31 @@ static ssize_t platform_profile_choices_show(struct device *dev,
>>   					     struct device_attribute *attr,
>>   					     char *buf)
>>   {
>> -	unsigned long aggregate[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
>> +	struct aggregate_choices_data data = {
>> +		.aggregate = { [0 ... BITS_TO_LONGS(PLATFORM_PROFILE_LAST) - 1] = ~0UL },
>> +		.count = 0,
>> +	};
>>   	int err;
>>   
>> -	set_bit(PLATFORM_PROFILE_LAST, aggregate);
>> +	set_bit(PLATFORM_PROFILE_LAST, data.aggregate);
>>   	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
>>   		err = class_for_each_device(&platform_profile_class, NULL,
>> -					    aggregate, _aggregate_choices);
>> +					    &data, _aggregate_choices);
>>   		if (err)
>>   			return err;
>> +		if (data.count == 1) {
>> +			err = class_for_each_device(&platform_profile_class, NULL,
>> +						    &data, _remove_hidden_choices);
>> +			if (err)
>> +				return err;
>> +		}
>>   	}
>>   
>>   	/* no profile handler registered any more */
>> -	if (bitmap_empty(aggregate, PLATFORM_PROFILE_LAST))
>> +	if (bitmap_empty(data.aggregate, PLATFORM_PROFILE_LAST))
>>   		return -EINVAL;
>>   
>> -	return _commmon_choices_show(aggregate, buf);
>> +	return _commmon_choices_show(data.aggregate, buf);
>>   }
>>   
>>   /**
>> @@ -373,7 +411,10 @@ static ssize_t platform_profile_store(struct device *dev,
>>   				      struct device_attribute *attr,
>>   				      const char *buf, size_t count)
>>   {
>> -	unsigned long choices[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
>> +	struct aggregate_choices_data data = {
>> +		.aggregate = { [0 ... BITS_TO_LONGS(PLATFORM_PROFILE_LAST) - 1] = ~0UL },
>> +		.count = 0,
>> +	};
>>   	int ret;
>>   	int i;
>>   
>> @@ -381,13 +422,13 @@ static ssize_t platform_profile_store(struct device *dev,
>>   	i = sysfs_match_string(profile_names, buf);
>>   	if (i < 0 || i == PLATFORM_PROFILE_CUSTOM)
>>   		return -EINVAL;
>> -	set_bit(PLATFORM_PROFILE_LAST, choices);
>> +	set_bit(PLATFORM_PROFILE_LAST, data.aggregate);
>>   	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
>>   		ret = class_for_each_device(&platform_profile_class, NULL,
>> -					    choices, _aggregate_choices);
>> +					    &data, _aggregate_choices);
>>   		if (ret)
>>   			return ret;
>> -		if (!test_bit(i, choices))
>> +		if (!test_bit(i, data.aggregate))
>>   			return -EOPNOTSUPP;
>>   
>>   		ret = class_for_each_device(&platform_profile_class, NULL, &i,
>> @@ -453,12 +494,15 @@ EXPORT_SYMBOL_GPL(platform_profile_notify);
>>    */
>>   int platform_profile_cycle(void)
>>   {
>> +	struct aggregate_choices_data data = {
>> +		.aggregate = { [0 ... BITS_TO_LONGS(PLATFORM_PROFILE_LAST) - 1] = ~0UL },
>> +		.count = 0,
>> +	};
>>   	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);
>> +	set_bit(PLATFORM_PROFILE_LAST, data.aggregate);
>>   	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
>>   		err = class_for_each_device(&platform_profile_class, NULL,
>>   					    &profile, _aggregate_profiles);
>> @@ -470,14 +514,14 @@ int platform_profile_cycle(void)
>>   			return -EINVAL;
>>   
>>   		err = class_for_each_device(&platform_profile_class, NULL,
>> -					    choices, _aggregate_choices);
>> +					    &data, _aggregate_choices);
>>   		if (err)
>>   			return err;
>>   
>>   		/* never iterate into a custom if all drivers supported it */
>> -		clear_bit(PLATFORM_PROFILE_CUSTOM, choices);
>> +		clear_bit(PLATFORM_PROFILE_CUSTOM, data.aggregate);
>>   
>> -		next = find_next_bit_wrap(choices,
>> +		next = find_next_bit_wrap(data.aggregate,
>>   					  PLATFORM_PROFILE_LAST,
>>   					  profile + 1);
>>   
>> @@ -532,6 +576,14 @@ struct device *platform_profile_register(struct device *dev, const char *name,
>>   		return ERR_PTR(-EINVAL);
>>   	}
>>   
>> +	if (ops->hidden_choices) {
>> +		err = ops->hidden_choices(drvdata, pprof->hidden_choices);
>> +		if (err) {
>> +			dev_err(dev, "platform_profile hidden_choices failed\n");
>> +			return ERR_PTR(err);
>> +		}
>> +	}
>> +
>>   	guard(mutex)(&profile_lock);
>>   
>>   	/* create class interface for individual handler */
>> diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h
>> index 8ab5b0e8eb2c1..8c9df7dadd5d3 100644
>> --- a/include/linux/platform_profile.h
>> +++ b/include/linux/platform_profile.h
>> @@ -33,6 +33,8 @@ enum platform_profile_option {
>>    * @probe: Callback to setup choices available to the new class device. These
>>    *	   choices will only be enforced when setting a new profile, not when
>>    *	   getting the current one.
>> + * @hidden_choices: Callback to setup choices that are not visible to the user
>> + *		    but can be set by the driver.
>>    * @profile_get: Callback that will be called when showing the current platform
>>    *		 profile in sysfs.
>>    * @profile_set: Callback that will be called when storing a new platform
>> @@ -40,6 +42,7 @@ enum platform_profile_option {
>>    */
>>   struct platform_profile_ops {
>>   	int (*probe)(void *drvdata, unsigned long *choices);
>> +	int (*hidden_choices)(void *drvdata, unsigned long *choices);
>>   	int (*profile_get)(struct device *dev, enum platform_profile_option *profile);
>>   	int (*profile_set)(struct device *dev, enum platform_profile_option profile);
>>   };
> 
> This approach works really well for the PMF driver because the
> profile_get callback retrieves the raw profile that the profile_set
> callback cached. However this is not the case for quite a few drivers,
> which usually just retrieve the current profile from WMI for example.
> 
> This means that writing a profile to the legacy platform_profile
> attribute, which a driver has selected as a "hidden choice" may result
> in the operation succeeding, but if the user were to immediately read
> from platform_profile it would display "custom", because the profiles
> for different handlers may be unsynchronized.

I guess we need to think about how many other drivers would really need 
hidden choices added.

Is it just PMF?

> 
> This makes me wonder if the added complexity this patch brings, is
> really worth it.
> 
> IMHO we should do what Armin suggested in the patch proposed by Antheas.
> In fact, I would suggest an even simpler version:
> 
>    1. The legacy platform_profile_choices should aggregate `choices`
>       with bitmap_or instead of bitmap_and. i.e. It should display all
>       available choices
>    2. When writing a profile to the legacy platform_profile, if a handler
>       doesn't support it, we simply ignore it without failing and
>       continue to the next
> 
> I believe this works well with power-profiles-daemon, but I'm not
> entirely sure. Maybe you know more about it.
> 
> This of course has the problem that profiles would be unsync and
> platform_profile might display "custom" immediately after setting a
> profile, but this patch has the same "issue".
> 
> For me this "custom" issue, is not really an issue. The legacy interface
> should be deprecated in favor of the class interface, and new/old
> user-space tools should use/migrate to that instead.
> 
> Let me know what you think!

I don't really like that profiles can get out of sync, this is asking 
for a non-deterministic behavior that can be difficult to diagnose 
issues and also difficult for userspace to work with.

Re: [PATCH 1/3] ACPI: platform_profile: Add support for hidden choices

[Antheas Kapenekakis]

On Sat, 1 Mar 2025 at 04:19, Mario Limonciello <superm1@kernel.org> wrote:
>
> On 2/28/2025 16:08, Kurt Borja wrote:
> > Hi Mario,
> >
> > On Fri Feb 28, 2025 at 12:01 PM -05, Mario Limonciello 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.
> >>
> >> To allow two drivers to disagree, add support for "hidden choices".
> >> Hidden choices are platform profiles that a driver supports to be
> >> compatible with the platform profile of another driver.
> >>
> >> 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>
> >> ---
> >> Cc: "Luke D. Jones" <luke@ljones.dev>
> >>   drivers/acpi/platform_profile.c  | 94 +++++++++++++++++++++++++-------
> >>   include/linux/platform_profile.h |  3 +
> >>   2 files changed, 76 insertions(+), 21 deletions(-)
> >>
> >> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
> >> index 2ad53cc6aae53..ef9444482db19 100644
> >> --- a/drivers/acpi/platform_profile.c
> >> +++ b/drivers/acpi/platform_profile.c
> >> @@ -21,9 +21,15 @@ struct platform_profile_handler {
> >>      struct device dev;
> >>      int minor;
> >>      unsigned long choices[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
> >> +    unsigned long hidden_choices[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
> >>      const struct platform_profile_ops *ops;
> >>   };
> >>
> >> +struct aggregate_choices_data {
> >> +    unsigned long aggregate[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
> >> +    int count;
> >> +};
> >> +
> >>   static const char * const profile_names[] = {
> >>      [PLATFORM_PROFILE_LOW_POWER] = "low-power",
> >>      [PLATFORM_PROFILE_COOL] = "cool",
> >> @@ -73,7 +79,7 @@ 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))
> >> +    if (!test_bit(*bit, handler->choices) && !test_bit(*bit, handler->hidden_choices))
> >>              return -EOPNOTSUPP;
> >>
> >>      return handler->ops->profile_set(dev, *bit);
> >> @@ -239,21 +245,44 @@ static const struct class platform_profile_class = {
> >>   /**
> >>    * _aggregate_choices - Aggregate the available profile choices
> >>    * @dev: The device
> >> - * @data: The available profile choices
> >> + * @arg: struct aggregate_choices_data
> >>    *
> >>    * Return: 0 on success, -errno on failure
> >>    */
> >> -static int _aggregate_choices(struct device *dev, void *data)
> >> +static int _aggregate_choices(struct device *dev, void *arg)
> >>   {
> >> +    unsigned long tmp[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
> >> +    struct aggregate_choices_data *data = arg;
> >>      struct platform_profile_handler *handler;
> >> -    unsigned long *aggregate = data;
> >>
> >>      lockdep_assert_held(&profile_lock);
> >>      handler = to_pprof_handler(dev);
> >> -    if (test_bit(PLATFORM_PROFILE_LAST, aggregate))
> >> -            bitmap_copy(aggregate, handler->choices, PLATFORM_PROFILE_LAST);
> >> +    bitmap_or(tmp, handler->choices, handler->hidden_choices, PLATFORM_PROFILE_LAST);
> >> +    if (test_bit(PLATFORM_PROFILE_LAST, data->aggregate))
> >> +            bitmap_copy(data->aggregate, tmp, PLATFORM_PROFILE_LAST);
> >>      else
> >> -            bitmap_and(aggregate, handler->choices, aggregate, PLATFORM_PROFILE_LAST);
> >> +            bitmap_and(data->aggregate, tmp, data->aggregate, PLATFORM_PROFILE_LAST);
> >> +    data->count++;
> >> +
> >> +    return 0;
> >> +}
> >> +
> >> +/**
> >> + * _remove_hidden_choices - Remove hidden choices from aggregate data
> >> + * @dev: The device
> >> + * @arg: struct aggregate_choices_data
> >> + *
> >> + * Return: 0 on success, -errno on failure
> >> + */
> >> +static int _remove_hidden_choices(struct device *dev, void *arg)
> >> +{
> >> +    struct aggregate_choices_data *data = arg;
> >> +    struct platform_profile_handler *handler;
> >> +
> >> +    lockdep_assert_held(&profile_lock);
> >> +    handler = to_pprof_handler(dev);
> >> +    bitmap_andnot(data->aggregate, handler->choices,
> >> +                  handler->hidden_choices, PLATFORM_PROFILE_LAST);
> >>
> >>      return 0;
> >>   }
> >> @@ -270,22 +299,31 @@ static ssize_t platform_profile_choices_show(struct device *dev,
> >>                                           struct device_attribute *attr,
> >>                                           char *buf)
> >>   {
> >> -    unsigned long aggregate[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
> >> +    struct aggregate_choices_data data = {
> >> +            .aggregate = { [0 ... BITS_TO_LONGS(PLATFORM_PROFILE_LAST) - 1] = ~0UL },
> >> +            .count = 0,
> >> +    };
> >>      int err;
> >>
> >> -    set_bit(PLATFORM_PROFILE_LAST, aggregate);
> >> +    set_bit(PLATFORM_PROFILE_LAST, data.aggregate);
> >>      scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
> >>              err = class_for_each_device(&platform_profile_class, NULL,
> >> -                                        aggregate, _aggregate_choices);
> >> +                                        &data, _aggregate_choices);
> >>              if (err)
> >>                      return err;
> >> +            if (data.count == 1) {
> >> +                    err = class_for_each_device(&platform_profile_class, NULL,
> >> +                                                &data, _remove_hidden_choices);
> >> +                    if (err)
> >> +                            return err;
> >> +            }
> >>      }
> >>
> >>      /* no profile handler registered any more */
> >> -    if (bitmap_empty(aggregate, PLATFORM_PROFILE_LAST))
> >> +    if (bitmap_empty(data.aggregate, PLATFORM_PROFILE_LAST))
> >>              return -EINVAL;
> >>
> >> -    return _commmon_choices_show(aggregate, buf);
> >> +    return _commmon_choices_show(data.aggregate, buf);
> >>   }
> >>
> >>   /**
> >> @@ -373,7 +411,10 @@ static ssize_t platform_profile_store(struct device *dev,
> >>                                    struct device_attribute *attr,
> >>                                    const char *buf, size_t count)
> >>   {
> >> -    unsigned long choices[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
> >> +    struct aggregate_choices_data data = {
> >> +            .aggregate = { [0 ... BITS_TO_LONGS(PLATFORM_PROFILE_LAST) - 1] = ~0UL },
> >> +            .count = 0,
> >> +    };
> >>      int ret;
> >>      int i;
> >>
> >> @@ -381,13 +422,13 @@ static ssize_t platform_profile_store(struct device *dev,
> >>      i = sysfs_match_string(profile_names, buf);
> >>      if (i < 0 || i == PLATFORM_PROFILE_CUSTOM)
> >>              return -EINVAL;
> >> -    set_bit(PLATFORM_PROFILE_LAST, choices);
> >> +    set_bit(PLATFORM_PROFILE_LAST, data.aggregate);
> >>      scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
> >>              ret = class_for_each_device(&platform_profile_class, NULL,
> >> -                                        choices, _aggregate_choices);
> >> +                                        &data, _aggregate_choices);
> >>              if (ret)
> >>                      return ret;
> >> -            if (!test_bit(i, choices))
> >> +            if (!test_bit(i, data.aggregate))
> >>                      return -EOPNOTSUPP;
> >>
> >>              ret = class_for_each_device(&platform_profile_class, NULL, &i,
> >> @@ -453,12 +494,15 @@ EXPORT_SYMBOL_GPL(platform_profile_notify);
> >>    */
> >>   int platform_profile_cycle(void)
> >>   {
> >> +    struct aggregate_choices_data data = {
> >> +            .aggregate = { [0 ... BITS_TO_LONGS(PLATFORM_PROFILE_LAST) - 1] = ~0UL },
> >> +            .count = 0,
> >> +    };
> >>      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);
> >> +    set_bit(PLATFORM_PROFILE_LAST, data.aggregate);
> >>      scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
> >>              err = class_for_each_device(&platform_profile_class, NULL,
> >>                                          &profile, _aggregate_profiles);
> >> @@ -470,14 +514,14 @@ int platform_profile_cycle(void)
> >>                      return -EINVAL;
> >>
> >>              err = class_for_each_device(&platform_profile_class, NULL,
> >> -                                        choices, _aggregate_choices);
> >> +                                        &data, _aggregate_choices);
> >>              if (err)
> >>                      return err;
> >>
> >>              /* never iterate into a custom if all drivers supported it */
> >> -            clear_bit(PLATFORM_PROFILE_CUSTOM, choices);
> >> +            clear_bit(PLATFORM_PROFILE_CUSTOM, data.aggregate);
> >>
> >> -            next = find_next_bit_wrap(choices,
> >> +            next = find_next_bit_wrap(data.aggregate,
> >>                                        PLATFORM_PROFILE_LAST,
> >>                                        profile + 1);
> >>
> >> @@ -532,6 +576,14 @@ struct device *platform_profile_register(struct device *dev, const char *name,
> >>              return ERR_PTR(-EINVAL);
> >>      }
> >>
> >> +    if (ops->hidden_choices) {
> >> +            err = ops->hidden_choices(drvdata, pprof->hidden_choices);
> >> +            if (err) {
> >> +                    dev_err(dev, "platform_profile hidden_choices failed\n");
> >> +                    return ERR_PTR(err);
> >> +            }
> >> +    }
> >> +
> >>      guard(mutex)(&profile_lock);
> >>
> >>      /* create class interface for individual handler */
> >> diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h
> >> index 8ab5b0e8eb2c1..8c9df7dadd5d3 100644
> >> --- a/include/linux/platform_profile.h
> >> +++ b/include/linux/platform_profile.h
> >> @@ -33,6 +33,8 @@ enum platform_profile_option {
> >>    * @probe: Callback to setup choices available to the new class device. These
> >>    *    choices will only be enforced when setting a new profile, not when
> >>    *    getting the current one.
> >> + * @hidden_choices: Callback to setup choices that are not visible to the user
> >> + *              but can be set by the driver.
> >>    * @profile_get: Callback that will be called when showing the current platform
> >>    *          profile in sysfs.
> >>    * @profile_set: Callback that will be called when storing a new platform
> >> @@ -40,6 +42,7 @@ enum platform_profile_option {
> >>    */
> >>   struct platform_profile_ops {
> >>      int (*probe)(void *drvdata, unsigned long *choices);
> >> +    int (*hidden_choices)(void *drvdata, unsigned long *choices);
> >>      int (*profile_get)(struct device *dev, enum platform_profile_option *profile);
> >>      int (*profile_set)(struct device *dev, enum platform_profile_option profile);
> >>   };
> >
> > This approach works really well for the PMF driver because the
> > profile_get callback retrieves the raw profile that the profile_set
> > callback cached. However this is not the case for quite a few drivers,
> > which usually just retrieve the current profile from WMI for example.
> >
> > This means that writing a profile to the legacy platform_profile
> > attribute, which a driver has selected as a "hidden choice" may result
> > in the operation succeeding, but if the user were to immediately read
> > from platform_profile it would display "custom", because the profiles
> > for different handlers may be unsynchronized.
>
> I guess we need to think about how many other drivers would really need
> hidden choices added.
>
> Is it just PMF?
>
> >
> > This makes me wonder if the added complexity this patch brings, is
> > really worth it.
> >
> > IMHO we should do what Armin suggested in the patch proposed by Antheas.
> > In fact, I would suggest an even simpler version:
> >
> >    1. The legacy platform_profile_choices should aggregate `choices`
> >       with bitmap_or instead of bitmap_and. i.e. It should display all
> >       available choices
> >    2. When writing a profile to the legacy platform_profile, if a handler
> >       doesn't support it, we simply ignore it without failing and
> >       continue to the next
> >
> > I believe this works well with power-profiles-daemon, but I'm not
> > entirely sure. Maybe you know more about it.
> >
> > This of course has the problem that profiles would be unsync and
> > platform_profile might display "custom" immediately after setting a
> > profile, but this patch has the same "issue".
> >
> > For me this "custom" issue, is not really an issue. The legacy interface
> > should be deprecated in favor of the class interface, and new/old
> > user-space tools should use/migrate to that instead.
> >
> > Let me know what you think!
>
> I don't really like that profiles can get out of sync, this is asking
> for a non-deterministic behavior that can be difficult to diagnose
> issues and also difficult for userspace to work with.

I agree with Mario here. Imagine two drivers, one with low-power and
one with quiet. They both begin at performance.

Then, userspace software gets confused (incl. ppd) and sets firmware
profile to low-power. The latter gets left in performance, causing
excess drain.

I do not believe the legacy interface should be deprecated. Right now,
amd-pmf is a NOOP in most devices so there is actually 0 reason for
generic power handlers to move to the new API. Just extra work. So
lets make sure the legacy endpoint works properly for the foreseeable
future.

Also, when power handlers start moving to the new interface, they will
hardcode choices based on the name. As they should. TDP needs to be
customized per device/manufacturer. So moving handlers between
low-power and quiet will not be possible.

@Mario: I do not have a device with an amd-pmf integration. All of
mine have stub handlers. I would expect that a properly configured pmf
handler for e.g., Asus would do the same as the armoury interface, so
that users do not have to rely to vendor software on WIndows. Then
power profiles would be synced between windows and armoury. In that
case, we have a problem of setting the power mode twice. What would be
the mitigation for something like that?

Antheas

Re: [PATCH 1/3] ACPI: platform_profile: Add support for hidden choices

[Mario Limonciello]

>>> Let me know what you think!
>>
>> I don't really like that profiles can get out of sync, this is asking
>> for a non-deterministic behavior that can be difficult to diagnose
>> issues and also difficult for userspace to work with.
> 
> I agree with Mario here. Imagine two drivers, one with low-power and
> one with quiet. They both begin at performance.
> 
> Then, userspace software gets confused (incl. ppd) and sets firmware
> profile to low-power. The latter gets left in performance, causing
> excess drain.
> 
> I do not believe the legacy interface should be deprecated. Right now,
> amd-pmf is a NOOP in most devices 

"Most" devices is not accurate.  There are a lot of devices that it does 
enable.  In the gaming space right now it's often behaving as a no-op.

> so there is actually 0 reason for
> generic power handlers to move to the new API. Just extra work. So
> lets make sure the legacy endpoint works properly for the foreseeable
> future.
> 
> Also, when power handlers start moving to the new interface, they will
> hardcode choices based on the name. As they should. TDP needs to be
> customized per device/manufacturer. So moving handlers between
> low-power and quiet will not be possible.
> 
> @Mario: I do not have a device with an amd-pmf integration. All of
> mine have stub handlers. I would expect that a properly configured pmf
> handler for e.g., Asus would do the same as the armoury interface, so
> that users do not have to rely to vendor software on WIndows. Then
> power profiles would be synced between windows and armoury. In that
> case, we have a problem of setting the power mode twice. What would be
> the mitigation for something like that?
> 
> Antheas

"Power mode" is a concept, it doesn't just apply to configuring sPPT and 
fPPT.  I envisage that a vendor that actively uses PMF and their own 
interface would be changing different things by the different interfaces.

For "example" PMF may reconfigure sPPT, fPPT, STT and STAPM but their 
driver may notify their EC to change a fan curve.

If we really end up with a situation that vendor interface and PMF do 
the same thing we can cross that bridge then.

Re: [PATCH 1/3] ACPI: platform_profile: Add support for hidden choices

[Antheas Kapenekakis]

On Sat, 1 Mar 2025 at 14:52, Mario Limonciello <superm1@kernel.org> wrote:
>
> >>> Let me know what you think!
> >>
> >> I don't really like that profiles can get out of sync, this is asking
> >> for a non-deterministic behavior that can be difficult to diagnose
> >> issues and also difficult for userspace to work with.
> >
> > I agree with Mario here. Imagine two drivers, one with low-power and
> > one with quiet. They both begin at performance.
> >
> > Then, userspace software gets confused (incl. ppd) and sets firmware
> > profile to low-power. The latter gets left in performance, causing
> > excess drain.
> >
> > I do not believe the legacy interface should be deprecated. Right now,
> > amd-pmf is a NOOP in most devices
>
> "Most" devices is not accurate.  There are a lot of devices that it does
> enable.  In the gaming space right now it's often behaving as a no-op.

That would be a fair description. Can you give some examples of
devices that use the interface? Devices with and without vendor
software.

> > so there is actually 0 reason for
> > generic power handlers to move to the new API. Just extra work. So
> > lets make sure the legacy endpoint works properly for the foreseeable
> > future.
> >
> > Also, when power handlers start moving to the new interface, they will
> > hardcode choices based on the name. As they should. TDP needs to be
> > customized per device/manufacturer. So moving handlers between
> > low-power and quiet will not be possible.
> >
> > @Mario: I do not have a device with an amd-pmf integration. All of
> > mine have stub handlers. I would expect that a properly configured pmf
> > handler for e.g., Asus would do the same as the armoury interface, so
> > that users do not have to rely to vendor software on WIndows. Then
> > power profiles would be synced between windows and armoury. In that
> > case, we have a problem of setting the power mode twice. What would be
> > the mitigation for something like that?
> >
> > Antheas
>
> "Power mode" is a concept, it doesn't just apply to configuring sPPT and
> fPPT.  I envisage that a vendor that actively uses PMF and their own
> interface would be changing different things by the different interfaces.
>
> For "example" PMF may reconfigure sPPT, fPPT, STT and STAPM but their
> driver may notify their EC to change a fan curve.

No. If PMF changes these values it also needs to change the fan curve
itself via the BIOS notification. Doing otherwise would lead to
situations where users do not install the vendor driver and cook their
device. So I expect that when PMF controls things it controls
everything. I would expect if vendors fallback to the pmf firmware
notifications while also providing vendor software there would be some
synergy between them, such as changing which fan preset is selected by
the PMF interface.

> If we really end up with a situation that vendor interface and PMF do
> the same thing we can cross that bridge then.

Re: [PATCH 1/3] ACPI: platform_profile: Add support for hidden choices

[Mario Limonciello]

On 3/1/25 08:06, Antheas Kapenekakis wrote:
> On Sat, 1 Mar 2025 at 14:52, Mario Limonciello <superm1@kernel.org> wrote:
>>
>>>>> Let me know what you think!
>>>>
>>>> I don't really like that profiles can get out of sync, this is asking
>>>> for a non-deterministic behavior that can be difficult to diagnose
>>>> issues and also difficult for userspace to work with.
>>>
>>> I agree with Mario here. Imagine two drivers, one with low-power and
>>> one with quiet. They both begin at performance.
>>>
>>> Then, userspace software gets confused (incl. ppd) and sets firmware
>>> profile to low-power. The latter gets left in performance, causing
>>> excess drain.
>>>
>>> I do not believe the legacy interface should be deprecated. Right now,
>>> amd-pmf is a NOOP in most devices
>>
>> "Most" devices is not accurate.  There are a lot of devices that it does
>> enable.  In the gaming space right now it's often behaving as a no-op.
> 
> That would be a fair description. Can you give some examples of
> devices that use the interface? Devices with and without vendor
> software.

Off hand the Framework 13 and 16 AMD both use PMF exclusively.  So do a 
bunch of HP commercial laptops.

Mark can keep me honest, but I want to say the Strix Thinkpad laptops 
have both PMF and vendor interface (thinkpad-acpi).
  >>
>> "Power mode" is a concept, it doesn't just apply to configuring sPPT and
>> fPPT.  I envisage that a vendor that actively uses PMF and their own
>> interface would be changing different things by the different interfaces.
>>
>> For "example" PMF may reconfigure sPPT, fPPT, STT and STAPM but their
>> driver may notify their EC to change a fan curve.
> 
> No. If PMF changes these values it also needs to change the fan curve
> itself via the BIOS notification. Doing otherwise would lead to
> situations where users do not install the vendor driver and cook their
> device. 

Fan curves are just that; curves.  They just control how quickly fans 
ramp up not whether or not they "work".

But in any case; that's a firmware issue not a platform profile design 
issue.

> So I expect that when PMF controls things it controls
> everything. I would expect if vendors fallback to the pmf firmware
> notifications while also providing vendor software there would be some
> synergy between them, such as changing which fan preset is selected by
> the PMF interface.
> 

I can't control what vendors do; it's their decision how to manage their 
systems.  All I can do is provide infrastructure to help.

Re: [PATCH 1/3] ACPI: platform_profile: Add support for hidden choices

[Antheas Kapenekakis]

On Sat, 1 Mar 2025 at 17:04, Mario Limonciello <superm1@kernel.org> wrote:
>
>
>
> On 3/1/25 08:06, Antheas Kapenekakis wrote:
> > On Sat, 1 Mar 2025 at 14:52, Mario Limonciello <superm1@kernel.org> wrote:
> >>
> >>>>> Let me know what you think!
> >>>>
> >>>> I don't really like that profiles can get out of sync, this is asking
> >>>> for a non-deterministic behavior that can be difficult to diagnose
> >>>> issues and also difficult for userspace to work with.
> >>>
> >>> I agree with Mario here. Imagine two drivers, one with low-power and
> >>> one with quiet. They both begin at performance.
> >>>
> >>> Then, userspace software gets confused (incl. ppd) and sets firmware
> >>> profile to low-power. The latter gets left in performance, causing
> >>> excess drain.
> >>>
> >>> I do not believe the legacy interface should be deprecated. Right now,
> >>> amd-pmf is a NOOP in most devices
> >>
> >> "Most" devices is not accurate.  There are a lot of devices that it does
> >> enable.  In the gaming space right now it's often behaving as a no-op.
> >
> > That would be a fair description. Can you give some examples of
> > devices that use the interface? Devices with and without vendor
> > software.
>
> Off hand the Framework 13 and 16 AMD both use PMF exclusively.  So do a
> bunch of HP commercial laptops.

I will ask Kyle to check it out.

> Mark can keep me honest, but I want to say the Strix Thinkpad laptops
> have both PMF and vendor interface (thinkpad-acpi).

Hm, yeah that would be interesting to hear about

>   >>
> >> "Power mode" is a concept, it doesn't just apply to configuring sPPT and
> >> fPPT.  I envisage that a vendor that actively uses PMF and their own
> >> interface would be changing different things by the different interfaces.
> >>
> >> For "example" PMF may reconfigure sPPT, fPPT, STT and STAPM but their
> >> driver may notify their EC to change a fan curve.
> >
> > No. If PMF changes these values it also needs to change the fan curve
> > itself via the BIOS notification. Doing otherwise would lead to
> > situations where users do not install the vendor driver and cook their
> > device.
>
> Fan curves are just that; curves.  They just control how quickly fans
> ramp up not whether or not they "work".

The APU reaches a similar temperature (Tctl) across a wide TDP range,
so temperature cannot be used on its own to determine fan speed.
Manufacturers that provide different fan curves depending on the TDP
mode usually cap the maximum fan speed on low TDPs. So you can get
funny situations where the device is set to 30W, but the fan runs as
if its using 10W leading to thermal soaking. So it is very important
for those to be inline.

> But in any case; that's a firmware issue not a platform profile design
> issue.

It would be a hypothetical scenario. I do not expect such a device to exist.

> > So I expect that when PMF controls things it controls
> > everything. I would expect if vendors fallback to the pmf firmware
> > notifications while also providing vendor software there would be some
> > synergy between them, such as changing which fan preset is selected by
> > the PMF interface.
> >
>
> I can't control what vendors do; it's their decision how to manage their
> systems.  All I can do is provide infrastructure to help.

This was more of my intuition of how I would expect amd-pmf
integration to be done in Windows where one of the drivers might be
missing.

Since only thinkpads are expected to do both, perhaps Mark can check
out how they work. I have a thinkpad that is 11th gen intel.

Antheas

Re: [PATCH 1/3] ACPI: platform_profile: Add support for hidden choices

[Mark Pearson]

On Sat, Mar 1, 2025, at 11:15 AM, Antheas Kapenekakis wrote:
> On Sat, 1 Mar 2025 at 17:04, Mario Limonciello <superm1@kernel.org> wrote:
>>
>>
>>
>> On 3/1/25 08:06, Antheas Kapenekakis wrote:
>> > On Sat, 1 Mar 2025 at 14:52, Mario Limonciello <superm1@kernel.org> wrote:
>> >>
>> >>>>> Let me know what you think!
>> >>>>
>> >>>> I don't really like that profiles can get out of sync, this is asking
>> >>>> for a non-deterministic behavior that can be difficult to diagnose
>> >>>> issues and also difficult for userspace to work with.
>> >>>
>> >>> I agree with Mario here. Imagine two drivers, one with low-power and
>> >>> one with quiet. They both begin at performance.
>> >>>
>> >>> Then, userspace software gets confused (incl. ppd) and sets firmware
>> >>> profile to low-power. The latter gets left in performance, causing
>> >>> excess drain.
>> >>>
>> >>> I do not believe the legacy interface should be deprecated. Right now,
>> >>> amd-pmf is a NOOP in most devices
>> >>
>> >> "Most" devices is not accurate.  There are a lot of devices that it does
>> >> enable.  In the gaming space right now it's often behaving as a no-op.
>> >
>> > That would be a fair description. Can you give some examples of
>> > devices that use the interface? Devices with and without vendor
>> > software.
>>
>> Off hand the Framework 13 and 16 AMD both use PMF exclusively.  So do a
>> bunch of HP commercial laptops.
>
> I will ask Kyle to check it out.
>
>> Mark can keep me honest, but I want to say the Strix Thinkpad laptops
>> have both PMF and vendor interface (thinkpad-acpi).
>
> Hm, yeah that would be interesting to hear about
>

Yep, support both.

>>   >>
>> >> "Power mode" is a concept, it doesn't just apply to configuring sPPT and
>> >> fPPT.  I envisage that a vendor that actively uses PMF and their own
>> >> interface would be changing different things by the different interfaces.
>> >>
>> >> For "example" PMF may reconfigure sPPT, fPPT, STT and STAPM but their
>> >> driver may notify their EC to change a fan curve.
>> >
>> > No. If PMF changes these values it also needs to change the fan curve
>> > itself via the BIOS notification. Doing otherwise would lead to
>> > situations where users do not install the vendor driver and cook their
>> > device.
>>
>> Fan curves are just that; curves.  They just control how quickly fans
>> ramp up not whether or not they "work".
>
> The APU reaches a similar temperature (Tctl) across a wide TDP range,
> so temperature cannot be used on its own to determine fan speed.
> Manufacturers that provide different fan curves depending on the TDP
> mode usually cap the maximum fan speed on low TDPs. So you can get
> funny situations where the device is set to 30W, but the fan runs as
> if its using 10W leading to thermal soaking. So it is very important
> for those to be inline.
>
>> But in any case; that's a firmware issue not a platform profile design
>> issue.
>
> It would be a hypothetical scenario. I do not expect such a device to exist.
>
>> > So I expect that when PMF controls things it controls
>> > everything. I would expect if vendors fallback to the pmf firmware
>> > notifications while also providing vendor software there would be some
>> > synergy between them, such as changing which fan preset is selected by
>> > the PMF interface.
>> >
>>
>> I can't control what vendors do; it's their decision how to manage their
>> systems.  All I can do is provide infrastructure to help.
>
> This was more of my intuition of how I would expect amd-pmf
> integration to be done in Windows where one of the drivers might be
> missing.
>
> Since only thinkpads are expected to do both, perhaps Mark can check
> out how they work. I have a thinkpad that is 11th gen intel.
>

I'll do some checking next week (away this weekend). Mario will ping you offline for best testing to do.

Mark

[PATCH 2/3] platform/x86/amd: pmf: Add 'quiet' to hidden choices

[Mario Limonciello]

From: Mario Limonciello <mario.limonciello@amd.com>

When amd-pmf and asus-wmi are both bound no low power option shows
up in sysfs.  Add a hidden choice for amd-pmf to support 'quiet' mode
to let both bind.

Fixes: 688834743d67 ("ACPI: platform_profile: Allow multiple handlers")
Suggested-by: Antheas Kapenekakis <lkml@antheas.dev>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
Cc: "Luke D. Jones" <luke@ljones.dev>
 drivers/platform/x86/amd/pmf/sps.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/platform/x86/amd/pmf/sps.c b/drivers/platform/x86/amd/pmf/sps.c
index e6cf0b22dac33..3a0079c17cb17 100644
--- a/drivers/platform/x86/amd/pmf/sps.c
+++ b/drivers/platform/x86/amd/pmf/sps.c
@@ -303,6 +303,7 @@ int amd_pmf_get_pprof_modes(struct amd_pmf_dev *pmf)
 		mode = POWER_MODE_BALANCED_POWER;
 		break;
 	case PLATFORM_PROFILE_LOW_POWER:
+	case PLATFORM_PROFILE_QUIET:
 		mode = POWER_MODE_POWER_SAVER;
 		break;
 	default:
@@ -387,6 +388,13 @@ static int amd_pmf_profile_set(struct device *dev,
 	return 0;
 }
 
+static int amd_pmf_hidden_choices(void *drvdata, unsigned long *choices)
+{
+	set_bit(PLATFORM_PROFILE_QUIET, choices);
+
+	return 0;
+}
+
 static int amd_pmf_profile_probe(void *drvdata, unsigned long *choices)
 {
 	set_bit(PLATFORM_PROFILE_LOW_POWER, choices);
@@ -398,6 +406,7 @@ static int amd_pmf_profile_probe(void *drvdata, unsigned long *choices)
 
 static const struct platform_profile_ops amd_pmf_profile_ops = {
 	.probe = amd_pmf_profile_probe,
+	.hidden_choices = amd_pmf_hidden_choices,
 	.profile_get = amd_pmf_profile_get,
 	.profile_set = amd_pmf_profile_set,
 };
-- 
2.43.0

[PATCH 3/3] platform/x86/amd: pmf: Add balanced-performance to hidden choices

[Mario Limonciello]

From: Mario Limonciello <mario.limonciello@amd.com>

Acer's WMI driver uses balanced-performance but AMD-PMF doesn't.
In case a machine binds with both drivers let amd-pmf use
balanced-performance as well.

Fixes: 688834743d67 ("ACPI: platform_profile: Allow multiple handlers")
Suggested-by: Antheas Kapenekakis <lkml@antheas.dev>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
Cc: "Luke D. Jones" <luke@ljones.dev>
 drivers/platform/x86/amd/pmf/sps.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/platform/x86/amd/pmf/sps.c b/drivers/platform/x86/amd/pmf/sps.c
index 3a0079c17cb17..d3083383f11fb 100644
--- a/drivers/platform/x86/amd/pmf/sps.c
+++ b/drivers/platform/x86/amd/pmf/sps.c
@@ -297,6 +297,7 @@ int amd_pmf_get_pprof_modes(struct amd_pmf_dev *pmf)
 
 	switch (pmf->current_profile) {
 	case PLATFORM_PROFILE_PERFORMANCE:
+	case PLATFORM_PROFILE_BALANCED_PERFORMANCE:
 		mode = POWER_MODE_PERFORMANCE;
 		break;
 	case PLATFORM_PROFILE_BALANCED:
@@ -391,6 +392,7 @@ static int amd_pmf_profile_set(struct device *dev,
 static int amd_pmf_hidden_choices(void *drvdata, unsigned long *choices)
 {
 	set_bit(PLATFORM_PROFILE_QUIET, choices);
+	set_bit(PLATFORM_PROFILE_BALANCED_PERFORMANCE, choices);
 
 	return 0;
 }
-- 
2.43.0

Re: [PATCH 0/3] Add support for hidden choices to platform_profile

[Mark Pearson]

Hi Mario,

On Fri, Feb 28, 2025, at 12:01 PM, Mario Limonciello wrote:
> From: Mario Limonciello <mario.limonciello@amd.com>
>
> When two drivers provide platform profile handlers but use different
> strings to mean (essentially) the same thing the legacy interface won't
> export them because it only shows profiles common to multiple drivers.
>
> This causes an unexpected behavior to people who have upgraded from an
> earlier kernel because if multiple drivers have bound platform profile
> handlers they might not be able to access profiles they were expecting.
>
> Introduce a concept of a "hidden choice" that drivers can register and
> the platform profile handler code will utilize when using the legacy
> interface.
>
> There have been some other attempts at solving this issue in other ways.
> This serves as an alternative to those attempts.
>
> Link: 
> https://lore.kernel.org/platform-driver-x86/e64b771e-3255-42ad-9257-5b8fc6c24ac9@gmx.de/T/#t
> Link: 
> https://lore.kernel.org/platform-driver-x86/CAGwozwF-WVEgiAbWbRCiUaXf=BVa3KqmMJfs06trdMQHpTGmjQ@mail.gmail.com/T/#m2f3929e2d4f73cc0eedd14738170dad45232fd18
> Cc: Antheas Kapenekakis <lkml@antheas.dev>
> Cc: "Luke D. Jones" <luke@ljones.dev>
>
> Mario Limonciello (3):
>   ACPI: platform_profile: Add support for hidden choices
>   platform/x86/amd: pmf: Add 'quiet' to hidden choices
>   platform/x86/amd: pmf: Add balanced-performance to hidden choices
>
>  drivers/acpi/platform_profile.c    | 94 +++++++++++++++++++++++-------
>  drivers/platform/x86/amd/pmf/sps.c | 11 ++++
>  include/linux/platform_profile.h   |  3 +
>  3 files changed, 87 insertions(+), 21 deletions(-)
>
> -- 
> 2.43.0

The patches are all good - but my question is do we really need the whole hidden implementation bit?

If the options are not hidden, and someone chooses quiet or balanced-performance for the amd-pmf driver - does it really matter that it's going to do the same as low-power or performance?

So, same feedback as I had for Antheas's patches. I understand why this is being proposed but for me it is making things unnecessarily complicated.

My personal vote remains that the amd_pmf driver carries the superset to keep everyone happy (sorry - it sucks to be the CPU vendor that has to play nice with everyone).

Mark

Re: [PATCH 0/3] Add support for hidden choices to platform_profile

[Mario Limonciello]

On 2/28/2025 13:39, Mark Pearson wrote:
> Hi Mario,
> 
> On Fri, Feb 28, 2025, at 12:01 PM, Mario Limonciello wrote:
>> From: Mario Limonciello <mario.limonciello@amd.com>
>>
>> When two drivers provide platform profile handlers but use different
>> strings to mean (essentially) the same thing the legacy interface won't
>> export them because it only shows profiles common to multiple drivers.
>>
>> This causes an unexpected behavior to people who have upgraded from an
>> earlier kernel because if multiple drivers have bound platform profile
>> handlers they might not be able to access profiles they were expecting.
>>
>> Introduce a concept of a "hidden choice" that drivers can register and
>> the platform profile handler code will utilize when using the legacy
>> interface.
>>
>> There have been some other attempts at solving this issue in other ways.
>> This serves as an alternative to those attempts.
>>
>> Link:
>> https://lore.kernel.org/platform-driver-x86/e64b771e-3255-42ad-9257-5b8fc6c24ac9@gmx.de/T/#t
>> Link:
>> https://lore.kernel.org/platform-driver-x86/CAGwozwF-WVEgiAbWbRCiUaXf=BVa3KqmMJfs06trdMQHpTGmjQ@mail.gmail.com/T/#m2f3929e2d4f73cc0eedd14738170dad45232fd18
>> Cc: Antheas Kapenekakis <lkml@antheas.dev>
>> Cc: "Luke D. Jones" <luke@ljones.dev>
>>
>> Mario Limonciello (3):
>>    ACPI: platform_profile: Add support for hidden choices
>>    platform/x86/amd: pmf: Add 'quiet' to hidden choices
>>    platform/x86/amd: pmf: Add balanced-performance to hidden choices
>>
>>   drivers/acpi/platform_profile.c    | 94 +++++++++++++++++++++++-------
>>   drivers/platform/x86/amd/pmf/sps.c | 11 ++++
>>   include/linux/platform_profile.h   |  3 +
>>   3 files changed, 87 insertions(+), 21 deletions(-)
>>
>> -- 
>> 2.43.0
> 
> The patches are all good - but my question is do we really need the whole hidden implementation bit?
> 
> If the options are not hidden, and someone chooses quiet or balanced-performance for the amd-pmf driver - does it really matter that it's going to do the same as low-power or performance?
> 
> So, same feedback as I had for Antheas's patches. I understand why this is being proposed but for me it is making things unnecessarily complicated.
> 
> My personal vote remains that the amd_pmf driver carries the superset to keep everyone happy (sorry - it sucks to be the CPU vendor that has to play nice with everyone).
> 
> Mark

Well so the problem with having all of them is specifically what happens 
when "only" amd-pmf is bound?

If you advertise both "low power" and "quiet" it's really confusing to 
userspace what the difference is.

The fact that it's actually 100% the same brings me to my personal 
opinion on all of this.  Although I spent time writing up this series to 
do it this way my "preference" is that we permanently alias "low power" 
and "quiet" to one another and update all drivers to use "low power" 
instead.

Granted that doesn't help the case of balance-performance being hidden 
that Antheas mentioned for acer-wmi and legion-wmi but I don't know 
serious of a problem that actually is.

Re: [PATCH 0/3] Add support for hidden choices to platform_profile

[Antheas Kapenekakis]

On Fri, 28 Feb 2025 at 20:45, Mario Limonciello <superm1@kernel.org> wrote:
>
> On 2/28/2025 13:39, Mark Pearson wrote:
> > Hi Mario,
> >
> > On Fri, Feb 28, 2025, at 12:01 PM, Mario Limonciello wrote:
> >> From: Mario Limonciello <mario.limonciello@amd.com>
> >>
> >> When two drivers provide platform profile handlers but use different
> >> strings to mean (essentially) the same thing the legacy interface won't
> >> export them because it only shows profiles common to multiple drivers.
> >>
> >> This causes an unexpected behavior to people who have upgraded from an
> >> earlier kernel because if multiple drivers have bound platform profile
> >> handlers they might not be able to access profiles they were expecting.
> >>
> >> Introduce a concept of a "hidden choice" that drivers can register and
> >> the platform profile handler code will utilize when using the legacy
> >> interface.
> >>
> >> There have been some other attempts at solving this issue in other ways.
> >> This serves as an alternative to those attempts.
> >>
> >> Link:
> >> https://lore.kernel.org/platform-driver-x86/e64b771e-3255-42ad-9257-5b8fc6c24ac9@gmx.de/T/#t
> >> Link:
> >> https://lore.kernel.org/platform-driver-x86/CAGwozwF-WVEgiAbWbRCiUaXf=BVa3KqmMJfs06trdMQHpTGmjQ@mail.gmail.com/T/#m2f3929e2d4f73cc0eedd14738170dad45232fd18
> >> Cc: Antheas Kapenekakis <lkml@antheas.dev>
> >> Cc: "Luke D. Jones" <luke@ljones.dev>
> >>
> >> Mario Limonciello (3):
> >>    ACPI: platform_profile: Add support for hidden choices
> >>    platform/x86/amd: pmf: Add 'quiet' to hidden choices
> >>    platform/x86/amd: pmf: Add balanced-performance to hidden choices
> >>
> >>   drivers/acpi/platform_profile.c    | 94 +++++++++++++++++++++++-------
> >>   drivers/platform/x86/amd/pmf/sps.c | 11 ++++
> >>   include/linux/platform_profile.h   |  3 +
> >>   3 files changed, 87 insertions(+), 21 deletions(-)
> >>
> >> --
> >> 2.43.0
> >
> > The patches are all good - but my question is do we really need the whole hidden implementation bit?
> >
> > If the options are not hidden, and someone chooses quiet or balanced-performance for the amd-pmf driver - does it really matter that it's going to do the same as low-power or performance?
> >
> > So, same feedback as I had for Antheas's patches. I understand why this is being proposed but for me it is making things unnecessarily complicated.
> >
> > My personal vote remains that the amd_pmf driver carries the superset to keep everyone happy (sorry - it sucks to be the CPU vendor that has to play nice with everyone).
> >
> > Mark
>
> Well so the problem with having all of them is specifically what happens
> when "only" amd-pmf is bound?
>
> If you advertise both "low power" and "quiet" it's really confusing to
> userspace what the difference is.
>
> The fact that it's actually 100% the same brings me to my personal
> opinion on all of this.  Although I spent time writing up this series to
> do it this way my "preference" is that we permanently alias "low power"
> and "quiet" to one another and update all drivers to use "low power"
> instead.
>
> Granted that doesn't help the case of balance-performance being hidden
> that Antheas mentioned for acer-wmi and legion-wmi but I don't know
> serious of a problem that actually is.

Hi Mark,
So I agree with Mario here on that. I actually made the patch you
suggested last Sunday [1].

But then I looked at it and thought to myself that I can't ship this,
so I did a v2, which is what I sent on Tuesday.

My priority here is to not disrupt the existing ABI with 6.14. This
would allow extending this discussion by a couple of weeks, so a more
permanent solution can be found.

If that would be the case, perhaps my patch series is more preferable
as, since it is smaller, it would be cleaner to revert.

Antheas

[1] https://github.com/hhd-dev/patchwork/commit/aae724e8c90da3605bd131672fea07cd866af55f

Re: [PATCH 0/3] Add support for hidden choices to platform_profile

[Mario Limonciello]

On 2/28/2025 13:53, Antheas Kapenekakis wrote:
> On Fri, 28 Feb 2025 at 20:45, Mario Limonciello <superm1@kernel.org> wrote:
>>
>> On 2/28/2025 13:39, Mark Pearson wrote:
>>> Hi Mario,
>>>
>>> On Fri, Feb 28, 2025, at 12:01 PM, Mario Limonciello wrote:
>>>> From: Mario Limonciello <mario.limonciello@amd.com>
>>>>
>>>> When two drivers provide platform profile handlers but use different
>>>> strings to mean (essentially) the same thing the legacy interface won't
>>>> export them because it only shows profiles common to multiple drivers.
>>>>
>>>> This causes an unexpected behavior to people who have upgraded from an
>>>> earlier kernel because if multiple drivers have bound platform profile
>>>> handlers they might not be able to access profiles they were expecting.
>>>>
>>>> Introduce a concept of a "hidden choice" that drivers can register and
>>>> the platform profile handler code will utilize when using the legacy
>>>> interface.
>>>>
>>>> There have been some other attempts at solving this issue in other ways.
>>>> This serves as an alternative to those attempts.
>>>>
>>>> Link:
>>>> https://lore.kernel.org/platform-driver-x86/e64b771e-3255-42ad-9257-5b8fc6c24ac9@gmx.de/T/#t
>>>> Link:
>>>> https://lore.kernel.org/platform-driver-x86/CAGwozwF-WVEgiAbWbRCiUaXf=BVa3KqmMJfs06trdMQHpTGmjQ@mail.gmail.com/T/#m2f3929e2d4f73cc0eedd14738170dad45232fd18
>>>> Cc: Antheas Kapenekakis <lkml@antheas.dev>
>>>> Cc: "Luke D. Jones" <luke@ljones.dev>
>>>>
>>>> Mario Limonciello (3):
>>>>     ACPI: platform_profile: Add support for hidden choices
>>>>     platform/x86/amd: pmf: Add 'quiet' to hidden choices
>>>>     platform/x86/amd: pmf: Add balanced-performance to hidden choices
>>>>
>>>>    drivers/acpi/platform_profile.c    | 94 +++++++++++++++++++++++-------
>>>>    drivers/platform/x86/amd/pmf/sps.c | 11 ++++
>>>>    include/linux/platform_profile.h   |  3 +
>>>>    3 files changed, 87 insertions(+), 21 deletions(-)
>>>>
>>>> --
>>>> 2.43.0
>>>
>>> The patches are all good - but my question is do we really need the whole hidden implementation bit?
>>>
>>> If the options are not hidden, and someone chooses quiet or balanced-performance for the amd-pmf driver - does it really matter that it's going to do the same as low-power or performance?
>>>
>>> So, same feedback as I had for Antheas's patches. I understand why this is being proposed but for me it is making things unnecessarily complicated.
>>>
>>> My personal vote remains that the amd_pmf driver carries the superset to keep everyone happy (sorry - it sucks to be the CPU vendor that has to play nice with everyone).
>>>
>>> Mark
>>
>> Well so the problem with having all of them is specifically what happens
>> when "only" amd-pmf is bound?
>>
>> If you advertise both "low power" and "quiet" it's really confusing to
>> userspace what the difference is.
>>
>> The fact that it's actually 100% the same brings me to my personal
>> opinion on all of this.  Although I spent time writing up this series to
>> do it this way my "preference" is that we permanently alias "low power"
>> and "quiet" to one another and update all drivers to use "low power"
>> instead.
>>
>> Granted that doesn't help the case of balance-performance being hidden
>> that Antheas mentioned for acer-wmi and legion-wmi but I don't know
>> serious of a problem that actually is.
> 
> Hi Mark,
> So I agree with Mario here on that. I actually made the patch you
> suggested last Sunday [1].

My suggestion is actually more drastic than what you linked.  It's that 
we make a change in the core to special case the aliased strings, and 
then adjust all in-tree drivers to pick one or the other.

> 
> But then I looked at it and thought to myself that I can't ship this,
> so I did a v2, which is what I sent on Tuesday.
> 
> My priority here is to not disrupt the existing ABI with 6.14. This
> would allow extending this discussion by a couple of weeks, so a more
> permanent solution can be found.
> 
> If that would be the case, perhaps my patch series is more preferable
> as, since it is smaller, it would be cleaner to revert.
> 
> Antheas
> 
> [1] https://github.com/hhd-dev/patchwork/commit/aae724e8c90da3605bd131672fea07cd866af55f

Re: [PATCH 0/3] Add support for hidden choices to platform_profile

[Mark Pearson]

On Fri, Feb 28, 2025, at 2:44 PM, Mario Limonciello wrote:
> On 2/28/2025 13:39, Mark Pearson wrote:
>> Hi Mario,
>> 
>> On Fri, Feb 28, 2025, at 12:01 PM, Mario Limonciello wrote:
>>> From: Mario Limonciello <mario.limonciello@amd.com>
>>>
>>> When two drivers provide platform profile handlers but use different
>>> strings to mean (essentially) the same thing the legacy interface won't
>>> export them because it only shows profiles common to multiple drivers.
>>>
>>> This causes an unexpected behavior to people who have upgraded from an
>>> earlier kernel because if multiple drivers have bound platform profile
>>> handlers they might not be able to access profiles they were expecting.
>>>
>>> Introduce a concept of a "hidden choice" that drivers can register and
>>> the platform profile handler code will utilize when using the legacy
>>> interface.
>>>
>>> There have been some other attempts at solving this issue in other ways.
>>> This serves as an alternative to those attempts.
>>>
>>> Link:
>>> https://lore.kernel.org/platform-driver-x86/e64b771e-3255-42ad-9257-5b8fc6c24ac9@gmx.de/T/#t
>>> Link:
>>> https://lore.kernel.org/platform-driver-x86/CAGwozwF-WVEgiAbWbRCiUaXf=BVa3KqmMJfs06trdMQHpTGmjQ@mail.gmail.com/T/#m2f3929e2d4f73cc0eedd14738170dad45232fd18
>>> Cc: Antheas Kapenekakis <lkml@antheas.dev>
>>> Cc: "Luke D. Jones" <luke@ljones.dev>
>>>
>>> Mario Limonciello (3):
>>>    ACPI: platform_profile: Add support for hidden choices
>>>    platform/x86/amd: pmf: Add 'quiet' to hidden choices
>>>    platform/x86/amd: pmf: Add balanced-performance to hidden choices
>>>
>>>   drivers/acpi/platform_profile.c    | 94 +++++++++++++++++++++++-------
>>>   drivers/platform/x86/amd/pmf/sps.c | 11 ++++
>>>   include/linux/platform_profile.h   |  3 +
>>>   3 files changed, 87 insertions(+), 21 deletions(-)
>>>
>>> -- 
>>> 2.43.0
>> 
>> The patches are all good - but my question is do we really need the whole hidden implementation bit?
>> 
>> If the options are not hidden, and someone chooses quiet or balanced-performance for the amd-pmf driver - does it really matter that it's going to do the same as low-power or performance?
>> 
>> So, same feedback as I had for Antheas's patches. I understand why this is being proposed but for me it is making things unnecessarily complicated.
>> 
>> My personal vote remains that the amd_pmf driver carries the superset to keep everyone happy (sorry - it sucks to be the CPU vendor that has to play nice with everyone).
>> 
>> Mark
>
> Well so the problem with having all of them is specifically what happens 
> when "only" amd-pmf is bound?
>
> If you advertise both "low power" and "quiet" it's really confusing to 
> userspace what the difference is.

Ah - I guess you get platforms without profile support where amd-pmf is the only thing. I hadn't considered that.

FWIW, I believe we (Lenovo) have both low power and quiet on Windows...and they really don't make much difference (which is why the thermal team didn't do them both on Linux). 
I don't know if Windows users are more or less confused (or maybe they've just abandoned all hope and are migrating to Linux...)

You have a better feeling as to how many issues you'll get raised if they behave the same, and have to support a wider ecosystem, so I'm happy to be over-ruled. I just wanted to wave my flag that I think the driver is getting too complicated. I'm slightly dreading having to debug customer issues at this point.

>
> The fact that it's actually 100% the same brings me to my personal 
> opinion on all of this.  Although I spent time writing up this series to 
> do it this way my "preference" is that we permanently alias "low power" 
> and "quiet" to one another and update all drivers to use "low power" 
> instead.
>

Guaranteed if you do that some vendor will have something that differentiates.

I can see having a 'use as much power as possible without needing fans' for quiet, and 'make the battery last as long as humanely possible whilst keeping the system usable' mode for low-power. Don't think anyone has done it...but they could.

> Granted that doesn't help the case of balance-performance being hidden 
> that Antheas mentioned for acer-wmi and legion-wmi but I don't know 
> serious of a problem that actually is.

I really have to go play with this on the Legion-Go. I need a time-machine.....

Mark

Re: [PATCH 0/3] Add support for hidden choices to platform_profile

[Derek John Clark]

On Fri, Feb 28, 2025 at 9:02 AM Mario Limonciello <superm1@kernel.org> wrote:
>
> From: Mario Limonciello <mario.limonciello@amd.com>
>
> When two drivers provide platform profile handlers but use different
> strings to mean (essentially) the same thing the legacy interface won't
> export them because it only shows profiles common to multiple drivers.
>
> This causes an unexpected behavior to people who have upgraded from an
> earlier kernel because if multiple drivers have bound platform profile
> handlers they might not be able to access profiles they were expecting.
>
> Introduce a concept of a "hidden choice" that drivers can register and
> the platform profile handler code will utilize when using the legacy
> interface.
>
> There have been some other attempts at solving this issue in other ways.
> This serves as an alternative to those attempts.
>
> Link: https://lore.kernel.org/platform-driver-x86/e64b771e-3255-42ad-9257-5b8fc6c24ac9@gmx.de/T/#t
> Link: https://lore.kernel.org/platform-driver-x86/CAGwozwF-WVEgiAbWbRCiUaXf=BVa3KqmMJfs06trdMQHpTGmjQ@mail.gmail.com/T/#m2f3929e2d4f73cc0eedd14738170dad45232fd18
> Cc: Antheas Kapenekakis <lkml@antheas.dev>
> Cc: "Luke D. Jones" <luke@ljones.dev>
>
> Mario Limonciello (3):
>   ACPI: platform_profile: Add support for hidden choices
>   platform/x86/amd: pmf: Add 'quiet' to hidden choices
>   platform/x86/amd: pmf: Add balanced-performance to hidden choices
>
>  drivers/acpi/platform_profile.c    | 94 +++++++++++++++++++++++-------
>  drivers/platform/x86/amd/pmf/sps.c | 11 ++++
>  include/linux/platform_profile.h   |  3 +
>  3 files changed, 87 insertions(+), 21 deletions(-)
>
> --
> 2.43.0
>

Everything seems to be working as intended. I applied these patches on
top of my lenovo-wmi series modified to always show
balanced-performance and with quiet as the lowest profile. Testing was
done on the Legion Go.
Results:

$ cat /sys/firmware/acpi/platform_profile_choices
quiet balanced balanced-performance performance
$ for f in *; do cat $f/name; cat $f/choices; done;
lenovo-wmi-gamezone
quiet balanced balanced-performance performance custom
amd-pmf
low-power balanced performance
$ echo quiet | sudo tee /sys/firmware/acpi/platform_profile
quiet
$ for f in *; do cat $f/name; cat $f/profile; done;
lenovo-wmi-gamezone
quiet
amd-pmf
quiet
$ echo balanced-performance | sudo tee /sys/firmware/acpi/platform_profile
balanced-performance
$ for f in *; do cat $f/name; cat $f/profile; done;
lenovo-wmi-gamezone
balanced-performance
amd-pmf
balanced-performance
$ echo low-power | sudo tee /sys/firmware/acpi/platform_profile
low-power
tee: /sys/firmware/acpi/platform_profile: Operation not supported
$ for f in *; do cat $f/name; cat $f/profile; done;
lenovo-wmi-gamezone
balanced-performance
amd-pmf
balanced-performance

Tested-by: Derek J. Clark <derekjohn.clark@gmail.com>

Re: [PATCH 0/3] Add support for hidden choices to platform_profile

[Antheas Kapenekakis]

I just tested this. The behavior of this patch series matches mine
1-1. Feel free to add a tested-by.

IMO it is a bit cleaner/thought through than my series, so I am fine
with dropping mine. Should be as it is essentially a V3

Antheas

On Fri, 28 Feb 2025 at 18:02, Mario Limonciello <superm1@kernel.org> wrote:
>
> From: Mario Limonciello <mario.limonciello@amd.com>
>
> When two drivers provide platform profile handlers but use different
> strings to mean (essentially) the same thing the legacy interface won't
> export them because it only shows profiles common to multiple drivers.
>
> This causes an unexpected behavior to people who have upgraded from an
> earlier kernel because if multiple drivers have bound platform profile
> handlers they might not be able to access profiles they were expecting.
>
> Introduce a concept of a "hidden choice" that drivers can register and
> the platform profile handler code will utilize when using the legacy
> interface.
>
> There have been some other attempts at solving this issue in other ways.
> This serves as an alternative to those attempts.
>
> Link: https://lore.kernel.org/platform-driver-x86/e64b771e-3255-42ad-9257-5b8fc6c24ac9@gmx.de/T/#t
> Link: https://lore.kernel.org/platform-driver-x86/CAGwozwF-WVEgiAbWbRCiUaXf=BVa3KqmMJfs06trdMQHpTGmjQ@mail.gmail.com/T/#m2f3929e2d4f73cc0eedd14738170dad45232fd18
> Cc: Antheas Kapenekakis <lkml@antheas.dev>
> Cc: "Luke D. Jones" <luke@ljones.dev>
>
> Mario Limonciello (3):
>   ACPI: platform_profile: Add support for hidden choices
>   platform/x86/amd: pmf: Add 'quiet' to hidden choices
>   platform/x86/amd: pmf: Add balanced-performance to hidden choices
>
>  drivers/acpi/platform_profile.c    | 94 +++++++++++++++++++++++-------
>  drivers/platform/x86/amd/pmf/sps.c | 11 ++++
>  include/linux/platform_profile.h   |  3 +
>  3 files changed, 87 insertions(+), 21 deletions(-)
>
> --
> 2.43.0
>

Re: [PATCH 0/3] Add support for hidden choices to platform_profile

[Mario Limonciello]

On 3/1/25 05:09, Antheas Kapenekakis wrote:
> I just tested this. The behavior of this patch series matches mine
> 1-1. Feel free to add a tested-by.

I understand your intent, but can you please explicitly type out your 
tag?  This is especially important because maintainers often use 'b4' to 
pull all tags out of an email thread when accepting patches.

> 
> IMO it is a bit cleaner/thought through than my series, so I am fine
> with dropping mine. Should be as it is essentially a V3
> 
> 

<strip>

Also; Rafael mentioned this in another thread, but please refrain from 
top posting when possible [1].

[1] 
https://www.kernel.org/doc/html/v6.14-rc4/process/submitting-patches.html#use-trimmed-interleaved-replies-in-email-discussions

Re: [PATCH 0/3] Add support for hidden choices to platform_profile

[Antheas Kapenekakis]

On Sat, 1 Mar 2025 at 14:44, Mario Limonciello <superm1@kernel.org> wrote:
>
> On 3/1/25 05:09, Antheas Kapenekakis wrote:
> > I just tested this. The behavior of this patch series matches mine
> > 1-1. Feel free to add a tested-by.
>
> I understand your intent, but can you please explicitly type out your
> tag?  This is especially important because maintainers often use 'b4' to
> pull all tags out of an email thread when accepting patches.

Tested-by: Antheas Kapenekakis <lkml@antheas.dev>

> >
> > IMO it is a bit cleaner/thought through than my series, so I am fine
> > with dropping mine. Should be as it is essentially a V3
> >
> >
>
> <strip>
>
> Also; Rafael mentioned this in another thread, but please refrain from
> top posting when possible [1].
>
> [1]
> https://www.kernel.org/doc/html/v6.14-rc4/process/submitting-patches.html#use-trimmed-interleaved-replies-in-email-discussions

Ack

Re: [PATCH 0/3] Add support for hidden choices to platform_profile

[Ilpo Järvinen]

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

On Fri, 28 Feb 2025, Mario Limonciello wrote:

> From: Mario Limonciello <mario.limonciello@amd.com>
> 
> When two drivers provide platform profile handlers but use different
> strings to mean (essentially) the same thing the legacy interface won't
> export them because it only shows profiles common to multiple drivers.
> 
> This causes an unexpected behavior to people who have upgraded from an
> earlier kernel because if multiple drivers have bound platform profile
> handlers they might not be able to access profiles they were expecting.
> 
> Introduce a concept of a "hidden choice" that drivers can register and
> the platform profile handler code will utilize when using the legacy
> interface.
> 
> There have been some other attempts at solving this issue in other ways.
> This serves as an alternative to those attempts.
> 
> Link: https://lore.kernel.org/platform-driver-x86/e64b771e-3255-42ad-9257-5b8fc6c24ac9@gmx.de/T/#t
> Link: https://lore.kernel.org/platform-driver-x86/CAGwozwF-WVEgiAbWbRCiUaXf=BVa3KqmMJfs06trdMQHpTGmjQ@mail.gmail.com/T/#m2f3929e2d4f73cc0eedd14738170dad45232fd18
> Cc: Antheas Kapenekakis <lkml@antheas.dev>
> Cc: "Luke D. Jones" <luke@ljones.dev>
> 
> Mario Limonciello (3):
>   ACPI: platform_profile: Add support for hidden choices
>   platform/x86/amd: pmf: Add 'quiet' to hidden choices
>   platform/x86/amd: pmf: Add balanced-performance to hidden choices
> 
>  drivers/acpi/platform_profile.c    | 94 +++++++++++++++++++++++-------
>  drivers/platform/x86/amd/pmf/sps.c | 11 ++++
>  include/linux/platform_profile.h   |  3 +
>  3 files changed, 87 insertions(+), 21 deletions(-)
> 
> 

Acked-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

Re: [PATCH 0/3] Add support for hidden choices to platform_profile

[Rafael J. Wysocki]

On Tue, Mar 4, 2025 at 5:22 PM Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> wrote:
>
> On Fri, 28 Feb 2025, Mario Limonciello wrote:
>
> > From: Mario Limonciello <mario.limonciello@amd.com>
> >
> > When two drivers provide platform profile handlers but use different
> > strings to mean (essentially) the same thing the legacy interface won't
> > export them because it only shows profiles common to multiple drivers.
> >
> > This causes an unexpected behavior to people who have upgraded from an
> > earlier kernel because if multiple drivers have bound platform profile
> > handlers they might not be able to access profiles they were expecting.
> >
> > Introduce a concept of a "hidden choice" that drivers can register and
> > the platform profile handler code will utilize when using the legacy
> > interface.
> >
> > There have been some other attempts at solving this issue in other ways.
> > This serves as an alternative to those attempts.
> >
> > Link: https://lore.kernel.org/platform-driver-x86/e64b771e-3255-42ad-9257-5b8fc6c24ac9@gmx.de/T/#t
> > Link: https://lore.kernel.org/platform-driver-x86/CAGwozwF-WVEgiAbWbRCiUaXf=BVa3KqmMJfs06trdMQHpTGmjQ@mail.gmail.com/T/#m2f3929e2d4f73cc0eedd14738170dad45232fd18
> > Cc: Antheas Kapenekakis <lkml@antheas.dev>
> > Cc: "Luke D. Jones" <luke@ljones.dev>
> >
> > Mario Limonciello (3):
> >   ACPI: platform_profile: Add support for hidden choices
> >   platform/x86/amd: pmf: Add 'quiet' to hidden choices
> >   platform/x86/amd: pmf: Add balanced-performance to hidden choices
> >
> >  drivers/acpi/platform_profile.c    | 94 +++++++++++++++++++++++-------
> >  drivers/platform/x86/amd/pmf/sps.c | 11 ++++
> >  include/linux/platform_profile.h   |  3 +
> >  3 files changed, 87 insertions(+), 21 deletions(-)
> >
> >
>
> Acked-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

All 3 patches applied as 6.14-rc6 material, thanks!