|SUCCESS| [3/3] power: use common decimal macro definition

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

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

git sha: 20260509084503.2917038-2-lihuisong at huawei.com (patch 1/3)
Author: Huisong Li <lihuisong at huawei.com>
Subject: [PATCH 1/3] power: move power state structure to power cpufreq header

This patch moves the enum power_state definition from individual cpufreq
drivers to a common header file for better maintainability.

> diff --git a/lib/power/power_cpufreq.h b/lib/power/power_cpufreq.h
> index fb0b7feb82..41bfaed7bb 100644
> --- a/lib/power/power_cpufreq.h
> +++ b/lib/power/power_cpufreq.h
> @@ -18,6 +18,16 @@
>  
>  #define RTE_POWER_DRIVER_NAMESZ 24
>  
> +/**
> + * Power state of cpufreq driver
> + */
> +enum power_state {
> +	POWER_IDLE = 0,
> +	POWER_ONGOING,
> +	POWER_USED,
> +	POWER_UNKNOWN
> +};

The comment "Power state of cpufreq driver" is quite brief. Can this
documentation be expanded to explain what each state represents and when
transitions between states occur? This would help users understand the
state machine behavior.

Also, does this enum need to be exposed in a public header? If these
states are only used internally by the cpufreq drivers, should this be
in an internal header instead?

[ ... ]

---

git sha: 20260509084503.2917038-3-lihuisong at huawei.com (patch 2/3)
Author: Huisong Li <lihuisong at huawei.com>
Subject: [PATCH 2/3] power: unify decimal format macro for strtoul

This patch consolidates the POWER_CONVERT_TO_DECIMAL macro definition
into a common header.

> diff --git a/lib/power/power_common.h b/lib/power/power_common.h
> index 3f56b1103d..e2d5b68a17 100644
> --- a/lib/power/power_common.h
> +++ b/lib/power/power_common.h
> @@ -23,6 +23,8 @@ extern int rte_power_logtype;
>  #define POWER_DEBUG_LOG(...)
>  #endif
>  
> +#define POWER_CONVERT_TO_DECIMAL 10
> +
>  /* check if scaling driver matches one we want */
>  __rte_internal
>  int cpufreq_check_scaling_driver(const char *driver);

The macro POWER_CONVERT_TO_DECIMAL appears to represent the base
parameter for strtoul(). The name is somewhat misleading - it suggests
a conversion operation rather than a constant base value. Would
POWER_DECIMAL_BASE or similar be clearer?

Additionally, this header (power_common.h) appears to be internal. Does
the naming convention need to be more explicit about internal vs public
APIs? The lack of an RTE_ prefix suggests it's internal, but the file
organization should be clear.

[ ... ]

---

git sha: 20260509084503.2917038-4-lihuisong at huawei.com (patch 3/3)
Author: Huisong Li <lihuisong at huawei.com>
Subject: [PATCH 3/3] power: use common decimal macro definition

This patch replaces a magic number with the unified decimal macro.

> diff --git a/lib/power/rte_power_qos.c b/lib/power/rte_power_qos.c
> index be230d1c50..f991230532 100644
> --- a/lib/power/rte_power_qos.c
> +++ b/lib/power/rte_power_qos.c
> @@ -115,7 +115,7 @@ rte_power_qos_get_cpu_resume_latency(uint16_t lcore_id)
>  	if (strcmp(buf, "n/a") == 0)
>  		latency = RTE_POWER_QOS_STRICT_LATENCY_VALUE;
>  	else {
> -		latency = strtoul(buf, NULL, 10);
> +		latency = strtoul(buf, NULL, POWER_CONVERT_TO_DECIMAL);
>  		latency = latency == 0 ? RTE_POWER_QOS_RESUME_LATENCY_NO_CONSTRAINT : latency;
>  	}

Does this code properly handle strtoul() error cases? The second
parameter (endptr) is NULL, which means parsing errors won't be
detected. Can malformed input in buf cause incorrect latency values to
be silently accepted?

The comparison "latency == 0" after strtoul() is testing the result.
According to DPDK coding style, should this be "latency == 0" or
"!latency"? The current form appears acceptable for numeric comparisons
against zero.

However, the compound ternary assignment on a single line makes the code
harder to read. Would splitting this into separate statements improve
clarity?