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

[Mario Limonciello]

On 2/24/2025 00:05, Dhananjay Ugwekar wrote:
> On 2/20/2025 2:32 AM, Mario Limonciello wrote:
> > From: Mario Limonciello <mario.limonciello@xxxxxxx>
> >
> > 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@xxxxxxx>

Thanks!

> > Reviewed-by: Gautham R. Shenoy <gautham.shenoy@xxxxxxx>
> > Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>
> > ---
> >   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]