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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <bd079286-b7c9-4231-b8d1-1b0bcf937997@amd.com>
Date: Thu, 5 Dec 2024 08:46:41 -0600
From: Mario Limonciello <mario.limonciello@....com>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: Hans de Goede <hdegoede@...hat.com>,
 "Rafael J . Wysocki" <rafael@...nel.org>, Len Brown <lenb@...nel.org>,
 Maximilian Luz <luzmaximilian@...il.com>, Lee Chun-Yi <jlee@...e.com>,
 Shyam Sundar S K <Shyam-sundar.S-k@....com>,
 Corentin Chary <corentin.chary@...il.com>, "Luke D . Jones"
 <luke@...nes.dev>, Ike Panhc <ike.pan@...onical.com>,
 Henrique de Moraes Holschuh <hmh@....eng.br>,
 Alexis Belmonte <alexbelm48@...il.com>,
 Uwe Kleine-König <u.kleine-koenig@...gutronix.de>,
 Ai Chao <aichao@...inos.cn>, Gergo Koteles <soyer@....hu>,
 open list <linux-kernel@...r.kernel.org>,
 "open list:ACPI" <linux-acpi@...r.kernel.org>,
 "open list:MICROSOFT SURFACE PLATFORM PROFILE DRIVER"
 <platform-driver-x86@...r.kernel.org>,
 "open list:THINKPAD ACPI EXTRAS DRIVER"
 <ibm-acpi-devel@...ts.sourceforge.net>,
 Mark Pearson <mpearson-lenovo@...ebb.ca>,
 Matthew Schwartz <matthew.schwartz@...ux.dev>, Armin Wolf <W_Armin@....de>
Subject: Re: [PATCH v9 18/22] ACPI: platform_profile: Check all profile
 handler to calculate next

On 12/5/2024 08:22, Ilpo Järvinen wrote:
> On Sun, 1 Dec 2024, Mario Limonciello wrote:
> 
>> As multiple platform profile handlers might not all support the same
>> profile, cycling to the next profile could have a different result
>> depending on what handler are registered.
>>
>> Check what is active and supported by all handlers to decide what
>> to do.
>>
>> Reviewed-by: Armin Wolf <W_Armin@....de>
>> Tested-by: Mark Pearson <mpearson-lenovo@...ebb.ca>
>> Reviewed-by: Mark Pearson <mpearson-lenovo@...ebb.ca>
>> Signed-off-by: Mario Limonciello <mario.limonciello@....com>
>> ---
>>   drivers/acpi/platform_profile.c | 30 +++++++++++++++++++++---------
>>   1 file changed, 21 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
>> index d5f0679d59d50..16746d9b9aa7c 100644
>> --- a/drivers/acpi/platform_profile.c
>> +++ b/drivers/acpi/platform_profile.c
>> @@ -407,25 +407,37 @@ EXPORT_SYMBOL_GPL(platform_profile_notify);
>>   
>>   int platform_profile_cycle(void)
>>   {
>> -	enum platform_profile_option profile;
>> -	enum platform_profile_option next;
>> +	enum platform_profile_option next = PLATFORM_PROFILE_LAST;
>> +	enum platform_profile_option profile = PLATFORM_PROFILE_LAST;
>> +	unsigned long choices[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
>>   	int err;
>>   
>> +	set_bit(PLATFORM_PROFILE_LAST, choices);
>>   	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
>> -		if (!cur_profile)
>> -			return -ENODEV;
>> +		err = class_for_each_device(&platform_profile_class, NULL,
>> +					    &profile, _aggregate_profiles);
>> +		if (err)
>> +			return err;
>>   
>> -		err = cur_profile->profile_get(cur_profile, &profile);
>> +		if (profile == PLATFORM_PROFILE_CUSTOM ||
>> +		    profile == PLATFORM_PROFILE_LAST)
>> +			return -EINVAL;
>> +
>> +		err = class_for_each_device(&platform_profile_class, NULL,
>> +					    choices, _aggregate_choices);
>>   		if (err)
>>   			return err;
>>   
>> -		next = find_next_bit_wrap(cur_profile->choices, PLATFORM_PROFILE_LAST,
>> +		/* never iterate into a custom if all drivers supported it */
>> +		clear_bit(PLATFORM_PROFILE_CUSTOM, choices);
> 
> I'm confused by the comment. I was under impression the custom "profile"
> is just a framework construct when the _framework_ couldn't find a
> consistent profile? How can a driver decide to "support it"? It sounds
> like a driver overstepping its intended domain of operation.
> 
> If the intention really is for the driver to "support" or "not support"
> custom profile, then you should adjust the commit message of the patch
> which introduced it.
> 

Yes I had envisioned that a driver could potentially set custom as well.

This idea was introduced by my RFC series that precluded doing the
multiple driver handlers.

The basic idea is that some drivers (for example asus-wmi and 
asus-armoury) have the ability for the user to change a sysfs file that 
represents sPPT or fPPT directly.

If this has been done they're "off the beating path" of a predfined
profile because they're picking and choosing individual knobs.

So if a user touches those a driver could set profile as "custom" and if 
a user chooses the platform profile the driver will override all of 
those and report a pre-defined profile.

So, yes I had that all in my mind but as you point out I definitely 
forgot to mention it in the commit messages.

Do you agree with it?  If so, I'll amend the next version where 
applicable (probably the patch that introduces custom and the 
documentation patch).