On Thu, Jan 31, 2013 at 11:59 PM, Zhang Rui <rui.zh...@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 <g...@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.zh...@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.zh...@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 majord...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
