linux-kernel - Re: [PATCH v3 3/8] cpufreq: amd-pstate: change driver to be built-in type

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 for Android: free password hash cracker in your pocket

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

Message-ID: <392fb511-5d16-167d-77ab-dfaa43dca4cb@amd.com>
Date:   Mon, 7 Nov 2022 12:29:50 -0600
From:   "Limonciello, Mario" <mario.limonciello@....com>
To:     Perry Yuan <Perry.Yuan@....com>, rafael.j.wysocki@...el.com,
        ray.huang@....com, viresh.kumar@...aro.org, wyes.karny@....com
Cc:     Deepak.Sharma@....com, Nathan.Fontenot@....com,
        Alexander.Deucher@....com, Shimmer.Huang@....com,
        Xiaojian.Du@....com, Li.Meng@....com, linux-pm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 3/8] cpufreq: amd-pstate: change driver to be built-in
 type

+ wyes.karny@....com

You should sync with Wyes Karny on this patch, I think he had some 
different ideas that you guys should fold together for v4 of this 
series.  I'll leave some direct comments on your implementation below.

Also, include him in on CC for your v4.

On 11/7/2022 11:57, Perry Yuan wrote:
> Change the `amd-pstate` driver as the built-in type which can help to
> load the driver before the acpi_cpufreq driver as the default pstate
> driver for the AMD processors.
> 
> for the processors do not have the dedicated MSR functions, add
> `amd-pstate=legacy_cppc` to grub which enable shared memmory interface
> to communicate with cppc_acpi module to control pstate hints.

1) s/memmory/memory/
2) Although many users will use GRUB to configure their kernel command 
line you should not assume it in the commit message.

> 
> Signed-off-by: Perry Yuan <Perry.Yuan@....com>
> ---
>   drivers/cpufreq/amd-pstate.c | 23 +++++++++++++----------
>   1 file changed, 13 insertions(+), 10 deletions(-)

You need to document the new early parameter support in 
kernel-parameters.txt, and should also put it in amd-pstate.rst.

> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index ace7d50cf2ac..14906431dc15 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -59,10 +59,7 @@
>    * we disable it by default to go acpi-cpufreq on these processors and add a
>    * module parameter to be able to enable it manually for debugging.
>    */
> -static bool shared_mem = false;
> -module_param(shared_mem, bool, 0444);
> -MODULE_PARM_DESC(shared_mem,
> -		 "enable amd-pstate on processors with shared memory solution (false = disabled (default), true = enabled)");
> +static bool shared_mem __read_mostly;
>   
>   static struct cpufreq_driver amd_pstate_driver;
>   
> @@ -653,16 +650,22 @@ static int __init amd_pstate_init(void)
>   
>   	return ret;
>   }
> +device_initcall(amd_pstate_init);
>   
> -static void __exit amd_pstate_exit(void)
> +static int __init amd_pstate_param(char *str)
>   {
> -	cpufreq_unregister_driver(&amd_pstate_driver);
> +	if (!str)
> +		return -EINVAL;
>   
> -	amd_pstate_enable(false);
> -}
> +	/* enable shared memory type CPPC ,if you processor has no MSR, you have to add this
> +	 * to your grub to make cppc driver loaded successfully.

Don't reference GRUB here, it should be referenced from the kernel 
command line.

> +	 */
> +	if (!strcmp(str, "legacy_cppc"))
> +		shared_mem = true;
Sync with Wyes about this.  He had some different strings and flow in 
mind which I think would be more preferable.

>   
> -module_init(amd_pstate_init);
> -module_exit(amd_pstate_exit);
> +	return 0;
> +}
> +early_param("amd-pstate", amd_pstate_param);
>   
>   MODULE_AUTHOR("Huang Rui <ray.huang@....com>");
>   MODULE_DESCRIPTION("AMD Processor P-state Frequency Driver");