Re: [PATCH V2] cpufreq: Disallow ->resolve_freq() for drivers providing ->target_index()

[Rafael J. Wysocki]

On Friday, July 22, 2016 08:13:27 AM Viresh Kumar wrote:
> On 21-07-16, 17:34, Steve Muckle wrote:
> > On Fri, Jul 22, 2016 at 02:18:54AM +0200, Rafael J. Wysocki wrote:
> > > > My thinking was that one of these two would be preferable:
> > > >
> > > > - Forcing ->target() drivers to install a ->resolve_freq callback,
> > > >   enforcing this at cpufreq driver init time.
> > > 
> > > That would have been possible, but your series didn't do that.
> > > 
> > > >   My understanding is
> > > >   ->target() drivers are deprecated anyway
> > > 
> > > No, they aren't.
> > 
> > Ok. I didn't follow Documentation/cpu-freq/cpu-drivers.txt section 1.5
> > then - it suggests something about target() is deprecated, perhaps it's
> > out of date.
> 
> They are kind of deprecated for the new uesrs, but we still have
> handful of users of it.

No, they aren't deprecated, not even sort of.

Of course, stuff that can use frequency tables should implement ->target_index,
because there's no valid reason for it not to do that.

However, there are cases (and not legacy) where frequency tables are simply
impractical and those drivers have no choice but to implement ->target.

And if you want to try to force them into the frequency tables model
regardless, then think twice, because I'm not going to let you do that.

> > Sorry, that should've been "check that either ->target_index() or
> > ->resolve_freq() is implemented." 
> > 
> > Implementing resolve_freq for the target() drivers and requiring it at
> > driver init time is probably the better way to go though. Perhaps I can
> > work on this at some point.
> 
> As I said earlier as well in one of the emails, if you are worried
> about the extra 'if' check in the hot path, then wouldn't this fix it
> for you?
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 3dd4884c6f9e..91d8ec4c8eb7 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -517,7 +517,7 @@ unsigned int cpufreq_driver_resolve_freq(struct cpufreq_policy *policy,
>                 return policy->freq_table[idx].frequency;
>         }
>  
> -       if (cpufreq_driver->resolve_freq)
> +       if (likely(cpufreq_driver->resolve_freq))
>                 return cpufreq_driver->resolve_freq(policy, target_freq);
>  
>         return target_freq;

A CPU with good enough branch prediction logic should be able to figure out
whether or not the test is "likely" after a few repetitions of it.

Thanks,
Rafael