[PATCH v5 0/4] arm64: Dereference CPU operations indirectly

[PATCH v5 0/4] arm64: Dereference CPU operations indirectly

[Gavin Shan]

The subject perhaps can't precisely indicate what this series does, but
keep it same as before for consistency.

In current implementation, an array (@cpu_ops[NR_CPUS]) is maintained
to dereference the CPU operations. 2KB memory are consumed when NR_CPUS
is configured to 256. It's too much than what I expected. This series
reworks the implementation to dereference the CPU operations by using
two CPU operations pointer with assumption - all secondary CPUs should
have unified CPU operations. With this, 16-bytes memory will be used
for same purpose.

PATCH[1/4] isn't too much relevant, to declare ACPI parking protocol only
when CONFIG_ARM64_ACPI_PARKING_PROTOCOL has been enabled. PATCH[2/4]
renames cpu_read_ops() to init_cpu_ops(), which is obviously more precise
because it's initializing the CPU operations. PATCH[3/4] introduces
get_cpu_ops(), preparing for droping the array of CPU operations.
PATCH[4/4] removes the CPU operations deferencing array and replaces
it with two pointers with the assumption: all secondary CPUs should have
same enablement method.

Changelog
=========
v5:
   * Rebase to 5.6.rc6 and retest                        (Gavin Shan)
   * Improved commit log for PATCH[1]                    (Gavin Shan)
   * Add helper function __cpu_try_die()                 (Mark Rutland)
   * Two pointers to track the operations for boot CPU
     and the secondary CPUs separately                   (Mark Rutland)
   * Drop PATCH[5] because @cpu parameter is still
     needed by get_cpu_ops()                             (Gavin Shan)
v4:
   * Rebase to 5.6.rc3 and retest                        (Gavin Shan)
   * Improved commit log for PATCH[4/5] with link tag    (Lorenzo Pieralisi)
   * Using pointer instead of index to dereference the
     unified CPU operations                              (Lorenzo Pieralisi)
   * Merge logic of cpu_get_ops() to get_cpu_method()    (Gavin Shan)
v3:
   * Assume all CPUs have same enablement method. With this, the used
     memory is further squeezed from 64 bytes to 4 bytes (Lorenzo Pieralisi)
   * Add PATCH[5/5] to remove argument of get_cpu_ops()  (Gavin Shan)
v2:
   * Pack 4 CPUs' indexes into one byte. 64 bytes are consumed in order
     to get the CPU operations                            (Robin Murphy)
   * Use ARRAY_SIZE() to iterate @cpu_ops[]               (Robin Murphy)
   * Make index-0 valid                                   (Robin Murphy)

Gavin Shan (4):
  arm64: Declare ACPI parking protocol CPU operation if needed
  arm64: Rename cpu_read_ops() to init_cpu_ops()
  arm64: Introduce get_cpu_ops() helper function
  arm64: Remove CPU operations dereferencing array

 arch/arm64/include/asm/cpu_ops.h |  8 +--
 arch/arm64/kernel/cpu_ops.c      | 84 +++++++++++++++++---------------
 arch/arm64/kernel/cpuidle.c      |  9 ++--
 arch/arm64/kernel/setup.c        |  8 +--
 arch/arm64/kernel/smp.c          | 72 +++++++++++++++++----------
 5 files changed, 107 insertions(+), 74 deletions(-)

-- 
2.23.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v5 1/4] arm64: Declare ACPI parking protocol CPU operation if needed

[Gavin Shan]

It's obvious we needn't declare the corresponding CPU operation when
CONFIG_ARM64_ACPI_PARKING_PROTOCOL is disabled, even it doesn't cause
any compiling warnings.

Signed-off-by: Gavin Shan <gshan@redhat.com>
Acked-by: Mark Rutland <mark.rutland@arm.com>
---
 arch/arm64/kernel/cpu_ops.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/kernel/cpu_ops.c b/arch/arm64/kernel/cpu_ops.c
index 7e07072757af..2082cfb1be86 100644
--- a/arch/arm64/kernel/cpu_ops.c
+++ b/arch/arm64/kernel/cpu_ops.c
@@ -15,7 +15,9 @@
 #include <asm/smp_plat.h>
 
 extern const struct cpu_operations smp_spin_table_ops;
+#ifdef CONFIG_ARM64_ACPI_PARKING_PROTOCOL
 extern const struct cpu_operations acpi_parking_protocol_ops;
+#endif
 extern const struct cpu_operations cpu_psci_ops;
 
 const struct cpu_operations *cpu_ops[NR_CPUS] __ro_after_init;
-- 
2.23.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v5 2/4] arm64: Rename cpu_read_ops() to init_cpu_ops()

[Gavin Shan]

This renames cpu_read_ops() to init_cpu_ops() as the function is only
called in initialization phase. Also, we will introduce get_cpu_ops() in
the subsequent patches, to retireve the CPU operation by the given CPU
index. The usage of cpu_read_ops() and get_cpu_ops() are difficult to be
distinguished from their names.

Signed-off-by: Gavin Shan <gshan@redhat.com>
Acked-by: Mark Rutland <mark.rutland@arm.com>
---
 arch/arm64/include/asm/cpu_ops.h | 6 +++---
 arch/arm64/kernel/cpu_ops.c      | 2 +-
 arch/arm64/kernel/setup.c        | 2 +-
 arch/arm64/kernel/smp.c          | 2 +-
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/include/asm/cpu_ops.h b/arch/arm64/include/asm/cpu_ops.h
index 86aabf1e0199..baa13b5db2ca 100644
--- a/arch/arm64/include/asm/cpu_ops.h
+++ b/arch/arm64/include/asm/cpu_ops.h
@@ -56,11 +56,11 @@ struct cpu_operations {
 };
 
 extern const struct cpu_operations *cpu_ops[NR_CPUS];
-int __init cpu_read_ops(int cpu);
+int __init init_cpu_ops(int cpu);
 
-static inline void __init cpu_read_bootcpu_ops(void)
+static inline void __init init_bootcpu_ops(void)
 {
-	cpu_read_ops(0);
+	init_cpu_ops(0);
 }
 
 #endif /* ifndef __ASM_CPU_OPS_H */
diff --git a/arch/arm64/kernel/cpu_ops.c b/arch/arm64/kernel/cpu_ops.c
index 2082cfb1be86..a6c3c816b618 100644
--- a/arch/arm64/kernel/cpu_ops.c
+++ b/arch/arm64/kernel/cpu_ops.c
@@ -96,7 +96,7 @@ static const char *__init cpu_read_enable_method(int cpu)
 /*
  * Read a cpu's enable method and record it in cpu_ops.
  */
-int __init cpu_read_ops(int cpu)
+int __init init_cpu_ops(int cpu)
 {
 	const char *enable_method = cpu_read_enable_method(cpu);
 
diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
index a34890bf309f..f66bd260cce8 100644
--- a/arch/arm64/kernel/setup.c
+++ b/arch/arm64/kernel/setup.c
@@ -344,7 +344,7 @@ void __init setup_arch(char **cmdline_p)
 	else
 		psci_acpi_init();
 
-	cpu_read_bootcpu_ops();
+	init_bootcpu_ops();
 	smp_init_cpus();
 	smp_build_mpidr_hash();
 
diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index d4ed9a19d8fe..6f8477d7f3be 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -488,7 +488,7 @@ static bool __init is_mpidr_duplicate(unsigned int cpu, u64 hwid)
  */
 static int __init smp_cpu_setup(int cpu)
 {
-	if (cpu_read_ops(cpu))
+	if (init_cpu_ops(cpu))
 		return -ENODEV;
 
 	if (cpu_ops[cpu]->cpu_init(cpu))
-- 
2.23.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v5 3/4] arm64: Introduce get_cpu_ops() helper function

[Gavin Shan]

This introduces get_cpu_ops() to return the CPU operations according to
the given CPU index. For now, it simply returns the @cpu_ops[cpu] as
before. Also, helper function __cpu_try_die() is introduced to be shared
by cpu_die() and ipi_cpu_crash_stop(). So it shouldn't introduce any
functional changes.

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 arch/arm64/include/asm/cpu_ops.h |  2 +-
 arch/arm64/kernel/cpu_ops.c      |  7 +++-
 arch/arm64/kernel/cpuidle.c      |  9 ++--
 arch/arm64/kernel/setup.c        |  6 ++-
 arch/arm64/kernel/smp.c          | 70 +++++++++++++++++++++-----------
 5 files changed, 62 insertions(+), 32 deletions(-)

diff --git a/arch/arm64/include/asm/cpu_ops.h b/arch/arm64/include/asm/cpu_ops.h
index baa13b5db2ca..d28e8f37d3b4 100644
--- a/arch/arm64/include/asm/cpu_ops.h
+++ b/arch/arm64/include/asm/cpu_ops.h
@@ -55,8 +55,8 @@ struct cpu_operations {
 #endif
 };
 
-extern const struct cpu_operations *cpu_ops[NR_CPUS];
 int __init init_cpu_ops(int cpu);
+extern const struct cpu_operations *get_cpu_ops(int cpu);
 
 static inline void __init init_bootcpu_ops(void)
 {
diff --git a/arch/arm64/kernel/cpu_ops.c b/arch/arm64/kernel/cpu_ops.c
index a6c3c816b618..e133011f64b5 100644
--- a/arch/arm64/kernel/cpu_ops.c
+++ b/arch/arm64/kernel/cpu_ops.c
@@ -20,7 +20,7 @@ extern const struct cpu_operations acpi_parking_protocol_ops;
 #endif
 extern const struct cpu_operations cpu_psci_ops;
 
-const struct cpu_operations *cpu_ops[NR_CPUS] __ro_after_init;
+static const struct cpu_operations *cpu_ops[NR_CPUS] __ro_after_init;
 
 static const struct cpu_operations *const dt_supported_cpu_ops[] __initconst = {
 	&smp_spin_table_ops,
@@ -111,3 +111,8 @@ int __init init_cpu_ops(int cpu)
 
 	return 0;
 }
+
+const struct cpu_operations *get_cpu_ops(int cpu)
+{
+	return cpu_ops[cpu];
+}
diff --git a/arch/arm64/kernel/cpuidle.c b/arch/arm64/kernel/cpuidle.c
index e4d6af2fdec7..b512b5503f6e 100644
--- a/arch/arm64/kernel/cpuidle.c
+++ b/arch/arm64/kernel/cpuidle.c
@@ -18,11 +18,11 @@
 
 int arm_cpuidle_init(unsigned int cpu)
 {
+	const struct cpu_operations *ops = get_cpu_ops(cpu);
 	int ret = -EOPNOTSUPP;
 
-	if (cpu_ops[cpu] && cpu_ops[cpu]->cpu_suspend &&
-			cpu_ops[cpu]->cpu_init_idle)
-		ret = cpu_ops[cpu]->cpu_init_idle(cpu);
+	if (ops && ops->cpu_suspend && ops->cpu_init_idle)
+		ret = ops->cpu_init_idle(cpu);
 
 	return ret;
 }
@@ -37,8 +37,9 @@ int arm_cpuidle_init(unsigned int cpu)
 int arm_cpuidle_suspend(int index)
 {
 	int cpu = smp_processor_id();
+	const struct cpu_operations *ops = get_cpu_ops(cpu);
 
-	return cpu_ops[cpu]->cpu_suspend(index);
+	return ops->cpu_suspend(index);
 }
 
 #ifdef CONFIG_ACPI
diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
index f66bd260cce8..3fd2c11c09fc 100644
--- a/arch/arm64/kernel/setup.c
+++ b/arch/arm64/kernel/setup.c
@@ -371,8 +371,10 @@ void __init setup_arch(char **cmdline_p)
 static inline bool cpu_can_disable(unsigned int cpu)
 {
 #ifdef CONFIG_HOTPLUG_CPU
-	if (cpu_ops[cpu] && cpu_ops[cpu]->cpu_can_disable)
-		return cpu_ops[cpu]->cpu_can_disable(cpu);
+	const struct cpu_operations *ops = get_cpu_ops(cpu);
+
+	if (ops && ops->cpu_can_disable)
+		return ops->cpu_can_disable(cpu);
 #endif
 	return false;
 }
diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index 6f8477d7f3be..e5c9862c271b 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -93,8 +93,10 @@ static inline int op_cpu_kill(unsigned int cpu)
  */
 static int boot_secondary(unsigned int cpu, struct task_struct *idle)
 {
-	if (cpu_ops[cpu]->cpu_boot)
-		return cpu_ops[cpu]->cpu_boot(cpu);
+	const struct cpu_operations *ops = get_cpu_ops(cpu);
+
+	if (ops->cpu_boot)
+		return ops->cpu_boot(cpu);
 
 	return -EOPNOTSUPP;
 }
@@ -196,6 +198,7 @@ asmlinkage notrace void secondary_start_kernel(void)
 {
 	u64 mpidr = read_cpuid_mpidr() & MPIDR_HWID_BITMASK;
 	struct mm_struct *mm = &init_mm;
+	const struct cpu_operations *ops;
 	unsigned int cpu;
 
 	cpu = task_cpu(current);
@@ -227,8 +230,9 @@ asmlinkage notrace void secondary_start_kernel(void)
 	 */
 	check_local_cpu_capabilities();
 
-	if (cpu_ops[cpu]->cpu_postboot)
-		cpu_ops[cpu]->cpu_postboot();
+	ops = get_cpu_ops(cpu);
+	if (ops->cpu_postboot)
+		ops->cpu_postboot();
 
 	/*
 	 * Log the CPU info before it is marked online and might get read.
@@ -266,19 +270,21 @@ asmlinkage notrace void secondary_start_kernel(void)
 #ifdef CONFIG_HOTPLUG_CPU
 static int op_cpu_disable(unsigned int cpu)
 {
+	const struct cpu_operations *ops = get_cpu_ops(cpu);
+
 	/*
 	 * If we don't have a cpu_die method, abort before we reach the point
 	 * of no return. CPU0 may not have an cpu_ops, so test for it.
 	 */
-	if (!cpu_ops[cpu] || !cpu_ops[cpu]->cpu_die)
+	if (!ops || !ops->cpu_die)
 		return -EOPNOTSUPP;
 
 	/*
 	 * We may need to abort a hot unplug for some other mechanism-specific
 	 * reason.
 	 */
-	if (cpu_ops[cpu]->cpu_disable)
-		return cpu_ops[cpu]->cpu_disable(cpu);
+	if (ops->cpu_disable)
+		return ops->cpu_disable(cpu);
 
 	return 0;
 }
@@ -314,15 +320,17 @@ int __cpu_disable(void)
 
 static int op_cpu_kill(unsigned int cpu)
 {
+	const struct cpu_operations *ops = get_cpu_ops(cpu);
+
 	/*
 	 * If we have no means of synchronising with the dying CPU, then assume
 	 * that it is really dead. We can only wait for an arbitrary length of
 	 * time and hope that it's dead, so let's skip the wait and just hope.
 	 */
-	if (!cpu_ops[cpu]->cpu_kill)
+	if (!ops->cpu_kill)
 		return 0;
 
-	return cpu_ops[cpu]->cpu_kill(cpu);
+	return ops->cpu_kill(cpu);
 }
 
 /*
@@ -357,6 +365,7 @@ void __cpu_die(unsigned int cpu)
 void cpu_die(void)
 {
 	unsigned int cpu = smp_processor_id();
+	const struct cpu_operations *ops = get_cpu_ops(cpu);
 
 	idle_task_exit();
 
@@ -370,12 +379,22 @@ void cpu_die(void)
 	 * mechanism must perform all required cache maintenance to ensure that
 	 * no dirty lines are lost in the process of shutting down the CPU.
 	 */
-	cpu_ops[cpu]->cpu_die(cpu);
+	ops->cpu_die(cpu);
 
 	BUG();
 }
 #endif
 
+static void __cpu_try_die(int cpu)
+{
+#ifdef CONFIG_HOTPLUG_CPU
+	const struct cpu_operations *ops = get_cpu_ops(cpu);
+
+	if (ops && ops->cpu_die)
+		ops->cpu_die(cpu);
+#endif
+}
+
 /*
  * Kill the calling secondary CPU, early in bringup before it is turned
  * online.
@@ -389,12 +408,11 @@ void cpu_die_early(void)
 	/* Mark this CPU absent */
 	set_cpu_present(cpu, 0);
 
-#ifdef CONFIG_HOTPLUG_CPU
-	update_cpu_boot_status(CPU_KILL_ME);
-	/* Check if we can park ourselves */
-	if (cpu_ops[cpu] && cpu_ops[cpu]->cpu_die)
-		cpu_ops[cpu]->cpu_die(cpu);
-#endif
+	if (IS_ENABLED(CONFIG_HOTPLUG_CPU)) {
+		update_cpu_boot_status(CPU_KILL_ME);
+		__cpu_try_die(cpu);
+	}
+
 	update_cpu_boot_status(CPU_STUCK_IN_KERNEL);
 
 	cpu_park_loop();
@@ -488,10 +506,13 @@ static bool __init is_mpidr_duplicate(unsigned int cpu, u64 hwid)
  */
 static int __init smp_cpu_setup(int cpu)
 {
+	const struct cpu_operations *ops;
+
 	if (init_cpu_ops(cpu))
 		return -ENODEV;
 
-	if (cpu_ops[cpu]->cpu_init(cpu))
+	ops = get_cpu_ops(cpu);
+	if (ops->cpu_init(cpu))
 		return -ENODEV;
 
 	set_cpu_possible(cpu, true);
@@ -714,6 +735,7 @@ void __init smp_init_cpus(void)
 
 void __init smp_prepare_cpus(unsigned int max_cpus)
 {
+	const struct cpu_operations *ops;
 	int err;
 	unsigned int cpu;
 	unsigned int this_cpu;
@@ -744,10 +766,11 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
 		if (cpu == smp_processor_id())
 			continue;
 
-		if (!cpu_ops[cpu])
+		ops = get_cpu_ops(cpu);
+		if (!ops)
 			continue;
 
-		err = cpu_ops[cpu]->cpu_prepare(cpu);
+		err = ops->cpu_prepare(cpu);
 		if (err)
 			continue;
 
@@ -863,10 +886,8 @@ static void ipi_cpu_crash_stop(unsigned int cpu, struct pt_regs *regs)
 	local_irq_disable();
 	sdei_mask_local_cpu();
 
-#ifdef CONFIG_HOTPLUG_CPU
-	if (cpu_ops[cpu]->cpu_die)
-		cpu_ops[cpu]->cpu_die(cpu);
-#endif
+	if (IS_ENABLED(CONFIG_HOTPLUG_CPU))
+		__cpu_try_die(cpu);
 
 	/* just in case */
 	cpu_park_loop();
@@ -1044,8 +1065,9 @@ static bool have_cpu_die(void)
 {
 #ifdef CONFIG_HOTPLUG_CPU
 	int any_cpu = raw_smp_processor_id();
+	const struct cpu_operations *ops = get_cpu_ops(any_cpu);
 
-	if (cpu_ops[any_cpu] && cpu_ops[any_cpu]->cpu_die)
+	if (ops && ops->cpu_die)
 		return true;
 #endif
 	return false;
-- 
2.23.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v5 4/4] arm64: Remove CPU operations dereferencing array

[Gavin Shan]

One CPU operations is maintained through array @cpu_ops[NR_CPUS]. 2KB
memory is consumed when CONFIG_NR_CPUS is set to 256. It seems too
much memory has been used for this. Also, all secondary CPUs must use
same CPU operations and we shouldn't bring up the broken CPU as Lorenzo
Pieralisi and Mark Rutland pointed out.

This introduces two variables (@{boot,secondary}_cpu_ops) to store the
CPU operations for boot CPU and secondary CPUs separately, which are
figured out from device tree or ACPI table. The secondary CPUs which
have inconsistent operations won't be brought up. With this, the CPU
operations dereferencing array is removed and 2KB memory is saved. Note
the logic of cpu_get_ops() is merged to get_cpu_method() since the logic
is simple enough and no need to have a separate function for it.

Link: https://lore.kernel.org/linux-arm-kernel/20200211114553.GA21093@e121166-lin.cambridge.arm.com
Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 arch/arm64/kernel/cpu_ops.c | 77 +++++++++++++++++++------------------
 1 file changed, 39 insertions(+), 38 deletions(-)

diff --git a/arch/arm64/kernel/cpu_ops.c b/arch/arm64/kernel/cpu_ops.c
index e133011f64b5..a0f647d22e36 100644
--- a/arch/arm64/kernel/cpu_ops.c
+++ b/arch/arm64/kernel/cpu_ops.c
@@ -20,41 +20,20 @@ extern const struct cpu_operations acpi_parking_protocol_ops;
 #endif
 extern const struct cpu_operations cpu_psci_ops;
 
-static const struct cpu_operations *cpu_ops[NR_CPUS] __ro_after_init;
-
-static const struct cpu_operations *const dt_supported_cpu_ops[] __initconst = {
+static const struct cpu_operations *const available_cpu_ops[] __initconst = {
 	&smp_spin_table_ops,
-	&cpu_psci_ops,
-	NULL,
-};
-
-static const struct cpu_operations *const acpi_supported_cpu_ops[] __initconst = {
 #ifdef CONFIG_ARM64_ACPI_PARKING_PROTOCOL
 	&acpi_parking_protocol_ops,
 #endif
 	&cpu_psci_ops,
-	NULL,
 };
+static const struct cpu_operations *boot_cpu_ops __ro_after_init;
+static const struct cpu_operations *secondary_cpu_ops __ro_after_init;
 
-static const struct cpu_operations * __init cpu_get_ops(const char *name)
-{
-	const struct cpu_operations *const *ops;
-
-	ops = acpi_disabled ? dt_supported_cpu_ops : acpi_supported_cpu_ops;
-
-	while (*ops) {
-		if (!strcmp(name, (*ops)->name))
-			return *ops;
-
-		ops++;
-	}
-
-	return NULL;
-}
-
-static const char *__init cpu_read_enable_method(int cpu)
+static const struct cpu_operations * __init get_cpu_method(int cpu)
 {
 	const char *enable_method;
+	int i;
 
 	if (acpi_disabled) {
 		struct device_node *dn = of_get_cpu_node(cpu, NULL);
@@ -91,22 +70,44 @@ static const char *__init cpu_read_enable_method(int cpu)
 		}
 	}
 
-	return enable_method;
+	if (!enable_method) {
+		pr_warn("No enable-method found on CPU %d\n", cpu);
+		return NULL;
+	}
+
+	/* Search in the array with method */
+	for (i = 0; i < ARRAY_SIZE(available_cpu_ops); i++) {
+		if (!strcmp(available_cpu_ops[i]->name, enable_method))
+			return available_cpu_ops[i];
+	}
+
+	return NULL;
 }
-/*
- * Read a cpu's enable method and record it in cpu_ops.
- */
+
 int __init init_cpu_ops(int cpu)
 {
-	const char *enable_method = cpu_read_enable_method(cpu);
+	const struct cpu_operations *ops = get_cpu_method(cpu);
 
-	if (!enable_method)
-		return -ENODEV;
-
-	cpu_ops[cpu] = cpu_get_ops(enable_method);
-	if (!cpu_ops[cpu]) {
-		pr_warn("Unsupported enable-method: %s\n", enable_method);
+	if (!ops)
 		return -EOPNOTSUPP;
+
+	/* Update boot CPU operations */
+	if (!cpu) {
+		boot_cpu_ops = ops;
+		return 0;
+	}
+
+	/* Update secondary CPU operations if it's not initialized yet */
+	if (!secondary_cpu_ops) {
+		secondary_cpu_ops = ops;
+		return 0;
+	}
+
+	/* We should have unified secondary CPU operations */
+	if (ops != secondary_cpu_ops) {
+		pr_warn("Invalid CPU operations %s (%s) on secondary CPU %d\n",
+			ops->name, secondary_cpu_ops->name, cpu);
+		return -EINVAL;
 	}
 
 	return 0;
@@ -114,5 +115,5 @@ int __init init_cpu_ops(int cpu)
 
 const struct cpu_operations *get_cpu_ops(int cpu)
 {
-	return cpu_ops[cpu];
+	return cpu ? secondary_cpu_ops : boot_cpu_ops;
 }
-- 
2.23.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v5 3/4] arm64: Introduce get_cpu_ops() helper function

[Mark Rutland]

On Thu, Mar 19, 2020 at 10:01:44AM +1100, Gavin Shan wrote:
> This introduces get_cpu_ops() to return the CPU operations according to
> the given CPU index. For now, it simply returns the @cpu_ops[cpu] as
> before. Also, helper function __cpu_try_die() is introduced to be shared
> by cpu_die() and ipi_cpu_crash_stop(). So it shouldn't introduce any
> functional changes.
> 
> Signed-off-by: Gavin Shan <gshan@redhat.com>

I think this makes the code easier to read, and the addition of a few
lines is worthwhile, so:

Acked-by: Mark Rutland <mark.rutland@arm.com>

Mark.

> ---
>  arch/arm64/include/asm/cpu_ops.h |  2 +-
>  arch/arm64/kernel/cpu_ops.c      |  7 +++-
>  arch/arm64/kernel/cpuidle.c      |  9 ++--
>  arch/arm64/kernel/setup.c        |  6 ++-
>  arch/arm64/kernel/smp.c          | 70 +++++++++++++++++++++-----------
>  5 files changed, 62 insertions(+), 32 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/cpu_ops.h b/arch/arm64/include/asm/cpu_ops.h
> index baa13b5db2ca..d28e8f37d3b4 100644
> --- a/arch/arm64/include/asm/cpu_ops.h
> +++ b/arch/arm64/include/asm/cpu_ops.h
> @@ -55,8 +55,8 @@ struct cpu_operations {
>  #endif
>  };
>  
> -extern const struct cpu_operations *cpu_ops[NR_CPUS];
>  int __init init_cpu_ops(int cpu);
> +extern const struct cpu_operations *get_cpu_ops(int cpu);
>  
>  static inline void __init init_bootcpu_ops(void)
>  {
> diff --git a/arch/arm64/kernel/cpu_ops.c b/arch/arm64/kernel/cpu_ops.c
> index a6c3c816b618..e133011f64b5 100644
> --- a/arch/arm64/kernel/cpu_ops.c
> +++ b/arch/arm64/kernel/cpu_ops.c
> @@ -20,7 +20,7 @@ extern const struct cpu_operations acpi_parking_protocol_ops;
>  #endif
>  extern const struct cpu_operations cpu_psci_ops;
>  
> -const struct cpu_operations *cpu_ops[NR_CPUS] __ro_after_init;
> +static const struct cpu_operations *cpu_ops[NR_CPUS] __ro_after_init;
>  
>  static const struct cpu_operations *const dt_supported_cpu_ops[] __initconst = {
>  	&smp_spin_table_ops,
> @@ -111,3 +111,8 @@ int __init init_cpu_ops(int cpu)
>  
>  	return 0;
>  }
> +
> +const struct cpu_operations *get_cpu_ops(int cpu)
> +{
> +	return cpu_ops[cpu];
> +}
> diff --git a/arch/arm64/kernel/cpuidle.c b/arch/arm64/kernel/cpuidle.c
> index e4d6af2fdec7..b512b5503f6e 100644
> --- a/arch/arm64/kernel/cpuidle.c
> +++ b/arch/arm64/kernel/cpuidle.c
> @@ -18,11 +18,11 @@
>  
>  int arm_cpuidle_init(unsigned int cpu)
>  {
> +	const struct cpu_operations *ops = get_cpu_ops(cpu);
>  	int ret = -EOPNOTSUPP;
>  
> -	if (cpu_ops[cpu] && cpu_ops[cpu]->cpu_suspend &&
> -			cpu_ops[cpu]->cpu_init_idle)
> -		ret = cpu_ops[cpu]->cpu_init_idle(cpu);
> +	if (ops && ops->cpu_suspend && ops->cpu_init_idle)
> +		ret = ops->cpu_init_idle(cpu);
>  
>  	return ret;
>  }
> @@ -37,8 +37,9 @@ int arm_cpuidle_init(unsigned int cpu)
>  int arm_cpuidle_suspend(int index)
>  {
>  	int cpu = smp_processor_id();
> +	const struct cpu_operations *ops = get_cpu_ops(cpu);
>  
> -	return cpu_ops[cpu]->cpu_suspend(index);
> +	return ops->cpu_suspend(index);
>  }
>  
>  #ifdef CONFIG_ACPI
> diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
> index f66bd260cce8..3fd2c11c09fc 100644
> --- a/arch/arm64/kernel/setup.c
> +++ b/arch/arm64/kernel/setup.c
> @@ -371,8 +371,10 @@ void __init setup_arch(char **cmdline_p)
>  static inline bool cpu_can_disable(unsigned int cpu)
>  {
>  #ifdef CONFIG_HOTPLUG_CPU
> -	if (cpu_ops[cpu] && cpu_ops[cpu]->cpu_can_disable)
> -		return cpu_ops[cpu]->cpu_can_disable(cpu);
> +	const struct cpu_operations *ops = get_cpu_ops(cpu);
> +
> +	if (ops && ops->cpu_can_disable)
> +		return ops->cpu_can_disable(cpu);
>  #endif
>  	return false;
>  }
> diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
> index 6f8477d7f3be..e5c9862c271b 100644
> --- a/arch/arm64/kernel/smp.c
> +++ b/arch/arm64/kernel/smp.c
> @@ -93,8 +93,10 @@ static inline int op_cpu_kill(unsigned int cpu)
>   */
>  static int boot_secondary(unsigned int cpu, struct task_struct *idle)
>  {
> -	if (cpu_ops[cpu]->cpu_boot)
> -		return cpu_ops[cpu]->cpu_boot(cpu);
> +	const struct cpu_operations *ops = get_cpu_ops(cpu);
> +
> +	if (ops->cpu_boot)
> +		return ops->cpu_boot(cpu);
>  
>  	return -EOPNOTSUPP;
>  }
> @@ -196,6 +198,7 @@ asmlinkage notrace void secondary_start_kernel(void)
>  {
>  	u64 mpidr = read_cpuid_mpidr() & MPIDR_HWID_BITMASK;
>  	struct mm_struct *mm = &init_mm;
> +	const struct cpu_operations *ops;
>  	unsigned int cpu;
>  
>  	cpu = task_cpu(current);
> @@ -227,8 +230,9 @@ asmlinkage notrace void secondary_start_kernel(void)
>  	 */
>  	check_local_cpu_capabilities();
>  
> -	if (cpu_ops[cpu]->cpu_postboot)
> -		cpu_ops[cpu]->cpu_postboot();
> +	ops = get_cpu_ops(cpu);
> +	if (ops->cpu_postboot)
> +		ops->cpu_postboot();
>  
>  	/*
>  	 * Log the CPU info before it is marked online and might get read.
> @@ -266,19 +270,21 @@ asmlinkage notrace void secondary_start_kernel(void)
>  #ifdef CONFIG_HOTPLUG_CPU
>  static int op_cpu_disable(unsigned int cpu)
>  {
> +	const struct cpu_operations *ops = get_cpu_ops(cpu);
> +
>  	/*
>  	 * If we don't have a cpu_die method, abort before we reach the point
>  	 * of no return. CPU0 may not have an cpu_ops, so test for it.
>  	 */
> -	if (!cpu_ops[cpu] || !cpu_ops[cpu]->cpu_die)
> +	if (!ops || !ops->cpu_die)
>  		return -EOPNOTSUPP;
>  
>  	/*
>  	 * We may need to abort a hot unplug for some other mechanism-specific
>  	 * reason.
>  	 */
> -	if (cpu_ops[cpu]->cpu_disable)
> -		return cpu_ops[cpu]->cpu_disable(cpu);
> +	if (ops->cpu_disable)
> +		return ops->cpu_disable(cpu);
>  
>  	return 0;
>  }
> @@ -314,15 +320,17 @@ int __cpu_disable(void)
>  
>  static int op_cpu_kill(unsigned int cpu)
>  {
> +	const struct cpu_operations *ops = get_cpu_ops(cpu);
> +
>  	/*
>  	 * If we have no means of synchronising with the dying CPU, then assume
>  	 * that it is really dead. We can only wait for an arbitrary length of
>  	 * time and hope that it's dead, so let's skip the wait and just hope.
>  	 */
> -	if (!cpu_ops[cpu]->cpu_kill)
> +	if (!ops->cpu_kill)
>  		return 0;
>  
> -	return cpu_ops[cpu]->cpu_kill(cpu);
> +	return ops->cpu_kill(cpu);
>  }
>  
>  /*
> @@ -357,6 +365,7 @@ void __cpu_die(unsigned int cpu)
>  void cpu_die(void)
>  {
>  	unsigned int cpu = smp_processor_id();
> +	const struct cpu_operations *ops = get_cpu_ops(cpu);
>  
>  	idle_task_exit();
>  
> @@ -370,12 +379,22 @@ void cpu_die(void)
>  	 * mechanism must perform all required cache maintenance to ensure that
>  	 * no dirty lines are lost in the process of shutting down the CPU.
>  	 */
> -	cpu_ops[cpu]->cpu_die(cpu);
> +	ops->cpu_die(cpu);
>  
>  	BUG();
>  }
>  #endif
>  
> +static void __cpu_try_die(int cpu)
> +{
> +#ifdef CONFIG_HOTPLUG_CPU
> +	const struct cpu_operations *ops = get_cpu_ops(cpu);
> +
> +	if (ops && ops->cpu_die)
> +		ops->cpu_die(cpu);
> +#endif
> +}
> +
>  /*
>   * Kill the calling secondary CPU, early in bringup before it is turned
>   * online.
> @@ -389,12 +408,11 @@ void cpu_die_early(void)
>  	/* Mark this CPU absent */
>  	set_cpu_present(cpu, 0);
>  
> -#ifdef CONFIG_HOTPLUG_CPU
> -	update_cpu_boot_status(CPU_KILL_ME);
> -	/* Check if we can park ourselves */
> -	if (cpu_ops[cpu] && cpu_ops[cpu]->cpu_die)
> -		cpu_ops[cpu]->cpu_die(cpu);
> -#endif
> +	if (IS_ENABLED(CONFIG_HOTPLUG_CPU)) {
> +		update_cpu_boot_status(CPU_KILL_ME);
> +		__cpu_try_die(cpu);
> +	}
> +
>  	update_cpu_boot_status(CPU_STUCK_IN_KERNEL);
>  
>  	cpu_park_loop();
> @@ -488,10 +506,13 @@ static bool __init is_mpidr_duplicate(unsigned int cpu, u64 hwid)
>   */
>  static int __init smp_cpu_setup(int cpu)
>  {
> +	const struct cpu_operations *ops;
> +
>  	if (init_cpu_ops(cpu))
>  		return -ENODEV;
>  
> -	if (cpu_ops[cpu]->cpu_init(cpu))
> +	ops = get_cpu_ops(cpu);
> +	if (ops->cpu_init(cpu))
>  		return -ENODEV;
>  
>  	set_cpu_possible(cpu, true);
> @@ -714,6 +735,7 @@ void __init smp_init_cpus(void)
>  
>  void __init smp_prepare_cpus(unsigned int max_cpus)
>  {
> +	const struct cpu_operations *ops;
>  	int err;
>  	unsigned int cpu;
>  	unsigned int this_cpu;
> @@ -744,10 +766,11 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
>  		if (cpu == smp_processor_id())
>  			continue;
>  
> -		if (!cpu_ops[cpu])
> +		ops = get_cpu_ops(cpu);
> +		if (!ops)
>  			continue;
>  
> -		err = cpu_ops[cpu]->cpu_prepare(cpu);
> +		err = ops->cpu_prepare(cpu);
>  		if (err)
>  			continue;
>  
> @@ -863,10 +886,8 @@ static void ipi_cpu_crash_stop(unsigned int cpu, struct pt_regs *regs)
>  	local_irq_disable();
>  	sdei_mask_local_cpu();
>  
> -#ifdef CONFIG_HOTPLUG_CPU
> -	if (cpu_ops[cpu]->cpu_die)
> -		cpu_ops[cpu]->cpu_die(cpu);
> -#endif
> +	if (IS_ENABLED(CONFIG_HOTPLUG_CPU))
> +		__cpu_try_die(cpu);
>  
>  	/* just in case */
>  	cpu_park_loop();
> @@ -1044,8 +1065,9 @@ static bool have_cpu_die(void)
>  {
>  #ifdef CONFIG_HOTPLUG_CPU
>  	int any_cpu = raw_smp_processor_id();
> +	const struct cpu_operations *ops = get_cpu_ops(any_cpu);
>  
> -	if (cpu_ops[any_cpu] && cpu_ops[any_cpu]->cpu_die)
> +	if (ops && ops->cpu_die)
>  		return true;
>  #endif
>  	return false;
> -- 
> 2.23.0
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v5 4/4] arm64: Remove CPU operations dereferencing array

[Mark Rutland]

On Thu, Mar 19, 2020 at 10:01:45AM +1100, Gavin Shan wrote:
> One CPU operations is maintained through array @cpu_ops[NR_CPUS]. 2KB
> memory is consumed when CONFIG_NR_CPUS is set to 256. It seems too
> much memory has been used for this. Also, all secondary CPUs must use
> same CPU operations and we shouldn't bring up the broken CPU as Lorenzo
> Pieralisi and Mark Rutland pointed out.
> 
> This introduces two variables (@{boot,secondary}_cpu_ops) to store the
> CPU operations for boot CPU and secondary CPUs separately, which are
> figured out from device tree or ACPI table. The secondary CPUs which
> have inconsistent operations won't be brought up. With this, the CPU
> operations dereferencing array is removed and 2KB memory is saved. Note
> the logic of cpu_get_ops() is merged to get_cpu_method() since the logic
> is simple enough and no need to have a separate function for it.

To be honest, I'm not too keen on this. We've generally tried to bucket
things as either global or per-cpu, and it's odd to go against that.

Is 2K a problem because it forms part of the static Image size? If so,
could we make this a percpu pointer instead, or is there a problem with
that?

Thanks,
Mark.

> 
> Link: https://lore.kernel.org/linux-arm-kernel/20200211114553.GA21093@e121166-lin.cambridge.arm.com
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> ---
>  arch/arm64/kernel/cpu_ops.c | 77 +++++++++++++++++++------------------
>  1 file changed, 39 insertions(+), 38 deletions(-)
> 
> diff --git a/arch/arm64/kernel/cpu_ops.c b/arch/arm64/kernel/cpu_ops.c
> index e133011f64b5..a0f647d22e36 100644
> --- a/arch/arm64/kernel/cpu_ops.c
> +++ b/arch/arm64/kernel/cpu_ops.c
> @@ -20,41 +20,20 @@ extern const struct cpu_operations acpi_parking_protocol_ops;
>  #endif
>  extern const struct cpu_operations cpu_psci_ops;
>  
> -static const struct cpu_operations *cpu_ops[NR_CPUS] __ro_after_init;
> -
> -static const struct cpu_operations *const dt_supported_cpu_ops[] __initconst = {
> +static const struct cpu_operations *const available_cpu_ops[] __initconst = {
>  	&smp_spin_table_ops,
> -	&cpu_psci_ops,
> -	NULL,
> -};
> -
> -static const struct cpu_operations *const acpi_supported_cpu_ops[] __initconst = {
>  #ifdef CONFIG_ARM64_ACPI_PARKING_PROTOCOL
>  	&acpi_parking_protocol_ops,
>  #endif
>  	&cpu_psci_ops,
> -	NULL,
>  };
> +static const struct cpu_operations *boot_cpu_ops __ro_after_init;
> +static const struct cpu_operations *secondary_cpu_ops __ro_after_init;
>  
> -static const struct cpu_operations * __init cpu_get_ops(const char *name)
> -{
> -	const struct cpu_operations *const *ops;
> -
> -	ops = acpi_disabled ? dt_supported_cpu_ops : acpi_supported_cpu_ops;
> -
> -	while (*ops) {
> -		if (!strcmp(name, (*ops)->name))
> -			return *ops;
> -
> -		ops++;
> -	}
> -
> -	return NULL;
> -}
> -
> -static const char *__init cpu_read_enable_method(int cpu)
> +static const struct cpu_operations * __init get_cpu_method(int cpu)
>  {
>  	const char *enable_method;
> +	int i;
>  
>  	if (acpi_disabled) {
>  		struct device_node *dn = of_get_cpu_node(cpu, NULL);
> @@ -91,22 +70,44 @@ static const char *__init cpu_read_enable_method(int cpu)
>  		}
>  	}
>  
> -	return enable_method;
> +	if (!enable_method) {
> +		pr_warn("No enable-method found on CPU %d\n", cpu);
> +		return NULL;
> +	}
> +
> +	/* Search in the array with method */
> +	for (i = 0; i < ARRAY_SIZE(available_cpu_ops); i++) {
> +		if (!strcmp(available_cpu_ops[i]->name, enable_method))
> +			return available_cpu_ops[i];
> +	}
> +
> +	return NULL;
>  }
> -/*
> - * Read a cpu's enable method and record it in cpu_ops.
> - */
> +
>  int __init init_cpu_ops(int cpu)
>  {
> -	const char *enable_method = cpu_read_enable_method(cpu);
> +	const struct cpu_operations *ops = get_cpu_method(cpu);
>  
> -	if (!enable_method)
> -		return -ENODEV;
> -
> -	cpu_ops[cpu] = cpu_get_ops(enable_method);
> -	if (!cpu_ops[cpu]) {
> -		pr_warn("Unsupported enable-method: %s\n", enable_method);
> +	if (!ops)
>  		return -EOPNOTSUPP;
> +
> +	/* Update boot CPU operations */
> +	if (!cpu) {
> +		boot_cpu_ops = ops;
> +		return 0;
> +	}
> +
> +	/* Update secondary CPU operations if it's not initialized yet */
> +	if (!secondary_cpu_ops) {
> +		secondary_cpu_ops = ops;
> +		return 0;
> +	}
> +
> +	/* We should have unified secondary CPU operations */
> +	if (ops != secondary_cpu_ops) {
> +		pr_warn("Invalid CPU operations %s (%s) on secondary CPU %d\n",
> +			ops->name, secondary_cpu_ops->name, cpu);
> +		return -EINVAL;
>  	}
>  
>  	return 0;
> @@ -114,5 +115,5 @@ int __init init_cpu_ops(int cpu)
>  
>  const struct cpu_operations *get_cpu_ops(int cpu)
>  {
> -	return cpu_ops[cpu];
> +	return cpu ? secondary_cpu_ops : boot_cpu_ops;
>  }
> -- 
> 2.23.0
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v5 4/4] arm64: Remove CPU operations dereferencing array

[Gavin Shan]

On 3/20/20 6:38 AM, Mark Rutland wrote:
> On Thu, Mar 19, 2020 at 10:01:45AM +1100, Gavin Shan wrote:
>> One CPU operations is maintained through array @cpu_ops[NR_CPUS]. 2KB
>> memory is consumed when CONFIG_NR_CPUS is set to 256. It seems too
>> much memory has been used for this. Also, all secondary CPUs must use
>> same CPU operations and we shouldn't bring up the broken CPU as Lorenzo
>> Pieralisi and Mark Rutland pointed out.
>>
>> This introduces two variables (@{boot,secondary}_cpu_ops) to store the
>> CPU operations for boot CPU and secondary CPUs separately, which are
>> figured out from device tree or ACPI table. The secondary CPUs which
>> have inconsistent operations won't be brought up. With this, the CPU
>> operations dereferencing array is removed and 2KB memory is saved. Note
>> the logic of cpu_get_ops() is merged to get_cpu_method() since the logic
>> is simple enough and no need to have a separate function for it.
> 
> To be honest, I'm not too keen on this. We've generally tried to bucket
> things as either global or per-cpu, and it's odd to go against that.
> 
> Is 2K a problem because it forms part of the static Image size? If so,
> could we make this a percpu pointer instead, or is there a problem with
> that?
> 

Yes, I agree the usual option is global array or per-cpu. The global array
is increasing the image size, which isn't nice. I don't think the per-cpu
can be used in this case because the per-cpu offset (@__per_cpu_offset[])
isn't initialized yet at that point.

    start_kernel
       setup_arch
          init_bootcpu_ops      # unable to access per-cpu bucket yet
       setup_per_cpu_areas      # per-cpu is initialized

As mentioned early, there are two options if we really want to decrease
the image size (for 2KB): (1) Two CPU operation pointers for boot CPU
and the secondary CPUs separately, which is exactly this patch does.
(2) Use a 2-bits index to the global CPU operation array, 64-bytes are
needed if 256 CPUs are configured.

On the other hand, the cleanup still sounds something to have if we don't
care extra 2KB size introduced to the image:

    * Merge array @dt_supported_cpu_ops[] and @acpi_supported_cpu_ops[]
    * Merge the logic of cpu_get_ops() and cpu_read_enable_method()
    * Comments cleanup.

Please let me know your preference so that I can give it another respin
if needed.

Thanks,
Gavin

>>
>> Link: https://lore.kernel.org/linux-arm-kernel/20200211114553.GA21093@e121166-lin.cambridge.arm.com
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>> ---
>>   arch/arm64/kernel/cpu_ops.c | 77 +++++++++++++++++++------------------
>>   1 file changed, 39 insertions(+), 38 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/cpu_ops.c b/arch/arm64/kernel/cpu_ops.c
>> index e133011f64b5..a0f647d22e36 100644
>> --- a/arch/arm64/kernel/cpu_ops.c
>> +++ b/arch/arm64/kernel/cpu_ops.c
>> @@ -20,41 +20,20 @@ extern const struct cpu_operations acpi_parking_protocol_ops;
>>   #endif
>>   extern const struct cpu_operations cpu_psci_ops;
>>   
>> -static const struct cpu_operations *cpu_ops[NR_CPUS] __ro_after_init;
>> -
>> -static const struct cpu_operations *const dt_supported_cpu_ops[] __initconst = {
>> +static const struct cpu_operations *const available_cpu_ops[] __initconst = {
>>   	&smp_spin_table_ops,
>> -	&cpu_psci_ops,
>> -	NULL,
>> -};
>> -
>> -static const struct cpu_operations *const acpi_supported_cpu_ops[] __initconst = {
>>   #ifdef CONFIG_ARM64_ACPI_PARKING_PROTOCOL
>>   	&acpi_parking_protocol_ops,
>>   #endif
>>   	&cpu_psci_ops,
>> -	NULL,
>>   };
>> +static const struct cpu_operations *boot_cpu_ops __ro_after_init;
>> +static const struct cpu_operations *secondary_cpu_ops __ro_after_init;
>>   
>> -static const struct cpu_operations * __init cpu_get_ops(const char *name)
>> -{
>> -	const struct cpu_operations *const *ops;
>> -
>> -	ops = acpi_disabled ? dt_supported_cpu_ops : acpi_supported_cpu_ops;
>> -
>> -	while (*ops) {
>> -		if (!strcmp(name, (*ops)->name))
>> -			return *ops;
>> -
>> -		ops++;
>> -	}
>> -
>> -	return NULL;
>> -}
>> -
>> -static const char *__init cpu_read_enable_method(int cpu)
>> +static const struct cpu_operations * __init get_cpu_method(int cpu)
>>   {
>>   	const char *enable_method;
>> +	int i;
>>   
>>   	if (acpi_disabled) {
>>   		struct device_node *dn = of_get_cpu_node(cpu, NULL);
>> @@ -91,22 +70,44 @@ static const char *__init cpu_read_enable_method(int cpu)
>>   		}
>>   	}
>>   
>> -	return enable_method;
>> +	if (!enable_method) {
>> +		pr_warn("No enable-method found on CPU %d\n", cpu);
>> +		return NULL;
>> +	}
>> +
>> +	/* Search in the array with method */
>> +	for (i = 0; i < ARRAY_SIZE(available_cpu_ops); i++) {
>> +		if (!strcmp(available_cpu_ops[i]->name, enable_method))
>> +			return available_cpu_ops[i];
>> +	}
>> +
>> +	return NULL;
>>   }
>> -/*
>> - * Read a cpu's enable method and record it in cpu_ops.
>> - */
>> +
>>   int __init init_cpu_ops(int cpu)
>>   {
>> -	const char *enable_method = cpu_read_enable_method(cpu);
>> +	const struct cpu_operations *ops = get_cpu_method(cpu);
>>   
>> -	if (!enable_method)
>> -		return -ENODEV;
>> -
>> -	cpu_ops[cpu] = cpu_get_ops(enable_method);
>> -	if (!cpu_ops[cpu]) {
>> -		pr_warn("Unsupported enable-method: %s\n", enable_method);
>> +	if (!ops)
>>   		return -EOPNOTSUPP;
>> +
>> +	/* Update boot CPU operations */
>> +	if (!cpu) {
>> +		boot_cpu_ops = ops;
>> +		return 0;
>> +	}
>> +
>> +	/* Update secondary CPU operations if it's not initialized yet */
>> +	if (!secondary_cpu_ops) {
>> +		secondary_cpu_ops = ops;
>> +		return 0;
>> +	}
>> +
>> +	/* We should have unified secondary CPU operations */
>> +	if (ops != secondary_cpu_ops) {
>> +		pr_warn("Invalid CPU operations %s (%s) on secondary CPU %d\n",
>> +			ops->name, secondary_cpu_ops->name, cpu);
>> +		return -EINVAL;
>>   	}
>>   
>>   	return 0;
>> @@ -114,5 +115,5 @@ int __init init_cpu_ops(int cpu)
>>   
>>   const struct cpu_operations *get_cpu_ops(int cpu)
>>   {
>> -	return cpu_ops[cpu];
>> +	return cpu ? secondary_cpu_ops : boot_cpu_ops;
>>   }
>> -- 
>> 2.23.0
>>
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v5 0/4] arm64: Dereference CPU operations indirectly

[Catalin Marinas]

On Thu, Mar 19, 2020 at 10:01:41AM +1100, Gavin Shan wrote:
> Gavin Shan (4):
>   arm64: Declare ACPI parking protocol CPU operation if needed
>   arm64: Rename cpu_read_ops() to init_cpu_ops()
>   arm64: Introduce get_cpu_ops() helper function
>   arm64: Remove CPU operations dereferencing array

I queued the first 3 patches for 5.7, they are useful on their own as a
code clean-up. I'll wait for the debate with Mark to settle on the last
patch.

Thanks.

-- 
Catalin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v5 0/4] arm64: Dereference CPU operations indirectly

[Gavin Shan]

On 3/25/20 4:29 AM, Catalin Marinas wrote:
> On Thu, Mar 19, 2020 at 10:01:41AM +1100, Gavin Shan wrote:
>> Gavin Shan (4):
>>    arm64: Declare ACPI parking protocol CPU operation if needed
>>    arm64: Rename cpu_read_ops() to init_cpu_ops()
>>    arm64: Introduce get_cpu_ops() helper function
>>    arm64: Remove CPU operations dereferencing array
> 
> I queued the first 3 patches for 5.7, they are useful on their own as a
> code clean-up. I'll wait for the debate with Mark to settle on the last
> patch.
> 
> Thanks.
> 

Thanks, Catalin. I'm also waiting for further comments on the last patch
from Mark. The patch can also be dropped if your guys think it's not needed.
Anyway, please let me know :)

Thanks,
Gavin


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel