|SUCCESS| [v1, 7/7] examples/l3fwd-power: switch to new init/deinit uncore API

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

I'll review this DPDK patch series focusing on correctness, coding style, and guideline compliance.

commit sha: 20260512023513.460169-2-lihuisong at huawei.com (Patch 1/7)
Author: Huisong Li <lihuisong at huawei.com>
examples/l3fwd-power: fix uncore deinit for non-legacy

This patch fixes uncore resources not being deinitialized in non-legacy
modes by moving uncore deinitialization out of the legacy-only code
path.

> 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;
> +			}

Does this code leak resources if rte_power_init() fails on a later
lcore_id? The successfully initialized lcores would remain initialized.
Should there be cleanup of previously initialized lcores before returning?

[ ... ]

> @@ -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 this code properly cleanup remaining lcores if rte_power_exit() fails
partway through? When an early return happens, should the function continue
attempting to cleanup subsequent resources?

---

commit sha: 20260512023513.460169-4-lihuisong at huawei.com (Patch 3/7)
Author: Huisong Li <lihuisong at huawei.com>
examples/l3fwd-power: fix uncore help and log info

This patch corrects help text and error messages related to uncore
frequency control.

> diff --git a/examples/l3fwd-power/main.c b/examples/l3fwd-power/main.c
> index ba2be5bf32..45b6697c85 100644
> --- a/examples/l3fwd-power/main.c
> +++ b/examples/l3fwd-power/main.c
> @@ -1498,7 +1498,7 @@ print_usage(const char *prgname)
>  		"  -P: enable promiscuous mode\n"
>  		"  -u: set min/max frequency for uncore to minimum value\n"
>  		"  -U: set min/max frequency for uncore to maximum value\n"
> -		"  -i (frequency index): set min/max frequency for uncore to specified frequency index\n"
> +		"  -i (frequency index): set target frequency for uncore by specified frequency index\n"

The help text changes appear reasonable.

[ ... ]

---

commit sha: 20260512023513.460169-5-lihuisong at huawei.com (Patch 4/7)
Author: Huisong Li <lihuisong at huawei.com>
examples/l3fwd-power: relocate uncore initialization

This patch moves uncore initialization from the parse function to
init_power_library().

> @@ -1793,25 +1723,21 @@ parse_args(int argc, char **argv)
>  			promiscuous_on = 1;
>  			break;
>  		case 'u':
> -			enabled_uncore = parse_uncore_options(UNCORE_MIN, NULL);
> -			if (enabled_uncore < 0) {
> -				print_usage(prgname);
> -				return -1;
> -			}
> +			enabled_uncore = 0;
> +			g_uncore_cfg.uncore_choice = UNCORE_MIN;
>  			break;
>  		case 'U':
> -			enabled_uncore = parse_uncore_options(UNCORE_MAX, NULL);
> -			if (enabled_uncore < 0) {
> -				print_usage(prgname);
> -				return -1;
> -			}
> +			enabled_uncore = 0;
> +			g_uncore_cfg.uncore_choice = UNCORE_MAX;
>  			break;
>  		case 'i':
> -			enabled_uncore = parse_uncore_options(UNCORE_IDX, optarg);
> -			if (enabled_uncore < 0) {
> +			enabled_uncore = 0;
> +			if (parse_uint(optarg, UINT32_MAX, &g_uncore_cfg.freq_idx) != 0) {
> +				RTE_LOG(INFO, L3FWD_POWER, "Index given is not a valid number.");

This RTE_LOG message should end with a newline character.

[ ... ]

> @@ -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;

Does this implicit comparison comply with coding style? Should it be
"if (enabled_uncore != -1)"?

> +
> +	ret = rte_power_set_uncore_env(RTE_UNCORE_PM_ENV_AUTO_DETECT);
> +	if (ret < 0) {

Does this implicit comparison comply with coding style? Should it be
"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)

This comparison with zero appears appropriate here.

> +		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) {

Does this code properly cleanup previously initialized pkg/die pairs when
rte_power_uncore_init() fails? Should there be an error path that calls
rte_power_uncore_exit() for the successfully initialized pairs?

> +				RTE_LOG(INFO, L3FWD_POWER, "Unable to initialize uncore for pkg %02u die %02u\n"
> +				, pkg, die);
> +				return ret;
> +			}
> +			if (g_uncore_cfg.uncore_choice == UNCORE_MIN) {
> +				ret = rte_power_uncore_freq_min(pkg, die);
> +				if (ret == -1) {
> +					RTE_LOG(INFO, L3FWD_POWER,
> +					"Unable to set the uncore frequency to minimum value for pkg %02u die %02u\n"
> +					, pkg, die);
> +					return ret;

Does this return path cleanup the uncore initialization that succeeded?
Should this path call rte_power_uncore_exit()?

---

commit sha: 20260512023513.460169-6-lihuisong at huawei.com (Patch 5/7)
Author: Huisong Li <lihuisong at huawei.com>
power: support automatic detection of uncore driver

This patch adds automatic probing for uncore drivers.

> diff --git a/lib/power/rte_power_uncore.c b/lib/power/rte_power_uncore.c
> index e26919acce..a5d080e09e 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>

Does this new include placement comply with the coding style guide? System
headers should come before DPDK headers.

> @@ -47,6 +48,37 @@ rte_power_register_uncore_ops(struct rte_power_uncore_ops *driver_ops)
>  	return 0;
>  }
>  
> +static uint32_t rte_power_uncore_driver_name2env(char *name)

Does this parameter need "const char *name" to match standard practice for
read-only string parameters?

> +{
> +	for (uint32_t i = 0; i < RTE_DIM(uncore_env_str); i++) {
> +		if (!strcmp(name, uncore_env_str[i]))

Does this implicit boolean comparison comply with coding style? Should it be
"if (strcmp(name, uncore_env_str[i]) == 0)"?

> +			return i;
> +	}
> +
> +	return UINT32_MAX;
> +}
> +
> +static int rte_power_probe_uncore_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) {

Does this implicit comparison comply with coding style? Should it be
"if (ret == 0)"?

> +			global_uncore_env =
> +				rte_power_uncore_driver_name2env(ops->name);
> +			global_uncore_ops = ops;
> +			ops->exit(0, 0);
> +			break;
> +		}
> +	}
> +
> +	return global_uncore_ops ? 0 : -ENODEV;

Does this ternary with implicit comparison comply with coding style? Should
it be "return (global_uncore_ops != NULL) ? 0 : -ENODEV"?

---

commit sha: 20260512023513.460169-7-lihuisong at huawei.com (Patch 6/7)
Author: Huisong Li <lihuisong at huawei.com>
power: add global uncore init and deinit interface

This patch adds new APIs for global uncore initialization and
deinitialization.

> diff --git a/lib/power/rte_power_uncore.c b/lib/power/rte_power_uncore.c
> index a5d080e09e..0611a60d33 100644
> --- a/lib/power/rte_power_uncore.c
> +++ b/lib/power/rte_power_uncore.c
> @@ -135,6 +141,88 @@ rte_power_get_uncore_env(void)
>  	return global_uncore_env;
>  }
>  
> +RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_power_uncore_driver_init, 26.07)
> +int
> +rte_power_uncore_driver_init(void)
> +{
> +	int die, pkg, max_pkg, max_die;
> +	int ret;
> +
> +	rte_spinlock_lock(&global_env_cfg_lock);
> +	ret = rte_power_probe_uncore_driver();
> +	if (ret) {

Does this implicit comparison comply with coding style? Should it be
"if (ret != 0)"?

> +		POWER_LOG(ERR, "Probe uncore driver failed, ret = %d.", ret);
> +		goto out;
> +	}
> +
> +	max_pkg = rte_power_uncore_get_num_pkgs();
> +	if (max_pkg == 0) {
> +		ret = -EINVAL;
> +		goto remove_uncore_drv;
> +	}
> +
> +	for (pkg = 0; pkg < max_pkg; pkg++) {
> +		max_die = rte_power_uncore_get_num_dies(pkg);
> +		if (max_die == 0) {
> +			ret = -EINVAL;
> +			goto remove_uncore_drv;
> +		}
> +
> +		for (die = 0; die < max_die; die++) {
> +			ret = rte_power_uncore_init(pkg, die);
> +			if (ret) {

Does this implicit comparison comply with coding style? Should it