|SUCCESS| [V2,15/15] 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/163770

_AI Code Review_

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

# Review of DPDK Power Library Patch Series

Author: Huisong Li <lihuisong at huawei.com>

This review covers the patch series introducing lcore validation improvements
across the DPDK power subsystem.

## Patch 01/15: eal: add interface to check if lcore is EAL managed

> diff --git a/lib/eal/common/eal_common_lcore.c b/lib/eal/common/eal_common_lcore.c
> index 39411f9370..dfbd96875a 100644
> --- 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_SYMBOL(rte_lcore_is_eal_managed)
> +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;
> +}

The placement of RTE_EXPORT_SYMBOL appears incorrect. According to DPDK
coding standards, the export should follow the function, not precede it.
Does this code compile correctly with RTE_EXPORT_SYMBOL before the
function definition?

The return statement spans multiple lines without clear operator
precedence. Can the return statement be reformatted to:

    return (cfg->lcore_role[lcore_id] == ROLE_RTE ||
            cfg->lcore_role[lcore_id] == ROLE_SERVICE);

to clarify precedence and improve readability?

> diff --git a/lib/eal/include/rte_lcore.h b/lib/eal/include/rte_lcore.h
> index 10f965b4f0..cbfd50a936 100644
> --- a/lib/eal/include/rte_lcore.h
> +++ b/lib/eal/include/rte_lcore.h
> @@ -195,6 +195,17 @@ rte_cpuset_t rte_lcore_cpuset(unsigned int lcore_id);
>   */
>  int rte_lcore_is_enabled(unsigned int lcore_id);
>  
> +/**
> + * Test if an lcore is ROLE_RTE or ROLE_SERVICE.
> + *
> + * @param lcore_id
> + *   The identifier of the lcore, which MUST be between 0 and
> + *   RTE_MAX_LCORE-1.
> + * @return
> + *   True if the given lcore is ROLE_RTE or ROLE_SERVICE; false otherwise.
> + */
> +int rte_lcore_is_eal_managed(unsigned int lcore_id);

The function documentation states the return value is "True if..." and
"false otherwise," but the actual return type is int. Should the return
value documentation be updated to specify "Non-zero if..." and "zero
otherwise" to accurately reflect the int return type?

The API documentation indicates lcore_id "MUST be between 0 and
RTE_MAX_LCORE-1" but the implementation returns 0 for invalid values
rather than asserting. Does this inconsistency between the documented
precondition and actual behavior need clarification?

## Patch 02/15: power/kvm_vm: validate lcore role in cpufreq API

> diff --git a/drivers/power/kvm_vm/kvm_vm.c b/drivers/power/kvm_vm/kvm_vm.c
> index 5754a441cd..133d1f0882 100644
> --- a/drivers/power/kvm_vm/kvm_vm.c
> +++ b/drivers/power/kvm_vm/kvm_vm.c
> @@ -24,11 +24,11 @@ power_kvm_vm_check_supported(void)
>  int
>  power_kvm_vm_init(unsigned int lcore_id)
>  {
> -	if (lcore_id >= RTE_MAX_LCORE) {
> -		POWER_LOG(ERR, "Core(%u) is out of range 0...%d",
> -				lcore_id, RTE_MAX_LCORE-1);
> +	if (!rte_lcore_is_eal_managed(lcore_id)) {
> +		POWER_LOG(ERR, "lcore id %u is invalid.", lcore_id);
>  		return -1;
>  	}
> +
>  	pkt[lcore_id].command = RTE_POWER_CPU_POWER;

The change removes the informative error message showing the valid range
and replaces it with a generic "invalid" message. Does this reduction in
diagnostic information make debugging more difficult for users?

After validation, does the code access pkt[lcore_id] safely? The lcore_id
is validated but is pkt[] dimensioned to handle all valid lcore IDs
including service cores?

## Patch 07/15: power: add a common macro to verify lcore ID

> diff --git a/lib/power/power_common.h b/lib/power/power_common.h
> index 3f56b1103d..78cebef59b 100644
> --- 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)

The macro uses a multi-statement do-while construct but does not wrap
compound statements within the if block in braces beyond the outer
do-while. According to DPDK coding style for macros with multiple
statements, does this formatting follow the guidelines?

The macro name uses uppercase which is appropriate, but does this follow
the power library's existing naming convention for validation macros? Are
there similar macros that should use a consistent prefix?

## Patch 08/15: power/cpufreq: add the lcore ID verification to framework

> diff --git a/lib/power/rte_power_cpufreq.c b/lib/power/rte_power_cpufreq.c
> index f63e976dc2..54872debfa 100644
> --- a/lib/power/rte_power_cpufreq.c
> +++ b/lib/power/rte_power_cpufreq.c
> @@ -1,7 +1,6 @@
>  /* SPDX-License-Identifier: BSD-3-Clause
>   * Copyright(c) 2010-2014 Intel Corporation
>   */
> -
>  #include <eal_export.h>

Does the removal of this blank line align with the coding style guideline
that requires a blank line between the license header and includes?

> @@ -116,6 +115,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);

Adding validation at the framework level means drivers receive only valid
lcore IDs. However, does this create redundant validation when
global_default_env is set, since global_cpufreq_ops->init() will be called
and the driver's own validation (now removed in subsequent patches) would
have caught this?

## Patch 09/15: power/acpi_cpufreq: remove the verification of lcore ID

[ ... ]

> @@ -372,11 +362,6 @@ power_acpi_cpufreq_freqs(unsigned int lcore_id, uint32_t *freqs, uint32_t num)
>  {
>  	struct acpi_power_info *pi;
>  
> -	if (!rte_lcore_is_eal_managed(lcore_id)) {
> -		POWER_LOG(ERR, "lcore id %u is invalid.", lcore_id);
> -		return 0;
> -	}
> -
>  	if (freqs == NULL) {
>  		POWER_LOG(ERR, "NULL buffer supplied");
>  		return 0;

After removing validation, does power_acpi_cpufreq_freqs() safely access
lcore_power_info[lcore_id]? The framework validates before calling, but
can this function be called through any path that bypasses framework
validation?

[ ... ]

## Patch 14/15: power: allow the service core to config power QoS

> diff --git a/lib/power/rte_power_qos.c b/lib/power/rte_power_qos.c
> index be230d1c50..f6630b08f7 100644
> --- 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);

The original code used rte_lcore_is_enabled() which checks only ROLE_RTE,
while the new macro uses rte_lcore_is_eal_managed() which includes
ROLE_SERVICE. This changes the API contract. Does the commit message
adequately explain this behavioral change and its implications for existing
applications?

## Patch 15/15: power: add lcore ID check for PMD mgmt

> diff --git a/lib/power/rte_power_pmd_mgmt.c b/lib/power/rte_power_pmd_mgmt.c
> index a4d53aac2a..a5fc1c3a94 100644
> --- 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)) {

Does this change from checking "lcore_id >= RTE_MAX_LCORE" to
"!rte_lcore_is_enabled(lcore_id)" intentionally exclude service cores?
The commit message indicates PMD mgmt is for ROLE_RTE cores, but does this
restriction break any existing use cases where service cores might manage
PMD power states?

> @@ -627,7 +628,7 @@ rte_power_ethdev_pmgmt_queue_disable(unsigned int lcore_id,
>  
>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
>  
> -	if (lcore_id >= RTE_MAX_LCORE || queue_id >= RTE_MAX_QUEUES_PER_PORT)
> +	if (!rte_lcore_is_enabled(lcore_id) || queue_id >= RTE_MAX_QUEUES_PER_PORT)
>  		return -EINVAL;

The line now exceeds 79 characters. Does this line need to be wrapped?