Re: [PATCH v5 3/3] arm64: topology: Setup AMU FIE for online CPUs only

[Ionela Voinescu <ionela.voinescu@arm.com>]

Hi,

On Tuesday 19 Aug 2025 at 15:29:31 (+0800), Lifeng Zheng wrote:
> When boot with maxcpu=1 restrict, and LPI(Low Power Idle States) is on,
> only CPU0 will go online. The support AMU flag of CPU0 will be set but the
> flags of other CPUs will not. This will cause AMU FIE set up fail for CPU0
> when it shares a cpufreq policy with other CPU(s). After that, when other
> CPUs are finally online and the support AMU flags of them are set, they'll
> never have a chance to set up AMU FIE, even though they're eligible.
> 
> To solve this problem, the process of setting up AMU FIE needs to be
> modified as follows:
> 
> 1. Set up AMU FIE only for the online CPUs.
> 
> 2. Try to set up AMU FIE each time a CPU goes online and do the
> freq_counters_valid() check. If this check fails, clear scale freq source
> of all the CPUs related to the same policy, in case they use different
> source of the freq scale.
> 
> At the same time, this change also be applied to cpufreq when calling
> arch_set_freq_scale.
> 
> Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
> ---
>  arch/arm64/kernel/topology.c | 54 ++++++++++++++++++++++++++++++++++--
>  drivers/cpufreq/cpufreq.c    |  4 +--
>  2 files changed, 54 insertions(+), 4 deletions(-)
> 
[..]
>  
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 78ca68ea754d..d1890a2af1af 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -417,7 +417,7 @@ void cpufreq_freq_transition_end(struct cpufreq_policy *policy,
>  
>  	cpufreq_notify_post_transition(policy, freqs, transition_failed);
>  
> -	arch_set_freq_scale(policy->related_cpus,
> +	arch_set_freq_scale(policy->cpus,
>  			    policy->cur,
>  			    arch_scale_freq_ref(policy->cpu));
>  
> @@ -2219,7 +2219,7 @@ unsigned int cpufreq_driver_fast_switch(struct cpufreq_policy *policy,
>  		return 0;
>  
>  	policy->cur = freq;
> -	arch_set_freq_scale(policy->related_cpus, freq,
> +	arch_set_freq_scale(policy->cpus, freq,
>  			    arch_scale_freq_ref(policy->cpu));

I think it might be good to keep these calls to arch_set_freq_scale() for
all related CPUs and not only online ones. This can result in CPUs coming
out of hotplug with a wrong scale factor, because while they were out, any
frequency transitions of the policy only modified the scale factor of
online CPUs. When they come out of hotplug, arch_set_freq_scale() will not
be called for them until there's a new frequency transition.

I understand that if this is not changed to only pass online CPUs,
supports_scale_freq_counters() will now fail when called in
topology_set_freq_scale() for scenarios when only some CPUs in a policy
are online - e.g. the scenario in your commit message. But I think a
simple change in supports_scale_freq_counters() that instead checks that
at least one CPU in the policy supports AMU-based FIE, instead of all,
is a better fix that does not break the cpufreq-based FIE. If at least
one CPU is marked as supporting AMUs for FIE we know that the AMU setup
path is in progress and we should bail out of
topology_set_freq_scale()/arch_set_freq_scale(). 

Hope it helps,
Ionela.

>  	cpufreq_stats_record_transition(policy, freq);
>  
> -- 
> 2.33.0
> 
>