On Friday, November 15, 2013 06:20:43 PM Nishanth Menon wrote: > commit 46a310b ([CPUFREQ] Don't set stat->last_index to -1 if the > pol->cur has incorrect value.) tries to handle case where policy->cur > does not match any entry in freq_table. > > As indicated in the above commit, the exact match search of > freq_table_get index will return a -1 which is stored in > stat->last_index. However, as a result of the above commit, > cpufreq_stat_notifier_trans which updates the statistics, fails > to update any *further* valid transitions that take place as > stat->last_index is -1 as the condition occurs at boot and never > solved. > > So, instead of having a statistics information that never ever > reflects valid data in the mentioned case and scratching our heads, we > instead, refuse to populate any of the statistics entries and note in > kernel log the error condition for developers to fix. The only useable > information are the available frequencies which is already available > in other cpufreq sysfs entries. > > Cc: Tobias Diedrich <ranma+xen@xxxxxxxxxxxx> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx> > Cc: Dave Jones <davej@xxxxxxxxxx> > Reported-by: Carlos Hernandez <ceh@xxxxxx> > Signed-off-by: Nishanth Menon <nm@xxxxxx> I like this one. Any objections from anyone? > --- > > Patch based on v3.12 tag > > Reported by Carlos on TI vendor kernel (v3.12tag based): > > OMAP5uEVM: http://pastebin.mozilla.org/3612196 > AM335x-evm: http://pastebin.mozilla.org/3612220 > > As can be seen, the translation table are present, however none of the > transition information is ever updated. > With the current patch, this becomes: http://pastebin.mozilla.org/3612256 > > Original commit thread seems to be: > https://lkml.org/lkml/2011/6/16/760 but I dont see much information if > this was discussed further. > > An alternate approach possible is to try and recover from this case by > using an 'atmost' match to find a closest match and then giveup when > we cant find one, but that does not really indicate a proper statistic > (if freq1 < cur_freq < freq2, was the intent to be at freq1 or freq2? > stats should not make that policy decision) > http://pastebin.mozilla.org/3612179 (alternate approach 1) > > Yet another option might be to update last_index using the first > transition into a valid index, but then, we wont have statistics > information 100% correct as we did not store the very first frequency > stat. > http://pastebin.mozilla.org/3612241 (alternate approach 2) > > drivers/cpufreq/cpufreq_stats.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c > index 4cf0d28..4c8a501 100644 > --- a/drivers/cpufreq/cpufreq_stats.c > +++ b/drivers/cpufreq/cpufreq_stats.c > @@ -251,6 +251,14 @@ static int cpufreq_stats_create_table(struct cpufreq_policy *policy, > stat->last_time = get_jiffies_64(); > stat->last_index = freq_table_get_index(stat, policy->cur); > spin_unlock(&cpufreq_stats_lock); > + > + if (stat->last_index == -1) { > + pr_err("%s: No match for current freq %u in table. Disabled!\n", > + __func__, policy->cur); > + ret = -EINVAL; > + goto error_out; > + } > + > cpufreq_cpu_put(current_policy); > return 0; > error_out: > -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- To unsubscribe from this list: send the line "unsubscribe cpufreq" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
