Skip to contentSkip to conversation

arch_topology: Minor cleanups

PATCH In review Viresh Kumar 2 participants 7 messages Jun 23, 2017 – Jun 27, 2017

Subsystems (2)
Driver core Generic architecture topology
lore
Share & tools
lore

Viresh Kumar

author
#0 lore raw

v3 cover arch_topology: Minor cleanups

Hi Greg,

You weren't included in the first [1] version of this series, as it was targeting arch/arm*/ directories then.

Here are some cleanups for the arch_topology core.

Tested on ARM64 Hikey board by setting following in cpu nodes in DT:

        capacity-dmips-mhz = <1000>;

V2->V3:
- topology_parse_cpu_capacity() returns bool now (Juri).
- "Get rid of cap_parsing_done" is the last patch now and its implementation is change a bit (Juri).

V1->V2:
- based over linux-next/master (to get Juri's recent changes)
- More cleanups included. V1 only had the first patch.
- Rename of cap_parsing_failed isn't required anymore (as it is localized to a single routine now).

--
viresh

[1] https://marc.info/?l=linux-arm-kernel&m=149795553024005

Viresh Kumar (5):
arch_topology: Don't break lines unnecessarily
arch_topology: Convert switch block to if block
arch_topology: Change return type of topology_parse_cpu_capacity() to
bool
arch_topology: Localize cap_parsing_failed to
topology_parse_cpu_capacity()
arch_topology: Get rid of cap_parsing_done

 drivers/base/arch_topology.c  | 78 +++++++++++++++++++++----------------------
 include/linux/arch_topology.h |  4 ++-
 2 files changed, 42 insertions(+), 40 deletions(-)
Signature · 1 line
2.13.0.71.gd7076ec9c9cb
author
#1 lore raw

PATCH v3 1/5 arch_topology: Don't break lines unnecessarily

There is no need of line break at few places, avoid them.

Signed-off-by Viresh Kumar <[email protected]>
---
 drivers/base/arch_topology.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)
drivers/base/arch_topology.c +3 -6
index d1c33a85059e..0ad79b5cd56d 100644
@@ -41,8 +41,7 @@ static ssize_t cpu_capacity_show(struct device *dev,
 {
 	struct cpu *cpu = container_of(dev, struct cpu, dev);
 
-	return sprintf(buf, "%lu\n",
-			topology_get_cpu_scale(NULL, cpu->dev.id));
+	return sprintf(buf, "%lu\n", topology_get_cpu_scale(NULL, cpu->dev.id));
 }
 
 static ssize_t cpu_capacity_store(struct device *dev,

@@ -128,8 +127,7 @@ int __init topology_parse_cpu_capacity(struct device_node *cpu_node, int cpu)
 	if (cap_parsing_failed)
 		return !ret;
 
-	ret = of_property_read_u32(cpu_node,
-				   "capacity-dmips-mhz",
+	ret = of_property_read_u32(cpu_node, "capacity-dmips-mhz",
 				   &cpu_capacity);
 	if (!ret) {
 		if (!raw_capacity) {

@@ -181,8 +179,7 @@ init_cpu_capacity_callback(struct notifier_block *nb,
 		pr_debug("cpu_capacity: init cpu capacity for CPUs [%*pbl] (to_visit=%*pbl)\n",
 				cpumask_pr_args(policy->related_cpus),
 				cpumask_pr_args(cpus_to_visit));
-		cpumask_andnot(cpus_to_visit,
-			       cpus_to_visit,
+		cpumask_andnot(cpus_to_visit, cpus_to_visit,
 			       policy->related_cpus);
 		for_each_cpu(cpu, policy->related_cpus) {
 			raw_capacity[cpu] = topology_get_cpu_scale(NULL, cpu) *
Signature · 1 line
2.13.0.71.gd7076ec9c9cb
author
#2 lore raw

PATCH v3 2/5 arch_topology: Convert switch block to if block

In reply to Viresh Kumar

We only need to take care of one case here (CPUFREQ_NOTIFY) and there is no need to add an extra level of indentation to the case specific code by using a switch block. Use an if block instead.

Also add some blank lines to make the code look better.

Signed-off-by Viresh Kumar <[email protected]>
---
 drivers/base/arch_topology.c | 41 ++++++++++++++++++++++-------------------
 1 file changed, 22 insertions(+), 19 deletions(-)
drivers/base/arch_topology.c +22 -19
index 0ad79b5cd56d..a3cd7c869c3e 100644
@@ -174,26 +174,29 @@ init_cpu_capacity_callback(struct notifier_block *nb,
 	if (cap_parsing_failed || cap_parsing_done)
 		return 0;
 
-	switch (val) {
-	case CPUFREQ_NOTIFY:
-		pr_debug("cpu_capacity: init cpu capacity for CPUs [%*pbl] (to_visit=%*pbl)\n",
-				cpumask_pr_args(policy->related_cpus),
-				cpumask_pr_args(cpus_to_visit));
-		cpumask_andnot(cpus_to_visit, cpus_to_visit,
-			       policy->related_cpus);
-		for_each_cpu(cpu, policy->related_cpus) {
-			raw_capacity[cpu] = topology_get_cpu_scale(NULL, cpu) *
-					    policy->cpuinfo.max_freq / 1000UL;
-			capacity_scale = max(raw_capacity[cpu], capacity_scale);
-		}
-		if (cpumask_empty(cpus_to_visit)) {
-			topology_normalize_cpu_scale();
-			kfree(raw_capacity);
-			pr_debug("cpu_capacity: parsing done\n");
-			cap_parsing_done = true;
-			schedule_work(&parsing_done_work);
-		}
+	if (val != CPUFREQ_NOTIFY)
+		return 0;
+
+	pr_debug("cpu_capacity: init cpu capacity for CPUs [%*pbl] (to_visit=%*pbl)\n",
+		 cpumask_pr_args(policy->related_cpus),
+		 cpumask_pr_args(cpus_to_visit));
+
+	cpumask_andnot(cpus_to_visit, cpus_to_visit, policy->related_cpus);
+
+	for_each_cpu(cpu, policy->related_cpus) {
+		raw_capacity[cpu] = topology_get_cpu_scale(NULL, cpu) *
+				    policy->cpuinfo.max_freq / 1000UL;
+		capacity_scale = max(raw_capacity[cpu], capacity_scale);
 	}
+
+	if (cpumask_empty(cpus_to_visit)) {
+		topology_normalize_cpu_scale();
+		kfree(raw_capacity);
+		pr_debug("cpu_capacity: parsing done\n");
+		cap_parsing_done = true;
+		schedule_work(&parsing_done_work);
+	}
+
 	return 0;
 }
Signature · 1 line
2.13.0.71.gd7076ec9c9cb
author
#3 lore raw

PATCH v3 3/5 arch_topology: Change return type of topology_parse_cpu_capacity() to bool

In reply to Viresh Kumar

topology_parse_cpu_capacity() returns 1 on success and 0 on errors. Make it return bool instead of int as that suits the purpose better.

Signed-off-by Viresh Kumar <[email protected]>
---
 drivers/base/arch_topology.c  | 8 ++++----
 include/linux/arch_topology.h | 4 +++-
 2 files changed, 7 insertions(+), 5 deletions(-)
drivers/base/arch_topology.c +4 -4
index a3cd7c869c3e..5728e2fbb765 100644
@@ -119,13 +119,13 @@ void topology_normalize_cpu_scale(void)
 	mutex_unlock(&cpu_scale_mutex);
 }
 
-int __init topology_parse_cpu_capacity(struct device_node *cpu_node, int cpu)
+bool __init topology_parse_cpu_capacity(struct device_node *cpu_node, int cpu)
 {
-	int ret = 1;
+	int ret;
 	u32 cpu_capacity;
 
 	if (cap_parsing_failed)
-		return !ret;
+		return false;
 
 	ret = of_property_read_u32(cpu_node, "capacity-dmips-mhz",
 				   &cpu_capacity);

@@ -137,7 +137,7 @@ int __init topology_parse_cpu_capacity(struct device_node *cpu_node, int cpu)
 			if (!raw_capacity) {
 				pr_err("cpu_capacity: failed to allocate memory for raw capacities\n");
 				cap_parsing_failed = true;
-				return 0;
+				return false;
 			}
 		}
 		capacity_scale = max(cpu_capacity, capacity_scale);
include/linux/arch_topology.h +3 -1
index 9af3c174c03a..716ce587247e 100644
@@ -4,10 +4,12 @@
 #ifndef _LINUX_ARCH_TOPOLOGY_H_
 #define _LINUX_ARCH_TOPOLOGY_H_
 
+#include <linux/types.h>
+
 void topology_normalize_cpu_scale(void);
 
 struct device_node;
-int topology_parse_cpu_capacity(struct device_node *cpu_node, int cpu);
+bool topology_parse_cpu_capacity(struct device_node *cpu_node, int cpu);
 
 struct sched_domain;
 unsigned long topology_get_cpu_scale(struct sched_domain *sd, int cpu);
Signature · 1 line
2.13.0.71.gd7076ec9c9cb
author
#4 lore raw

PATCH v3 4/5 arch_topology: Localize cap_parsing_failed to topology_parse_cpu_capacity()

In reply to Viresh Kumar

cap_parsing_failed is only required in topology_parse_cpu_capacity() to know if we have already tried to allocate raw_capacity and failed, or if at least one of the cpu_node didn't had the required "capacity-dmips-mhz" property.

All other users can use raw_capacity instead of cap_parsing_failed.

Make sure we set raw_capacity to NULL after we free it.

Signed-off-by Viresh Kumar <[email protected]>
---
 drivers/base/arch_topology.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)
drivers/base/arch_topology.c +13 -11
index 5728e2fbb765..9e4d2107f4fa 100644
@@ -95,14 +95,21 @@ subsys_initcall(register_cpu_capacity_sysctl);
 
 static u32 capacity_scale;
 static u32 *raw_capacity;
-static bool cap_parsing_failed;
+
+static int __init free_raw_capacity(void)
+{
+	kfree(raw_capacity);
+	raw_capacity = NULL;
+
+	return 0;
+}
 
 void topology_normalize_cpu_scale(void)
 {
 	u64 capacity;
 	int cpu;
 
-	if (!raw_capacity || cap_parsing_failed)
+	if (!raw_capacity)
 		return;
 
 	pr_debug("cpu_capacity: capacity_scale=%u\n", capacity_scale);

@@ -121,6 +128,7 @@ void topology_normalize_cpu_scale(void)
 
 bool __init topology_parse_cpu_capacity(struct device_node *cpu_node, int cpu)
 {
+	static bool cap_parsing_failed;
 	int ret;
 	u32 cpu_capacity;
 

@@ -151,7 +159,7 @@ bool __init topology_parse_cpu_capacity(struct device_node *cpu_node, int cpu)
 			pr_err("cpu_capacity: partial information: fallback to 1024 for all CPUs\n");
 		}
 		cap_parsing_failed = true;
-		kfree(raw_capacity);
+		free_raw_capacity();
 	}
 
 	return !ret;

@@ -171,7 +179,7 @@ init_cpu_capacity_callback(struct notifier_block *nb,
 	struct cpufreq_policy *policy = data;
 	int cpu;
 
-	if (cap_parsing_failed || cap_parsing_done)
+	if (!raw_capacity || cap_parsing_done)
 		return 0;
 
 	if (val != CPUFREQ_NOTIFY)

@@ -191,7 +199,7 @@ init_cpu_capacity_callback(struct notifier_block *nb,
 
 	if (cpumask_empty(cpus_to_visit)) {
 		topology_normalize_cpu_scale();
-		kfree(raw_capacity);
+		free_raw_capacity();
 		pr_debug("cpu_capacity: parsing done\n");
 		cap_parsing_done = true;
 		schedule_work(&parsing_done_work);

@@ -233,11 +241,5 @@ static void parsing_done_workfn(struct work_struct *work)
 }
 
 #else
-static int __init free_raw_capacity(void)
-{
-	kfree(raw_capacity);
-
-	return 0;
-}
 core_initcall(free_raw_capacity);
 #endif
Signature · 1 line
2.13.0.71.gd7076ec9c9cb
author
#5 lore raw

PATCH v3 5/5 arch_topology: Get rid of cap_parsing_done

In reply to Viresh Kumar

There is no need to check for cap_parsing_done flag anymore as !raw_capacity flag alone is enough for us. Remove the (now) useless flag cap_parsing_done.

Signed-off-by Viresh Kumar <[email protected]>
---
 drivers/base/arch_topology.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
drivers/base/arch_topology.c +1 -3
index 9e4d2107f4fa..74043ead9da1 100644
@@ -167,7 +167,6 @@ bool __init topology_parse_cpu_capacity(struct device_node *cpu_node, int cpu)
 
 #ifdef CONFIG_CPU_FREQ
 static cpumask_var_t cpus_to_visit;
-static bool cap_parsing_done;
 static void parsing_done_workfn(struct work_struct *work);
 static DECLARE_WORK(parsing_done_work, parsing_done_workfn);
 

@@ -179,7 +178,7 @@ init_cpu_capacity_callback(struct notifier_block *nb,
 	struct cpufreq_policy *policy = data;
 	int cpu;
 
-	if (!raw_capacity || cap_parsing_done)
+	if (!raw_capacity)
 		return 0;
 
 	if (val != CPUFREQ_NOTIFY)

@@ -201,7 +200,6 @@ init_cpu_capacity_callback(struct notifier_block *nb,
 		topology_normalize_cpu_scale();
 		free_raw_capacity();
 		pr_debug("cpu_capacity: parsing done\n");
-		cap_parsing_done = true;
 		schedule_work(&parsing_done_work);
 	}
Signature · 1 line
2.13.0.71.gd7076ec9c9cb
#6 lore raw
In reply to Viresh Kumar

Hi Viresh,

Quoted 10 lines · Viresh Kumar

Hi Greg,

You weren't included in the first [1] version of this series, as it was targeting arch/arm*/ directories then.

Here are some cleanups for the arch_topology core.

Tested on ARM64 Hikey board by setting following in cpu nodes in DT:

        capacity-dmips-mhz = <1000>;

The set looks ok to me. Also tested on JunoR2 and TC2.

You can add my

Reviewed-and-tested-by Juri Lelli <[email protected]>

Best,

- Juri