[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <aLqRt0K2-UH8cOcI@arm.com>
Date: Fri, 5 Sep 2025 09:31:03 +0200
From: Beata Michalska <beata.michalska@....com>
To: Ionela Voinescu <ionela.voinescu@....com>
Cc: Lifeng Zheng <zhenglifeng1@...wei.com>, catalin.marinas@....com,
will@...nel.org, rafael@...nel.org, viresh.kumar@...aro.org,
sudeep.holla@....com, linux-arm-kernel@...ts.infradead.org,
linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org,
linuxarm@...wei.com, jonathan.cameron@...wei.com,
vincent.guittot@...aro.org, yangyicong@...ilicon.com,
zhanjie9@...ilicon.com, lihuisong@...wei.com, yubowen8@...wei.com,
zhangpengjie2@...wei.com, linhongye@...artners.com
Subject: Re: [PATCH v5 3/3] arm64: topology: Setup AMU FIE for online CPUs
only
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@...wei.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
> >
> >
Powered by blists - more mailing lists