Clean up ARM SMP/CPU hotplug implementations

The boot_lock is something that was required for ARM development platforms to ensure that the delay calibration worked properly. This is not necessary for modern platforms that have better bus bandwidth and do not need to calibrate the delay loop for secondary cores. Remove the boot_lock entirely.

Signed-off-by Russell King <[email protected]>

Quoted 8 lines

The boot_lock is something that was required for ARM development platforms to ensure that the delay calibration worked properly. This is not necessary for modern platforms that have better bus bandwidth and do not need to calibrate the delay loop for secondary cores. Remove the boot_lock entirely.

Signed-off-by Russell King <[email protected]>

Andy have you had a chance to test/apply this patch?

I think the majority of Qcom systems use CLKSRC_QCOM which registers an arch delay timer and swiftly bypasses the calibration anyways so this code is pretty much pointless.

There is a question about MSM8974 though. This appears to not use CLKSRC_QCOM but rather HAVE_ARM_ARCH_TIMER. drivers/clocksource/arm_arch_timer.c seems to
not register a delay timer, which is probably something that should be fixed?

Mark/Marc: is there any specific reason to why the ARM arch timer does not register an arch delay timer?

Quoting Linus Walleij (2019-01-10 04:50:56)

Quoted 2 lines

Mark/Marc: is there any specific reason to why the ARM arch timer does not register an arch delay timer?

Doesn't the ARM arch timer get registered in arch/arm/kernel/arch_timer.c via arch_timer_delay_timer_register()?

Quoted 7 lines · Russell King

The boot_lock is something that was required for ARM development platforms to ensure that the delay calibration worked properly. This is not necessary for modern platforms that have better bus bandwidth and do not need to calibrate the delay loop for secondary cores. Remove the boot_lock entirely.

Signed-off-by Russell King <[email protected]>

Andy have you had a chance to test/apply this patch?

Quoted 10 lines · Linus Walleij

Quoted 7 lines · Russell King

The boot_lock is something that was required for ARM development platforms to ensure that the delay calibration worked properly. This is not necessary for modern platforms that have better bus bandwidth and do not need to calibrate the delay loop for secondary cores. Remove the boot_lock entirely.

Signed-off-by Russell King <[email protected]>

Quoted 1 line

Andy have you had a chance to test/apply this patch?

I would rather folk do not _apply_ these patches - the final patch is dependent on all the preceeding patches, and if they're applied to individual trees, it will mean that the final patch has to be delayed by a complete kernel development cycle to avoid bisect breakage.

The boot_lock is something that was required for ARM development platforms to ensure that the delay calibration worked properly. This is not necessary for modern platforms that have better bus bandwidth and do not need to calibrate the delay loop for secondary cores. Remove the boot_lock entirely.

Signed-off-by Russell King <[email protected]>

---

The CPU hotplug implementation on this platform is cargo-culted from the plat-versatile implementation, and is buggy. Once a CPU hits the "low power" loop, it will wait for pen_release to be set to the CPU number to wake up again - but nothing in this implementation does that.

So, once a CPU has entered cpu_die() it will never, ever leave.

Remove this useless cargo-culted implementation.

Signed-off-by Russell King <[email protected]>

---
 arch/arm/mach-oxnas/Makefile  |   1 -
 arch/arm/mach-oxnas/hotplug.c | 109 ------------------------------------------
 arch/arm/mach-oxnas/platsmp.c |   4 --
 3 files changed, 114 deletions(-)
 delete mode 100644 arch/arm/mach-oxnas/hotplug.c

arch/arm/mach-oxnas/Makefile +0 -1

index b625906a9970..61a34e1c0f22 100644
@@ -1,2 +1 @@
 obj-$(CONFIG_SMP)		+= platsmp.o headsmp.o
-obj-$(CONFIG_HOTPLUG_CPU) 	+= hotplug.o

arch/arm/mach-oxnas/hotplug.c deleted +0 -109

deleted file mode 100644
index 854f29b8cba6..000000000000
@@ -1,109 +0,0 @@
-/*
- *  Copyright (C) 2002 ARM Ltd.
- *  All Rights Reserved
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- */
-#include <linux/kernel.h>
-#include <linux/errno.h>
-#include <linux/smp.h>
-
-#include <asm/cp15.h>
-#include <asm/smp_plat.h>
-
-static inline void cpu_enter_lowpower(void)
-{
-	unsigned int v;
-
-	asm volatile(
-	"	mcr	p15, 0, %1, c7, c5, 0\n"
-	"	mcr	p15, 0, %1, c7, c10, 4\n"
-	/*
-	 * Turn off coherency
-	 */
-	"	mrc	p15, 0, %0, c1, c0, 1\n"
-	"	bic	%0, %0, #0x20\n"
-	"	mcr	p15, 0, %0, c1, c0, 1\n"
-	"	mrc	p15, 0, %0, c1, c0, 0\n"
-	"	bic	%0, %0, %2\n"
-	"	mcr	p15, 0, %0, c1, c0, 0\n"
-	  : "=&r" (v)
-	  : "r" (0), "Ir" (CR_C)
-	  : "cc");
-}
-
-static inline void cpu_leave_lowpower(void)
-{
-	unsigned int v;
-
-	asm volatile(	"mrc	p15, 0, %0, c1, c0, 0\n"
-	"	orr	%0, %0, %1\n"
-	"	mcr	p15, 0, %0, c1, c0, 0\n"
-	"	mrc	p15, 0, %0, c1, c0, 1\n"
-	"	orr	%0, %0, #0x20\n"
-	"	mcr	p15, 0, %0, c1, c0, 1\n"
-	  : "=&r" (v)
-	  : "Ir" (CR_C)
-	  : "cc");
-}
-
-static inline void platform_do_lowpower(unsigned int cpu, int *spurious)
-{
-	/*
-	 * there is no power-control hardware on this platform, so all
-	 * we can do is put the core into WFI; this is safe as the calling
-	 * code will have already disabled interrupts
-	 */
-	for (;;) {
-		/*
-		 * here's the WFI
-		 */
-		asm(".word	0xe320f003\n"
-		    :
-		    :
-		    : "memory", "cc");
-
-		if (pen_release == cpu_logical_map(cpu)) {
-			/*
-			 * OK, proper wakeup, we're done
-			 */
-			break;
-		}
-
-		/*
-		 * Getting here, means that we have come out of WFI without
-		 * having been woken up - this shouldn't happen
-		 *
-		 * Just note it happening - when we're woken, we can report
-		 * its occurrence.
-		 */
-		(*spurious)++;
-	}
-}
-
-/*
- * platform-specific code to shutdown a CPU
- *
- * Called with IRQs disabled
- */
-void ox820_cpu_die(unsigned int cpu)
-{
-	int spurious = 0;
-
-	/*
-	 * we're ready for shutdown now, so do it
-	 */
-	cpu_enter_lowpower();
-	platform_do_lowpower(cpu, &spurious);
-
-	/*
-	 * bring this CPU back into the world of cache
-	 * coherency, and then restore interrupts
-	 */
-	cpu_leave_lowpower();
-
-	if (spurious)
-		pr_warn("CPU%u: %u spurious wakeup calls\n", cpu, spurious);
-}

arch/arm/mach-oxnas/platsmp.c +0 -1

index 442cc8a2f7dc..735141c0e3a3 100644
@@ -19,7 +19,6 @@
 #include <asm/smp_scu.h>
 
 extern void ox820_secondary_startup(void);
-extern void ox820_cpu_die(unsigned int cpu);
 
 static void __iomem *cpu_ctrl;
 static void __iomem *gic_cpu_ctrl;

The pen_release implementation was created for Versatile platforms to work around boot loaders that did not differentiate between the various different secondary CPUs on this ARM development platform. This should not be true of modern platforms where we send IPIs to specific CPUs to wake them up. Remove the pen_release stuff from SoCs that make use of the per-CPU IPI mechanism.

The boot_lock is something that was required for ARM development platforms to ensure that the delay calibration worked properly. This is not necessary for modern platforms that have better bus bandwidth and do not need to calibrate the delay loop for secondary cores. Remove the boot_lock entirely.

Signed-off-by Russell King <[email protected]>

---
 arch/arm/mach-sti/Makefile  |  2 +-
 arch/arm/mach-sti/headsmp.S | 43 -------------------------------
 arch/arm/mach-sti/platsmp.c | 62 ++-------------------------------------------
 3 files changed, 3 insertions(+), 104 deletions(-)
 delete mode 100644 arch/arm/mach-sti/headsmp.S

arch/arm/mach-sti/Makefile +1 -1

index acb330916333..f85ff059cfba 100644
@@ -1,2 +1,2 @@
-obj-$(CONFIG_SMP)		+= platsmp.o headsmp.o
+obj-$(CONFIG_SMP)		+= platsmp.o
 obj-$(CONFIG_ARCH_STI) 		+= board-dt.o

arch/arm/mach-sti/headsmp.S deleted +0 -43

deleted file mode 100644
index e0ad451700d5..000000000000
@@ -1,43 +0,0 @@
-/*
- *  arch/arm/mach-sti/headsmp.S
- *
- * Copyright (C) 2013 STMicroelectronics (R&D) Limited.
- *		http://www.st.com
- *
- * Cloned from linux/arch/arm/mach-vexpress/headsmp.S
- *
- *  Copyright (c) 2003 ARM Limited
- *  All Rights Reserved
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- */
-#include <linux/linkage.h>
-#include <linux/init.h>
-
-/*
- * ST specific entry point for secondary CPUs.  This provides
- * a "holding pen" into which all secondary cores are held until we're
- * ready for them to initialise.
- */
-ENTRY(sti_secondary_startup)
-	mrc	p15, 0, r0, c0, c0, 5
-	and	r0, r0, #15
-	adr	r4, 1f
-	ldmia	r4, {r5, r6}
-	sub	r4, r4, r5
-	add	r6, r6, r4
-pen:	ldr	r7, [r6]
-	cmp	r7, r0
-	bne	pen
-
-	/*
-	 * we've been released from the holding pen: secondary_stack
-	 * should now contain the SVC stack for this core
-	 */
-	b	secondary_startup
-ENDPROC(sti_secondary_startup)
-
-1:	.long	.
-	.long	pen_release

arch/arm/mach-sti/platsmp.c +2 -59

index 231f19e17436..21668501c9bb 100644
@@ -28,72 +28,15 @@
 
 #include "smp.h"
 
-static void write_pen_release(int val)
-{
-	pen_release = val;
-	smp_wmb();
-	sync_cache_w(&pen_release);
-}
-
-static DEFINE_SPINLOCK(boot_lock);
-
-static void sti_secondary_init(unsigned int cpu)
-{
-	/*
-	 * let the primary processor know we're out of the
-	 * pen, then head off into the C entry point
-	 */
-	write_pen_release(-1);
-
-	/*
-	 * Synchronise with the boot thread.
-	 */
-	spin_lock(&boot_lock);
-	spin_unlock(&boot_lock);
-}
-
 static int sti_boot_secondary(unsigned int cpu, struct task_struct *idle)
 {
-	unsigned long timeout;
-
-	/*
-	 * set synchronisation state between this boot processor
-	 * and the secondary one
-	 */
-	spin_lock(&boot_lock);
-
-	/*
-	 * The secondary processor is waiting to be released from
-	 * the holding pen - release it, then wait for it to flag
-	 * that it has been released by resetting pen_release.
-	 *
-	 * Note that "pen_release" is the hardware CPU ID, whereas
-	 * "cpu" is Linux's internal ID.
-	 */
-	write_pen_release(cpu_logical_map(cpu));
-
 	/*
 	 * Send the secondary CPU a soft interrupt, thereby causing
 	 * it to jump to the secondary entrypoint.
 	 */
 	arch_send_wakeup_ipi_mask(cpumask_of(cpu));
 
-	timeout = jiffies + (1 * HZ);
-	while (time_before(jiffies, timeout)) {
-		smp_rmb();
-		if (pen_release == -1)
-			break;
-
-		udelay(10);
-	}
-
-	/*
-	 * now the secondary core is starting up let it run its
-	 * calibrations, then wait for it to finish
-	 */
-	spin_unlock(&boot_lock);
-
-	return pen_release != -1 ? -ENOSYS : 0;
+	return 0;
 }
 
 static void __init sti_smp_prepare_cpus(unsigned int max_cpus)

@@ -103,7 +46,7 @@ static void __init sti_smp_prepare_cpus(unsigned int max_cpus)
 	u32 __iomem *cpu_strt_ptr;
 	u32 release_phys;
 	int cpu;
-	unsigned long entry_pa = __pa_symbol(sti_secondary_startup);
+	unsigned long entry_pa = __pa_symbol(secondary_startup);
 
 	np = of_find_compatible_node(NULL, NULL, "arm,cortex-a9-scu");
 

arch/arm/mach-spear/generic.h +0 -0

 extern void spear13xx_timer_init(void);
 extern void spear3xx_timer_init(void);
 extern struct pl022_ssp_controller pl022_plat_data;

arch/arm/mach-spear/headsmp.S +1 -1

index c52192dc3d9f..6e250b6c0aa2 100644
@@ -43,5 +43,5 @@ pen:	ldr	r7, [r6]
 
 	.align
 1:	.long	.
-	.long	pen_release
+	.long	spear_pen_release
 ENDPROC(spear13xx_secondary_startup)

arch/arm/mach-spear/hotplug.c +3 -1

index 12edd1cf8a12..0dd84f609627 100644
@@ -16,6 +16,8 @@
 #include <asm/cp15.h>
 #include <asm/smp_plat.h>
 
+#include "generic.h"
+
 static inline void cpu_enter_lowpower(void)
 {
 	unsigned int v;

@@ -57,7 +59,7 @@ static inline void spear13xx_do_lowpower(unsigned int cpu, int *spurious)
 	for (;;) {
 		wfi();
 
-		if (pen_release == cpu) {
+		if (spear_pen_release == cpu) {
 			/*
 			 * OK, proper wakeup, we're done
 			 */

arch/arm/mach-spear/platsmp.c +16 -11

index 39038a03836a..b1ff4bb86f6d 100644
@@ -20,16 +20,21 @@
 #include <mach/spear.h>
 #include "generic.h"
 
+/* XXX spear_pen_release is cargo culted code - DO NOT COPY XXX */
+volatile int spear_pen_release = -1;
+
 /*
- * Write pen_release in a way that is guaranteed to be visible to all
- * observers, irrespective of whether they're taking part in coherency
+ * XXX CARGO CULTED CODE - DO NOT COPY XXX
+ *
+ * Write spear_pen_release in a way that is guaranteed to be visible to
+ * all observers, irrespective of whether they're taking part in coherency
  * or not.  This is necessary for the hotplug code to work reliably.
  */
-static void write_pen_release(int val)
+static void spear_write_pen_release(int val)
 {
-	pen_release = val;
+	spear_pen_release = val;
 	smp_wmb();
-	sync_cache_w(&pen_release);
+	sync_cache_w(&spear_pen_release);
 }
 
 static DEFINE_SPINLOCK(boot_lock);

@@ -42,7 +47,7 @@ static void spear13xx_secondary_init(unsigned int cpu)
 	 * let the primary processor know we're out of the
 	 * pen, then head off into the C entry point
 	 */
-	write_pen_release(-1);
+	spear_write_pen_release(-1);
 
 	/*
 	 * Synchronise with the boot thread.

@@ -64,17 +69,17 @@ static int spear13xx_boot_secondary(unsigned int cpu, struct task_struct *idle)
 	/*
 	 * The secondary processor is waiting to be released from
 	 * the holding pen - release it, then wait for it to flag
-	 * that it has been released by resetting pen_release.
+	 * that it has been released by resetting spear_pen_release.
 	 *
-	 * Note that "pen_release" is the hardware CPU ID, whereas
+	 * Note that "spear_pen_release" is the hardware CPU ID, whereas
 	 * "cpu" is Linux's internal ID.
 	 */
-	write_pen_release(cpu);
+	spear_write_pen_release(cpu);
 
 	timeout = jiffies + (1 * HZ);
 	while (time_before(jiffies, timeout)) {
 		smp_rmb();
-		if (pen_release == -1)
+		if (spear_pen_release == -1)
 			break;
 
 		udelay(10);

@@ -86,7 +91,7 @@ static int spear13xx_boot_secondary(unsigned int cpu, struct task_struct *idle)
 	 */
 	spin_unlock(&boot_lock);
 
-	return pen_release != -1 ? -ENOSYS : 0;
+	return spear_pen_release != -1 ? -ENOSYS : 0;
 }
 
 /*

Signature · 1 line

2.7.4

Quoted 6 lines · Russell King

arch/arm/mach-spear/generic.h +0 -0

index 909b97c0b237..815333fc320c 100644
@@ -20,6 +20,8 @@
 

arch/arm/mach-spear/generic.h line 23

Quoted 3 lines

arch/arm/mach-spear/generic.h +1 -0

 #include <asm/mach/time.h>
 
+extern volatile int prima2_pen_release;

prima2 ?

I haven't tried but this may cause build regressions as well, I am surprised on how this passed for you.

Quoted 109 lines

arch/arm/mach-spear/generic.h +0 -0

 extern void spear13xx_timer_init(void);
 extern void spear3xx_timer_init(void);
 extern struct pl022_ssp_controller pl022_plat_data;

arch/arm/mach-spear/headsmp.S +1 -1

index c52192dc3d9f..6e250b6c0aa2 100644
@@ -43,5 +43,5 @@ pen:	ldr	r7, [r6]
 
 	.align
 1:	.long	.
-	.long	pen_release
+	.long	spear_pen_release
 ENDPROC(spear13xx_secondary_startup)

arch/arm/mach-spear/hotplug.c +3 -1

index 12edd1cf8a12..0dd84f609627 100644
@@ -16,6 +16,8 @@
 #include <asm/cp15.h>
 #include <asm/smp_plat.h>
 
+#include "generic.h"
+
 static inline void cpu_enter_lowpower(void)
 {
 	unsigned int v;

@@ -57,7 +59,7 @@ static inline void spear13xx_do_lowpower(unsigned int cpu, int *spurious)
 	for (;;) {
 		wfi();
 
-		if (pen_release == cpu) {
+		if (spear_pen_release == cpu) {
 			/*
 			 * OK, proper wakeup, we're done
 			 */

arch/arm/mach-spear/platsmp.c +16 -11

index 39038a03836a..b1ff4bb86f6d 100644
@@ -20,16 +20,21 @@
 #include <mach/spear.h>
 #include "generic.h"
 
+/* XXX spear_pen_release is cargo culted code - DO NOT COPY XXX */
+volatile int spear_pen_release = -1;
+
 /*
- * Write pen_release in a way that is guaranteed to be visible to all
- * observers, irrespective of whether they're taking part in coherency
+ * XXX CARGO CULTED CODE - DO NOT COPY XXX
+ *
+ * Write spear_pen_release in a way that is guaranteed to be visible to
+ * all observers, irrespective of whether they're taking part in coherency
  * or not.  This is necessary for the hotplug code to work reliably.
  */
-static void write_pen_release(int val)
+static void spear_write_pen_release(int val)
 {
-	pen_release = val;
+	spear_pen_release = val;
 	smp_wmb();
-	sync_cache_w(&pen_release);
+	sync_cache_w(&spear_pen_release);
 }
 
 static DEFINE_SPINLOCK(boot_lock);

@@ -42,7 +47,7 @@ static void spear13xx_secondary_init(unsigned int cpu)
 	 * let the primary processor know we're out of the
 	 * pen, then head off into the C entry point
 	 */
-	write_pen_release(-1);
+	spear_write_pen_release(-1);
 
 	/*
 	 * Synchronise with the boot thread.

@@ -64,17 +69,17 @@ static int spear13xx_boot_secondary(unsigned int cpu, struct task_struct *idle)
 	/*
 	 * The secondary processor is waiting to be released from
 	 * the holding pen - release it, then wait for it to flag
-	 * that it has been released by resetting pen_release.
+	 * that it has been released by resetting spear_pen_release.
 	 *
-	 * Note that "pen_release" is the hardware CPU ID, whereas
+	 * Note that "spear_pen_release" is the hardware CPU ID, whereas
 	 * "cpu" is Linux's internal ID.
 	 */
-	write_pen_release(cpu);
+	spear_write_pen_release(cpu);
 
 	timeout = jiffies + (1 * HZ);
 	while (time_before(jiffies, timeout)) {
 		smp_rmb();
-		if (pen_release == -1)
+		if (spear_pen_release == -1)
 			break;
 
 		udelay(10);

@@ -86,7 +91,7 @@ static int spear13xx_boot_secondary(unsigned int cpu, struct task_struct *idle)
 	 */
 	spin_unlock(&boot_lock);
 
-	return pen_release != -1 ? -ENOSYS : 0;
+	return spear_pen_release != -1 ? -ENOSYS : 0;
 }
 
 /*

Signature · 1 line

2.7.4

Signature · 6 lines

viresh

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 13 lines · Viresh Kumar

arch/arm/mach-spear/generic.h line 23

Quoted 9 lines · Russell King

arch/arm/mach-spear/generic.h +1 -0

index 909b97c0b237..815333fc320c 100644
@@ -20,6 +20,8 @@
 
 #include <asm/mach/time.h>
 
+extern volatile int prima2_pen_release;

prima2 ?

Quoted 2 lines

I haven't tried but this may cause build regressions as well, I am surprised on how this passed for you.

Thanks.

These patches aren't all build tested -

I don't see the point of wasting
hours build testing when these platforms /really/ need something better.

This is not supposed to be a finished patch, but a patch to get folk to do something about this issue.

Please fix this issue properly. I can't give you a patch to do that, you need to sort it yourself, but failure to do anything _will_ result in an updated version of this patch being merged.

There is a lot of apparent copied code in arch/arm for handling SMP/ CPU hotplug, much of which is inappropriate or plain buggy. This seems to be a topic that occasionally comes up.

The "pen_release" thing was created for ARM Ltd development platforms where there was no way to individually control secondary CPUs leaving the boot loader - they all jumped to whatever physical code address was supplied at the same time. This made it necessary for _these_ platforms to have a "holding pen" for the CPUs while the kernel initialised.

The "boot_lock" thing was also created for ARM Ltd development platforms which had restricted bus bandwidth, and which used the loops_per_jiffy delay mechanism, which was calibrated for each secondary CPU. With the restricted bus bandwidth, activity from the boot CPU would affect the delay calibration adversely.

Lastly, the Versatile CPU hotplug implementation is an entirely ficticious one - these platforms do _not_ support CPU hotplug as there is no way to actually disable any of the secondary CPUs, or reset them. Such an implementation is not acceptable when supporting features such as suspend or kexec. As the Versatile platforms are ARM development platforms which do not have suspend support, this is acceptable there, but not for production hardware.

None of these three facilities/implementations should be used on modern production hardware, yet we have a number of copies of this code.

This series addresses that by removing the inappropriate copies of some Realview/Versatile Express specific workarounds, and makes it (hopefully) more clear that introducing this code is really not acceptable.

To discourage copying the Versatile code, further comments are added and the functions renamed for CPU hotplug to be "immitation" to make it clear that it's not a real implementation.

We tried reducing the duplication in the past with ideas around consolidating the pen_release/boot_lock/immitation hotplug stuff, but I nacked that because it's not an acceptable implementation for production hardware. However, we did decide to consolidate the "pen_release" definition. In hind sight, that was a mistake, because that gave more credence to that way of doing things, and also gave rise to buggy implementations which only read from that variable - meaning it served no useful purpose.

There are some rather complex cases that remain, and those need the SoC folk to fix.

I have left the Actions Semi patch in place since following patches depend on it, but there is a five-patch series from Linus Walleij that address this platform which should replace this patch - with the patch concerned marked as "RFT" - request for testing.

 arch/arm/include/asm/smp.h                         |   1 -
 arch/arm/kernel/smp.c                              |   6 --
 arch/arm/mach-actions/platsmp.c                    |  15 ---
 arch/arm/mach-exynos/headsmp.S                     |   2 +-
 arch/arm/mach-exynos/platsmp.c                     |  31 +++---
 arch/arm/mach-omap2/omap-smp.c                     |  20 ----
 arch/arm/mach-oxnas/Makefile                       |   1 -
 arch/arm/mach-oxnas/hotplug.c                      | 109 --------------------
 arch/arm/mach-oxnas/platsmp.c                      |   4 -
 arch/arm/mach-prima2/common.h                      |   2 +
 arch/arm/mach-prima2/headsmp.S                     |   2 +-
 arch/arm/mach-prima2/hotplug.c                     |   3 +-
 arch/arm/mach-prima2/platsmp.c                     |  17 ++--
 arch/arm/mach-qcom/platsmp.c                       |  26 -----
 arch/arm/mach-realview/Makefile                    |   1 -
 arch/arm/mach-realview/hotplug.c                   | 111 ---------------------
 arch/arm/mach-realview/hotplug.h                   |   1 -
 arch/arm/mach-realview/platsmp-dt.c                |   8 +-
 arch/arm/mach-spear/generic.h                      |   2 +
 arch/arm/mach-spear/headsmp.S                      |   2 +-
 arch/arm/mach-spear/hotplug.c                      |   4 +-
 arch/arm/mach-spear/platsmp.c                      |  27 +++--
 arch/arm/mach-sti/Makefile                         |   2 +-
 arch/arm/mach-sti/headsmp.S                        |  43 --------
 arch/arm/mach-sti/platsmp.c                        |  62 +-----------
 arch/arm/mach-vexpress/Makefile                    |   1 -
 arch/arm/mach-vexpress/core.h                      |   2 -
 arch/arm/mach-vexpress/platsmp.c                   |   7 ++
 arch/arm/plat-versatile/Makefile                   |   1 +
 arch/arm/plat-versatile/headsmp.S                  |   2 +-
 .../{mach-vexpress => plat-versatile}/hotplug.c    |  47 ++++-----
 arch/arm/plat-versatile/include/plat/platsmp.h     |   2 +
 arch/arm/plat-versatile/platsmp.c                  |  47 ++++++---
 33 files changed, 132 insertions(+), 479 deletions(-)
 delete mode 100644 arch/arm/mach-oxnas/hotplug.c
 delete mode 100644 arch/arm/mach-realview/hotplug.c
 delete mode 100644 arch/arm/mach-realview/hotplug.h
 delete mode 100644 arch/arm/mach-sti/headsmp.S
 rename arch/arm/{mach-vexpress => plat-versatile}/hotplug.c (56%)

Signature · 8 lines

RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Since almost no one has responded, my intention is to queue up patches 1-3,5-8 for the Christmas-time merge window through my tree. They will be in linux-next tonight.

Quoted 100 lines

There is a lot of apparent copied code in arch/arm for handling SMP/ CPU hotplug, much of which is inappropriate or plain buggy. This seems to be a topic that occasionally comes up.

The "pen_release" thing was created for ARM Ltd development platforms where there was no way to individually control secondary CPUs leaving the boot loader - they all jumped to whatever physical code address was supplied at the same time. This made it necessary for _these_ platforms to have a "holding pen" for the CPUs while the kernel initialised.

The "boot_lock" thing was also created for ARM Ltd development platforms which had restricted bus bandwidth, and which used the loops_per_jiffy delay mechanism, which was calibrated for each secondary CPU. With the restricted bus bandwidth, activity from the boot CPU would affect the delay calibration adversely.

Lastly, the Versatile CPU hotplug implementation is an entirely ficticious one - these platforms do _not_ support CPU hotplug as there is no way to actually disable any of the secondary CPUs, or reset them. Such an implementation is not acceptable when supporting features such as suspend or kexec. As the Versatile platforms are ARM development platforms which do not have suspend support, this is acceptable there, but not for production hardware.

None of these three facilities/implementations should be used on modern production hardware, yet we have a number of copies of this code.

This series addresses that by removing the inappropriate copies of some Realview/Versatile Express specific workarounds, and makes it (hopefully) more clear that introducing this code is really not acceptable.

To discourage copying the Versatile code, further comments are added and the functions renamed for CPU hotplug to be "immitation" to make it clear that it's not a real implementation.

We tried reducing the duplication in the past with ideas around consolidating the pen_release/boot_lock/immitation hotplug stuff, but I nacked that because it's not an acceptable implementation for production hardware. However, we did decide to consolidate the "pen_release" definition. In hind sight, that was a mistake, because that gave more credence to that way of doing things, and also gave rise to buggy implementations which only read from that variable - meaning it served no useful purpose.

There are some rather complex cases that remain, and those need the SoC folk to fix.

I have left the Actions Semi patch in place since following patches depend on it, but there is a five-patch series from Linus Walleij that address this platform which should replace this patch - with the patch concerned marked as "RFT" - request for testing.

 arch/arm/include/asm/smp.h                         |   1 -
 arch/arm/kernel/smp.c                              |   6 --
 arch/arm/mach-actions/platsmp.c                    |  15 ---
 arch/arm/mach-exynos/headsmp.S                     |   2 +-
 arch/arm/mach-exynos/platsmp.c                     |  31 +++---
 arch/arm/mach-omap2/omap-smp.c                     |  20 ----
 arch/arm/mach-oxnas/Makefile                       |   1 -
 arch/arm/mach-oxnas/hotplug.c                      | 109 --------------------
 arch/arm/mach-oxnas/platsmp.c                      |   4 -
 arch/arm/mach-prima2/common.h                      |   2 +
 arch/arm/mach-prima2/headsmp.S                     |   2 +-
 arch/arm/mach-prima2/hotplug.c                     |   3 +-
 arch/arm/mach-prima2/platsmp.c                     |  17 ++--
 arch/arm/mach-qcom/platsmp.c                       |  26 -----
 arch/arm/mach-realview/Makefile                    |   1 -
 arch/arm/mach-realview/hotplug.c                   | 111 ---------------------
 arch/arm/mach-realview/hotplug.h                   |   1 -
 arch/arm/mach-realview/platsmp-dt.c                |   8 +-
 arch/arm/mach-spear/generic.h                      |   2 +
 arch/arm/mach-spear/headsmp.S                      |   2 +-
 arch/arm/mach-spear/hotplug.c                      |   4 +-
 arch/arm/mach-spear/platsmp.c                      |  27 +++--
 arch/arm/mach-sti/Makefile                         |   2 +-
 arch/arm/mach-sti/headsmp.S                        |  43 --------
 arch/arm/mach-sti/platsmp.c                        |  62 +-----------
 arch/arm/mach-vexpress/Makefile                    |   1 -
 arch/arm/mach-vexpress/core.h                      |   2 -
 arch/arm/mach-vexpress/platsmp.c                   |   7 ++
 arch/arm/plat-versatile/Makefile                   |   1 +
 arch/arm/plat-versatile/headsmp.S                  |   2 +-
 .../{mach-vexpress => plat-versatile}/hotplug.c    |  47 ++++-----
 arch/arm/plat-versatile/include/plat/platsmp.h     |   2 +
 arch/arm/plat-versatile/platsmp.c                  |  47 ++++++---
 33 files changed, 132 insertions(+), 479 deletions(-)
 delete mode 100644 arch/arm/mach-oxnas/hotplug.c
 delete mode 100644 arch/arm/mach-realview/hotplug.c
 delete mode 100644 arch/arm/mach-realview/hotplug.h
 delete mode 100644 arch/arm/mach-sti/headsmp.S
 rename arch/arm/{mach-vexpress => plat-versatile}/hotplug.c (56%)

--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up

Quoted 4 lines

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up

Quoted 4 lines · Russell King - ARM Linux

Since almost no one has responded, my intention is to queue up patches 1-3,5-8 for the Christmas-time merge window through my tree. They will be in linux-next tonight.

AFAIU, the patch 9 (or entire patchset) was not build tested, did not compile and therefore was not in linux-next. Sending something, which was not building, to linux-next just few days before Christmas merge window makes the schedule really tight. Especially that during Christmas some people might be offline.

I think it should sit in linux-next for few weeks... it should sit there already so the auto-testers would try it. At least if it were in linux-next, the booting and few simple tests were already done, e.g. by my boards, without any additional effort.