Re: [PATCH v2] arm64: topology: Fix false warning in counters_read_on_cpu() for same-CPU reads

[Sumit Gupta <sumitg@nvidia.com>]

Hi Marc,


On 24/02/26 23:25, Marc Zyngier wrote:
> External email: Use caution opening links or attachments
>
>
> On Tue, 24 Feb 2026 09:27:14 +0000,
> Sumit Gupta <sumitg@nvidia.com> wrote:
>> The counters_read_on_cpu() function warns when called with IRQs
>> disabled to prevent deadlock in smp_call_function_single(). However,
>> this warning is spurious when reading counters on the current CPU,
>> since no IPI is needed for same CPU reads.
>>
>> Commit 12eb8f4fff24 ("cpufreq: CPPC: Update FIE arch_freq_scale in
>> ticks for non-PCC regs") changed the CPPC Frequency Invariance Engine
>> to read AMU counters directly from the scheduler tick for non-PCC
>> register spaces (like FFH), instead of deferring to a kthread. This
>> means counters_read_on_cpu() is now called with IRQs disabled from
>> the tick handler, triggering the warning:
>>
>> | WARNING: arch/arm64/kernel/topology.c:410 at counters_read_on_cpu
>> | ...
>> | Call trace:
>> |  counters_read_on_cpu+0x88/0xa8 (P)
>> |  cpc_read_ffh+0xdc/0x148
>> |  cpc_read+0x260/0x518
>> |  cppc_get_perf_ctrs+0xf0/0x398
>> |  __cppc_scale_freq_tick+0x4c/0x148 [cppc_cpufreq]
>> |  cppc_scale_freq_tick+0x44/0x88 [cppc_cpufreq]
>> |  topology_scale_freq_tick+0x34/0x58
>> |  sched_tick+0x58/0x300
>> |  update_process_times+0xcc/0x120
>> |  tick_nohz_handler+0xa8/0x260
>> |  __hrtimer_run_queues+0x154/0x360
>> |  hrtimer_interrupt+0xf4/0x2b0
>> |  arch_timer_handler_phys+0x4c/0x78
>> |  ....
>> |  CPPC Cpufreq:__cppc_scale_freq_tick: failed to read perf counters
>> |  ....
>>
>> Fix this by calling the counter read function directly for same CPU
>> case, bypassing smp_call_function_single(). Use get_cpu() to disable
>> preemption, as the counter read functions call this_cpu_has_cap()
>> which requires a non-preemptible context.
>>
>> Fixes: 997c021abc6e ("cpufreq: CPPC: Update FIE arch_freq_scale in ticks for non-PCC regs")
>> Signed-off-by: Sumit Gupta <sumitg@nvidia.com>
>> Reviewed-by: Jie Zhan <zhanjie9@hisilicon.com>
>> ---
>> v1 -> v2:
>>   - Rebased on v7.0-rc1
>>   - Updated Fixes tag to match upstream commit SHA
>> ---
>>   arch/arm64/kernel/topology.c | 21 +++++++++++++++++++--
>>   1 file changed, 19 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
>> index 3fe1faab0362..c3e883e99aa0 100644
>> --- a/arch/arm64/kernel/topology.c
>> +++ b/arch/arm64/kernel/topology.c
>> @@ -400,12 +400,29 @@ static inline
>>   int counters_read_on_cpu(int cpu, smp_call_func_t func, u64 *val)
>>   {
>>        /*
>> -      * Abort call on counterless CPU or when interrupts are
>> -      * disabled - can lead to deadlock in smp sync call.
>> +      * Abort call on counterless CPU.
>>         */
>>        if (!cpu_has_amu_feat(cpu))
>>                return -EOPNOTSUPP;
>>
>> +     /*
>> +      * For same-CPU reads, call the function directly since no IPI
>> +      * is needed and this is safe even with IRQs disabled.
>> +      * Use get_cpu() to disable preemption as the counter read
>> +      * functions call this_cpu_has_cap() which requires a
>> +      * non-preemptible context.
>> +      */
>> +     if (cpu == get_cpu()) {
>> +             func(val);
>> +             put_cpu();
>> +             return 0;
>> +     }
>> +     put_cpu();
> A slightly more elegant way to write this would be:
>
> diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
> index 3fe1faab03620..83c7b346dc8cf 100644
> --- a/arch/arm64/kernel/topology.c
> +++ b/arch/arm64/kernel/topology.c
> @@ -406,6 +406,13 @@ int counters_read_on_cpu(int cpu, smp_call_func_t func, u64 *val)
>          if (!cpu_has_amu_feat(cpu))
>                  return -EOPNOTSUPP;
>
> +       scoped_guard(preempt) {
> +               if (cpu == raw_smp_processor_id()) {
> +                       func(val);
> +                       return 0;
> +               }
> +       }
> +
>          if (WARN_ON_ONCE(irqs_disabled()))
>                  return -EPERM;

Thank you for the suggestion.
Will switched to this in v3.


> But I'm more concerned by the overall pattern of doing these things in
> random contexts. Going back to the original patch (997c021abc6e) that
> states:
>
> "However, this deferred update mechanism is unnecessary and introduces extra
>   overhead for non-PCC register spaces (e.g. System Memory or FFH), where
>   accessing the regs won't sleep and can be safely performed from the tick
>   context."
>
> Clearly, the AMU registers cannot be arbitrarily accessed without
> blocking when accessed from one CPU to another, so either this
> function is never called in a cross-cpu context (and the warning
> should be removed), or the premises of this change are wrong.
>
> Which one is it?
>
> Thanks,
>
>          M.

The function is also called in cross-CPU contexts, but only from
process context (get_rate, sysfs show) with IRQs enabled and
calling smp_call_function_single() is safe. In the tick path,
cppc_scale_freq_tick() uses per_cpu(cppc_freq_inv, smp_processor_id()).
So we always read the current CPU's counters and no cross-CPU access.

The premise of 997c021abc6e applies to same-CPU accesses only:
Reading the local CPU's AMU regs does not sleep and is safe from
tick context.
The irqs_disabled() WARN is still needed to guard against unsafe
cross-CPU calls. This also returns '-EPERM' unlike the WARN inside
smp_call_function_single(). So we fail instead of risking deadlock.

Thank you,
Sumit Gupta