On Thu, Feb 09, 2017 at 09:25:22AM +0000, Juri Lelli wrote:

arm and arm64 share lot of code relative to parsing CPU capacity
information from DT, using that information for appropriate scaling and
exposing a sysfs interface for chaging such values at runtime.

Factorize such code in a common place (driver/base/arch_topology.c) in
preparation for further additions.

Suggested-by: Will Deacon <redacted>
Suggested-by: Mark Rutland <mark.rutland@arm.com>
Suggested-by: Catalin Marinas <catalin.marinas@arm.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <redacted>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Juri Lelli <redacted>
---

Changes from v1:
 - keep the original GPLv2 header
---
 arch/arm/Kconfig             |   1 +
 arch/arm/kernel/topology.c   | 213 ++------------------------------------
 arch/arm64/Kconfig           |   1 +
 arch/arm64/kernel/topology.c | 219 +--------------------------------------
 drivers/base/Kconfig         |   8 ++
 drivers/base/Makefile        |   1 +
 drivers/base/arch_topology.c | 237 +++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 257 insertions(+), 423 deletions(-)
 create mode 100644 drivers/base/arch_topology.c

Ah, so you want _me_ to maintain this, ok, I better review it...

Mix of spaces and tabs :(

No need to keep the filename in the file, you know what it is called :)

+ *
+ * Copyright (C) 2016, ARM Ltd.
+ * Written by: Juri Lelli, ARM Ltd.
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License.  See the file "COPYING" in the main directory of this archive
+ * for more details.

So, v2 only?  Please be specific.  Even better yet, use a SPDX header if
you want to, those are always nice.

+ */
+
+#include <linux/acpi.h>
+#include <linux/cpu.h>
+#include <linux/cpufreq.h>
+#include <linux/device.h>
+#include <linux/of.h>
+#include <linux/slab.h>
+#include <linux/string.h>
+#include <linux/topology.h>
+
+static DEFINE_MUTEX(cpu_scale_mutex);
+static DEFINE_PER_CPU(unsigned long, cpu_scale) = SCHED_CAPACITY_SCALE;
+
+unsigned long arch_scale_cpu_capacity(struct sched_domain *sd, int cpu)

Why do you have sd here?  You never use it:

+{
+	return per_cpu(cpu_scale, cpu);

See?  What am I missing?

+}
+
+void set_capacity_scale(unsigned int cpu, unsigned long capacity)
+{
+	per_cpu(cpu_scale, cpu) = capacity;
+}
+
+static ssize_t cpu_capacity_show(struct device *dev,
+				 struct device_attribute *attr,
+				 char *buf)
+{
+	struct cpu *cpu = container_of(dev, struct cpu, dev);
+
+	return sprintf(buf, "%lu\n",
+			arch_scale_cpu_capacity(NULL, cpu->dev.id));
+}
+
+static ssize_t cpu_capacity_store(struct device *dev,
+				  struct device_attribute *attr,
+				  const char *buf,
+				  size_t count)
+{
+	struct cpu *cpu = container_of(dev, struct cpu, dev);
+	int this_cpu = cpu->dev.id, i;

new line for:
	int i;
please.

+	unsigned long new_capacity;
+	ssize_t ret;
+
+	if (count) {

	if (!count)
		return 0;

then you can get on with the rest of the logic.  Don't indent if you
don't have to.

+		ret = kstrtoul(buf, 0, &new_capacity);
+		if (ret)
+			return ret;
+		if (new_capacity > SCHED_CAPACITY_SCALE)
+			return -EINVAL;
+
+		mutex_lock(&cpu_scale_mutex);
+		for_each_cpu(i, &cpu_topology[this_cpu].core_sibling)
+			set_capacity_scale(i, new_capacity);
+		mutex_unlock(&cpu_scale_mutex);
+	}
+
+	return count;
+}

No documentation for these sysfs file?  Not good :(

+
+static DEVICE_ATTR_RW(cpu_capacity);
+
+static int register_cpu_capacity_sysctl(void)
+{
+	int i;
+	struct device *cpu;
+
+	for_each_possible_cpu(i) {
+		cpu = get_cpu_device(i);
+		if (!cpu) {
+			pr_err("%s: too early to get CPU%d device!\n",
+			       __func__, i);

What is this going to help with?

+			continue;
+		}
+		device_create_file(cpu, &dev_attr_cpu_capacity);

You realize you just raced userspace, right?  Why do it this way and not
register the files when the CPU device is created/removed?

+	}
+
+	return 0;
+}
+subsys_initcall(register_cpu_capacity_sysctl);
+
+u32 capacity_scale;
+u32 *raw_capacity;
+bool cap_parsing_failed;

globals?  really?  That's bold :(

+
+void normalize_cpu_capacity(void)

naming is hard, but try to put a good, descriptive, prefix on everything
you are exporting in the same file, the same prefix.

cpu_capacity_normalize()?
cpu_capacity_register_sysctl()?

and so on.

+{
+	u64 capacity;
+	int cpu;
+
+	if (!raw_capacity || cap_parsing_failed)
+		return;
+
+	pr_debug("cpu_capacity: capacity_scale=%u\n", capacity_scale);
+	mutex_lock(&cpu_scale_mutex);
+	for_each_possible_cpu(cpu) {
+		pr_debug("cpu_capacity: cpu=%d raw_capacity=%u\n",
+			 cpu, raw_capacity[cpu]);
+		capacity = (raw_capacity[cpu] << SCHED_CAPACITY_SHIFT)
+			/ capacity_scale;
+		set_capacity_scale(cpu, capacity);
+		pr_debug("cpu_capacity: CPU%d cpu_capacity=%lu\n",
+			cpu, arch_scale_cpu_capacity(NULL, cpu));
+	}
+	mutex_unlock(&cpu_scale_mutex);
+}
+
+int __init parse_cpu_capacity(struct device_node *cpu_node, int cpu)

cpu_capacity_parse()?

thanks,

greg k-h