Re: [PATCH] cpufreq: cpufreq_stats: make last_index signed int

From: Bo Yan
Date: Mon Sep 18 2017 - 13:43:43 EST

On 09/17/2017 06:50 PM, Viresh Kumar wrote:
On 15-09-17, 13:13, Bo Yan wrote:
It is possible for last_index to get a -1 if current frequency
is not found in the freq table when stats is created. If the
function "cpufreq_stats_update" is called before last_index is
updated with a valid value, the "-1" will be used as index to
update stats->time_in_state, triggering an exception.

No, that's not how it works AFAIK and if it did, then can you explain how your
solution fixes it?

AFAIK, what happens right now is that stats->last_index eventually stores
2147483647 (uint max) and the exception comes while accessing that value.

While with your change, it will become -1 and accessing array[-1] is fine by C
standards, though it is still the wrong thing to do as you are accessing
something outside of the array.

We should just check last_index == -1 before calling cpufreq_stats_update(),
which is already done by one of the callers.

Currently, the "last_index" is being checked before cpufreq_stats_update(stats) inside function "cpufreq_stats_record_transition", so it's taken care of.

However, the function "show_time_in_state" also calls cpufreq_stats_update, the similar check should be done there too, like this:

diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
index e75880eb037d..15305b5ec322 100644
--- a/drivers/cpufreq/cpufreq_stats.c
+++ b/drivers/cpufreq/cpufreq_stats.c
@@ -62,7 +62,8 @@ static ssize_t show_time_in_state(struct cpufreq_policy *policy, char *buf)
if (policy->fast_switch_enabled)
return 0;

- cpufreq_stats_update(stats);
+ if ((int)stats->last_index >= 0)
+ cpufreq_stats_update(stats);
for (i = 0; i < stats->state_num; i++) {
len += sprintf(buf + len, "%u %llu\n", stats->freq_table[i],
(unsigned long long)

This is only needed when policy->cur is not in frequency table when stats table is created, in which case, stats->last_index will get -1, then user does a "cat time_in_state" before any frequency transition.

Does this make sense?