Re: [PATCH 2/2] cpufreq: CPPC: Fix error handling in cppc_scale_freq_workfn()

[Jie Zhan <zhanjie9@hisilicon.com>]

On 31/07/2025 06:34, Prashant Malani wrote:
> Thanks for adding me, Viresh.
> 
> On Tue, 29 Jul 2025 at 23:39, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>>
>> + Prashant/Beata/Ionela
>>
>> On 30-07-25, 11:23, Bowen Yu wrote:
>>> From: Jie Zhan <zhanjie9@hisilicon.com>
>>>
>>> Perf counters could be 0 if the cpu is in a low-power idle state. Just try
>>> it again next time and update the frequency scale when the cpu is active
>>> and perf counters successfully return.
>>>
>>> Also, remove the FIE source on an actual failure.
>>>
>>> Signed-off-by: Jie Zhan <zhanjie9@hisilicon.com>
>>> ---
>>>  drivers/cpufreq/cppc_cpufreq.c | 13 ++++++++++++-
>>>  1 file changed, 12 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
>>> index 904006027df2..e95844d3d366 100644
>>> --- a/drivers/cpufreq/cppc_cpufreq.c
>>> +++ b/drivers/cpufreq/cppc_cpufreq.c
>>> @@ -78,12 +78,23 @@ static void cppc_scale_freq_workfn(struct kthread_work *work)
>>>       struct cppc_cpudata *cpu_data;
>>>       unsigned long local_freq_scale;
>>>       u64 perf;
>>> +     int ret;
>>>
>>>       cppc_fi = container_of(work, struct cppc_freq_invariance, work);
>>>       cpu_data = cppc_fi->cpu_data;
>>>
>>> -     if (cppc_get_perf_ctrs(cppc_fi->cpu, &fb_ctrs)) {
>>> +     ret = cppc_get_perf_ctrs(cppc_fi->cpu, &fb_ctrs);
>>> +     /*
>>> +      * Perf counters could be 0 if the cpu is in a low-power idle state.
>>> +      * Just try it again next time.
>>> +      */
> 
> FWIU the performance counters shouldn't be returning 0 in an idle state.
> Per the UEFI spec [1], they increment any time the CPU is active,
> so they should just return their last counter value before they went into idle
> (of course in the FFH case an IPI is performed on the target CPU, so even
> if the CPU was idle, it will get woken up).

Hi Prashant,

The perf counters could return 0 when a CPU is enters a low-power idle
state, e.g. reset or powered down, and the perf counters are in the system
memory space (the target CPU is not woken up unfortunately).

On our platform, and I suppose so on most ARM64 platforms, perf counters
are mapped to AMU counters.  Per ARM spec, AMEVCNTR0 is 0 on reset.  BTW,
that's also why ARM Trusted Firmware needs to save and restore AMU counters
before and after powering down.
https://github.com/ARM-software/arm-trusted-firmware/blob/master/lib/extensions/amu/aarch64/amu.c

I hope this explains your confusion.  Then we can carry on discussion if we
reach on this consensus.

Thanks for taking a look!
Jie

> 
> As such it is better to either :
> - Check for idle_cpu() directly and return (see [2] for the function)
> or
> - Always clear the source on encountering an error return value.
> 
> [1] https://uefi.org/specs/ACPI/6.5/08_Processor_Configuration_and_Control.html?highlight=cppc#performance-counters
> [2] https://lore.kernel.org/linux-pm/20250619000925.415528-2-pmalani@google.com/
> 
> HTH,
> 
> -Prashant