Re: [PATCH v9 1/6] power: refactor core power management library

["lihuisong (C)" <lihuisong@huawei.com>]

Hi Sivaprasad,

LGTM except for some trivial comments inline,
With belows to change, you can add
Acked-by: Huisong Li <lihuisong@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.tummala@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>