| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Z7MescMK+rIX9QtK@BLRRASHENOY1.amd.com>
Date: Mon, 17 Feb 2025 17:04:09 +0530
From: "Gautham R. Shenoy" <gautham.shenoy@....com>
To: Mario Limonciello <superm1@...nel.org>
Cc: Perry Yuan <perry.yuan@....com>,
Dhananjay Ugwekar <Dhananjay.Ugwekar@....com>,
"open list:X86 ARCHITECTURE (32-BIT AND 64-BIT)" <linux-kernel@...r.kernel.org>,
"open list:CPU FREQUENCY SCALING FRAMEWORK" <linux-pm@...r.kernel.org>,
Mario Limonciello <mario.limonciello@....com>
Subject: Re: [PATCH v2 08/17] cpufreq/amd-pstate-ut: Drop SUCCESS and FAIL
enums
On Fri, Feb 14, 2025 at 06:52:35PM -0600, Mario Limonciello wrote:
> From: Mario Limonciello <mario.limonciello@....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.
Yeah, having the failure code in the error message is helpful.
Reviewed-by: Gautham R. Shenoy <gautham.shenoy@....com>
--
Thanks and Regards
gautham.
>
> Signed-off-by: Mario Limonciello <mario.limonciello@....com>
> ---
> v2:
> * new patch
>
> 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;
>
> 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;
> @@ -142,9 +127,8 @@ static void amd_pstate_ut_check_perf(u32 index)
> if (get_shared_mem()) {
> ret = cppc_get_perf_caps(cpu, &cppc_perf);
> if (ret) {
> - amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_FAIL;
> pr_err("%s cppc_get_perf_caps ret=%d error!\n", __func__, ret);
> - return;
> + return ret;
> }
>
> highest_perf = cppc_perf.highest_perf;
> @@ -154,9 +138,8 @@ static void amd_pstate_ut_check_perf(u32 index)
> } else {
> ret = rdmsrl_safe_on_cpu(cpu, MSR_AMD_CPPC_CAP1, &cap1);
> if (ret) {
> - amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_FAIL;
> pr_err("%s read CPPC_CAP1 ret=%d error!\n", __func__, ret);
> - return;
> + return ret;
> }
>
> highest_perf = AMD_CPPC_HIGHEST_PERF(cap1);
> @@ -169,32 +152,30 @@ static void amd_pstate_ut_check_perf(u32 index)
> if (highest_perf != cur_perf.highest_perf && !cpudata->hw_prefcore) {
> pr_err("%s cpu%d highest=%d %d highest perf doesn't match\n",
> __func__, cpu, highest_perf, cpudata->perf.highest_perf);
> - return;
> + return -EINVAL;
> }
> if (nominal_perf != cur_perf.nominal_perf ||
> (lowest_nonlinear_perf != cur_perf.lowest_nonlinear_perf) ||
> (lowest_perf != cur_perf.lowest_perf)) {
> - amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_FAIL;
> pr_err("%s cpu%d nominal=%d %d lowest_nonlinear=%d %d lowest=%d %d, they should be equal!\n",
> __func__, cpu, nominal_perf, cpudata->perf.nominal_perf,
> lowest_nonlinear_perf, cpudata->perf.lowest_nonlinear_perf,
> lowest_perf, cpudata->perf.lowest_perf);
> - return;
> + return -EINVAL;
> }
>
> if (!((highest_perf >= nominal_perf) &&
> (nominal_perf > lowest_nonlinear_perf) &&
> (lowest_nonlinear_perf >= lowest_perf) &&
> (lowest_perf > 0))) {
> - amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_FAIL;
> pr_err("%s cpu%d highest=%d >= nominal=%d > lowest_nonlinear=%d > lowest=%d > 0, the formula is incorrect!\n",
> __func__, cpu, highest_perf, nominal_perf,
> lowest_nonlinear_perf, lowest_perf);
> - return;
> + return -EINVAL;
> }
> }
>
> - amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_PASS;
> + return 0;
> }
>
> /*
> @@ -202,7 +183,7 @@ static void amd_pstate_ut_check_perf(u32 index)
> * max_freq >= nominal_freq > lowest_nonlinear_freq > min_freq > 0
> * check max freq when set support boost mode.
> */
> -static void amd_pstate_ut_check_freq(u32 index)
> +static int amd_pstate_ut_check_freq(u32 index)
> {
> int cpu = 0;
> struct amd_cpudata *cpudata = NULL;
> @@ -219,39 +200,33 @@ static void amd_pstate_ut_check_freq(u32 index)
> (cpudata->nominal_freq > cpudata->lowest_nonlinear_freq) &&
> (cpudata->lowest_nonlinear_freq >= policy->cpuinfo.min_freq) &&
> (policy->cpuinfo.min_freq > 0))) {
> - amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_FAIL;
> pr_err("%s cpu%d max=%d >= nominal=%d > lowest_nonlinear=%d > min=%d > 0, the formula is incorrect!\n",
> __func__, cpu, policy->cpuinfo.max_freq, cpudata->nominal_freq,
> cpudata->lowest_nonlinear_freq, policy->cpuinfo.min_freq);
> - return;
> + return -EINVAL;
> }
>
> if (cpudata->lowest_nonlinear_freq != policy->min) {
> - amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_FAIL;
> pr_err("%s cpu%d cpudata_lowest_nonlinear_freq=%d policy_min=%d, they should be equal!\n",
> __func__, cpu, cpudata->lowest_nonlinear_freq, policy->min);
> - return;
> + return -EINVAL;
> }
>
> if (cpudata->boost_supported) {
> - if ((policy->max == policy->cpuinfo.max_freq) ||
> - (policy->max == cpudata->nominal_freq))
> - amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_PASS;
> - else {
> - amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_FAIL;
> + if ((policy->max != policy->cpuinfo.max_freq) &&
> + (policy->max != cpudata->nominal_freq)) {
> pr_err("%s cpu%d policy_max=%d should be equal cpu_max=%d or cpu_nominal=%d !\n",
> __func__, cpu, policy->max, policy->cpuinfo.max_freq,
> cpudata->nominal_freq);
> - return;
> + return -EINVAL;
> }
> } else {
> - amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_FAIL;
> pr_err("%s cpu%d must support boost!\n", __func__, cpu);
> - return;
> + return -EINVAL;
> }
> }
>
> - amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_PASS;
> + return 0;
> }
>
> static int amd_pstate_set_mode(enum amd_pstate_mode mode)
> @@ -263,32 +238,28 @@ static int amd_pstate_set_mode(enum amd_pstate_mode mode)
> return amd_pstate_update_status(mode_str, strlen(mode_str));
> }
>
> -static void amd_pstate_ut_check_driver(u32 index)
> +static int amd_pstate_ut_check_driver(u32 index)
> {
> enum amd_pstate_mode mode1, mode2 = AMD_PSTATE_DISABLE;
> - int ret;
>
> for (mode1 = AMD_PSTATE_DISABLE; mode1 < AMD_PSTATE_MAX; mode1++) {
> - ret = amd_pstate_set_mode(mode1);
> + int ret = amd_pstate_set_mode(mode1);
> if (ret)
> - goto out;
> + return ret;
> for (mode2 = AMD_PSTATE_DISABLE; mode2 < AMD_PSTATE_MAX; mode2++) {
> if (mode1 == mode2)
> continue;
> ret = amd_pstate_set_mode(mode2);
> - if (ret)
> - goto out;
> + if (ret) {
> + pr_err("%s: failed to update status for %s->%s\n", __func__,
> + amd_pstate_get_mode_string(mode1),
> + amd_pstate_get_mode_string(mode2));
> + return ret;
> + }
> }
> }
> -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;
> +
> + return 0;
> }
>
> static int __init amd_pstate_ut_init(void)
> @@ -296,16 +267,12 @@ static int __init amd_pstate_ut_init(void)
> u32 i = 0, arr_size = ARRAY_SIZE(amd_pstate_ut_cases);
>
> for (i = 0; i < arr_size; i++) {
> - amd_pstate_ut_cases[i].func(i);
> - switch (amd_pstate_ut_cases[i].result) {
> - case AMD_PSTATE_UT_RESULT_PASS:
> + int ret = amd_pstate_ut_cases[i].func(i);
> +
> + if (ret)
> + pr_err("%-4d %-20s\t fail: %d!\n", i+1, amd_pstate_ut_cases[i].name, ret);
> + else
> pr_info("%-4d %-20s\t success!\n", i+1, amd_pstate_ut_cases[i].name);
> - break;
> - case AMD_PSTATE_UT_RESULT_FAIL:
> - default:
> - pr_info("%-4d %-20s\t fail!\n", i+1, amd_pstate_ut_cases[i].name);
> - break;
> - }
> }
>
> return 0;
> --
> 2.43.0
>
Powered by blists - more mailing lists