| Message ID | 1365465287-24530-1-git-send-email-abrestic@chromium.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Zhang Rui |
| Headers | show |
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; > } > -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > > -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > > -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; }
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(-)