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

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

On 2/25/2025 5:35 AM, Mario Limonciello wrote:
> On 2/24/2025 00:05, Dhananjay Ugwekar wrote:
>> 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>
> 
> Thanks!
> 
>>
>>> 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 ?
>>
> 
> I think it's a good idea.  Would you mind working that out for after this series lands?

Sure, I'll take it up.

> 
>> Thanks,
>> Dhananjay