Re: [RFC PATCH 1/2] power: refactor core power management library

["Hunt, David" <david.hunt@intel.com>]

On 01/03/2024 02:56, lihuisong (C) wrote:
>
> 在 2024/2/20 23:33, Sivaprasad Tummala 写道:
>> This patch introduces a comprehensive refactor to the core power
>> management library. The primary focus is on improving modularity
>> and organization by relocating specific driver implementations
>> from the 'lib/power' directory to dedicated directories within
>> 'drivers/power/core/*'. The adjustment of meson.build files
>> enables the selective activation of individual drivers.
>>
>> These changes contribute to a significant enhancement in code
>> organization, providing a clearer structure for driver implementations.
>> The refactor aims to improve overall code clarity and boost
>> maintainability. Additionally, it establishes a foundation for
>> future development, allowing for more focused work on individual
>> drivers and seamless integration of forthcoming enhancements.
>
> Good job. +1 to refacotor.
>
> <...>
>

Also a +1 from me, looks like a sensible re-organisation of the power code.

Regards,
Dave.



>> diff --git a/drivers/meson.build b/drivers/meson.build
>> index f2be71bc05..e293c3945f 100644
>> --- a/drivers/meson.build
>> +++ b/drivers/meson.build
>> @@ -28,6 +28,7 @@ subdirs = [
>>           'event',          # depends on common, bus, mempool and net.
>>           'baseband',       # depends on common and bus.
>>           'gpu',            # depends on common and bus.
>> +        'power',          # depends on common (in future).
>>   ]
>>     if meson.is_cross_build()
>> diff --git a/drivers/power/core/acpi/meson.build 
>> b/drivers/power/core/acpi/meson.build
>> new file mode 100644
>> index 0000000000..d10ec8ee94
>> --- /dev/null
>> +++ b/drivers/power/core/acpi/meson.build
>> @@ -0,0 +1,8 @@
>> +# SPDX-License-Identifier: BSD-3-Clause
>> +# Copyright(c) 2024 AMD Limited
>> +
>> +sources = files('power_acpi_cpufreq.c')
>> +
>> +headers = files('power_acpi_cpufreq.h')
>> +
>> +deps += ['power']
>> diff --git a/lib/power/power_acpi_cpufreq.c 
>> b/drivers/power/core/acpi/power_acpi_cpufreq.c
>> similarity index 95%
>> rename from lib/power/power_acpi_cpufreq.c
>> rename to drivers/power/core/acpi/power_acpi_cpufreq.c
> This file is in power lib.
> How about remove the 'power' prefix of this file name?
> like acpi_cpufreq.c, cppc_cpufreq.c.
>> index f8d978d03d..69d80ad2ae 100644
>> --- a/lib/power/power_acpi_cpufreq.c
>> +++ b/drivers/power/core/acpi/power_acpi_cpufreq.c
>> @@ -577,3 +577,22 @@ int power_acpi_get_capabilities(unsigned int 
>> lcore_id,
>>         return 0;
>>   }
>> +
>> +static struct rte_power_ops acpi_ops = {
> How about use the following structure name?
> "struct rte_power_cpufreq_ops" or "struct rte_power_core_ops"
> After all, we also have other power ops, like uncore, right?
>> +    .init = power_acpi_cpufreq_init,
>> +    .exit = power_acpi_cpufreq_exit,
>> +    .check_env_support = power_acpi_cpufreq_check_supported,
>> +    .get_avail_freqs = power_acpi_cpufreq_freqs,
>> +    .get_freq = power_acpi_cpufreq_get_freq,
>> +    .set_freq = power_acpi_cpufreq_set_freq,
>> +    .freq_down = power_acpi_cpufreq_freq_down,
>> +    .freq_up = power_acpi_cpufreq_freq_up,
>> +    .freq_max = power_acpi_cpufreq_freq_max,
>> +    .freq_min = power_acpi_cpufreq_freq_min,
>> +    .turbo_status = power_acpi_turbo_status,
>> +    .enable_turbo = power_acpi_enable_turbo,
>> +    .disable_turbo = power_acpi_disable_turbo,
>> +    .get_caps = power_acpi_get_capabilities
>> +};
>> +
>> +RTE_POWER_REGISTER_OPS(acpi_ops);
>> diff --git a/lib/power/power_acpi_cpufreq.h 
>> b/drivers/power/core/acpi/power_acpi_cpufreq.h
>> similarity index 100%
>> rename from lib/power/power_acpi_cpufreq.h
>> rename to drivers/power/core/acpi/power_acpi_cpufreq.h
>> diff --git a/drivers/power/core/amd-pstate/meson.build 
>> b/drivers/power/core/amd-pstate/meson.build
>> new file mode 100644
>> index 0000000000..8ec4c960f5
>> --- /dev/null
>> +++ b/drivers/power/core/amd-pstate/meson.build
>> @@ -0,0 +1,8 @@
>> +# SPDX-License-Identifier: BSD-3-Clause
>> +# Copyright(c) 2024 AMD Limited
>> +
>> +sources = files('power_amd_pstate_cpufreq.c')
>> +
>> +headers = files('power_amd_pstate_cpufreq.h')
>> +
>> +deps += ['power']
>> diff --git a/lib/power/power_amd_pstate_cpufreq.c 
>> b/drivers/power/core/amd-pstate/power_amd_pstate_cpufreq.c
>> similarity index 95%
>> rename from lib/power/power_amd_pstate_cpufreq.c
>> rename to drivers/power/core/amd-pstate/power_amd_pstate_cpufreq.c
>> index 028f84416b..9938de72a6 100644
>> --- a/lib/power/power_amd_pstate_cpufreq.c
>> +++ b/drivers/power/core/amd-pstate/power_amd_pstate_cpufreq.c
>> @@ -700,3 +700,22 @@ power_amd_pstate_get_capabilities(unsigned int 
>> lcore_id,
>>         return 0;
>>   }
>> +
>> +static struct rte_power_ops amd_pstate_ops = {
>> +    .init = power_amd_pstate_cpufreq_init,
>> +    .exit = power_amd_pstate_cpufreq_exit,
>> +    .check_env_support = power_amd_pstate_cpufreq_check_supported,
>> +    .get_avail_freqs = power_amd_pstate_cpufreq_freqs,
>> +    .get_freq = power_amd_pstate_cpufreq_get_freq,
>> +    .set_freq = power_amd_pstate_cpufreq_set_freq,
>> +    .freq_down = power_amd_pstate_cpufreq_freq_down,
>> +    .freq_up = power_amd_pstate_cpufreq_freq_up,
>> +    .freq_max = power_amd_pstate_cpufreq_freq_max,
>> +    .freq_min = power_amd_pstate_cpufreq_freq_min,
>> +    .turbo_status = power_amd_pstate_turbo_status,
>> +    .enable_turbo = power_amd_pstate_enable_turbo,
>> +    .disable_turbo = power_amd_pstate_disable_turbo,
>> +    .get_caps = power_amd_pstate_get_capabilities
>> +};
>> +
>> +RTE_POWER_REGISTER_OPS(amd_pstate_ops);
>> diff --git a/lib/power/power_amd_pstate_cpufreq.h 
>> b/drivers/power/core/amd-pstate/power_amd_pstate_cpufreq.h
>> similarity index 100%
>> rename from lib/power/power_amd_pstate_cpufreq.h
>> rename to drivers/power/core/amd-pstate/power_amd_pstate_cpufreq.h
>> diff --git a/drivers/power/core/cppc/meson.build 
>> b/drivers/power/core/cppc/meson.build
>> new file mode 100644
>> index 0000000000..06f3b99bb8
>> --- /dev/null
>> +++ b/drivers/power/core/cppc/meson.build
>> @@ -0,0 +1,8 @@
>> +# SPDX-License-Identifier: BSD-3-Clause
>> +# Copyright(c) 2024 AMD Limited
>> +
>> +sources = files('power_cppc_cpufreq.c')
>> +
>> +headers = files('power_cppc_cpufreq.h')
>> +
>> +deps += ['power']
>> diff --git a/lib/power/power_cppc_cpufreq.c 
>> b/drivers/power/core/cppc/power_cppc_cpufreq.c
>> similarity index 96%
>> rename from lib/power/power_cppc_cpufreq.c
>> rename to drivers/power/core/cppc/power_cppc_cpufreq.c
>> index 3ddf39bd76..605f633309 100644
>> --- a/lib/power/power_cppc_cpufreq.c
>> +++ b/drivers/power/core/cppc/power_cppc_cpufreq.c
>> @@ -685,3 +685,22 @@ power_cppc_get_capabilities(unsigned int lcore_id,
>>         return 0;
>>   }
>> +
>> +static struct rte_power_ops cppc_ops = {
>> +    .init = power_cppc_cpufreq_init,
>> +    .exit = power_cppc_cpufreq_exit,
>> +    .check_env_support = power_cppc_cpufreq_check_supported,
>> +    .get_avail_freqs = power_cppc_cpufreq_freqs,
>> +    .get_freq = power_cppc_cpufreq_get_freq,
>> +    .set_freq = power_cppc_cpufreq_set_freq,
>> +    .freq_down = power_cppc_cpufreq_freq_down,
>> +    .freq_up = power_cppc_cpufreq_freq_up,
>> +    .freq_max = power_cppc_cpufreq_freq_max,
>> +    .freq_min = power_cppc_cpufreq_freq_min,
>> +    .turbo_status = power_cppc_turbo_status,
>> +    .enable_turbo = power_cppc_enable_turbo,
>> +    .disable_turbo = power_cppc_disable_turbo,
>> +    .get_caps = power_cppc_get_capabilities
>> +};
>> +
>> +RTE_POWER_REGISTER_OPS(cppc_ops);
>> diff --git a/lib/power/power_cppc_cpufreq.h 
>> b/drivers/power/core/cppc/power_cppc_cpufreq.h
>> similarity index 100%
>> rename from lib/power/power_cppc_cpufreq.h
>> rename to drivers/power/core/cppc/power_cppc_cpufreq.h
>> diff --git a/lib/power/guest_channel.c 
>> b/drivers/power/core/kvm-vm/guest_channel.c
>> similarity index 100%
>> rename from lib/power/guest_channel.c
>> rename to drivers/power/core/kvm-vm/guest_channel.c
>> diff --git a/lib/power/guest_channel.h 
>> b/drivers/power/core/kvm-vm/guest_channel.h
>> similarity index 100%
>> rename from lib/power/guest_channel.h
>> rename to drivers/power/core/kvm-vm/guest_channel.h
>> diff --git a/drivers/power/core/kvm-vm/meson.build 
>> b/drivers/power/core/kvm-vm/meson.build
>> new file mode 100644
>> index 0000000000..3150c6674b
>> --- /dev/null
>> +++ b/drivers/power/core/kvm-vm/meson.build
>> @@ -0,0 +1,20 @@
>> +# SPDX-License-Identifier: BSD-3-Clause
>> +# Copyright(C) 2024 AMD Limited.
>> +#
>> +
>> +if not is_linux
>> +    build = false
>> +    reason = 'only supported on Linux'
>> +    subdir_done()
>> +endif
>> +
>> +sources = files(
>> +        'guest_channel.c',
>> +        'power_kvm_vm.c',
>> +)
>> +
>> +headers = files(
>> +        'guest_channel.h',
>> +        'power_kvm_vm.h',
>> +)
>> +deps += ['power']
>> diff --git a/lib/power/power_kvm_vm.c 
>> b/drivers/power/core/kvm-vm/power_kvm_vm.c
>> similarity index 83%
>> rename from lib/power/power_kvm_vm.c
>> rename to drivers/power/core/kvm-vm/power_kvm_vm.c
>> index f15be8fac5..a5d6984d26 100644
>> --- a/lib/power/power_kvm_vm.c
>> +++ b/drivers/power/core/kvm-vm/power_kvm_vm.c
>> @@ -137,3 +137,22 @@ int power_kvm_vm_get_capabilities(__rte_unused 
>> unsigned int lcore_id,
>>       POWER_LOG(ERR, "rte_power_get_capabilities is not implemented 
>> for Virtual Machine Power Management");
>>       return -ENOTSUP;
>>   }
>> +
>> +static struct rte_power_ops kvm_vm_ops = {
>> +    .init = power_kvm_vm_init,
>> +    .exit = power_kvm_vm_exit,
>> +    .check_env_support = power_kvm_vm_check_supported,
>> +    .get_avail_freqs = power_kvm_vm_freqs,
>> +    .get_freq = power_kvm_vm_get_freq,
>> +    .set_freq = power_kvm_vm_set_freq,
>> +    .freq_down = power_kvm_vm_freq_down,
>> +    .freq_up = power_kvm_vm_freq_up,
>> +    .freq_max = power_kvm_vm_freq_max,
>> +    .freq_min = power_kvm_vm_freq_min,
>> +    .turbo_status = power_kvm_vm_turbo_status,
>> +    .enable_turbo = power_kvm_vm_enable_turbo,
>> +    .disable_turbo = power_kvm_vm_disable_turbo,
>> +    .get_caps = power_kvm_vm_get_capabilities
>> +};
>> +
>> +RTE_POWER_REGISTER_OPS(kvm_vm_ops);
>> diff --git a/lib/power/power_kvm_vm.h 
>> b/drivers/power/core/kvm-vm/power_kvm_vm.h
>> similarity index 100%
>> rename from lib/power/power_kvm_vm.h
>> rename to drivers/power/core/kvm-vm/power_kvm_vm.h
>> diff --git a/drivers/power/core/meson.build 
>> b/drivers/power/core/meson.build
>> new file mode 100644
>> index 0000000000..4081dafaa0
>> --- /dev/null
>> +++ b/drivers/power/core/meson.build
>> @@ -0,0 +1,12 @@
>> +# SPDX-License-Identifier: BSD-3-Clause
>> +# Copyright(c) 2024 AMD Limited
>> +
>> +drivers = [
>> +        'acpi',
>> +        'amd-pstate',
>> +        'cppc',
>> +        'kvm-vm',
>> +        'pstate'
>> +]
>> +
>> +std_deps = ['power']
>> diff --git a/drivers/power/core/pstate/meson.build 
>> b/drivers/power/core/pstate/meson.build
>> new file mode 100644
>> index 0000000000..1025c64e48
>> --- /dev/null
>> +++ b/drivers/power/core/pstate/meson.build
>> @@ -0,0 +1,8 @@
>> +# SPDX-License-Identifier: BSD-3-Clause
>> +# Copyright(c) 2024 AMD Limited
>> +
>> +sources = files('power_pstate_cpufreq.c')
>> +
>> +headers = files('power_pstate_cpufreq.h')
>> +
>> +deps += ['power']
>> diff --git a/lib/power/power_pstate_cpufreq.c 
>> b/drivers/power/core/pstate/power_pstate_cpufreq.c
>> similarity index 96%
>> rename from lib/power/power_pstate_cpufreq.c
>> rename to drivers/power/core/pstate/power_pstate_cpufreq.c
>> index 73138dc4e4..d4c3645ff8 100644
>> --- a/lib/power/power_pstate_cpufreq.c
>> +++ b/drivers/power/core/pstate/power_pstate_cpufreq.c
>> @@ -888,3 +888,22 @@ int power_pstate_get_capabilities(unsigned int 
>> lcore_id,
>>         return 0;
>>   }
>> +
>> +static struct rte_power_ops pstate_ops = {
>> +    .init = power_pstate_cpufreq_init,
>> +    .exit = power_pstate_cpufreq_exit,
>> +    .check_env_support = power_pstate_cpufreq_check_supported,
>> +    .get_avail_freqs = power_pstate_cpufreq_freqs,
>> +    .get_freq = power_pstate_cpufreq_get_freq,
>> +    .set_freq = power_pstate_cpufreq_set_freq,
>> +    .freq_down = power_pstate_cpufreq_freq_down,
>> +    .freq_up = power_pstate_cpufreq_freq_up,
>> +    .freq_max = power_pstate_cpufreq_freq_max,
>> +    .freq_min = power_pstate_cpufreq_freq_min,
>> +    .turbo_status = power_pstate_turbo_status,
>> +    .enable_turbo = power_pstate_enable_turbo,
>> +    .disable_turbo = power_pstate_disable_turbo,
>> +    .get_caps = power_pstate_get_capabilities
>> +};
>> +
>> +RTE_POWER_REGISTER_OPS(pstate_ops);
>> diff --git a/lib/power/power_pstate_cpufreq.h 
>> b/drivers/power/core/pstate/power_pstate_cpufreq.h
>> similarity index 100%
>> rename from lib/power/power_pstate_cpufreq.h
>> rename to drivers/power/core/pstate/power_pstate_cpufreq.h
>> diff --git a/drivers/power/meson.build b/drivers/power/meson.build
>> new file mode 100644
>> index 0000000000..7d9034c7ac
>> --- /dev/null
>> +++ b/drivers/power/meson.build
>> @@ -0,0 +1,8 @@
>> +# SPDX-License-Identifier: BSD-3-Clause
>> +# Copyright(c) 2024 AMD Limited
>> +
>> +drivers = [
>> +        'core',
>> +]
>> +
>> +std_deps = ['power']
>> diff --git a/lib/power/meson.build b/lib/power/meson.build
>> index b8426589b2..207d96d877 100644
>> --- a/lib/power/meson.build
>> +++ b/lib/power/meson.build
>> @@ -12,14 +12,8 @@ if not is_linux
>>       reason = 'only supported on Linux'
>>   endif
>>   sources = files(
>> -        'guest_channel.c',
>> -        'power_acpi_cpufreq.c',
>> -        'power_amd_pstate_cpufreq.c',
>>           'power_common.c',
>> -        'power_cppc_cpufreq.c',
>> -        'power_kvm_vm.c',
>>           'power_intel_uncore.c',
>> -        'power_pstate_cpufreq.c',
>>           'rte_power.c',
>>           'rte_power_uncore.c',
>>           'rte_power_pmd_mgmt.c',
>> diff --git a/lib/power/power_common.h b/lib/power/power_common.h
>> index 30966400ba..c90b611f4f 100644
>> --- a/lib/power/power_common.h
>> +++ b/lib/power/power_common.h
>> @@ -23,13 +23,24 @@ extern int power_logtype;
>>   #endif
>>     /* check if scaling driver matches one we want */
>> +__rte_internal
>>   int cpufreq_check_scaling_driver(const char *driver);
>> +
>> +__rte_internal
>>   int power_set_governor(unsigned int lcore_id, const char 
>> *new_governor,
>>           char *orig_governor, size_t orig_governor_len);
>> +
>> +__rte_internal
>>   int open_core_sysfs_file(FILE **f, const char *mode, const char 
>> *format, ...)
>>           __rte_format_printf(3, 4);
>> +
>> +__rte_internal
>>   int read_core_sysfs_u32(FILE *f, uint32_t *val);
>> +
>> +__rte_internal
>>   int read_core_sysfs_s(FILE *f, char *buf, unsigned int len);
>> +
>> +__rte_internal
>>   int write_core_sysfs_s(FILE *f, const char *str);
>>     #endif /* _POWER_COMMON_H_ */
>> diff --git a/lib/power/rte_power.c b/lib/power/rte_power.c
>> index 36c3f3da98..70176807f4 100644
>> --- a/lib/power/rte_power.c
>> +++ b/lib/power/rte_power.c
>> @@ -8,64 +8,80 @@
>>   #include <rte_spinlock.h>
>>     #include "rte_power.h"
>> -#include "power_acpi_cpufreq.h"
>> -#include "power_cppc_cpufreq.h"
>>   #include "power_common.h"
>> -#include "power_kvm_vm.h"
>> -#include "power_pstate_cpufreq.h"
>> -#include "power_amd_pstate_cpufreq.h"
>>     enum power_management_env global_default_env = PM_ENV_NOT_SET;
> use a pointer to save the current power cpufreq ops?
>>     static rte_spinlock_t global_env_cfg_lock = 
>> RTE_SPINLOCK_INITIALIZER;
>> +static struct rte_power_ops rte_power_ops[PM_ENV_MAX];
>>   -/* function pointers */
>> -rte_power_freqs_t rte_power_freqs  = NULL;
>> -rte_power_get_freq_t rte_power_get_freq = NULL;
>> -rte_power_set_freq_t rte_power_set_freq = NULL;
>> -rte_power_freq_change_t rte_power_freq_up = NULL;
>> -rte_power_freq_change_t rte_power_freq_down = NULL;
>> -rte_power_freq_change_t rte_power_freq_max = NULL;
>> -rte_power_freq_change_t rte_power_freq_min = NULL;
>> -rte_power_freq_change_t rte_power_turbo_status;
>> -rte_power_freq_change_t rte_power_freq_enable_turbo;
>> -rte_power_freq_change_t rte_power_freq_disable_turbo;
>> -rte_power_get_capabilities_t rte_power_get_capabilities;
>> -
>> -static void
>> -reset_power_function_ptrs(void)
>> +/* register the ops struct in rte_power_ops, return 0 on success. */
>> +int
>> +rte_power_register_ops(const struct rte_power_ops *op)
>> +{
>> +    struct rte_power_ops *ops;
>> +
>> +    if (op->env >= PM_ENV_MAX) {
>> +        POWER_LOG(ERR, "Unsupported power management environment\n");
>> +        return -EINVAL;
>> +    }
>> +
>> +    if (op->status != 0) {
>> +        POWER_LOG(ERR, "Power management env[%d] ops registered 
>> already\n",
>> +            op->env);
>> +        return -EINVAL;
>> +    }
>> +
>> +    if (!op->init || !op->exit || !op->check_env_support ||
>> +        !op->get_avail_freqs || !op->get_freq || !op->set_freq ||
>> +        !op->freq_up || !op->freq_down || !op->freq_max ||
>> +        !op->freq_min || !op->turbo_status || !op->enable_turbo ||
>> +        !op->disable_turbo || !op->get_caps) {
>> +        POWER_LOG(ERR, "Missing callbacks while registering power 
>> ops\n");
>> +        return -EINVAL;
>> +    }
>> +
>> +    ops = &rte_power_ops[op->env];
> It is better to use a global linked list instead of an array.
> And we should extract a list structure including this ops structure 
> and this ops's owner.
>> +    ops->env = op->env;
>> +    ops->init = op->init;
>> +    ops->exit = op->exit;
>> +    ops->check_env_support = op->check_env_support;
>> +    ops->get_avail_freqs = op->get_avail_freqs;
>> +    ops->get_freq = op->get_freq;
>> +    ops->set_freq = op->set_freq;
>> +    ops->freq_up = op->freq_up;
>> +    ops->freq_down = op->freq_down;
>> +    ops->freq_max = op->freq_max;
>> +    ops->freq_min = op->freq_min;
>> +    ops->turbo_status = op->turbo_status;
>> +    ops->enable_turbo = op->enable_turbo;
>> +    ops->disable_turbo = op->disable_turbo;
> *ops = *op?
>> +    ops->status = 1; /* registered */
> status --> registered?
> But if use ops linked list, this flag also can be removed.
>> +
>> +    return 0;
>> +}
>> +
>> +struct rte_power_ops *
>> +rte_power_get_ops(int ops_index)
> AFAICS, there is only one cpufreq driver on one platform and just have 
> one power_cpufreq_ops to use for user.
> We don't need user to get other power ops, and user just want to know 
> the power ops using currently, right?
> So using 'index' toget this ops is not good.
>>   {
>> -    rte_power_freqs  = NULL;
>> -    rte_power_get_freq = NULL;
>> -    rte_power_set_freq = NULL;
>> -    rte_power_freq_up = NULL;
>> -    rte_power_freq_down = NULL;
>> -    rte_power_freq_max = NULL;
>> -    rte_power_freq_min = NULL;
>> -    rte_power_turbo_status = NULL;
>> -    rte_power_freq_enable_turbo = NULL;
>> -    rte_power_freq_disable_turbo = NULL;
>> -    rte_power_get_capabilities = NULL;
>> +    RTE_VERIFY((ops_index >= PM_ENV_NOT_SET) && (ops_index < 
>> PM_ENV_MAX));
>> +    RTE_VERIFY(rte_power_ops[ops_index].status != 0);
>> +
>> +    return &rte_power_ops[ops_index];
>>   }
>>     int
>>   rte_power_check_env_supported(enum power_management_env env)
>>   {
>> -    switch (env) {
>> -    case PM_ENV_ACPI_CPUFREQ:
>> -        return power_acpi_cpufreq_check_supported();
>> -    case PM_ENV_PSTATE_CPUFREQ:
>> -        return power_pstate_cpufreq_check_supported();
>> -    case PM_ENV_KVM_VM:
>> -        return power_kvm_vm_check_supported();
>> -    case PM_ENV_CPPC_CPUFREQ:
>> -        return power_cppc_cpufreq_check_supported();
>> -    case PM_ENV_AMD_PSTATE_CPUFREQ:
>> -        return power_amd_pstate_cpufreq_check_supported();
>> -    default:
>> -        rte_errno = EINVAL;
>> -        return -1;
>> +    struct rte_power_ops *ops;
>> +
>> +    if ((env > PM_ENV_NOT_SET) && (env < PM_ENV_MAX)) {
>> +        ops = rte_power_get_ops(env);
>> +        return ops->check_env_support();
>>       }
>> +
>> +    rte_errno = EINVAL;
>> +    return -1;
>>   }
>>     int
>> @@ -80,80 +96,26 @@ rte_power_set_env(enum power_management_env env)
>>       }
>>         int ret = 0;
>> +    struct rte_power_ops *ops;
>> +
>> +    if ((env == PM_ENV_NOT_SET) || (env >= PM_ENV_MAX)) {
>> +        POWER_LOG(ERR, "Invalid Power Management Environment(%d)"
>> +                " set\n", env);
>> +        ret = -1;
>> +    }
> <...>
>> +    ops = rte_power_get_ops(env);
> To find the target ops from the global list according to the env?
>> +    if (ops->status == 0) {
>> +        POWER_LOG(ERR, WER,
>> +            "Power Management Environment(%d) not"
>> +            " registered\n", env);
>>           ret = -1;
>>       }
>>         if (ret == 0)
>>           global_default_env = env;
> It is more convenient to use a global variable to point to the default 
> power_cpufreq ops or its list node.
>> -    else {
>> +    else
>>           global_default_env = PM_ENV_NOT_SET;
>> -        reset_power_function_ptrs();
>> -    }
>>         rte_spinlock_unlock(&global_env_cfg_lock);
>>       return ret;
>> @@ -164,7 +126,6 @@ rte_power_unset_env(void)
>>   {
>>       rte_spinlock_lock(&global_env_cfg_lock);
>>       global_default_env = PM_ENV_NOT_SET;
>> -    reset_power_function_ptrs();
>>       rte_spinlock_unlock(&global_env_cfg_lock);
>>   }
>>   @@ -177,59 +138,76 @@ int
>>   rte_power_init(unsigned int lcore_id)
>>   {
>>       int ret = -1;
>> +    struct rte_power_ops *ops;
>>   -    switch (global_default_env) {
>> -    case PM_ENV_ACPI_CPUFREQ:
>> -        return power_acpi_cpufreq_init(lcore_id);
>> -    case PM_ENV_KVM_VM:
>> -        return power_kvm_vm_init(lcore_id);
>> -    case PM_ENV_PSTATE_CPUFREQ:
>> -        return power_pstate_cpufreq_init(lcore_id);
>> -    case PM_ENV_CPPC_CPUFREQ:
>> -        return power_cppc_cpufreq_init(lcore_id);
>> -    case PM_ENV_AMD_PSTATE_CPUFREQ:
>> -        return power_amd_pstate_cpufreq_init(lcore_id);
>> -    default:
>> -        POWER_LOG(INFO, "Env isn't set yet!");
>> +    if (global_default_env != PM_ENV_NOT_SET) {
>> +        ops = &rte_power_ops[global_default_env];
>> +        if (!ops->status) {
>> +            POWER_LOG(ERR, "Power management env[%d] not"
>> +                " supported\n", global_default_env);
>> +            goto out;
>> +        }
>> +        return ops->init(lcore_id);
>>       }
>> +    POWER_LOG(INFO, POWER, "Env isn't set yet!\n");
>>         /* Auto detect Environment */
>> -    POWER_LOG(INFO, "Attempting to initialise ACPI cpufreq power 
>> management...");
>> -    ret = power_acpi_cpufreq_init(lcore_id);
>> -    if (ret == 0) {
>> -        rte_power_set_env(PM_ENV_ACPI_CPUFREQ);
>> -        goto out;
>> +    POWER_LOG(INFO, "Attempting to initialise ACPI cpufreq"
>> +            " power management...\n");
>> +    ops = &rte_power_ops[PM_ENV_ACPI_CPUFREQ];
>> +    if (ops->status) {
>> +        ret = ops->init(lcore_id);
>> +        if (ret == 0) {
>> +            rte_power_set_env(PM_ENV_ACPI_CPUFREQ);
>> +            goto out;
>> +        }
>>       }
>>   -    POWER_LOG(INFO, "Attempting to initialise PSTAT power 
>> management...");
>> -    ret = power_pstate_cpufreq_init(lcore_id);
>> -    if (ret == 0) {
>> -        rte_power_set_env(PM_ENV_PSTATE_CPUFREQ);
>> -        goto out;
>> +    POWER_LOG(INFO, "Attempting to initialise PSTAT"
>> +            " power management...\n");
>> +    ops = &rte_power_ops[PM_ENV_PSTATE_CPUFREQ];
>> +    if (ops->status) {
>> +        ret = ops->init(lcore_id);
>> +        if (ret == 0) {
>> +            rte_power_set_env(PM_ENV_PSTATE_CPUFREQ);
>> +            goto out;
>> +        }
>>       }
>>   -    POWER_LOG(INFO, "Attempting to initialise AMD PSTATE power 
>> management...");
>> -    ret = power_amd_pstate_cpufreq_init(lcore_id);
>> -    if (ret == 0) {
>> -        rte_power_set_env(PM_ENV_AMD_PSTATE_CPUFREQ);
>> -        goto out;
>> +    POWER_LOG(INFO,    "Attempting to initialise AMD PSTATE"
>> +            " power management...\n");
>> +    ops = &rte_power_ops[PM_ENV_AMD_PSTATE_CPUFREQ];
>> +    if (ops->status) {
>> +        ret = ops->init(lcore_id);
>> +        if (ret == 0) {
>> +            rte_power_set_env(PM_ENV_AMD_PSTATE_CPUFREQ);
>> +            goto out;
>> +        }
>>       }
>>   -    POWER_LOG(INFO, "Attempting to initialise CPPC power 
>> management...");
>> -    ret = power_cppc_cpufreq_init(lcore_id);
>> -    if (ret == 0) {
>> -        rte_power_set_env(PM_ENV_CPPC_CPUFREQ);
>> -        goto out;
>> +    POWER_LOG(INFO, "Attempting to initialise CPPC power"
>> +            " management...\n");
>> +    ops = &rte_power_ops[PM_ENV_CPPC_CPUFREQ];
>> +    if (ops->status) {
>> +        ret = ops->init(lcore_id);
>> +        if (ret == 0) {
>> +            rte_power_set_env(PM_ENV_CPPC_CPUFREQ);
>> +            goto out;
>> +        }
>>       }
>>   -    POWER_LOG(INFO, "Attempting to initialise VM power 
>> management...");
>> -    ret = power_kvm_vm_init(lcore_id);
>> -    if (ret == 0) {
>> -        rte_power_set_env(PM_ENV_KVM_VM);
>> -        goto out;
>> +    POWER_LOG(INFO, "Attempting to initialise VM power"
>> +            " management...\n");
>> +    ops = &rte_power_ops[PM_ENV_KVM_VM];
>> +    if (ops->status) {
>> +        ret = ops->init(lcore_id);
>> +        if (ret == 0) {
>> +            rte_power_set_env(PM_ENV_KVM_VM);
>> +            goto out;
>> +        }
>>       }
> If we use a linked list, above code can be simpled like this:
> ->
> for_each_power_cpufreq_ops(ops, ...) {
>     ret = ops->init()
>     if (ret) {
>         ....
>     }
> }
>> -    POWER_LOG(ERR, "Unable to set Power Management Environment for 
>> lcore "
>> -            "%u", lcore_id);
>> +    POWER_LOG(ERR, "Unable to set Power Management Environment"
>> +            " for lcore %u\n", lcore_id);
>>   out:
>>       return ret;
>>   }
>> @@ -237,21 +215,14 @@ rte_power_init(unsigned int lcore_id)
>>   int
>>   rte_power_exit(unsigned int lcore_id)
>>   {
>> -    switch (global_default_env) {
>> -    case PM_ENV_ACPI_CPUFREQ:
>> -        return power_acpi_cpufreq_exit(lcore_id);
>> -    case PM_ENV_KVM_VM:
>> -        return power_kvm_vm_exit(lcore_id);
>> -    case PM_ENV_PSTATE_CPUFREQ:
>> -        return power_pstate_cpufreq_exit(lcore_id);
>> -    case PM_ENV_CPPC_CPUFREQ:
>> -        return power_cppc_cpufreq_exit(lcore_id);
>> -    case PM_ENV_AMD_PSTATE_CPUFREQ:
>> -        return power_amd_pstate_cpufreq_exit(lcore_id);
>> -    default:
>> -        POWER_LOG(ERR, "Environment has not been set, unable to exit 
>> gracefully");
>> +    struct rte_power_ops *ops;
>>   +    if (global_default_env != PM_ENV_NOT_SET) {
>> +        ops = &rte_power_ops[global_default_env];
>> +        return ops->exit(lcore_id);
>>       }
>> -    return -1;
>> +    POWER_LOG(ERR, "Environment has not been set, unable "
>> +            "to exit gracefully\n");
>>   +    return -1;
>>   }
>> diff --git a/lib/power/rte_power.h b/lib/power/rte_power.h
>> index 4fa4afe399..749bb823ab 100644
>> --- a/lib/power/rte_power.h
>> +++ b/lib/power/rte_power.h
>> @@ -1,5 +1,6 @@
>>   /* SPDX-License-Identifier: BSD-3-Clause
>>    * Copyright(c) 2010-2014 Intel Corporation
>> + * Copyright(c) 2024 AMD Limited
>>    */
>>     #ifndef _RTE_POWER_H
>> @@ -21,7 +22,7 @@ extern "C" {
>>   /* Power Management Environment State */
>>   enum power_management_env {PM_ENV_NOT_SET, PM_ENV_ACPI_CPUFREQ, 
>> PM_ENV_KVM_VM,
>>           PM_ENV_PSTATE_CPUFREQ, PM_ENV_CPPC_CPUFREQ,
>> -        PM_ENV_AMD_PSTATE_CPUFREQ};
>> +        PM_ENV_AMD_PSTATE_CPUFREQ, PM_ENV_MAX};
> "enum power_management_env" is not good. may be like "enum 
> power_cpufreq_driver_type"?
> In previous linked list structure to be defined, may be directly use a 
> string name instead of a fixed enum is better.
> Becuase the new "PM_ENV_MAX" will  lead to break ABI when add a new 
> cpufreq driver.
>>     /**
>>    * Check if a specific power management environment type is 
>> supported on a
>> @@ -66,6 +67,97 @@ void rte_power_unset_env(void);
>>    */
>>   enum power_management_env rte_power_get_env(void);
>>   +typedef int (*rte_power_cpufreq_init_t)(unsigned int lcore_id);
>> +typedef int (*rte_power_cpufreq_exit_t)(unsigned int lcore_id);
>> +typedef int (*rte_power_check_env_support_t)(void);
>> +
>> +typedef uint32_t (*rte_power_freqs_t)(unsigned int lcore_id, 
>> uint32_t *freqs,
>> +                    uint32_t num);
>> +typedef uint32_t (*rte_power_get_freq_t)(unsigned int lcore_id);
>> +typedef int (*rte_power_set_freq_t)(unsigned int lcore_id, uint32_t 
>> index);
>> +typedef int (*rte_power_freq_change_t)(unsigned int lcore_id);
>> +
>> +/**
>> + * Function pointer definition for generic frequency change 
>> functions. Review
>> + * each environments specific documentation for usage.
>> + *
>> + * @param lcore_id
>> + *  lcore id.
>> + *
>> + * @return
>> + *  - 1 on success with frequency changed.
>> + *  - 0 on success without frequency changed.
>> + *  - Negative on error.
>> + */
>> +
>> +/**
>> + * Power capabilities summary.
>> + */
>> +struct rte_power_core_capabilities {
>> +    union {
>> +        uint64_t capabilities;
>> +        struct {
>> +            uint64_t turbo:1;       /**< Turbo can be enabled. */
>> +            uint64_t priority:1;    /**< SST-BF high freq core */
>> +        };
>> +    };
>> +};
>> +
>> +typedef int (*rte_power_get_capabilities_t)(unsigned int lcore_id,
>> +                struct rte_power_core_capabilities *caps);
>> +
>> +/** Structure defining core power operations structure */
>> +struct rte_power_ops {
>> +uint8_t status;                         /**< ops register status. */
>> +    enum power_management_env env;          /**< power mgmt env. */
>> +    rte_power_cpufreq_init_t init;    /**< Initialize power 
>> management. */
>> +    rte_power_cpufreq_exit_t exit;    /**< Exit power management. */
>> +    rte_power_check_env_support_t check_env_support; /**< verify env 
>> is supported. */
>> +    rte_power_freqs_t get_avail_freqs; /**< Get the available 
>> frequencies. */
>> +    rte_power_get_freq_t get_freq; /**< Get frequency index. */
>> +    rte_power_set_freq_t set_freq; /**< Set frequency index. */
>> +    rte_power_freq_change_t freq_up;   /**< Scale up frequency. */
>> +    rte_power_freq_change_t freq_down; /**< Scale down frequency. */
>> +    rte_power_freq_change_t freq_max;  /**< Scale up frequency to 
>> highest. */
>> +    rte_power_freq_change_t freq_min;  /**< Scale up frequency to 
>> lowest. */
>> +    rte_power_freq_change_t turbo_status; /**< Get Turbo status. */
>> +    rte_power_freq_change_t enable_turbo; /**< Enable Turbo. */
>> +    rte_power_freq_change_t disable_turbo; /**< Disable Turbo. */
>> +    rte_power_get_capabilities_t get_caps; /**< power capabilities. */
>> +} __rte_cache_aligned;
> Suggest that fix this sturcture, like:
> struct rte_power_cpufreq_list {
>     char name[];   // like "cppc_cpufreq", "pstate_cpufreq"
>     struct rte_power_cpufreq *ops;
>     struct rte_power_cpufreq_list *node;
> }
>> +
>> +/**
>> + * Register power cpu frequency operations.
>> + *
>> + * @param ops
>> + *   Pointer to an ops structure to register.
>> + * @return
>> + *   - >=0: Success; return the index of the ops struct in the table.
>> + *   - -EINVAL - error while registering ops struct.
>> + */
>> +__rte_internal
>> +int rte_power_register_ops(const struct rte_power_ops *ops);
>> +
>> +/**
>> + * Macro to statically register the ops of a cpufreq driver.
>> + */
>> +#define RTE_POWER_REGISTER_OPS(ops)        \
>> +    (RTE_INIT(power_hdlr_init_##ops)    \
>> +    {                    \
>> +        rte_power_register_ops(&ops);    \
>> +    })
>> +
>> +/**
>> + * @internal Get the power ops struct from its index.
>> + *
>> + * @param ops_index
>> + *   The index of the ops struct in the ops struct table.
>> + * @return
>> + *   The pointer to the ops struct in the table if registered.
>> + */
>> +struct rte_power_ops *
>> +rte_power_get_ops(int ops_index);
>> +
>>   /**
>>    * Initialize power management for a specific lcore. If 
>> rte_power_set_env() has
>>    * not been called then an auto-detect of the environment will 
>> start and
>> @@ -108,10 +200,14 @@ int rte_power_exit(unsigned int lcore_id);
>>    * @return
>>    *  The number of available frequencies.
>>    */
>> -typedef uint32_t (*rte_power_freqs_t)(unsigned int lcore_id, 
>> uint32_t *freqs,
>> -        uint32_t num);
>> +static inline uint32_t
>> +rte_power_freqs(unsigned int lcore_id, uint32_t *freqs, uint32_t n)
>> +{
>> +    struct rte_power_ops *ops;
>>   -extern rte_power_freqs_t rte_power_freqs;
>> +    ops = rte_power_get_ops(rte_power_get_env());
>> +    return ops->get_avail_freqs(lcore_id, freqs, n);
>> +}
> nice.
> <...>