lkml.org 
[lkml]   [2013]   [Apr]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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;
> }
>



\
 
 \ /
  Last update: 2013-04-09 17:21    [W:0.059 / U:0.120 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site