Hi Sivaprasad,

LGTM except for some trivial comments inline,
With belows to change, you can add
Acked-by: Huisong Li <lihuis...@huawei.com>

/Huisong
在 2024/10/23 13:11, 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.

v8:
  - marked rte_power_logtype as internal
  - removed c++ guards for internal header files
  - renamed rte_power_cpufreq_api.h for naming convention
  - renamed rte_power_register_ops for naming convention

v6:
  - fixed compilation error with symbol export in API
  - exported power_get_lcore_mapped_cpu_id as internal API to be
    used in drivers/power/*

v5:
  - fixed code style warning

v4:
  - fixed build error with RTE_ASSERT

v3:
  - renamed rte_power_core_ops.h as rte_power_cpufreq_api.h
  - re-worked on auto detection logic

v2:
  - added NULL check for global_core_ops in rte_power_get_core_ops

Signed-off-by: Sivaprasad Tummala <sivaprasad.tumm...@amd.com>
---
<snip>
+
+/**
+ * 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.
Not the index in the table, need to fix it.
BTW, this API always success now. so no return value.
+ */
+__rte_internal
+int rte_power_register_cpufreq_ops(struct rte_power_cpufreq_ops *ops);
+
+/**
+ * Macro to statically register the ops of a cpufreq driver.
+ */
+#define RTE_POWER_REGISTER_CPUFREQ_OPS(ops) \
+RTE_INIT(power_hdlr_init_##ops) \
+{ \
+       rte_power_register_cpufreq_ops(&ops); \
+}
+
+#endif
diff --git a/lib/power/rte_power.c b/lib/power/rte_power.c
index 36c3f3da98..3168b6d301 100644
--- a/lib/power/rte_power.c
+++ b/lib/power/rte_power.c
@@ -6,155 +6,88 @@
#include <rte_errno.h>
  #include <rte_spinlock.h>
+#include <rte_debug.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_cpufreq_ops *global_cpufreq_ops;
static rte_spinlock_t global_env_cfg_lock = RTE_SPINLOCK_INITIALIZER;
-
-/* 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)
+static RTE_TAILQ_HEAD(, rte_power_cpufreq_ops) cpufreq_ops_list =
+                       TAILQ_HEAD_INITIALIZER(cpufreq_ops_list);
+
+const char *power_env_str[] = {
+       "not set",
+       "acpi",
+       "kvm-vm",
+       "pstate",
+       "cppc",
+       "amd-pstate"
+};

How use the "not set"? I don't know what its usage is. Do we need to consider removing it later?

+
+/* register the ops struct in rte_power_cpufreq_ops, return 0 on success. */
+int
+rte_power_register_cpufreq_ops(struct rte_power_cpufreq_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 cpufreq 
ops");
+               return -EINVAL;
+       }
+
+       TAILQ_INSERT_TAIL(&cpufreq_ops_list, driver_ops, next);
+
+       return 0;
  }
suggest that change function return value as above mention.
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_cpufreq_ops *ops;
+
+       if (env >= RTE_DIM(power_env_str))
+               return 0;
+
+       RTE_TAILQ_FOREACH(ops, &cpufreq_ops_list, next)
+               if (strncmp(ops->name, power_env_str[env],
+                               RTE_POWER_DRIVER_NAMESZ) == 0)
+                       return ops->check_env_support();
+
+       return 0;
  }
<snip>

Reply via email to