[PATCH 2/2] ARM: rename is_smp()

[PATCH 2/2] ARM: rename is_smp()

[Russell King]

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 <rmk+kernel@armlinux.org.uk>
---
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(-)

diff --git a/arch/arm/include/asm/cputype.h b/arch/arm/include/asm/cputype.h
index 522b5feb4eaa..8c82e6b4961d 100644
--- a/arch/arm/include/asm/cputype.h
+++ b/arch/arm/include/asm/cputype.h
@@ -109,7 +109,7 @@ extern unsigned int processor_id;
 
 /*
  * The memory clobber prevents gcc 4.5 from reordering the mrc before
- * any is_smp() tests, which can cause undefined instruction aborts on
+ * any cpu_smp() tests, which can cause undefined instruction aborts on
  * ARM1136 r0 due to the missing extended CP15 registers.
  */
 #define read_cpuid_ext(ext_reg)						\
diff --git a/arch/arm/include/asm/irq_work.h b/arch/arm/include/asm/irq_work.h
index 712d03e5973a..2dc8d7995b48 100644
--- a/arch/arm/include/asm/irq_work.h
+++ b/arch/arm/include/asm/irq_work.h
@@ -5,7 +5,7 @@
 
 static inline bool arch_irq_work_has_interrupt(void)
 {
-	return is_smp();
+	return cpu_smp();
 }
 
 #endif /* _ASM_ARM_IRQ_WORK_H */
diff --git a/arch/arm/include/asm/smp_plat.h b/arch/arm/include/asm/smp_plat.h
index 43f246b73ce7..bdba301d01e4 100644
--- a/arch/arm/include/asm/smp_plat.h
+++ b/arch/arm/include/asm/smp_plat.h
@@ -12,9 +12,10 @@
 #include <asm/cputype.h>
 
 /*
- * Return true if we are running on a SMP platform
+ * Return true if we are running on a CPU which supports SMP, and the
+ * kernel supports SMP.
  */
-static inline bool is_smp(void)
+static inline bool cpu_smp(void)
 {
 #ifndef CONFIG_SMP
 	return false;
@@ -43,7 +44,8 @@ static inline unsigned int smp_cpuid_part(int cpu)
 #else
 static inline int tlb_ops_need_broadcast(void)
 {
-	if (!is_smp())
+	/* Non-SMP CPUs don't need to check for broadcast */
+	if (!cpu_smp())
 		return 0;
 
 	return ((read_cpuid_ext(CPUID_EXT_MMFR3) >> 12) & 0xf) < 2;
@@ -55,7 +57,8 @@ static inline int tlb_ops_need_broadcast(void)
 #else
 static inline int cache_ops_need_broadcast(void)
 {
-	if (!is_smp())
+	/* Non-SMP CPUs don't need to check for broadcast */
+	if (!cpu_smp())
 		return 0;
 
 	return ((read_cpuid_ext(CPUID_EXT_MMFR3) >> 12) & 0xf) < 1;
diff --git a/arch/arm/kernel/devtree.c b/arch/arm/kernel/devtree.c
index f676febbb270..19a9653df6d2 100644
--- a/arch/arm/kernel/devtree.c
+++ b/arch/arm/kernel/devtree.c
@@ -78,7 +78,7 @@ void __init arm_dt_init_cpu_maps(void)
 	struct device_node *cpu, *cpus;
 	int found_method = 0;
 	u32 i, j, cpuidx = 1;
-	u32 mpidr = is_smp() ? read_cpuid_mpidr() & MPIDR_HWID_BITMASK : 0;
+	u32 mpidr = cpu_smp() ? read_cpuid_mpidr() & MPIDR_HWID_BITMASK : 0;
 
 	u32 tmp_map[NR_CPUS] = { [0 ... NR_CPUS-1] = MPIDR_INVALID };
 	bool bootcpu_valid = false;
diff --git a/arch/arm/kernel/module.c b/arch/arm/kernel/module.c
index 4f14b5ce6535..0b49aa426180 100644
--- a/arch/arm/kernel/module.c
+++ b/arch/arm/kernel/module.c
@@ -374,12 +374,20 @@ int module_finalize(const Elf32_Ehdr *hdr, const Elf_Shdr *sechdrs,
 		fixup_pv_table((void *)s->sh_addr, s->sh_size);
 #endif
 	s = find_mod_section(hdr, sechdrs, ".alt.smp.init");
-	if (s && !is_smp())
+	if (s && !cpu_smp()) {
+		/*
+		 * Modules running on non-SMP capable CPUs must not use SMP
+		 * instructions, as they may cause undefined instruction
+		 * exceptions.  This means we have to fix up the SMP
+		 * alternatives on SMP-on-UP modules, and are unable to load
+		 * modules built for SMP-only configurations.
+		 */
 #ifdef CONFIG_SMP_ON_UP
 		fixup_smp((void *)s->sh_addr, s->sh_size);
 #else
 		return -EINVAL;
 #endif
+	}
 	return 0;
 }
 
diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
index 80f45b01fbaa..cdf71941c129 100644
--- a/arch/arm/kernel/setup.c
+++ b/arch/arm/kernel/setup.c
@@ -584,7 +584,7 @@ u32 __cpu_logical_map[NR_CPUS] = { [0 ... NR_CPUS-1] = MPIDR_INVALID };
 void __init smp_setup_processor_id(void)
 {
 	int i;
-	u32 mpidr = is_smp() ? read_cpuid_mpidr() & MPIDR_HWID_BITMASK : 0;
+	u32 mpidr = cpu_smp() ? read_cpuid_mpidr() & MPIDR_HWID_BITMASK : 0;
 	u32 cpu = MPIDR_AFFINITY_LEVEL(mpidr, 0);
 
 	cpu_logical_map(0) = cpu;
@@ -1112,7 +1112,8 @@ void __init setup_arch(char **cmdline_p)
 	arm_dt_init_cpu_maps();
 	psci_dt_init();
 #ifdef CONFIG_SMP
-	if (is_smp()) {
+	if (cpu_smp()) {
+		/* Ignore SMP on non-SMP capable CPUs */
 		if (!mdesc->smp_init || !mdesc->smp_init()) {
 			if (psci_smp_available())
 				smp_set_ops(&psci_smp_ops);
@@ -1124,7 +1125,7 @@ void __init setup_arch(char **cmdline_p)
 	}
 #endif
 
-	if (!is_smp())
+	if (!cpu_smp())
 		hyp_mode_check();
 
 	reserve_crashkernel();
diff --git a/arch/arm/mach-mvebu/coherency.c b/arch/arm/mach-mvebu/coherency.c
index ae2a018b9305..fe4b1e15ebb8 100644
--- a/arch/arm/mach-mvebu/coherency.c
+++ b/arch/arm/mach-mvebu/coherency.c
@@ -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
@@ -226,7 +226,7 @@ static int coherency_type(void)
 	 * where we don't know yet on which SoC we are running.
 
 	 */
-	if (!is_smp())
+	if (!cpu_smp())
 		return COHERENCY_FABRIC_TYPE_NONE;
 
 	np = of_find_matching_node_and_match(NULL, of_coherency_table, &match);
diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
index 4001dd15818d..d3dc758d5391 100644
--- a/arch/arm/mm/mmu.c
+++ b/arch/arm/mm/mmu.c
@@ -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;
-- 
2.7.4

[PATCH 2/2] ARM: rename is_smp()

[Gregory CLEMENT]

Hi Russell,
 
 On lun., nov. 14 2016, Russell King <rmk+kernel@armlinux.org.uk> wrote:

> 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 <rmk+kernel@armlinux.org.uk>
> ---
> 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(-)

[...]

> diff --git a/arch/arm/mach-mvebu/coherency.c b/arch/arm/mach-mvebu/coherency.c
> index ae2a018b9305..fe4b1e15ebb8 100644
> --- a/arch/arm/mach-mvebu/coherency.c
> +++ b/arch/arm/mach-mvebu/coherency.c
> @@ -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
> @@ -226,7 +226,7 @@ static int coherency_type(void)
>  	 * where we don't know yet on which SoC we are running.
>  
>  	 */
> -	if (!is_smp())
> +	if (!cpu_smp())
>  		return COHERENCY_FABRIC_TYPE_NONE;
>  
>  	np = of_find_matching_node_and_match(NULL, of_coherency_table, &match);

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.

So, for this chunk:

Acked-by: Gregory CLEMENT <gregory.clement@free-electrons.com>

Thanks,

Gregory


> diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
> index 4001dd15818d..d3dc758d5391 100644
> --- a/arch/arm/mm/mmu.c
> +++ b/arch/arm/mm/mmu.c
> @@ -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;
> -- 
> 2.7.4
>

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 2/2] ARM: rename is_smp()

[Russell King - ARM Linux]

On Mon, Nov 14, 2016 at 02:24:43PM +0100, Gregory CLEMENT wrote:
> 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.

That's correct - it's an exercise in renaming to a clearer name.

Thanks for the ack.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH RFC 3/2] ARM: improve arch_irq_work_has_interrupt()

[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 <rmk+kernel@armlinux.org.uk>
--- 
 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(-)

diff --git a/arch/arm/include/asm/irq_work.h b/arch/arm/include/asm/irq_work.h
index 2dc8d7995b48..d7262a3c2f2e 100644
--- a/arch/arm/include/asm/irq_work.h
+++ b/arch/arm/include/asm/irq_work.h
@@ -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 */
diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index 7dd14e8395e6..1fa9412cc4aa 100644
--- a/arch/arm/kernel/smp.c
+++ b/arch/arm/kernel/smp.c
@@ -473,6 +473,9 @@ void __init set_smp_cross_call(void (*fn)(const struct cpumask *, unsigned int))
 		__smp_cross_call = fn;
 }
 
+/* This indicates whether the IRQ controller can IPI (including self-IPI) */
+bool irq_controller_can_ipi;
+
 static const char *ipi_types[NR_IPI] __tracepoint_string = {
 #define S(x,s)	[x] = s
 	S(IPI_WAKEUP, "CPU wakeup interrupts"),
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index d6c404b3584d..abe8d5807c0f 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -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) {
 		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;
+#endif
+#endif
+	}
 
 	return ret;
 }

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently@9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH RFC 3/2] ARM: improve arch_irq_work_has_interrupt()

[Marc Zyngier]

Hi Russell,

On 14/11/16 15:36, Russell King - ARM Linux wrote:
> 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 <rmk+kernel@armlinux.org.uk>
> --- 
>  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(-)
> 
> diff --git a/arch/arm/include/asm/irq_work.h b/arch/arm/include/asm/irq_work.h
> index 2dc8d7995b48..d7262a3c2f2e 100644
> --- a/arch/arm/include/asm/irq_work.h
> +++ b/arch/arm/include/asm/irq_work.h
> @@ -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 */
> diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
> index 7dd14e8395e6..1fa9412cc4aa 100644
> --- a/arch/arm/kernel/smp.c
> +++ b/arch/arm/kernel/smp.c
> @@ -473,6 +473,9 @@ void __init set_smp_cross_call(void (*fn)(const struct cpumask *, unsigned int))
>  		__smp_cross_call = fn;
>  }
>  
> +/* 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.

> +
>  static const char *ipi_types[NR_IPI] __tracepoint_string = {
>  #define S(x,s)	[x] = s
>  	S(IPI_WAKEUP, "CPU wakeup interrupts"),
> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index d6c404b3584d..abe8d5807c0f 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -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) {
>  		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:

diff --git a/arch/arm/include/asm/smp.h b/arch/arm/include/asm/smp.h
index 3d6dc8b..45612d2 100644
--- a/arch/arm/include/asm/smp.h
+++ b/arch/arm/include/asm/smp.h
@@ -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?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH RFC 3/2] ARM: improve arch_irq_work_has_interrupt()

[Russell King - ARM Linux]

On Mon, Nov 14, 2016 at 04:30:57PM +0000, Marc Zyngier wrote:
> Hi Russell,
> 
> On 14/11/16 15:36, Russell King - ARM Linux wrote:
> > 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 <rmk+kernel@armlinux.org.uk>
> > --- 
> >  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(-)
> > 
> > diff --git a/arch/arm/include/asm/irq_work.h b/arch/arm/include/asm/irq_work.h
> > index 2dc8d7995b48..d7262a3c2f2e 100644
> > --- a/arch/arm/include/asm/irq_work.h
> > +++ b/arch/arm/include/asm/irq_work.h
> > @@ -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 */
> > diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
> > index 7dd14e8395e6..1fa9412cc4aa 100644
> > --- a/arch/arm/kernel/smp.c
> > +++ b/arch/arm/kernel/smp.c
> > @@ -473,6 +473,9 @@ void __init set_smp_cross_call(void (*fn)(const struct cpumask *, unsigned int))
> >  		__smp_cross_call = fn;
> >  }
> >  
> > +/* 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.

C programming 101: BSS variables are initialised to zero at the start
of the program.

> > diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> > index d6c404b3584d..abe8d5807c0f 100644
> > --- a/drivers/irqchip/irq-gic.c
> > +++ b/drivers/irqchip/irq-gic.c
> > @@ -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) {
> >  		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?

I hope not, but I want to duplicate here the conditions where
gic_raise_softirq() actually _works_ so we stop running into corner
cases where we get "irq work fails" etc.

> Also, maybe it would make some sense to have a more 
> streamlined interface to the architecture code. Something along the 
> lines of:
> 
> diff --git a/arch/arm/include/asm/smp.h b/arch/arm/include/asm/smp.h
> index 3d6dc8b..45612d2 100644
> --- a/arch/arm/include/asm/smp.h
> +++ b/arch/arm/include/asm/smp.h
> @@ -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?

I'd prefer to keep the two things separate, but we should definitely
provide a stub for set_smp_cross_call() for when SMP is disabled.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH RFC 3/2] ARM: improve arch_irq_work_has_interrupt()

[Russell King - ARM Linux]

On Mon, Nov 14, 2016 at 04:30:57PM +0000, Marc Zyngier wrote:
> diff --git a/arch/arm/include/asm/smp.h b/arch/arm/include/asm/smp.h
> index 3d6dc8b..45612d2 100644
> --- a/arch/arm/include/asm/smp.h
> +++ b/arch/arm/include/asm/smp.h
> @@ -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?

Note that asm/smp.h is not included if CONFIG_SMP is not set, so it's
not that easy.

Both ARM64 and ARM have asm/smp_plat.h which contains the platform
specifics of SMP, which is where set_smp_cross_call() really should
be anyway, and the GIC code already includes this.  So I'm moving
set_smp_cross_call() there for both arches.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.