cpufreq: cpufreq_stats: make last_index signed int
PATCH 1/1 cpufreq: cpufreq_stats: make last_index signed int
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.
Change the type of last_index of cpufreq_stats from unsigned int to signed int.
Move last_time to location right before time_in_state. This helps save 4 bytes in a LP64 programming model.
--- drivers/cpufreq/cpufreq_stats.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/cpufreq/cpufreq_stats.c +2 -2
@@ -18,10 +18,10 @@ static DEFINE_SPINLOCK(cpufreq_stats_lock);struct cpufreq_stats {unsigned int total_trans; -unsigned long long last_time;unsigned int max_state;unsigned int state_num; -unsigned int last_index; +int last_index; +unsigned long long last_time;u64 *time_in_state;unsigned int *freq_table;unsigned int *trans_table;
Signature · 1 line
2.7.4
Viresh Kumar
maintainerQuoted 5 lines · Bo Yan
It is possible for
last_indexto 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 beforelast_indexis updated with a valid value, the "-1" will be used as index to updatestats->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.
Signature · 1 line
viresh
Quoted 13 lines · Viresh Kumar
Quoted 5 lines · Bo Yan
It is possible for
last_indexto 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 beforelast_indexis updated with a valid value, the "-1" will be used as index to updatestats->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_indexeventually stores 2147483647 (uint max) and the exception comes while accessing that value.
Quoted 7 lines
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 callingcpufreq_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:
drivers/cpufreq/cpufreq_stats.c +2 -1
@@ -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?
Viresh Kumar
maintainerQuoted 6 lines · Bo Yan
Currently, the "
last_index" is being checked beforecpufreq_stats_update(stats)inside function "cpufreq_stats_record_transition", so it's taken care of.However, the function "
show_time_in_state" also callscpufreq_stats_update, the similar check should be done there too, like this:
Yeah, that's what I suggested.
drivers/cpufreq/cpufreq_stats.c static ssize_t show_time_in_state(struct cpufreq_policy · line 66
Quoted 12 lines
drivers/cpufreq/cpufreq_stats.c +1
-1@@ -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 )
You can rather do:
if (stats->last_index != -1)
Quoted 11 lines
drivers/cpufreq/cpufreq_stats.c +1
-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->curis not in frequency table when stats table is created, in which case,stats->last_indexwill get -1, then user does a "cattime_in_state" before any frequency transition.Does this make sense?
Signature · 1 line
viresh