public inbox for cpufreq@vger.kernel.org
 help / color / mirror / Atom feed
From: Nishanth Menon <nm@ti.com>
To: "Rafael J. Wysocki" <rjw@rjwysocki.net>,
	Viresh Kumar <viresh.kumar@linaro.org>
Cc: cpufreq@vger.kernel.org, linux-pm@vger.kernel.org,
	linux-kernel@vger.kernel.org, ceh@ti.com,
	Nishanth Menon <nm@ti.com>,
	Tobias Diedrich <ranma+xen@tdiedrich.de>,
	Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
	Dave Jones <davej@redhat.com>
Subject: [PATCH] cpufreq: stats: Do not populate stats when policy->cur has no exact match
Date: Fri, 15 Nov 2013 18:20:43 -0600	[thread overview]
Message-ID: <1384561243-30391-1-git-send-email-nm@ti.com> (raw)

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@tdiedrich.de>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Dave Jones <davej@redhat.com>
Reported-by: Carlos Hernandez <ceh@ti.com>
Signed-off-by: Nishanth Menon <nm@ti.com>
---

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:
-- 
1.7.9.5


             reply	other threads:[~2013-11-16  0:20 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-11-16  0:20 Nishanth Menon [this message]
2013-11-16  1:10 ` [PATCH] cpufreq: stats: Do not populate stats when policy->cur has no exact match Rafael J. Wysocki
2013-11-16  5:22   ` viresh kumar
2013-11-18 15:08     ` Nishanth Menon
2013-11-18 15:22       ` Viresh Kumar
2013-11-18 15:34         ` Nishanth Menon
2013-11-18 15:43           ` Viresh Kumar

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1384561243-30391-1-git-send-email-nm@ti.com \
    --to=nm@ti.com \
    --cc=ceh@ti.com \
    --cc=cpufreq@vger.kernel.org \
    --cc=davej@redhat.com \
    --cc=konrad.wilk@oracle.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=ranma+xen@tdiedrich.de \
    --cc=rjw@rjwysocki.net \
    --cc=viresh.kumar@linaro.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox