RE: [PATCH v2 1/4] power: refactor core power management library

["Tummala, Sivaprasad" <Sivaprasad.Tummala@amd.com>]

[AMD Official Use Only - AMD Internal Distribution Only]

Hi Huisong,

Please find my response inline.

> -----Original Message-----
> From: lihuisong (C) <lihuisong@huawei.com>
> Sent: Tuesday, August 27, 2024 1:51 PM
> To: Tummala, Sivaprasad <Sivaprasad.Tummala@amd.com>
> Cc: dev@dpdk.org; david.hunt@intel.com; anatoly.burakov@intel.com;
> radu.nicolau@intel.com; cristian.dumitrescu@intel.com; jerinj@marvell.com;
> konstantin.ananyev@huawei.com; Yigit, Ferruh <Ferruh.Yigit@amd.com>;
> gakhil@marvell.com
> Subject: Re: [PATCH v2 1/4] power: refactor core power management library
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> Hi Sivaprasa,
>
> Some comments inline.
>
> /Huisong
>
> 在 2024/8/26 21:06, 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.
> >
> > v2:
> >   - added NULL check for global_core_ops in rte_power_get_core_ops
> >
> > Signed-off-by: Sivaprasad Tummala <sivaprasad.tummala@amd.com>
> > ---
> >   drivers/meson.build                           |   1 +
> >   .../power/acpi/acpi_cpufreq.c                 |  22 +-
> >   .../power/acpi/acpi_cpufreq.h                 |   6 +-
> >   drivers/power/acpi/meson.build                |  10 +
> >   .../power/amd_pstate/amd_pstate_cpufreq.c     |  24 +-
> >   .../power/amd_pstate/amd_pstate_cpufreq.h     |   8 +-
> >   drivers/power/amd_pstate/meson.build          |  10 +
> >   .../power/cppc/cppc_cpufreq.c                 |  22 +-
> >   .../power/cppc/cppc_cpufreq.h                 |   8 +-
> >   drivers/power/cppc/meson.build                |  10 +
> >   .../power/kvm_vm}/guest_channel.c             |   0
> >   .../power/kvm_vm}/guest_channel.h             |   0
> >   .../power/kvm_vm/kvm_vm.c                     |  22 +-
> >   .../power/kvm_vm/kvm_vm.h                     |   6 +-
> >   drivers/power/kvm_vm/meson.build              |  16 +
> >   drivers/power/meson.build                     |  12 +
> >   drivers/power/pstate/meson.build              |  10 +
> >   .../power/pstate/pstate_cpufreq.c             |  22 +-
> >   .../power/pstate/pstate_cpufreq.h             |   6 +-
> >   lib/power/meson.build                         |   7 +-
> >   lib/power/power_common.c                      |   2 +-
> >   lib/power/power_common.h                      |  16 +-
> >   lib/power/rte_power.c                         | 291 ++++++------------
> >   lib/power/rte_power.h                         | 139 ++++++---
> >   lib/power/rte_power_core_ops.h                | 208 +++++++++++++
> >   lib/power/version.map                         |  14 +
> >   26 files changed, 621 insertions(+), 271 deletions(-)
> >   rename lib/power/power_acpi_cpufreq.c => drivers/power/acpi/acpi_cpufreq.c
> (95%)
> >   rename lib/power/power_acpi_cpufreq.h => drivers/power/acpi/acpi_cpufreq.h
> (98%)
> >   create mode 100644 drivers/power/acpi/meson.build
> >   rename lib/power/power_amd_pstate_cpufreq.c =>
> drivers/power/amd_pstate/amd_pstate_cpufreq.c (95%)
> >   rename lib/power/power_amd_pstate_cpufreq.h =>
> drivers/power/amd_pstate/amd_pstate_cpufreq.h (97%)
> >   create mode 100644 drivers/power/amd_pstate/meson.build
> >   rename lib/power/power_cppc_cpufreq.c => drivers/power/cppc/cppc_cpufreq.c
> (95%)
> >   rename lib/power/power_cppc_cpufreq.h => drivers/power/cppc/cppc_cpufreq.h
> (97%)
> >   create mode 100644 drivers/power/cppc/meson.build
> >   rename {lib/power => drivers/power/kvm_vm}/guest_channel.c (100%)
> >   rename {lib/power => drivers/power/kvm_vm}/guest_channel.h (100%)
> >   rename lib/power/power_kvm_vm.c => drivers/power/kvm_vm/kvm_vm.c (82%)
> >   rename lib/power/power_kvm_vm.h => drivers/power/kvm_vm/kvm_vm.h (98%)
> >   create mode 100644 drivers/power/kvm_vm/meson.build
> >   create mode 100644 drivers/power/meson.build
> >   create mode 100644 drivers/power/pstate/meson.build
> >   rename lib/power/power_pstate_cpufreq.c =>
> drivers/power/pstate/pstate_cpufreq.c (96%)
> >   rename lib/power/power_pstate_cpufreq.h =>
> drivers/power/pstate/pstate_cpufreq.h (98%)
> >   create mode 100644 lib/power/rte_power_core_ops.h
> How about use the following directory structure?
> *For power libs*
> lib/power/power_common.*
> lib/power/rte_power_pmd_mgmt.*
> lib/power/rte_power_cpufreq_api.* （replacing rte_power.c file maybe simple for us.
> but I'm not sure if we can put the init of core, uncore and pmd mgmt to
> rte_power_init.c in rte_power.c.)
> lib/power/rte_power_uncore_freq_api.*
Yes, renaming rte_power.c is definitely a possible incremental change that could be considered later.
However, for the time being, our focus will be on refactoring the cpufreq drivers only.
>
> *And has directories under drivers/power:*
> 1> For core dvfs driver:
> drivers/power/cpufreq/acpi_cpufreq.c
> drivers/power/cpufreq/cppc_cpufreq.c
> drivers/power/cpufreq/amd_pstate_cpufreq.c
> drivers/power/cpufreq/intel_pstate_cpufreq.c
> drivers/power/cpufreq/kvm_cpufreq.c
> The code of each cpufreq driver is not too much and doesn't probably increase. So
> don't need to use a directory for it.
>
> 2> For uncore dvfs driver:
> drivers/power/uncorefreq/intel_uncore.*
> > diff --git a/drivers/meson.build b/drivers/meson.build index
> > 66931d4241..9d77e0deab 100644
> > --- a/drivers/meson.build
> > +++ b/drivers/meson.build
> > @@ -29,6 +29,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/lib/power/power_acpi_cpufreq.c
> > b/drivers/power/acpi/acpi_cpufreq.c
> > similarity index 95%
> > rename from lib/power/power_acpi_cpufreq.c rename to
> > drivers/power/acpi/acpi_cpufreq.c
> do not suggest to create one directory for each cpufreq driver.
> Because pstate drivers also comply with ACPI spec, right？
> In addition, the code of each cpufreq drivers are not too much.
> There is just one file under one directory which is not good.
One of our objectives for the refactoring is to selectively disable non-essential drivers using Meson build options.
However, by rearranging the driver structure, we risk disrupting this capability.
> > index 81996e1c13..8637c69703 100644
> > --- a/lib/power/power_acpi_cpufreq.c
> > +++ b/drivers/power/acpi/acpi_cpufreq.c
> > @@ -10,7 +10,7 @@
> >   #include <rte_stdatomic.h>
> >   #include <rte_string_fns.h>
> >
> > -#include "power_acpi_cpufreq.h"
> > +#include "acpi_cpufreq.h"
> >   #include "power_common.h"
> >
> <...>
> > +if not is_linux
> > +    build = false
> > +    reason = 'only supported on Linux'
> > +endif
> > +sources = files('pstate_cpufreq.c')
> > +
> > +deps += ['power']
> > diff --git a/lib/power/power_pstate_cpufreq.c
> > b/drivers/power/pstate/pstate_cpufreq.c
> > similarity index 96%
> > rename from lib/power/power_pstate_cpufreq.c rename to
> > drivers/power/pstate/pstate_cpufreq.c
> pstate_cpufreq.c is actually intel_pstate cpufreq driver, right?
> So how about modify this file name to intel_pstate_cpufreq.c?
Yes, will fix this in next version.
> > index 2343121621..c32b1adabc 100644
> > --- a/lib/power/power_pstate_cpufreq.c
> > +++ b/drivers/power/pstate/pstate_cpufreq.c
> > @@ -15,7 +15,7 @@
> >   #include <rte_stdatomic.h>
> >
> >   #include "rte_power_pmd_mgmt.h"
> > -#include "power_pstate_cpufreq.h"
> > +#include "pstate_cpufreq.h"
> >   #include "power_common.h"
> >
> >   /* macros used for rounding frequency to nearest 100000 */ @@ -888,3
> > +888,23 @@ int power_pstate_get_capabilities(unsigned int lcore_id,
> >
> >       return 0;
> >   }
> > +
> <...>
> > diff --git a/lib/power/power_common.c b/lib/power/power_common.c index
> > 590986d5ef..6c06411e8b 100644
> > --- a/lib/power/power_common.c
> > +++ b/lib/power/power_common.c
> > @@ -12,7 +12,7 @@
> >
> >   #include "power_common.h"
> >
> > -RTE_LOG_REGISTER_DEFAULT(power_logtype, INFO);
> > +RTE_LOG_REGISTER_DEFAULT(rte_power_logtype, INFO);
> >
> >   #define POWER_SYSFILE_SCALING_DRIVER   \
> >               "/sys/devices/system/cpu/cpu%u/cpufreq/scaling_driver"
> > diff --git a/lib/power/power_common.h b/lib/power/power_common.h index
> > 83f742f42a..767686ee12 100644
> > --- a/lib/power/power_common.h
> > +++ b/lib/power/power_common.h
> > @@ -6,12 +6,13 @@
> >   #define _POWER_COMMON_H_
> >
> >   #include <rte_common.h>
> > +#include <rte_compat.h>
> >   #include <rte_log.h>
> >
> >   #define RTE_POWER_INVALID_FREQ_INDEX (~0)
> >
> > -extern int power_logtype;
> > -#define RTE_LOGTYPE_POWER power_logtype
> > +extern int rte_power_logtype;
> > +#define RTE_LOGTYPE_POWER rte_power_logtype
> >   #define POWER_LOG(level, ...) \
> >       RTE_LOG_LINE(level, POWER, "" __VA_ARGS__)
> >
> > @@ -23,13 +24,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);
> suggest that move cpufreq interfaces like this to the
> rte_power_cpufreq_api.* I proposed above.
This is an internal API and isn’t intended for direct use by applications.
By moving it to rte_power_*, we risk exposing it inadvertently.

> The interfaces in power_comm.* can be used by all power modules, like
> core/uncore/pmd mgmt.
> > +
> > +__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
> The name of the rte_power.c file is impropriate now. The context in this file is just for
> cpufreq, right?
> So I suggest that we need to rename this file as the rte_power_cpufreq_api.c
Yes, renaming rte_power.c to rte_power_cpufreq.c is definitely a possible incremental change
and will fix this as a separate patch.
.

> > index 36c3f3da98..2bf6d40517 100644
> > --- a/lib/power/rte_power.c
> > +++ b/lib/power/rte_power.c
> > @@ -8,153 +8,86 @@
> >   #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;
> > +static enum power_management_env global_default_env =
> PM_ENV_NOT_SET;
> > +static struct rte_power_core_ops *global_power_core_ops;
> >
> >   static rte_spinlock_t global_env_cfg_lock =
> > RTE_SPINLOCK_INITIALIZER;
> > +static RTE_TAILQ_HEAD(, rte_power_core_ops) core_ops_list =
> > +                     TAILQ_HEAD_INITIALIZER(core_ops_list);
> >
> > -/* 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)
> > +
> > +const char *power_env_str[] = {
> > +     "not set",
> > +     "acpi",
> > +     "kvm-vm",
> > +     "pstate",
> > +     "cppc",
> > +     "amd-pstate"
> > +};
> > +
> > +/* register the ops struct in rte_power_core_ops, return 0 on
> > +success. */ int rte_power_register_ops(struct rte_power_core_ops
> > +*driver_ops)
> >   {
> > -     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;
> > +     if (!driver_ops->init || !driver_ops->exit ||
> > +             !driver_ops->check_env_support || !driver_ops->get_avail_freqs ||
> > +             !driver_ops->get_freq || !driver_ops->set_freq ||
> > +             !driver_ops->freq_up || !driver_ops->freq_down ||
> > +             !driver_ops->freq_max || !driver_ops->freq_min ||
> > +             !driver_ops->turbo_status || !driver_ops->enable_turbo ||
> > +             !driver_ops->disable_turbo || !driver_ops->get_caps) {
> > +             POWER_LOG(ERR, "Missing callbacks while registering
> > + power ops");
> turbo_status(), enable_turbo() and disable turbo() are not necessary, right?
Nope, this is required to get the current status unlike the capability API (get_caps()).
> These depand on the capabilities from get_caps().
> > +             return -EINVAL;
> > +     }
> > +
> > +     TAILQ_INSERT_TAIL(&core_ops_list, driver_ops, next);
> > +
> > +     return 0;
> >   }
> >
> >   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_core_ops *ops;
> > +
> > +     if (env >= RTE_DIM(power_env_str))
> > +             return 0;
> > +
> > +     RTE_TAILQ_FOREACH(ops, &core_ops_list, next)
> > +             if (strncmp(ops->name, power_env_str[env],
> > +                             RTE_POWER_DRIVER_NAMESZ) == 0)
> > +                     return ops->check_env_support();
> > +
> > +     return 0;
> >   }
> >
> >   int
> >   rte_power_set_env(enum power_management_env env)
> >   {
> > +     struct rte_power_core_ops *ops;
> > +     int ret = -1;
> > +
> >       rte_spinlock_lock(&global_env_cfg_lock);
> >
> >       if (global_default_env != PM_ENV_NOT_SET) {
> >               POWER_LOG(ERR, "Power Management Environment already set.");
> > -             rte_spinlock_unlock(&global_env_cfg_lock);
> > -             return -1;
> > -     }
> > -
> <...>
> > -     if (ret == 0)
> > -             global_default_env = env;
> > -     else {
> > -             global_default_env = PM_ENV_NOT_SET;
> > -             reset_power_function_ptrs();
> > -     }
> > +     RTE_TAILQ_FOREACH(ops, &core_ops_list, next)
> > +             if (strncmp(ops->name, power_env_str[env],
> > +                             RTE_POWER_DRIVER_NAMESZ) == 0) {
> > +                     global_power_core_ops = ops;
> > +                     global_default_env = env;
> > +                     ret = 0;
> > +                     goto out;
> > +             }
> > +     POWER_LOG(ERR, "Invalid Power Management Environment(%d) set",
> > +                     env);
> >
> > +out:
> >       rte_spinlock_unlock(&global_env_cfg_lock);
> >       return ret;
> >   }
> > @@ -164,94 +97,66 @@ rte_power_unset_env(void)
> >   {
> >       rte_spinlock_lock(&global_env_cfg_lock);
> >       global_default_env = PM_ENV_NOT_SET;
> > -     reset_power_function_ptrs();
> > +     global_power_core_ops = NULL;
> >       rte_spinlock_unlock(&global_env_cfg_lock);
> >   }
> >
> >   enum power_management_env
> > -rte_power_get_env(void) {
> > +rte_power_get_env(void)
> > +{
> >       return global_default_env;
> >   }
> >
> > -int
> > -rte_power_init(unsigned int lcore_id)
> > +struct rte_power_core_ops *
> > +rte_power_get_core_ops(void)
> >   {
> > -     int ret = -1;
> > +     RTE_ASSERT(global_power_core_ops != NULL);
> >
> > -     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!");
> > -     }
> > +     return global_power_core_ops;
> > +}
> >
> > -     /* 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;
> > -     }
> > +int
> > +rte_power_init(unsigned int lcore_id) {
> > +     struct rte_power_core_ops *ops;
> > +     uint8_t env;
> >
> > -     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;
> > -     }
> > +     if (global_default_env != PM_ENV_NOT_SET)
> > +             return global_power_core_ops->init(lcore_id);
> >
> > -     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, "Env isn't set yet!");
> remove this log?
> >
> > -     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;
> > -     }
> > +     /* Auto detect Environment */
> > +     RTE_TAILQ_FOREACH(ops, &core_ops_list, next)
> > +             if (ops) {
> > +                     POWER_LOG(INFO,
> > +                             "Attempting to initialise %s cpufreq power management...",
> > +                             ops->name);
> > +                     if (ops->init(lcore_id) == 0) {
> > +                             for (env = 0; env < RTE_DIM(power_env_str); env++)
> > +                                     if (strncmp(ops->name, power_env_str[env],
> > +                                                     RTE_POWER_DRIVER_NAMESZ) == 0) {
> > +                                             rte_power_set_env(env);
> > +                                             return 0;
> > +                                     }
> > +                     }
> > +             }
> Can we change the logic of rte_power_set_env()? like:
>      RTE_TAILQ_FOREACH(ops, &core_ops_list, next) {
>          for (env = 0; env < RTE_DIM(power_env_str); env++) {
>                  if (strncmp(ops->name, power_env_str[env],
> RTE_POWER_DRIVER_NAMESZ) == 0 &&
>                      ops->init(lcore_id) == 0) {
>                      global_power_core_ops = ops;
>                      global_default_env = env;
>                  }
>          }
>      }
> That is easier to follow code.
Yes, will fix in next version.

> > +
> > +     POWER_LOG(ERR,
> > +             "Unable to set Power Management Environment for lcore %u",
> > +             lcore_id);
> >
> > -     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(ERR, "Unable to set Power Management Environment for lcore
> "
> > -                     "%u", lcore_id);
> > -out:
> > -     return ret;
> > +     return -1;
> >   }
> >
> >   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");
> > +     if (global_default_env != PM_ENV_NOT_SET)
> > +             return global_power_core_ops->exit(lcore_id);
> >
> > -     }
> > -     return -1;
> > +     POWER_LOG(ERR,
> > +             "Environment has not been set, unable to exit
> > + gracefully");
> >
> > +     return -1;
> >   }
> > diff --git a/lib/power/rte_power.h b/lib/power/rte_power.h index
> > 4fa4afe399..5e4aacf08b 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 Advanced Micro Devices, Inc.
> >    */
> >
> >   #ifndef _RTE_POWER_H
> > @@ -14,14 +15,21 @@
> >   #include <rte_log.h>
> >   #include <rte_power_guest_channel.h>
> >
> > +#include "rte_power_core_ops.h"
> > +
> >   #ifdef __cplusplus
> >   extern "C" {
> >   #endif
> >
> >   /* 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};
> > +enum power_management_env {
> > +     PM_ENV_NOT_SET = 0,
> > +     PM_ENV_ACPI_CPUFREQ,
> > +     PM_ENV_KVM_VM,
> > +     PM_ENV_PSTATE_CPUFREQ,
> > +     PM_ENV_CPPC_CPUFREQ,
> > +     PM_ENV_AMD_PSTATE_CPUFREQ
> > +};
> >
> >   /**
> >    * Check if a specific power management environment type is
> > supported on a @@ -66,6 +74,15 @@ void rte_power_unset_env(void);
> >    */
> >   enum power_management_env rte_power_get_env(void);
>
> I'd like to let user not know used which cpufreq driver, which is friendly to user.
>
> So we can rethink if this API is necessary.
For any API changes, could we handle this as a separate RFC for discussion?
It’s important that these changes are not included within the scope of this patch.
>
> >
> > +/**
> > + * @internal Get the power ops struct from its index.
> > + *
> > + * @return
> > + *   The pointer to the ops struct in the table if registered.
> > + */
> > +struct rte_power_core_ops *
> > +rte_power_get_core_ops(void);
> > +
> >   /**
> >    * 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 +125,13 @@ 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_core_ops *ops = rte_power_get_core_ops();
> >
> > -extern rte_power_freqs_t rte_power_freqs;
> > +     return ops->get_avail_freqs(lcore_id, freqs, n); }
> >
> >   /**
> >    * Return the current index of available frequencies of a specific lcore.
> > @@ -124,9 +144,13 @@ extern rte_power_freqs_t rte_power_freqs;
> >    * @return
> >    *  The current index of available frequencies.
> >    */
> > -typedef uint32_t (*rte_power_get_freq_t)(unsigned int lcore_id);
> > +static inline uint32_t
> > +rte_power_get_freq(unsigned int lcore_id) {
> > +     struct rte_power_core_ops *ops = rte_power_get_core_ops();
> >
> > -extern rte_power_get_freq_t rte_power_get_freq;
> > +     return ops->get_freq(lcore_id);
> > +}
> >
> >   /**
> >    * Set the new frequency for a specific lcore by indicating the
> > index of @@ -144,82 +168,101 @@ extern rte_power_get_freq_t
> rte_power_get_freq;
> >    *  - 0 on success without frequency changed.
> >    *  - Negative on error.
> >    */
> > -typedef int (*rte_power_set_freq_t)(unsigned int lcore_id, uint32_t
> > index);
> > -
> > -extern rte_power_set_freq_t rte_power_set_freq;
> > +static inline uint32_t
> > +rte_power_set_freq(unsigned int lcore_id, uint32_t index) {
> > +     struct rte_power_core_ops *ops = rte_power_get_core_ops();
> >
> > -/**
> > - * 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.
> > - */
> > -typedef int (*rte_power_freq_change_t)(unsigned int lcore_id);
> > +     return ops->set_freq(lcore_id, index); }
> >
> >   /**
> >    * Scale up the frequency of a specific lcore according to the available
> >    * frequencies.
> >    * Review each environments specific documentation for usage.
> >    */
> > -extern rte_power_freq_change_t rte_power_freq_up;
> > +static inline int
> > +rte_power_freq_up(unsigned int lcore_id) {
> > +     struct rte_power_core_ops *ops = rte_power_get_core_ops();
> > +
> > +     return ops->freq_up(lcore_id);
> > +}
> >
> >   /**
> >    * Scale down the frequency of a specific lcore according to the available
> >    * frequencies.
> >    * Review each environments specific documentation for usage.
> >    */
> > -extern rte_power_freq_change_t rte_power_freq_down;
> > +static inline int
> > +rte_power_freq_down(unsigned int lcore_id) {
> > +     struct rte_power_core_ops *ops = rte_power_get_core_ops();
> > +
> > +     return ops->freq_down(lcore_id); }
> >
> >   /**
> >    * Scale up the frequency of a specific lcore to the highest according to the
> >    * available frequencies.
> >    * Review each environments specific documentation for usage.
> >    */
> > -extern rte_power_freq_change_t rte_power_freq_max;
> > +static inline int
> > +rte_power_freq_max(unsigned int lcore_id) {
> > +     struct rte_power_core_ops *ops = rte_power_get_core_ops();
> > +
> > +     return ops->freq_max(lcore_id);
> > +}
> >
> >   /**
> >    * Scale down the frequency of a specific lcore to the lowest according to the
> >    * available frequencies.
> >    * Review each environments specific documentation for usage..
> >    */
> > -extern rte_power_freq_change_t rte_power_freq_min;
> > +static inline int
> > +rte_power_freq_min(unsigned int lcore_id) {
> > +     struct rte_power_core_ops *ops = rte_power_get_core_ops();
> > +
> > +     return ops->freq_min(lcore_id);
> > +}
> >
> >   /**
> >    * Query the Turbo Boost status of a specific lcore.
> >    * Review each environments specific documentation for usage..
> >    */
> > -extern rte_power_freq_change_t rte_power_turbo_status;
> > +static inline int
> > +rte_power_turbo_status(unsigned int lcore_id) {
> > +     struct rte_power_core_ops *ops = rte_power_get_core_ops();
> > +
> > +     return ops->turbo_status(lcore_id); }
> >
> >   /**
> >    * Enable Turbo Boost for this lcore.
> >    * Review each environments specific documentation for usage..
> >    */
> > -extern rte_power_freq_change_t rte_power_freq_enable_turbo;
> > +static inline int
> > +rte_power_freq_enable_turbo(unsigned int lcore_id) {
> > +     struct rte_power_core_ops *ops = rte_power_get_core_ops();
> > +
> > +     return ops->enable_turbo(lcore_id); }
> >
> >   /**
> >    * Disable Turbo Boost for this lcore.
> >    * Review each environments specific documentation for usage..
> >    */
> > -extern rte_power_freq_change_t rte_power_freq_disable_turbo;
> > +static inline int
> > +rte_power_freq_disable_turbo(unsigned int lcore_id) {
> > +     struct rte_power_core_ops *ops = rte_power_get_core_ops();
> >
> > -/**
> > - * 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 */
> > -             };
> > -     };
> > -};
> > +     return ops->disable_turbo(lcore_id); }
> >
> >   /**
> >    * Returns power capabilities for a specific lcore.
> > @@ -235,10 +278,14 @@ struct rte_power_core_capabilities {
> >    *  - 0 on success.
> >    *  - Negative on error.
> >    */
> > -typedef int (*rte_power_get_capabilities_t)(unsigned int lcore_id,
> > -             struct rte_power_core_capabilities *caps);
> > +static inline int
> > +rte_power_get_capabilities(unsigned int lcore_id,
> > +             struct rte_power_core_capabilities *caps) {
> > +     struct rte_power_core_ops *ops = rte_power_get_core_ops();
> >
> > -extern rte_power_get_capabilities_t rte_power_get_capabilities;
> > +     return ops->get_caps(lcore_id, caps); }
> >
> >   #ifdef __cplusplus
> >   }
> > diff --git a/lib/power/rte_power_core_ops.h
> > b/lib/power/rte_power_core_ops.h new file mode 100644 index
> > 0000000000..356a64df79
> > --- /dev/null
> > +++ b/lib/power/rte_power_core_ops.h
> > @@ -0,0 +1,208 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright(c) 2010-2014 Intel Corporation
> > + * Copyright(c) 2024 Advanced Micro Devices, Inc.
> > + */
> > +
> > +#ifndef _RTE_POWER_CORE_OPS_H
> > +#define _RTE_POWER_CORE_OPS_H
> > +
> suggest rename the file as rte_power_cpufreq_api.h.
> If so, the role of this file is more clearly.
> > +__rte_internal
> > +int rte_power_register_ops(struct rte_power_core_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.
> > + *
> > + * @return
> > + *   The pointer to the ops struct in the table if registered.
> > + */
> > +struct rte_power_core_ops *
> > +rte_power_get_core_ops(void);
> > +
> > +#ifdef __cplusplus
> > +}
> > +#endif
> > +
> > +#endif
> > diff --git a/lib/power/version.map b/lib/power/version.map index
> > c9a226614e..bd64e0828f 100644
> > --- a/lib/power/version.map
> > +++ b/lib/power/version.map
> > @@ -51,4 +51,18 @@ EXPERIMENTAL {
> >       rte_power_set_uncore_env;
> >       rte_power_uncore_freqs;
> >       rte_power_unset_uncore_env;
> > +     # added in 24.07
> 24.07-->24.11?
> > +     rte_power_logtype;
> > +};
> > +
> > +INTERNAL {
> > +     global:
> > +
> > +     rte_power_register_ops;
> > +     cpufreq_check_scaling_driver;
> > +     power_set_governor;
> > +     open_core_sysfs_file;
> > +     read_core_sysfs_u32;
> > +     read_core_sysfs_s;
> > +     write_core_sysfs_s;
> >   };