Re: [PATCH V3 4/4] cpufreq: CPPC: Add support for frequency invariance

[Viresh Kumar]

On 28-06-21, 11:49, Ionela Voinescu wrote:
> To be honest I would like to have more time on this before you merge the
> set, to better understand Qian's results and some observations I have
> for Thunder X2 (I will share in a bit).

Ideally, this code was already merged in 5.13 and would have required
us to fix any problems as we encounter them. I did revert it because
it caused a kernel crash and I wasn't sure if there was a sane/easy
way of fixing that so late in the release cycle. That was the right
thing to do then.

All those issues are gone now, we may have an issue around rounding of
counters or some hardware specific issues, it isn't clear yet.

But the stuff works fine otherwise, doesn't make the kernel crash and
it is controlled with a CONFIG_ option, so those who don't want to use
it can still disable it.

The merge window is here now, if we don't merge it now, it gets
delayed by a full cycle (roughly two months) and if we merge it now
and are able to narrow down the rounding issues, if there are any, we
will have full two months to make a fix for that and still push it in
5.14 itself.

And so I would like to get it merged in this merge window itself, it
also makes sure more people would get to test it, like Qian was able
to figure out a problem here for us.

> For the code, I think it's fine. I have a single observation regarding
> the following code:
> 
> > +static void cppc_cpufreq_cpu_fie_init(struct cpufreq_policy *policy)
> > +{
> > +	struct cppc_freq_invariance *cppc_fi;
> > +	int cpu, ret;
> > +
> > +	if (cppc_cpufreq_driver.get == hisi_cppc_cpufreq_get_rate)
> > +		return;
> > +
> > +	for_each_cpu(cpu, policy->cpus) {
> > +		cppc_fi = &per_cpu(cppc_freq_inv, cpu);
> > +		cppc_fi->cpu = cpu;
> > +		cppc_fi->cpu_data = policy->driver_data;
> > +		kthread_init_work(&cppc_fi->work, cppc_scale_freq_workfn);
> > +		init_irq_work(&cppc_fi->irq_work, cppc_irq_work);
> > +
> > +		ret = cppc_get_perf_ctrs(cpu, &cppc_fi->prev_perf_fb_ctrs);
> > +		if (ret) {
> > +			pr_warn("%s: failed to read perf counters for cpu:%d: %d\n",
> > +				__func__, cpu, ret);
> > +			return;
> > +		}
> 
> For this condition above, think about a scenario where reading counters
> for offline CPUs returns an error. I'm not sure if that can happen, to
> be honest. That would mean here that you will never initialise the freq
> source unless all CPUs in the policy are online at policy creation.
> 
> My recommendation is to warn about the failed read of perf counters but
> only return from this function if the target CPU is online as well when
> reading counters fails.
> 
> This is probably a nit, so I'll let you decide if you want to do something
> about this.

That is a very good observation actually. Thanks for that. This is how
I fixed it.

diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
index d688877e8fbe..f6540068d0fe 100644
--- a/drivers/cpufreq/cppc_cpufreq.c
+++ b/drivers/cpufreq/cppc_cpufreq.c
@@ -171,7 +171,13 @@ static void cppc_cpufreq_cpu_fie_init(struct cpufreq_policy *policy)
                if (ret) {
                        pr_warn("%s: failed to read perf counters for cpu:%d: %d\n",
                                __func__, cpu, ret);
-                       return;
+
+                       /*
+                        * Don't abort if the CPU was offline while the driver
+                        * was getting registered.
+                        */
+                       if (cpu_online(cpu))
+                               return;
                }
        }

-- 
viresh