linux-kernel - Re: [PATCH v2 2/2] ACPI: platform_profile: make amd-pmf a secondary handler

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: <26c21df0-c885-4948-8902-685dcb7f13b8@amd.com>
Date: Thu, 27 Feb 2025 10:45:57 -0600
From: Mario Limonciello <mario.limonciello@....com>
To: Antheas Kapenekakis <lkml@...heas.dev>, mpearson-lenovo@...ebb.ca
Cc: ilpo.jarvinen@...ux.intel.com, lenb@...nel.org,
 linux-acpi@...r.kernel.org, linux-kernel@...r.kernel.org,
 platform-driver-x86@...r.kernel.org, rafael@...nel.org, hdegoede@...hat.com,
 me@...egospodneti.ch, luke@...nes.dev
Subject: Re: [PATCH v2 2/2] ACPI: platform_profile: make amd-pmf a secondary
 handler

On 2/27/2025 09:36, Antheas Kapenekakis wrote:
> Since amd-pmf is expected to run alongside other platform handlers, it
> should be able to accept all platform profiles. Therefore, mark it as
> secondary and in the case of a custom profile, make it NOOP without an
> error to allow primary handlers to receive a custom profile.
> The sysfs endpoint will still report custom, after all.
> 
> Signed-off-by: Antheas Kapenekakis <lkml@...heas.dev>
> ---
>   drivers/platform/x86/amd/pmf/spc.c | 3 +++
>   drivers/platform/x86/amd/pmf/sps.c | 8 ++++++++
>   2 files changed, 11 insertions(+)
> 
> diff --git a/drivers/platform/x86/amd/pmf/spc.c b/drivers/platform/x86/amd/pmf/spc.c
> index f34f3130c330..99c48378f943 100644
> --- a/drivers/platform/x86/amd/pmf/spc.c
> +++ b/drivers/platform/x86/amd/pmf/spc.c
> @@ -219,12 +219,15 @@ static int amd_pmf_get_slider_info(struct amd_pmf_dev *dev, struct ta_pmf_enact_
>   
>   	switch (dev->current_profile) {
>   	case PLATFORM_PROFILE_PERFORMANCE:
> +	case PLATFORM_PROFILE_BALANCED_PERFORMANCE:
>   		val = TA_BEST_PERFORMANCE;
>   		break;
>   	case PLATFORM_PROFILE_BALANCED:
>   		val = TA_BETTER_PERFORMANCE;
>   		break;
>   	case PLATFORM_PROFILE_LOW_POWER:
> +	case PLATFORM_PROFILE_COOL:
> +	case PLATFORM_PROFILE_QUIET:
>   		val = TA_BEST_BATTERY;

I would really prefer we do the absolute bare minimum to help this issue 
on ASUS (just special case quiet) and leave adding compat for other 
profiles for other development.

The reason for this is that if you look at power_modes_v2 there are 
actually 4 'possible' modes for v2 platforms.  So there is a bit of 
nuance involved and it's really not a 'bug fix' anymore by doing so much 
at once.

>   		break;
>   	default:
> diff --git a/drivers/platform/x86/amd/pmf/sps.c b/drivers/platform/x86/amd/pmf/sps.c
> index e6cf0b22dac3..a2a8511768ce 100644
> --- a/drivers/platform/x86/amd/pmf/sps.c
> +++ b/drivers/platform/x86/amd/pmf/sps.c
> @@ -297,12 +297,15 @@ 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:
>   		mode = POWER_MODE_BALANCED_POWER;
>   		break;
>   	case PLATFORM_PROFILE_LOW_POWER:
> +	case PLATFORM_PROFILE_COOL:
> +	case PLATFORM_PROFILE_QUIET:
>   		mode = POWER_MODE_POWER_SAVER;
>   		break;
>   	default:
> @@ -369,6 +372,10 @@ static int amd_pmf_profile_set(struct device *dev,
>   	struct amd_pmf_dev *pmf = dev_get_drvdata(dev);
>   	int ret = 0;
>   
> +	/* If the profile is custom, bail without an error. */
> +	if (profile == PLATFORM_PROFILE_CUSTOM)
> +		return 0;
> +

The legacy interface doesn't support writing custom.

https://github.com/torvalds/linux/blob/v6.14-rc3/drivers/acpi/platform_profile.c#L382

IoW this is dead code.

>   	pmf->current_profile = profile;
>   
>   	/* Notify EC about the slider position change */
> @@ -400,6 +407,7 @@ static const struct platform_profile_ops amd_pmf_profile_ops = {
>   	.probe = amd_pmf_profile_probe,
>   	.profile_get = amd_pmf_profile_get,
>   	.profile_set = amd_pmf_profile_set,
> +	.secondary = true,

I really don't understand the need for declaring primary / secondary. 
It really doesn't matter which driver can do it.  This same problem 
could happen in any direction.

As a different suggestion; how about a new "generic" callback for
'compatibility' profiles?

Right now the .probe() callback amd_pmf_get_pprof_modes() will set bits 
for visible profiles.

How about an optional .compat_profiles() for the hidden one(s)?  This 
would allow any driver to implement them.

>   };
>   
>   int amd_pmf_init_sps(struct amd_pmf_dev *dev)