ARM: rename is_smp()

is_smp() is causing some confusion - rename it to indicate that it's a property of the CPU that we're running on, which is not the same as the system. Document what and why it is being used at most sites.

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

---
Some people are reporting that "is_smp()" is broken wrt Cortex-A15 CPUs when they integrate a single Cortex-A15 SMP capable CPU into a uniprocessor system. This is most likely because of a misunderstanding about what is_smp() is really detected from: it's detected from the CPU capabilities, not from the system capabilities. If the CPU says that it is SMP capable (and it's not a broken Cortex-A9 core) we will make use of various instructions which appear on SMP cores, and we set is_smp() to follow that. So, is_smp() is more of a CPU capability rather than a system capability.

Trying to use it as a system capability will lead to problems. Arguably, the use of it in arch_irq_work_has_interrupt() is wrong, because we don't know whether the GIC is SMP capable or not, but it's currently the best we can do.

I felt the other two sites I left undocumented (which read the MPIDR) were rather obvious - a uniprocessor only capable CPU doesn't have a MPIDR.

Really, !cpu_smp() is an indication that the CPU is UP-only, not that it is _S_MP capable, so even this is lightly misleading. I suppose we could replace it with cpu_up_only() but I think that makes the code harder to understand (due to double-negatives appearing in places.)

 arch/arm/include/asm/cputype.h  |  2 +-
 arch/arm/include/asm/irq_work.h |  2 +-
 arch/arm/include/asm/smp_plat.h | 11 +++++++----
 arch/arm/kernel/devtree.c       |  2 +-
 arch/arm/kernel/module.c        | 10 +++++++++-
 arch/arm/kernel/setup.c         |  7 ++++---
 arch/arm/mach-mvebu/coherency.c |  4 ++--
 arch/arm/mm/mmu.c               | 13 ++++++++++++-
 8 files changed, 37 insertions(+), 14 deletions(-)

arch/arm/mach-mvebu/coherency.c +1 -1

index ae2a018b9305..fe4b1e15ebb8 100644
@@ -216,7 +216,7 @@ static int coherency_type(void)
 	 *
 	 * Note that this means that on Armada 370, there is currently
 	 * no way to use hardware I/O coherency, because even when
-	 * CONFIG_SMP is enabled, is_smp() returns false due to the
+	 * CONFIG_SMP is enabled, cpu_smp() returns false due to the
 	 * Armada 370 being a single-core processor. To lift this
 	 * limitation, we would have to find a way to make the cache
 	 * policy set to write-allocate (on all Armada SoCs), and to

arch/arm/mm/mmu.c +12 -1

index 4001dd15818d..d3dc758d5391 100644
@@ -449,11 +449,22 @@ static void __init build_mem_type_table(void)
 		ecc_mask = 0;
 	}
 
-	if (is_smp()) {
+	if (cpu_smp()) {
+		/*
+		 * SMP requires a write-allocate cache policy for proper
+		 * functioning of the coherency protocol.  If the CPU supports
+		 * MP extensions, assume we are part of a SMP system.
+		 */
 		if (cachepolicy != CPOLICY_WRITEALLOC) {
 			pr_warn("Forcing write-allocate cache policy for SMP\n");
 			cachepolicy = CPOLICY_WRITEALLOC;
 		}
+
+		/*
+		 * SMP systems depend on the S bit being shared for coherency
+		 * between the CPUs.  However, setting this for non-SMP CPUs
+		 * may result in the mappings being treated as uncached.
+		 */
 		if (!(initial_pmd_value & PMD_SECT_S)) {
 			pr_warn("Forcing shared mappings for SMP\n");
 			initial_pmd_value |= PMD_SECT_S;

Signature · 1 line

2.7.4

Hi Russell,

Unless I am wrong I don't see any modification of the code behavior:
just renaming the function and adding more comments. So it won't affect our platform and I am OK with the new name which also match our comments.

Following on from the previous patch, I think this makes more sense to determine whether we can support IRQ work interrupts.

Whether we can support them or not depends on two things:

(a) whether the kernel has support for receiving IPIs
(b) whether it's possible to send an IPI to CPUs including the raising CPU.

(a) is a function of how the kernel is built - and in the case of ARM, it depends whether the kernel is built with SMP enabled or not.
(b) is a property of the interrupt controller.

It hasn't ever been a function of the CPU or architecture.

Commit 059e232089e4 ("irqchip/gic: Allow self-SGIs for SMP on UP configurations") changes the GIC IPI code such that we can raise SGIs on uniprocessor systems running on a SMP kernel, which means we can support IRQ work interrupts here as well.

So, we shouldn't be using cpu_smp() (or its previous is_smp() here at all. Use a flag to indicate whether we can IPI and use that to indicate whether we support irq work interrupts.

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

---

 arch/arm/include/asm/irq_work.h | 11 +++++++++--
 arch/arm/kernel/irq.c           |  0
 arch/arm/kernel/smp.c           |  3 +++
 drivers/irqchip/irq-gic.c       | 17 +++++++++++++----
 4 files changed, 25 insertions(+), 6 deletions(-)

arch/arm/include/asm/irq_work.h +9 -2

index 2dc8d7995b48..d7262a3c2f2e 100644
@@ -1,11 +1,18 @@
 #ifndef __ASM_ARM_IRQ_WORK_H
 #define __ASM_ARM_IRQ_WORK_H
 
-#include <asm/smp_plat.h>
+extern bool irq_controller_can_ipi;
+#define irq_controller_can_ipi irq_controller_can_ipi
 
 static inline bool arch_irq_work_has_interrupt(void)
 {
-	return cpu_smp();
+#ifdef CONFIG_SMP
+	/* This depends on the IRQ controller */
+	return irq_controller_can_ipi;
+#else
+	/* The kernel is not built to support IPIs */
+	return false;
+#endif
 }
 
 #endif /* _ASM_ARM_IRQ_WORK_H */

arch/arm/kernel/smp.c +0 -0

index 7dd14e8395e6..1fa9412cc4aa 100644

arch/arm/kernel/smp.c +1 -0

+
 static const char *ipi_types[NR_IPI] __tracepoint_string = {
 #define S(x,s)	[x] = s
 	S(IPI_WAKEUP, "CPU wakeup interrupts"),

drivers/irqchip/irq-gic.c +1 -4

index d6c404b3584d..abe8d5807c0f 100644
@@ -1187,9 +1187,6 @@ static int __init __gic_init_bases(struct gic_chip_data *gic,
 		 */
 		for (i = 0; i < NR_GIC_CPU_IF; i++)
 			gic_cpu_map[i] = 0xff;
-#ifdef CONFIG_SMP
-		set_smp_cross_call(gic_raise_softirq);
-#endif
 		cpuhp_setup_state_nocalls(CPUHP_AP_IRQ_GIC_STARTING,
 					  "AP_IRQ_GIC_STARTING",
 					  gic_starting_cpu, NULL);

@@ -1207,8 +1204,20 @@ static int __init __gic_init_bases(struct gic_chip_data *gic,
 	}
 
 	ret = gic_init_bases(gic, irq_start, handle);
-	if (ret)
+	if (ret) {

Hi Russell,

Quoted 64 lines · Russell King - ARM Linux

Following on from the previous patch, I think this makes more sense to determine whether we can support IRQ work interrupts.

Whether we can support them or not depends on two things:

(a) whether the kernel has support for receiving IPIs
(b) whether it's possible to send an IPI to CPUs including the raising CPU.

(a) is a function of how the kernel is built - and in the case of ARM, it depends whether the kernel is built with SMP enabled or not.
(b) is a property of the interrupt controller.

It hasn't ever been a function of the CPU or architecture.

Commit 059e232089e4 ("irqchip/gic: Allow self-SGIs for SMP on UP configurations") changes the GIC IPI code such that we can raise SGIs on uniprocessor systems running on a SMP kernel, which means we can support IRQ work interrupts here as well.

So, we shouldn't be using cpu_smp() (or its previous is_smp() here at all. Use a flag to indicate whether we can IPI and use that to indicate whether we support irq work interrupts.

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

---

 arch/arm/include/asm/irq_work.h | 11 +++++++++--
 arch/arm/kernel/irq.c           |  0
 arch/arm/kernel/smp.c           |  3 +++
 drivers/irqchip/irq-gic.c       | 17 +++++++++++++----
 4 files changed, 25 insertions(+), 6 deletions(-)

arch/arm/include/asm/irq_work.h +9 -2

index 2dc8d7995b48..d7262a3c2f2e 100644
@@ -1,11 +1,18 @@
 #ifndef __ASM_ARM_IRQ_WORK_H
 #define __ASM_ARM_IRQ_WORK_H
 
-#include <asm/smp_plat.h>
+extern bool irq_controller_can_ipi;
+#define irq_controller_can_ipi irq_controller_can_ipi
 
 static inline bool arch_irq_work_has_interrupt(void)
 {
-	return cpu_smp();
+#ifdef CONFIG_SMP
+	/* This depends on the IRQ controller */
+	return irq_controller_can_ipi;
+#else
+	/* The kernel is not built to support IPIs */
+	return false;
+#endif
 }
 
 #endif /* _ASM_ARM_IRQ_WORK_H */

arch/arm/kernel/smp.c +0 -0

index 7dd14e8395e6..1fa9412cc4aa 100644
@@ -473,6 +473,9 @@ void __init set_smp_cross_call(void (*fn)(const struct cpumask *, unsigned int))
 		__smp_cross_call = fn;
 }
 

arch/arm/kernel/smp.c void __init set_smp_cross_call(void (*fn)(const struct cpumask *, unsigned int)) · line 477

Quoted 2 lines

arch/arm/kernel/smp.c +2 -0

+/* This indicates whether the IRQ controller can IPI (including self-IPI) */
+bool irq_controller_can_ipi;

We probably need to initialize this to false, since we have at least 4 other users of set_smp_cross_call() in the tree.

Quoted 17 lines

drivers/irqchip/irq-gic.c +0 -3

index d6c404b3584d..abe8d5807c0f 100644
@@ -1187,9 +1187,6 @@ static int __init __gic_init_bases(struct gic_chip_data *gic,
 		 */
 		for (i = 0; i < NR_GIC_CPU_IF; i++)
 			gic_cpu_map[i] = 0xff;
-#ifdef CONFIG_SMP
-		set_smp_cross_call(gic_raise_softirq);
-#endif
 		cpuhp_setup_state_nocalls(CPUHP_AP_IRQ_GIC_STARTING,
 					  "AP_IRQ_GIC_STARTING",
 					  gic_starting_cpu, NULL);

@@ -1207,8 +1204,20 @@ static int __init __gic_init_bases(struct gic_chip_data *gic,
 	}
 

drivers/irqchip/irq-gic.c static int __init __gic_init_bases(struct gic_chip_data *gic, · line 1217

Quoted 13 lines

drivers/irqchip/irq-gic.c +10 -1

 	ret = gic_init_bases(gic, irq_start, handle);
-	if (ret)
+	if (ret) {
 		kfree(name);
+		return ret;
+	}
+
+	if (gic == &gic_data[0]) {
+#ifdef CONFIG_SMP
+		set_smp_cross_call(gic_raise_softirq);
+#ifdef irq_controller_can_ipi
+		if (nr_cpu_ids == 1 || hweight8(gic_cpu_map[0]) == 1)
+			irq_controller_can_ipi = true;

Am I missing something, or is there any sane configuration where this isn't true?

Also, maybe it would make some sense to have a more streamlined interface to the architecture code. Something along the lines of:

arch/arm/include/asm/smp.h +10 -0

index 3d6dc8b..45612d2 100644
@@ -48,6 +48,16 @@ extern void smp_init_cpus(void);
  */
 extern void set_smp_cross_call(void (*)(const struct cpumask *, unsigned int));
 
+#ifdef CONFIG_SMP
+#define setup_smp_ipi(f,i)			\
+	do {					\
+		set_smp_cross_call(f);		\
+		irq_controller_can_ipi = (i);	\
+	} while(0)
+#else
+#define setup_smp_ipi(f,i)	do { } while (0)
+#endif
+
 /*
  * Called from platform specific assembly code, this is the
  * secondary CPU entry point.

with the similar entry point for arm64?

arch/arm/include/asm/smp.h +10 -0

index 3d6dc8b..45612d2 100644
@@ -48,6 +48,16 @@ extern void smp_init_cpus(void);
  */
 extern void set_smp_cross_call(void (*)(const struct cpumask *, unsigned int));
 
+#ifdef CONFIG_SMP
+#define setup_smp_ipi(f,i)			\
+	do {					\
+		set_smp_cross_call(f);		\
+		irq_controller_can_ipi = (i);	\
+	} while(0)
+#else
+#define setup_smp_ipi(f,i)	do { } while (0)
+#endif
+
 /*
  * Called from platform specific assembly code, this is the
  * secondary CPU entry point.

with the similar entry point for arm64?