Message-ID: <6f56571a-3090-4323-a29d-008b916abf39@kernel.org>
Date: Fri, 28 Feb 2025 21:19:21 -0600
From: Mario Limonciello <superm1@...nel.org>
To: Kurt Borja <kuurtb@...il.com>, 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

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

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.

Powered by blists - more mailing lists