Address an untested error where the nominal_freq was returned in KHz
instead of the correct MHz units, this oversight led to a wrong
nominal_freq set and resued, it will cause the max frequency of core to
be initialized with a wrong frequency value.
Cc: stable@vger.kernel.org
Fixes: ec437d71db7 ("cpufreq: amd-pstate: Introduce a new AMD P-State driver to support future processors")
Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
Signed-off-by: Perry Yuan <perry.yuan@amd.com>
---
drivers/cpufreq/amd-pstate.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index 08e112444c27..ac7faa98a450 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -640,8 +640,7 @@ static int amd_get_nominal_freq(struct amd_cpudata *cpudata)
if (ret)
return ret;
- /* Switch to khz */
- return cppc_perf.nominal_freq * 1000;
+ return cppc_perf.nominal_freq;
}
static int amd_get_lowest_nonlinear_freq(struct amd_cpudata *cpudata)
--
2.34.1On Thu, Feb 08, 2024 at 11:46:29AM +0800, Perry Yuan wrote:
> Address an untested error where the nominal_freq was returned in KHz
> instead of the correct MHz units, this oversight led to a wrong
> nominal_freq set and resued, it will cause the max frequency of core to
> be initialized with a wrong frequency value.
The _CPC object on my Zen2 EPYC server which has CPPC v3, I see :
0x00000190, //LowestFrequency
0x000007D0 //NominalFrequency
})
}
where 0x7D0 is 2000 Mhz. Thus the value returend by cppc_perf.nominal_freq is in Mhz.
In this patch, we are changing making amd_get_nominal_freq() return
2000 (Mhz) instead of 2000000 (Khz) as it was doing previously.
amd_get_min_freq(), amd_get_max_freq(),
amd_get_lowest_nonlinear_perf() -all these functions return the
frequency in Khz units.
These functions are used the initialize the value of
cpudata->max_freq, cpu_data->min_freq, .... all of which are in Khz
units.
In, amd_pstate_set_boost(), we have:
if (state)
policy->cpuinfo.max_freq = cpudata->max_freq; <---- In Khz
else
policy->cpuinfo.max_freq = cpudata->nominal_freq; <--- Now in Mhz
Since policy->cpuinfo.max_freq is expected to be in Khz, isn't this
patch introducing an error ? Or perhaps this patch series is based on
another patchset ?
--
Thanks and Regards
gautham.
>
> Cc: stable@vger.kernel.org
> Fixes: ec437d71db7 ("cpufreq: amd-pstate: Introduce a new AMD P-State driver to support future processors")
> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> Signed-off-by: Perry Yuan <perry.yuan@amd.com>
> ---
> drivers/cpufreq/amd-pstate.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 08e112444c27..ac7faa98a450 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -640,8 +640,7 @@ static int amd_get_nominal_freq(struct amd_cpudata *cpudata)
> if (ret)
> return ret;
>
> - /* Switch to khz */
> - return cppc_perf.nominal_freq * 1000;
> + return cppc_perf.nominal_freq;
> }
>
> static int amd_get_lowest_nonlinear_freq(struct amd_cpudata *cpudata)
> --
> 2.34.1
>
On 2/8/2024 11:37 AM, Gautham R. Shenoy wrote:
> On Thu, Feb 08, 2024 at 11:46:29AM +0800, Perry Yuan wrote:
>> Address an untested error where the nominal_freq was returned in KHz
>> instead of the correct MHz units, this oversight led to a wrong
>> nominal_freq set and resued, it will cause the max frequency of core to
>> be initialized with a wrong frequency value.
>
> The _CPC object on my Zen2 EPYC server which has CPPC v3, I see :
> 0x00000190, //LowestFrequency
> 0x000007D0 //NominalFrequency
> })
> }
>
> where 0x7D0 is 2000 Mhz. Thus the value returend by cppc_perf.nominal_freq is in Mhz.
>
> In this patch, we are changing making amd_get_nominal_freq() return
> 2000 (Mhz) instead of 2000000 (Khz) as it was doing previously.
>
> amd_get_min_freq(), amd_get_max_freq(),
> amd_get_lowest_nonlinear_perf() -all these functions return the
> frequency in Khz units.
>
> These functions are used the initialize the value of
> cpudata->max_freq, cpu_data->min_freq, .... all of which are in Khz
> units.
>
> In, amd_pstate_set_boost(), we have:
>
>
> if (state)
> policy->cpuinfo.max_freq = cpudata->max_freq; <---- In Khz
> else
> policy->cpuinfo.max_freq = cpudata->nominal_freq; <--- Now in Mhz
>
> Since policy->cpuinfo.max_freq is expected to be in Khz, isn't this
> patch introducing an error ? Or perhaps this patch series is based on
> another patchset ?
>
> --
> Thanks and Regards
> gautham.
>
>
I can confirm on Gautham's comment, when boost is disabled with the scaling
driver set as amd-pstate, the max frequency reported is incorrect, as it
shows the frequency in MHz.
I tested this patch on Zen4 Genoa server platform, below are the
inconsistent min and max frequencies I observed.
[cpufreq]# grep . *freq
amd_pstate_lowest_nonlinear_freq:1804000
amd_pstate_max_freq:3514000
cpuinfo_max_freq:2151
cpuinfo_min_freq:400000
scaling_cur_freq:2151
scaling_max_freq:2151
scaling_min_freq:2151
[cpufreq]# pwd
/sys/devices/system/cpu/cpu0/cpufreq
According to Documentation/admin-guide/pm/cpufreq.rst, all the frequency
values need to be in KHz.
Thanks,
Dhananjay
>>
>> Cc: stable@vger.kernel.org
>> Fixes: ec437d71db7 ("cpufreq: amd-pstate: Introduce a new AMD P-State driver to support future processors")
>> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
>> Signed-off-by: Perry Yuan <perry.yuan@amd.com>
>> ---
>> drivers/cpufreq/amd-pstate.c | 3 +--
>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
>> index 08e112444c27..ac7faa98a450 100644
>> --- a/drivers/cpufreq/amd-pstate.c
>> +++ b/drivers/cpufreq/amd-pstate.c
>> @@ -640,8 +640,7 @@ static int amd_get_nominal_freq(struct amd_cpudata *cpudata)
>> if (ret)
>> return ret;
>>
>> - /* Switch to khz */
>> - return cppc_perf.nominal_freq * 1000;
>> + return cppc_perf.nominal_freq;
>> }
>>
>> static int amd_get_lowest_nonlinear_freq(struct amd_cpudata *cpudata)
>> --
>> 2.34.1
>>
© 2016 - 2025 Red Hat, Inc.