Message-Id: <D84F6QF8EU3D.3RUI1PKXP2DZ3@gmail.com>
Date: Fri, 28 Feb 2025 17:08:21 -0500
From: "Kurt Borja" <kuurtb@...il.com>
To: "Mario Limonciello" <superm1@...nel.org>, "Shyam Sundar S K"
 <Shyam-sundar.S-k@....com>, "Rafael J . Wysocki" <rafael@...nel.org>, "Hans
 de Goede" <hdegoede@...hat.com>, Ilpo Järvinen
 <ilpo.jarvinen@...ux.intel.com>, "Luke D . Jones" <luke@...nes.dev>, "Mark
 Pearson" <mpearson-lenovo@...ebb.ca>
Cc: "open list:AMD PMF DRIVER" <platform-driver-x86@...r.kernel.org>, "open
 list" <linux-kernel@...r.kernel.org>, "open list:ACPI"
 <linux-acpi@...r.kernel.org>, "Derek J . Clark"
 <derekjohn.clark@...il.com>, "Antheas Kapenekakis" <lkml@...heas.dev>,
 <me@...egospodneti.ch>, "Denis Benato" <benato.denis96@...il.com>, "Mario
 Limonciello" <mario.limonciello@....com>, "Armin Wolf" <W_Armin@....de>
Subject: Re: [PATCH 1/3] ACPI: platform_profile: Add support for hidden
 choices

Hi Mario,

On Fri Feb 28, 2025 at 12:01 PM -05, Mario Limonciello wrote:
> From: Mario Limonciello <mario.limonciello@....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@...heas.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@....com>
> ---
> Cc: "Luke D. Jones" <luke@...nes.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

Powered by blists - more mailing lists