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

[Mario Limonciello <superm1@kernel.org>]

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]