Re: [PATCH V2 1/3] cpufreq: Add start_cpu() and stop_cpu() callbacks

[Viresh Kumar <viresh.kumar@linaro.org>]

On 17-06-21, 15:33, Rafael J. Wysocki wrote:
> On Wed, Jun 16, 2021 at 8:48 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > +       /* Do CPU specific de-initialization if required */
> > +       if (cpufreq_driver->stop_cpu)
> > +               cpufreq_driver->stop_cpu(policy, cpu);
> > +
> >         /* Start governor again for active policy */
> >         if (!policy_is_inactive(policy)) {
> >                 if (has_target()) {
> > @@ -1597,9 +1611,6 @@ static int cpufreq_offline(unsigned int cpu)
> >                 policy->cdev = NULL;
> >         }
> >
> > -       if (cpufreq_driver->stop_cpu)
> > -               cpufreq_driver->stop_cpu(policy);
> > -
> 
> This should be a separate patch IMO, after you've migrated everyone to
> ->offline/->exit.

Right, anyway this patch may not be required anymore. I will send a patch to
remove this.

> BTW, IMO it might be better to migrate ->stop_cpu to ->offline rather
> than to ->exit.

This is a bit tricky IMO.

First, offline() isn't implemented by everyone, out of the three implementations
which were using stop_cpu(), only intel-pstate had offline() as well.

Second, the primary purpose of online/offline callbacks was suspend/resume
oriented and for the same reason, we don't call online() when the policy first
comes up and so in case of errors during bring up, we end up calling exit()
directly and not offline().

IMO this is a very specific thing to drivers and they need to see what fits best
for them, exit() or offline() or both.

-- 
viresh