Messages in this thread | | | Date | Tue, 9 Apr 2013 10:55:17 -0400 | From | Eduardo Valentin <> | Subject | Re: [PATCH] thermal: fix frequency table lookup bugs |
| |
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@chromium.org> > --- > 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?
> > @@ -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? Besides why would you be comparing level against 0 all the time (you have initialized level_index to 0 at this point).
> - i++; > + level_index++;
level_index wont be updated in case of INVALID entry.
> } > 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
Can you please split your patch into smaller changes? > 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?
> } > return ret; > } >
| |