[PATCH V3 0/9] cpufreq: Sort policy->freq_table

[PATCH V3 0/9] cpufreq: Sort policy->freq_table

[Viresh Kumar]

Hi Rafael,

So all my patches are contained in two series. The first one is:

[PATCH V3 0/8] cpufreq: cleanups and reorganization

which I have sent this morning. It does some cleanup and shall be
applied regardless of this series.

This series improves the performance of cpufreq_frequency_table_target()
routine by storing the policy->freq_table sorted in ascending order. On
one hand it shall improve the performance of current governors for
drivers providing freq-table to cpufreq core and on another hand this
API can be used directly from schedutil governor as well.

As Steve has requested earlier, these APIs are moved to a .h file to
avoid function calls.

Steve's series can use this API now without any performance lag.

The first 7 patches makes sure that current drivers wouldn't break because
of reordering of policy->freq_table and the 8th patch updates cpufreq
core to sort policy->freq_table and make
cpufreq_frequency_table_target() much more efficient.

This series is tested on ARM exynos platform and all the frequencies are
switching properly with both ondemand and schedutil governor.

V2->V3:
- Combined two series as you suggested
- Moved the new APIs and cpufreq_frequency_table_target() to a .h

All the patches are pushed here for testing in case anyone wants to try:

git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git cpufreq/sorted-freq-table

Thanks

--
viresh

Viresh Kumar (9):
  cpufreq: Use policy->freq_table in ->target_index()
  cpufreq: blackfin: Use 'index' only to index into policy->freq_table
  cpufreq: elanfreq: Use 'index' only to index into policy->freq_table
  cpufreq: exynos: Use 'index' only to index into policy->freq_table
  cpufreq: ia64: Use 'index' only to index into policy->freq_table
  cpufreq: imx: Use 'index' only to index into policy->freq_table
  cpufreq: maple: Use 'index' only to index into policy->freq_table
  cpufreq: Keep policy->freq_table sorted in ascending order
  cpufreq: drivers: Free frequency tables after being used

 MAINTAINERS                            |   1 +
 drivers/cpufreq/acpi-cpufreq.c         |  23 ++---
 drivers/cpufreq/arm_big_little.c       |   2 +-
 drivers/cpufreq/at32ap-cpufreq.c       |   8 +-
 drivers/cpufreq/blackfin-cpufreq.c     |  17 +++-
 drivers/cpufreq/cpufreq-dt.c           |   9 +-
 drivers/cpufreq/cpufreq.c              |  20 ++--
 drivers/cpufreq/cpufreq_ondemand.h     |   1 +
 drivers/cpufreq/cris-artpec3-cpufreq.c |   2 +-
 drivers/cpufreq/cris-etraxfs-cpufreq.c |   2 +-
 drivers/cpufreq/dbx500-cpufreq.c       |   3 +-
 drivers/cpufreq/e_powersaver.c         |  26 +++---
 drivers/cpufreq/elanfreq.c             |   8 +-
 drivers/cpufreq/exynos5440-cpufreq.c   |  13 ++-
 drivers/cpufreq/freq_table.c           | 163 +++++++++++++--------------------
 drivers/cpufreq/ia64-acpi-cpufreq.c    |  16 +++-
 drivers/cpufreq/imx6q-cpufreq.c        |  13 ++-
 drivers/cpufreq/kirkwood-cpufreq.c     |   2 +-
 drivers/cpufreq/loongson1-cpufreq.c    |  10 +-
 drivers/cpufreq/loongson2_cpufreq.c    |   5 +-
 drivers/cpufreq/maple-cpufreq.c        |   6 ++
 drivers/cpufreq/powernv-cpufreq.c      |   1 +
 drivers/cpufreq/s3c24xx-cpufreq.c      |   1 +
 drivers/cpufreq/s5pv210-cpufreq.c      |   1 +
 include/linux/cpufreq.h                |   3 -
 include/linux/cpufreq_table.h          | 139 ++++++++++++++++++++++++++++
 26 files changed, 317 insertions(+), 178 deletions(-)
 create mode 100644 include/linux/cpufreq_table.h

-- 
2.7.1.410.g6faf27b

[PATCH V3 1/9] cpufreq: Use policy->freq_table in ->target_index()

[Viresh Kumar]

The 'policy' already contains a pointer to the freq table, use that
instead of using driver specific tables name.

This is done in order to make sure that the 'index' passed to the
->target_index() callback is used *only* to index into the
policy->freq_table and nothing else.

Later patches would make changes in cpufreq core, after which
policy->freq_table may be reordered by cpufreq core and it wouldn't be
safe anymore to use 'index' for any other local array.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/arm_big_little.c       | 2 +-
 drivers/cpufreq/at32ap-cpufreq.c       | 2 +-
 drivers/cpufreq/blackfin-cpufreq.c     | 2 +-
 drivers/cpufreq/cris-artpec3-cpufreq.c | 2 +-
 drivers/cpufreq/cris-etraxfs-cpufreq.c | 2 +-
 drivers/cpufreq/dbx500-cpufreq.c       | 3 ++-
 drivers/cpufreq/e_powersaver.c         | 2 +-
 drivers/cpufreq/exynos5440-cpufreq.c   | 3 +--
 drivers/cpufreq/imx6q-cpufreq.c        | 2 +-
 drivers/cpufreq/kirkwood-cpufreq.c     | 2 +-
 drivers/cpufreq/loongson2_cpufreq.c    | 5 ++---
 11 files changed, 13 insertions(+), 14 deletions(-)

diff --git a/drivers/cpufreq/arm_big_little.c b/drivers/cpufreq/arm_big_little.c
index 418042201e6d..fc9e863e39d6 100644
--- a/drivers/cpufreq/arm_big_little.c
+++ b/drivers/cpufreq/arm_big_little.c
@@ -217,7 +217,7 @@ static int bL_cpufreq_set_target(struct cpufreq_policy *policy,
 	cur_cluster = cpu_to_cluster(cpu);
 	new_cluster = actual_cluster = per_cpu(physical_cluster, cpu);
 
-	freqs_new = freq_table[cur_cluster][index].frequency;
+	freqs_new = policy->freq_table[index].frequency;
 
 	if (is_bL_switching_enabled()) {
 		if ((actual_cluster == A15_CLUSTER) &&
diff --git a/drivers/cpufreq/at32ap-cpufreq.c b/drivers/cpufreq/at32ap-cpufreq.c
index 7b612c8bb09e..9231b1efb70d 100644
--- a/drivers/cpufreq/at32ap-cpufreq.c
+++ b/drivers/cpufreq/at32ap-cpufreq.c
@@ -31,7 +31,7 @@ static int at32_set_target(struct cpufreq_policy *policy, unsigned int index)
 	unsigned int old_freq, new_freq;
 
 	old_freq = policy->cur;
-	new_freq = freq_table[index].frequency;
+	new_freq = policy->freq_table[index].frequency;
 
 	if (!ref_freq) {
 		ref_freq = old_freq;
diff --git a/drivers/cpufreq/blackfin-cpufreq.c b/drivers/cpufreq/blackfin-cpufreq.c
index 12e97d8a9db0..1650c213f465 100644
--- a/drivers/cpufreq/blackfin-cpufreq.c
+++ b/drivers/cpufreq/blackfin-cpufreq.c
@@ -142,7 +142,7 @@ static int bfin_target(struct cpufreq_policy *policy, unsigned int index)
 #endif
 
 	old_freq = bfin_getfreq_khz(0);
-	new_freq = bfin_freq_table[index].frequency;
+	new_freq = policy->freq_table[index].frequency;
 
 #ifndef CONFIG_BF60x
 	plldiv = (bfin_read_PLL_DIV() & SSEL) | dpm_state_table[index].csel;
diff --git a/drivers/cpufreq/cris-artpec3-cpufreq.c b/drivers/cpufreq/cris-artpec3-cpufreq.c
index 601b88c490cf..4e7dc8f1e619 100644
--- a/drivers/cpufreq/cris-artpec3-cpufreq.c
+++ b/drivers/cpufreq/cris-artpec3-cpufreq.c
@@ -36,7 +36,7 @@ static int cris_freq_target(struct cpufreq_policy *policy, unsigned int state)
 
 	/* Even though we may be SMP they will share the same clock
 	 * so all settings are made on CPU0. */
-	if (cris_freq_table[state].frequency == 200000)
+	if (policy->freq_table[state].frequency == 200000)
 		clk_ctrl.pll = 1;
 	else
 		clk_ctrl.pll = 0;
diff --git a/drivers/cpufreq/cris-etraxfs-cpufreq.c b/drivers/cpufreq/cris-etraxfs-cpufreq.c
index 22b2cdde74d9..6ee434a54cae 100644
--- a/drivers/cpufreq/cris-etraxfs-cpufreq.c
+++ b/drivers/cpufreq/cris-etraxfs-cpufreq.c
@@ -36,7 +36,7 @@ static int cris_freq_target(struct cpufreq_policy *policy, unsigned int state)
 
 	/* Even though we may be SMP they will share the same clock
 	 * so all settings are made on CPU0. */
-	if (cris_freq_table[state].frequency == 200000)
+	if (policy->freq_table[state].frequency == 200000)
 		clk_ctrl.pll = 1;
 	else
 		clk_ctrl.pll = 0;
diff --git a/drivers/cpufreq/dbx500-cpufreq.c b/drivers/cpufreq/dbx500-cpufreq.c
index 5c3ec1dd4921..84968889ab29 100644
--- a/drivers/cpufreq/dbx500-cpufreq.c
+++ b/drivers/cpufreq/dbx500-cpufreq.c
@@ -23,7 +23,8 @@ static int dbx500_cpufreq_target(struct cpufreq_policy *policy,
 				unsigned int index)
 {
 	/* update armss clk frequency */
-	return clk_set_rate(armss_clk, freq_table[index].frequency * 1000);
+	return clk_set_rate(armss_clk,
+			    policy->freq_table[index].frequency * 1000);
 }
 
 static int dbx500_cpufreq_init(struct cpufreq_policy *policy)
diff --git a/drivers/cpufreq/e_powersaver.c b/drivers/cpufreq/e_powersaver.c
index cdf097b29862..a284bddfb067 100644
--- a/drivers/cpufreq/e_powersaver.c
+++ b/drivers/cpufreq/e_powersaver.c
@@ -163,7 +163,7 @@ static int eps_target(struct cpufreq_policy *policy, unsigned int index)
 	centaur = eps_cpu[cpu];
 
 	/* Make frequency transition */
-	dest_state = centaur->freq_table[index].driver_data & 0xffff;
+	dest_state = policy->freq_table[index].driver_data & 0xffff;
 	ret = eps_set_state(centaur, policy, dest_state);
 	if (ret)
 		pr_err("Timeout!\n");
diff --git a/drivers/cpufreq/exynos5440-cpufreq.c b/drivers/cpufreq/exynos5440-cpufreq.c
index c0f3373706f4..085f07d31ef0 100644
--- a/drivers/cpufreq/exynos5440-cpufreq.c
+++ b/drivers/cpufreq/exynos5440-cpufreq.c
@@ -212,12 +212,11 @@ static int exynos_target(struct cpufreq_policy *policy, unsigned int index)
 {
 	unsigned int tmp;
 	int i;
-	struct cpufreq_frequency_table *freq_table = dvfs_info->freq_table;
 
 	mutex_lock(&cpufreq_lock);
 
 	freqs.old = policy->cur;
-	freqs.new = freq_table[index].frequency;
+	freqs.new = policy->freq_table[index].frequency;
 
 	cpufreq_freq_transition_begin(policy, &freqs);
 
diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
index ef1fa8145419..3858dc7e617b 100644
--- a/drivers/cpufreq/imx6q-cpufreq.c
+++ b/drivers/cpufreq/imx6q-cpufreq.c
@@ -49,7 +49,7 @@ static int imx6q_set_target(struct cpufreq_policy *policy, unsigned int index)
 	unsigned int old_freq, new_freq;
 	int ret;
 
-	new_freq = freq_table[index].frequency;
+	new_freq = policy->freq_table[index].frequency;
 	freq_hz = new_freq * 1000;
 	old_freq = clk_get_rate(arm_clk) / 1000;
 
diff --git a/drivers/cpufreq/kirkwood-cpufreq.c b/drivers/cpufreq/kirkwood-cpufreq.c
index be42f103db60..f63bf4fcb6fe 100644
--- a/drivers/cpufreq/kirkwood-cpufreq.c
+++ b/drivers/cpufreq/kirkwood-cpufreq.c
@@ -54,7 +54,7 @@ static unsigned int kirkwood_cpufreq_get_cpu_frequency(unsigned int cpu)
 static int kirkwood_cpufreq_target(struct cpufreq_policy *policy,
 			    unsigned int index)
 {
-	unsigned int state = kirkwood_freq_table[index].driver_data;
+	unsigned int state = policy->freq_table[index].driver_data;
 	unsigned long reg;
 
 	local_irq_disable();
diff --git a/drivers/cpufreq/loongson2_cpufreq.c b/drivers/cpufreq/loongson2_cpufreq.c
index 6bbdac1065ff..e96c98f9245a 100644
--- a/drivers/cpufreq/loongson2_cpufreq.c
+++ b/drivers/cpufreq/loongson2_cpufreq.c
@@ -58,9 +58,8 @@ static int loongson2_cpufreq_target(struct cpufreq_policy *policy,
 	cpus_allowed = current->cpus_allowed;
 	set_cpus_allowed_ptr(current, cpumask_of(cpu));
 
-	freq =
-	    ((cpu_clock_freq / 1000) *
-	     loongson2_clockmod_table[index].driver_data) / 8;
+	freq = (cpu_clock_freq / 1000) * policy->freq_table[index].driver_data;
+	freq /= 8;
 
 	set_cpus_allowed_ptr(current, &cpus_allowed);
 
-- 
2.7.1.410.g6faf27b

[PATCH V3 2/9] cpufreq: blackfin: Use 'index' only to index into policy->freq_table

[Viresh Kumar]

Later patches would make changes in cpufreq core, after which
policy->freq_table may be reordered by cpufreq core and it wouldn't be
safe anymore to use 'index' for any other local arrays.

To prepare for that, use policy->freq_table[index].driver_data for other
driver specific usage of 'index'. The 'driver_data' fields are already
set properly by the driver.

Cc: Steven Miao <realmz6@gmail.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/blackfin-cpufreq.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/cpufreq/blackfin-cpufreq.c b/drivers/cpufreq/blackfin-cpufreq.c
index 1650c213f465..101daa851c81 100644
--- a/drivers/cpufreq/blackfin-cpufreq.c
+++ b/drivers/cpufreq/blackfin-cpufreq.c
@@ -135,6 +135,7 @@ static int bfin_target(struct cpufreq_policy *policy, unsigned int index)
 	static unsigned long lpj_ref;
 	static unsigned int  lpj_ref_freq;
 	unsigned int old_freq, new_freq;
+	int dpm_index;
 	int ret = 0;
 
 #if defined(CONFIG_CYCLES_CLOCKSOURCE)
@@ -144,8 +145,14 @@ static int bfin_target(struct cpufreq_policy *policy, unsigned int index)
 	old_freq = bfin_getfreq_khz(0);
 	new_freq = policy->freq_table[index].frequency;
 
+	/*
+	 * policy->freq_table may be sorted differently, get the index value we
+	 * are concerned about.
+	 */
+	dpm_index = policy->freq_table[index].driver_data;
+
 #ifndef CONFIG_BF60x
-	plldiv = (bfin_read_PLL_DIV() & SSEL) | dpm_state_table[index].csel;
+	plldiv = (bfin_read_PLL_DIV() & SSEL) | dpm_state_table[dpm_index].csel;
 	bfin_write_PLL_DIV(plldiv);
 #else
 	ret = cpu_set_cclk(policy->cpu, new_freq * 1000);
@@ -154,13 +161,13 @@ static int bfin_target(struct cpufreq_policy *policy, unsigned int index)
 		return ret;
 	}
 #endif
-	on_each_cpu(bfin_adjust_core_timer, &index, 1);
+	on_each_cpu(bfin_adjust_core_timer, &dpm_index, 1);
 #if defined(CONFIG_CYCLES_CLOCKSOURCE)
 	cycles = get_cycles();
 	SSYNC();
 	cycles += 10; /* ~10 cycles we lose after get_cycles() */
-	__bfin_cycles_off += (cycles << __bfin_cycles_mod) - (cycles << index);
-	__bfin_cycles_mod = index;
+	__bfin_cycles_off += (cycles << __bfin_cycles_mod) - (cycles << dpm_index);
+	__bfin_cycles_mod = dpm_index;
 #endif
 	if (!lpj_ref_freq) {
 		lpj_ref = loops_per_jiffy;
-- 
2.7.1.410.g6faf27b

[PATCH V3 3/9] cpufreq: elanfreq: Use 'index' only to index into policy->freq_table

[Viresh Kumar]

Later patches would make changes in cpufreq core, after which
policy->freq_table may be reordered by cpufreq core and it wouldn't be
safe anymore to use 'index' for any other local arrays.

To prepare for that, use policy->freq_table[index].driver_data for other
driver specific usage of 'index'. The 'driver_data' fields are already
set properly by the driver.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/elanfreq.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/cpufreq/elanfreq.c b/drivers/cpufreq/elanfreq.c
index bfce11cba1df..bd4a51091986 100644
--- a/drivers/cpufreq/elanfreq.c
+++ b/drivers/cpufreq/elanfreq.c
@@ -108,9 +108,15 @@ static unsigned int elanfreq_get_cpu_frequency(unsigned int cpu)
 
 
 static int elanfreq_target(struct cpufreq_policy *policy,
-			    unsigned int state)
+			    unsigned int index)
 {
 	/*
+	 * policy->freq_table may be sorted differently, get the index value we
+	 * are concerned about.
+	 */
+	unsigned int state = policy->freq_table[index].driver_data;
+
+	/*
 	 * Access to the Elan's internal registers is indexed via
 	 * 0x22: Chip Setup & Control Register Index Register (CSCI)
 	 * 0x23: Chip Setup & Control Register Data  Register (CSCD)
-- 
2.7.1.410.g6faf27b

[PATCH V3 4/9] cpufreq: exynos: Use 'index' only to index into policy->freq_table

[Viresh Kumar]

Later patches would make changes in cpufreq core, after which
policy->freq_table may be reordered by cpufreq core and it wouldn't be
safe anymore to use 'index' for any other local arrays.

To prepare for that, use policy->freq_table[index].driver_data for other
driver specific usage of 'index'. The 'driver_data' fields are already
set properly by the driver.

Cc: Kukjin Kim <kgene@kernel.org>
Cc: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/exynos5440-cpufreq.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/cpufreq/exynos5440-cpufreq.c b/drivers/cpufreq/exynos5440-cpufreq.c
index 085f07d31ef0..6138cbb7594e 100644
--- a/drivers/cpufreq/exynos5440-cpufreq.c
+++ b/drivers/cpufreq/exynos5440-cpufreq.c
@@ -210,7 +210,7 @@ static void exynos_enable_dvfs(unsigned int cur_frequency)
 
 static int exynos_target(struct cpufreq_policy *policy, unsigned int index)
 {
-	unsigned int tmp;
+	unsigned int tmp, rindex;
 	int i;
 
 	mutex_lock(&cpufreq_lock);
@@ -218,13 +218,19 @@ static int exynos_target(struct cpufreq_policy *policy, unsigned int index)
 	freqs.old = policy->cur;
 	freqs.new = policy->freq_table[index].frequency;
 
+	/*
+	 * policy->freq_table may be sorted differently, get the index value we
+	 * are concerned about.
+	 */
+	rindex = policy->freq_table[index].driver_data;
+
 	cpufreq_freq_transition_begin(policy, &freqs);
 
 	/* Set the target frequency in all C0_3_PSTATE register */
 	for_each_cpu(i, policy->cpus) {
 		tmp = __raw_readl(dvfs_info->base + XMU_C0_3_PSTATE + i * 4);
 		tmp &= ~(P_VALUE_MASK << C0_3_PSTATE_NEW_SHIFT);
-		tmp |= (index << C0_3_PSTATE_NEW_SHIFT);
+		tmp |= (rindex << C0_3_PSTATE_NEW_SHIFT);
 
 		__raw_writel(tmp, dvfs_info->base + XMU_C0_3_PSTATE + i * 4);
 	}
-- 
2.7.1.410.g6faf27b

[PATCH V3 5/9] cpufreq: ia64: Use 'index' only to index into policy->freq_table

[Viresh Kumar]

Later patches would make changes in cpufreq core, after which
policy->freq_table may be reordered by cpufreq core and it wouldn't be
safe anymore to use 'index' for any other local arrays.

To prepare for that, use policy->freq_table[index].driver_data for other
driver specific usage of 'index'. The 'driver_data' fields are set
properly by the driver now.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/ia64-acpi-cpufreq.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/cpufreq/ia64-acpi-cpufreq.c b/drivers/cpufreq/ia64-acpi-cpufreq.c
index 759612da4fdc..cc8bb1e5ac50 100644
--- a/drivers/cpufreq/ia64-acpi-cpufreq.c
+++ b/drivers/cpufreq/ia64-acpi-cpufreq.c
@@ -210,7 +210,12 @@ acpi_cpufreq_target (
 	struct cpufreq_policy   *policy,
 	unsigned int index)
 {
-	return processor_set_freq(acpi_io_data[policy->cpu], policy, index);
+	/*
+	 * policy->freq_table may be sorted differently, get the index value we
+	 * are concerned about.
+	 */
+	return processor_set_freq(acpi_io_data[policy->cpu], policy,
+				  policy->freq_table[index].driver_data);
 }
 
 static int
@@ -282,6 +287,8 @@ acpi_cpufreq_cpu_init (
 		} else {
 			freq_table[i].frequency = CPUFREQ_TABLE_END;
 		}
+
+		freq_table[i].driver_data = i;
 	}
 
 	result = cpufreq_table_validate_and_show(policy, freq_table);
-- 
2.7.1.410.g6faf27b

[PATCH V3 6/9] cpufreq: imx: Use 'index' only to index into policy->freq_table

[Viresh Kumar]

Later patches would make changes in cpufreq core, after which
policy->freq_table may be reordered by cpufreq core and it wouldn't be
safe anymore to use 'index' for any other local arrays.

To prepare for that, use policy->freq_table[index].driver_data for other
driver specific usage of 'index'. The 'driver_data' fields are already
set properly by the driver.

Cc: Shawn Guo <shawn.guo@freescale.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/imx6q-cpufreq.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
index 3858dc7e617b..e7da85890e8c 100644
--- a/drivers/cpufreq/imx6q-cpufreq.c
+++ b/drivers/cpufreq/imx6q-cpufreq.c
@@ -42,14 +42,21 @@ static unsigned int transition_latency;
 static u32 *imx6_soc_volt;
 static u32 soc_opp_count;
 
-static int imx6q_set_target(struct cpufreq_policy *policy, unsigned int index)
+static int imx6q_set_target(struct cpufreq_policy *policy, unsigned int pindex)
 {
 	struct dev_pm_opp *opp;
 	unsigned long freq_hz, volt, volt_old;
-	unsigned int old_freq, new_freq;
+	unsigned int old_freq, new_freq, index;
 	int ret;
 
-	new_freq = policy->freq_table[index].frequency;
+	new_freq = policy->freq_table[pindex].frequency;
+
+	/*
+	 * policy->freq_table may be sorted differently, get the index value we
+	 * are concerned about.
+	 */
+	index = policy->freq_table[pindex].driver_data;
+
 	freq_hz = new_freq * 1000;
 	old_freq = clk_get_rate(arm_clk) / 1000;
 
-- 
2.7.1.410.g6faf27b

[PATCH V3 7/9] cpufreq: maple: Use 'index' only to index into policy->freq_table

[Viresh Kumar]

Later patches would make changes in cpufreq core, after which
policy->freq_table may be reordered by cpufreq core and it wouldn't be
safe anymore to use 'index' for any other local arrays.

To prepare for that, use policy->freq_table[index].driver_data for other
driver specific usage of 'index'. The 'driver_data' fields are already
set properly by the driver.

Cc: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/maple-cpufreq.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/cpufreq/maple-cpufreq.c b/drivers/cpufreq/maple-cpufreq.c
index d9df89392b84..8ce92ee7dd8d 100644
--- a/drivers/cpufreq/maple-cpufreq.c
+++ b/drivers/cpufreq/maple-cpufreq.c
@@ -133,6 +133,12 @@ static int maple_scom_query_freq(void)
 static int maple_cpufreq_target(struct cpufreq_policy *policy,
 	unsigned int index)
 {
+	/*
+	 * policy->freq_table may be sorted differently, get the index value we
+	 * are concerned about.
+	 */
+	index = policy->freq_table[index].driver_data;
+
 	return maple_scom_switch_freq(index);
 }
 
-- 
2.7.1.410.g6faf27b

[PATCH V3 8/9] cpufreq: Keep policy->freq_table sorted in ascending order

[Viresh Kumar]

The drivers aren't required to provide a sorted frequency table today,
and its not optimal to work on an unsorted frequency tables.

To simplify and improve code performance, always keep policy->freq_table
sorted in ascending order.

Now that freq_table is sorted, update cpufreq_frequency_table_target()
to make it more efficient with help of new helpers.

As these helpers will be used by scheduler hotpath later on, keep them
in a new header cpufreq_table.h to avoid unnecessary function calls.

Also update acpi-cpufreq driver to use the efficient
cpufreq_frequency_table_target() routine, as we can't assume the
descending order of frequencies in freq_table anymore.

Tested on Exynos board with both ondemand and schedutil governor and
confirmed with help of various print messages that we are eventually
switching to the desired frequency based on a target frequency.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 MAINTAINERS                        |   1 +
 drivers/cpufreq/acpi-cpufreq.c     |  16 ++--
 drivers/cpufreq/cpufreq.c          |  20 +++--
 drivers/cpufreq/cpufreq_ondemand.h |   1 +
 drivers/cpufreq/freq_table.c       | 163 +++++++++++++++----------------------
 drivers/cpufreq/powernv-cpufreq.c  |   1 +
 drivers/cpufreq/s3c24xx-cpufreq.c  |   1 +
 drivers/cpufreq/s5pv210-cpufreq.c  |   1 +
 include/linux/cpufreq.h            |   3 -
 include/linux/cpufreq_table.h      | 139 +++++++++++++++++++++++++++++++
 10 files changed, 227 insertions(+), 119 deletions(-)
 create mode 100644 include/linux/cpufreq_table.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 7304d2e37a98..315d49d68500 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3205,6 +3205,7 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git
 T:	git git://git.linaro.org/people/vireshk/linux.git (For ARM Updates)
 F:	drivers/cpufreq/
 F:	include/linux/cpufreq.h
+F:	include/linux/cpufreq_table.h
 
 CPU FREQUENCY DRIVERS - ARM BIG LITTLE
 M:	Viresh Kumar <viresh.kumar@linaro.org>
diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
index 32a15052f363..364b86119f3f 100644
--- a/drivers/cpufreq/acpi-cpufreq.c
+++ b/drivers/cpufreq/acpi-cpufreq.c
@@ -33,6 +33,7 @@
 #include <linux/smp.h>
 #include <linux/sched.h>
 #include <linux/cpufreq.h>
+#include <linux/cpufreq_table.h>
 #include <linux/compiler.h>
 #include <linux/dmi.h>
 #include <linux/slab.h>
@@ -468,20 +469,15 @@ unsigned int acpi_cpufreq_fast_switch(struct cpufreq_policy *policy,
 	struct acpi_cpufreq_data *data = policy->driver_data;
 	struct acpi_processor_performance *perf;
 	struct cpufreq_frequency_table *entry;
-	unsigned int next_perf_state, next_freq, freq;
+	unsigned int next_perf_state, next_freq, index;
 
 	/*
 	 * Find the closest frequency above target_freq.
-	 *
-	 * The table is sorted in the reverse order with respect to the
-	 * frequency and all of the entries are valid (see the initialization).
 	 */
-	entry = policy->freq_table;
-	do {
-		entry++;
-		freq = entry->frequency;
-	} while (freq >= target_freq && freq != CPUFREQ_TABLE_END);
-	entry--;
+	index = cpufreq_frequency_table_target(policy, target_freq,
+					       CPUFREQ_RELATION_L);
+
+	entry = &policy->freq_table[index];
 	next_freq = entry->frequency;
 	next_perf_state = entry->driver_data;
 
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 9ae58a18ccb9..91f33bc28fa4 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -19,6 +19,7 @@
 
 #include <linux/cpu.h>
 #include <linux/cpufreq.h>
+#include <linux/cpufreq_table.h>
 #include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/init.h>
@@ -1137,6 +1138,16 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy, bool notify)
 	kfree(policy);
 }
 
+static void cpufreq_policy_exit(struct cpufreq_policy *policy)
+{
+	if (!cpufreq_driver->exit)
+		return;
+
+	cpufreq_driver->exit(policy);
+	kfree(policy->freq_table);
+	policy->freq_table = NULL;
+}
+
 static int cpufreq_online(unsigned int cpu)
 {
 	struct cpufreq_policy *policy;
@@ -1291,9 +1302,7 @@ static int cpufreq_online(unsigned int cpu)
 
 out_exit_policy:
 	up_write(&policy->rwsem);
-
-	if (cpufreq_driver->exit)
-		cpufreq_driver->exit(policy);
+	cpufreq_policy_exit(policy);
 out_free_policy:
 	cpufreq_policy_free(policy, !new_policy);
 	return ret;
@@ -1378,10 +1387,7 @@ static void cpufreq_offline(unsigned int cpu)
 	 * since this is a core component, and is essential for the
 	 * subsequent light-weight ->init() to succeed.
 	 */
-	if (cpufreq_driver->exit) {
-		cpufreq_driver->exit(policy);
-		policy->freq_table = NULL;
-	}
+	cpufreq_policy_exit(policy);
 
 unlock:
 	up_write(&policy->rwsem);
diff --git a/drivers/cpufreq/cpufreq_ondemand.h b/drivers/cpufreq/cpufreq_ondemand.h
index 640ea4e97106..dc90c07ace8e 100644
--- a/drivers/cpufreq/cpufreq_ondemand.h
+++ b/drivers/cpufreq/cpufreq_ondemand.h
@@ -9,6 +9,7 @@
  * published by the Free Software Foundation.
  */
 
+#include <linux/cpufreq_table.h>
 #include "cpufreq_governor.h"
 
 struct od_policy_dbs_info {
diff --git a/drivers/cpufreq/freq_table.c b/drivers/cpufreq/freq_table.c
index eac8bcbdaad1..740b3a9fce8e 100644
--- a/drivers/cpufreq/freq_table.c
+++ b/drivers/cpufreq/freq_table.c
@@ -13,6 +13,7 @@
 
 #include <linux/cpufreq.h>
 #include <linux/module.h>
+#include <linux/slab.h>
 
 /*********************************************************************
  *                     FREQUENCY TABLE HELPERS                       *
@@ -113,100 +114,6 @@ int cpufreq_generic_frequency_table_verify(struct cpufreq_policy *policy)
 }
 EXPORT_SYMBOL_GPL(cpufreq_generic_frequency_table_verify);
 
-int cpufreq_frequency_table_target(struct cpufreq_policy *policy,
-				    unsigned int target_freq,
-				    unsigned int relation)
-{
-	struct cpufreq_frequency_table optimal = {
-		.driver_data = ~0,
-		.frequency = 0,
-	};
-	struct cpufreq_frequency_table suboptimal = {
-		.driver_data = ~0,
-		.frequency = 0,
-	};
-	struct cpufreq_frequency_table *pos;
-	struct cpufreq_frequency_table *table = policy->freq_table;
-	unsigned int freq, diff, i = 0;
-	int index;
-
-	pr_debug("request for target %u kHz (relation: %u) for cpu %u\n",
-					target_freq, relation, policy->cpu);
-
-	switch (relation) {
-	case CPUFREQ_RELATION_H:
-		suboptimal.frequency = ~0;
-		break;
-	case CPUFREQ_RELATION_L:
-	case CPUFREQ_RELATION_C:
-		optimal.frequency = ~0;
-		break;
-	}
-
-	cpufreq_for_each_valid_entry(pos, table) {
-		freq = pos->frequency;
-
-		i = pos - table;
-		if ((freq < policy->min) || (freq > policy->max))
-			continue;
-		if (freq == target_freq) {
-			optimal.driver_data = i;
-			break;
-		}
-		switch (relation) {
-		case CPUFREQ_RELATION_H:
-			if (freq < target_freq) {
-				if (freq >= optimal.frequency) {
-					optimal.frequency = freq;
-					optimal.driver_data = i;
-				}
-			} else {
-				if (freq <= suboptimal.frequency) {
-					suboptimal.frequency = freq;
-					suboptimal.driver_data = i;
-				}
-			}
-			break;
-		case CPUFREQ_RELATION_L:
-			if (freq > target_freq) {
-				if (freq <= optimal.frequency) {
-					optimal.frequency = freq;
-					optimal.driver_data = i;
-				}
-			} else {
-				if (freq >= suboptimal.frequency) {
-					suboptimal.frequency = freq;
-					suboptimal.driver_data = i;
-				}
-			}
-			break;
-		case CPUFREQ_RELATION_C:
-			diff = abs(freq - target_freq);
-			if (diff < optimal.frequency ||
-			    (diff == optimal.frequency &&
-			     freq > table[optimal.driver_data].frequency)) {
-				optimal.frequency = diff;
-				optimal.driver_data = i;
-			}
-			break;
-		}
-	}
-	if (optimal.driver_data > i) {
-		if (suboptimal.driver_data > i) {
-			WARN(1, "Invalid frequency table: %d\n", policy->cpu);
-			return 0;
-		}
-
-		index = suboptimal.driver_data;
-	} else
-		index = optimal.driver_data;
-
-	pr_debug("target index is %u, freq is:%u kHz\n", index,
-		 table[index].frequency);
-	return index;
-}
-EXPORT_SYMBOL_GPL(cpufreq_frequency_table_target);
-
 int cpufreq_frequency_table_get_index(struct cpufreq_policy *policy,
 		unsigned int freq)
 {
@@ -297,15 +204,73 @@ struct freq_attr *cpufreq_generic_attr[] = {
 };
 EXPORT_SYMBOL_GPL(cpufreq_generic_attr);
 
+static int next_larger(struct cpufreq_policy *policy, unsigned int freq,
+		       struct cpufreq_frequency_table *table)
+{
+	struct cpufreq_frequency_table *pos;
+	unsigned int next_freq = ~0;
+	int index = -EINVAL;
+
+	cpufreq_for_each_valid_entry(pos, table) {
+		if (pos->frequency <= freq)
+			continue;
+
+		if (next_freq > pos->frequency) {
+			next_freq = pos->frequency;
+			index = pos - table;
+		}
+	}
+
+	return index;
+}
+
+static int create_sorted_freq_table(struct cpufreq_policy *policy,
+				    struct cpufreq_frequency_table *table)
+{
+	struct cpufreq_frequency_table *pos, *new_table;
+	unsigned int freq, index, i, count = 0;
+
+	cpufreq_for_each_valid_entry(pos, table)
+		count++;
+
+	/* Extra entry for CPUFREQ_TABLE_END */
+	count++;
+
+	new_table = kmalloc_array(count, sizeof(*new_table), GFP_KERNEL);
+	if (!new_table)
+		return -ENOMEM;
+
+	for (i = 0, freq = 0; i < count - 1; i++) {
+		index = next_larger(policy, freq, table);
+		if (index == -EINVAL)
+			break;
+
+		new_table[i].frequency = table[index].frequency;
+		new_table[i].driver_data = table[index].driver_data;
+		new_table[i].flags = table[index].flags;
+
+		freq = table[index].frequency;
+	}
+
+	new_table[i].frequency = CPUFREQ_TABLE_END;
+	policy->freq_table = new_table;
+
+	return 0;
+}
+
 int cpufreq_table_validate_and_show(struct cpufreq_policy *policy,
-				      struct cpufreq_frequency_table *table)
+				    struct cpufreq_frequency_table *table)
 {
-	int ret = cpufreq_frequency_table_cpuinfo(policy, table);
+	int ret;
+
+	if (!table)
+		return -EINVAL;
 
-	if (!ret)
-		policy->freq_table = table;
+	ret = cpufreq_frequency_table_cpuinfo(policy, table);
+	if (ret)
+		return ret;
 
-	return ret;
+	return create_sorted_freq_table(policy, table);
 }
 EXPORT_SYMBOL_GPL(cpufreq_table_validate_and_show);
 
diff --git a/drivers/cpufreq/powernv-cpufreq.c b/drivers/cpufreq/powernv-cpufreq.c
index b29c5c20c3a1..06c0f39d0133 100644
--- a/drivers/cpufreq/powernv-cpufreq.c
+++ b/drivers/cpufreq/powernv-cpufreq.c
@@ -24,6 +24,7 @@
 #include <linux/cpumask.h>
 #include <linux/module.h>
 #include <linux/cpufreq.h>
+#include <linux/cpufreq_table.h>
 #include <linux/smp.h>
 #include <linux/of.h>
 #include <linux/reboot.h>
diff --git a/drivers/cpufreq/s3c24xx-cpufreq.c b/drivers/cpufreq/s3c24xx-cpufreq.c
index 7b596fa38ad2..1ff12e869e02 100644
--- a/drivers/cpufreq/s3c24xx-cpufreq.c
+++ b/drivers/cpufreq/s3c24xx-cpufreq.c
@@ -17,6 +17,7 @@
 #include <linux/interrupt.h>
 #include <linux/ioport.h>
 #include <linux/cpufreq.h>
+#include <linux/cpufreq_table.h>
 #include <linux/cpu.h>
 #include <linux/clk.h>
 #include <linux/err.h>
diff --git a/drivers/cpufreq/s5pv210-cpufreq.c b/drivers/cpufreq/s5pv210-cpufreq.c
index 4f4e9df9b7fc..89e0ae4c7b11 100644
--- a/drivers/cpufreq/s5pv210-cpufreq.c
+++ b/drivers/cpufreq/s5pv210-cpufreq.c
@@ -18,6 +18,7 @@
 #include <linux/clk.h>
 #include <linux/io.h>
 #include <linux/cpufreq.h>
+#include <linux/cpufreq_table.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/platform_device.h>
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index c378776628b4..ee23b25b6c61 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -597,9 +597,6 @@ int cpufreq_frequency_table_verify(struct cpufreq_policy *policy,
 				   struct cpufreq_frequency_table *table);
 int cpufreq_generic_frequency_table_verify(struct cpufreq_policy *policy);
 
-int cpufreq_frequency_table_target(struct cpufreq_policy *policy,
-				   unsigned int target_freq,
-				   unsigned int relation);
 int cpufreq_frequency_table_get_index(struct cpufreq_policy *policy,
 		unsigned int freq);
 
diff --git a/include/linux/cpufreq_table.h b/include/linux/cpufreq_table.h
new file mode 100644
index 000000000000..a3e4a09ac58f
--- /dev/null
+++ b/include/linux/cpufreq_table.h
@@ -0,0 +1,139 @@
+/*
+ * linux/include/linux/cpufreq_table.h
+ *
+ * Copyright (C) 2016 Viresh Kumar <viresh.kumar@linaro.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+#ifndef _LINUX_CPUFREQ_TABLE_H
+#define _LINUX_CPUFREQ_TABLE_H
+
+#include <linux/cpufreq.h>
+
+static inline int cpufreq_find_index_l(struct cpufreq_policy *policy,
+				       unsigned int target_freq)
+{
+	struct cpufreq_frequency_table *table = policy->freq_table;
+	struct cpufreq_frequency_table *pos, *best = NULL;
+	unsigned int freq;
+
+	cpufreq_for_each_valid_entry(pos, table) {
+		freq = pos->frequency;
+
+		if ((freq < policy->min) || (freq > policy->max))
+			continue;
+
+		if (freq >= target_freq)
+			return pos - table;
+
+		best = pos;
+	}
+
+	if (best)
+		return best - table;
+
+	return -EINVAL;
+}
+
+static inline int cpufreq_find_index_h(struct cpufreq_policy *policy,
+				       unsigned int target_freq)
+{
+	struct cpufreq_frequency_table *table = policy->freq_table;
+	struct cpufreq_frequency_table *pos, *best = NULL;
+	unsigned int freq;
+
+	cpufreq_for_each_valid_entry(pos, table) {
+		freq = pos->frequency;
+
+		if ((freq < policy->min) || (freq > policy->max))
+			continue;
+
+		if (freq == target_freq)
+			return pos - table;
+
+		if (freq < target_freq) {
+			best = pos;
+			continue;
+		}
+
+		/* No freq found below target_freq */
+		if (!best)
+			best = pos;
+		break;
+	}
+
+	if (best)
+		return best - table;
+
+	return -EINVAL;
+}
+
+static inline int cpufreq_find_index_c(struct cpufreq_policy *policy,
+				       unsigned int target_freq)
+{
+	struct cpufreq_frequency_table *table = policy->freq_table;
+	struct cpufreq_frequency_table *pos, *best = NULL;
+	unsigned int freq;
+
+	cpufreq_for_each_valid_entry(pos, table) {
+		freq = pos->frequency;
+
+		if ((freq < policy->min) || (freq > policy->max))
+			continue;
+
+		if (freq == target_freq)
+			return pos - table;
+
+		if (freq < target_freq) {
+			best = pos;
+			continue;
+		}
+
+		/* No freq found below target_freq */
+		if (!best) {
+			best = pos;
+			break;
+		}
+
+		/* Choose the closest freq */
+		if (target_freq - best->frequency > freq - target_freq)
+			best = pos;
+
+		break;
+	}
+
+	if (best)
+		return best - table;
+
+	return -EINVAL;
+}
+
+static inline int cpufreq_frequency_table_target(struct cpufreq_policy *policy,
+						 unsigned int target_freq,
+						 unsigned int relation)
+{
+	int index;
+
+	switch (relation) {
+	case CPUFREQ_RELATION_L:
+		index = cpufreq_find_index_l(policy, target_freq);
+		break;
+	case CPUFREQ_RELATION_H:
+		index = cpufreq_find_index_h(policy, target_freq);
+		break;
+	case CPUFREQ_RELATION_C:
+		index = cpufreq_find_index_c(policy, target_freq);
+		break;
+	default:
+		pr_err("%s: Invalid relation: %d\n", __func__, relation);
+		return -EINVAL;
+	}
+
+	if (index == -EINVAL)
+		WARN(1, "Invalid frequency table: %d\n", policy->cpu);
+
+	return index;
+}
+#endif /* _LINUX_CPUFREQ_TABLE_H */
-- 
2.7.1.410.g6faf27b

[PATCH V3 9/9] cpufreq: drivers: Free frequency tables after being used

[Viresh Kumar]

The cpufreq core doesn't use these tables anymore after
cpufreq_table_validate_and_show() has returned.  And so these can be
freed early.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/acpi-cpufreq.c      |  7 +++----
 drivers/cpufreq/at32ap-cpufreq.c    |  6 +++---
 drivers/cpufreq/cpufreq-dt.c        |  9 ++++-----
 drivers/cpufreq/e_powersaver.c      | 24 ++++++++++++++----------
 drivers/cpufreq/ia64-acpi-cpufreq.c |  7 +++----
 drivers/cpufreq/loongson1-cpufreq.c | 10 +---------
 6 files changed, 28 insertions(+), 35 deletions(-)

diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
index 364b86119f3f..2e36677e5b36 100644
--- a/drivers/cpufreq/acpi-cpufreq.c
+++ b/drivers/cpufreq/acpi-cpufreq.c
@@ -815,8 +815,10 @@ static int acpi_cpufreq_cpu_init(struct cpufreq_policy *policy)
 	perf->state = 0;
 
 	result = cpufreq_table_validate_and_show(policy, freq_table);
+	kfree(freq_table);
+
 	if (result)
-		goto err_freqfree;
+		goto err_unreg;
 
 	if (perf->states[0].core_frequency * 1000 != policy->cpuinfo.max_freq)
 		pr_warn(FW_WARN "P-state 0 is not max freq\n");
@@ -860,8 +862,6 @@ static int acpi_cpufreq_cpu_init(struct cpufreq_policy *policy)
 
 	return result;
 
-err_freqfree:
-	kfree(freq_table);
 err_unreg:
 	acpi_processor_unregister_performance(cpu);
 err_free_mask:
@@ -883,7 +883,6 @@ static int acpi_cpufreq_cpu_exit(struct cpufreq_policy *policy)
 	policy->driver_data = NULL;
 	acpi_processor_unregister_performance(data->acpi_perf_cpu);
 	free_cpumask_var(data->freqdomain_cpus);
-	kfree(policy->freq_table);
 	kfree(data);
 
 	return 0;
diff --git a/drivers/cpufreq/at32ap-cpufreq.c b/drivers/cpufreq/at32ap-cpufreq.c
index 9231b1efb70d..c9751572ac8b 100644
--- a/drivers/cpufreq/at32ap-cpufreq.c
+++ b/drivers/cpufreq/at32ap-cpufreq.c
@@ -21,8 +21,6 @@
 #include <linux/export.h>
 #include <linux/slab.h>
 
-static struct cpufreq_frequency_table *freq_table;
-
 static unsigned int	ref_freq;
 static unsigned long	loops_per_jiffy_ref;
 
@@ -51,6 +49,7 @@ static int at32_set_target(struct cpufreq_policy *policy, unsigned int index)
 
 static int at32_cpufreq_driver_init(struct cpufreq_policy *policy)
 {
+	struct cpufreq_frequency_table *freq_table;
 	unsigned int frequency, rate, min_freq;
 	struct clk *cpuclk;
 	int retval, steps, i;
@@ -99,12 +98,13 @@ static int at32_cpufreq_driver_init(struct cpufreq_policy *policy)
 	freq_table[steps - 1].frequency = CPUFREQ_TABLE_END;
 
 	retval = cpufreq_table_validate_and_show(policy, freq_table);
+	kfree(freq_table);
+
 	if (!retval) {
 		printk("cpufreq: AT32AP CPU frequency driver\n");
 		return 0;
 	}
 
-	kfree(freq_table);
 out_err_put_clk:
 	clk_put(cpuclk);
 out_err:
diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
index 3957de801ae8..d46741b69c59 100644
--- a/drivers/cpufreq/cpufreq-dt.c
+++ b/drivers/cpufreq/cpufreq-dt.c
@@ -253,10 +253,12 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 	rcu_read_unlock();
 
 	ret = cpufreq_table_validate_and_show(policy, freq_table);
+	dev_pm_opp_free_cpufreq_table(cpu_dev, &freq_table);
+
 	if (ret) {
 		dev_err(cpu_dev, "%s: invalid frequency table: %d\n", __func__,
 			ret);
-		goto out_free_cpufreq_table;
+		goto out_free_priv;
 	}
 
 	/* Support turbo/boost mode */
@@ -264,7 +266,7 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 		/* This gets disabled by core on driver unregister */
 		ret = cpufreq_enable_boost_support();
 		if (ret)
-			goto out_free_cpufreq_table;
+			goto out_free_priv;
 		cpufreq_dt_attr[1] = &cpufreq_freq_attr_scaling_boost_freqs;
 	}
 
@@ -276,8 +278,6 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 
 	return 0;
 
-out_free_cpufreq_table:
-	dev_pm_opp_free_cpufreq_table(cpu_dev, &freq_table);
 out_free_priv:
 	kfree(priv);
 out_free_opp:
@@ -295,7 +295,6 @@ static int cpufreq_exit(struct cpufreq_policy *policy)
 	struct private_data *priv = policy->driver_data;
 
 	cpufreq_cooling_unregister(priv->cdev);
-	dev_pm_opp_free_cpufreq_table(priv->cpu_dev, &policy->freq_table);
 	dev_pm_opp_of_cpumask_remove_table(policy->related_cpus);
 	if (priv->reg_name)
 		dev_pm_opp_put_regulator(priv->cpu_dev);
diff --git a/drivers/cpufreq/e_powersaver.c b/drivers/cpufreq/e_powersaver.c
index a284bddfb067..6c6090492889 100644
--- a/drivers/cpufreq/e_powersaver.c
+++ b/drivers/cpufreq/e_powersaver.c
@@ -38,7 +38,6 @@ struct eps_cpu_data {
 #if IS_ENABLED(CONFIG_ACPI_PROCESSOR)
 	u32 bios_limit;
 #endif
-	struct cpufreq_frequency_table freq_table[];
 };
 
 static struct eps_cpu_data *eps_cpu[NR_CPUS];
@@ -324,11 +323,17 @@ static int eps_cpu_init(struct cpufreq_policy *policy)
 		states = 2;
 
 	/* Allocate private data and frequency table for current cpu */
-	centaur = kzalloc(sizeof(*centaur)
-		    + (states + 1) * sizeof(struct cpufreq_frequency_table),
-		    GFP_KERNEL);
+	centaur = kzalloc(sizeof(*centaur), GFP_KERNEL);
 	if (!centaur)
 		return -ENOMEM;
+
+	f_table = kzalloc((states + 1) * sizeof(struct cpufreq_frequency_table),
+			  GFP_KERNEL);
+	if (!f_table) {
+		kfree(centaur);
+		return -ENOMEM;
+	}
+
 	eps_cpu[0] = centaur;
 
 	/* Copy basic values */
@@ -338,7 +343,6 @@ static int eps_cpu_init(struct cpufreq_policy *policy)
 #endif
 
 	/* Fill frequency and MSR value table */
-	f_table = &centaur->freq_table[0];
 	if (brand != EPS_BRAND_C7M) {
 		f_table[0].frequency = fsb * min_multiplier;
 		f_table[0].driver_data = (min_multiplier << 8) | min_voltage;
@@ -360,13 +364,13 @@ static int eps_cpu_init(struct cpufreq_policy *policy)
 
 	policy->cpuinfo.transition_latency = 140000; /* 844mV -> 700mV in ns */
 
-	ret = cpufreq_table_validate_and_show(policy, &centaur->freq_table[0]);
-	if (ret) {
+	ret = cpufreq_table_validate_and_show(policy, f_table);
+	if (ret)
 		kfree(centaur);
-		return ret;
-	}
 
-	return 0;
+	kfree(f_table);
+
+	return ret;
 }
 
 static int eps_cpu_exit(struct cpufreq_policy *policy)
diff --git a/drivers/cpufreq/ia64-acpi-cpufreq.c b/drivers/cpufreq/ia64-acpi-cpufreq.c
index cc8bb1e5ac50..10e3bfac84d5 100644
--- a/drivers/cpufreq/ia64-acpi-cpufreq.c
+++ b/drivers/cpufreq/ia64-acpi-cpufreq.c
@@ -292,8 +292,10 @@ acpi_cpufreq_cpu_init (
 	}
 
 	result = cpufreq_table_validate_and_show(policy, freq_table);
+	kfree(freq_table);
+
 	if (result) {
-		goto err_freqfree;
+		goto err_unreg;
 	}
 
 	/* notify BIOS that we exist */
@@ -317,8 +319,6 @@ acpi_cpufreq_cpu_init (
 
 	return (result);
 
- err_freqfree:
-	kfree(freq_table);
  err_unreg:
 	acpi_processor_unregister_performance(cpu);
  err_free:
@@ -340,7 +340,6 @@ acpi_cpufreq_cpu_exit (
 	if (data) {
 		acpi_io_data[policy->cpu] = NULL;
 		acpi_processor_unregister_performance(policy->cpu);
-		kfree(policy->freq_table);
 		kfree(data);
 	}
 
diff --git a/drivers/cpufreq/loongson1-cpufreq.c b/drivers/cpufreq/loongson1-cpufreq.c
index be89416e2358..2d35d3cc2ad8 100644
--- a/drivers/cpufreq/loongson1-cpufreq.c
+++ b/drivers/cpufreq/loongson1-cpufreq.c
@@ -103,18 +103,11 @@ static int ls1x_cpufreq_init(struct cpufreq_policy *policy)
 
 	policy->clk = cpufreq->clk;
 	ret = cpufreq_generic_init(policy, freq_tbl, 0);
-	if (ret)
-		kfree(freq_tbl);
+	kfree(freq_tbl);
 
 	return ret;
 }
 
-static int ls1x_cpufreq_exit(struct cpufreq_policy *policy)
-{
-	kfree(policy->freq_table);
-	return 0;
-}
-
 static struct cpufreq_driver ls1x_cpufreq_driver = {
 	.name		= "cpufreq-ls1x",
 	.flags		= CPUFREQ_STICKY | CPUFREQ_NEED_INITIAL_FREQ_CHECK,
@@ -122,7 +115,6 @@ static struct cpufreq_driver ls1x_cpufreq_driver = {
 	.target_index	= ls1x_cpufreq_target,
 	.get		= cpufreq_generic_get,
 	.init		= ls1x_cpufreq_init,
-	.exit		= ls1x_cpufreq_exit,
 	.attr		= cpufreq_generic_attr,
 };
 
-- 
2.7.1.410.g6faf27b

Re: [PATCH V3 8/9] cpufreq: Keep policy->freq_table sorted in ascending order

[Steve Muckle]

On Fri, Jun 03, 2016 at 07:05:14PM +0530, Viresh Kumar wrote:
...
> @@ -468,20 +469,15 @@ unsigned int acpi_cpufreq_fast_switch(struct cpufreq_policy *policy,
>  	struct acpi_cpufreq_data *data = policy->driver_data;
>  	struct acpi_processor_performance *perf;
>  	struct cpufreq_frequency_table *entry;
> -	unsigned int next_perf_state, next_freq, freq;
> +	unsigned int next_perf_state, next_freq, index;
>  
>  	/*
>  	 * Find the closest frequency above target_freq.
> -	 *
> -	 * The table is sorted in the reverse order with respect to the
> -	 * frequency and all of the entries are valid (see the initialization).
>  	 */
> -	entry = policy->freq_table;
> -	do {
> -		entry++;
> -		freq = entry->frequency;
> -	} while (freq >= target_freq && freq != CPUFREQ_TABLE_END);
> -	entry--;
> +	index = cpufreq_frequency_table_target(policy, target_freq,
> +					       CPUFREQ_RELATION_L);

Can we call cpufreq_find_index_l directly here? Seems like we could
phase out cpufreq_frequency_table_target() for the most part and call
the helpers directly. It would avoid some code bloat, an unnecessary
switch statement and an error check for an invalid frequency table which
seems unnecessary for every frequency table lookup.

thanks,
Steve

Re: [PATCH V3 8/9] cpufreq: Keep policy->freq_table sorted in ascending order

[Viresh Kumar]

On 03-06-16, 16:48, Steve Muckle wrote:
> On Fri, Jun 03, 2016 at 07:05:14PM +0530, Viresh Kumar wrote:
> ...
> > @@ -468,20 +469,15 @@ unsigned int acpi_cpufreq_fast_switch(struct cpufreq_policy *policy,
> >  	struct acpi_cpufreq_data *data = policy->driver_data;
> >  	struct acpi_processor_performance *perf;
> >  	struct cpufreq_frequency_table *entry;
> > -	unsigned int next_perf_state, next_freq, freq;
> > +	unsigned int next_perf_state, next_freq, index;
> >  
> >  	/*
> >  	 * Find the closest frequency above target_freq.
> > -	 *
> > -	 * The table is sorted in the reverse order with respect to the
> > -	 * frequency and all of the entries are valid (see the initialization).
> >  	 */
> > -	entry = policy->freq_table;
> > -	do {
> > -		entry++;
> > -		freq = entry->frequency;
> > -	} while (freq >= target_freq && freq != CPUFREQ_TABLE_END);
> > -	entry--;
> > +	index = cpufreq_frequency_table_target(policy, target_freq,
> > +					       CPUFREQ_RELATION_L);
> 
> Can we call cpufreq_find_index_l directly here? Seems like we could
> phase out cpufreq_frequency_table_target() for the most part and call
> the helpers directly. It would avoid some code bloat, an unnecessary
> switch statement and an error check for an invalid frequency table which
> seems unnecessary for every frequency table lookup.

I agree with that, though that requires larger changes across multiple
sites. I hope it will be fine if I do it in a separate patch on top of
all this. Right ?

-- 
viresh

Re: [PATCH V3 6/9] cpufreq: imx: Use 'index' only to index into policy->freq_table

[Viresh Kumar]

On 03-06-16, 19:05, Viresh Kumar wrote:
> Later patches would make changes in cpufreq core, after which
> policy->freq_table may be reordered by cpufreq core and it wouldn't be
> safe anymore to use 'index' for any other local arrays.
> 
> To prepare for that, use policy->freq_table[index].driver_data for other
> driver specific usage of 'index'. The 'driver_data' fields are already
> set properly by the driver.
> 
> Cc: Shawn Guo <shawn.guo@freescale.com>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/imx6q-cpufreq.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)

Fixing Shawn's id.

-- 
viresh

Re: [PATCH V3 4/9] cpufreq: exynos: Use 'index' only to index into policy->freq_table

[Krzysztof Kozlowski]

On 06/03/2016 03:35 PM, Viresh Kumar wrote:
> Later patches would make changes in cpufreq core, after which
> policy->freq_table may be reordered by cpufreq core and it wouldn't be
> safe anymore to use 'index' for any other local arrays.
> 
> To prepare for that, use policy->freq_table[index].driver_data for other
> driver specific usage of 'index'. The 'driver_data' fields are already
> set properly by the driver.
> 
> Cc: Kukjin Kim <kgene@kernel.org>
> Cc: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/exynos5440-cpufreq.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)

Acked-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

Best regards,
Krzysztof

Re: [PATCH V3 8/9] cpufreq: Keep policy->freq_table sorted in ascending order

[Rafael J. Wysocki]

On Monday, June 06, 2016 09:22:31 AM Viresh Kumar wrote:
> On 03-06-16, 16:48, Steve Muckle wrote:
> > On Fri, Jun 03, 2016 at 07:05:14PM +0530, Viresh Kumar wrote:
> > ...
> > > @@ -468,20 +469,15 @@ unsigned int acpi_cpufreq_fast_switch(struct cpufreq_policy *policy,
> > >  	struct acpi_cpufreq_data *data = policy->driver_data;
> > >  	struct acpi_processor_performance *perf;
> > >  	struct cpufreq_frequency_table *entry;
> > > -	unsigned int next_perf_state, next_freq, freq;
> > > +	unsigned int next_perf_state, next_freq, index;
> > >  
> > >  	/*
> > >  	 * Find the closest frequency above target_freq.
> > > -	 *
> > > -	 * The table is sorted in the reverse order with respect to the
> > > -	 * frequency and all of the entries are valid (see the initialization).
> > >  	 */
> > > -	entry = policy->freq_table;
> > > -	do {
> > > -		entry++;
> > > -		freq = entry->frequency;
> > > -	} while (freq >= target_freq && freq != CPUFREQ_TABLE_END);
> > > -	entry--;
> > > +	index = cpufreq_frequency_table_target(policy, target_freq,
> > > +					       CPUFREQ_RELATION_L);
> > 
> > Can we call cpufreq_find_index_l directly here? Seems like we could
> > phase out cpufreq_frequency_table_target() for the most part and call
> > the helpers directly. It would avoid some code bloat, an unnecessary
> > switch statement and an error check for an invalid frequency table which
> > seems unnecessary for every frequency table lookup.
> 
> I agree with that, though that requires larger changes across multiple
> sites.

What changes and where?

> I hope it will be fine if I do it in a separate patch on top of
> all this. Right ?

Depending.

Re: [PATCH V3 8/9] cpufreq: Keep policy->freq_table sorted in ascending order

[Viresh Kumar]

On 6 June 2016 at 17:40, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Monday, June 06, 2016 09:22:31 AM Viresh Kumar wrote:

>> I agree with that, though that requires larger changes across multiple
>> sites.
>
> What changes and where?

s/larger/some :)

So we can change all the callers of cpufreq_frequency_table_target(),
like the governors, ondemand-bias stub drivers, core, etc and call
the dedicated 'relation' specific routines directly, as we mostly know
in advance the 'relation' in which we want to update the freq.

And, so doing that with a dedicated patch might be better.

--
vireshj

Re: [PATCH V3 8/9] cpufreq: Keep policy->freq_table sorted in ascending order

[Rafael J. Wysocki]

On Mon, Jun 6, 2016 at 2:24 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 6 June 2016 at 17:40, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> On Monday, June 06, 2016 09:22:31 AM Viresh Kumar wrote:
>
>>> I agree with that, though that requires larger changes across multiple
>>> sites.
>>
>> What changes and where?
>
> s/larger/some :)
>
> So we can change all the callers of cpufreq_frequency_table_target(),

But why?

It just works as a static inline wrapper around cpufreq_find_index_l()
for the code in question after this patch, doesn't it?

So if the caller knows it will always ask for RELATION_L, why bother
with using the wrapper?

Also I'm wondering about the cpufreq_for_each_valid_entry() used all
over.  Can't the things be arranged so all of the entries are valid?

Re: [PATCH V3 8/9] cpufreq: Keep policy->freq_table sorted in ascending order

[Viresh Kumar]

On 6 June 2016 at 18:27, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Mon, Jun 6, 2016 at 2:24 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>> On 6 June 2016 at 17:40, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>> On Monday, June 06, 2016 09:22:31 AM Viresh Kumar wrote:
>>
>>>> I agree with that, though that requires larger changes across multiple
>>>> sites.
>>>
>>> What changes and where?
>>
>> s/larger/some :)
>>
>> So we can change all the callers of cpufreq_frequency_table_target(),
>
> But why?
>
> It just works as a static inline wrapper around cpufreq_find_index_l()
> for the code in question after this patch, doesn't it?
>
> So if the caller knows it will always ask for RELATION_L, why bother
> with using the wrapper?

Sorry, I got a bit confused. Are you saying that we should do that change
right in the patch?

Because I am also saying that yes, there is no point calling the wrapper.

I can update this patch to make direct calls to the relation specific routines
if you want.

> Also I'm wondering about the cpufreq_for_each_valid_entry() used all
> over.  Can't the things be arranged so all of the entries are valid?

Yeah, there would be multiple opportunities available to optimize code
after this series is in. The policy->table after this series is all sorted
properly and all the entries are valid as well.

But surely that should be done in a separate series

Re: [PATCH V3 0/9] cpufreq: Sort policy->freq_table

[Rafael J. Wysocki]

On Fri, Jun 3, 2016 at 3:35 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> Hi Rafael,
>
> So all my patches are contained in two series. The first one is:
>
> [PATCH V3 0/8] cpufreq: cleanups and reorganization
>
> which I have sent this morning. It does some cleanup and shall be
> applied regardless of this series.
>
> This series improves the performance of cpufreq_frequency_table_target()
> routine by storing the policy->freq_table sorted in ascending order. On
> one hand it shall improve the performance of current governors for
> drivers providing freq-table to cpufreq core and on another hand this
> API can be used directly from schedutil governor as well.
>
> As Steve has requested earlier, these APIs are moved to a .h file to
> avoid function calls.
>
> Steve's series can use this API now without any performance lag.
>
> The first 7 patches makes sure that current drivers wouldn't break because
> of reordering of policy->freq_table and the 8th patch updates cpufreq
> core to sort policy->freq_table and make
> cpufreq_frequency_table_target() much more efficient.

Is there any particular reason why patches [2-7/9] are separate?

They seem to be making analogous changes in several drivers (and even
the changelog seems to be the same), so why don't you make a single
patch out of them?

Re: [PATCH V3 8/9] cpufreq: Keep policy->freq_table sorted in ascending order

[Rafael J. Wysocki]

On Mon, Jun 6, 2016 at 6:25 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 6 June 2016 at 18:27, Rafael J. Wysocki <rafael@kernel.org> wrote:
>> On Mon, Jun 6, 2016 at 2:24 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>>> On 6 June 2016 at 17:40, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>>> On Monday, June 06, 2016 09:22:31 AM Viresh Kumar wrote:
>>>
>>>>> I agree with that, though that requires larger changes across multiple
>>>>> sites.
>>>>
>>>> What changes and where?
>>>
>>> s/larger/some :)
>>>
>>> So we can change all the callers of cpufreq_frequency_table_target(),
>>
>> But why?
>>
>> It just works as a static inline wrapper around cpufreq_find_index_l()
>> for the code in question after this patch, doesn't it?
>>
>> So if the caller knows it will always ask for RELATION_L, why bother
>> with using the wrapper?
>
> Sorry, I got a bit confused. Are you saying that we should do that change
> right in the patch?
>
> Because I am also saying that yes, there is no point calling the wrapper.

OK

> I can update this patch to make direct calls to the relation specific routines
> if you want.

I'm not sure if I like this patch at all in the first place.

>> Also I'm wondering about the cpufreq_for_each_valid_entry() used all
>> over.  Can't the things be arranged so all of the entries are valid?
>
> Yeah, there would be multiple opportunities available to optimize code
> after this series is in. The policy->table after this series is all sorted
> properly and all the entries are valid as well.
>
> But surely that should be done in a separate series

So I'm reading this as "I will add overhead to that code now, but I
can remove it later" which makes me go "What?!" right away.

Moreover, you seem to be saying something like "all of the entries are
valid now, but I'm using cpufreq_for_each_valid_entry() to walk freq
tables anyway, so that I can get rid of it in a future patch" which
makes me go "What?!" again.

Since you are adding new code, you can write it so it doesn't do
unnecessary checks from the start.

While at it, the "if ((freq < policy->min) || (freq > policy->max))"
checks in cpufreq_find_index_l() and cpufreq_find_index_h() don't look
good to me, because they very well may cause those function to return
-EINVAL even when there's a valid table and that may cause
acpi_cpufreq_fast_switch() to do bad things.

Also, if you are going to return an index, why don't you iterate over
indexes and avoid using pointer subtractions to compute the return
value?

With that, cpufreq_find_index_l() would look like

static inline int cpufreq_find_index_l(struct cpufreq_policy *policy,
                       unsigned int target_freq)
{
    struct cpufreq_frequency_table *table = policy->freq_table;
    int i;

    for (i = 0; table[i].frequency != CPUFREQ_TABLE_END; i++)
        if (table[i].frequency >= target_freq)
            return i;

    return i > 0 ? --i : 0;
}

and that can go into cpufreq.h IMO (ie. no need for the new header file).

Re: [PATCH V3 9/9] cpufreq: drivers: Free frequency tables after being used

[Rafael J. Wysocki]

On Fri, Jun 3, 2016 at 3:35 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> The cpufreq core doesn't use these tables anymore after
> cpufreq_table_validate_and_show() has returned.  And so these can be
> freed early.

That doesn't look particularly efficient.

The driver has to allocate memory first and populate it and then the
core needs to allocate more memory again to store the same information
in it, but perhaps in a different order and then the driver can free
the memory allocated before right away.  It looks like there's one
excessive memory allocation here, so maybe the core can simply sort
the frequency table in place and overwrite the invalid entries in the
process?

Re: [PATCH V3 0/9] cpufreq: Sort policy->freq_table

[Viresh Kumar]

On 06-06-16, 23:09, Rafael J. Wysocki wrote:
> Is there any particular reason why patches [2-7/9] are separate?

I just wanted them to be separately reviewable by platform driver owners.
Nothing else.

Will merge them in next version.

-- 
viresh

Re: [PATCH V3 8/9] cpufreq: Keep policy->freq_table sorted in ascending order

[Viresh Kumar]

On 06-06-16, 23:56, Rafael J. Wysocki wrote:
> Since you are adding new code, you can write it so it doesn't do
> unnecessary checks from the start.

Hmm, I will do all that in this series only now.

> While at it, the "if ((freq < policy->min) || (freq > policy->max))"
> checks in cpufreq_find_index_l() and cpufreq_find_index_h() don't look
> good to me, because they very well may cause those function to return
> -EINVAL even when there's a valid table and that may cause
> acpi_cpufreq_fast_switch() to do bad things.

Hmm. So, the checks are for sure required here, otherwise we may end up
returning a frequency which we aren't allowed to. Also note that 'freq' here
isn't the target-freq, but the entry in the freq-table.

This routine should be returning a valid freq within the ranges specified by
policy->min/max.

Also note that these routines shall *never* return -EINVAL, otherwise it is
mostly a bug we are hitting.

We have enough checks in place to make sure that there is at least one valid
entry in the freq-table which is >= policy->min and <= policy->max.

I will take care of rest of the comments though. Thanks.

-- 
viresh

Re: [PATCH V3 8/9] cpufreq: Keep policy->freq_table sorted in ascending order

[Rafael J. Wysocki]

On Tuesday, June 07, 2016 09:58:07 AM Viresh Kumar wrote:
> On 06-06-16, 23:56, Rafael J. Wysocki wrote:
> > Since you are adding new code, you can write it so it doesn't do
> > unnecessary checks from the start.
> 
> Hmm, I will do all that in this series only now.
> 
> > While at it, the "if ((freq < policy->min) || (freq > policy->max))"
> > checks in cpufreq_find_index_l() and cpufreq_find_index_h() don't look
> > good to me, because they very well may cause those function to return
> > -EINVAL even when there's a valid table and that may cause
> > acpi_cpufreq_fast_switch() to do bad things.
> 
> Hmm. So, the checks are for sure required here, otherwise we may end up
> returning a frequency which we aren't allowed to. Also note that 'freq' here
> isn't the target-freq, but the entry in the freq-table.
> 
> This routine should be returning a valid freq within the ranges specified by
> policy->min/max.

Which in principle may not be possible if the range doesn't include any
frequency in the table, eg. min == max and between the table entries.

However, the CPU has to run at *some* frequency, even if there's none in the
min/max range.

And if we are sure that there is at least one valid frequency between min
and max, please note that target_freq has already been clamped between them,
so clamping again is rather unuseful.  And of course it is racy in general,
which makes it even more unuseful.

> Also note that these routines shall *never* return -EINVAL, otherwise it is
> mostly a bug we are hitting.

So make them explicitly return a valid frequency every time.

> We have enough checks in place to make sure that there is at least one valid
> entry in the freq-table which is >= policy->min and <= policy->max.

That assuming that the driver will always do the right thing in its ->verify
callback.

> I will take care of rest of the comments though. Thanks.

Thanks!

Re: [PATCH V3 8/9] cpufreq: Keep policy->freq_table sorted in ascending order

[Viresh Kumar]

On 08-06-16, 02:38, Rafael J. Wysocki wrote:
> On Tuesday, June 07, 2016 09:58:07 AM Viresh Kumar wrote:
> > On 06-06-16, 23:56, Rafael J. Wysocki wrote:
> > > Since you are adding new code, you can write it so it doesn't do
> > > unnecessary checks from the start.
> > 
> > Hmm, I will do all that in this series only now.
> > 
> > > While at it, the "if ((freq < policy->min) || (freq > policy->max))"
> > > checks in cpufreq_find_index_l() and cpufreq_find_index_h() don't look
> > > good to me, because they very well may cause those function to return
> > > -EINVAL even when there's a valid table and that may cause
> > > acpi_cpufreq_fast_switch() to do bad things.
> > 
> > Hmm. So, the checks are for sure required here, otherwise we may end up
> > returning a frequency which we aren't allowed to. Also note that 'freq' here
> > isn't the target-freq, but the entry in the freq-table.
> > 
> > This routine should be returning a valid freq within the ranges specified by
> > policy->min/max.
> 
> Which in principle may not be possible if the range doesn't include any
> frequency in the table, eg. min == max and between the table entries.

By within ranges I meant, policy->min <= freq <= policy->max, and that's how all
our checks are. So even if the table will have a single valid frequency, we will
return that only.

> However, the CPU has to run at *some* frequency, even if there's none in the
> min/max range.

I completely agree. But the error will be fired only if there is no frequency
within ranges we can switch to. And that's a bug somewhere else then.

> And if we are sure that there is at least one valid frequency between min
> and max, please note that target_freq has already been clamped between them,

Yeah, its already clamped by the freq-change helpers in cpufreq core, but others
may not be doing it properly.

> > Also note that these routines shall *never* return -EINVAL, otherwise it is
> > mostly a bug we are hitting.
> 
> So make them explicitly return a valid frequency every time.

I thought about return Index 0 on such errors, will that be fine ? Anyway the
new patches have added a WARN() for such cases.

> > We have enough checks in place to make sure that there is at least one valid
> > entry in the freq-table which is >= policy->min and <= policy->max.
> 
> That assuming that the driver will always do the right thing in its ->verify
> callback.

Yeah.

-- 
viresh