Re: [PATCH v2 2/2] cpufreq: acpi-cpufreq: fix P-state index mismatch in get_cur_freq_on_cpu()

[Zhongqiu Han]

On 8/13/2026 5:01 PM, lirongqing wrote:
> From: Li RongQing <lirongqing@xxxxxxxxx>
>
> get_cur_freq_on_cpu() reads the cached frequency as
> policy->freq_table[to_perf_data(data)->state], mixing two different index
> spaces: perf->state indexes perf->states[], while policy->freq_table[] is
> built with duplicate frequencies removed and stores the original P-state
> index in freq_table[].driver_data.
>
> Once any _PSS entry has been skipped the two arrays no longer line up, so
> the cached frequency used to detect a "BIOS changed frequency behind our
> back" event could be taken from the wrong table slot.

A further consequence of this index-space mismatch should be that,
depending on which entry is picked, the check either fails on every call
for P-states whose freq_table index differs from their _PSS index,
causing data->resume to force a redundant control-register rewrite on
every ->target(), or silently passes when the wrong slot happens to hold
the frequency the firmware actually moved the CPU to, causing
acpi_cpufreq_target() to short-circuit and leave the CPU running at a
frequency the core does not expect until a different P-state is
requested.

> Look up the freq_table entry whose driver_data matches perf->state instead
> of indexing freq_table[] with perf->state directly.
>
> Fixes: 8cee1eed8e78 ("cpufreq: ACPI: Remove freq_table from acpi_cpufreq_data")

The real Fixes tag should be e56a727b023d ("[CPUFREQ] Make acpi-cpufreq
more robust against BIOS freq changes behind our back.")

> Reported-by: Zhongqiu Han <zhongqiu.han@xxxxxxxxxxxxxxxx>
> Signed-off-by: Li RongQing <lirongqing@xxxxxxxxx>
> ---
>   drivers/cpufreq/acpi-cpufreq.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
> index 1abe9ab..61ede49c 100644
> --- a/drivers/cpufreq/acpi-cpufreq.c
> +++ b/drivers/cpufreq/acpi-cpufreq.c
> @@ -353,6 +353,7 @@ static u32 get_cur_val(const struct cpumask *mask, struct acpi_cpufreq_data *dat
>   
>   static unsigned int get_cur_freq_on_cpu(unsigned int cpu)
>   {
> +	struct cpufreq_frequency_table *pos;
>   	struct acpi_cpufreq_data *data;
>   	struct cpufreq_policy *policy;
>   	unsigned int freq;
> @@ -368,7 +369,13 @@ static unsigned int get_cur_freq_on_cpu(unsigned int cpu)
>   	if (unlikely(!data || !policy->freq_table))
>   		return 0;
>   
> -	cached_freq = policy->freq_table[to_perf_data(data)->state].frequency;

How about:
	struct acpi_processor_performance *perf = to_perf_data(data);
	...
	cached_freq = perf->states[perf->state].core_frequency * 1000;

get_cur_freq_on_cpu() is only called on ACPI_ADR_SPACE_FIXED_HARDWARE
platforms, and on such platforms perf->state is only assigned in the
following functions:

(1) acpi_cpufreq_target(): perf->state is then the index of the P-state
    last written to the hardware.
(2) acpi_cpufreq_fast_switch(): same as above (1).

(3) acpi_cpufreq_cpu_init(): this sets the initial value perf->state =
    0. In cpufreq_online(), after .init() has been called, .get() - i.e.
    get_cur_freq_on_cpu() - is called once.  The freq read at that point
    may be a leftover value from the hardware, but whether or not
    "if (freq != cached_freq)" holds, the only consequence is
    data->resume = 1, and data->resume has already been initialised to 1
    in .init() anyway.


> +	cached_freq = 0;
> +	cpufreq_for_each_entry(pos, policy->freq_table)
> +		if (pos->driver_data == to_perf_data(data)->state) {
> +			cached_freq = pos->frequency;
> +			break;
> +		}
> +
>   	freq = extract_freq(policy, get_cur_val(cpumask_of(cpu), data));
>   	if (freq != cached_freq) {
>   		/*


--
Thx and BRs,
Zhongqiu Han