|SUCCESS| [v2, 5/5] power: support automatic detection of uncore driver

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/164397

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-05-26

commit 1122aeb930 (from series.patch)
Author: Huisong Li <lihuisong at huawei.com>
Subject: [PATCH v2 1/5] examples/l3fwd-power: fix uncore deinit for non-legacy

The commit addresses missing uncore frequency deinitialization in
non-legacy modes by relocating the deinitialization logic outside
the legacy mode conditional block.

> diff --git a/examples/l3fwd-power/main.c b/examples/l3fwd-power/main.c
> index 02ec17d799..1122aeb930 100644
> --- a/examples/l3fwd-power/main.c
> +++ b/examples/l3fwd-power/main.c
> @@ -2271,28 +2271,31 @@ init_power_library(void)
>  	unsigned int lcore_id;
>  	int ret = 0;
>  
> -	RTE_LCORE_FOREACH(lcore_id) {
> -		/* init power management library */
> -		ret = rte_power_init(lcore_id);
> -		if (ret) {
> -			RTE_LOG(ERR, L3FWD_POWER,
> -				"Library initialization failed on core %u\n",
> -				lcore_id);
> -			return ret;
> -		}
> -		/* we're not supporting the VM channel mode */
> -		env = rte_power_get_env();
> -		if (env != PM_ENV_ACPI_CPUFREQ &&
> -				env != PM_ENV_PSTATE_CPUFREQ &&
> -				env != PM_ENV_AMD_PSTATE_CPUFREQ &&
> -				env != PM_ENV_CPPC_CPUFREQ) {
> -			RTE_LOG(ERR, L3FWD_POWER,
> -				"Only ACPI and PSTATE mode are supported\n");
> -			return -1;
> +	/* only legacy mode relies on the initialization of cpufreq library */
> +	if (app_mode == APP_MODE_LEGACY) {
> +		RTE_LCORE_FOREACH(lcore_id) {
> +			/* init power management library */
> +			ret = rte_power_init(lcore_id);
> +			if (ret) {
> +				RTE_LOG(ERR, L3FWD_POWER,
> +					"Library initialization failed on core %u\n",
> +					lcore_id);
> +				return ret;
> +			}
> +			/* we're not supporting the VM channel mode */
> +			env = rte_power_get_env();
> +			if (env != PM_ENV_ACPI_CPUFREQ &&
> +					env != PM_ENV_PSTATE_CPUFREQ &&
> +					env != PM_ENV_AMD_PSTATE_CPUFREQ &&
> +					env != PM_ENV_CPPC_CPUFREQ) {
> +				RTE_LOG(ERR, L3FWD_POWER,
> +					"Only ACPI and PSTATE mode are supported\n");
> +				return -1;
> +			}
>  		}
>  	}

Does init_power_library() properly clean up the initialized lcore resources
when rte_power_init() fails partway through in app_mode APP_MODE_LEGACY?

If rte_power_init() fails on lcore N after successfully initializing lcores
0 through N-1, does this code return immediately without calling
rte_power_exit() on the successfully initialized lcores?

[ ... ]

> @@ -2329,14 +2332,16 @@ deinit_power_library(void)
>  	unsigned int lcore_id, max_pkg, max_die, die, pkg;
>  	int ret = 0;
>  
> -	RTE_LCORE_FOREACH(lcore_id) {
> -		/* deinit power management library */
> -		ret = rte_power_exit(lcore_id);
> -		if (ret) {
> -			RTE_LOG(ERR, L3FWD_POWER,
> -				"Library deinitialization failed on core %u\n",
> -				lcore_id);
> -			return ret;
> +	if (app_mode == APP_MODE_LEGACY) {
> +		RTE_LCORE_FOREACH(lcore_id) {
> +			/* deinit power management library */
> +			ret = rte_power_exit(lcore_id);
> +			if (ret) {
> +				RTE_LOG(ERR, L3FWD_POWER,
> +					"Library deinitialization failed on core %u\n",
> +					lcore_id);
> +				return ret;
> +			}
>  		}
>  	}

Does deinit_power_library() still leak uncore resources when
rte_power_exit() fails on an early lcore in APP_MODE_LEGACY?

When rte_power_exit() returns an error for lcore N, does the function
return early and skip uncore cleanup for all packages and dies?

[ ... ]

commit ba2be5bf32
Author: Huisong Li <lihuisong at huawei.com>
Subject: [PATCH v2 2/5] examples/l3fwd-power: enable power QoS for all modes

The commit enables power QoS feature for all application modes by
removing the app_mode conditional check.

[ ... ]

commit 45b6697c85
Author: Huisong Li <lihuisong at huawei.com>
Subject: [PATCH v2 3/5] examples/l3fwd-power: fix uncore help and log info

The commit corrects the help text and error messages for uncore
frequency options.

[ ... ]

commit a22634a04e
Author: Huisong Li <lihuisong at huawei.com>
Subject: [PATCH v2 4/5] examples/l3fwd-power: relocate uncore initialization

The commit moves uncore initialization from parse_args() to
init_power_library() to improve code organization.

> diff --git a/examples/l3fwd-power/main.c b/examples/l3fwd-power/main.c
> index 45b6697c85..a22634a04e 100644
> --- a/examples/l3fwd-power/main.c
> +++ b/examples/l3fwd-power/main.c
> @@ -149,9 +149,6 @@ static struct rte_timer telemetry_timer;
>  /* stats index returned by metrics lib */
>  int telstats_index;
>  
> -/* flag to check if uncore option enabled */
> -int enabled_uncore = -1;
> -
>  struct telstats_name {
>  	char name[RTE_ETH_XSTATS_NAME_SIZE];
>  };

[ ... ]

> @@ -212,6 +203,19 @@ enum freq_scale_hint_t
>  	FREQ_HIGHEST  =       2
>  };
>  
> +enum uncore_choice {
> +	UNCORE_MIN = 0,
> +	UNCORE_MAX = 1,
> +	UNCORE_IDX = 2
> +};
> +
> +/* flag to check if uncore option enabled */
> +int enabled_uncore = -1;
> +struct uncore_cfg {
> +	enum uncore_choice uncore_choice;
> +	uint32_t freq_idx;
> +} g_uncore_cfg;

Does this code initialize g_uncore_cfg.freq_idx anywhere before it is used
in power_uncore_init() when g_uncore_cfg.uncore_choice equals UNCORE_MIN or
UNCORE_MAX?

Does this global variable g_uncore_cfg require static initialization or is
zero-initialization sufficient to prevent use of uninitialized freq_idx?

[ ... ]

> @@ -2264,6 +2190,78 @@ static int check_ptype(uint16_t portid)
>  
>  }
>  
> +static int
> +power_uncore_init(void)
> +{
> +	unsigned int die, pkg, max_pkg, max_die;
> +	int ret;
> +
> +	if (enabled_uncore == -1)
> +		return 0;
> +
> +	ret = rte_power_set_uncore_env(RTE_UNCORE_PM_ENV_AUTO_DETECT);
> +	if (ret < 0) {
> +		RTE_LOG(INFO, L3FWD_POWER, "Failed to set uncore env\n");
> +		return ret;
> +	}
> +
> +	max_pkg = rte_power_uncore_get_num_pkgs();
> +	if (max_pkg == 0)
> +		return -1;
> +
> +	for (pkg = 0; pkg < max_pkg; pkg++) {
> +		max_die = rte_power_uncore_get_num_dies(pkg);
> +		if (max_die == 0)
> +			return -1;
> +		for (die = 0; die < max_die; die++) {
> +			ret = rte_power_uncore_init(pkg, die);
> +			if (ret == -1) {
> +				RTE_LOG(INFO, L3FWD_POWER, "Unable to initialize uncore for pkg %02u die %02u\n"
> +				, pkg, die);
> +				return ret;
> +			}

Does power_uncore_init() leak uncore resources for previously initialized
pkg/die pairs when rte_power_uncore_init() fails on a later iteration?

In the nested loops over pkg and die, when rte_power_uncore_init() returns
an error for some die D in package P, does this code return immediately
without calling rte_power_uncore_exit() on earlier successfully initialized
pkg/die combinations?

[ ... ]

commit ff11f0105c
Author: Huisong Li <lihuisong at huawei.com>
Subject: [PATCH v2 5/5] power: support automatic detection of uncore driver

The commit adds support for automatic probing of multiple uncore drivers
when using RTE_UNCORE_PM_ENV_AUTO_DETECT.

> diff --git a/lib/power/rte_power_uncore.c b/lib/power/rte_power_uncore.c
> index 25bdb113c5..ff11f0105c 100644
> --- a/lib/power/rte_power_uncore.c
> +++ b/lib/power/rte_power_uncore.c
> @@ -2,6 +2,7 @@
>   * Copyright(c) 2010-2014 Intel Corporation
>   * Copyright(c) 2023 AMD Corporation
>   */
> +#include <errno.h>
>  
>  #include <eal_export.h>
>  #include <rte_spinlock.h>
> @@ -46,6 +47,39 @@ rte_power_register_uncore_ops(struct rte_power_uncore_ops *driver_ops)
>  	return 0;
>  }
>  
> +static uint32_t power_uncore_driver_name2env(char *name)
> +{
> +	for (uint32_t i = 0; i < RTE_DIM(uncore_env_str); i++) {
> +		if (!strcmp(name, uncore_env_str[i]))
> +			return i;
> +	}
> +
> +	return UINT32_MAX;
> +}

Does power_uncore_driver_name2env() properly handle a NULL name parameter
passed from power_uncore_probe_driver()->power_uncore_driver_name2env()?

Does this function check whether ops->name is NULL before calling strcmp()?

> +
> +static int power_uncore_probe_driver(void)
> +{
> +	struct rte_power_uncore_ops *ops;
> +	int ret;
> +
> +	global_uncore_ops = NULL;
> +	/* Use package-0 and die-0 to probe uncore driver. */
> +	RTE_TAILQ_FOREACH(ops, &uncore_ops_list, next) {
> +		ret = ops->init(0, 0);
> +		if (ret == 0) {
> +			uint32_t env = power_uncore_driver_name2env(ops->name);
> +			if (env == UINT32_MAX)
> +				continue;
> +			global_uncore_env = env;
> +			global_uncore_ops = ops;
> +			ops->exit(0, 0);
> +			break;
> +		}
> +	}
> +
> +	return global_uncore_ops ? 0 : -ENODEV;
> +}

Does power_uncore_probe_driver() leak resources when ops->init(0, 0)
succeeds but power_uncore_driver_name2env() returns UINT32_MAX?

When env equals UINT32_MAX, does the code continue to the next ops without
calling ops->exit(0, 0) to clean up the successfully initialized driver?

[ ... ]

> @@ -60,12 +94,12 @@ rte_power_set_uncore_env(enum rte_uncore_power_mgmt_env env)
>  		goto out;
>  	}
>  
> -	if (env == RTE_UNCORE_PM_ENV_AUTO_DETECT)
> -		/* Currently only intel_uncore is supported.
> -		 * This will be extended with auto-detection support
> -		 * for multiple uncore implementations.
> -		 */
> -		env = RTE_UNCORE_PM_ENV_INTEL_UNCORE;
> +	if (env == RTE_UNCORE_PM_ENV_AUTO_DETECT) {
> +		ret = power_uncore_probe_driver();
> +		if (ret != 0)
> +			POWER_LOG(ERR, "Probe uncore driver failed, ret = %d", ret);
> +		goto out;
> +	}

Does rte_power_set_uncore_env() change behavior when
power_uncore_probe_driver() returns an error?

When power_uncore_probe_driver() fails, does this code log an error but
still proceed with ret being non-zero, potentially causing incorrect
behavior in callers expecting success?