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

[Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>]

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> 

> 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 ?

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]