[PATCH 00/11] cpufreq: Keep policy->freq_table sorted

[PATCH 00/11] cpufreq: Keep policy->freq_table sorted

[Viresh Kumar]

Hi Rafael,

This series fixes all cpufreq drivers that provide a 'target_index'
callback or in other words, which provide a freq-table to cpufreq core,
to make sure they *only* use the 'index' argument to ->target_index()
with the policy->freq_table.

This change allows us to remove the (duplicate) sorted-freq-table, which
was added by following series:

[PATCH V2 0/2] cpufreq: Use sorted frequency tables

The final code looks like this:
- drivers provide a freq table to the cpufreq core
- core makes a copy of that and sort that in ascending order of
  frequencies. This is what we get from policy->freq_table.
- drivers can now free the freq-table they provided earlier.
- ->target_index() contains the 'index' to this sorted
  policy->freq_table.

This is based of the two series I have posted until now:

[PATCH V2 0/6] cpufreq: cleanups and reorganization
[PATCH V2 0/2] cpufreq: Use sorted frequency tables

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

The 3 series combined makes freq-table traversing to find a match really
fast and efficient. Which will also allow us to use it for new governors
like schedutil.

--
viresh

Viresh Kumar (11):
  ARM: davinci: Sort frequency table
  cpufreq: davinci: Reuse cpufreq_generic_frequency_table_verify()
  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 a single (sorted) freq_table
  cpufreq: drivers: Free frequency tables after being used

 arch/arm/mach-davinci/da850.c          | 16 ++++++++-------
 drivers/cpufreq/acpi-cpufreq.c         |  7 +++----
 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              |  6 +-----
 drivers/cpufreq/cris-artpec3-cpufreq.c |  2 +-
 drivers/cpufreq/cris-etraxfs-cpufreq.c |  2 +-
 drivers/cpufreq/davinci-cpufreq.c      | 22 +--------------------
 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           | 36 +++++++++++++---------------------
 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 ++++++
 include/linux/cpufreq.h                |  9 ++-------
 22 files changed, 117 insertions(+), 121 deletions(-)

-- 
2.7.1.410.g6faf27b

[PATCH 01/11] ARM: davinci: Sort frequency table

[Viresh Kumar]

This is required for some of the changes in cpufreq core. There was only
one function dependent on the order of the table, that is fixed as well.

Cc: Sekhar Nori <nsekhar@ti.com>
Cc: Kevin Hilman <khilman@kernel.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 arch/arm/mach-davinci/da850.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/arch/arm/mach-davinci/da850.c b/arch/arm/mach-davinci/da850.c
index 239886299968..f683c119cfed 100644
--- a/arch/arm/mach-davinci/da850.c
+++ b/arch/arm/mach-davinci/da850.c
@@ -1004,13 +1004,14 @@ static const struct da850_opp da850_opp_96 = {
 		.frequency = freq * 1000, \
 	}
 
+/* Table sorted in ascending order of frequencies */
 static struct cpufreq_frequency_table da850_freq_table[] = {
-	OPP(456),
-	OPP(408),
-	OPP(372),
-	OPP(300),
-	OPP(200),
 	OPP(96),
+	OPP(200),
+	OPP(300),
+	OPP(372),
+	OPP(408),
+	OPP(456),
 	{
 		.driver_data		= 0,
 		.frequency	= CPUFREQ_TABLE_END,
@@ -1076,8 +1077,9 @@ int da850_register_cpufreq(char *async_clk)
 		clk_add_alias("async", da850_cpufreq_device.name,
 							async_clk, NULL);
 	for (i = 0; i < ARRAY_SIZE(da850_freq_table); i++) {
-		if (da850_freq_table[i].frequency <= da850_max_speed) {
-			cpufreq_info.freq_table = &da850_freq_table[i];
+		if (da850_freq_table[i].frequency > da850_max_speed) {
+			&da850_freq_table[i].driver_data = 0;
+			&da850_freq_table[i].frequency = CPUFREQ_TABLE_END;
 			break;
 		}
 	}
-- 
2.7.1.410.g6faf27b

[PATCH 02/11] cpufreq: davinci: Reuse cpufreq_generic_frequency_table_verify()

[Viresh Kumar]

policy->freq_table will always be valid for this platform, otherwise
driver's probe() would fail. And so this routine will *always* return
after calling cpufreq_frequency_table_verify().

This can be done using the generic callback provided by core, lets use
it instead.

Cc: Sekhar Nori <nsekhar@ti.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/davinci-cpufreq.c | 22 +---------------------
 1 file changed, 1 insertion(+), 21 deletions(-)

diff --git a/drivers/cpufreq/davinci-cpufreq.c b/drivers/cpufreq/davinci-cpufreq.c
index 7e336d20c184..b95a872800ec 100644
--- a/drivers/cpufreq/davinci-cpufreq.c
+++ b/drivers/cpufreq/davinci-cpufreq.c
@@ -38,26 +38,6 @@ struct davinci_cpufreq {
 };
 static struct davinci_cpufreq cpufreq;
 
-static int davinci_verify_speed(struct cpufreq_policy *policy)
-{
-	struct davinci_cpufreq_config *pdata = cpufreq.dev->platform_data;
-	struct cpufreq_frequency_table *freq_table = pdata->freq_table;
-	struct clk *armclk = cpufreq.armclk;
-
-	if (freq_table)
-		return cpufreq_frequency_table_verify(policy, freq_table);
-
-	if (policy->cpu)
-		return -EINVAL;
-
-	cpufreq_verify_within_cpu_limits(policy);
-	policy->min = clk_round_rate(armclk, policy->min * 1000) / 1000;
-	policy->max = clk_round_rate(armclk, policy->max * 1000) / 1000;
-	cpufreq_verify_within_limits(policy, policy->cpuinfo.min_freq,
-						policy->cpuinfo.max_freq);
-	return 0;
-}
-
 static int davinci_target(struct cpufreq_policy *policy, unsigned int idx)
 {
 	struct davinci_cpufreq_config *pdata = cpufreq.dev->platform_data;
@@ -121,7 +101,7 @@ static int davinci_cpu_init(struct cpufreq_policy *policy)
 
 static struct cpufreq_driver davinci_driver = {
 	.flags		= CPUFREQ_STICKY | CPUFREQ_NEED_INITIAL_FREQ_CHECK,
-	.verify		= davinci_verify_speed,
+	.verify		= cpufreq_generic_frequency_table_verify,
 	.target_index	= davinci_target,
 	.get		= cpufreq_generic_get,
 	.init		= davinci_cpu_init,
-- 
2.7.1.410.g6faf27b

[PATCH 03/11] 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 04/11] 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 05/11] 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 06/11] 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 07/11] 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 08/11] 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 09/11] 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 10/11] cpufreq: Keep a single (sorted) freq_table

[Viresh Kumar]

Now that all drivers providing ->target_index() callback are updated to
use 'index' only for indexing into policy->freq_table, we can safely
avoid keeping two separate freq-tables.

Which also means that cpufreq core doesn't use the freq_table passed to
cpufreq_table_validate_and_show(), once that routine has returned.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c    |  6 +-----
 drivers/cpufreq/freq_table.c | 36 ++++++++++++++----------------------
 include/linux/cpufreq.h      |  9 ++-------
 3 files changed, 17 insertions(+), 34 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 10c5f7abc205..47983cb0601d 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1143,7 +1143,7 @@ static void cpufreq_policy_exit(struct cpufreq_policy *policy)
 		return;
 
 	cpufreq_driver->exit(policy);
-	free_sorted_freq_table(policy);
+	kfree(policy->freq_table);
 	policy->freq_table = NULL;
 }
 
@@ -1188,10 +1188,6 @@ static int cpufreq_online(unsigned int cpu)
 		goto out_free_policy;
 	}
 
-	ret = create_sorted_freq_table(policy);
-	if (ret)
-		goto out_exit_policy;
-
 	down_write(&policy->rwsem);
 
 	if (new_policy) {
diff --git a/drivers/cpufreq/freq_table.c b/drivers/cpufreq/freq_table.c
index 15c4a2462c68..7afe2c017267 100644
--- a/drivers/cpufreq/freq_table.c
+++ b/drivers/cpufreq/freq_table.c
@@ -350,20 +350,11 @@ static int next_larger(struct cpufreq_policy *policy, unsigned int freq,
 	return index;
 }
 
-void free_sorted_freq_table(struct cpufreq_policy *policy)
-{
-	kfree(policy->sorted_freq_table);
-	policy->sorted_freq_table = NULL;
-}
-
-int create_sorted_freq_table(struct cpufreq_policy *policy)
+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;
-	struct cpufreq_frequency_table *table = policy->freq_table;
-
-	if (!table)
-		return 0;
 
 	cpufreq_for_each_valid_entry(pos, table)
 		count++;
@@ -380,31 +371,32 @@ int create_sorted_freq_table(struct cpufreq_policy *policy)
 		if (index == -EINVAL)
 			break;
 
-		/*
-		 * driver_data of the sorted table points to the index of the
-		 * unsorted table.
-		 */
-		new_table[i].driver_data = index;
 		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->sorted_freq_table = new_table;
+	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/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 5aabec611e87..9df7c569cfbb 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -87,7 +87,6 @@ struct cpufreq_policy {
 
 	struct cpufreq_user_policy user_policy;
 	struct cpufreq_frequency_table	*freq_table;
-	struct cpufreq_frequency_table	*sorted_freq_table;
 
 	struct list_head        policy_list;
 	struct kobject		kobj;
@@ -593,8 +592,6 @@ static inline void dev_pm_opp_free_cpufreq_table(struct device *dev,
 
 int cpufreq_frequency_table_cpuinfo(struct cpufreq_policy *policy,
 				    struct cpufreq_frequency_table *table);
-int create_sorted_freq_table(struct cpufreq_policy *policy);
-void free_sorted_freq_table(struct cpufreq_policy *policy);
 
 int cpufreq_frequency_table_verify(struct cpufreq_policy *policy,
 				   struct cpufreq_frequency_table *table);
@@ -616,10 +613,8 @@ static inline int cpufreq_frequency_table_target(struct cpufreq_policy *policy,
 						 unsigned int target_freq,
 						 unsigned int relation)
 {
-	int index = cpufreq_find_target_index(policy, policy->sorted_freq_table,
-					      target_freq, relation);
-
-	return policy->sorted_freq_table[index].driver_data;
+	return cpufreq_find_target_index(policy, policy->freq_table,
+					 target_freq, relation);
 }
 
 #ifdef CONFIG_CPU_FREQ
-- 
2.7.1.410.g6faf27b

[PATCH 11/11] 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 4f9f7504a17c..047425bcaa84 100644
--- a/drivers/cpufreq/acpi-cpufreq.c
+++ b/drivers/cpufreq/acpi-cpufreq.c
@@ -814,8 +814,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");
@@ -859,8 +861,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:
@@ -882,7 +882,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 00/11] cpufreq: Keep policy->freq_table sorted

[Rafael J. Wysocki]

On Thu, Jun 2, 2016 at 4:19 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> Hi Rafael,
>
> This series fixes all cpufreq drivers that provide a 'target_index'
> callback or in other words, which provide a freq-table to cpufreq core,
> to make sure they *only* use the 'index' argument to ->target_index()
> with the policy->freq_table.
>
> This change allows us to remove the (duplicate) sorted-freq-table, which
> was added by following series:
>
> [PATCH V2 0/2] cpufreq: Use sorted frequency tables

Which I'm not going to apply.

If this series depended on that one, please rework it.

Thanks,
Rafael

Re: [PATCH 00/11] cpufreq: Keep policy->freq_table sorted

[Viresh Kumar]

On 2 June 2016 at 20:38, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Thu, Jun 2, 2016 at 4:19 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>> Hi Rafael,
>>
>> This series fixes all cpufreq drivers that provide a 'target_index'
>> callback or in other words, which provide a freq-table to cpufreq core,
>> to make sure they *only* use the 'index' argument to ->target_index()
>> with the policy->freq_table.
>>
>> This change allows us to remove the (duplicate) sorted-freq-table, which
>> was added by following series:
>>
>> [PATCH V2 0/2] cpufreq: Use sorted frequency tables
>
> Which I'm not going to apply.
>
> If this series depended on that one, please rework it.

Hi Rafael,

The first 9 patches can be applied without any dependency
on the earlier series, but not the last two.

The target of this series is to make freq-table lookup faster
and so most of its patches would make sense only if we
are trying to solve that problem. Otherwise they may not
be required at all.

I hope that you will be replying to the earlier series [1] as well,
to convey the concerns you have with it.

Thanks

--
viresh

[1] [PATCH V2 0/2] cpufreq: Use sorted frequency tables

Re: [PATCH 00/11] cpufreq: Keep policy->freq_table sorted

[Rafael J. Wysocki]

On Thu, Jun 2, 2016 at 5:42 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 2 June 2016 at 20:38, Rafael J. Wysocki <rafael@kernel.org> wrote:
>> On Thu, Jun 2, 2016 at 4:19 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>>> Hi Rafael,
>>>
>>> This series fixes all cpufreq drivers that provide a 'target_index'
>>> callback or in other words, which provide a freq-table to cpufreq core,
>>> to make sure they *only* use the 'index' argument to ->target_index()
>>> with the policy->freq_table.
>>>
>>> This change allows us to remove the (duplicate) sorted-freq-table, which
>>> was added by following series:
>>>
>>> [PATCH V2 0/2] cpufreq: Use sorted frequency tables
>>
>> Which I'm not going to apply.
>>
>> If this series depended on that one, please rework it.
>
> Hi Rafael,
>
> The first 9 patches can be applied without any dependency
> on the earlier series, but not the last two.
>
> The target of this series is to make freq-table lookup faster
> and so most of its patches would make sense only if we
> are trying to solve that problem. Otherwise they may not
> be required at all.

Well, in that case I'm not sure about them.

> I hope that you will be replying to the earlier series [1] as well,
> to convey the concerns you have with it.

Oh, that's very simple: I don't see a reason to apply that series.

Quoting from this very cover letter "This change allows us to remove
the (duplicate) sorted-freq-table, which
was added by following series:", so why to add it in the first place?

Besides, there already is a number of tables (per policy which in some
important cases pretty much means per CPU) in cpufreq that contain
more-or-less the same information.  For example, if acpi-cpufreq is in
use, the ACPI layer has a table coming from _PSS, the driver creates
freq_table to pass to the core and there is an additional one for the
stats.  And your series adds one more just so it is ordered.  Come on.

If you want to clean that up, fine, but please don't do that in a
hurry.  Let's talk about it a bit more without sending any more
patches in that area for the time being.

At this point, I'm lost in the patches you've been sending for the
last couple of days.  I don't know what depends on what, what will be
updated and so on and quite frankly I don't really have the time to
figure that out.  Overall, the most recent burst of activity regarding
the sorting of frequency tables hasn't been very helpful from my
perspective.

If you have cleanups that look obvious and don't interfere with
anything already posted by Steve, please send them *again* in one
ordered series and let everybody know whom you would like to apply
them etc.

And now I'm going to look at the Steve's patches again and start over
from there.

Thanks,
Rafael

Re: [PATCH 00/11] cpufreq: Keep policy->freq_table sorted

[Viresh Kumar]

On 02-06-16, 22:35, Rafael J. Wysocki wrote:
> Quoting from this very cover letter "This change allows us to remove
> the (duplicate) sorted-freq-table, which
> was added by following series:", so why to add it in the first place?

Okay, that's fine.

> Besides, there already is a number of tables (per policy which in some
> important cases pretty much means per CPU) in cpufreq that contain
> more-or-less the same information.  For example, if acpi-cpufreq is in
> use, the ACPI layer has a table coming from _PSS, the driver creates
> freq_table to pass to the core and there is an additional one for the
> stats.  And your series adds one more just so it is ordered.  Come on.

Of course.

> If you want to clean that up, fine, but please don't do that in a
> hurry.  Let's talk about it a bit more without sending any more
> patches in that area for the time being.

Okay, I will send all the fixes that you can apply cleanly now in a
separate set.

So, yeah, I get your overall concern. What about this:
- A single patchset to make sure the current policy->freq_table is
  always sorted in Ascending order of frequencies.
- And this sorting will be done per policy only when the policy is
  first created.
- Which would eventually mean merging this series with the [v2 0/2]
  one.

Will that work ?

-- 
viresh

Re: [PATCH 00/11] cpufreq: Keep policy->freq_table sorted

[Rafael J. Wysocki]

On Friday, June 03, 2016 05:31:34 AM Viresh Kumar wrote:
> On 02-06-16, 22:35, Rafael J. Wysocki wrote:
> > Quoting from this very cover letter "This change allows us to remove
> > the (duplicate) sorted-freq-table, which
> > was added by following series:", so why to add it in the first place?
> 
> Okay, that's fine.
> 
> > Besides, there already is a number of tables (per policy which in some
> > important cases pretty much means per CPU) in cpufreq that contain
> > more-or-less the same information.  For example, if acpi-cpufreq is in
> > use, the ACPI layer has a table coming from _PSS, the driver creates
> > freq_table to pass to the core and there is an additional one for the
> > stats.  And your series adds one more just so it is ordered.  Come on.
> 
> Of course.
> 
> > If you want to clean that up, fine, but please don't do that in a
> > hurry.  Let's talk about it a bit more without sending any more
> > patches in that area for the time being.
> 
> Okay, I will send all the fixes that you can apply cleanly now in a
> separate set.

Thanks!

> So, yeah, I get your overall concern. What about this:
> - A single patchset to make sure the current policy->freq_table is
>   always sorted in Ascending order of frequencies.

Be careful here.  acpi-cpufreq sorts the table in the descending order
and at least acpi_cpufreq_fast_switch() assumes that.

> - And this sorting will be done per policy only when the policy is
>   first created.
> - Which would eventually mean merging this series with the [v2 0/2]
>   one.
> 
> Will that work ?

Well, it may. :-)

I would like you to talk to Steve and agree on the approach, including which
changes to make first, though.  You are both from Linaro after all ...

Thanks,
Rafael

Re: [PATCH 00/11] cpufreq: Keep policy->freq_table sorted

[Viresh Kumar]

On 03-06-16, 03:43, Rafael J. Wysocki wrote:
> On Friday, June 03, 2016 05:31:34 AM Viresh Kumar wrote:
> > So, yeah, I get your overall concern. What about this:
> > - A single patchset to make sure the current policy->freq_table is
> >   always sorted in Ascending order of frequencies.
> 
> Be careful here.  acpi-cpufreq sorts the table in the descending order
> and at least acpi_cpufreq_fast_switch() assumes that.

Yeah, it was already fixed in [V2 0/2] series. Thanks.

> > - And this sorting will be done per policy only when the policy is
> >   first created.
> > - Which would eventually mean merging this series with the [v2 0/2]
> >   one.
> > 
> > Will that work ?
> 
> Well, it may. :-)
> 
> I would like you to talk to Steve and agree on the approach, including which
> changes to make first, though.  You are both from Linaro after all ...

Sure, we can get that done and I am not particular here on whose patches should
get in first. The outcome should be same whatever order we follow :)

Thanks.

-- 
viresh