Re: [PATCH] Thermal: fix iteration over CPU frequency list

[Zhang Rui <rui.zhang@intel.com>]

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