[PATCH 0/8] cppc_cpufreq: fix, clarify and improve support

[PATCH 0/8] cppc_cpufreq: fix, clarify and improve support

[Ionela Voinescu]

Hi guys,

I found myself staring a bit too much at this driver in the past weeks
and that's likely the cause for me coming up with this series of 8
patches that cleans up, clarifies and reworks parts of it, as follows:

 - patches 1-3/8: trivial clean-up and renaming with the purpose to
                  improve readability
 - patch 4/8: replace previous per-cpu data structures with lists of
              domains and CPUs to get more efficient storage for driver
              data and fix previous issues in case of CPU hotplugging,
              as discussed at [1].
 - patches 5-6/8: a few fixes and clarifications: mostly making sure
                  the behavior described in the comments and debug
                  messages matches the code and there is clear
                  indication of what is supported and how.
 - patch 7/8: use the existing freqdomains_cpus attribute to inform
              the user on frequency domains.
 - patch 8/8: acpi: replace ALL coordination with NONE coordination
                    when errors are find parsing the _PSD domains
              (as described in the comments in the code).

Hopefully you'll find this useful for ease of maintenance and ease of
future development of the driver.

This functionality was tested on a Juno platform with modified _PSD
tables to test the functionality for all currently supported
coordination types: ANY, HW, NONE.

The current code is based on v5.10-rc2.

Thanks,
Ionela.

[1] https://lore.kernel.org/linux-pm/20200922162540.GB796@arm.com/

Ionela Voinescu (8):
  cppc_cpufreq: fix misspelling, code style and readability issues
  cppc_cpufreq: clean up cpu, cpu_num and cpunum variable use
  cppc_cpufreq: simplify use of performance capabilities
  cppc_cpufreq: replace per-cpu structures with lists
  cppc_cpufreq: use policy->cpu as driver of frequency setting
  cppc_cpufreq: clarify support for coordination types
  cppc_cpufreq: expose information on frequency domains
  acpi: fix NONE coordination for domain mapping failure

 .../ABI/testing/sysfs-devices-system-cpu      |   3 +-
 drivers/acpi/cppc_acpi.c                      | 126 +++---
 drivers/acpi/processor_perflib.c              |   2 +-
 drivers/cpufreq/cppc_cpufreq.c                | 358 +++++++++++-------
 include/acpi/cppc_acpi.h                      |  14 +-
 5 files changed, 277 insertions(+), 226 deletions(-)

-- 
2.17.1

[PATCH 1/8] cppc_cpufreq: fix misspelling, code style and readability issues

[Ionela Voinescu]

Fix a few trivial issues in the cppc_cpufreq driver:

 - indentation of function arguments
 - consistent use of tabs (vs space) in defines
 - spelling: s/Offest/Offset, s/trasition/transition
 - order of local variables, from long pointers to structures to
   short ret and i (index) variables, to improve readability

Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cppc_cpufreq.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
index f29e8d0553a8..0b6058ab695f 100644
--- a/drivers/cpufreq/cppc_cpufreq.c
+++ b/drivers/cpufreq/cppc_cpufreq.c
@@ -26,8 +26,8 @@
 /* Minimum struct length needed for the DMI processor entry we want */
 #define DMI_ENTRY_PROCESSOR_MIN_LENGTH	48
 
-/* Offest in the DMI processor structure for the max frequency */
-#define DMI_PROCESSOR_MAX_SPEED  0x14
+/* Offset in the DMI processor structure for the max frequency */
+#define DMI_PROCESSOR_MAX_SPEED		0x14
 
 /*
  * These structs contain information parsed from per CPU
@@ -97,10 +97,10 @@ static u64 cppc_get_dmi_max_khz(void)
  * For perf/freq > Nominal, we use the ratio perf:freq at Nominal for conversion
  */
 static unsigned int cppc_cpufreq_perf_to_khz(struct cppc_cpudata *cpu,
-					unsigned int perf)
+					     unsigned int perf)
 {
-	static u64 max_khz;
 	struct cppc_perf_caps *caps = &cpu->perf_caps;
+	static u64 max_khz;
 	u64 mul, div;
 
 	if (caps->lowest_freq && caps->nominal_freq) {
@@ -121,10 +121,10 @@ static unsigned int cppc_cpufreq_perf_to_khz(struct cppc_cpudata *cpu,
 }
 
 static unsigned int cppc_cpufreq_khz_to_perf(struct cppc_cpudata *cpu,
-					unsigned int freq)
+					     unsigned int freq)
 {
-	static u64 max_khz;
 	struct cppc_perf_caps *caps = &cpu->perf_caps;
+	static u64 max_khz;
 	u64  mul, div;
 
 	if (caps->lowest_freq && caps->nominal_freq) {
@@ -146,11 +146,11 @@ static unsigned int cppc_cpufreq_khz_to_perf(struct cppc_cpudata *cpu,
 }
 
 static int cppc_cpufreq_set_target(struct cpufreq_policy *policy,
-		unsigned int target_freq,
-		unsigned int relation)
+				   unsigned int target_freq,
+				   unsigned int relation)
 {
-	struct cppc_cpudata *cpu;
 	struct cpufreq_freqs freqs;
+	struct cppc_cpudata *cpu;
 	u32 desired_perf;
 	int ret = 0;
 
@@ -171,7 +171,7 @@ static int cppc_cpufreq_set_target(struct cpufreq_policy *policy,
 
 	if (ret)
 		pr_debug("Failed to set target on CPU:%d. ret:%d\n",
-				cpu->cpu, ret);
+			 cpu->cpu, ret);
 
 	return ret;
 }
@@ -193,13 +193,13 @@ static void cppc_cpufreq_stop_cpu(struct cpufreq_policy *policy)
 	ret = cppc_set_perf(cpu_num, &cpu->perf_ctrls);
 	if (ret)
 		pr_debug("Err setting perf value:%d on CPU:%d. ret:%d\n",
-				cpu->perf_caps.lowest_perf, cpu_num, ret);
+			 cpu->perf_caps.lowest_perf, cpu_num, ret);
 }
 
 /*
  * The PCC subspace describes the rate at which platform can accept commands
  * on the shared PCC channel (including READs which do not count towards freq
- * trasition requests), so ideally we need to use the PCC values as a fallback
+ * transition requests), so ideally we need to use the PCC values as a fallback
  * if we don't have a platform specific transition_delay_us
  */
 #ifdef CONFIG_ARM64
@@ -241,8 +241,8 @@ static unsigned int cppc_cpufreq_get_transition_delay_us(int cpu)
 
 static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
 {
-	struct cppc_cpudata *cpu;
 	unsigned int cpu_num = policy->cpu;
+	struct cppc_cpudata *cpu;
 	int ret = 0;
 
 	cpu = all_cpu_data[policy->cpu];
@@ -252,7 +252,7 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
 
 	if (ret) {
 		pr_debug("Err reading CPU%d perf capabilities. ret:%d\n",
-				cpu_num, ret);
+			 cpu_num, ret);
 		return ret;
 	}
 
@@ -313,7 +313,7 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
 	ret = cppc_set_perf(cpu_num, &cpu->perf_ctrls);
 	if (ret)
 		pr_debug("Err setting perf value:%d on CPU:%d. ret:%d\n",
-				cpu->perf_caps.highest_perf, cpu_num, ret);
+			 cpu->perf_caps.highest_perf, cpu_num, ret);
 
 	return ret;
 }
@@ -450,8 +450,8 @@ static void cppc_check_hisi_workaround(void)
 
 static int __init cppc_cpufreq_init(void)
 {
-	int i, ret = 0;
 	struct cppc_cpudata *cpu;
+	int i, ret = 0;
 
 	if (acpi_disabled)
 		return -ENODEV;
-- 
2.17.1

Re: [PATCH 1/8] cppc_cpufreq: fix misspelling, code style and readability issues

[Viresh Kumar]

On 05-11-20, 12:55, Ionela Voinescu wrote:
> Fix a few trivial issues in the cppc_cpufreq driver:
> 
>  - indentation of function arguments
>  - consistent use of tabs (vs space) in defines
>  - spelling: s/Offest/Offset, s/trasition/transition
>  - order of local variables, from long pointers to structures to
>    short ret and i (index) variables, to improve readability
> 
> Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com>
> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/cppc_cpufreq.c | 32 ++++++++++++++++----------------
>  1 file changed, 16 insertions(+), 16 deletions(-)

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

[PATCH 2/8] cppc_cpufreq: clean up cpu, cpu_num and cpunum variable use

[Ionela Voinescu]

In order to maintain the typical naming convention in the cpufreq
framework:
 - replace the use of "cpu" variable name for cppc_cpudata pointers
   with "cpu_data"
 - replace variable names "cpu_num" and "cpunum" with "cpu"
 - make cpu variables unsigned int

Where pertinent, also move the initialisation of cpu_data variable to
its declaration and make consistent use of the local "cpu" variable.

Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cppc_cpufreq.c | 143 ++++++++++++++++-----------------
 1 file changed, 69 insertions(+), 74 deletions(-)

diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
index 0b6058ab695f..317169453549 100644
--- a/drivers/cpufreq/cppc_cpufreq.c
+++ b/drivers/cpufreq/cppc_cpufreq.c
@@ -96,10 +96,10 @@ static u64 cppc_get_dmi_max_khz(void)
  * and extrapolate the rest
  * For perf/freq > Nominal, we use the ratio perf:freq at Nominal for conversion
  */
-static unsigned int cppc_cpufreq_perf_to_khz(struct cppc_cpudata *cpu,
+static unsigned int cppc_cpufreq_perf_to_khz(struct cppc_cpudata *cpu_data,
 					     unsigned int perf)
 {
-	struct cppc_perf_caps *caps = &cpu->perf_caps;
+	struct cppc_perf_caps *caps = &cpu_data->perf_caps;
 	static u64 max_khz;
 	u64 mul, div;
 
@@ -120,10 +120,10 @@ static unsigned int cppc_cpufreq_perf_to_khz(struct cppc_cpudata *cpu,
 	return (u64)perf * mul / div;
 }
 
-static unsigned int cppc_cpufreq_khz_to_perf(struct cppc_cpudata *cpu,
+static unsigned int cppc_cpufreq_khz_to_perf(struct cppc_cpudata *cpu_data,
 					     unsigned int freq)
 {
-	struct cppc_perf_caps *caps = &cpu->perf_caps;
+	struct cppc_perf_caps *caps = &cpu_data->perf_caps;
 	static u64 max_khz;
 	u64  mul, div;
 
@@ -149,29 +149,27 @@ static int cppc_cpufreq_set_target(struct cpufreq_policy *policy,
 				   unsigned int target_freq,
 				   unsigned int relation)
 {
+	struct cppc_cpudata *cpu_data = all_cpu_data[policy->cpu];
 	struct cpufreq_freqs freqs;
-	struct cppc_cpudata *cpu;
 	u32 desired_perf;
 	int ret = 0;
 
-	cpu = all_cpu_data[policy->cpu];
-
-	desired_perf = cppc_cpufreq_khz_to_perf(cpu, target_freq);
+	desired_perf = cppc_cpufreq_khz_to_perf(cpu_data, target_freq);
 	/* Return if it is exactly the same perf */
-	if (desired_perf == cpu->perf_ctrls.desired_perf)
+	if (desired_perf == cpu_data->perf_ctrls.desired_perf)
 		return ret;
 
-	cpu->perf_ctrls.desired_perf = desired_perf;
+	cpu_data->perf_ctrls.desired_perf = desired_perf;
 	freqs.old = policy->cur;
 	freqs.new = target_freq;
 
 	cpufreq_freq_transition_begin(policy, &freqs);
-	ret = cppc_set_perf(cpu->cpu, &cpu->perf_ctrls);
+	ret = cppc_set_perf(cpu_data->cpu, &cpu_data->perf_ctrls);
 	cpufreq_freq_transition_end(policy, &freqs, ret != 0);
 
 	if (ret)
 		pr_debug("Failed to set target on CPU:%d. ret:%d\n",
-			 cpu->cpu, ret);
+			 cpu_data->cpu, ret);
 
 	return ret;
 }
@@ -184,16 +182,16 @@ static int cppc_verify_policy(struct cpufreq_policy_data *policy)
 
 static void cppc_cpufreq_stop_cpu(struct cpufreq_policy *policy)
 {
-	int cpu_num = policy->cpu;
-	struct cppc_cpudata *cpu = all_cpu_data[cpu_num];
+	struct cppc_cpudata *cpu_data = all_cpu_data[policy->cpu];
+	unsigned int cpu = policy->cpu;
 	int ret;
 
-	cpu->perf_ctrls.desired_perf = cpu->perf_caps.lowest_perf;
+	cpu_data->perf_ctrls.desired_perf = cpu_data->perf_caps.lowest_perf;
 
-	ret = cppc_set_perf(cpu_num, &cpu->perf_ctrls);
+	ret = cppc_set_perf(cpu, &cpu_data->perf_ctrls);
 	if (ret)
 		pr_debug("Err setting perf value:%d on CPU:%d. ret:%d\n",
-			 cpu->perf_caps.lowest_perf, cpu_num, ret);
+			 cpu_data->perf_caps.lowest_perf, cpu, ret);
 }
 
 /*
@@ -205,7 +203,7 @@ static void cppc_cpufreq_stop_cpu(struct cpufreq_policy *policy)
 #ifdef CONFIG_ARM64
 #include <asm/cputype.h>
 
-static unsigned int cppc_cpufreq_get_transition_delay_us(int cpu)
+static unsigned int cppc_cpufreq_get_transition_delay_us(unsigned int cpu)
 {
 	unsigned long implementor = read_cpuid_implementor();
 	unsigned long part_num = read_cpuid_part_number();
@@ -233,7 +231,7 @@ static unsigned int cppc_cpufreq_get_transition_delay_us(int cpu)
 
 #else
 
-static unsigned int cppc_cpufreq_get_transition_delay_us(int cpu)
+static unsigned int cppc_cpufreq_get_transition_delay_us(unsigned int cpu)
 {
 	return cppc_get_transition_latency(cpu) / NSEC_PER_USEC;
 }
@@ -241,54 +239,52 @@ static unsigned int cppc_cpufreq_get_transition_delay_us(int cpu)
 
 static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
 {
-	unsigned int cpu_num = policy->cpu;
-	struct cppc_cpudata *cpu;
+	struct cppc_cpudata *cpu_data = all_cpu_data[policy->cpu];
+	unsigned int cpu = policy->cpu;
 	int ret = 0;
 
-	cpu = all_cpu_data[policy->cpu];
-
-	cpu->cpu = cpu_num;
-	ret = cppc_get_perf_caps(policy->cpu, &cpu->perf_caps);
+	cpu_data->cpu = cpu;
+	ret = cppc_get_perf_caps(cpu, &cpu_data->perf_caps);
 
 	if (ret) {
 		pr_debug("Err reading CPU%d perf capabilities. ret:%d\n",
-			 cpu_num, ret);
+			 cpu, ret);
 		return ret;
 	}
 
 	/* Convert the lowest and nominal freq from MHz to KHz */
-	cpu->perf_caps.lowest_freq *= 1000;
-	cpu->perf_caps.nominal_freq *= 1000;
+	cpu_data->perf_caps.lowest_freq *= 1000;
+	cpu_data->perf_caps.nominal_freq *= 1000;
 
 	/*
 	 * Set min to lowest nonlinear perf to avoid any efficiency penalty (see
 	 * Section 8.4.7.1.1.5 of ACPI 6.1 spec)
 	 */
-	policy->min = cppc_cpufreq_perf_to_khz(cpu, cpu->perf_caps.lowest_nonlinear_perf);
-	policy->max = cppc_cpufreq_perf_to_khz(cpu, cpu->perf_caps.nominal_perf);
+	policy->min = cppc_cpufreq_perf_to_khz(cpu_data, cpu_data->perf_caps.lowest_nonlinear_perf);
+	policy->max = cppc_cpufreq_perf_to_khz(cpu_data, cpu_data->perf_caps.nominal_perf);
 
 	/*
 	 * Set cpuinfo.min_freq to Lowest to make the full range of performance
 	 * available if userspace wants to use any perf between lowest & lowest
 	 * nonlinear perf
 	 */
-	policy->cpuinfo.min_freq = cppc_cpufreq_perf_to_khz(cpu, cpu->perf_caps.lowest_perf);
-	policy->cpuinfo.max_freq = cppc_cpufreq_perf_to_khz(cpu, cpu->perf_caps.nominal_perf);
+	policy->cpuinfo.min_freq = cppc_cpufreq_perf_to_khz(cpu_data, cpu_data->perf_caps.lowest_perf);
+	policy->cpuinfo.max_freq = cppc_cpufreq_perf_to_khz(cpu_data, cpu_data->perf_caps.nominal_perf);
 
-	policy->transition_delay_us = cppc_cpufreq_get_transition_delay_us(cpu_num);
-	policy->shared_type = cpu->shared_type;
+	policy->transition_delay_us = cppc_cpufreq_get_transition_delay_us(cpu);
+	policy->shared_type = cpu_data->shared_type;
 
 	if (policy->shared_type == CPUFREQ_SHARED_TYPE_ANY) {
 		int i;
 
-		cpumask_copy(policy->cpus, cpu->shared_cpu_map);
+		cpumask_copy(policy->cpus, cpu_data->shared_cpu_map);
 
 		for_each_cpu(i, policy->cpus) {
-			if (unlikely(i == policy->cpu))
+			if (unlikely(i == cpu))
 				continue;
 
-			memcpy(&all_cpu_data[i]->perf_caps, &cpu->perf_caps,
-			       sizeof(cpu->perf_caps));
+			memcpy(&all_cpu_data[i]->perf_caps, &cpu_data->perf_caps,
+			       sizeof(cpu_data->perf_caps));
 		}
 	} else if (policy->shared_type == CPUFREQ_SHARED_TYPE_ALL) {
 		/* Support only SW_ANY for now. */
@@ -296,24 +292,24 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
 		return -EFAULT;
 	}
 
-	cpu->cur_policy = policy;
+	cpu_data->cur_policy = policy;
 
 	/*
 	 * If 'highest_perf' is greater than 'nominal_perf', we assume CPU Boost
 	 * is supported.
 	 */
-	if (cpu->perf_caps.highest_perf > cpu->perf_caps.nominal_perf)
+	if (cpu_data->perf_caps.highest_perf > cpu_data->perf_caps.nominal_perf)
 		boost_supported = true;
 
 	/* Set policy->cur to max now. The governors will adjust later. */
-	policy->cur = cppc_cpufreq_perf_to_khz(cpu,
-					cpu->perf_caps.highest_perf);
-	cpu->perf_ctrls.desired_perf = cpu->perf_caps.highest_perf;
+	policy->cur = cppc_cpufreq_perf_to_khz(cpu_data,
+					cpu_data->perf_caps.highest_perf);
+	cpu_data->perf_ctrls.desired_perf = cpu_data->perf_caps.highest_perf;
 
-	ret = cppc_set_perf(cpu_num, &cpu->perf_ctrls);
+	ret = cppc_set_perf(cpu, &cpu_data->perf_ctrls);
 	if (ret)
 		pr_debug("Err setting perf value:%d on CPU:%d. ret:%d\n",
-			 cpu->perf_caps.highest_perf, cpu_num, ret);
+			 cpu_data->perf_caps.highest_perf, cpu, ret);
 
 	return ret;
 }
@@ -326,7 +322,7 @@ static inline u64 get_delta(u64 t1, u64 t0)
 	return (u32)t1 - (u32)t0;
 }
 
-static int cppc_get_rate_from_fbctrs(struct cppc_cpudata *cpu,
+static int cppc_get_rate_from_fbctrs(struct cppc_cpudata *cpu_data,
 				     struct cppc_perf_fb_ctrs fb_ctrs_t0,
 				     struct cppc_perf_fb_ctrs fb_ctrs_t1)
 {
@@ -345,33 +341,33 @@ static int cppc_get_rate_from_fbctrs(struct cppc_cpudata *cpu,
 		delivered_perf = (reference_perf * delta_delivered) /
 					delta_reference;
 	else
-		delivered_perf = cpu->perf_ctrls.desired_perf;
+		delivered_perf = cpu_data->perf_ctrls.desired_perf;
 
-	return cppc_cpufreq_perf_to_khz(cpu, delivered_perf);
+	return cppc_cpufreq_perf_to_khz(cpu_data, delivered_perf);
 }
 
-static unsigned int cppc_cpufreq_get_rate(unsigned int cpunum)
+static unsigned int cppc_cpufreq_get_rate(unsigned int cpu)
 {
 	struct cppc_perf_fb_ctrs fb_ctrs_t0 = {0}, fb_ctrs_t1 = {0};
-	struct cppc_cpudata *cpu = all_cpu_data[cpunum];
+	struct cppc_cpudata *cpu_data = all_cpu_data[cpu];
 	int ret;
 
-	ret = cppc_get_perf_ctrs(cpunum, &fb_ctrs_t0);
+	ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t0);
 	if (ret)
 		return ret;
 
 	udelay(2); /* 2usec delay between sampling */
 
-	ret = cppc_get_perf_ctrs(cpunum, &fb_ctrs_t1);
+	ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t1);
 	if (ret)
 		return ret;
 
-	return cppc_get_rate_from_fbctrs(cpu, fb_ctrs_t0, fb_ctrs_t1);
+	return cppc_get_rate_from_fbctrs(cpu_data, fb_ctrs_t0, fb_ctrs_t1);
 }
 
 static int cppc_cpufreq_set_boost(struct cpufreq_policy *policy, int state)
 {
-	struct cppc_cpudata *cpudata;
+	struct cppc_cpudata *cpu_data = all_cpu_data[policy->cpu];
 	int ret;
 
 	if (!boost_supported) {
@@ -379,13 +375,12 @@ static int cppc_cpufreq_set_boost(struct cpufreq_policy *policy, int state)
 		return -EINVAL;
 	}
 
-	cpudata = all_cpu_data[policy->cpu];
 	if (state)
-		policy->max = cppc_cpufreq_perf_to_khz(cpudata,
-					cpudata->perf_caps.highest_perf);
+		policy->max = cppc_cpufreq_perf_to_khz(cpu_data,
+					cpu_data->perf_caps.highest_perf);
 	else
-		policy->max = cppc_cpufreq_perf_to_khz(cpudata,
-					cpudata->perf_caps.nominal_perf);
+		policy->max = cppc_cpufreq_perf_to_khz(cpu_data,
+					cpu_data->perf_caps.nominal_perf);
 	policy->cpuinfo.max_freq = policy->max;
 
 	ret = freq_qos_update_request(policy->max_freq_req, policy->max);
@@ -412,17 +407,17 @@ static struct cpufreq_driver cppc_cpufreq_driver = {
  * platform specific mechanism. We reuse the desired performance register to
  * store the real performance calculated by the platform.
  */
-static unsigned int hisi_cppc_cpufreq_get_rate(unsigned int cpunum)
+static unsigned int hisi_cppc_cpufreq_get_rate(unsigned int cpu)
 {
-	struct cppc_cpudata *cpudata = all_cpu_data[cpunum];
+	struct cppc_cpudata *cpu_data = all_cpu_data[cpu];
 	u64 desired_perf;
 	int ret;
 
-	ret = cppc_get_desired_perf(cpunum, &desired_perf);
+	ret = cppc_get_desired_perf(cpu, &desired_perf);
 	if (ret < 0)
 		return -EIO;
 
-	return cppc_cpufreq_perf_to_khz(cpudata, desired_perf);
+	return cppc_cpufreq_perf_to_khz(cpu_data, desired_perf);
 }
 
 static void cppc_check_hisi_workaround(void)
@@ -450,7 +445,7 @@ static void cppc_check_hisi_workaround(void)
 
 static int __init cppc_cpufreq_init(void)
 {
-	struct cppc_cpudata *cpu;
+	struct cppc_cpudata *cpu_data;
 	int i, ret = 0;
 
 	if (acpi_disabled)
@@ -466,8 +461,8 @@ static int __init cppc_cpufreq_init(void)
 		if (!all_cpu_data[i])
 			goto out;
 
-		cpu = all_cpu_data[i];
-		if (!zalloc_cpumask_var(&cpu->shared_cpu_map, GFP_KERNEL))
+		cpu_data = all_cpu_data[i];
+		if (!zalloc_cpumask_var(&cpu_data->shared_cpu_map, GFP_KERNEL))
 			goto out;
 	}
 
@@ -487,11 +482,11 @@ static int __init cppc_cpufreq_init(void)
 
 out:
 	for_each_possible_cpu(i) {
-		cpu = all_cpu_data[i];
-		if (!cpu)
+		cpu_data = all_cpu_data[i];
+		if (!cpu_data)
 			break;
-		free_cpumask_var(cpu->shared_cpu_map);
-		kfree(cpu);
+		free_cpumask_var(cpu_data->shared_cpu_map);
+		kfree(cpu_data);
 	}
 
 	kfree(all_cpu_data);
@@ -500,15 +495,15 @@ static int __init cppc_cpufreq_init(void)
 
 static void __exit cppc_cpufreq_exit(void)
 {
-	struct cppc_cpudata *cpu;
+	struct cppc_cpudata *cpu_data;
 	int i;
 
 	cpufreq_unregister_driver(&cppc_cpufreq_driver);
 
 	for_each_possible_cpu(i) {
-		cpu = all_cpu_data[i];
-		free_cpumask_var(cpu->shared_cpu_map);
-		kfree(cpu);
+		cpu_data = all_cpu_data[i];
+		free_cpumask_var(cpu_data->shared_cpu_map);
+		kfree(cpu_data);
 	}
 
 	kfree(all_cpu_data);
-- 
2.17.1

Re: [PATCH 2/8] cppc_cpufreq: clean up cpu, cpu_num and cpunum variable use

[Viresh Kumar]

On 05-11-20, 12:55, Ionela Voinescu wrote:
> In order to maintain the typical naming convention in the cpufreq
> framework:
>  - replace the use of "cpu" variable name for cppc_cpudata pointers
>    with "cpu_data"
>  - replace variable names "cpu_num" and "cpunum" with "cpu"
>  - make cpu variables unsigned int
> 
> Where pertinent, also move the initialisation of cpu_data variable to
> its declaration and make consistent use of the local "cpu" variable.
> 
> Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com>
> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/cppc_cpufreq.c | 143 ++++++++++++++++-----------------
>  1 file changed, 69 insertions(+), 74 deletions(-)

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

[PATCH 3/8] cppc_cpufreq: simplify use of performance capabilities

[Ionela Voinescu]

The CPPC performance capabilities are used significantly throughout
the driver. Simplify the use of them by introducing a local pointer
"caps" to point to cpu_data->perf_caps, in functions that access
performance capabilities often.

Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cppc_cpufreq.c | 40 +++++++++++++++++++---------------
 1 file changed, 23 insertions(+), 17 deletions(-)

diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
index 317169453549..7cc9bd8568de 100644
--- a/drivers/cpufreq/cppc_cpufreq.c
+++ b/drivers/cpufreq/cppc_cpufreq.c
@@ -183,15 +183,16 @@ static int cppc_verify_policy(struct cpufreq_policy_data *policy)
 static void cppc_cpufreq_stop_cpu(struct cpufreq_policy *policy)
 {
 	struct cppc_cpudata *cpu_data = all_cpu_data[policy->cpu];
+	struct cppc_perf_caps *caps = &cpu_data->perf_caps;
 	unsigned int cpu = policy->cpu;
 	int ret;
 
-	cpu_data->perf_ctrls.desired_perf = cpu_data->perf_caps.lowest_perf;
+	cpu_data->perf_ctrls.desired_perf = caps->lowest_perf;
 
 	ret = cppc_set_perf(cpu, &cpu_data->perf_ctrls);
 	if (ret)
 		pr_debug("Err setting perf value:%d on CPU:%d. ret:%d\n",
-			 cpu_data->perf_caps.lowest_perf, cpu, ret);
+			 caps->lowest_perf, cpu, ret);
 }
 
 /*
@@ -240,11 +241,12 @@ static unsigned int cppc_cpufreq_get_transition_delay_us(unsigned int cpu)
 static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
 {
 	struct cppc_cpudata *cpu_data = all_cpu_data[policy->cpu];
+	struct cppc_perf_caps *caps = &cpu_data->perf_caps;
 	unsigned int cpu = policy->cpu;
 	int ret = 0;
 
 	cpu_data->cpu = cpu;
-	ret = cppc_get_perf_caps(cpu, &cpu_data->perf_caps);
+	ret = cppc_get_perf_caps(cpu, caps);
 
 	if (ret) {
 		pr_debug("Err reading CPU%d perf capabilities. ret:%d\n",
@@ -253,23 +255,27 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
 	}
 
 	/* Convert the lowest and nominal freq from MHz to KHz */
-	cpu_data->perf_caps.lowest_freq *= 1000;
-	cpu_data->perf_caps.nominal_freq *= 1000;
+	caps->lowest_freq *= 1000;
+	caps->nominal_freq *= 1000;
 
 	/*
 	 * Set min to lowest nonlinear perf to avoid any efficiency penalty (see
 	 * Section 8.4.7.1.1.5 of ACPI 6.1 spec)
 	 */
-	policy->min = cppc_cpufreq_perf_to_khz(cpu_data, cpu_data->perf_caps.lowest_nonlinear_perf);
-	policy->max = cppc_cpufreq_perf_to_khz(cpu_data, cpu_data->perf_caps.nominal_perf);
+	policy->min = cppc_cpufreq_perf_to_khz(cpu_data,
+					       caps->lowest_nonlinear_perf);
+	policy->max = cppc_cpufreq_perf_to_khz(cpu_data,
+					       caps->nominal_perf);
 
 	/*
 	 * Set cpuinfo.min_freq to Lowest to make the full range of performance
 	 * available if userspace wants to use any perf between lowest & lowest
 	 * nonlinear perf
 	 */
-	policy->cpuinfo.min_freq = cppc_cpufreq_perf_to_khz(cpu_data, cpu_data->perf_caps.lowest_perf);
-	policy->cpuinfo.max_freq = cppc_cpufreq_perf_to_khz(cpu_data, cpu_data->perf_caps.nominal_perf);
+	policy->cpuinfo.min_freq = cppc_cpufreq_perf_to_khz(cpu_data,
+							    caps->lowest_perf);
+	policy->cpuinfo.max_freq = cppc_cpufreq_perf_to_khz(cpu_data,
+							    caps->nominal_perf);
 
 	policy->transition_delay_us = cppc_cpufreq_get_transition_delay_us(cpu);
 	policy->shared_type = cpu_data->shared_type;
@@ -283,7 +289,7 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
 			if (unlikely(i == cpu))
 				continue;
 
-			memcpy(&all_cpu_data[i]->perf_caps, &cpu_data->perf_caps,
+			memcpy(&all_cpu_data[i]->perf_caps, caps,
 			       sizeof(cpu_data->perf_caps));
 		}
 	} else if (policy->shared_type == CPUFREQ_SHARED_TYPE_ALL) {
@@ -298,18 +304,17 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
 	 * If 'highest_perf' is greater than 'nominal_perf', we assume CPU Boost
 	 * is supported.
 	 */
-	if (cpu_data->perf_caps.highest_perf > cpu_data->perf_caps.nominal_perf)
+	if (caps->highest_perf > caps->nominal_perf)
 		boost_supported = true;
 
 	/* Set policy->cur to max now. The governors will adjust later. */
-	policy->cur = cppc_cpufreq_perf_to_khz(cpu_data,
-					cpu_data->perf_caps.highest_perf);
-	cpu_data->perf_ctrls.desired_perf = cpu_data->perf_caps.highest_perf;
+	policy->cur = cppc_cpufreq_perf_to_khz(cpu_data, caps->highest_perf);
+	cpu_data->perf_ctrls.desired_perf =  caps->highest_perf;
 
 	ret = cppc_set_perf(cpu, &cpu_data->perf_ctrls);
 	if (ret)
 		pr_debug("Err setting perf value:%d on CPU:%d. ret:%d\n",
-			 cpu_data->perf_caps.highest_perf, cpu, ret);
+			 caps->highest_perf, cpu, ret);
 
 	return ret;
 }
@@ -368,6 +373,7 @@ static unsigned int cppc_cpufreq_get_rate(unsigned int cpu)
 static int cppc_cpufreq_set_boost(struct cpufreq_policy *policy, int state)
 {
 	struct cppc_cpudata *cpu_data = all_cpu_data[policy->cpu];
+	struct cppc_perf_caps *caps = &cpu_data->perf_caps;
 	int ret;
 
 	if (!boost_supported) {
@@ -377,10 +383,10 @@ static int cppc_cpufreq_set_boost(struct cpufreq_policy *policy, int state)
 
 	if (state)
 		policy->max = cppc_cpufreq_perf_to_khz(cpu_data,
-					cpu_data->perf_caps.highest_perf);
+						       caps->highest_perf);
 	else
 		policy->max = cppc_cpufreq_perf_to_khz(cpu_data,
-					cpu_data->perf_caps.nominal_perf);
+						       caps->nominal_perf);
 	policy->cpuinfo.max_freq = policy->max;
 
 	ret = freq_qos_update_request(policy->max_freq_req, policy->max);
-- 
2.17.1

Re: [PATCH 3/8] cppc_cpufreq: simplify use of performance capabilities

[Viresh Kumar]

On 05-11-20, 12:55, Ionela Voinescu wrote:
> The CPPC performance capabilities are used significantly throughout
> the driver. Simplify the use of them by introducing a local pointer
> "caps" to point to cpu_data->perf_caps, in functions that access
> performance capabilities often.
> 
> Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com>
> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/cppc_cpufreq.c | 40 +++++++++++++++++++---------------
>  1 file changed, 23 insertions(+), 17 deletions(-)

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

[PATCH 4/8] cppc_cpufreq: replace per-cpu structures with lists

[Ionela Voinescu]

The cppc_cpudata per-cpu storage was inefficient (1) additional to causing
functional issues (2) when CPUs are hotplugged out, due to per-cpu data
being improperly initialised.

(1) The amount of information needed for CPPC performance control in its
    cpufreq driver depends on the domain (PSD) coordination type:

    ANY:    One set of CPPC control and capability data (e.g desired
            performance, highest/lowest performance, etc) applies to all
            CPUs in the domain.

    ALL:    Same as ANY. To be noted that this type is not currently
            supported. When supported, information about which CPUs
            belong to a domain is needed in order for frequency change
            requests to be sent to each of them.

    HW:     It's necessary to store CPPC control and capability
            information for all the CPUs. HW will then coordinate the
            performance state based on their limitations and requests.

    NONE:   Same as HW. No HW coordination is expected.

    Despite this, the previous initialisation code would indiscriminately
    allocate memory for all CPUs (all_cpu_data) and unnecessarily
    duplicate performance capabilities and the domain sharing mask and type
    for each possible CPU.

(2) With the current per-cpu structure, when having ANY coordination,
    the cppc_cpudata cpu information is not initialised (will remain 0)
    for all CPUs in a policy, other than policy->cpu. When policy->cpu is
    hotplugged out, the driver will incorrectly use the uninitialised (0)
    value of the other CPUs when making frequency changes. Additionally,
    the previous values stored in the perf_ctrls.desired_perf will be
    lost when policy->cpu changes.

Due to the current storage of driver data not being fit for purpose,
replace the per-cpu CPPC/PSD data with a list of domains which in turn
will point to a list of CPUs with their controls and capabilities,
belonging to the domain.

This new structure has the following benefits:
 - Clearly separates PSD (domain) data from CPPC (performance monitoring
   and control) data and stores one mask of CPUs belonging to a domain
   and its type in a single structure, for each domain.

 - For ANY domain coordination, a single cppc_cpudata set of capabilities
   and controls are stored, for policy->cpu, and used for describing
   performance controls and capabilities even when policy->cpu changes as
   a result of hotplug. All the CPUs in the domain will belong to the
   same policy.

 - For HW or lack of coordination, a full map of domains and CPUs is
   obtained. Each CPU will have its own policy, but for HW, as a result
   of PSD information, a shared_cpu_map mask could be used to identify
   the domain CPU content.

Additional changes:

 - A pointer to the policy's cppc_cpudata is stored in policy->driver_data

 - All data allocation is done from the driver's init function. At that
   point the domain is either created or retrieved. For this purpose
   acpi_get_psd_map() was changed to create a map of the domain of a
   single CPU, rather than the full system domain map.

 - When CPU's are hotplugged out and in, old information can be retrieved.

Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Len Brown <lenb@kernel.org>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/acpi/cppc_acpi.c       | 126 +++++++------------
 drivers/cpufreq/cppc_cpufreq.c | 217 ++++++++++++++++++++-------------
 include/acpi/cppc_acpi.h       |  14 ++-
 3 files changed, 190 insertions(+), 167 deletions(-)

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index 7a99b19bb893..75e36b909ae6 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -414,108 +414,72 @@ static int acpi_get_psd(struct cpc_desc *cpc_ptr, acpi_handle handle)
 }
 
 /**
- * acpi_get_psd_map - Map the CPUs in a common freq domain.
- * @all_cpu_data: Ptrs to CPU specific CPPC data including PSD info.
+ * acpi_get_psd_map - Map the CPUs in the freq domain of a given cpu
+ * @cpu: Find all CPUs that share a domain with cpu.
+ * @domain: Pointer to given domain data storage
  *
  *	Return: 0 for success or negative value for err.
  */
-int acpi_get_psd_map(struct cppc_cpudata **all_cpu_data)
+int acpi_get_psd_map(unsigned int cpu, struct psd_data *domain)
 {
-	int count_target;
-	int retval = 0;
-	unsigned int i, j;
-	cpumask_var_t covered_cpus;
-	struct cppc_cpudata *pr, *match_pr;
-	struct acpi_psd_package *pdomain;
-	struct acpi_psd_package *match_pdomain;
 	struct cpc_desc *cpc_ptr, *match_cpc_ptr;
-
-	if (!zalloc_cpumask_var(&covered_cpus, GFP_KERNEL))
-		return -ENOMEM;
+	struct acpi_psd_package *match_pdomain;
+	struct acpi_psd_package *pdomain;
+	int count_target, i;
 
 	/*
 	 * Now that we have _PSD data from all CPUs, let's setup P-state
 	 * domain info.
 	 */
-	for_each_possible_cpu(i) {
-		if (cpumask_test_cpu(i, covered_cpus))
-			continue;
+	cpc_ptr = per_cpu(cpc_desc_ptr, cpu);
+	if (!cpc_ptr)
+		return -EFAULT;
 
-		pr = all_cpu_data[i];
-		cpc_ptr = per_cpu(cpc_desc_ptr, i);
-		if (!cpc_ptr) {
-			retval = -EFAULT;
-			goto err_ret;
-		}
+	pdomain = &(cpc_ptr->domain_info);
+	cpumask_set_cpu(cpu, domain->shared_cpu_map);
+	if (pdomain->num_processors <= 1)
+		return 0;
 
-		pdomain = &(cpc_ptr->domain_info);
-		cpumask_set_cpu(i, pr->shared_cpu_map);
-		cpumask_set_cpu(i, covered_cpus);
-		if (pdomain->num_processors <= 1)
-			continue;
+	/* Validate the Domain info */
+	count_target = pdomain->num_processors;
+	if (pdomain->coord_type == DOMAIN_COORD_TYPE_SW_ALL)
+		domain->shared_type = CPUFREQ_SHARED_TYPE_ALL;
+	else if (pdomain->coord_type == DOMAIN_COORD_TYPE_HW_ALL)
+		domain->shared_type = CPUFREQ_SHARED_TYPE_HW;
+	else if (pdomain->coord_type == DOMAIN_COORD_TYPE_SW_ANY)
+		domain->shared_type = CPUFREQ_SHARED_TYPE_ANY;
 
-		/* Validate the Domain info */
-		count_target = pdomain->num_processors;
-		if (pdomain->coord_type == DOMAIN_COORD_TYPE_SW_ALL)
-			pr->shared_type = CPUFREQ_SHARED_TYPE_ALL;
-		else if (pdomain->coord_type == DOMAIN_COORD_TYPE_HW_ALL)
-			pr->shared_type = CPUFREQ_SHARED_TYPE_HW;
-		else if (pdomain->coord_type == DOMAIN_COORD_TYPE_SW_ANY)
-			pr->shared_type = CPUFREQ_SHARED_TYPE_ANY;
-
-		for_each_possible_cpu(j) {
-			if (i == j)
-				continue;
-
-			match_cpc_ptr = per_cpu(cpc_desc_ptr, j);
-			if (!match_cpc_ptr) {
-				retval = -EFAULT;
-				goto err_ret;
-			}
-
-			match_pdomain = &(match_cpc_ptr->domain_info);
-			if (match_pdomain->domain != pdomain->domain)
-				continue;
+	for_each_possible_cpu(i) {
+		if (i == cpu)
+			continue;
 
-			/* Here i and j are in the same domain */
-			if (match_pdomain->num_processors != count_target) {
-				retval = -EFAULT;
-				goto err_ret;
-			}
+		match_cpc_ptr = per_cpu(cpc_desc_ptr, i);
+		if (!match_cpc_ptr)
+			goto err_fault;
 
-			if (pdomain->coord_type != match_pdomain->coord_type) {
-				retval = -EFAULT;
-				goto err_ret;
-			}
+		match_pdomain = &(match_cpc_ptr->domain_info);
+		if (match_pdomain->domain != pdomain->domain)
+			continue;
 
-			cpumask_set_cpu(j, covered_cpus);
-			cpumask_set_cpu(j, pr->shared_cpu_map);
-		}
+		/* Here i and cpu are in the same domain */
+		if (match_pdomain->num_processors != count_target)
+			goto err_fault;
 
-		for_each_cpu(j, pr->shared_cpu_map) {
-			if (i == j)
-				continue;
+		if (pdomain->coord_type != match_pdomain->coord_type)
+			goto err_fault;
 
-			match_pr = all_cpu_data[j];
-			match_pr->shared_type = pr->shared_type;
-			cpumask_copy(match_pr->shared_cpu_map,
-				     pr->shared_cpu_map);
-		}
+		cpumask_set_cpu(i, domain->shared_cpu_map);
 	}
-	goto out;
 
-err_ret:
-	for_each_possible_cpu(i) {
-		pr = all_cpu_data[i];
+	return 0;
 
-		/* Assume no coordination on any error parsing domain info */
-		cpumask_clear(pr->shared_cpu_map);
-		cpumask_set_cpu(i, pr->shared_cpu_map);
-		pr->shared_type = CPUFREQ_SHARED_TYPE_ALL;
-	}
-out:
-	free_cpumask_var(covered_cpus);
-	return retval;
+err_fault:
+	/* Assume no coordination on any error parsing domain info */
+	cpumask_clear(domain->shared_cpu_map);
+	cpumask_set_cpu(cpu, domain->shared_cpu_map);
+	domain->shared_type = CPUFREQ_SHARED_TYPE_ALL;
+
+	return -EFAULT;
 }
 EXPORT_SYMBOL_GPL(acpi_get_psd_map);
 
diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
index 7cc9bd8568de..ac95b4424a2e 100644
--- a/drivers/cpufreq/cppc_cpufreq.c
+++ b/drivers/cpufreq/cppc_cpufreq.c
@@ -30,13 +30,12 @@
 #define DMI_PROCESSOR_MAX_SPEED		0x14
 
 /*
- * These structs contain information parsed from per CPU
- * ACPI _CPC structures.
- * e.g. For each CPU the highest, lowest supported
- * performance capabilities, desired performance level
- * requested etc.
+ * This list contains information parsed from per CPU ACPI _CPC and _PSD
+ * structures: this is a list of domain data which in turn contains a list
+ * of cpus with their controls and capabilities, belonging to the domain.
  */
-static struct cppc_cpudata **all_cpu_data;
+static LIST_HEAD(domain_list);
+
 static bool boost_supported;
 
 struct cppc_workaround_oem_info {
@@ -148,8 +147,9 @@ static unsigned int cppc_cpufreq_khz_to_perf(struct cppc_cpudata *cpu_data,
 static int cppc_cpufreq_set_target(struct cpufreq_policy *policy,
 				   unsigned int target_freq,
 				   unsigned int relation)
+
 {
-	struct cppc_cpudata *cpu_data = all_cpu_data[policy->cpu];
+	struct cppc_cpudata *cpu_data = policy->driver_data;
 	struct cpufreq_freqs freqs;
 	u32 desired_perf;
 	int ret = 0;
@@ -182,7 +182,7 @@ static int cppc_verify_policy(struct cpufreq_policy_data *policy)
 
 static void cppc_cpufreq_stop_cpu(struct cpufreq_policy *policy)
 {
-	struct cppc_cpudata *cpu_data = all_cpu_data[policy->cpu];
+	struct cppc_cpudata *cpu_data = policy->driver_data;
 	struct cppc_perf_caps *caps = &cpu_data->perf_caps;
 	unsigned int cpu = policy->cpu;
 	int ret;
@@ -238,25 +238,107 @@ static unsigned int cppc_cpufreq_get_transition_delay_us(unsigned int cpu)
 }
 #endif
 
-static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
+static struct psd_data *cppc_cpufreq_get_domain(unsigned int cpu)
 {
-	struct cppc_cpudata *cpu_data = all_cpu_data[policy->cpu];
-	struct cppc_perf_caps *caps = &cpu_data->perf_caps;
-	unsigned int cpu = policy->cpu;
-	int ret = 0;
+	struct psd_data *domain;
+	int ret;
 
-	cpu_data->cpu = cpu;
-	ret = cppc_get_perf_caps(cpu, caps);
+	list_for_each_entry(domain, &domain_list, node) {
+		if (cpumask_test_cpu(cpu, domain->shared_cpu_map))
+			return domain;
+	}
 
+	domain = kzalloc(sizeof(struct psd_data), GFP_KERNEL);
+	if (!domain)
+		return NULL;
+	if (!zalloc_cpumask_var(&domain->shared_cpu_map, GFP_KERNEL))
+		goto free_domain;
+	INIT_LIST_HEAD(&domain->cpu_list);
+
+	ret = acpi_get_psd_map(cpu, domain);
 	if (ret) {
-		pr_debug("Err reading CPU%d perf capabilities. ret:%d\n",
-			 cpu, ret);
-		return ret;
+		pr_err("Error parsing PSD data for CPU%d.\n", cpu);
+		goto free_mask;
+	}
+
+	list_add(&domain->node, &domain_list);
+
+	return domain;
+
+free_mask:
+	free_cpumask_var(domain->shared_cpu_map);
+free_domain:
+	kfree(domain);
+
+	return NULL;
+}
+
+static struct cppc_cpudata *cppc_cpufreq_get_cpu_data(unsigned int cpu)
+{
+	struct cppc_cpudata *cpu_data;
+	struct psd_data *domain;
+	int ret;
+
+	domain = cppc_cpufreq_get_domain(cpu);
+	if (!domain) {
+		pr_err("Error acquiring domain for CPU.\n");
+		return NULL;
 	}
 
+	list_for_each_entry(cpu_data, &domain->cpu_list, node) {
+		if (cpu_data->cpu == cpu)
+			return cpu_data;
+	}
+
+	if ((domain->shared_type == CPUFREQ_SHARED_TYPE_ANY) &&
+	    !list_empty(&domain->cpu_list))
+		return list_first_entry(&domain->cpu_list,
+					struct cppc_cpudata,
+					node);
+
+	cpu_data = kzalloc(sizeof(struct cppc_cpudata), GFP_KERNEL);
+	if (!cpu_data)
+		return NULL;
+
+	cpu_data->cpu = cpu;
+	cpu_data->domain = domain;
+
+	ret = cppc_get_perf_caps(cpu, &cpu_data->perf_caps);
+	if (ret) {
+		pr_err("Err reading CPU%d perf capabilities. ret:%d\n",
+			cpu, ret);
+		goto free;
+	}
 	/* Convert the lowest and nominal freq from MHz to KHz */
-	caps->lowest_freq *= 1000;
-	caps->nominal_freq *= 1000;
+	cpu_data->perf_caps.lowest_freq *= 1000;
+	cpu_data->perf_caps.nominal_freq *= 1000;
+
+	list_add(&cpu_data->node, &domain->cpu_list);
+
+	return cpu_data;
+free:
+	kfree(cpu_data);
+
+	return NULL;
+}
+
+static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
+{
+	struct cppc_cpudata *cpu_data = NULL;
+	struct psd_data *domain = NULL;
+	unsigned int cpu = policy->cpu;
+	struct cppc_perf_caps *caps;
+	int ret = 0;
+
+	cpu_data = cppc_cpufreq_get_cpu_data(cpu);
+	if (!cpu_data) {
+		pr_err("Error in acquiring _CPC/_PSD data for CPU.\n");
+		return -ENODEV;
+	}
+
+	domain = cpu_data->domain;
+	caps = &cpu_data->perf_caps;
+	policy->driver_data = cpu_data;
 
 	/*
 	 * Set min to lowest nonlinear perf to avoid any efficiency penalty (see
@@ -278,20 +360,10 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
 							    caps->nominal_perf);
 
 	policy->transition_delay_us = cppc_cpufreq_get_transition_delay_us(cpu);
-	policy->shared_type = cpu_data->shared_type;
+	policy->shared_type = domain->shared_type;
 
 	if (policy->shared_type == CPUFREQ_SHARED_TYPE_ANY) {
-		int i;
-
-		cpumask_copy(policy->cpus, cpu_data->shared_cpu_map);
-
-		for_each_cpu(i, policy->cpus) {
-			if (unlikely(i == cpu))
-				continue;
-
-			memcpy(&all_cpu_data[i]->perf_caps, caps,
-			       sizeof(cpu_data->perf_caps));
-		}
+		cpumask_copy(policy->cpus, domain->shared_cpu_map);
 	} else if (policy->shared_type == CPUFREQ_SHARED_TYPE_ALL) {
 		/* Support only SW_ANY for now. */
 		pr_debug("Unsupported CPU co-ord type\n");
@@ -354,9 +426,12 @@ static int cppc_get_rate_from_fbctrs(struct cppc_cpudata *cpu_data,
 static unsigned int cppc_cpufreq_get_rate(unsigned int cpu)
 {
 	struct cppc_perf_fb_ctrs fb_ctrs_t0 = {0}, fb_ctrs_t1 = {0};
-	struct cppc_cpudata *cpu_data = all_cpu_data[cpu];
+	struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
+	struct cppc_cpudata *cpu_data = policy->driver_data;
 	int ret;
 
+	cpufreq_cpu_put(policy);
+
 	ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t0);
 	if (ret)
 		return ret;
@@ -372,7 +447,7 @@ static unsigned int cppc_cpufreq_get_rate(unsigned int cpu)
 
 static int cppc_cpufreq_set_boost(struct cpufreq_policy *policy, int state)
 {
-	struct cppc_cpudata *cpu_data = all_cpu_data[policy->cpu];
+	struct cppc_cpudata *cpu_data = policy->driver_data;
 	struct cppc_perf_caps *caps = &cpu_data->perf_caps;
 	int ret;
 
@@ -415,10 +490,13 @@ static struct cpufreq_driver cppc_cpufreq_driver = {
  */
 static unsigned int hisi_cppc_cpufreq_get_rate(unsigned int cpu)
 {
-	struct cppc_cpudata *cpu_data = all_cpu_data[cpu];
+	struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
+	struct cppc_cpudata *cpu_data = policy->driver_data;
 	u64 desired_perf;
 	int ret;
 
+	cpufreq_cpu_put(policy);
+
 	ret = cppc_get_desired_perf(cpu, &desired_perf);
 	if (ret < 0)
 		return -EIO;
@@ -451,68 +529,41 @@ static void cppc_check_hisi_workaround(void)
 
 static int __init cppc_cpufreq_init(void)
 {
-	struct cppc_cpudata *cpu_data;
-	int i, ret = 0;
-
 	if (acpi_disabled)
 		return -ENODEV;
 
-	all_cpu_data = kcalloc(num_possible_cpus(), sizeof(void *),
-			       GFP_KERNEL);
-	if (!all_cpu_data)
-		return -ENOMEM;
-
-	for_each_possible_cpu(i) {
-		all_cpu_data[i] = kzalloc(sizeof(struct cppc_cpudata), GFP_KERNEL);
-		if (!all_cpu_data[i])
-			goto out;
-
-		cpu_data = all_cpu_data[i];
-		if (!zalloc_cpumask_var(&cpu_data->shared_cpu_map, GFP_KERNEL))
-			goto out;
-	}
-
-	ret = acpi_get_psd_map(all_cpu_data);
-	if (ret) {
-		pr_debug("Error parsing PSD data. Aborting cpufreq registration.\n");
-		goto out;
-	}
-
 	cppc_check_hisi_workaround();
 
-	ret = cpufreq_register_driver(&cppc_cpufreq_driver);
-	if (ret)
-		goto out;
+	return cpufreq_register_driver(&cppc_cpufreq_driver);
+}
 
-	return ret;
+static inline void free_cpu_data(struct psd_data *domain)
+{
+	struct cppc_cpudata *iter, *tmp;
 
-out:
-	for_each_possible_cpu(i) {
-		cpu_data = all_cpu_data[i];
-		if (!cpu_data)
-			break;
-		free_cpumask_var(cpu_data->shared_cpu_map);
-		kfree(cpu_data);
+	list_for_each_entry_safe(iter, tmp, &domain->cpu_list, node) {
+		list_del(&iter->node);
+		kfree(iter);
 	}
-
-	kfree(all_cpu_data);
-	return -ENODEV;
 }
 
+static inline void free_domain_data(void)
+{
+	struct psd_data *iter, *tmp;
+
+	list_for_each_entry_safe(iter, tmp, &domain_list, node) {
+		list_del(&iter->node);
+		if (!list_empty(&iter->cpu_list))
+			free_cpu_data(iter);
+		free_cpumask_var(iter->shared_cpu_map);
+		kfree(iter);
+	}
+}
 static void __exit cppc_cpufreq_exit(void)
 {
-	struct cppc_cpudata *cpu_data;
-	int i;
-
 	cpufreq_unregister_driver(&cppc_cpufreq_driver);
 
-	for_each_possible_cpu(i) {
-		cpu_data = all_cpu_data[i];
-		free_cpumask_var(cpu_data->shared_cpu_map);
-		kfree(cpu_data);
-	}
-
-	kfree(all_cpu_data);
+	free_domain_data();
 }
 
 module_exit(cppc_cpufreq_exit);
diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h
index a6a9373ab863..c0081fb6032e 100644
--- a/include/acpi/cppc_acpi.h
+++ b/include/acpi/cppc_acpi.h
@@ -122,22 +122,30 @@ struct cppc_perf_fb_ctrs {
 	u64 wraparound_time;
 };
 
+/* Container of performance state domain data */
+struct psd_data {
+	struct list_head node;
+	unsigned int shared_type;
+	struct list_head cpu_list;
+	cpumask_var_t shared_cpu_map;
+};
+
 /* Per CPU container for runtime CPPC management. */
 struct cppc_cpudata {
 	int cpu;
+	struct list_head node;
+	struct psd_data *domain;
 	struct cppc_perf_caps perf_caps;
 	struct cppc_perf_ctrls perf_ctrls;
 	struct cppc_perf_fb_ctrs perf_fb_ctrs;
 	struct cpufreq_policy *cur_policy;
-	unsigned int shared_type;
-	cpumask_var_t shared_cpu_map;
 };
 
 extern int cppc_get_desired_perf(int cpunum, u64 *desired_perf);
 extern int cppc_get_perf_ctrs(int cpu, struct cppc_perf_fb_ctrs *perf_fb_ctrs);
 extern int cppc_set_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls);
 extern int cppc_get_perf_caps(int cpu, struct cppc_perf_caps *caps);
-extern int acpi_get_psd_map(struct cppc_cpudata **);
+extern int acpi_get_psd_map(unsigned int cpu, struct psd_data *domain);
 extern unsigned int cppc_get_transition_latency(int cpu);
 extern bool cpc_ffh_supported(void);
 extern int cpc_read_ffh(int cpunum, struct cpc_reg *reg, u64 *val);
-- 
2.17.1

Re: [PATCH 4/8] cppc_cpufreq: replace per-cpu structures with lists

[Jeremy Linton]

Hi,

On 11/5/20 6:55 AM, Ionela Voinescu wrote:
> The cppc_cpudata per-cpu storage was inefficient (1) additional to causing
> functional issues (2) when CPUs are hotplugged out, due to per-cpu data
> being improperly initialised.
> 
> (1) The amount of information needed for CPPC performance control in its
>      cpufreq driver depends on the domain (PSD) coordination type:
> 
>      ANY:    One set of CPPC control and capability data (e.g desired
>              performance, highest/lowest performance, etc) applies to all
>              CPUs in the domain.
> 
>      ALL:    Same as ANY. To be noted that this type is not currently
>              supported. When supported, information about which CPUs
>              belong to a domain is needed in order for frequency change
>              requests to be sent to each of them.
> 
>      HW:     It's necessary to store CPPC control and capability
>              information for all the CPUs. HW will then coordinate the
>              performance state based on their limitations and requests.
> 
>      NONE:   Same as HW. No HW coordination is expected.
> 
>      Despite this, the previous initialisation code would indiscriminately
>      allocate memory for all CPUs (all_cpu_data) and unnecessarily
>      duplicate performance capabilities and the domain sharing mask and type
>      for each possible CPU.

I should have mentioned this on the last set.

If the common case on arm/acpi machines is a single core per _PSD (which 
I believe it is), then you are actually increasing the overhead doing this.

> 
> (2) With the current per-cpu structure, when having ANY coordination,
>      the cppc_cpudata cpu information is not initialised (will remain 0)
>      for all CPUs in a policy, other than policy->cpu. When policy->cpu is
>      hotplugged out, the driver will incorrectly use the uninitialised (0)
>      value of the other CPUs when making frequency changes. Additionally,
>      the previous values stored in the perf_ctrls.desired_perf will be
>      lost when policy->cpu changes.
> 
> Due to the current storage of driver data not being fit for purpose,
> replace the per-cpu CPPC/PSD data with a list of domains which in turn
> will point to a list of CPUs with their controls and capabilities,
> belonging to the domain.
> 
> This new structure has the following benefits:
>   - Clearly separates PSD (domain) data from CPPC (performance monitoring
>     and control) data and stores one mask of CPUs belonging to a domain
>     and its type in a single structure, for each domain.
> 
>   - For ANY domain coordination, a single cppc_cpudata set of capabilities
>     and controls are stored, for policy->cpu, and used for describing
>     performance controls and capabilities even when policy->cpu changes as
>     a result of hotplug. All the CPUs in the domain will belong to the
>     same policy.
> 
>   - For HW or lack of coordination, a full map of domains and CPUs is
>     obtained. Each CPU will have its own policy, but for HW, as a result
>     of PSD information, a shared_cpu_map mask could be used to identify
>     the domain CPU content.
> 
> Additional changes:
> 
>   - A pointer to the policy's cppc_cpudata is stored in policy->driver_data
> 
>   - All data allocation is done from the driver's init function. At that
>     point the domain is either created or retrieved. For this purpose
>     acpi_get_psd_map() was changed to create a map of the domain of a
>     single CPU, rather than the full system domain map.
> 
>   - When CPU's are hotplugged out and in, old information can be retrieved.
> 
> Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com>
> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> Cc: Len Brown <lenb@kernel.org>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>   drivers/acpi/cppc_acpi.c       | 126 +++++++------------
>   drivers/cpufreq/cppc_cpufreq.c | 217 ++++++++++++++++++++-------------
>   include/acpi/cppc_acpi.h       |  14 ++-
>   3 files changed, 190 insertions(+), 167 deletions(-)
> 
> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> index 7a99b19bb893..75e36b909ae6 100644
> --- a/drivers/acpi/cppc_acpi.c
> +++ b/drivers/acpi/cppc_acpi.c
> @@ -414,108 +414,72 @@ static int acpi_get_psd(struct cpc_desc *cpc_ptr, acpi_handle handle)
>   }
>   
>   /**
> - * acpi_get_psd_map - Map the CPUs in a common freq domain.
> - * @all_cpu_data: Ptrs to CPU specific CPPC data including PSD info.
> + * acpi_get_psd_map - Map the CPUs in the freq domain of a given cpu
> + * @cpu: Find all CPUs that share a domain with cpu.
> + * @domain: Pointer to given domain data storage
>    *
>    *	Return: 0 for success or negative value for err.
>    */
> -int acpi_get_psd_map(struct cppc_cpudata **all_cpu_data)
> +int acpi_get_psd_map(unsigned int cpu, struct psd_data *domain)
>   {
> -	int count_target;
> -	int retval = 0;
> -	unsigned int i, j;
> -	cpumask_var_t covered_cpus;
> -	struct cppc_cpudata *pr, *match_pr;
> -	struct acpi_psd_package *pdomain;
> -	struct acpi_psd_package *match_pdomain;
>   	struct cpc_desc *cpc_ptr, *match_cpc_ptr;
> -
> -	if (!zalloc_cpumask_var(&covered_cpus, GFP_KERNEL))
> -		return -ENOMEM;
> +	struct acpi_psd_package *match_pdomain;
> +	struct acpi_psd_package *pdomain;
> +	int count_target, i;
>   
>   	/*
>   	 * Now that we have _PSD data from all CPUs, let's setup P-state
>   	 * domain info.
>   	 */
> -	for_each_possible_cpu(i) {
> -		if (cpumask_test_cpu(i, covered_cpus))
> -			continue;
> +	cpc_ptr = per_cpu(cpc_desc_ptr, cpu);
> +	if (!cpc_ptr)
> +		return -EFAULT;
>   
> -		pr = all_cpu_data[i];
> -		cpc_ptr = per_cpu(cpc_desc_ptr, i);
> -		if (!cpc_ptr) {
> -			retval = -EFAULT;
> -			goto err_ret;
> -		}
> +	pdomain = &(cpc_ptr->domain_info);
> +	cpumask_set_cpu(cpu, domain->shared_cpu_map);
> +	if (pdomain->num_processors <= 1)
> +		return 0;
>   
> -		pdomain = &(cpc_ptr->domain_info);
> -		cpumask_set_cpu(i, pr->shared_cpu_map);
> -		cpumask_set_cpu(i, covered_cpus);
> -		if (pdomain->num_processors <= 1)
> -			continue;
> +	/* Validate the Domain info */
> +	count_target = pdomain->num_processors;
> +	if (pdomain->coord_type == DOMAIN_COORD_TYPE_SW_ALL)
> +		domain->shared_type = CPUFREQ_SHARED_TYPE_ALL;
> +	else if (pdomain->coord_type == DOMAIN_COORD_TYPE_HW_ALL)
> +		domain->shared_type = CPUFREQ_SHARED_TYPE_HW;
> +	else if (pdomain->coord_type == DOMAIN_COORD_TYPE_SW_ANY)
> +		domain->shared_type = CPUFREQ_SHARED_TYPE_ANY;
>   
> -		/* Validate the Domain info */
> -		count_target = pdomain->num_processors;
> -		if (pdomain->coord_type == DOMAIN_COORD_TYPE_SW_ALL)
> -			pr->shared_type = CPUFREQ_SHARED_TYPE_ALL;
> -		else if (pdomain->coord_type == DOMAIN_COORD_TYPE_HW_ALL)
> -			pr->shared_type = CPUFREQ_SHARED_TYPE_HW;
> -		else if (pdomain->coord_type == DOMAIN_COORD_TYPE_SW_ANY)
> -			pr->shared_type = CPUFREQ_SHARED_TYPE_ANY;
> -
> -		for_each_possible_cpu(j) {
> -			if (i == j)
> -				continue;
> -
> -			match_cpc_ptr = per_cpu(cpc_desc_ptr, j);
> -			if (!match_cpc_ptr) {
> -				retval = -EFAULT;
> -				goto err_ret;
> -			}
> -
> -			match_pdomain = &(match_cpc_ptr->domain_info);
> -			if (match_pdomain->domain != pdomain->domain)
> -				continue;
> +	for_each_possible_cpu(i) {
> +		if (i == cpu)
> +			continue;
>   
> -			/* Here i and j are in the same domain */
> -			if (match_pdomain->num_processors != count_target) {
> -				retval = -EFAULT;
> -				goto err_ret;
> -			}
> +		match_cpc_ptr = per_cpu(cpc_desc_ptr, i);
> +		if (!match_cpc_ptr)
> +			goto err_fault;
>   
> -			if (pdomain->coord_type != match_pdomain->coord_type) {
> -				retval = -EFAULT;
> -				goto err_ret;
> -			}
> +		match_pdomain = &(match_cpc_ptr->domain_info);
> +		if (match_pdomain->domain != pdomain->domain)
> +			continue;
>   
> -			cpumask_set_cpu(j, covered_cpus);
> -			cpumask_set_cpu(j, pr->shared_cpu_map);
> -		}
> +		/* Here i and cpu are in the same domain */
> +		if (match_pdomain->num_processors != count_target)
> +			goto err_fault;
>   
> -		for_each_cpu(j, pr->shared_cpu_map) {
> -			if (i == j)
> -				continue;
> +		if (pdomain->coord_type != match_pdomain->coord_type)
> +			goto err_fault;
>   
> -			match_pr = all_cpu_data[j];
> -			match_pr->shared_type = pr->shared_type;
> -			cpumask_copy(match_pr->shared_cpu_map,
> -				     pr->shared_cpu_map);
> -		}
> +		cpumask_set_cpu(i, domain->shared_cpu_map);
>   	}
> -	goto out;
>   
> -err_ret:
> -	for_each_possible_cpu(i) {
> -		pr = all_cpu_data[i];
> +	return 0;
>   
> -		/* Assume no coordination on any error parsing domain info */
> -		cpumask_clear(pr->shared_cpu_map);
> -		cpumask_set_cpu(i, pr->shared_cpu_map);
> -		pr->shared_type = CPUFREQ_SHARED_TYPE_ALL;
> -	}
> -out:
> -	free_cpumask_var(covered_cpus);
> -	return retval;
> +err_fault:
> +	/* Assume no coordination on any error parsing domain info */
> +	cpumask_clear(domain->shared_cpu_map);
> +	cpumask_set_cpu(cpu, domain->shared_cpu_map);
> +	domain->shared_type = CPUFREQ_SHARED_TYPE_ALL;
> +
> +	return -EFAULT;
>   }
>   EXPORT_SYMBOL_GPL(acpi_get_psd_map);
>   
> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> index 7cc9bd8568de..ac95b4424a2e 100644
> --- a/drivers/cpufreq/cppc_cpufreq.c
> +++ b/drivers/cpufreq/cppc_cpufreq.c
> @@ -30,13 +30,12 @@
>   #define DMI_PROCESSOR_MAX_SPEED		0x14
>   
>   /*
> - * These structs contain information parsed from per CPU
> - * ACPI _CPC structures.
> - * e.g. For each CPU the highest, lowest supported
> - * performance capabilities, desired performance level
> - * requested etc.
> + * This list contains information parsed from per CPU ACPI _CPC and _PSD
> + * structures: this is a list of domain data which in turn contains a list
> + * of cpus with their controls and capabilities, belonging to the domain.
>    */
> -static struct cppc_cpudata **all_cpu_data;
> +static LIST_HEAD(domain_list);
> +
>   static bool boost_supported;
>   
>   struct cppc_workaround_oem_info {
> @@ -148,8 +147,9 @@ static unsigned int cppc_cpufreq_khz_to_perf(struct cppc_cpudata *cpu_data,
>   static int cppc_cpufreq_set_target(struct cpufreq_policy *policy,
>   				   unsigned int target_freq,
>   				   unsigned int relation)
> +
>   {
> -	struct cppc_cpudata *cpu_data = all_cpu_data[policy->cpu];
> +	struct cppc_cpudata *cpu_data = policy->driver_data;
>   	struct cpufreq_freqs freqs;
>   	u32 desired_perf;
>   	int ret = 0;
> @@ -182,7 +182,7 @@ static int cppc_verify_policy(struct cpufreq_policy_data *policy)
>   
>   static void cppc_cpufreq_stop_cpu(struct cpufreq_policy *policy)
>   {
> -	struct cppc_cpudata *cpu_data = all_cpu_data[policy->cpu];
> +	struct cppc_cpudata *cpu_data = policy->driver_data;
>   	struct cppc_perf_caps *caps = &cpu_data->perf_caps;
>   	unsigned int cpu = policy->cpu;
>   	int ret;
> @@ -238,25 +238,107 @@ static unsigned int cppc_cpufreq_get_transition_delay_us(unsigned int cpu)
>   }
>   #endif
>   
> -static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
> +static struct psd_data *cppc_cpufreq_get_domain(unsigned int cpu)
>   {
> -	struct cppc_cpudata *cpu_data = all_cpu_data[policy->cpu];
> -	struct cppc_perf_caps *caps = &cpu_data->perf_caps;
> -	unsigned int cpu = policy->cpu;
> -	int ret = 0;
> +	struct psd_data *domain;
> +	int ret;
>   
> -	cpu_data->cpu = cpu;
> -	ret = cppc_get_perf_caps(cpu, caps);
> +	list_for_each_entry(domain, &domain_list, node) {
> +		if (cpumask_test_cpu(cpu, domain->shared_cpu_map))
> +			return domain;
> +	}
>   
> +	domain = kzalloc(sizeof(struct psd_data), GFP_KERNEL);
> +	if (!domain)
> +		return NULL;
> +	if (!zalloc_cpumask_var(&domain->shared_cpu_map, GFP_KERNEL))
> +		goto free_domain;
> +	INIT_LIST_HEAD(&domain->cpu_list);
> +
> +	ret = acpi_get_psd_map(cpu, domain);
>   	if (ret) {
> -		pr_debug("Err reading CPU%d perf capabilities. ret:%d\n",
> -			 cpu, ret);
> -		return ret;
> +		pr_err("Error parsing PSD data for CPU%d.\n", cpu);
> +		goto free_mask;
> +	}
> +
> +	list_add(&domain->node, &domain_list);
> +
> +	return domain;
> +
> +free_mask:
> +	free_cpumask_var(domain->shared_cpu_map);
> +free_domain:
> +	kfree(domain);
> +
> +	return NULL;
> +}
> +
> +static struct cppc_cpudata *cppc_cpufreq_get_cpu_data(unsigned int cpu)
> +{
> +	struct cppc_cpudata *cpu_data;
> +	struct psd_data *domain;
> +	int ret;
> +
> +	domain = cppc_cpufreq_get_domain(cpu);
> +	if (!domain) {
> +		pr_err("Error acquiring domain for CPU.\n");
> +		return NULL;
>   	}
>   
> +	list_for_each_entry(cpu_data, &domain->cpu_list, node) {
> +		if (cpu_data->cpu == cpu)
> +			return cpu_data;
> +	}
> +
> +	if ((domain->shared_type == CPUFREQ_SHARED_TYPE_ANY) &&
> +	    !list_empty(&domain->cpu_list))
> +		return list_first_entry(&domain->cpu_list,
> +					struct cppc_cpudata,
> +					node);
> +
> +	cpu_data = kzalloc(sizeof(struct cppc_cpudata), GFP_KERNEL);
> +	if (!cpu_data)
> +		return NULL;
> +
> +	cpu_data->cpu = cpu;
> +	cpu_data->domain = domain;
> +
> +	ret = cppc_get_perf_caps(cpu, &cpu_data->perf_caps);
> +	if (ret) {
> +		pr_err("Err reading CPU%d perf capabilities. ret:%d\n",
> +			cpu, ret);
> +		goto free;
> +	}
>   	/* Convert the lowest and nominal freq from MHz to KHz */
> -	caps->lowest_freq *= 1000;
> -	caps->nominal_freq *= 1000;
> +	cpu_data->perf_caps.lowest_freq *= 1000;
> +	cpu_data->perf_caps.nominal_freq *= 1000;
> +
> +	list_add(&cpu_data->node, &domain->cpu_list);
> +
> +	return cpu_data;
> +free:
> +	kfree(cpu_data);
> +
> +	return NULL;
> +}
> +
> +static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
> +{
> +	struct cppc_cpudata *cpu_data = NULL;
> +	struct psd_data *domain = NULL;
> +	unsigned int cpu = policy->cpu;
> +	struct cppc_perf_caps *caps;
> +	int ret = 0;
> +
> +	cpu_data = cppc_cpufreq_get_cpu_data(cpu);
> +	if (!cpu_data) {
> +		pr_err("Error in acquiring _CPC/_PSD data for CPU.\n");
> +		return -ENODEV;
> +	}
> +
> +	domain = cpu_data->domain;
> +	caps = &cpu_data->perf_caps;
> +	policy->driver_data = cpu_data;
>   
>   	/*
>   	 * Set min to lowest nonlinear perf to avoid any efficiency penalty (see
> @@ -278,20 +360,10 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
>   							    caps->nominal_perf);
>   
>   	policy->transition_delay_us = cppc_cpufreq_get_transition_delay_us(cpu);
> -	policy->shared_type = cpu_data->shared_type;
> +	policy->shared_type = domain->shared_type;
>   
>   	if (policy->shared_type == CPUFREQ_SHARED_TYPE_ANY) {
> -		int i;
> -
> -		cpumask_copy(policy->cpus, cpu_data->shared_cpu_map);
> -
> -		for_each_cpu(i, policy->cpus) {
> -			if (unlikely(i == cpu))
> -				continue;
> -
> -			memcpy(&all_cpu_data[i]->perf_caps, caps,
> -			       sizeof(cpu_data->perf_caps));
> -		}
> +		cpumask_copy(policy->cpus, domain->shared_cpu_map);
>   	} else if (policy->shared_type == CPUFREQ_SHARED_TYPE_ALL) {
>   		/* Support only SW_ANY for now. */
>   		pr_debug("Unsupported CPU co-ord type\n");
> @@ -354,9 +426,12 @@ static int cppc_get_rate_from_fbctrs(struct cppc_cpudata *cpu_data,
>   static unsigned int cppc_cpufreq_get_rate(unsigned int cpu)
>   {
>   	struct cppc_perf_fb_ctrs fb_ctrs_t0 = {0}, fb_ctrs_t1 = {0};
> -	struct cppc_cpudata *cpu_data = all_cpu_data[cpu];
> +	struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
> +	struct cppc_cpudata *cpu_data = policy->driver_data;
>   	int ret;
>   
> +	cpufreq_cpu_put(policy);
> +
>   	ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t0);
>   	if (ret)
>   		return ret;
> @@ -372,7 +447,7 @@ static unsigned int cppc_cpufreq_get_rate(unsigned int cpu)
>   
>   static int cppc_cpufreq_set_boost(struct cpufreq_policy *policy, int state)
>   {
> -	struct cppc_cpudata *cpu_data = all_cpu_data[policy->cpu];
> +	struct cppc_cpudata *cpu_data = policy->driver_data;
>   	struct cppc_perf_caps *caps = &cpu_data->perf_caps;
>   	int ret;
>   
> @@ -415,10 +490,13 @@ static struct cpufreq_driver cppc_cpufreq_driver = {
>    */
>   static unsigned int hisi_cppc_cpufreq_get_rate(unsigned int cpu)
>   {
> -	struct cppc_cpudata *cpu_data = all_cpu_data[cpu];
> +	struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
> +	struct cppc_cpudata *cpu_data = policy->driver_data;
>   	u64 desired_perf;
>   	int ret;
>   
> +	cpufreq_cpu_put(policy);
> +
>   	ret = cppc_get_desired_perf(cpu, &desired_perf);
>   	if (ret < 0)
>   		return -EIO;
> @@ -451,68 +529,41 @@ static void cppc_check_hisi_workaround(void)
>   
>   static int __init cppc_cpufreq_init(void)
>   {
> -	struct cppc_cpudata *cpu_data;
> -	int i, ret = 0;
> -
>   	if (acpi_disabled)
>   		return -ENODEV;
>   
> -	all_cpu_data = kcalloc(num_possible_cpus(), sizeof(void *),
> -			       GFP_KERNEL);
> -	if (!all_cpu_data)
> -		return -ENOMEM;
> -
> -	for_each_possible_cpu(i) {
> -		all_cpu_data[i] = kzalloc(sizeof(struct cppc_cpudata), GFP_KERNEL);
> -		if (!all_cpu_data[i])
> -			goto out;
> -
> -		cpu_data = all_cpu_data[i];
> -		if (!zalloc_cpumask_var(&cpu_data->shared_cpu_map, GFP_KERNEL))
> -			goto out;
> -	}
> -
> -	ret = acpi_get_psd_map(all_cpu_data);
> -	if (ret) {
> -		pr_debug("Error parsing PSD data. Aborting cpufreq registration.\n");
> -		goto out;
> -	}
> -
>   	cppc_check_hisi_workaround();
>   
> -	ret = cpufreq_register_driver(&cppc_cpufreq_driver);
> -	if (ret)
> -		goto out;
> +	return cpufreq_register_driver(&cppc_cpufreq_driver);
> +}
>   
> -	return ret;
> +static inline void free_cpu_data(struct psd_data *domain)
> +{
> +	struct cppc_cpudata *iter, *tmp;
>   
> -out:
> -	for_each_possible_cpu(i) {
> -		cpu_data = all_cpu_data[i];
> -		if (!cpu_data)
> -			break;
> -		free_cpumask_var(cpu_data->shared_cpu_map);
> -		kfree(cpu_data);
> +	list_for_each_entry_safe(iter, tmp, &domain->cpu_list, node) {
> +		list_del(&iter->node);
> +		kfree(iter);
>   	}
> -
> -	kfree(all_cpu_data);
> -	return -ENODEV;
>   }
>   
> +static inline void free_domain_data(void)
> +{
> +	struct psd_data *iter, *tmp;
> +
> +	list_for_each_entry_safe(iter, tmp, &domain_list, node) {
> +		list_del(&iter->node);
> +		if (!list_empty(&iter->cpu_list))
> +			free_cpu_data(iter);
> +		free_cpumask_var(iter->shared_cpu_map);
> +		kfree(iter);
> +	}
> +}
>   static void __exit cppc_cpufreq_exit(void)
>   {
> -	struct cppc_cpudata *cpu_data;
> -	int i;
> -
>   	cpufreq_unregister_driver(&cppc_cpufreq_driver);
>   
> -	for_each_possible_cpu(i) {
> -		cpu_data = all_cpu_data[i];
> -		free_cpumask_var(cpu_data->shared_cpu_map);
> -		kfree(cpu_data);
> -	}
> -
> -	kfree(all_cpu_data);
> +	free_domain_data();
>   }
>   
>   module_exit(cppc_cpufreq_exit);
> diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h
> index a6a9373ab863..c0081fb6032e 100644
> --- a/include/acpi/cppc_acpi.h
> +++ b/include/acpi/cppc_acpi.h
> @@ -122,22 +122,30 @@ struct cppc_perf_fb_ctrs {
>   	u64 wraparound_time;
>   };
>   
> +/* Container of performance state domain data */
> +struct psd_data {
> +	struct list_head node;
> +	unsigned int shared_type;
> +	struct list_head cpu_list;
> +	cpumask_var_t shared_cpu_map;
> +};
> +
>   /* Per CPU container for runtime CPPC management. */
>   struct cppc_cpudata {
>   	int cpu;
> +	struct list_head node;
> +	struct psd_data *domain;
>   	struct cppc_perf_caps perf_caps;
>   	struct cppc_perf_ctrls perf_ctrls;
>   	struct cppc_perf_fb_ctrs perf_fb_ctrs;
>   	struct cpufreq_policy *cur_policy;
> -	unsigned int shared_type;
> -	cpumask_var_t shared_cpu_map;
>   };
>   
>   extern int cppc_get_desired_perf(int cpunum, u64 *desired_perf);
>   extern int cppc_get_perf_ctrs(int cpu, struct cppc_perf_fb_ctrs *perf_fb_ctrs);
>   extern int cppc_set_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls);
>   extern int cppc_get_perf_caps(int cpu, struct cppc_perf_caps *caps);
> -extern int acpi_get_psd_map(struct cppc_cpudata **);
> +extern int acpi_get_psd_map(unsigned int cpu, struct psd_data *domain);
>   extern unsigned int cppc_get_transition_latency(int cpu);
>   extern bool cpc_ffh_supported(void);
>   extern int cpc_read_ffh(int cpunum, struct cpc_reg *reg, u64 *val);
> 

Re: [PATCH 4/8] cppc_cpufreq: replace per-cpu structures with lists

[Ionela Voinescu]

Hi Jeremy,

On Thursday 05 Nov 2020 at 09:50:30 (-0600), Jeremy Linton wrote:
> Hi,
> 
> On 11/5/20 6:55 AM, Ionela Voinescu wrote:
> > The cppc_cpudata per-cpu storage was inefficient (1) additional to causing
> > functional issues (2) when CPUs are hotplugged out, due to per-cpu data
> > being improperly initialised.
> > 
> > (1) The amount of information needed for CPPC performance control in its
> >      cpufreq driver depends on the domain (PSD) coordination type:
> > 
> >      ANY:    One set of CPPC control and capability data (e.g desired
> >              performance, highest/lowest performance, etc) applies to all
> >              CPUs in the domain.
> > 
> >      ALL:    Same as ANY. To be noted that this type is not currently
> >              supported. When supported, information about which CPUs
> >              belong to a domain is needed in order for frequency change
> >              requests to be sent to each of them.
> > 
> >      HW:     It's necessary to store CPPC control and capability
> >              information for all the CPUs. HW will then coordinate the
> >              performance state based on their limitations and requests.
> > 
> >      NONE:   Same as HW. No HW coordination is expected.
> > 
> >      Despite this, the previous initialisation code would indiscriminately
> >      allocate memory for all CPUs (all_cpu_data) and unnecessarily
> >      duplicate performance capabilities and the domain sharing mask and type
> >      for each possible CPU.
> 
> I should have mentioned this on the last set.
> 
> If the common case on arm/acpi machines is a single core per _PSD (which I
> believe it is), then you are actually increasing the overhead doing this.
> 

Thanks for taking another look and pointing this out.

Yes, that would be quite inefficient as I'd be holding both CPU and domain
information uselessly, for that case. I could drop the domain
information without actually losing anything (shared type and shared cpu
map have no purpose for single CPUs in a domain).

Also, I don't actually need a list of CPUs in the domain, an array will
work just as well, as I know the number of CPUs when I allocate the
domain - that will allow me to remove the node from cppc_cpudata and
save me some pointers.

Also, I now remember I wanted to get rid of cpu and cur_policy from
cppc_cpudata as well, as they serve no purpose. Let me know if you guys
see a reason against this.

All of this should at least bring things on par for HW and NONE types,
while improving ANY and ALL types. Thanks again for bringing this up.

Regards,
Ionela.

[PATCH 5/8] cppc_cpufreq: use policy->cpu as driver of frequency setting

[Ionela Voinescu]

Considering only the currently supported coordination types (ANY, HW,
NONE), this change only makes a difference for the ANY type, when
policy->cpu is hotplugged out. In that case the new policy->cpu will
be different from ((struct cppc_cpudata *)policy->driver_data)->cpu.

While in this case the controls of *ANY* CPU could be used to drive
frequency changes, it's more consistent to use policy->cpu as the
leading CPU, as used in all other cppc_cpufreq functions. Additionally,
the debug prints in cppc_set_perf() would no longer create confusion
when referring to a CPU that is hotplugged out.

Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cppc_cpufreq.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
index ac95b4424a2e..fd2daeb59b49 100644
--- a/drivers/cpufreq/cppc_cpufreq.c
+++ b/drivers/cpufreq/cppc_cpufreq.c
@@ -150,6 +150,7 @@ static int cppc_cpufreq_set_target(struct cpufreq_policy *policy,
 
 {
 	struct cppc_cpudata *cpu_data = policy->driver_data;
+	unsigned int cpu = policy->cpu;
 	struct cpufreq_freqs freqs;
 	u32 desired_perf;
 	int ret = 0;
@@ -164,12 +165,12 @@ static int cppc_cpufreq_set_target(struct cpufreq_policy *policy,
 	freqs.new = target_freq;
 
 	cpufreq_freq_transition_begin(policy, &freqs);
-	ret = cppc_set_perf(cpu_data->cpu, &cpu_data->perf_ctrls);
+	ret = cppc_set_perf(cpu, &cpu_data->perf_ctrls);
 	cpufreq_freq_transition_end(policy, &freqs, ret != 0);
 
 	if (ret)
 		pr_debug("Failed to set target on CPU:%d. ret:%d\n",
-			 cpu_data->cpu, ret);
+			 cpu, ret);
 
 	return ret;
 }
-- 
2.17.1

Re: [PATCH 5/8] cppc_cpufreq: use policy->cpu as driver of frequency setting

[Viresh Kumar]

On 05-11-20, 12:55, Ionela Voinescu wrote:
> Considering only the currently supported coordination types (ANY, HW,
> NONE), this change only makes a difference for the ANY type, when
> policy->cpu is hotplugged out. In that case the new policy->cpu will
> be different from ((struct cppc_cpudata *)policy->driver_data)->cpu.
> 
> While in this case the controls of *ANY* CPU could be used to drive
> frequency changes, it's more consistent to use policy->cpu as the
> leading CPU, as used in all other cppc_cpufreq functions. Additionally,
> the debug prints in cppc_set_perf() would no longer create confusion
> when referring to a CPU that is hotplugged out.
> 
> Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com>
> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/cppc_cpufreq.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> index ac95b4424a2e..fd2daeb59b49 100644
> --- a/drivers/cpufreq/cppc_cpufreq.c
> +++ b/drivers/cpufreq/cppc_cpufreq.c
> @@ -150,6 +150,7 @@ static int cppc_cpufreq_set_target(struct cpufreq_policy *policy,
>  
>  {
>  	struct cppc_cpudata *cpu_data = policy->driver_data;
> +	unsigned int cpu = policy->cpu;
>  	struct cpufreq_freqs freqs;
>  	u32 desired_perf;
>  	int ret = 0;
> @@ -164,12 +165,12 @@ static int cppc_cpufreq_set_target(struct cpufreq_policy *policy,
>  	freqs.new = target_freq;
>  
>  	cpufreq_freq_transition_begin(policy, &freqs);
> -	ret = cppc_set_perf(cpu_data->cpu, &cpu_data->perf_ctrls);
> +	ret = cppc_set_perf(cpu, &cpu_data->perf_ctrls);
>  	cpufreq_freq_transition_end(policy, &freqs, ret != 0);
>  
>  	if (ret)
>  		pr_debug("Failed to set target on CPU:%d. ret:%d\n",
> -			 cpu_data->cpu, ret);
> +			 cpu, ret);
>  
>  	return ret;
>  }

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

[PATCH 6/8] cppc_cpufreq: clarify support for coordination types

[Ionela Voinescu]

The previous coordination type handling in the cppc_cpufreq init code
created some confusion: the comment mentioned "Support only SW_ANY for
now" while only the SW_ALL/ALL case resulted in a failure. The other
coordination types (HW_ALL/HW, NONE) were silently supported.

Clarify support for coordination types while describing in comments the
intended behavior.

Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cppc_cpufreq.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
index fd2daeb59b49..60ac7f8049b5 100644
--- a/drivers/cpufreq/cppc_cpufreq.c
+++ b/drivers/cpufreq/cppc_cpufreq.c
@@ -363,11 +363,22 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
 	policy->transition_delay_us = cppc_cpufreq_get_transition_delay_us(cpu);
 	policy->shared_type = domain->shared_type;
 
-	if (policy->shared_type == CPUFREQ_SHARED_TYPE_ANY) {
+	switch (policy->shared_type) {
+	case CPUFREQ_SHARED_TYPE_HW:
+	case CPUFREQ_SHARED_TYPE_NONE:
+		/* Nothing to be done - we'll have a policy for each CPU */
+		break;
+	case CPUFREQ_SHARED_TYPE_ANY:
+		/*
+		 * All CPUs in the domain will share a policy and all cpufreq
+		 * operations will use a single cppc_cpudata structure stored
+		 * in policy->driver_data.
+		 */
 		cpumask_copy(policy->cpus, domain->shared_cpu_map);
-	} else if (policy->shared_type == CPUFREQ_SHARED_TYPE_ALL) {
-		/* Support only SW_ANY for now. */
-		pr_debug("Unsupported CPU co-ord type\n");
+		break;
+	default:
+		pr_info("Unsupported cpufreq CPU co-ord type: %d\n",
+			policy->shared_type);
 		return -EFAULT;
 	}
 
-- 
2.17.1

Re: [PATCH 6/8] cppc_cpufreq: clarify support for coordination types

[Viresh Kumar]

On 05-11-20, 12:55, Ionela Voinescu wrote:
> The previous coordination type handling in the cppc_cpufreq init code
> created some confusion: the comment mentioned "Support only SW_ANY for
> now" while only the SW_ALL/ALL case resulted in a failure. The other
> coordination types (HW_ALL/HW, NONE) were silently supported.
> 
> Clarify support for coordination types while describing in comments the
> intended behavior.
> 
> Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com>
> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/cppc_cpufreq.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> index fd2daeb59b49..60ac7f8049b5 100644
> --- a/drivers/cpufreq/cppc_cpufreq.c
> +++ b/drivers/cpufreq/cppc_cpufreq.c
> @@ -363,11 +363,22 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
>  	policy->transition_delay_us = cppc_cpufreq_get_transition_delay_us(cpu);
>  	policy->shared_type = domain->shared_type;
>  
> -	if (policy->shared_type == CPUFREQ_SHARED_TYPE_ANY) {
> +	switch (policy->shared_type) {
> +	case CPUFREQ_SHARED_TYPE_HW:
> +	case CPUFREQ_SHARED_TYPE_NONE:
> +		/* Nothing to be done - we'll have a policy for each CPU */
> +		break;
> +	case CPUFREQ_SHARED_TYPE_ANY:
> +		/*
> +		 * All CPUs in the domain will share a policy and all cpufreq
> +		 * operations will use a single cppc_cpudata structure stored
> +		 * in policy->driver_data.
> +		 */
>  		cpumask_copy(policy->cpus, domain->shared_cpu_map);
> -	} else if (policy->shared_type == CPUFREQ_SHARED_TYPE_ALL) {
> -		/* Support only SW_ANY for now. */
> -		pr_debug("Unsupported CPU co-ord type\n");
> +		break;
> +	default:
> +		pr_info("Unsupported cpufreq CPU co-ord type: %d\n",
> +			policy->shared_type);
>  		return -EFAULT;
>  	}
>  

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

[PATCH 7/8] cppc_cpufreq: expose information on frequency domains

[Ionela Voinescu]

Use the existing sysfs attribute "freqdomain_cpus" to expose
information to userspace about CPUs in the same frequency domain.

Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
---
 Documentation/ABI/testing/sysfs-devices-system-cpu |  3 ++-
 drivers/cpufreq/cppc_cpufreq.c                     | 14 ++++++++++++++
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/testing/sysfs-devices-system-cpu b/Documentation/ABI/testing/sysfs-devices-system-cpu
index 1a04ca8162ad..0eee30b27ab6 100644
--- a/Documentation/ABI/testing/sysfs-devices-system-cpu
+++ b/Documentation/ABI/testing/sysfs-devices-system-cpu
@@ -264,7 +264,8 @@ Description:	Discover CPUs in the same CPU frequency coordination domain
 		attribute is useful for user space DVFS controllers to get better
 		power/performance results for platforms using acpi-cpufreq.
 
-		This file is only present if the acpi-cpufreq driver is in use.
+		This file is only present if the acpi-cpufreq or the cppc-cpufreq
+		drivers are in use.
 
 
 What:		/sys/devices/system/cpu/cpu*/cache/index3/cache_disable_{0,1}
diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
index 60ac7f8049b5..b4edeeb57d04 100644
--- a/drivers/cpufreq/cppc_cpufreq.c
+++ b/drivers/cpufreq/cppc_cpufreq.c
@@ -483,6 +483,19 @@ static int cppc_cpufreq_set_boost(struct cpufreq_policy *policy, int state)
 	return 0;
 }
 
+static ssize_t show_freqdomain_cpus(struct cpufreq_policy *policy, char *buf)
+{
+	struct cppc_cpudata *cpu_data = policy->driver_data;
+
+	return cpufreq_show_cpus(cpu_data->domain->shared_cpu_map, buf);
+}
+cpufreq_freq_attr_ro(freqdomain_cpus);
+
+static struct freq_attr *cppc_cpufreq_attr[] = {
+	&freqdomain_cpus,
+	NULL,
+};
+
 static struct cpufreq_driver cppc_cpufreq_driver = {
 	.flags = CPUFREQ_CONST_LOOPS,
 	.verify = cppc_verify_policy,
@@ -491,6 +504,7 @@ static struct cpufreq_driver cppc_cpufreq_driver = {
 	.init = cppc_cpufreq_cpu_init,
 	.stop_cpu = cppc_cpufreq_stop_cpu,
 	.set_boost = cppc_cpufreq_set_boost,
+	.attr = cppc_cpufreq_attr,
 	.name = "cppc_cpufreq",
 };
 
-- 
2.17.1

Re: [PATCH 7/8] cppc_cpufreq: expose information on frequency domains

[Viresh Kumar]

On 05-11-20, 12:55, Ionela Voinescu wrote:
> Use the existing sysfs attribute "freqdomain_cpus" to expose
> information to userspace about CPUs in the same frequency domain.
> 
> Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com>
> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  Documentation/ABI/testing/sysfs-devices-system-cpu |  3 ++-
>  drivers/cpufreq/cppc_cpufreq.c                     | 14 ++++++++++++++
>  2 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-devices-system-cpu b/Documentation/ABI/testing/sysfs-devices-system-cpu
> index 1a04ca8162ad..0eee30b27ab6 100644
> --- a/Documentation/ABI/testing/sysfs-devices-system-cpu
> +++ b/Documentation/ABI/testing/sysfs-devices-system-cpu
> @@ -264,7 +264,8 @@ Description:	Discover CPUs in the same CPU frequency coordination domain
>  		attribute is useful for user space DVFS controllers to get better
>  		power/performance results for platforms using acpi-cpufreq.
>  
> -		This file is only present if the acpi-cpufreq driver is in use.
> +		This file is only present if the acpi-cpufreq or the cppc-cpufreq
> +		drivers are in use.
>  
>  
>  What:		/sys/devices/system/cpu/cpu*/cache/index3/cache_disable_{0,1}
> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> index 60ac7f8049b5..b4edeeb57d04 100644
> --- a/drivers/cpufreq/cppc_cpufreq.c
> +++ b/drivers/cpufreq/cppc_cpufreq.c
> @@ -483,6 +483,19 @@ static int cppc_cpufreq_set_boost(struct cpufreq_policy *policy, int state)
>  	return 0;
>  }
>  
> +static ssize_t show_freqdomain_cpus(struct cpufreq_policy *policy, char *buf)
> +{
> +	struct cppc_cpudata *cpu_data = policy->driver_data;
> +
> +	return cpufreq_show_cpus(cpu_data->domain->shared_cpu_map, buf);
> +}
> +cpufreq_freq_attr_ro(freqdomain_cpus);
> +
> +static struct freq_attr *cppc_cpufreq_attr[] = {
> +	&freqdomain_cpus,
> +	NULL,
> +};
> +
>  static struct cpufreq_driver cppc_cpufreq_driver = {
>  	.flags = CPUFREQ_CONST_LOOPS,
>  	.verify = cppc_verify_policy,
> @@ -491,6 +504,7 @@ static struct cpufreq_driver cppc_cpufreq_driver = {
>  	.init = cppc_cpufreq_cpu_init,
>  	.stop_cpu = cppc_cpufreq_stop_cpu,
>  	.set_boost = cppc_cpufreq_set_boost,
> +	.attr = cppc_cpufreq_attr,
>  	.name = "cppc_cpufreq",
>  };

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

[PATCH 8/8] acpi: fix NONE coordination for domain mapping failure

[Ionela Voinescu]

For errors parsing the _PSD domains, a separate domain is returned for
each CPU in the failed _PSD domain with no coordination (as per previous
comment). But contrary to the intention, the code was setting
CPUFREQ_SHARED_TYPE_ALL as coordination type.

Change shared_type to CPUFREQ_SHARED_TYPE_NONE in case of errors parsing
the domain information. The function still return the error and the caller
is free to bail out the domain initialisation altogether in that case.

Given that both functions return domains with a single CPU, this change
does not affect the functionality, but clarifies the intention.

Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Len Brown <lenb@kernel.org>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/acpi/cppc_acpi.c         | 2 +-
 drivers/acpi/processor_perflib.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index 75e36b909ae6..e1e46cc66eeb 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -477,7 +477,7 @@ int acpi_get_psd_map(unsigned int cpu, struct psd_data *domain)
 	/* Assume no coordination on any error parsing domain info */
 	cpumask_clear(domain->shared_cpu_map);
 	cpumask_set_cpu(cpu, domain->shared_cpu_map);
-	domain->shared_type = CPUFREQ_SHARED_TYPE_ALL;
+	domain->shared_type = CPUFREQ_SHARED_TYPE_NONE;
 
 	return -EFAULT;
 }
diff --git a/drivers/acpi/processor_perflib.c b/drivers/acpi/processor_perflib.c
index 5909e8fa4013..5ce638537791 100644
--- a/drivers/acpi/processor_perflib.c
+++ b/drivers/acpi/processor_perflib.c
@@ -710,7 +710,7 @@ int acpi_processor_preregister_performance(
 		if (retval) {
 			cpumask_clear(pr->performance->shared_cpu_map);
 			cpumask_set_cpu(i, pr->performance->shared_cpu_map);
-			pr->performance->shared_type = CPUFREQ_SHARED_TYPE_ALL;
+			pr->performance->shared_type = CPUFREQ_SHARED_TYPE_NONE;
 		}
 		pr->performance = NULL; /* Will be set for real in register */
 	}
-- 
2.17.1

Re: [PATCH 8/8] acpi: fix NONE coordination for domain mapping failure

[Rafael J. Wysocki]

On Thu, Nov 5, 2020 at 1:57 PM Ionela Voinescu <ionela.voinescu@arm.com> wrote:
>
> For errors parsing the _PSD domains, a separate domain is returned for
> each CPU in the failed _PSD domain with no coordination (as per previous
> comment). But contrary to the intention, the code was setting
> CPUFREQ_SHARED_TYPE_ALL as coordination type.
>
> Change shared_type to CPUFREQ_SHARED_TYPE_NONE in case of errors parsing
> the domain information. The function still return the error and the caller
> is free to bail out the domain initialisation altogether in that case.
>
> Given that both functions return domains with a single CPU, this change
> does not affect the functionality, but clarifies the intention.

Is this related to any other patches in the series?

> Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com>
> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> Cc: Len Brown <lenb@kernel.org>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/acpi/cppc_acpi.c         | 2 +-
>  drivers/acpi/processor_perflib.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> index 75e36b909ae6..e1e46cc66eeb 100644
> --- a/drivers/acpi/cppc_acpi.c
> +++ b/drivers/acpi/cppc_acpi.c
> @@ -477,7 +477,7 @@ int acpi_get_psd_map(unsigned int cpu, struct psd_data *domain)
>         /* Assume no coordination on any error parsing domain info */
>         cpumask_clear(domain->shared_cpu_map);
>         cpumask_set_cpu(cpu, domain->shared_cpu_map);
> -       domain->shared_type = CPUFREQ_SHARED_TYPE_ALL;
> +       domain->shared_type = CPUFREQ_SHARED_TYPE_NONE;
>
>         return -EFAULT;
>  }
> diff --git a/drivers/acpi/processor_perflib.c b/drivers/acpi/processor_perflib.c
> index 5909e8fa4013..5ce638537791 100644
> --- a/drivers/acpi/processor_perflib.c
> +++ b/drivers/acpi/processor_perflib.c
> @@ -710,7 +710,7 @@ int acpi_processor_preregister_performance(
>                 if (retval) {
>                         cpumask_clear(pr->performance->shared_cpu_map);
>                         cpumask_set_cpu(i, pr->performance->shared_cpu_map);
> -                       pr->performance->shared_type = CPUFREQ_SHARED_TYPE_ALL;
> +                       pr->performance->shared_type = CPUFREQ_SHARED_TYPE_NONE;
>                 }
>                 pr->performance = NULL; /* Will be set for real in register */
>         }
> --
> 2.17.1
>

Re: [PATCH 8/8] acpi: fix NONE coordination for domain mapping failure

[Ionela Voinescu]

Hi Rafael,

On Thursday 05 Nov 2020 at 14:05:55 (+0100), Rafael J. Wysocki wrote:
> On Thu, Nov 5, 2020 at 1:57 PM Ionela Voinescu <ionela.voinescu@arm.com> wrote:
> >
> > For errors parsing the _PSD domains, a separate domain is returned for
> > each CPU in the failed _PSD domain with no coordination (as per previous
> > comment). But contrary to the intention, the code was setting
> > CPUFREQ_SHARED_TYPE_ALL as coordination type.
> >
> > Change shared_type to CPUFREQ_SHARED_TYPE_NONE in case of errors parsing
> > the domain information. The function still return the error and the caller
> > is free to bail out the domain initialisation altogether in that case.
> >
> > Given that both functions return domains with a single CPU, this change
> > does not affect the functionality, but clarifies the intention.
> 
> Is this related to any other patches in the series?
> 

It does not depend on any of the other patches. I first noticed this in
acpi_get_psd_map() which is solely used by cppc_cpufreq.c, but looking
some more into it showed processor_perflib.c's
acpi_processor_preregister_performance() had the same inconsistency.

I can submit this separately, if that works better.

Thanks,
Ionela.

> > Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com>
> > Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> > Cc: Len Brown <lenb@kernel.org>
> > Cc: Viresh Kumar <viresh.kumar@linaro.org>
> > ---
> >  drivers/acpi/cppc_acpi.c         | 2 +-
> >  drivers/acpi/processor_perflib.c | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> > index 75e36b909ae6..e1e46cc66eeb 100644
> > --- a/drivers/acpi/cppc_acpi.c
> > +++ b/drivers/acpi/cppc_acpi.c
> > @@ -477,7 +477,7 @@ int acpi_get_psd_map(unsigned int cpu, struct psd_data *domain)
> >         /* Assume no coordination on any error parsing domain info */
> >         cpumask_clear(domain->shared_cpu_map);
> >         cpumask_set_cpu(cpu, domain->shared_cpu_map);
> > -       domain->shared_type = CPUFREQ_SHARED_TYPE_ALL;
> > +       domain->shared_type = CPUFREQ_SHARED_TYPE_NONE;
> >
> >         return -EFAULT;
> >  }
> > diff --git a/drivers/acpi/processor_perflib.c b/drivers/acpi/processor_perflib.c
> > index 5909e8fa4013..5ce638537791 100644
> > --- a/drivers/acpi/processor_perflib.c
> > +++ b/drivers/acpi/processor_perflib.c
> > @@ -710,7 +710,7 @@ int acpi_processor_preregister_performance(
> >                 if (retval) {
> >                         cpumask_clear(pr->performance->shared_cpu_map);
> >                         cpumask_set_cpu(i, pr->performance->shared_cpu_map);
> > -                       pr->performance->shared_type = CPUFREQ_SHARED_TYPE_ALL;
> > +                       pr->performance->shared_type = CPUFREQ_SHARED_TYPE_NONE;
> >                 }
> >                 pr->performance = NULL; /* Will be set for real in register */
> >         }
> > --
> > 2.17.1
> >

Re: [PATCH 8/8] acpi: fix NONE coordination for domain mapping failure

[Rafael J. Wysocki]

On Thursday, November 5, 2020 3:02:02 PM CET Ionela Voinescu wrote:
> Hi Rafael,
> 
> On Thursday 05 Nov 2020 at 14:05:55 (+0100), Rafael J. Wysocki wrote:
> > On Thu, Nov 5, 2020 at 1:57 PM Ionela Voinescu <ionela.voinescu@arm.com> wrote:
> > >
> > > For errors parsing the _PSD domains, a separate domain is returned for
> > > each CPU in the failed _PSD domain with no coordination (as per previous
> > > comment). But contrary to the intention, the code was setting
> > > CPUFREQ_SHARED_TYPE_ALL as coordination type.
> > >
> > > Change shared_type to CPUFREQ_SHARED_TYPE_NONE in case of errors parsing
> > > the domain information. The function still return the error and the caller
> > > is free to bail out the domain initialisation altogether in that case.
> > >
> > > Given that both functions return domains with a single CPU, this change
> > > does not affect the functionality, but clarifies the intention.
> > 
> > Is this related to any other patches in the series?
> > 
> 
> It does not depend on any of the other patches. I first noticed this in
> acpi_get_psd_map() which is solely used by cppc_cpufreq.c, but looking
> some more into it showed processor_perflib.c's
> acpi_processor_preregister_performance() had the same inconsistency.
> 
> I can submit this separately, if that works better.

No need this time, but in general sending unrelated changes separately is less
confusing.

Thanks!

Re: [PATCH 8/8] acpi: fix NONE coordination for domain mapping failure

[Viresh Kumar]

On 05-11-20, 12:55, Ionela Voinescu wrote:
> For errors parsing the _PSD domains, a separate domain is returned for
> each CPU in the failed _PSD domain with no coordination (as per previous
> comment). But contrary to the intention, the code was setting
> CPUFREQ_SHARED_TYPE_ALL as coordination type.
> 
> Change shared_type to CPUFREQ_SHARED_TYPE_NONE in case of errors parsing
> the domain information. The function still return the error and the caller
> is free to bail out the domain initialisation altogether in that case.
> 
> Given that both functions return domains with a single CPU, this change
> does not affect the functionality, but clarifies the intention.
> 
> Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com>
> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> Cc: Len Brown <lenb@kernel.org>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/acpi/cppc_acpi.c         | 2 +-
>  drivers/acpi/processor_perflib.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> index 75e36b909ae6..e1e46cc66eeb 100644
> --- a/drivers/acpi/cppc_acpi.c
> +++ b/drivers/acpi/cppc_acpi.c
> @@ -477,7 +477,7 @@ int acpi_get_psd_map(unsigned int cpu, struct psd_data *domain)
>  	/* Assume no coordination on any error parsing domain info */
>  	cpumask_clear(domain->shared_cpu_map);
>  	cpumask_set_cpu(cpu, domain->shared_cpu_map);
> -	domain->shared_type = CPUFREQ_SHARED_TYPE_ALL;
> +	domain->shared_type = CPUFREQ_SHARED_TYPE_NONE;
>  
>  	return -EFAULT;
>  }
> diff --git a/drivers/acpi/processor_perflib.c b/drivers/acpi/processor_perflib.c
> index 5909e8fa4013..5ce638537791 100644
> --- a/drivers/acpi/processor_perflib.c
> +++ b/drivers/acpi/processor_perflib.c
> @@ -710,7 +710,7 @@ int acpi_processor_preregister_performance(
>  		if (retval) {
>  			cpumask_clear(pr->performance->shared_cpu_map);
>  			cpumask_set_cpu(i, pr->performance->shared_cpu_map);
> -			pr->performance->shared_type = CPUFREQ_SHARED_TYPE_ALL;
> +			pr->performance->shared_type = CPUFREQ_SHARED_TYPE_NONE;
>  		}
>  		pr->performance = NULL; /* Will be set for real in register */
>  	}

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

Re: [PATCH 0/8] cppc_cpufreq: fix, clarify and improve support

[Rafael J. Wysocki]

On Thu, Nov 5, 2020 at 1:56 PM Ionela Voinescu <ionela.voinescu@arm.com> wrote:
>
> Hi guys,
>
> I found myself staring a bit too much at this driver in the past weeks
> and that's likely the cause for me coming up with this series of 8
> patches that cleans up, clarifies and reworks parts of it, as follows:
>
>  - patches 1-3/8: trivial clean-up and renaming with the purpose to
>                   improve readability
>  - patch 4/8: replace previous per-cpu data structures with lists of
>               domains and CPUs to get more efficient storage for driver
>               data and fix previous issues in case of CPU hotplugging,
>               as discussed at [1].
>  - patches 5-6/8: a few fixes and clarifications: mostly making sure
>                   the behavior described in the comments and debug
>                   messages matches the code and there is clear
>                   indication of what is supported and how.
>  - patch 7/8: use the existing freqdomains_cpus attribute to inform
>               the user on frequency domains.
>  - patch 8/8: acpi: replace ALL coordination with NONE coordination
>                     when errors are find parsing the _PSD domains
>               (as described in the comments in the code).
>
> Hopefully you'll find this useful for ease of maintenance and ease of
> future development of the driver.
>
> This functionality was tested on a Juno platform with modified _PSD
> tables to test the functionality for all currently supported
> coordination types: ANY, HW, NONE.
>
> The current code is based on v5.10-rc2.
>
> Thanks,
> Ionela.
>
> [1] https://lore.kernel.org/linux-pm/20200922162540.GB796@arm.com/
>
> Ionela Voinescu (8):
>   cppc_cpufreq: fix misspelling, code style and readability issues
>   cppc_cpufreq: clean up cpu, cpu_num and cpunum variable use
>   cppc_cpufreq: simplify use of performance capabilities
>   cppc_cpufreq: replace per-cpu structures with lists
>   cppc_cpufreq: use policy->cpu as driver of frequency setting
>   cppc_cpufreq: clarify support for coordination types
>   cppc_cpufreq: expose information on frequency domains
>   acpi: fix NONE coordination for domain mapping failure
>
>  .../ABI/testing/sysfs-devices-system-cpu      |   3 +-
>  drivers/acpi/cppc_acpi.c                      | 126 +++---
>  drivers/acpi/processor_perflib.c              |   2 +-
>  drivers/cpufreq/cppc_cpufreq.c                | 358 +++++++++++-------
>  include/acpi/cppc_acpi.h                      |  14 +-
>  5 files changed, 277 insertions(+), 226 deletions(-)
>
> --

All patches applied as 5.11 material (with a minor subject edit in the
last patch), thanks!

In the future, though, please CC all/any ACPI-related changes to the
linux-acpi mailing list.

Re: [PATCH 0/8] cppc_cpufreq: fix, clarify and improve support

[Ionela Voinescu]

Hi Rafael,

On Tuesday 17 Nov 2020 at 15:59:24 (+0100), Rafael J. Wysocki wrote:
> On Thu, Nov 5, 2020 at 1:56 PM Ionela Voinescu <ionela.voinescu@arm.com> wrote:
> >
> > Hi guys,
> >
> > I found myself staring a bit too much at this driver in the past weeks
> > and that's likely the cause for me coming up with this series of 8
> > patches that cleans up, clarifies and reworks parts of it, as follows:
> >
> >  - patches 1-3/8: trivial clean-up and renaming with the purpose to
> >                   improve readability
> >  - patch 4/8: replace previous per-cpu data structures with lists of
> >               domains and CPUs to get more efficient storage for driver
> >               data and fix previous issues in case of CPU hotplugging,
> >               as discussed at [1].
> >  - patches 5-6/8: a few fixes and clarifications: mostly making sure
> >                   the behavior described in the comments and debug
> >                   messages matches the code and there is clear
> >                   indication of what is supported and how.
> >  - patch 7/8: use the existing freqdomains_cpus attribute to inform
> >               the user on frequency domains.
> >  - patch 8/8: acpi: replace ALL coordination with NONE coordination
> >                     when errors are find parsing the _PSD domains
> >               (as described in the comments in the code).
> >
> > Hopefully you'll find this useful for ease of maintenance and ease of
> > future development of the driver.
> >
> > This functionality was tested on a Juno platform with modified _PSD
> > tables to test the functionality for all currently supported
> > coordination types: ANY, HW, NONE.
> >
> > The current code is based on v5.10-rc2.
> >
> > Thanks,
> > Ionela.
> >
> > [1] https://lore.kernel.org/linux-pm/20200922162540.GB796@arm.com/
> >
> > Ionela Voinescu (8):
> >   cppc_cpufreq: fix misspelling, code style and readability issues
> >   cppc_cpufreq: clean up cpu, cpu_num and cpunum variable use
> >   cppc_cpufreq: simplify use of performance capabilities
> >   cppc_cpufreq: replace per-cpu structures with lists
> >   cppc_cpufreq: use policy->cpu as driver of frequency setting
> >   cppc_cpufreq: clarify support for coordination types
> >   cppc_cpufreq: expose information on frequency domains
> >   acpi: fix NONE coordination for domain mapping failure
> >
> >  .../ABI/testing/sysfs-devices-system-cpu      |   3 +-
> >  drivers/acpi/cppc_acpi.c                      | 126 +++---
> >  drivers/acpi/processor_perflib.c              |   2 +-
> >  drivers/cpufreq/cppc_cpufreq.c                | 358 +++++++++++-------
> >  include/acpi/cppc_acpi.h                      |  14 +-
> >  5 files changed, 277 insertions(+), 226 deletions(-)
> >
> > --
> 
> All patches applied as 5.11 material (with a minor subject edit in the
> last patch), thanks!
> 

Patch 4/8 was not acked. I was about to push a new version in which I
fix the scenario that Jeremy mentioned. Would you like me to push that
as a separate patch on top of this series, or should I push a new
version of this series?

All the other patches will be the same.

> In the future, though, please CC all/any ACPI-related changes to the
> linux-acpi mailing list.

Will do!

Thank you,
Ionela.

Re: [PATCH 0/8] cppc_cpufreq: fix, clarify and improve support

[Rafael J. Wysocki]

On Tue, Nov 17, 2020 at 4:32 PM Ionela Voinescu <ionela.voinescu@arm.com> wrote:
>
> Hi Rafael,
>
> On Tuesday 17 Nov 2020 at 15:59:24 (+0100), Rafael J. Wysocki wrote:
> > On Thu, Nov 5, 2020 at 1:56 PM Ionela Voinescu <ionela.voinescu@arm.com> wrote:
> > >
> > > Hi guys,
> > >
> > > I found myself staring a bit too much at this driver in the past weeks
> > > and that's likely the cause for me coming up with this series of 8
> > > patches that cleans up, clarifies and reworks parts of it, as follows:
> > >
> > >  - patches 1-3/8: trivial clean-up and renaming with the purpose to
> > >                   improve readability
> > >  - patch 4/8: replace previous per-cpu data structures with lists of
> > >               domains and CPUs to get more efficient storage for driver
> > >               data and fix previous issues in case of CPU hotplugging,
> > >               as discussed at [1].
> > >  - patches 5-6/8: a few fixes and clarifications: mostly making sure
> > >                   the behavior described in the comments and debug
> > >                   messages matches the code and there is clear
> > >                   indication of what is supported and how.
> > >  - patch 7/8: use the existing freqdomains_cpus attribute to inform
> > >               the user on frequency domains.
> > >  - patch 8/8: acpi: replace ALL coordination with NONE coordination
> > >                     when errors are find parsing the _PSD domains
> > >               (as described in the comments in the code).
> > >
> > > Hopefully you'll find this useful for ease of maintenance and ease of
> > > future development of the driver.
> > >
> > > This functionality was tested on a Juno platform with modified _PSD
> > > tables to test the functionality for all currently supported
> > > coordination types: ANY, HW, NONE.
> > >
> > > The current code is based on v5.10-rc2.
> > >
> > > Thanks,
> > > Ionela.
> > >
> > > [1] https://lore.kernel.org/linux-pm/20200922162540.GB796@arm.com/
> > >
> > > Ionela Voinescu (8):
> > >   cppc_cpufreq: fix misspelling, code style and readability issues
> > >   cppc_cpufreq: clean up cpu, cpu_num and cpunum variable use
> > >   cppc_cpufreq: simplify use of performance capabilities
> > >   cppc_cpufreq: replace per-cpu structures with lists
> > >   cppc_cpufreq: use policy->cpu as driver of frequency setting
> > >   cppc_cpufreq: clarify support for coordination types
> > >   cppc_cpufreq: expose information on frequency domains
> > >   acpi: fix NONE coordination for domain mapping failure
> > >
> > >  .../ABI/testing/sysfs-devices-system-cpu      |   3 +-
> > >  drivers/acpi/cppc_acpi.c                      | 126 +++---
> > >  drivers/acpi/processor_perflib.c              |   2 +-
> > >  drivers/cpufreq/cppc_cpufreq.c                | 358 +++++++++++-------
> > >  include/acpi/cppc_acpi.h                      |  14 +-
> > >  5 files changed, 277 insertions(+), 226 deletions(-)
> > >
> > > --
> >
> > All patches applied as 5.11 material (with a minor subject edit in the
> > last patch), thanks!
> >
>
> Patch 4/8 was not acked. I was about to push a new version in which I
> fix the scenario that Jeremy mentioned.

Well, it wasn't clear to me what you wanted to do about it.

> Would you like me to push that
> as a separate patch on top of this series,

Yes, please.

Re: [PATCH 0/8] cppc_cpufreq: fix, clarify and improve support

[Ionela Voinescu]

On Tuesday 17 Nov 2020 at 17:30:33 (+0100), Rafael J. Wysocki wrote:
[..]
> > > > Ionela Voinescu (8):
> > > >   cppc_cpufreq: fix misspelling, code style and readability issues
> > > >   cppc_cpufreq: clean up cpu, cpu_num and cpunum variable use
> > > >   cppc_cpufreq: simplify use of performance capabilities
> > > >   cppc_cpufreq: replace per-cpu structures with lists
> > > >   cppc_cpufreq: use policy->cpu as driver of frequency setting
> > > >   cppc_cpufreq: clarify support for coordination types
> > > >   cppc_cpufreq: expose information on frequency domains
> > > >   acpi: fix NONE coordination for domain mapping failure
> > > >
> > > >  .../ABI/testing/sysfs-devices-system-cpu      |   3 +-
> > > >  drivers/acpi/cppc_acpi.c                      | 126 +++---
> > > >  drivers/acpi/processor_perflib.c              |   2 +-
> > > >  drivers/cpufreq/cppc_cpufreq.c                | 358 +++++++++++-------
> > > >  include/acpi/cppc_acpi.h                      |  14 +-
> > > >  5 files changed, 277 insertions(+), 226 deletions(-)
> > > >
> > > > --
> > >
> > > All patches applied as 5.11 material (with a minor subject edit in the
> > > last patch), thanks!
> > >
> >
> > Patch 4/8 was not acked. I was about to push a new version in which I
> > fix the scenario that Jeremy mentioned.
> 
> Well, it wasn't clear to me what you wanted to do about it.
> 

Sorry about the confusion.

> > Would you like me to push that
> > as a separate patch on top of this series,
> 
> Yes, please.

Sent at:
https://lore.kernel.org/linux-pm/20201117184920.17036-1-ionela.voinescu@arm.com/

Thank you,
Ionela.

[PATCH] cppc_cpufreq: optimise memory allocation for HW and NONE coordination

[Ionela Voinescu]

While the current domain and cpu lists are appropriate for ALL and ANY
coordination types where single structures are kept for the domain and
CPU data, they can be inefficient for NONE and HW coordination types,
where domain information can end up being allocated either for a single
CPU or a small number of CPUs.

Therefore remove the psd_data structure and maintain a single CPU list.
The cppc_cpudata structure will contain information about the PSD domain
of the CPU: the ACPI coordination type and CPU content. This does result
in the duplication of domain information in the cppc_cpudata structure
(type and map), but it is more memory efficient for all coordination
types, compared to allocating separate structures.

In order to accommodate the struct list_head node in the cppc_cpudata
structure, the now unused cpu and cur_policy variables are removed.

This change affects all ACPI coordination types, with the greatest
savings obtained for HW and NONE coordination, when the number of CPUs
is large.

For example, on a arm64 Juno platform with 6 CPUs:
 - (0) 0, 1, 2, 3, 4, 5 - NONE coordination
 - (1) (0, 1, 2, 3) in PSD1, (4, 5) in PSD2 - ANY coordination

memory allocation comparison shows:

Before patch: psd_data and cppc_cpudata structures are allocated for each
              CPU (0) or domain (1).

 - (0) NONE coordination:
    total    slack      req alloc/free  caller
       0        0        0     0/6     _kernel_size_le_hi32+0x0xffff800008ff7958
     768      336      432     6/0     _kernel_size_le_hi32+0x0xffff800008ff7444
       0        0        0     0/6     _kernel_size_le_hi32+0x0xffff800008ff7990
     768       96      672     6/0     _kernel_size_le_hi32+0x0xffff800008ff7604

 - (1) ANY coordination:
    total    slack      req alloc/free  caller
       0        0        0     0/2     _kernel_size_le_hi32+0x0xffff800008fe7990
     256      112      144     2/0     _kernel_size_le_hi32+0x0xffff800008fe7444
     256       32      224     2/0     _kernel_size_le_hi32+0x0xffff800008fe7604
       0        0        0     0/2     _kernel_size_le_hi32+0x0xffff800008fe7958

After patch: only cppc_cpudata is allocated for each CPU (0) or domain (1).

 - (0) NONE coordination:
    total    slack      req alloc/free  caller
     768        0      768     6/0     _kernel_size_le_hi32+0x0xffff800008ffd410
       0        0        0     0/6     _kernel_size_le_hi32+0x0xffff800008ffd274

 - (1) ANY coordination:
    total    slack      req alloc/free  caller
     256        0      256     2/0     _kernel_size_le_hi32+0x0xffff800008fed410
       0        0        0     0/2     _kernel_size_le_hi32+0x0xffff800008fed274

Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
---

Hi guys,

This is an optimisation/fix for the inefficient allocation that Jeremy
mentioned for patch 4/8 in the series at [1]. This reverts the use of a
separate psd_data structure and some of the changes done in cppc_cpudata,
while adding the list_head needed to maintain a cpu list and removing the
unused cpu and cur_policy variables.

This patch is based on v5.10-rc4.

Thanks,
Ionela.

[1] https://lore.kernel.org/linux-pm/20201105125524.4409-1-ionela.voinescu@arm.com/


 drivers/acpi/cppc_acpi.c       |  20 ++---
 drivers/cpufreq/cppc_cpufreq.c | 131 +++++++++++----------------------
 include/acpi/cppc_acpi.h       |  15 +---
 3 files changed, 54 insertions(+), 112 deletions(-)

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index e1e46cc66eeb..4e478f751ff7 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -416,11 +416,11 @@ static int acpi_get_psd(struct cpc_desc *cpc_ptr, acpi_handle handle)
 /**
  * acpi_get_psd_map - Map the CPUs in the freq domain of a given cpu
  * @cpu: Find all CPUs that share a domain with cpu.
- * @domain: Pointer to given domain data storage
+ * @cpu_data: Pointer to CPU specific CPPC data including PSD info.
  *
  *	Return: 0 for success or negative value for err.
  */
-int acpi_get_psd_map(unsigned int cpu, struct psd_data *domain)
+int acpi_get_psd_map(unsigned int cpu, struct cppc_cpudata *cpu_data)
 {
 	struct cpc_desc *cpc_ptr, *match_cpc_ptr;
 	struct acpi_psd_package *match_pdomain;
@@ -436,18 +436,18 @@ int acpi_get_psd_map(unsigned int cpu, struct psd_data *domain)
 		return -EFAULT;
 
 	pdomain = &(cpc_ptr->domain_info);
-	cpumask_set_cpu(cpu, domain->shared_cpu_map);
+	cpumask_set_cpu(cpu, cpu_data->shared_cpu_map);
 	if (pdomain->num_processors <= 1)
 		return 0;
 
 	/* Validate the Domain info */
 	count_target = pdomain->num_processors;
 	if (pdomain->coord_type == DOMAIN_COORD_TYPE_SW_ALL)
-		domain->shared_type = CPUFREQ_SHARED_TYPE_ALL;
+		cpu_data->shared_type = CPUFREQ_SHARED_TYPE_ALL;
 	else if (pdomain->coord_type == DOMAIN_COORD_TYPE_HW_ALL)
-		domain->shared_type = CPUFREQ_SHARED_TYPE_HW;
+		cpu_data->shared_type = CPUFREQ_SHARED_TYPE_HW;
 	else if (pdomain->coord_type == DOMAIN_COORD_TYPE_SW_ANY)
-		domain->shared_type = CPUFREQ_SHARED_TYPE_ANY;
+		cpu_data->shared_type = CPUFREQ_SHARED_TYPE_ANY;
 
 	for_each_possible_cpu(i) {
 		if (i == cpu)
@@ -468,16 +468,16 @@ int acpi_get_psd_map(unsigned int cpu, struct psd_data *domain)
 		if (pdomain->coord_type != match_pdomain->coord_type)
 			goto err_fault;
 
-		cpumask_set_cpu(i, domain->shared_cpu_map);
+		cpumask_set_cpu(i, cpu_data->shared_cpu_map);
 	}
 
 	return 0;
 
 err_fault:
 	/* Assume no coordination on any error parsing domain info */
-	cpumask_clear(domain->shared_cpu_map);
-	cpumask_set_cpu(cpu, domain->shared_cpu_map);
-	domain->shared_type = CPUFREQ_SHARED_TYPE_NONE;
+	cpumask_clear(cpu_data->shared_cpu_map);
+	cpumask_set_cpu(cpu, cpu_data->shared_cpu_map);
+	cpu_data->shared_type = CPUFREQ_SHARED_TYPE_NONE;
 
 	return -EFAULT;
 }
diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
index b4edeeb57d04..bb4c068601db 100644
--- a/drivers/cpufreq/cppc_cpufreq.c
+++ b/drivers/cpufreq/cppc_cpufreq.c
@@ -31,10 +31,11 @@
 
 /*
  * This list contains information parsed from per CPU ACPI _CPC and _PSD
- * structures: this is a list of domain data which in turn contains a list
- * of cpus with their controls and capabilities, belonging to the domain.
+ * structures: e.g. the highest and lowest supported performance, capabilities,
+ * desired performance, level requested etc. Depending on the share_type, not
+ * all CPUs will have an entry in the list.
  */
-static LIST_HEAD(domain_list);
+static LIST_HEAD(cpu_data_list);
 
 static bool boost_supported;
 
@@ -194,6 +195,12 @@ static void cppc_cpufreq_stop_cpu(struct cpufreq_policy *policy)
 	if (ret)
 		pr_debug("Err setting perf value:%d on CPU:%d. ret:%d\n",
 			 caps->lowest_perf, cpu, ret);
+
+	/* Remove CPU node from list and free driver data for policy */
+	free_cpumask_var(cpu_data->shared_cpu_map);
+	list_del(&cpu_data->node);
+	kfree(policy->driver_data);
+	policy->driver_data = NULL;
 }
 
 /*
@@ -239,105 +246,59 @@ static unsigned int cppc_cpufreq_get_transition_delay_us(unsigned int cpu)
 }
 #endif
 
-static struct psd_data *cppc_cpufreq_get_domain(unsigned int cpu)
-{
-	struct psd_data *domain;
-	int ret;
-
-	list_for_each_entry(domain, &domain_list, node) {
-		if (cpumask_test_cpu(cpu, domain->shared_cpu_map))
-			return domain;
-	}
-
-	domain = kzalloc(sizeof(struct psd_data), GFP_KERNEL);
-	if (!domain)
-		return NULL;
-	if (!zalloc_cpumask_var(&domain->shared_cpu_map, GFP_KERNEL))
-		goto free_domain;
-	INIT_LIST_HEAD(&domain->cpu_list);
-
-	ret = acpi_get_psd_map(cpu, domain);
-	if (ret) {
-		pr_err("Error parsing PSD data for CPU%d.\n", cpu);
-		goto free_mask;
-	}
-
-	list_add(&domain->node, &domain_list);
-
-	return domain;
-
-free_mask:
-	free_cpumask_var(domain->shared_cpu_map);
-free_domain:
-	kfree(domain);
-
-	return NULL;
-}
 
 static struct cppc_cpudata *cppc_cpufreq_get_cpu_data(unsigned int cpu)
 {
 	struct cppc_cpudata *cpu_data;
-	struct psd_data *domain;
 	int ret;
 
-	domain = cppc_cpufreq_get_domain(cpu);
-	if (!domain) {
-		pr_err("Error acquiring domain for CPU.\n");
-		return NULL;
-	}
-
-	list_for_each_entry(cpu_data, &domain->cpu_list, node) {
-		if (cpu_data->cpu == cpu)
-			return cpu_data;
-	}
-
-	if ((domain->shared_type == CPUFREQ_SHARED_TYPE_ANY) &&
-	    !list_empty(&domain->cpu_list))
-		return list_first_entry(&domain->cpu_list,
-					struct cppc_cpudata,
-					node);
-
 	cpu_data = kzalloc(sizeof(struct cppc_cpudata), GFP_KERNEL);
 	if (!cpu_data)
-		return NULL;
+		goto out;
+
+	if (!zalloc_cpumask_var(&cpu_data->shared_cpu_map, GFP_KERNEL))
+		goto free_cpu;
 
-	cpu_data->cpu = cpu;
-	cpu_data->domain = domain;
+	ret = acpi_get_psd_map(cpu, cpu_data);
+	if (ret) {
+		pr_debug("Err parsing CPU%d PSD data: ret:%d\n", cpu, ret);
+		goto free_mask;
+	}
 
 	ret = cppc_get_perf_caps(cpu, &cpu_data->perf_caps);
 	if (ret) {
-		pr_err("Err reading CPU%d perf capabilities. ret:%d\n",
-			cpu, ret);
-		goto free;
+		pr_debug("Err reading CPU%d perf caps: ret:%d\n", cpu, ret);
+		goto free_mask;
 	}
+
 	/* Convert the lowest and nominal freq from MHz to KHz */
 	cpu_data->perf_caps.lowest_freq *= 1000;
 	cpu_data->perf_caps.nominal_freq *= 1000;
 
-	list_add(&cpu_data->node, &domain->cpu_list);
+	list_add(&cpu_data->node, &cpu_data_list);
 
 	return cpu_data;
-free:
-	kfree(cpu_data);
 
+free_mask:
+	free_cpumask_var(cpu_data->shared_cpu_map);
+free_cpu:
+	kfree(cpu_data);
+out:
 	return NULL;
 }
 
 static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
 {
-	struct cppc_cpudata *cpu_data = NULL;
-	struct psd_data *domain = NULL;
 	unsigned int cpu = policy->cpu;
+	struct cppc_cpudata *cpu_data;
 	struct cppc_perf_caps *caps;
-	int ret = 0;
+	int ret;
 
 	cpu_data = cppc_cpufreq_get_cpu_data(cpu);
 	if (!cpu_data) {
-		pr_err("Error in acquiring _CPC/_PSD data for CPU.\n");
+		pr_err("Error in acquiring _CPC/_PSD data for CPU%d.\n", cpu);
 		return -ENODEV;
 	}
-
-	domain = cpu_data->domain;
 	caps = &cpu_data->perf_caps;
 	policy->driver_data = cpu_data;
 
@@ -361,7 +322,7 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
 							    caps->nominal_perf);
 
 	policy->transition_delay_us = cppc_cpufreq_get_transition_delay_us(cpu);
-	policy->shared_type = domain->shared_type;
+	policy->shared_type = cpu_data->shared_type;
 
 	switch (policy->shared_type) {
 	case CPUFREQ_SHARED_TYPE_HW:
@@ -374,7 +335,7 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
 		 * operations will use a single cppc_cpudata structure stored
 		 * in policy->driver_data.
 		 */
-		cpumask_copy(policy->cpus, domain->shared_cpu_map);
+		cpumask_copy(policy->cpus, cpu_data->shared_cpu_map);
 		break;
 	default:
 		pr_info("Unsupported cpufreq CPU co-ord type: %d\n",
@@ -382,8 +343,6 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
 		return -EFAULT;
 	}
 
-	cpu_data->cur_policy = policy;
-
 	/*
 	 * If 'highest_perf' is greater than 'nominal_perf', we assume CPU Boost
 	 * is supported.
@@ -487,7 +446,7 @@ static ssize_t show_freqdomain_cpus(struct cpufreq_policy *policy, char *buf)
 {
 	struct cppc_cpudata *cpu_data = policy->driver_data;
 
-	return cpufreq_show_cpus(cpu_data->domain->shared_cpu_map, buf);
+	return cpufreq_show_cpus(cpu_data->shared_cpu_map, buf);
 }
 cpufreq_freq_attr_ro(freqdomain_cpus);
 
@@ -558,38 +517,30 @@ static int __init cppc_cpufreq_init(void)
 	if (acpi_disabled)
 		return -ENODEV;
 
+	INIT_LIST_HEAD(&cpu_data_list);
+
 	cppc_check_hisi_workaround();
 
 	return cpufreq_register_driver(&cppc_cpufreq_driver);
 }
 
-static inline void free_cpu_data(struct psd_data *domain)
+static inline void free_cpu_data(void)
 {
 	struct cppc_cpudata *iter, *tmp;
 
-	list_for_each_entry_safe(iter, tmp, &domain->cpu_list, node) {
+	list_for_each_entry_safe(iter, tmp, &cpu_data_list, node) {
+		free_cpumask_var(iter->shared_cpu_map);
 		list_del(&iter->node);
 		kfree(iter);
 	}
-}
-
-static inline void free_domain_data(void)
-{
-	struct psd_data *iter, *tmp;
 
-	list_for_each_entry_safe(iter, tmp, &domain_list, node) {
-		list_del(&iter->node);
-		if (!list_empty(&iter->cpu_list))
-			free_cpu_data(iter);
-		free_cpumask_var(iter->shared_cpu_map);
-		kfree(iter);
-	}
 }
+
 static void __exit cppc_cpufreq_exit(void)
 {
 	cpufreq_unregister_driver(&cppc_cpufreq_driver);
 
-	free_domain_data();
+	free_cpu_data();
 }
 
 module_exit(cppc_cpufreq_exit);
diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h
index c0081fb6032e..dab6b3b4e315 100644
--- a/include/acpi/cppc_acpi.h
+++ b/include/acpi/cppc_acpi.h
@@ -122,30 +122,21 @@ struct cppc_perf_fb_ctrs {
 	u64 wraparound_time;
 };
 
-/* Container of performance state domain data */
-struct psd_data {
-	struct list_head node;
-	unsigned int shared_type;
-	struct list_head cpu_list;
-	cpumask_var_t shared_cpu_map;
-};
-
 /* Per CPU container for runtime CPPC management. */
 struct cppc_cpudata {
-	int cpu;
 	struct list_head node;
-	struct psd_data *domain;
 	struct cppc_perf_caps perf_caps;
 	struct cppc_perf_ctrls perf_ctrls;
 	struct cppc_perf_fb_ctrs perf_fb_ctrs;
-	struct cpufreq_policy *cur_policy;
+	unsigned int shared_type;
+	cpumask_var_t shared_cpu_map;
 };
 
 extern int cppc_get_desired_perf(int cpunum, u64 *desired_perf);
 extern int cppc_get_perf_ctrs(int cpu, struct cppc_perf_fb_ctrs *perf_fb_ctrs);
 extern int cppc_set_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls);
 extern int cppc_get_perf_caps(int cpu, struct cppc_perf_caps *caps);
-extern int acpi_get_psd_map(unsigned int cpu, struct psd_data *domain);
+extern int acpi_get_psd_map(unsigned int cpu, struct cppc_cpudata *cpu_data);
 extern unsigned int cppc_get_transition_latency(int cpu);
 extern bool cpc_ffh_supported(void);
 extern int cpc_read_ffh(int cpunum, struct cpc_reg *reg, u64 *val);
-- 
2.17.1

Re: [PATCH] cppc_cpufreq: optimise memory allocation for HW and NONE coordination

[Rafael J. Wysocki]

On Tue, Nov 17, 2020 at 7:50 PM Ionela Voinescu <ionela.voinescu@arm.com> wrote:
>
> While the current domain and cpu lists are appropriate for ALL and ANY
> coordination types where single structures are kept for the domain and
> CPU data, they can be inefficient for NONE and HW coordination types,
> where domain information can end up being allocated either for a single
> CPU or a small number of CPUs.
>
> Therefore remove the psd_data structure and maintain a single CPU list.
> The cppc_cpudata structure will contain information about the PSD domain
> of the CPU: the ACPI coordination type and CPU content. This does result
> in the duplication of domain information in the cppc_cpudata structure
> (type and map), but it is more memory efficient for all coordination
> types, compared to allocating separate structures.
>
> In order to accommodate the struct list_head node in the cppc_cpudata
> structure, the now unused cpu and cur_policy variables are removed.
>
> This change affects all ACPI coordination types, with the greatest
> savings obtained for HW and NONE coordination, when the number of CPUs
> is large.
>
> For example, on a arm64 Juno platform with 6 CPUs:
>  - (0) 0, 1, 2, 3, 4, 5 - NONE coordination
>  - (1) (0, 1, 2, 3) in PSD1, (4, 5) in PSD2 - ANY coordination
>
> memory allocation comparison shows:
>
> Before patch: psd_data and cppc_cpudata structures are allocated for each
>               CPU (0) or domain (1).
>
>  - (0) NONE coordination:
>     total    slack      req alloc/free  caller
>        0        0        0     0/6     _kernel_size_le_hi32+0x0xffff800008ff7958
>      768      336      432     6/0     _kernel_size_le_hi32+0x0xffff800008ff7444
>        0        0        0     0/6     _kernel_size_le_hi32+0x0xffff800008ff7990
>      768       96      672     6/0     _kernel_size_le_hi32+0x0xffff800008ff7604
>
>  - (1) ANY coordination:
>     total    slack      req alloc/free  caller
>        0        0        0     0/2     _kernel_size_le_hi32+0x0xffff800008fe7990
>      256      112      144     2/0     _kernel_size_le_hi32+0x0xffff800008fe7444
>      256       32      224     2/0     _kernel_size_le_hi32+0x0xffff800008fe7604
>        0        0        0     0/2     _kernel_size_le_hi32+0x0xffff800008fe7958
>
> After patch: only cppc_cpudata is allocated for each CPU (0) or domain (1).
>
>  - (0) NONE coordination:
>     total    slack      req alloc/free  caller
>      768        0      768     6/0     _kernel_size_le_hi32+0x0xffff800008ffd410
>        0        0        0     0/6     _kernel_size_le_hi32+0x0xffff800008ffd274
>
>  - (1) ANY coordination:
>     total    slack      req alloc/free  caller
>      256        0      256     2/0     _kernel_size_le_hi32+0x0xffff800008fed410
>        0        0        0     0/2     _kernel_size_le_hi32+0x0xffff800008fed274
>
> Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com>
> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>
> Hi guys,
>
> This is an optimisation/fix for the inefficient allocation that Jeremy
> mentioned for patch 4/8 in the series at [1]. This reverts the use of a
> separate psd_data structure and some of the changes done in cppc_cpudata,
> while adding the list_head needed to maintain a cpu list and removing the
> unused cpu and cur_policy variables.
>
> This patch is based on v5.10-rc4.
>
> Thanks,
> Ionela.
>
> [1] https://lore.kernel.org/linux-pm/20201105125524.4409-1-ionela.voinescu@arm.com/
>
>
>  drivers/acpi/cppc_acpi.c       |  20 ++---
>  drivers/cpufreq/cppc_cpufreq.c | 131 +++++++++++----------------------
>  include/acpi/cppc_acpi.h       |  15 +---
>  3 files changed, 54 insertions(+), 112 deletions(-)
>
> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> index e1e46cc66eeb..4e478f751ff7 100644
> --- a/drivers/acpi/cppc_acpi.c
> +++ b/drivers/acpi/cppc_acpi.c
> @@ -416,11 +416,11 @@ static int acpi_get_psd(struct cpc_desc *cpc_ptr, acpi_handle handle)
>  /**
>   * acpi_get_psd_map - Map the CPUs in the freq domain of a given cpu
>   * @cpu: Find all CPUs that share a domain with cpu.
> - * @domain: Pointer to given domain data storage
> + * @cpu_data: Pointer to CPU specific CPPC data including PSD info.
>   *
>   *     Return: 0 for success or negative value for err.
>   */
> -int acpi_get_psd_map(unsigned int cpu, struct psd_data *domain)
> +int acpi_get_psd_map(unsigned int cpu, struct cppc_cpudata *cpu_data)
>  {
>         struct cpc_desc *cpc_ptr, *match_cpc_ptr;
>         struct acpi_psd_package *match_pdomain;
> @@ -436,18 +436,18 @@ int acpi_get_psd_map(unsigned int cpu, struct psd_data *domain)
>                 return -EFAULT;
>
>         pdomain = &(cpc_ptr->domain_info);
> -       cpumask_set_cpu(cpu, domain->shared_cpu_map);
> +       cpumask_set_cpu(cpu, cpu_data->shared_cpu_map);
>         if (pdomain->num_processors <= 1)
>                 return 0;
>
>         /* Validate the Domain info */
>         count_target = pdomain->num_processors;
>         if (pdomain->coord_type == DOMAIN_COORD_TYPE_SW_ALL)
> -               domain->shared_type = CPUFREQ_SHARED_TYPE_ALL;
> +               cpu_data->shared_type = CPUFREQ_SHARED_TYPE_ALL;
>         else if (pdomain->coord_type == DOMAIN_COORD_TYPE_HW_ALL)
> -               domain->shared_type = CPUFREQ_SHARED_TYPE_HW;
> +               cpu_data->shared_type = CPUFREQ_SHARED_TYPE_HW;
>         else if (pdomain->coord_type == DOMAIN_COORD_TYPE_SW_ANY)
> -               domain->shared_type = CPUFREQ_SHARED_TYPE_ANY;
> +               cpu_data->shared_type = CPUFREQ_SHARED_TYPE_ANY;
>
>         for_each_possible_cpu(i) {
>                 if (i == cpu)
> @@ -468,16 +468,16 @@ int acpi_get_psd_map(unsigned int cpu, struct psd_data *domain)
>                 if (pdomain->coord_type != match_pdomain->coord_type)
>                         goto err_fault;
>
> -               cpumask_set_cpu(i, domain->shared_cpu_map);
> +               cpumask_set_cpu(i, cpu_data->shared_cpu_map);
>         }
>
>         return 0;
>
>  err_fault:
>         /* Assume no coordination on any error parsing domain info */
> -       cpumask_clear(domain->shared_cpu_map);
> -       cpumask_set_cpu(cpu, domain->shared_cpu_map);
> -       domain->shared_type = CPUFREQ_SHARED_TYPE_NONE;
> +       cpumask_clear(cpu_data->shared_cpu_map);
> +       cpumask_set_cpu(cpu, cpu_data->shared_cpu_map);
> +       cpu_data->shared_type = CPUFREQ_SHARED_TYPE_NONE;
>
>         return -EFAULT;
>  }
> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> index b4edeeb57d04..bb4c068601db 100644
> --- a/drivers/cpufreq/cppc_cpufreq.c
> +++ b/drivers/cpufreq/cppc_cpufreq.c
> @@ -31,10 +31,11 @@
>
>  /*
>   * This list contains information parsed from per CPU ACPI _CPC and _PSD
> - * structures: this is a list of domain data which in turn contains a list
> - * of cpus with their controls and capabilities, belonging to the domain.
> + * structures: e.g. the highest and lowest supported performance, capabilities,
> + * desired performance, level requested etc. Depending on the share_type, not
> + * all CPUs will have an entry in the list.
>   */
> -static LIST_HEAD(domain_list);
> +static LIST_HEAD(cpu_data_list);
>
>  static bool boost_supported;
>
> @@ -194,6 +195,12 @@ static void cppc_cpufreq_stop_cpu(struct cpufreq_policy *policy)
>         if (ret)
>                 pr_debug("Err setting perf value:%d on CPU:%d. ret:%d\n",
>                          caps->lowest_perf, cpu, ret);
> +
> +       /* Remove CPU node from list and free driver data for policy */
> +       free_cpumask_var(cpu_data->shared_cpu_map);
> +       list_del(&cpu_data->node);
> +       kfree(policy->driver_data);
> +       policy->driver_data = NULL;
>  }
>
>  /*
> @@ -239,105 +246,59 @@ static unsigned int cppc_cpufreq_get_transition_delay_us(unsigned int cpu)
>  }
>  #endif
>
> -static struct psd_data *cppc_cpufreq_get_domain(unsigned int cpu)
> -{
> -       struct psd_data *domain;
> -       int ret;
> -
> -       list_for_each_entry(domain, &domain_list, node) {
> -               if (cpumask_test_cpu(cpu, domain->shared_cpu_map))
> -                       return domain;
> -       }
> -
> -       domain = kzalloc(sizeof(struct psd_data), GFP_KERNEL);
> -       if (!domain)
> -               return NULL;
> -       if (!zalloc_cpumask_var(&domain->shared_cpu_map, GFP_KERNEL))
> -               goto free_domain;
> -       INIT_LIST_HEAD(&domain->cpu_list);
> -
> -       ret = acpi_get_psd_map(cpu, domain);
> -       if (ret) {
> -               pr_err("Error parsing PSD data for CPU%d.\n", cpu);
> -               goto free_mask;
> -       }
> -
> -       list_add(&domain->node, &domain_list);
> -
> -       return domain;
> -
> -free_mask:
> -       free_cpumask_var(domain->shared_cpu_map);
> -free_domain:
> -       kfree(domain);
> -
> -       return NULL;
> -}
>
>  static struct cppc_cpudata *cppc_cpufreq_get_cpu_data(unsigned int cpu)
>  {
>         struct cppc_cpudata *cpu_data;
> -       struct psd_data *domain;
>         int ret;
>
> -       domain = cppc_cpufreq_get_domain(cpu);
> -       if (!domain) {
> -               pr_err("Error acquiring domain for CPU.\n");
> -               return NULL;
> -       }
> -
> -       list_for_each_entry(cpu_data, &domain->cpu_list, node) {
> -               if (cpu_data->cpu == cpu)
> -                       return cpu_data;
> -       }
> -
> -       if ((domain->shared_type == CPUFREQ_SHARED_TYPE_ANY) &&
> -           !list_empty(&domain->cpu_list))
> -               return list_first_entry(&domain->cpu_list,
> -                                       struct cppc_cpudata,
> -                                       node);
> -
>         cpu_data = kzalloc(sizeof(struct cppc_cpudata), GFP_KERNEL);
>         if (!cpu_data)
> -               return NULL;
> +               goto out;
> +
> +       if (!zalloc_cpumask_var(&cpu_data->shared_cpu_map, GFP_KERNEL))
> +               goto free_cpu;
>
> -       cpu_data->cpu = cpu;
> -       cpu_data->domain = domain;
> +       ret = acpi_get_psd_map(cpu, cpu_data);
> +       if (ret) {
> +               pr_debug("Err parsing CPU%d PSD data: ret:%d\n", cpu, ret);
> +               goto free_mask;
> +       }
>
>         ret = cppc_get_perf_caps(cpu, &cpu_data->perf_caps);
>         if (ret) {
> -               pr_err("Err reading CPU%d perf capabilities. ret:%d\n",
> -                       cpu, ret);
> -               goto free;
> +               pr_debug("Err reading CPU%d perf caps: ret:%d\n", cpu, ret);
> +               goto free_mask;
>         }
> +
>         /* Convert the lowest and nominal freq from MHz to KHz */
>         cpu_data->perf_caps.lowest_freq *= 1000;
>         cpu_data->perf_caps.nominal_freq *= 1000;
>
> -       list_add(&cpu_data->node, &domain->cpu_list);
> +       list_add(&cpu_data->node, &cpu_data_list);
>
>         return cpu_data;
> -free:
> -       kfree(cpu_data);
>
> +free_mask:
> +       free_cpumask_var(cpu_data->shared_cpu_map);
> +free_cpu:
> +       kfree(cpu_data);
> +out:
>         return NULL;
>  }
>
>  static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
>  {
> -       struct cppc_cpudata *cpu_data = NULL;
> -       struct psd_data *domain = NULL;
>         unsigned int cpu = policy->cpu;
> +       struct cppc_cpudata *cpu_data;
>         struct cppc_perf_caps *caps;
> -       int ret = 0;
> +       int ret;
>
>         cpu_data = cppc_cpufreq_get_cpu_data(cpu);
>         if (!cpu_data) {
> -               pr_err("Error in acquiring _CPC/_PSD data for CPU.\n");
> +               pr_err("Error in acquiring _CPC/_PSD data for CPU%d.\n", cpu);
>                 return -ENODEV;
>         }
> -
> -       domain = cpu_data->domain;
>         caps = &cpu_data->perf_caps;
>         policy->driver_data = cpu_data;
>
> @@ -361,7 +322,7 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
>                                                             caps->nominal_perf);
>
>         policy->transition_delay_us = cppc_cpufreq_get_transition_delay_us(cpu);
> -       policy->shared_type = domain->shared_type;
> +       policy->shared_type = cpu_data->shared_type;
>
>         switch (policy->shared_type) {
>         case CPUFREQ_SHARED_TYPE_HW:
> @@ -374,7 +335,7 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
>                  * operations will use a single cppc_cpudata structure stored
>                  * in policy->driver_data.
>                  */
> -               cpumask_copy(policy->cpus, domain->shared_cpu_map);
> +               cpumask_copy(policy->cpus, cpu_data->shared_cpu_map);
>                 break;
>         default:
>                 pr_info("Unsupported cpufreq CPU co-ord type: %d\n",
> @@ -382,8 +343,6 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
>                 return -EFAULT;
>         }
>
> -       cpu_data->cur_policy = policy;
> -
>         /*
>          * If 'highest_perf' is greater than 'nominal_perf', we assume CPU Boost
>          * is supported.
> @@ -487,7 +446,7 @@ static ssize_t show_freqdomain_cpus(struct cpufreq_policy *policy, char *buf)
>  {
>         struct cppc_cpudata *cpu_data = policy->driver_data;
>
> -       return cpufreq_show_cpus(cpu_data->domain->shared_cpu_map, buf);
> +       return cpufreq_show_cpus(cpu_data->shared_cpu_map, buf);
>  }
>  cpufreq_freq_attr_ro(freqdomain_cpus);
>
> @@ -558,38 +517,30 @@ static int __init cppc_cpufreq_init(void)
>         if (acpi_disabled)
>                 return -ENODEV;
>
> +       INIT_LIST_HEAD(&cpu_data_list);
> +
>         cppc_check_hisi_workaround();
>
>         return cpufreq_register_driver(&cppc_cpufreq_driver);
>  }
>
> -static inline void free_cpu_data(struct psd_data *domain)
> +static inline void free_cpu_data(void)
>  {
>         struct cppc_cpudata *iter, *tmp;
>
> -       list_for_each_entry_safe(iter, tmp, &domain->cpu_list, node) {
> +       list_for_each_entry_safe(iter, tmp, &cpu_data_list, node) {
> +               free_cpumask_var(iter->shared_cpu_map);
>                 list_del(&iter->node);
>                 kfree(iter);
>         }
> -}
> -
> -static inline void free_domain_data(void)
> -{
> -       struct psd_data *iter, *tmp;
>
> -       list_for_each_entry_safe(iter, tmp, &domain_list, node) {
> -               list_del(&iter->node);
> -               if (!list_empty(&iter->cpu_list))
> -                       free_cpu_data(iter);
> -               free_cpumask_var(iter->shared_cpu_map);
> -               kfree(iter);
> -       }
>  }
> +
>  static void __exit cppc_cpufreq_exit(void)
>  {
>         cpufreq_unregister_driver(&cppc_cpufreq_driver);
>
> -       free_domain_data();
> +       free_cpu_data();
>  }
>
>  module_exit(cppc_cpufreq_exit);
> diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h
> index c0081fb6032e..dab6b3b4e315 100644
> --- a/include/acpi/cppc_acpi.h
> +++ b/include/acpi/cppc_acpi.h
> @@ -122,30 +122,21 @@ struct cppc_perf_fb_ctrs {
>         u64 wraparound_time;
>  };
>
> -/* Container of performance state domain data */
> -struct psd_data {
> -       struct list_head node;
> -       unsigned int shared_type;
> -       struct list_head cpu_list;
> -       cpumask_var_t shared_cpu_map;
> -};
> -
>  /* Per CPU container for runtime CPPC management. */
>  struct cppc_cpudata {
> -       int cpu;
>         struct list_head node;
> -       struct psd_data *domain;
>         struct cppc_perf_caps perf_caps;
>         struct cppc_perf_ctrls perf_ctrls;
>         struct cppc_perf_fb_ctrs perf_fb_ctrs;
> -       struct cpufreq_policy *cur_policy;
> +       unsigned int shared_type;
> +       cpumask_var_t shared_cpu_map;
>  };
>
>  extern int cppc_get_desired_perf(int cpunum, u64 *desired_perf);
>  extern int cppc_get_perf_ctrs(int cpu, struct cppc_perf_fb_ctrs *perf_fb_ctrs);
>  extern int cppc_set_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls);
>  extern int cppc_get_perf_caps(int cpu, struct cppc_perf_caps *caps);
> -extern int acpi_get_psd_map(unsigned int cpu, struct psd_data *domain);
> +extern int acpi_get_psd_map(unsigned int cpu, struct cppc_cpudata *cpu_data);
>  extern unsigned int cppc_get_transition_latency(int cpu);
>  extern bool cpc_ffh_supported(void);
>  extern int cpc_read_ffh(int cpunum, struct cpc_reg *reg, u64 *val);
> --

Applied as 5.11 material (on top of the previous cppc_cpufreq patches), thanks!