* [PATCH] thermal: fix frequency table lookup bugs @ 2013-04-08 23:54 Andrew Bresticker 2013-04-09 14:55 ` Eduardo Valentin 0 siblings, 1 reply; 6+ messages in thread From: Andrew Bresticker @ 2013-04-08 23:54 UTC (permalink / raw) To: Zhang Rui; +Cc: linux-pm, linux-kernel, Andrew Bresticker 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; @@ -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; - i++; + level_index++; } 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--; } 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++; } return ret; } -- 1.8.1.3 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] thermal: fix frequency table lookup bugs 2013-04-08 23:54 [PATCH] thermal: fix frequency table lookup bugs Andrew Bresticker @ 2013-04-09 14:55 ` Eduardo Valentin 2013-04-09 17:02 ` Andrew Bresticker 0 siblings, 1 reply; 6+ messages in thread From: Eduardo Valentin @ 2013-04-09 14:55 UTC (permalink / raw) To: Andrew Bresticker; +Cc: Zhang Rui, linux-pm, linux-kernel, eduardo.valentin 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; > } > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] thermal: fix frequency table lookup bugs 2013-04-09 14:55 ` Eduardo Valentin @ 2013-04-09 17:02 ` Andrew Bresticker 2013-04-09 17:21 ` Eduardo Valentin 0 siblings, 1 reply; 6+ messages in thread From: Andrew Bresticker @ 2013-04-09 17:02 UTC (permalink / raw) To: Eduardo Valentin; +Cc: Andrew Bresticker, Zhang Rui, linux-pm, linux-kernel Hi Eduardo, On Tue, Apr 9, 2013 at 7:55 AM, Eduardo Valentin <eduardo.valentin@ti.com> 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@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? 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. > Can you please split your patch into smaller changes? I don't think there is a need for separate patches to cpu_cooling.c. > >> 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] thermal: fix frequency table lookup bugs 2013-04-09 17:02 ` Andrew Bresticker @ 2013-04-09 17:21 ` Eduardo Valentin 2013-04-09 18:27 ` Andrew Bresticker 0 siblings, 1 reply; 6+ messages in thread From: Eduardo Valentin @ 2013-04-09 17:21 UTC (permalink / raw) To: Andrew Bresticker Cc: Eduardo Valentin, Andrew Bresticker, Zhang Rui, linux-pm, linux-kernel On 09-04-2013 13:02, Andrew Bresticker wrote: > Hi Eduardo, > > On Tue, Apr 9, 2013 at 7:55 AM, Eduardo Valentin > <eduardo.valentin@ti.com> 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@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? > > 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 > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] thermal: fix frequency table lookup bugs 2013-04-09 17:21 ` Eduardo Valentin @ 2013-04-09 18:27 ` Andrew Bresticker 2013-04-09 18:33 ` Eduardo Valentin 0 siblings, 1 reply; 6+ messages in thread From: Andrew Bresticker @ 2013-04-09 18:27 UTC (permalink / raw) To: Eduardo Valentin; +Cc: Zhang Rui, linux-pm, linux-kernel > 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. So the issue is that the changes are not independent. With just the fix for the infinite loop, get_cpu_frequency() is still completely broken because it is not interpreting the level correctly and will return the wrong frequency (and thus not throttling correctly, which is bad). Perhaps the commit should be more general, like "fix handling of invalid frequency table entries"? What do you think? Thanks, Andrew ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] thermal: fix frequency table lookup bugs 2013-04-09 18:27 ` Andrew Bresticker @ 2013-04-09 18:33 ` Eduardo Valentin 0 siblings, 0 replies; 6+ messages in thread From: Eduardo Valentin @ 2013-04-09 18:33 UTC (permalink / raw) To: Andrew Bresticker; +Cc: Eduardo Valentin, Zhang Rui, linux-pm, linux-kernel On 09-04-2013 14:27, Andrew Bresticker wrote: >> 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. > > So the issue is that the changes are not independent. With just the > fix for the infinite loop, get_cpu_frequency() is still completely > broken because it is not interpreting the level correctly and will > return the wrong frequency (and thus not throttling correctly, which > is bad). Perhaps the commit should be more general, like "fix > handling of invalid frequency table entries"? What do you think? It fits better. I am OK if we improve the commit title and description, providing better explanation of what is the issue (you have done already) and how what needed to be done to fix it. > > Thanks, > Andrew > > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-04-09 18:33 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-04-08 23:54 [PATCH] thermal: fix frequency table lookup bugs Andrew Bresticker 2013-04-09 14:55 ` Eduardo Valentin 2013-04-09 17:02 ` Andrew Bresticker 2013-04-09 17:21 ` Eduardo Valentin 2013-04-09 18:27 ` Andrew Bresticker 2013-04-09 18:33 ` Eduardo Valentin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).