Re: [PATCH] thermal: fix frequency table lookup bugs
From: Eduardo Valentin
Date: Tue Apr 09 2013 - 13:21:58 EST
On 09-04-2013 13:02, Andrew Bresticker wrote:
Hi Eduardo,
On Tue, Apr 9, 2013 at 7:55 AM, Eduardo Valentin
<eduardo.valentin@xxxxxx> wrote:
Hi Andrew,
On 08-04-2013 19:54, Andrew Bresticker wrote:
The loops which are used to perform lookups in CPU frequency tables in
cpu_cooling and the Exynos thermal driver do not update the loop counter
if they encounter an invalid table entry, leading to an infinite loop in
that case.
Signed-off-by: Andrew Bresticker <abrestic@xxxxxxxxxxxx>
---
drivers/thermal/cpu_cooling.c | 19 ++++++++++---------
drivers/thermal/exynos_thermal.c | 8 ++++----
2 files changed, 14 insertions(+), 13 deletions(-)
diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
index 836828e..e6db441 100644
--- a/drivers/thermal/cpu_cooling.c
+++ b/drivers/thermal/cpu_cooling.c
@@ -124,14 +124,14 @@ static int is_cpufreq_valid(int cpu)
static unsigned int get_cpu_frequency(unsigned int cpu, unsigned long
level)
{
int ret = 0, i = 0;
- unsigned long level_index;
+ unsigned long level_index = 0;
bool descend = false;
struct cpufreq_frequency_table *table =
cpufreq_frequency_get_table(cpu);
if (!table)
return ret;
- while (table[i].frequency != CPUFREQ_TABLE_END) {
+ for (i = 0; table[i].frequency != CPUFREQ_TABLE_END; i++) {
if (table[i].frequency == CPUFREQ_ENTRY_INVALID)
continue;
Wouldn't be easier to just increase the index i before doing a continue?
I think this is cleaner. The code is iterating through an array -- it
should be a for loop.
@@ -143,24 +143,25 @@ static unsigned int get_cpu_frequency(unsigned int
cpu, unsigned long level)
}
/*return if level matched and table in descending order*/
- if (descend && i == level)
+ if (descend && level_index == level)
return table[i].frequency;
What this has to do with the patch description?
I'm using level_index as the counter of valid frequencies, where as i
is the index into the array. If there are invalid entries, they are
not necessarily equal. The point of this function is to find the
level-th *valid* frequency in the table.
Besides why would you be comparing level against 0 all the time (you have
initialized level_index to 0 at this point).
Huh? level_index is clearly incremented below...
- i++;
+ level_index++;
level_index wont be updated in case of INVALID entry.
That's the point.
}
i--;
+ level_index--;
- if (level > i || descend)
+ if (level > level_index || descend)
return ret;
- level_index = i - level;
+ level = level_index - level;
/*Scan the table in reverse order and match the level*/
- while (i >= 0) {
+ for (; i >= 0; i--) {
if (table[i].frequency == CPUFREQ_ENTRY_INVALID)
continue;
/*return if level matched*/
- if (i == level_index)
+ if (level_index == level)
return table[i].frequency;
- i--;
+ level_index--;
}
I believe you do more than what you have described in your intention under
you patch description
I disagree. I'm fixing the loop so that it properly handles invalid
entries and thus the infinite loop problem I mention in the commit
message.
In this case, I believe you should also rephrase your patch description,
explaining that you are also fixing a role for each index.
Can you please split your patch into smaller changes?
I don't think there is a need for separate patches to cpu_cooling.c.
You do two things in this change on cpu_cooling.c: (1) fix the case
where the loop is kept running indefinitely. (2) Reserve a specific role
for each index in this function.
For this reason, I suggested doing one thing per patch and splitting
this change into two for better review process. Having that split with a
good description for each change makes everyone life easier, don t you
think?
return ret;
}
diff --git a/drivers/thermal/exynos_thermal.c
b/drivers/thermal/exynos_thermal.c
index d5e6267..524b2a0 100644
--- a/drivers/thermal/exynos_thermal.c
+++ b/drivers/thermal/exynos_thermal.c
@@ -237,7 +237,7 @@ static int exynos_get_crit_temp(struct
thermal_zone_device *thermal,
static int exynos_get_frequency_level(unsigned int cpu, unsigned int
freq)
{
- int i = 0, ret = -EINVAL;
+ int i, level = 0, ret = -EINVAL;
struct cpufreq_frequency_table *table = NULL;
#ifdef CONFIG_CPU_FREQ
table = cpufreq_frequency_get_table(cpu);
@@ -245,12 +245,12 @@ static int exynos_get_frequency_level(unsigned int
cpu, unsigned int freq)
if (!table)
return ret;
- while (table[i].frequency != CPUFREQ_TABLE_END) {
+ for (i = 0; table[i].frequency != CPUFREQ_TABLE_END; i++) {
if (table[i].frequency == CPUFREQ_ENTRY_INVALID)
continue;
if (table[i].frequency == freq)
- return i;
- i++;
+ return level;
+ level++;
Can you please send a separate patch on this driver instead?
Sure.
}
return ret;
}
Thanks,
Andrew
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/