linux-kernel - Re: [PATCH 1/2] cpufreq/amd-pstate-ut: Handle the inconsistency

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]

Date: Tue, 25 Jun 2024 10:05:02 -0500
From: Mario Limonciello <mario.limonciello@....com>
To: Dhananjay Ugwekar <Dhananjay.Ugwekar@....com>, rafael@...nel.org,
 viresh.kumar@...aro.org, gautham.shenoy@....com, perry.yuan@....com,
 skhan@...uxfoundation.org, li.meng@....com, ray.huang@....com
Cc: linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] cpufreq/amd-pstate-ut: Handle the inconsistency

On 6/25/2024 08:41, Dhananjay Ugwekar wrote:
> cpudata->nominal_freq being in MHz whereas other frequencies being in
> KHz breaks the amd-pstate-ut frequency sanity check. This fixes it.
> 
> Fixes: 14eb1c96e3a3 ("cpufreq: amd-pstate: Add test module for amd-pstate driver")
> Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@....com>

The code change below looks fine to me, but I think the tag is wrong. 
It should go with the "fix" that caused the inconsistency.  Here is what
I think the correct tag should be:

Fixes: e4731baaf294 ("cpufreq: amd-pstate: Fix the inconsistency in max 
frequency units")

Reviewed-by: Mario Limonciello <mario.limonciello@....com>

> ---
>   drivers/cpufreq/amd-pstate-ut.c | 12 +++++++-----
>   1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/cpufreq/amd-pstate-ut.c b/drivers/cpufreq/amd-pstate-ut.c
> index fc275d41d51e..66b73c308ce6 100644
> --- a/drivers/cpufreq/amd-pstate-ut.c
> +++ b/drivers/cpufreq/amd-pstate-ut.c
> @@ -202,6 +202,7 @@ static void amd_pstate_ut_check_freq(u32 index)
>   	int cpu = 0;
>   	struct cpufreq_policy *policy = NULL;
>   	struct amd_cpudata *cpudata = NULL;
> +	u32 nominal_freq_khz;
>   
>   	for_each_possible_cpu(cpu) {
>   		policy = cpufreq_cpu_get(cpu);
> @@ -209,13 +210,14 @@ static void amd_pstate_ut_check_freq(u32 index)
>   			break;
>   		cpudata = policy->driver_data;
>   
> -		if (!((cpudata->max_freq >= cpudata->nominal_freq) &&
> -			(cpudata->nominal_freq > cpudata->lowest_nonlinear_freq) &&
> +		nominal_freq_khz = cpudata->nominal_freq*1000;
> +		if (!((cpudata->max_freq >= nominal_freq_khz) &&
> +			(nominal_freq_khz > cpudata->lowest_nonlinear_freq) &&
>   			(cpudata->lowest_nonlinear_freq > cpudata->min_freq) &&
>   			(cpudata->min_freq > 0))) {
>   			amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_FAIL;
>   			pr_err("%s cpu%d max=%d >= nominal=%d > lowest_nonlinear=%d > min=%d > 0, the formula is incorrect!\n",
> -				__func__, cpu, cpudata->max_freq, cpudata->nominal_freq,
> +				__func__, cpu, cpudata->max_freq, nominal_freq_khz,
>   				cpudata->lowest_nonlinear_freq, cpudata->min_freq);
>   			goto skip_test;
>   		}
> @@ -229,13 +231,13 @@ static void amd_pstate_ut_check_freq(u32 index)
>   
>   		if (cpudata->boost_supported) {
>   			if ((policy->max == cpudata->max_freq) ||
> -					(policy->max == cpudata->nominal_freq))
> +					(policy->max == nominal_freq_khz))
>   				amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_PASS;
>   			else {
>   				amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_FAIL;
>   				pr_err("%s cpu%d policy_max=%d should be equal cpu_max=%d or cpu_nominal=%d !\n",
>   					__func__, cpu, policy->max, cpudata->max_freq,
> -					cpudata->nominal_freq);
> +					nominal_freq_khz);
>   				goto skip_test;
>   			}
>   		} else {