|SUCCESS| power/amd_pstate: fix frequency matching for continuous scaling

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-03-30

git sha: 20260328193419.106100-1-stephen at networkplumber.org
Author: Stephen Hemminger <stephen at networkplumber.org>
Subject: [PATCH] power/amd_pstate: fix frequency matching for continuous scaling

This patch fixes frequency matching in the AMD pstate driver by
replacing exact match lookups with nearest-frequency search, and
corrects several issues in power_init_for_setting_freq() including
improper error handling for strtoul() and missing errno checks.

> diff --git a/drivers/power/amd_pstate/amd_pstate_cpufreq.c b/drivers/power/amd_pstate/amd_pstate_cpufreq.c
> index 95495bff7d..14625ffd73 100644
> --- a/drivers/power/amd_pstate/amd_pstate_cpufreq.c
> +++ b/drivers/power/amd_pstate/amd_pstate_cpufreq.c
> @@ -5,6 +5,7 @@
>   */
>  
>  #include <stdlib.h>
> +#include <errno.h>
>  
>  #include <rte_memcpy.h>
>  #include <rte_stdatomic.h>

[ ... ]

> +static inline unsigned long
> +abs_diff(unsigned long a, unsigned long b)
> +{
> +	return (a > b) ? a - b : b - a;
> +}

Does this helper function need inline linkage, or could it be static
without the inline keyword? The coding style guide prefers to let the
compiler make inlining decisions unless there is a compelling reason.

[ ... ]

> @@ -308,7 +315,6 @@ power_init_for_setting_freq(struct amd_pstate_power_info *pi)
>  				POWER_SYSFILE_SETSPEED);
>  		goto err;
>  	}
> -
>  	ret = read_core_sysfs_s(f, buf, sizeof(buf));
>  	if (ret < 0) {
>  		POWER_LOG(ERR, "Failed to read %s",
> @@ -316,25 +322,45 @@ power_init_for_setting_freq(struct amd_pstate_power_info *pi)
>  		goto err;
>  	}
>  
> -	freq = strtoul(buf, NULL, POWER_CONVERT_TO_DECIMAL);
> +	errno = 0;
> +	freq = strtoul(buf, &endptr, POWER_CONVERT_TO_DECIMAL);
> +	if (errno != 0 || endptr == buf || freq == 0) {
> +		POWER_LOG(ERR, "Failed to parse frequency '%s' for lcore %u",
> +				buf, pi->lcore_id);
> +		goto err;
> +	}

Can the check "(freq == 0)" cause a false positive rejection of a
valid zero frequency value? While unlikely in practice, the POSIX
standard allows strtoul() to return 0 as a valid conversion result
when errno is not set. Does the AMD pstate driver ever report zero
frequency values that should be considered valid?

Also, does the code handle the case where buf contains leading
whitespace or a negative sign? strtoul() will skip leading whitespace
and may accept a minus sign, converting what appears to be a negative
value. Should the validation explicitly check for this?

> +
>  	/* convert the frequency to nearest 1000 value
>  	 * Ex: if freq=1396789 then freq_conv=1397000
>  	 * Ex: if freq=800030 then freq_conv=800000
>  	 */
> -	unsigned int freq_conv = 0;
> -	freq_conv = (freq + FREQ_ROUNDING_DELTA)
> -				/ ROUND_FREQ_TO_N_1000;
> +	freq_conv = (freq + FREQ_ROUNDING_DELTA) / ROUND_FREQ_TO_N_1000;
>  	freq_conv = freq_conv * ROUND_FREQ_TO_N_1000;

The comment block here has misaligned asterisks that do not follow the
DPDK coding style for multi-line comments. Can the asterisks be aligned
vertically with the opening comment delimiter?

> +
> +	/* Find the nearest frequency in the table.
> +	 * With amd-pstate the CPU runs at continuously variable
> +	 * frequencies so the current frequency will not exactly
> +	 * match one of the synthesized frequency buckets.
> +	 */
> +	best_idx = 0;
> +	best_diff = abs_diff(freq_conv, pi->freqs[0]);
> +
> +	for (i = 1; i < pi->nb_freqs; i++) {
> +		diff = abs_diff(freq_conv, pi->freqs[i]);
> +		if (diff < best_diff) {
> +			best_diff = diff;
> +			best_idx = i;
> +		}
> +	}

Does this loop need bounds checking on pi->nb_freqs? Can nb_freqs ever
be zero, causing the initial reference to pi->freqs[0] to access
invalid memory? The power_get_available_freqs() function can succeed
while leaving nb_freqs as zero if no frequencies are found.

> +
> +	POWER_DEBUG_LOG("Freq %lu rounded to %lu matched bucket [%u] = %u "
> +			"for lcore %u", freq, freq_conv, best_idx,
> +			pi->freqs[best_idx], pi->lcore_id);

Does this debug log statement have a line break in the string literal?
The coding style guide discourages breaking string literals across
multiple lines unless necessary for readability. Can this be kept as a
single line, or if wrapping is required, should the format string be
assembled differently?

> +
> +	pi->curr_idx = best_idx;
> +	pi->f = f;
> +	return 0;
> +
>  err:
>  	if (f != NULL)
>  		fclose(f);

The error path checks f against NULL before calling fclose(), but is
this check necessary? In the success path at line "pi->f = f", the code
assumes f is valid. Does the control flow guarantee that f is always
either NULL or a valid FILE pointer when reaching the err label?