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@amd.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 {
--
2.34.1On 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@amd.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@amd.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 {
Hello Mario,
On 6/25/2024 8:35 PM, Mario Limonciello wrote:
> 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@amd.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")
Makes sense, will update the tag.
Thanks for reviewing!
Regards,
Dhananjay
>
> Reviewed-by: Mario Limonciello <mario.limonciello@amd.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 {
>
Minor modification, the commit subject is supposed to be
"cpufreq/amd-pstate-ut: Handle the inconsistency between nominal_freq and other *_freq units"
The second half disappeared due to the word wrapping I guess.
Regards,
Dhananjay
On 6/25/2024 7:11 PM, 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@amd.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 {
On 6/25/2024 08:51, Dhananjay Ugwekar wrote:
> Minor modification, the commit subject is supposed to be
> "cpufreq/amd-pstate-ut: Handle the inconsistency between nominal_freq and other *_freq units"
>
> The second half disappeared due to the word wrapping I guess.
I had some other feedback on the series, so when you submit a v2 can you
try to fix the title on the first patch?
>
> Regards,
> Dhananjay
>
> On 6/25/2024 7:11 PM, 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@amd.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 {
Hello Mario,
On 6/25/2024 8:41 PM, Mario Limonciello wrote:
> On 6/25/2024 08:51, Dhananjay Ugwekar wrote:
>> Minor modification, the commit subject is supposed to be
>> "cpufreq/amd-pstate-ut: Handle the inconsistency between nominal_freq and other *_freq units"
>>
>> The second half disappeared due to the word wrapping I guess.
>
> I had some other feedback on the series, so when you submit a v2 can you try to fix the title on the first patch?
Yup, will do!
Thanks,
Dhananjay
>
>>
>> Regards,
>> Dhananjay
>>
>> On 6/25/2024 7:11 PM, 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@amd.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 {
>
© 2016 - 2025 Red Hat, Inc.