RE: [PATCH 2/2] cpufreq/amd-pstate-ut: Add test case for mode switches

[Yuan, Perry]

[AMD Official Use Only - AMD Internal Distribution Only]

> -----Original Message-----
> From: Mario Limonciello <superm1@xxxxxxxxxx>
> Sent: Sunday, September 1, 2024 10:49 AM
> To: Meng, Li (Jassmine) <Li.Meng@xxxxxxx>; Shenoy, Gautham Ranjal
> <gautham.shenoy@xxxxxxx>; Yuan, Perry <Perry.Yuan@xxxxxxx>
> Cc: open list:X86 ARCHITECTURE (32-BIT AND 64-BIT) <linux-
> kernel@xxxxxxxxxxxxxxx>; open list:CPU FREQUENCY SCALING FRAMEWORK
> <linux-pm@xxxxxxxxxxxxxxx>; ptr1337@xxxxxxxxxxx; Limonciello, Mario
> <Mario.Limonciello@xxxxxxx>
> Subject: [PATCH 2/2] cpufreq/amd-pstate-ut: Add test case for mode
> switches
>
> From: Mario Limonciello <mario.limonciello@xxxxxxx>
>
> There is a state machine in the amd-pstate driver utilized for switches for all
> modes. To make sure that cleanup and setup works properly for each mode
> add a unit test case that tries all combinations.
>
> Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>
> ---
>  drivers/cpufreq/amd-pstate-ut.c | 41
> ++++++++++++++++++++++++++++++++-
>  1 file changed, 40 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/cpufreq/amd-pstate-ut.c b/drivers/cpufreq/amd-pstate-
> ut.c index b7318669485e4..c291b3dbec381 100644
> --- a/drivers/cpufreq/amd-pstate-ut.c
> +++ b/drivers/cpufreq/amd-pstate-ut.c
> @@ -54,12 +54,14 @@ 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 struct amd_pstate_ut_struct amd_pstate_ut_cases[] = {
>       {"amd_pstate_ut_acpi_cpc_valid",   amd_pstate_ut_acpi_cpc_valid   },
>       {"amd_pstate_ut_check_enabled",
> amd_pstate_ut_check_enabled    },
>       {"amd_pstate_ut_check_perf",       amd_pstate_ut_check_perf       },
> -     {"amd_pstate_ut_check_freq",       amd_pstate_ut_check_freq       }
> +     {"amd_pstate_ut_check_freq",       amd_pstate_ut_check_freq       },
> +     {"amd_pstate_ut_check_driver",
> amd_pstate_ut_check_driver     }
>  };
>
>  static bool get_shared_mem(void)
> @@ -257,6 +259,43 @@ static void amd_pstate_ut_check_freq(u32 index)
>       cpufreq_cpu_put(policy);
>  }
>
> +static int amd_pstate_set_mode(enum amd_pstate_mode mode) {
> +     const char *mode_str = amd_pstate_get_mode_string(mode);
> +
> +     pr_debug("->setting mode to %s\n", mode_str);
> +
> +     return amd_pstate_update_status(mode_str, strlen(mode_str)); }
> +
> +static void amd_pstate_ut_check_driver(u32 index) {
> +     enum amd_pstate_mode mode1, mode2;
> +     int ret;
> +
> +     for (mode1 = AMD_PSTATE_DISABLE; mode1 < AMD_PSTATE_MAX;
> mode1++) {
> +             ret = amd_pstate_set_mode(mode1);
> +             if (ret)
> +                     goto out;
> +             for (mode2 = AMD_PSTATE_DISABLE; mode2 <
> AMD_PSTATE_MAX; mode2++) {
> +                     if (mode1 == mode2)
> +                             continue;
> +                     ret = amd_pstate_set_mode(mode2);
> +                     if (ret)
> +                             goto out;
> +             }
> +     }

Dose the mode switching test need to add some delay between the previous and new mode ?
If lowlevel power firmware failed to handle the modes frequently,  you can consider adding delay in future.
Besides that, the patch looks good to me.

Reviewed-by: Perry Yuan <perry.yuan@xxxxxxx>

> +out:
> +     if (ret)
> +             pr_warn("%s: failed to update status for %s->%s: %d\n",
> __func__,
> +                     amd_pstate_get_mode_string(mode1),
> +                     amd_pstate_get_mode_string(mode2), ret);
> +
> +     amd_pstate_ut_cases[index].result = ret ?
> +                                         AMD_PSTATE_UT_RESULT_FAIL :
> +                                         AMD_PSTATE_UT_RESULT_PASS;
> +}
> +
>  static int __init amd_pstate_ut_init(void)  {
>       u32 i = 0, arr_size = ARRAY_SIZE(amd_pstate_ut_cases);
> --
> 2.43.0