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

Dhananjay Ugwekar
2025-02-24T06:06:08Z

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]