|SUCCESS| [v3,10/10] power: add lcore ID check for PMD mgmt

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

git sha: 20260522041110.2023062-2-lihuisong at huawei.com (patch 01/10)
git sha: 20260522041110.2023062-3-lihuisong at huawei.com (patch 02/10)
git sha: 20260522041110.2023062-4-lihuisong at huawei.com (patch 03/10)
git sha: 20260522041110.2023062-5-lihuisong at huawei.com (patch 04/10)
git sha: 20260522041110.2023062-6-lihuisong at huawei.com (patch 05/10)
git sha: 20260522041110.2023062-7-lihuisong at huawei.com (patch 06/10)
git sha: 20260522041110.2023062-8-lihuisong at huawei.com (patch 07/10)
git sha: 20260522041110.2023062-9-lihuisong at huawei.com (patch 08/10)
git sha: 20260522041110.2023062-10-lihuisong at huawei.com (patch 09/10)
git sha: 20260522041110.2023062-11-lihuisong at huawei.com (patch 10/10)

Author: Huisong Li <lihuisong at huawei.com>

This patch series adds a new helper function to check if an lcore is
EAL-managed (ROLE_RTE or ROLE_SERVICE), then uses it to standardize
lcore validation across power management libraries.

> --- a/lib/eal/common/eal_common_lcore.c
> +++ b/lib/eal/common/eal_common_lcore.c
> @@ -102,6 +102,17 @@ int rte_lcore_is_enabled(unsigned int lcore_id)
>  	return cfg->lcore_role[lcore_id] == ROLE_RTE;
>  }
>  
> +RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_lcore_is_eal_managed, 26.07)
> +int rte_lcore_is_eal_managed(unsigned int lcore_id)
> +{
> +	struct rte_config *cfg = rte_eal_get_configuration();
> +
> +	if (lcore_id >= RTE_MAX_LCORE)
> +		return 0;
> +	return cfg->lcore_role[lcore_id] == ROLE_RTE ||
> +		cfg->lcore_role[lcore_id] == ROLE_SERVICE;
> +}

In the new function rte_lcore_is_eal_managed(), can both conditions in
the return statement be tested using a single array access?  Currently,
cfg->lcore_role[lcore_id] is read twice.  Would
"(cfg->lcore_role[lcore_id] & (ROLE_RTE | ROLE_SERVICE))" be equivalent
and avoid redundant array lookups, or does the enum definition prevent
this?

[ ... ]

> --- a/lib/power/power_common.h
> +++ b/lib/power/power_common.h
> @@ -23,6 +23,13 @@ extern int rte_power_logtype;
>  #define POWER_DEBUG_LOG(...)
>  #endif
>  
> +#define RTE_POWER_VALID_LCOREID_OR_ERR_RET(lcore_id, retval) do { \
> +	if (!rte_lcore_is_eal_managed(lcore_id)) { \
> +		POWER_LOG(ERR, "lcore id %u is invalid", lcore_id); \
> +		return retval; \
> +	} \
> +} while (0)

In RTE_POWER_VALID_LCOREID_OR_ERR_RET, does wrapping the conditional and
return inside a do-while(0) block follow DPDK conventions?  The pattern
is common, but can the "do {} while (0)" be omitted if the macro body is
only used where a statement is expected?

[ ... ]

> --- a/lib/power/rte_power_cpufreq.c
> +++ b/lib/power/rte_power_cpufreq.c
> @@ -116,6 +116,7 @@ rte_power_init(unsigned int lcore_id)
>  	struct rte_power_cpufreq_ops *ops;
>  	uint8_t env;
>  
> +	RTE_POWER_VALID_LCOREID_OR_ERR_RET(lcore_id, -1);
>  	if (global_default_env != PM_ENV_NOT_SET)
>  		return global_cpufreq_ops->init(lcore_id);

After validating lcore_id with RTE_POWER_VALID_LCOREID_OR_ERR_RET in
rte_power_init(), does the subsequent call chain
global_cpufreq_ops->init(lcore_id) assume lcore_id is validated, or do
driver-specific init functions also perform redundant checks?  Some
driver init functions in this series remove lcore checks.  Does this
create an inconsistency if global_cpufreq_ops is NULL or points to an
unexpected ops table?

[ ... ]

> @@ -161,6 +163,7 @@ uint32_t
>  rte_power_freqs(unsigned int lcore_id, uint32_t *freqs, uint32_t n)
>  {
>  	RTE_ASSERT(global_cpufreq_ops != NULL);
> +	RTE_POWER_VALID_LCOREID_OR_ERR_RET(lcore_id, 0);
>  	return global_cpufreq_ops->get_avail_freqs(lcore_id, freqs, n);
>  }

In rte_power_freqs(), does adding the lcore_id validation after
RTE_ASSERT(global_cpufreq_ops != NULL) create a situation where the
assert fires before the lcore validation in debug builds?  If so, does
this produce less helpful diagnostics?

[ ... ]

> --- a/drivers/power/acpi/acpi_cpufreq.c
> +++ b/drivers/power/acpi/acpi_cpufreq.c
> @@ -242,12 +242,6 @@ power_acpi_cpufreq_init(unsigned int lcore_id)
>  		return -1;
>  	}
>  
> -	if (lcore_id >= RTE_MAX_LCORE) {
> -		POWER_LOG(ERR, "Lcore id %u can not exceeds %u",
> -				lcore_id, RTE_MAX_LCORE - 1U);
> -		return -1;
> -	}
> -
>  	pi = &lcore_power_info[lcore_id];

In power_acpi_cpufreq_init(), after removing the lcore_id >= 
RTE_MAX_LCORE check, does the line "pi = &lcore_power_info[lcore_id]"
rely entirely on the framework-level validation via
RTE_POWER_VALID_LCOREID_OR_ERR_RET?  If the framework check is somehow
bypassed or not present, does this indexing overflow lcore_power_info?

[ ... ]

> @@ -373,11 +362,6 @@ power_acpi_cpufreq_freqs(unsigned int lcore_id, uint32_t *freqs, uint32_t num)
>  {
>  	struct acpi_power_info *pi;
>  
> -	if (lcore_id >= RTE_MAX_LCORE) {
> -		POWER_LOG(ERR, "Invalid lcore ID");
> -		return 0;
> -	}
> -
>  	if (freqs == NULL) {
>  		POWER_LOG(ERR, "NULL buffer supplied");
>  		return 0;

In power_acpi_cpufreq_freqs(), does removing the lcore_id check and
relying on the framework mean that if freqs is NULL, the function logs
an error and returns 0, but if lcore_id was invalid, the framework
already returned -1?  Does this create inconsistent return values for
different error paths?

[ ... ]

> --- a/drivers/power/kvm_vm/guest_channel.c
> +++ b/drivers/power/kvm_vm/guest_channel.c
> @@ -61,11 +61,6 @@ guest_channel_host_connect(const char *path, unsigned int lcore_id)
>  	char fd_path[PATH_MAX];
>  	int fd = -1;
>  
> -	if (lcore_id >= RTE_MAX_LCORE) {
> -		GUEST_CHANNEL_LOG(ERR, "Channel(%u) is out of range 0...%d",
> -				lcore_id, RTE_MAX_LCORE-1);
> -		return -1;
> -	}
>  	/* check if path is already open */
>  	if (global_fds[lcore_id] != -1) {

In guest_channel_host_connect(), after removing the lcore_id >= 
RTE_MAX_LCORE check, does accessing global_fds[lcore_id] assume that
all callers have already validated lcore_id?  If guest_channel module
functions are called directly instead of through the power framework,
does this create a bounds-checking gap?

[ ... ]

> @@ -127,12 +122,6 @@ guest_channel_send_msg(struct rte_power_channel_packet *pkt,
>  	int ret, buffer_len = sizeof(*pkt);
>  	void *buffer = pkt;
>  
> -	if (lcore_id >= RTE_MAX_LCORE) {
> -		GUEST_CHANNEL_LOG(ERR, "Channel(%u) is out of range 0...%d",
> -				lcore_id, RTE_MAX_LCORE-1);
> -		return -1;
> -	}
> -
>  	if (global_fds[lcore_id] < 0) {
>  		GUEST_CHANNEL_LOG(ERR, "Channel is not connected");
>  		return -1;

In guest_channel_send_msg(), does the check "if (global_fds[lcore_id] <
0)" potentially overflow global_fds if lcore_id is invalid and no prior
validation occurred?

[ ... ]

> --- a/lib/power/rte_power_qos.c
> +++ b/lib/power/rte_power_qos.c
> @@ -27,10 +27,7 @@ rte_power_qos_set_cpu_resume_latency(uint16_t lcore_id, int latency)
>  	FILE *f;
>  	int ret;
>  
> -	if (!rte_lcore_is_enabled(lcore_id)) {
> -		POWER_LOG(ERR, "lcore id %u is not enabled", lcore_id);
> -		return -EINVAL;
> -	}
> +	RTE_POWER_VALID_LCOREID_OR_ERR_RET(lcore_id, -EINVAL);

In rte_power_qos_set_cpu_resume_latency(), does changing from
rte_lcore_is_enabled() to RTE_POWER_VALID_LCOREID_OR_ERR_RET (which uses
rte_lcore_is_eal_managed) alter the semantics such that service cores
are now accepted?  The commit message says this is intentional, but does
the new macro return -EINVAL or -1 on error, and does this match what
the function previously returned?

[ ... ]

> --- a/lib/power/rte_power_pmd_mgmt.c
> +++ b/lib/power/rte_power_pmd_mgmt.c
> @@ -511,7 +511,8 @@ rte_power_ethdev_pmgmt_queue_enable(unsigned int lcore_id, uint16_t port_id,
>  
>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
>  
> -	if (queue_id >= RTE_MAX_QUEUES_PER_PORT || lcore_id >= RTE_MAX_LCORE) {
> +	if (queue_id >= RTE_MAX_QUEUES_PER_PORT ||
> +	    !rte_lcore_is_enabled(lcore_id)) {

In rte_power_ethdev_pmgmt_queue_enable(), does changing the lcore_id
check from "lcore_id >= RTE_MAX_LCORE" to "!rte_lcore_is_enabled(lcore_id)"
mean that service cores are now rejected?  The commit message says
pmd_mgmt is used with data plane threads (ROLE_RTE only), but does this
create a behavior change if applications previously passed service core
IDs that happened to be < RTE_MAX_LCORE?

[ ... ]

> @@ -729,8 +730,8 @@ RTE_EXPORT_SYMBOL(rte_power_pmd_mgmt_set_scaling_freq_min)
>  int
>  rte_power_pmd_mgmt_set_scaling_freq_min(unsigned int lcore, unsigned int min)
>  {
> -	if (lcore >= RTE_MAX_LCORE) {
> -		POWER_LOG(ERR, "Invalid lcore ID: %u", lcore);
> +	if (!rte_lcore_is_enabled(lcore)) {
> +		POWER_LOG(ERR, "lcore id %u is not enabled", lcore);

In rte_power_pmd_mgmt_set_scaling_freq_min(), does the error message
change from "Invalid lcore ID" to "lcore id %u is not enabled" accurately
reflect the new check?  If an lcore ID is >= RTE_MAX_LCORE, is it more
accurate to say "not enabled" or "out of range"?