答复: [外部邮件] Re: [PATCH v2 2/2] cpufreq: acpi-cpufreq: fix P-state index mismatch in get_cur_freq_on_cpu()

[Li,Rongqing]

> -----邮件原件-----
> 发件人: Zhongqiu Han <zhongqiu.han@xxxxxxxxxxxxxxxx>
> 发送时间: 2026年8月18日 22:02
> 收件人: Li,Rongqing <lirongqing@xxxxxxxxx>; Rafael J . Wysocki
> <rafael@xxxxxxxxxx>; Viresh Kumar <viresh.kumar@xxxxxxxxxx>;
> linux-pm@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> 抄送: zhongqiu.han@xxxxxxxxxxxxxxxx
> 主题: [外部邮件] Re: [PATCH v2 2/2] cpufreq: acpi-cpufreq: fix P-state index
> mismatch in get_cur_freq_on_cpu()
> 
> 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
> > policy->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;
> 

Thanks, that's cleaner and I've adopted it in v3.

Using perf->state to index perf->states[] (its native index space) is exactly right: it avoids the freq_table[] index-space mismatch at the root, and perf->states[perf->state].core_frequency * 1000 is the same value that gets stored into freq_table[] at init time, so the freq != cached_freq resync check is unchanged.

It also removes a corner case in my earlier freq_table-walk version: if perf->state points at a P-state that was deduplicated out of freq_table[], the walk finds no match and leaves cached_freq at 0, misfiring the check. Indexing perf->states[] directly avoids that.

Will send as part of v3.

[Li,Rongqing] 


> 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