* [PATCH] Thermal: fix iteration over CPU frequency list @ 2013-01-24 15:24 Gu1 2013-02-01 7:59 ` Zhang Rui 2013-02-04 19:51 ` amit kachhap 0 siblings, 2 replies; 6+ messages in thread From: Gu1 @ 2013-01-24 15:24 UTC (permalink / raw) To: linux-pm, linux-kernel, Zhang Rui, Amit Daniel Kachhap; +Cc: Gu1 In different places in the Thermal code, the CPU frequency list is iterated in an incorrect way, leading to endless loops when the frequency list contains a CPUFREQ_TABLE_INVALID entry, which is the case by default in the the Exynos 4x12 cpufreq driver, for example. The frequency list is iterated with a while loop, and when a CPUFREQ_TABLE_INVALID entry is encountered, the continue; statement is used to skip it, but the index is not incremented, causing an endless loop. A similar bug was fixed by hongbo.zhang in commit: Thermal: fix bug of counting cpu frequencies Signed-off-by: Gu1 <gu1@aeroxteam.fr> --- drivers/thermal/cpu_cooling.c | 8 +++----- drivers/thermal/exynos_thermal.c | 9 +++++---- 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c index 836828e..51acd26 100644 --- a/drivers/thermal/cpu_cooling.c +++ b/drivers/thermal/cpu_cooling.c @@ -123,7 +123,7 @@ static int is_cpufreq_valid(int cpu) */ static unsigned int get_cpu_frequency(unsigned int cpu, unsigned long level) { - int ret = 0, i = 0; + int ret = 0, i; unsigned long level_index; bool descend = false; struct cpufreq_frequency_table *table = @@ -131,7 +131,7 @@ static unsigned int get_cpu_frequency(unsigned int cpu, unsigned long level) 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; @@ -145,7 +145,6 @@ 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) return table[i].frequency; - i++; } i--; @@ -154,13 +153,12 @@ static unsigned int get_cpu_frequency(unsigned int cpu, unsigned long level) level_index = i - 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) return table[i].frequency; - i--; } return ret; } diff --git a/drivers/thermal/exynos_thermal.c b/drivers/thermal/exynos_thermal.c index 224751e..fa9e1d7 100644 --- a/drivers/thermal/exynos_thermal.c +++ b/drivers/thermal/exynos_thermal.c @@ -233,7 +233,8 @@ 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, ret = -EINVAL; + unsigned int count = 0; struct cpufreq_frequency_table *table = NULL; #ifdef CONFIG_CPU_FREQ table = cpufreq_frequency_get_table(cpu); @@ -241,12 +242,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 count; + count++; } return ret; } -- 1.8.1.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] Thermal: fix iteration over CPU frequency list 2013-01-24 15:24 [PATCH] Thermal: fix iteration over CPU frequency list Gu1 @ 2013-02-01 7:59 ` Zhang Rui 2013-02-04 19:49 ` amit daniel kachhap 2013-02-04 19:51 ` amit kachhap 1 sibling, 1 reply; 6+ messages in thread From: Zhang Rui @ 2013-02-01 7:59 UTC (permalink / raw) To: Gu1; +Cc: linux-pm, linux-kernel, Amit Daniel Kachhap On Thu, 2013-01-24 at 16:24 +0100, Gu1 wrote: > In different places in the Thermal code, the CPU frequency list is iterated > in an incorrect way, leading to endless loops when the frequency list contains > a CPUFREQ_TABLE_INVALID entry, which is the case by default in the the Exynos > 4x12 cpufreq driver, for example. > > The frequency list is iterated with a while loop, and when a > CPUFREQ_TABLE_INVALID entry is encountered, the continue; statement is used to > skip it, but the index is not incremented, causing an endless loop. > > A similar bug was fixed by hongbo.zhang in commit: > Thermal: fix bug of counting cpu frequencies > > Signed-off-by: Gu1 <gu1@aeroxteam.fr> > --- > drivers/thermal/cpu_cooling.c | 8 +++----- > drivers/thermal/exynos_thermal.c | 9 +++++---- > 2 files changed, 8 insertions(+), 9 deletions(-) > > diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c > index 836828e..51acd26 100644 > --- a/drivers/thermal/cpu_cooling.c > +++ b/drivers/thermal/cpu_cooling.c > @@ -123,7 +123,7 @@ static int is_cpufreq_valid(int cpu) > */ > static unsigned int get_cpu_frequency(unsigned int cpu, unsigned long level) > { > - int ret = 0, i = 0; > + int ret = 0, i; > unsigned long level_index; > bool descend = false; > struct cpufreq_frequency_table *table = > @@ -131,7 +131,7 @@ static unsigned int get_cpu_frequency(unsigned int cpu, unsigned long level) > 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; > > @@ -145,7 +145,6 @@ 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) > return table[i].frequency; > - i++; > } > i--; > > @@ -154,13 +153,12 @@ static unsigned int get_cpu_frequency(unsigned int cpu, unsigned long level) > level_index = i - 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) > return table[i].frequency; > - i--; > } > return ret; > } so the "level" parameter is the index in the frequency table, right? > diff --git a/drivers/thermal/exynos_thermal.c b/drivers/thermal/exynos_thermal.c > index 224751e..fa9e1d7 100644 > --- a/drivers/thermal/exynos_thermal.c > +++ b/drivers/thermal/exynos_thermal.c > @@ -233,7 +233,8 @@ 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, ret = -EINVAL; > + unsigned int count = 0; > struct cpufreq_frequency_table *table = NULL; > #ifdef CONFIG_CPU_FREQ > table = cpufreq_frequency_get_table(cpu); > @@ -241,12 +242,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 count; > + count++; > } > return ret; > } but we ignore the invalid entry here. take the following cpufreq table for example, with your patch, entry frequency 0 2.4G 1 invalid 2 2G 3 invalid 4 1.6G 5 end in exynos_get_frequency_level(), freq 1.6G is translated to level 2, because count is increased only twice, for entry 0 and entry 2, right? but then, in get_cpu_frequency(), level 2 is translated to 2G HZ, which I do not think is what we want. I think we are doing something wrong here, and here is a cleanup patch I made to fix this issue, please review. >From a868d68fdcd94a29ac9d3998283119a453decb4b Mon Sep 17 00:00:00 2001 From: Zhang Rui <rui.zhang@intel.com> Date: Fri, 1 Feb 2013 15:41:47 +0800 Subject: [PATCH] Thermal cpu_cooling: fix inconsistent use of CPU frequency table there are three kinds of entries in CPU frequency table. 1. invalid entry, its frequency is CPUFREQ_ENTRY_INVALID. 2. duplicate entry, two entry may share the same frequency, And we should treat it as on valid entry. 3. valid entry with a proper frequency. And when talking about cpufreq cooling device cooling state, it should be the same thing, in both cpu_cooling.c and its users, AKA, max_cooling_state of a cpufreq cooling device equals the number of VALID entries (type #3 above) in CPU frequency table. And when setting a cpufreq cooling device to state X, it means set the cpufreq cooling device to Xth maximum frequency in the cpufreq frequency table. This patch does a cleanup in both drivers/thermal/cpu_cooling.c and drivers/thermal/exynos_thermal.c to make them be consistent in using cpu level/cpufreq cooling state. Signed-off-by: Zhang Rui <rui.zhang@intel.com> --- drivers/thermal/cpu_cooling.c | 122 ++++++++++++++++++++++++-------------- drivers/thermal/exynos_thermal.c | 17 +----- include/linux/cpu_cooling.h | 12 ++++ 3 files changed, 91 insertions(+), 60 deletions(-) diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c index 836828e..c16795b 100644 --- a/drivers/thermal/cpu_cooling.c +++ b/drivers/thermal/cpu_cooling.c @@ -116,6 +116,40 @@ static int is_cpufreq_valid(int cpu) } /** + * cpufreq_cooling_get_max_level - function to get max valid cpufreq levels + * @cpu: cpu for which frequency is fetched. + */ +int cpufreq_cooling_get_max_level(unsigned int cpu) +{ + int i, level; + unsigned int freq = CPUFREQ_ENTRY_INVALID; + struct cpufreq_frequency_table *table = + cpufreq_frequency_get_table(cpu); + + if (!table) + return -EINVAL; + + for (i = 0, level = 0; table[i].frequency != CPUFREQ_TABLE_END; i++) { + /* Invalid entry */ + if (table[i].frequency == CPUFREQ_ENTRY_INVALID) + continue; + + /* Duplicate entry */ + if (freq == table[i].frequency) + continue; + + /* First valid entry */ + if (freq == CPUFREQ_ENTRY_INVALID) + freq = table[i].frequency; + + level++; + } + + return level; +} +EXPORT_SYMBOL(cpufreq_cooling_get_max_level); + +/** * get_cpu_frequency - get the absolute value of frequency from level. * @cpu: cpu for which frequency is fetched. * @level: level of frequency of the CPU @@ -123,46 +157,58 @@ 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; + int i, count; + int max_level; + unsigned int freq = CPUFREQ_ENTRY_INVALID; bool descend = false; struct cpufreq_frequency_table *table = cpufreq_frequency_get_table(cpu); - if (!table) - return ret; - while (table[i].frequency != CPUFREQ_TABLE_END) { + max_level = cpufreq_cooling_get_max_level(cpu); + + if (max_level < 0) + return max_level; + + if (level > max_level) + return -EINVAL; + + for (i = 0; table[i].frequency != CPUFREQ_TABLE_END; i++) { if (table[i].frequency == CPUFREQ_ENTRY_INVALID) continue; - /*check if table in ascending or descending order*/ - if ((table[i + 1].frequency != CPUFREQ_TABLE_END) && - (table[i + 1].frequency < table[i].frequency) - && !descend) { - descend = true; + if (freq == CPUFREQ_ENTRY_INVALID) + /* first valid entry */ + freq = table[i].frequency; + else if (freq == table[i].frequency) + /* duplicate entry */ + continue; + else { + /* two valid entries, check frequency order */ + descend = !!(freq > table[i].frequency); + break; } - - /*return if level matched and table in descending order*/ - if (descend && i == level) - return table[i].frequency; - i++; } - i--; - if (level > i || descend) - return ret; - level_index = i - level; + /* level equals "the index of valid entries" in cpufreq table */ + level = descend ? level : max_level - level + 1; - /*Scan the table in reverse order and match the level*/ - while (i >= 0) { + freq = CPUFREQ_ENTRY_INVALID; + for (i = 0, count = 0; table[i].frequency != CPUFREQ_TABLE_END; i++) { + /* ignore invalid entry */ if (table[i].frequency == CPUFREQ_ENTRY_INVALID) continue; - /*return if level matched*/ - if (i == level_index) + + if (freq == CPUFREQ_ENTRY_INVALID) + /* first valid entry */ + freq = table[i].frequency; + else if (freq == table[i].frequency) + /* ignore duplicate entry */ + continue; + count++; + if (level == count) return table[i].frequency; - i--; } - return ret; + return -ENODEV; } /** @@ -244,29 +290,17 @@ static int cpufreq_get_max_state(struct thermal_cooling_device *cdev, struct cpufreq_cooling_device *cpufreq_device = cdev->devdata; struct cpumask *maskPtr = &cpufreq_device->allowed_cpus; unsigned int cpu; - struct cpufreq_frequency_table *table; - unsigned long count = 0; - int i = 0; + int max_level; cpu = cpumask_any(maskPtr); - table = cpufreq_frequency_get_table(cpu); - if (!table) { - *state = 0; - return 0; - } - - for (i = 0; (table[i].frequency != CPUFREQ_TABLE_END); i++) { - if (table[i].frequency == CPUFREQ_ENTRY_INVALID) - continue; - count++; - } - if (count > 0) { - *state = --count; - return 0; - } + max_level = cpufreq_cooling_get_max_level(cpu); - return -EINVAL; + if (max_level < 0) + return max_level; + else + *state = max_level; + return 0; } /** diff --git a/drivers/thermal/exynos_thermal.c b/drivers/thermal/exynos_thermal.c index cd71e24..5ac2de1 100644 --- a/drivers/thermal/exynos_thermal.c +++ b/drivers/thermal/exynos_thermal.c @@ -241,22 +241,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; - struct cpufreq_frequency_table *table = NULL; -#ifdef CONFIG_CPU_FREQ - table = cpufreq_frequency_get_table(cpu); -#endif - if (!table) - return ret; - - while (table[i].frequency != CPUFREQ_TABLE_END) { - if (table[i].frequency == CPUFREQ_ENTRY_INVALID) - continue; - if (table[i].frequency == freq) - return i; - i++; - } - return ret; + return cpufreq_cooling_get_max_level(cpu); } /* Bind callback functions for thermal zone */ diff --git a/include/linux/cpu_cooling.h b/include/linux/cpu_cooling.h index 40b4ef5..2de9319 100644 --- a/include/linux/cpu_cooling.h +++ b/include/linux/cpu_cooling.h @@ -42,6 +42,14 @@ struct thermal_cooling_device *cpufreq_cooling_register( * @cdev: thermal cooling device pointer. */ void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev); + +/** + * cpufreq_cooling_get_max_level - function to get maxinum cooling state + * of a cpufreq cooling device + * @cpu: cpu of which frequency is fetched. + */ +int cpufreq_cooling_get_max_level(unsigned int cpu); + #else /* !CONFIG_CPU_THERMAL */ static inline struct thermal_cooling_device *cpufreq_cooling_register( const struct cpumask *clip_cpus) @@ -53,6 +61,10 @@ static inline void cpufreq_cooling_unregister( { return; } +static inline int cpufreq_cooling_get_max_level(unsigned int cpu) +{ + return -ENODEV; +} #endif /* CONFIG_CPU_THERMAL */ #endif /* __CPU_COOLING_H__ */ -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] Thermal: fix iteration over CPU frequency list 2013-02-01 7:59 ` Zhang Rui @ 2013-02-04 19:49 ` amit daniel kachhap 2013-02-05 9:59 ` Zhang Rui 0 siblings, 1 reply; 6+ messages in thread From: amit daniel kachhap @ 2013-02-04 19:49 UTC (permalink / raw) To: Zhang Rui; +Cc: Gu1, linux-pm, linux-kernel On Thu, Jan 31, 2013 at 11:59 PM, Zhang Rui <rui.zhang@intel.com> wrote: > On Thu, 2013-01-24 at 16:24 +0100, Gu1 wrote: >> In different places in the Thermal code, the CPU frequency list is iterated >> in an incorrect way, leading to endless loops when the frequency list contains >> a CPUFREQ_TABLE_INVALID entry, which is the case by default in the the Exynos >> 4x12 cpufreq driver, for example. >> >> The frequency list is iterated with a while loop, and when a >> CPUFREQ_TABLE_INVALID entry is encountered, the continue; statement is used to >> skip it, but the index is not incremented, causing an endless loop. >> >> A similar bug was fixed by hongbo.zhang in commit: >> Thermal: fix bug of counting cpu frequencies >> >> Signed-off-by: Gu1 <gu1@aeroxteam.fr> >> --- >> drivers/thermal/cpu_cooling.c | 8 +++----- >> drivers/thermal/exynos_thermal.c | 9 +++++---- >> 2 files changed, 8 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c >> index 836828e..51acd26 100644 >> --- a/drivers/thermal/cpu_cooling.c >> +++ b/drivers/thermal/cpu_cooling.c >> @@ -123,7 +123,7 @@ static int is_cpufreq_valid(int cpu) >> */ >> static unsigned int get_cpu_frequency(unsigned int cpu, unsigned long level) >> { >> - int ret = 0, i = 0; >> + int ret = 0, i; >> unsigned long level_index; >> bool descend = false; >> struct cpufreq_frequency_table *table = >> @@ -131,7 +131,7 @@ static unsigned int get_cpu_frequency(unsigned int cpu, unsigned long level) >> 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; >> >> @@ -145,7 +145,6 @@ 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) >> return table[i].frequency; >> - i++; >> } >> i--; >> >> @@ -154,13 +153,12 @@ static unsigned int get_cpu_frequency(unsigned int cpu, unsigned long level) >> level_index = i - 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) >> return table[i].frequency; >> - i--; >> } >> return ret; >> } > > so the "level" parameter is the index in the frequency table, right? > >> diff --git a/drivers/thermal/exynos_thermal.c b/drivers/thermal/exynos_thermal.c >> index 224751e..fa9e1d7 100644 >> --- a/drivers/thermal/exynos_thermal.c >> +++ b/drivers/thermal/exynos_thermal.c >> @@ -233,7 +233,8 @@ 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, ret = -EINVAL; >> + unsigned int count = 0; >> struct cpufreq_frequency_table *table = NULL; >> #ifdef CONFIG_CPU_FREQ >> table = cpufreq_frequency_get_table(cpu); >> @@ -241,12 +242,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 count; >> + count++; >> } >> return ret; >> } > > but we ignore the invalid entry here. > > take the following cpufreq table for example, with your patch, > entry frequency > 0 2.4G > 1 invalid > 2 2G > 3 invalid > 4 1.6G > 5 end > > in exynos_get_frequency_level(), freq 1.6G is translated to level 2, > because count is increased only twice, for entry 0 and entry 2, right? Hi Rui, Gui fixes looks fine for infinite loop issue. After Gui's fixes 1.6G in translated to level 4 which is fine. Thanks, Amit Daniel > > but then, in get_cpu_frequency(), level 2 is translated to 2G HZ, which > I do not think is what we want. > > I think we are doing something wrong here, and here is a cleanup patch I made > to fix this issue, please review. > > From a868d68fdcd94a29ac9d3998283119a453decb4b Mon Sep 17 00:00:00 2001 > From: Zhang Rui <rui.zhang@intel.com> > Date: Fri, 1 Feb 2013 15:41:47 +0800 > Subject: [PATCH] Thermal cpu_cooling: fix inconsistent use of CPU frequency > table > > there are three kinds of entries in CPU frequency table. > 1. invalid entry, its frequency is CPUFREQ_ENTRY_INVALID. > 2. duplicate entry, two entry may share the same frequency, > And we should treat it as on valid entry. > 3. valid entry with a proper frequency. > > And when talking about cpufreq cooling device cooling state, > it should be the same thing, in both cpu_cooling.c and its users, > AKA, max_cooling_state of a cpufreq cooling device equals the > number of VALID entries (type #3 above) in CPU frequency table. > And when setting a cpufreq cooling device to state X, it means set > the cpufreq cooling device to Xth maximum frequency in the > cpufreq frequency table. > > This patch does a cleanup in both drivers/thermal/cpu_cooling.c > and drivers/thermal/exynos_thermal.c to make them be consistent > in using cpu level/cpufreq cooling state. > > Signed-off-by: Zhang Rui <rui.zhang@intel.com> > --- > drivers/thermal/cpu_cooling.c | 122 ++++++++++++++++++++++++-------------- > drivers/thermal/exynos_thermal.c | 17 +----- > include/linux/cpu_cooling.h | 12 ++++ > 3 files changed, 91 insertions(+), 60 deletions(-) > > diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c > index 836828e..c16795b 100644 > --- a/drivers/thermal/cpu_cooling.c > +++ b/drivers/thermal/cpu_cooling.c > @@ -116,6 +116,40 @@ static int is_cpufreq_valid(int cpu) > } > > /** > + * cpufreq_cooling_get_max_level - function to get max valid cpufreq levels > + * @cpu: cpu for which frequency is fetched. > + */ > +int cpufreq_cooling_get_max_level(unsigned int cpu) > +{ > + int i, level; > + unsigned int freq = CPUFREQ_ENTRY_INVALID; > + struct cpufreq_frequency_table *table = > + cpufreq_frequency_get_table(cpu); > + > + if (!table) > + return -EINVAL; > + > + for (i = 0, level = 0; table[i].frequency != CPUFREQ_TABLE_END; i++) { > + /* Invalid entry */ > + if (table[i].frequency == CPUFREQ_ENTRY_INVALID) > + continue; > + > + /* Duplicate entry */ > + if (freq == table[i].frequency) > + continue; > + > + /* First valid entry */ > + if (freq == CPUFREQ_ENTRY_INVALID) > + freq = table[i].frequency; > + > + level++; > + } > + > + return level; > +} > +EXPORT_SYMBOL(cpufreq_cooling_get_max_level); > + > +/** > * get_cpu_frequency - get the absolute value of frequency from level. > * @cpu: cpu for which frequency is fetched. > * @level: level of frequency of the CPU > @@ -123,46 +157,58 @@ 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; > + int i, count; > + int max_level; > + unsigned int freq = CPUFREQ_ENTRY_INVALID; > bool descend = false; > struct cpufreq_frequency_table *table = > cpufreq_frequency_get_table(cpu); > - if (!table) > - return ret; > > - while (table[i].frequency != CPUFREQ_TABLE_END) { > + max_level = cpufreq_cooling_get_max_level(cpu); > + > + if (max_level < 0) > + return max_level; > + > + if (level > max_level) > + return -EINVAL; > + > + for (i = 0; table[i].frequency != CPUFREQ_TABLE_END; i++) { > if (table[i].frequency == CPUFREQ_ENTRY_INVALID) > continue; > > - /*check if table in ascending or descending order*/ > - if ((table[i + 1].frequency != CPUFREQ_TABLE_END) && > - (table[i + 1].frequency < table[i].frequency) > - && !descend) { > - descend = true; > + if (freq == CPUFREQ_ENTRY_INVALID) > + /* first valid entry */ > + freq = table[i].frequency; > + else if (freq == table[i].frequency) > + /* duplicate entry */ > + continue; > + else { > + /* two valid entries, check frequency order */ > + descend = !!(freq > table[i].frequency); > + break; > } > - > - /*return if level matched and table in descending order*/ > - if (descend && i == level) > - return table[i].frequency; > - i++; > } > - i--; > > - if (level > i || descend) > - return ret; > - level_index = i - level; > + /* level equals "the index of valid entries" in cpufreq table */ > + level = descend ? level : max_level - level + 1; > > - /*Scan the table in reverse order and match the level*/ > - while (i >= 0) { > + freq = CPUFREQ_ENTRY_INVALID; > + for (i = 0, count = 0; table[i].frequency != CPUFREQ_TABLE_END; i++) { > + /* ignore invalid entry */ > if (table[i].frequency == CPUFREQ_ENTRY_INVALID) > continue; > - /*return if level matched*/ > - if (i == level_index) > + > + if (freq == CPUFREQ_ENTRY_INVALID) > + /* first valid entry */ > + freq = table[i].frequency; > + else if (freq == table[i].frequency) > + /* ignore duplicate entry */ > + continue; > + count++; > + if (level == count) > return table[i].frequency; > - i--; > } > - return ret; > + return -ENODEV; > } > > /** > @@ -244,29 +290,17 @@ static int cpufreq_get_max_state(struct thermal_cooling_device *cdev, > struct cpufreq_cooling_device *cpufreq_device = cdev->devdata; > struct cpumask *maskPtr = &cpufreq_device->allowed_cpus; > unsigned int cpu; > - struct cpufreq_frequency_table *table; > - unsigned long count = 0; > - int i = 0; > + int max_level; > > cpu = cpumask_any(maskPtr); > - table = cpufreq_frequency_get_table(cpu); > - if (!table) { > - *state = 0; > - return 0; > - } > - > - for (i = 0; (table[i].frequency != CPUFREQ_TABLE_END); i++) { > - if (table[i].frequency == CPUFREQ_ENTRY_INVALID) > - continue; > - count++; > - } > > - if (count > 0) { > - *state = --count; > - return 0; > - } > + max_level = cpufreq_cooling_get_max_level(cpu); > > - return -EINVAL; > + if (max_level < 0) > + return max_level; > + else > + *state = max_level; > + return 0; > } > > /** > diff --git a/drivers/thermal/exynos_thermal.c b/drivers/thermal/exynos_thermal.c > index cd71e24..5ac2de1 100644 > --- a/drivers/thermal/exynos_thermal.c > +++ b/drivers/thermal/exynos_thermal.c > @@ -241,22 +241,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; > - struct cpufreq_frequency_table *table = NULL; > -#ifdef CONFIG_CPU_FREQ > - table = cpufreq_frequency_get_table(cpu); > -#endif > - if (!table) > - return ret; > - > - while (table[i].frequency != CPUFREQ_TABLE_END) { > - if (table[i].frequency == CPUFREQ_ENTRY_INVALID) > - continue; > - if (table[i].frequency == freq) > - return i; > - i++; > - } > - return ret; > + return cpufreq_cooling_get_max_level(cpu); This is not correct as it always gives the highest level. > } > > /* Bind callback functions for thermal zone */ > diff --git a/include/linux/cpu_cooling.h b/include/linux/cpu_cooling.h > index 40b4ef5..2de9319 100644 > --- a/include/linux/cpu_cooling.h > +++ b/include/linux/cpu_cooling.h > @@ -42,6 +42,14 @@ struct thermal_cooling_device *cpufreq_cooling_register( > * @cdev: thermal cooling device pointer. > */ > void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev); > + > +/** > + * cpufreq_cooling_get_max_level - function to get maxinum cooling state > + * of a cpufreq cooling device > + * @cpu: cpu of which frequency is fetched. > + */ > +int cpufreq_cooling_get_max_level(unsigned int cpu); > + > #else /* !CONFIG_CPU_THERMAL */ > static inline struct thermal_cooling_device *cpufreq_cooling_register( > const struct cpumask *clip_cpus) > @@ -53,6 +61,10 @@ static inline void cpufreq_cooling_unregister( > { > return; > } > +static inline int cpufreq_cooling_get_max_level(unsigned int cpu) > +{ > + return -ENODEV; > +} > #endif /* CONFIG_CPU_THERMAL */ > > #endif /* __CPU_COOLING_H__ */ > -- > 1.7.9.5 > > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Thermal: fix iteration over CPU frequency list 2013-02-04 19:49 ` amit daniel kachhap @ 2013-02-05 9:59 ` Zhang Rui 2013-02-05 17:55 ` amit daniel kachhap 0 siblings, 1 reply; 6+ messages in thread From: Zhang Rui @ 2013-02-05 9:59 UTC (permalink / raw) To: amit daniel kachhap; +Cc: Gu1, linux-pm, linux-kernel On Mon, 2013-02-04 at 11:49 -0800, amit daniel kachhap wrote: > On Thu, Jan 31, 2013 at 11:59 PM, Zhang Rui <rui.zhang@intel.com> wrote: > > On Thu, 2013-01-24 at 16:24 +0100, Gu1 wrote: > >> In different places in the Thermal code, the CPU frequency list is iterated > >> in an incorrect way, leading to endless loops when the frequency list contains > >> a CPUFREQ_TABLE_INVALID entry, which is the case by default in the the Exynos > >> 4x12 cpufreq driver, for example. > >> > >> The frequency list is iterated with a while loop, and when a > >> CPUFREQ_TABLE_INVALID entry is encountered, the continue; statement is used to > >> skip it, but the index is not incremented, causing an endless loop. > >> > >> A similar bug was fixed by hongbo.zhang in commit: > >> Thermal: fix bug of counting cpu frequencies > >> > >> Signed-off-by: Gu1 <gu1@aeroxteam.fr> > >> --- > >> drivers/thermal/cpu_cooling.c | 8 +++----- > >> drivers/thermal/exynos_thermal.c | 9 +++++---- > >> 2 files changed, 8 insertions(+), 9 deletions(-) > >> > >> diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c > >> index 836828e..51acd26 100644 > >> --- a/drivers/thermal/cpu_cooling.c > >> +++ b/drivers/thermal/cpu_cooling.c > >> @@ -123,7 +123,7 @@ static int is_cpufreq_valid(int cpu) > >> */ > >> static unsigned int get_cpu_frequency(unsigned int cpu, unsigned long level) > >> { > >> - int ret = 0, i = 0; > >> + int ret = 0, i; > >> unsigned long level_index; > >> bool descend = false; > >> struct cpufreq_frequency_table *table = > >> @@ -131,7 +131,7 @@ static unsigned int get_cpu_frequency(unsigned int cpu, unsigned long level) > >> 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; > >> > >> @@ -145,7 +145,6 @@ 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) > >> return table[i].frequency; > >> - i++; > >> } > >> i--; > >> > >> @@ -154,13 +153,12 @@ static unsigned int get_cpu_frequency(unsigned int cpu, unsigned long level) > >> level_index = i - 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) > >> return table[i].frequency; > >> - i--; > >> } > >> return ret; > >> } > > > > so the "level" parameter is the index in the frequency table, right? > > > >> diff --git a/drivers/thermal/exynos_thermal.c b/drivers/thermal/exynos_thermal.c > >> index 224751e..fa9e1d7 100644 > >> --- a/drivers/thermal/exynos_thermal.c > >> +++ b/drivers/thermal/exynos_thermal.c > >> @@ -233,7 +233,8 @@ 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, ret = -EINVAL; > >> + unsigned int count = 0; > >> struct cpufreq_frequency_table *table = NULL; > >> #ifdef CONFIG_CPU_FREQ > >> table = cpufreq_frequency_get_table(cpu); > >> @@ -241,12 +242,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 count; > >> + count++; > >> } > >> return ret; > >> } > > > > but we ignore the invalid entry here. > > > > take the following cpufreq table for example, with your patch, > > entry frequency > > 0 2.4G > > 1 invalid > > 2 2G > > 3 invalid > > 4 1.6G > > 5 end > > > > in exynos_get_frequency_level(), freq 1.6G is translated to level 2, > > because count is increased only twice, for entry 0 and entry 2, right? > > Hi Rui, > > Gui fixes looks fine for infinite loop issue. After Gui's fixes 1.6G > in translated to level 4 which is fine. > > Thanks, > Amit Daniel > > > > > but then, in get_cpu_frequency(), level 2 is translated to 2G HZ, which > > I do not think is what we want. > > > > I think we are doing something wrong here, and here is a cleanup patch I made > > to fix this issue, please review. > > > > From a868d68fdcd94a29ac9d3998283119a453decb4b Mon Sep 17 00:00:00 2001 > > From: Zhang Rui <rui.zhang@intel.com> > > Date: Fri, 1 Feb 2013 15:41:47 +0800 > > Subject: [PATCH] Thermal cpu_cooling: fix inconsistent use of CPU frequency > > table > > > > there are three kinds of entries in CPU frequency table. > > 1. invalid entry, its frequency is CPUFREQ_ENTRY_INVALID. > > 2. duplicate entry, two entry may share the same frequency, > > And we should treat it as on valid entry. > > 3. valid entry with a proper frequency. > > > > And when talking about cpufreq cooling device cooling state, > > it should be the same thing, in both cpu_cooling.c and its users, > > AKA, max_cooling_state of a cpufreq cooling device equals the > > number of VALID entries (type #3 above) in CPU frequency table. > > And when setting a cpufreq cooling device to state X, it means set > > the cpufreq cooling device to Xth maximum frequency in the > > cpufreq frequency table. > > > > This patch does a cleanup in both drivers/thermal/cpu_cooling.c > > and drivers/thermal/exynos_thermal.c to make them be consistent > > in using cpu level/cpufreq cooling state. > > > > Signed-off-by: Zhang Rui <rui.zhang@intel.com> > > --- > > drivers/thermal/cpu_cooling.c | 122 ++++++++++++++++++++++++-------------- > > drivers/thermal/exynos_thermal.c | 17 +----- > > include/linux/cpu_cooling.h | 12 ++++ > > 3 files changed, 91 insertions(+), 60 deletions(-) > > > > diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c > > index 836828e..c16795b 100644 > > --- a/drivers/thermal/cpu_cooling.c > > +++ b/drivers/thermal/cpu_cooling.c > > @@ -116,6 +116,40 @@ static int is_cpufreq_valid(int cpu) > > } > > > > /** > > + * cpufreq_cooling_get_max_level - function to get max valid cpufreq levels > > + * @cpu: cpu for which frequency is fetched. > > + */ > > +int cpufreq_cooling_get_max_level(unsigned int cpu) > > +{ > > + int i, level; > > + unsigned int freq = CPUFREQ_ENTRY_INVALID; > > + struct cpufreq_frequency_table *table = > > + cpufreq_frequency_get_table(cpu); > > + > > + if (!table) > > + return -EINVAL; > > + > > + for (i = 0, level = 0; table[i].frequency != CPUFREQ_TABLE_END; i++) { > > + /* Invalid entry */ > > + if (table[i].frequency == CPUFREQ_ENTRY_INVALID) > > + continue; > > + > > + /* Duplicate entry */ > > + if (freq == table[i].frequency) > > + continue; > > + > > + /* First valid entry */ > > + if (freq == CPUFREQ_ENTRY_INVALID) > > + freq = table[i].frequency; > > + > > + level++; > > + } > > + > > + return level; > > +} > > +EXPORT_SYMBOL(cpufreq_cooling_get_max_level); > > + > > +/** > > * get_cpu_frequency - get the absolute value of frequency from level. > > * @cpu: cpu for which frequency is fetched. > > * @level: level of frequency of the CPU I have a question about this comment. it says that. * @level: level of frequency of the CPU * e.g level=1 --> 1st MAX FREQ, LEVEL=2 ---> 2nd MAX FREQ, ....etc" I think this is wrong because level equals cooling state, which starts from 0, rather than 1. IMO, it should be "level (cooling state) = 0 --> 1st MAX FREQ". what do you think? > > @@ -123,46 +157,58 @@ 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; > > + int i, count; > > + int max_level; > > + unsigned int freq = CPUFREQ_ENTRY_INVALID; > > bool descend = false; > > struct cpufreq_frequency_table *table = > > cpufreq_frequency_get_table(cpu); > > - if (!table) > > - return ret; > > > > - while (table[i].frequency != CPUFREQ_TABLE_END) { > > + max_level = cpufreq_cooling_get_max_level(cpu); > > + > > + if (max_level < 0) > > + return max_level; > > + > > + if (level > max_level) > > + return -EINVAL; > > + > > + for (i = 0; table[i].frequency != CPUFREQ_TABLE_END; i++) { > > if (table[i].frequency == CPUFREQ_ENTRY_INVALID) > > continue; > > > > - /*check if table in ascending or descending order*/ > > - if ((table[i + 1].frequency != CPUFREQ_TABLE_END) && > > - (table[i + 1].frequency < table[i].frequency) > > - && !descend) { > > - descend = true; > > + if (freq == CPUFREQ_ENTRY_INVALID) > > + /* first valid entry */ > > + freq = table[i].frequency; > > + else if (freq == table[i].frequency) > > + /* duplicate entry */ > > + continue; > > + else { > > + /* two valid entries, check frequency order */ > > + descend = !!(freq > table[i].frequency); > > + break; > > } > > - > > - /*return if level matched and table in descending order*/ > > - if (descend && i == level) > > - return table[i].frequency; > > - i++; > > } > > - i--; > > > > - if (level > i || descend) > > - return ret; > > - level_index = i - level; > > + /* level equals "the index of valid entries" in cpufreq table */ > > + level = descend ? level : max_level - level + 1; > > > > - /*Scan the table in reverse order and match the level*/ > > - while (i >= 0) { > > + freq = CPUFREQ_ENTRY_INVALID; > > + for (i = 0, count = 0; table[i].frequency != CPUFREQ_TABLE_END; i++) { > > + /* ignore invalid entry */ > > if (table[i].frequency == CPUFREQ_ENTRY_INVALID) > > continue; > > - /*return if level matched*/ > > - if (i == level_index) > > + > > + if (freq == CPUFREQ_ENTRY_INVALID) > > + /* first valid entry */ > > + freq = table[i].frequency; > > + else if (freq == table[i].frequency) > > + /* ignore duplicate entry */ > > + continue; > > + count++; > > + if (level == count) > > return table[i].frequency; > > - i--; > > } > > - return ret; > > + return -ENODEV; > > } > > > > /** > > @@ -244,29 +290,17 @@ static int cpufreq_get_max_state(struct thermal_cooling_device *cdev, > > struct cpufreq_cooling_device *cpufreq_device = cdev->devdata; > > struct cpumask *maskPtr = &cpufreq_device->allowed_cpus; > > unsigned int cpu; > > - struct cpufreq_frequency_table *table; > > - unsigned long count = 0; > > - int i = 0; > > + int max_level; > > > > cpu = cpumask_any(maskPtr); > > - table = cpufreq_frequency_get_table(cpu); > > - if (!table) { > > - *state = 0; > > - return 0; > > - } > > - > > - for (i = 0; (table[i].frequency != CPUFREQ_TABLE_END); i++) { > > - if (table[i].frequency == CPUFREQ_ENTRY_INVALID) > > - continue; > > - count++; > > - } > > > > - if (count > 0) { > > - *state = --count; > > - return 0; > > - } > > + max_level = cpufreq_cooling_get_max_level(cpu); > > > > - return -EINVAL; > > + if (max_level < 0) > > + return max_level; > > + else > > + *state = max_level; > > + return 0; > > } > > > > /** > > diff --git a/drivers/thermal/exynos_thermal.c b/drivers/thermal/exynos_thermal.c > > index cd71e24..5ac2de1 100644 > > --- a/drivers/thermal/exynos_thermal.c > > +++ b/drivers/thermal/exynos_thermal.c > > @@ -241,22 +241,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; > > - struct cpufreq_frequency_table *table = NULL; > > -#ifdef CONFIG_CPU_FREQ > > - table = cpufreq_frequency_get_table(cpu); > > -#endif > > - if (!table) > > - return ret; > > - > > - while (table[i].frequency != CPUFREQ_TABLE_END) { > > - if (table[i].frequency == CPUFREQ_ENTRY_INVALID) > > - continue; > > - if (table[i].frequency == freq) > > - return i; > > - i++; > > - } > > - return ret; > > + return cpufreq_cooling_get_max_level(cpu); > This is not correct as it always gives the highest level. you are right. I'm proposing another patch. thanks, rui > > } > > > > /* Bind callback functions for thermal zone */ > > diff --git a/include/linux/cpu_cooling.h b/include/linux/cpu_cooling.h > > index 40b4ef5..2de9319 100644 > > --- a/include/linux/cpu_cooling.h > > +++ b/include/linux/cpu_cooling.h > > @@ -42,6 +42,14 @@ struct thermal_cooling_device *cpufreq_cooling_register( > > * @cdev: thermal cooling device pointer. > > */ > > void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev); > > + > > +/** > > + * cpufreq_cooling_get_max_level - function to get maxinum cooling state > > + * of a cpufreq cooling device > > + * @cpu: cpu of which frequency is fetched. > > + */ > > +int cpufreq_cooling_get_max_level(unsigned int cpu); > > + > > #else /* !CONFIG_CPU_THERMAL */ > > static inline struct thermal_cooling_device *cpufreq_cooling_register( > > const struct cpumask *clip_cpus) > > @@ -53,6 +61,10 @@ static inline void cpufreq_cooling_unregister( > > { > > return; > > } > > +static inline int cpufreq_cooling_get_max_level(unsigned int cpu) > > +{ > > + return -ENODEV; > > +} > > #endif /* CONFIG_CPU_THERMAL */ > > > > #endif /* __CPU_COOLING_H__ */ > > -- > > 1.7.9.5 > > > > > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > Please read the FAQ at http://www.tux.org/lkml/ ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Thermal: fix iteration over CPU frequency list 2013-02-05 9:59 ` Zhang Rui @ 2013-02-05 17:55 ` amit daniel kachhap 0 siblings, 0 replies; 6+ messages in thread From: amit daniel kachhap @ 2013-02-05 17:55 UTC (permalink / raw) To: Zhang Rui; +Cc: Gu1, linux-pm, linux-kernel On Tue, Feb 5, 2013 at 1:59 AM, Zhang Rui <rui.zhang@intel.com> wrote: > On Mon, 2013-02-04 at 11:49 -0800, amit daniel kachhap wrote: >> On Thu, Jan 31, 2013 at 11:59 PM, Zhang Rui <rui.zhang@intel.com> wrote: >> > On Thu, 2013-01-24 at 16:24 +0100, Gu1 wrote: >> >> In different places in the Thermal code, the CPU frequency list is iterated >> >> in an incorrect way, leading to endless loops when the frequency list contains >> >> a CPUFREQ_TABLE_INVALID entry, which is the case by default in the the Exynos >> >> 4x12 cpufreq driver, for example. >> >> >> >> The frequency list is iterated with a while loop, and when a >> >> CPUFREQ_TABLE_INVALID entry is encountered, the continue; statement is used to >> >> skip it, but the index is not incremented, causing an endless loop. >> >> >> >> A similar bug was fixed by hongbo.zhang in commit: >> >> Thermal: fix bug of counting cpu frequencies >> >> >> >> Signed-off-by: Gu1 <gu1@aeroxteam.fr> >> >> --- >> >> drivers/thermal/cpu_cooling.c | 8 +++----- >> >> drivers/thermal/exynos_thermal.c | 9 +++++---- >> >> 2 files changed, 8 insertions(+), 9 deletions(-) >> >> >> >> diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c >> >> index 836828e..51acd26 100644 >> >> --- a/drivers/thermal/cpu_cooling.c >> >> +++ b/drivers/thermal/cpu_cooling.c >> >> @@ -123,7 +123,7 @@ static int is_cpufreq_valid(int cpu) >> >> */ >> >> static unsigned int get_cpu_frequency(unsigned int cpu, unsigned long level) >> >> { >> >> - int ret = 0, i = 0; >> >> + int ret = 0, i; >> >> unsigned long level_index; >> >> bool descend = false; >> >> struct cpufreq_frequency_table *table = >> >> @@ -131,7 +131,7 @@ static unsigned int get_cpu_frequency(unsigned int cpu, unsigned long level) >> >> 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; >> >> >> >> @@ -145,7 +145,6 @@ 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) >> >> return table[i].frequency; >> >> - i++; >> >> } >> >> i--; >> >> >> >> @@ -154,13 +153,12 @@ static unsigned int get_cpu_frequency(unsigned int cpu, unsigned long level) >> >> level_index = i - 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) >> >> return table[i].frequency; >> >> - i--; >> >> } >> >> return ret; >> >> } >> > >> > so the "level" parameter is the index in the frequency table, right? >> > >> >> diff --git a/drivers/thermal/exynos_thermal.c b/drivers/thermal/exynos_thermal.c >> >> index 224751e..fa9e1d7 100644 >> >> --- a/drivers/thermal/exynos_thermal.c >> >> +++ b/drivers/thermal/exynos_thermal.c >> >> @@ -233,7 +233,8 @@ 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, ret = -EINVAL; >> >> + unsigned int count = 0; >> >> struct cpufreq_frequency_table *table = NULL; >> >> #ifdef CONFIG_CPU_FREQ >> >> table = cpufreq_frequency_get_table(cpu); >> >> @@ -241,12 +242,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 count; >> >> + count++; >> >> } >> >> return ret; >> >> } >> > >> > but we ignore the invalid entry here. >> > >> > take the following cpufreq table for example, with your patch, >> > entry frequency >> > 0 2.4G >> > 1 invalid >> > 2 2G >> > 3 invalid >> > 4 1.6G >> > 5 end >> > >> > in exynos_get_frequency_level(), freq 1.6G is translated to level 2, >> > because count is increased only twice, for entry 0 and entry 2, right? >> >> Hi Rui, >> >> Gui fixes looks fine for infinite loop issue. After Gui's fixes 1.6G >> in translated to level 4 which is fine. >> >> Thanks, >> Amit Daniel >> >> > >> > but then, in get_cpu_frequency(), level 2 is translated to 2G HZ, which >> > I do not think is what we want. >> > >> > I think we are doing something wrong here, and here is a cleanup patch I made >> > to fix this issue, please review. >> > >> > From a868d68fdcd94a29ac9d3998283119a453decb4b Mon Sep 17 00:00:00 2001 >> > From: Zhang Rui <rui.zhang@intel.com> >> > Date: Fri, 1 Feb 2013 15:41:47 +0800 >> > Subject: [PATCH] Thermal cpu_cooling: fix inconsistent use of CPU frequency >> > table >> > >> > there are three kinds of entries in CPU frequency table. >> > 1. invalid entry, its frequency is CPUFREQ_ENTRY_INVALID. >> > 2. duplicate entry, two entry may share the same frequency, >> > And we should treat it as on valid entry. >> > 3. valid entry with a proper frequency. >> > >> > And when talking about cpufreq cooling device cooling state, >> > it should be the same thing, in both cpu_cooling.c and its users, >> > AKA, max_cooling_state of a cpufreq cooling device equals the >> > number of VALID entries (type #3 above) in CPU frequency table. >> > And when setting a cpufreq cooling device to state X, it means set >> > the cpufreq cooling device to Xth maximum frequency in the >> > cpufreq frequency table. >> > >> > This patch does a cleanup in both drivers/thermal/cpu_cooling.c >> > and drivers/thermal/exynos_thermal.c to make them be consistent >> > in using cpu level/cpufreq cooling state. >> > >> > Signed-off-by: Zhang Rui <rui.zhang@intel.com> >> > --- >> > drivers/thermal/cpu_cooling.c | 122 ++++++++++++++++++++++++-------------- >> > drivers/thermal/exynos_thermal.c | 17 +----- >> > include/linux/cpu_cooling.h | 12 ++++ >> > 3 files changed, 91 insertions(+), 60 deletions(-) >> > >> > diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c >> > index 836828e..c16795b 100644 >> > --- a/drivers/thermal/cpu_cooling.c >> > +++ b/drivers/thermal/cpu_cooling.c >> > @@ -116,6 +116,40 @@ static int is_cpufreq_valid(int cpu) >> > } >> > >> > /** >> > + * cpufreq_cooling_get_max_level - function to get max valid cpufreq levels >> > + * @cpu: cpu for which frequency is fetched. >> > + */ >> > +int cpufreq_cooling_get_max_level(unsigned int cpu) >> > +{ >> > + int i, level; >> > + unsigned int freq = CPUFREQ_ENTRY_INVALID; >> > + struct cpufreq_frequency_table *table = >> > + cpufreq_frequency_get_table(cpu); >> > + >> > + if (!table) >> > + return -EINVAL; >> > + >> > + for (i = 0, level = 0; table[i].frequency != CPUFREQ_TABLE_END; i++) { >> > + /* Invalid entry */ >> > + if (table[i].frequency == CPUFREQ_ENTRY_INVALID) >> > + continue; >> > + >> > + /* Duplicate entry */ >> > + if (freq == table[i].frequency) >> > + continue; >> > + >> > + /* First valid entry */ >> > + if (freq == CPUFREQ_ENTRY_INVALID) >> > + freq = table[i].frequency; >> > + >> > + level++; >> > + } >> > + >> > + return level; >> > +} >> > +EXPORT_SYMBOL(cpufreq_cooling_get_max_level); >> > + >> > +/** >> > * get_cpu_frequency - get the absolute value of frequency from level. >> > * @cpu: cpu for which frequency is fetched. >> > * @level: level of frequency of the CPU > > I have a question about this comment. > it says that. > * @level: level of frequency of the CPU > * e.g level=1 --> 1st MAX FREQ, LEVEL=2 ---> 2nd MAX > FREQ, ....etc" > I think this is wrong because level equals cooling state, which starts > from 0, rather than 1. > IMO, it should be "level (cooling state) = 0 --> 1st MAX FREQ". > what do you think? Yes the comments are not correct. level 0 means 1st max freq. Thanks, Amit Daniel > >> > @@ -123,46 +157,58 @@ 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; >> > + int i, count; >> > + int max_level; >> > + unsigned int freq = CPUFREQ_ENTRY_INVALID; >> > bool descend = false; >> > struct cpufreq_frequency_table *table = >> > cpufreq_frequency_get_table(cpu); >> > - if (!table) >> > - return ret; >> > >> > - while (table[i].frequency != CPUFREQ_TABLE_END) { >> > + max_level = cpufreq_cooling_get_max_level(cpu); >> > + >> > + if (max_level < 0) >> > + return max_level; >> > + >> > + if (level > max_level) >> > + return -EINVAL; >> > + >> > + for (i = 0; table[i].frequency != CPUFREQ_TABLE_END; i++) { >> > if (table[i].frequency == CPUFREQ_ENTRY_INVALID) >> > continue; >> > >> > - /*check if table in ascending or descending order*/ >> > - if ((table[i + 1].frequency != CPUFREQ_TABLE_END) && >> > - (table[i + 1].frequency < table[i].frequency) >> > - && !descend) { >> > - descend = true; >> > + if (freq == CPUFREQ_ENTRY_INVALID) >> > + /* first valid entry */ >> > + freq = table[i].frequency; >> > + else if (freq == table[i].frequency) >> > + /* duplicate entry */ >> > + continue; >> > + else { >> > + /* two valid entries, check frequency order */ >> > + descend = !!(freq > table[i].frequency); >> > + break; >> > } >> > - >> > - /*return if level matched and table in descending order*/ >> > - if (descend && i == level) >> > - return table[i].frequency; >> > - i++; >> > } >> > - i--; >> > >> > - if (level > i || descend) >> > - return ret; >> > - level_index = i - level; >> > + /* level equals "the index of valid entries" in cpufreq table */ >> > + level = descend ? level : max_level - level + 1; >> > >> > - /*Scan the table in reverse order and match the level*/ >> > - while (i >= 0) { >> > + freq = CPUFREQ_ENTRY_INVALID; >> > + for (i = 0, count = 0; table[i].frequency != CPUFREQ_TABLE_END; i++) { >> > + /* ignore invalid entry */ >> > if (table[i].frequency == CPUFREQ_ENTRY_INVALID) >> > continue; >> > - /*return if level matched*/ >> > - if (i == level_index) >> > + >> > + if (freq == CPUFREQ_ENTRY_INVALID) >> > + /* first valid entry */ >> > + freq = table[i].frequency; >> > + else if (freq == table[i].frequency) >> > + /* ignore duplicate entry */ >> > + continue; >> > + count++; >> > + if (level == count) >> > return table[i].frequency; >> > - i--; >> > } >> > - return ret; >> > + return -ENODEV; >> > } >> > >> > /** >> > @@ -244,29 +290,17 @@ static int cpufreq_get_max_state(struct thermal_cooling_device *cdev, >> > struct cpufreq_cooling_device *cpufreq_device = cdev->devdata; >> > struct cpumask *maskPtr = &cpufreq_device->allowed_cpus; >> > unsigned int cpu; >> > - struct cpufreq_frequency_table *table; >> > - unsigned long count = 0; >> > - int i = 0; >> > + int max_level; >> > >> > cpu = cpumask_any(maskPtr); >> > - table = cpufreq_frequency_get_table(cpu); >> > - if (!table) { >> > - *state = 0; >> > - return 0; >> > - } >> > - >> > - for (i = 0; (table[i].frequency != CPUFREQ_TABLE_END); i++) { >> > - if (table[i].frequency == CPUFREQ_ENTRY_INVALID) >> > - continue; >> > - count++; >> > - } >> > >> > - if (count > 0) { >> > - *state = --count; >> > - return 0; >> > - } >> > + max_level = cpufreq_cooling_get_max_level(cpu); >> > >> > - return -EINVAL; >> > + if (max_level < 0) >> > + return max_level; >> > + else >> > + *state = max_level; >> > + return 0; >> > } >> > >> > /** >> > diff --git a/drivers/thermal/exynos_thermal.c b/drivers/thermal/exynos_thermal.c >> > index cd71e24..5ac2de1 100644 >> > --- a/drivers/thermal/exynos_thermal.c >> > +++ b/drivers/thermal/exynos_thermal.c >> > @@ -241,22 +241,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; >> > - struct cpufreq_frequency_table *table = NULL; >> > -#ifdef CONFIG_CPU_FREQ >> > - table = cpufreq_frequency_get_table(cpu); >> > -#endif >> > - if (!table) >> > - return ret; >> > - >> > - while (table[i].frequency != CPUFREQ_TABLE_END) { >> > - if (table[i].frequency == CPUFREQ_ENTRY_INVALID) >> > - continue; >> > - if (table[i].frequency == freq) >> > - return i; >> > - i++; >> > - } >> > - return ret; >> > + return cpufreq_cooling_get_max_level(cpu); >> This is not correct as it always gives the highest level. > > you are right. I'm proposing another patch. > > thanks, > rui >> > } >> > >> > /* Bind callback functions for thermal zone */ >> > diff --git a/include/linux/cpu_cooling.h b/include/linux/cpu_cooling.h >> > index 40b4ef5..2de9319 100644 >> > --- a/include/linux/cpu_cooling.h >> > +++ b/include/linux/cpu_cooling.h >> > @@ -42,6 +42,14 @@ struct thermal_cooling_device *cpufreq_cooling_register( >> > * @cdev: thermal cooling device pointer. >> > */ >> > void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev); >> > + >> > +/** >> > + * cpufreq_cooling_get_max_level - function to get maxinum cooling state >> > + * of a cpufreq cooling device >> > + * @cpu: cpu of which frequency is fetched. >> > + */ >> > +int cpufreq_cooling_get_max_level(unsigned int cpu); >> > + >> > #else /* !CONFIG_CPU_THERMAL */ >> > static inline struct thermal_cooling_device *cpufreq_cooling_register( >> > const struct cpumask *clip_cpus) >> > @@ -53,6 +61,10 @@ static inline void cpufreq_cooling_unregister( >> > { >> > return; >> > } >> > +static inline int cpufreq_cooling_get_max_level(unsigned int cpu) >> > +{ >> > + return -ENODEV; >> > +} >> > #endif /* CONFIG_CPU_THERMAL */ >> > >> > #endif /* __CPU_COOLING_H__ */ >> > -- >> > 1.7.9.5 >> > >> > >> > >> > -- >> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in >> > the body of a message to majordomo@vger.kernel.org >> > More majordomo info at http://vger.kernel.org/majordomo-info.html >> > Please read the FAQ at http://www.tux.org/lkml/ > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Thermal: fix iteration over CPU frequency list 2013-01-24 15:24 [PATCH] Thermal: fix iteration over CPU frequency list Gu1 2013-02-01 7:59 ` Zhang Rui @ 2013-02-04 19:51 ` amit kachhap 1 sibling, 0 replies; 6+ messages in thread From: amit kachhap @ 2013-02-04 19:51 UTC (permalink / raw) To: Gu1, Zhang Rui; +Cc: linux-pm, linux-kernel On Thu, Jan 24, 2013 at 7:24 AM, Gu1 <gu1@aeroxteam.fr> wrote: > In different places in the Thermal code, the CPU frequency list is iterated > in an incorrect way, leading to endless loops when the frequency list contains > a CPUFREQ_TABLE_INVALID entry, which is the case by default in the the Exynos > 4x12 cpufreq driver, for example. > > The frequency list is iterated with a while loop, and when a > CPUFREQ_TABLE_INVALID entry is encountered, the continue; statement is used to > skip it, but the index is not incremented, causing an endless loop. > > A similar bug was fixed by hongbo.zhang in commit: > Thermal: fix bug of counting cpu frequencies > > Signed-off-by: Gu1 <gu1@aeroxteam.fr> This changes looks fine. Reviewed-by: Amit Daniel Kachhap <amit.daniel@samsung.com> Thanks, Amit Daniel > --- > drivers/thermal/cpu_cooling.c | 8 +++----- > drivers/thermal/exynos_thermal.c | 9 +++++---- > 2 files changed, 8 insertions(+), 9 deletions(-) > > diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c > index 836828e..51acd26 100644 > --- a/drivers/thermal/cpu_cooling.c > +++ b/drivers/thermal/cpu_cooling.c > @@ -123,7 +123,7 @@ static int is_cpufreq_valid(int cpu) > */ > static unsigned int get_cpu_frequency(unsigned int cpu, unsigned long level) > { > - int ret = 0, i = 0; > + int ret = 0, i; > unsigned long level_index; > bool descend = false; > struct cpufreq_frequency_table *table = > @@ -131,7 +131,7 @@ static unsigned int get_cpu_frequency(unsigned int cpu, unsigned long level) > 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; > > @@ -145,7 +145,6 @@ 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) > return table[i].frequency; > - i++; > } > i--; > > @@ -154,13 +153,12 @@ static unsigned int get_cpu_frequency(unsigned int cpu, unsigned long level) > level_index = i - 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) > return table[i].frequency; > - i--; > } > return ret; > } > diff --git a/drivers/thermal/exynos_thermal.c b/drivers/thermal/exynos_thermal.c > index 224751e..fa9e1d7 100644 > --- a/drivers/thermal/exynos_thermal.c > +++ b/drivers/thermal/exynos_thermal.c > @@ -233,7 +233,8 @@ 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, ret = -EINVAL; > + unsigned int count = 0; > struct cpufreq_frequency_table *table = NULL; > #ifdef CONFIG_CPU_FREQ > table = cpufreq_frequency_get_table(cpu); > @@ -241,12 +242,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 count; > + count++; > } > return ret; > } > -- > 1.8.1.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-02-05 17:55 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-01-24 15:24 [PATCH] Thermal: fix iteration over CPU frequency list Gu1 2013-02-01 7:59 ` Zhang Rui 2013-02-04 19:49 ` amit daniel kachhap 2013-02-05 9:59 ` Zhang Rui 2013-02-05 17:55 ` amit daniel kachhap 2013-02-04 19:51 ` amit kachhap
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).