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

[Beata Michalska <beata.michalska@arm.com>]

Hi Ionela,
On Mon, Sep 01, 2025 at 08:29:26AM +0100, Ionela Voinescu wrote:
> 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(). 
> 

Thank you for pointing that out - that indeed might be an issue, and the
solution you have suggested should do the trick.

---
BR
Beata

> Hope it helps,
> Ionela.
> 
> >  	cpufreq_stats_record_transition(policy, freq);
> >  
> > -- 
> > 2.33.0
> > 
> >