Re: [PATCH v4 09/19] cpufreq/amd-pstate-ut: Drop SUCCESS and FAIL enums

Mario Limonciello
2025-02-25T02:35:17Z

On 2/24/2025 00:05, Dhananjay Ugwekar wrote:
> On 2/20/2025 2:32 AM, Mario Limonciello wrote:
>> From: Mario Limonciello <mario.limonciello@amd.com>
>>
>> Enums are effectively used as a boolean and don't show
>> the return value of the failing call.
>>
>> Instead of using enums switch to returning the actual return
>> code from the unit test.
>>
>
> One query below, apart from that LGTM,
>
> Reviewed-by: Dhananjay Ugwekar <dhananjay.ugwekar@amd.com>

Thanks!

>
>> Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com>
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>> drivers/cpufreq/amd-pstate-ut.c | 143 ++++++++++++--------------------
>> 1 file changed, 55 insertions(+), 88 deletions(-)
>>
>> diff --git a/drivers/cpufreq/amd-pstate-ut.c b/drivers/cpufreq/amd-pstate-ut.c
>> index 0f0b867e271cc..028527a0019ca 100644
>> --- a/drivers/cpufreq/amd-pstate-ut.c
>> +++ b/drivers/cpufreq/amd-pstate-ut.c
>> @@ -32,30 +32,20 @@
>>
>> #include "amd-pstate.h"
>>
>> -/*
>> - * Abbreviations:
>> - * amd_pstate_ut: used as a shortform for AMD P-State unit test.
>> - * It helps to keep variable names smaller, simpler
>> - */
>> -enum amd_pstate_ut_result {
>> - AMD_PSTATE_UT_RESULT_PASS,
>> - AMD_PSTATE_UT_RESULT_FAIL,
>> -};
>>
>> struct amd_pstate_ut_struct {
>> const char *name;
>> - void (*func)(u32 index);
>> - enum amd_pstate_ut_result result;
>> + int (*func)(u32 index);
>> };
>>
>> /*
>> * Kernel module for testing the AMD P-State unit test
>> */
>> -static void amd_pstate_ut_acpi_cpc_valid(u32 index);
>> -static void amd_pstate_ut_check_enabled(u32 index);
>> -static void amd_pstate_ut_check_perf(u32 index);
>> -static void amd_pstate_ut_check_freq(u32 index);
>> -static void amd_pstate_ut_check_driver(u32 index);
>> +static int amd_pstate_ut_acpi_cpc_valid(u32 index);
>> +static int amd_pstate_ut_check_enabled(u32 index);
>> +static int amd_pstate_ut_check_perf(u32 index);
>> +static int amd_pstate_ut_check_freq(u32 index);
>> +static int amd_pstate_ut_check_driver(u32 index);
>>
>> static struct amd_pstate_ut_struct amd_pstate_ut_cases[] = {
>> {"amd_pstate_ut_acpi_cpc_valid", amd_pstate_ut_acpi_cpc_valid },
>> @@ -78,51 +68,46 @@ static bool get_shared_mem(void)
>> /*
>> * check the _CPC object is present in SBIOS.
>> */
>> -static void amd_pstate_ut_acpi_cpc_valid(u32 index)
>> +static int amd_pstate_ut_acpi_cpc_valid(u32 index)
>> {
>> - if (acpi_cpc_valid())
>> - amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_PASS;
>> - else {
>> - amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_FAIL;
>> + if (!acpi_cpc_valid()) {
>> pr_err("%s the _CPC object is not present in SBIOS!\n", __func__);
>> + return -EINVAL;
>> }
>> +
>> + return 0;
>> }
>>
>> -static void amd_pstate_ut_pstate_enable(u32 index)
>> +/*
>> + * check if amd pstate is enabled
>> + */
>> +static int amd_pstate_ut_check_enabled(u32 index)
>> {
>> - int ret = 0;
>> u64 cppc_enable = 0;
>> + int ret;
>> +
>> + if (get_shared_mem())
>> + return 0;
>
> What do you think about adding a "cppc_get_enable()" function in acpi_cppc.c so that we can
> run this check for shared mem systems as well ?
>

I think it's a good idea. Would you mind working that out for after
this series lands?

> Thanks,
> Dhananjay
>
>>
>> ret = rdmsrl_safe(MSR_AMD_CPPC_ENABLE, &cppc_enable);
>> if (ret) {
>> - amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_FAIL;
>> pr_err("%s rdmsrl_safe MSR_AMD_CPPC_ENABLE ret=%d error!\n", __func__, ret);
>> - return;
>> + return ret;
>> }
>> - if (cppc_enable)
>> - amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_PASS;
>> - else {
>> - amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_FAIL;
>> +
>> + if (!cppc_enable) {
>> pr_err("%s amd pstate must be enabled!\n", __func__);
>> + return -EINVAL;
>> }
>> -}
>>
>> -/*
>> - * check if amd pstate is enabled
>> - */
>> -static void amd_pstate_ut_check_enabled(u32 index)
>> -{
>> - if (get_shared_mem())
>> - amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_PASS;
>> - else
>> - amd_pstate_ut_pstate_enable(index);
>> + return 0;
>> }
>>
>> /*
>> * check if performance values are reasonable.
>> * highest_perf >= nominal_perf > lowest_nonlinear_perf > lowest_perf > 0
>> */
>> -static void amd_pstate_ut_check_perf(u32 index)
>> +static int amd_pstate_ut_check_perf(u32 index)
>> {
>> int cpu = 0, ret = 0;
>> u32 highest_perf = 0, nominal_perf = 0, lowest_nonlinear_perf = 0, lowest_perf = 0;
> [Snip]