Re: [PATCH v6] cpufreq: Add SFI based cpufreq driver support

["Kasagar, Srinidhi" <srinidhi.kasagar@intel.com>]

On Wed, Dec 03, 2014 at 10:21:57PM -0500, Len Brown wrote:
> I like this trend:

..and the trend will continue in the next version
as well :) Thank you and Viresh..

> 
> drivers/cpufreq/sfi-cpufreq.c |  529 +++++++++++++++++++++++++++++++++++++++++
> v1: drivers/cpufreq/sfi-cpufreq.c |  426
> +++++++++++++++++++++++++++++++++++++++++
> v2: drivers/cpufreq/sfi-cpufreq.c |  332
> +++++++++++++++++++++++++++++++++++++++++
> v3: drivers/cpufreq/sfi-cpufreq.c |  326
> +++++++++++++++++++++++++++++++++++++++++
> v4: drivers/cpufreq/sfi-cpufreq.c |  317
> +++++++++++++++++++++++++++++++++++++++++
> v5: drivers/cpufreq/sfi-cpufreq.c         |  238
> +++++++++++++++++++++++++++++++++
> v6: drivers/cpufreq/sfi-cpufreq.c         |  230
> +++++++++++++++++++++++++++++++++
> 
> 
> 
> On Tue, Nov 25, 2014 at 12:42 AM, Srinidhi Kasagar
> <srinidhi.kasagar@intel.com> wrote:
> 
> > +++ b/drivers/cpufreq/sfi-cpufreq.c
> 
> > +#define SFI_FREQ_MASK          0xff00
> > +
> > +static DEFINE_PER_CPU(struct sfi_cpufreq_data *, drv_data);
> > +static DEFINE_MUTEX(performance_mutex);
> 
> I'm not sure that cpufreq is able to call driver init's in parallel,
> so I'm not sure that the driver init needs to lock against other
> invocations of itself.
> But even if called in parallel, what data does performance_mutex
> actually protect?

hmm..I can drop this mutex stuff then..

[...]

> > +static int parse_freq(struct sfi_table_header *table)
> > +{
> > +       struct sfi_table_simple *sb;
> > +
> > +       sb = (struct sfi_table_simple *)table;
> > +
> > +       sfi_cpufreq_num = SFI_GET_NUM_ENTRIES(sb, struct sfi_freq_table_entry);
> > +       f_entry = (struct sfi_freq_table_entry *)sb->pentry;
> 
> are we sure that this pointer will still be valid after
> sfi_table_parse() returns?
> sfi_table_parse() appears to do an sfi_table_put() at the end...

That's why there used to be a memcpy earlier. I checked the instances
of sfi_table_parse and they mostly keep memcpy if that's global
and in our case, it is. So, I will retain that memcpy..

[...]

> > +static unsigned int get_cur_freq_on_cpu(unsigned int cpu)
> > +{
> > +       u32 lo, hi, sfi_ctrl;
> > +       struct cpufreq_frequency_table *pos;
> > +       struct sfi_cpufreq_data *data = per_cpu(drv_data, cpu);
> > +
> > +       rdmsr_on_cpu(cpu, MSR_IA32_PERF_CTL, &lo, &hi);
> > +       lo &= SFI_FREQ_MASK;
> > +
> > +       cpufreq_for_each_entry(pos, data->freq_table) {
> > +               sfi_ctrl = data->sfi_data->states[pos->driver_data].ctrl_val
> > +                       & SFI_FREQ_MASK;
> > +               if (sfi_ctrl == lo)
> > +                       return pos->frequency;
> > +       }
> > +       return data->freq_table[0].frequency;
> > +}
> 
> Not obvious (to me) that this in-accurate return value is more useful
> to the caller than simply returning data->freq_table[state].frequency.
> 
> Unless the caller is taking into account for hardware coordination
> of multiple PERF_CTRL MSR's, then it is garbage-in/garbage-out,
> not to mention that by the time this value is used, it may have changed...
> 
> I'm curious what breaks in your Android setup if you don't supply
> a get_cur_freq() at all...

Hmm..I do not think there is any mandate that the driver
should provide ->get callbacks to the core. However i'm
not sure if any library tools like libcpupower needs this.

I don't mind dropping this ->get callback, the driver still
works and I'm happy with the scaling_setspeed.

Srinidhi