[PATCH v10 03/19] arm: fiq: Replace default FIQ handler
From: Daniel Thompson <hidden>
Date: 2014-09-02 11:49:20
Also in:
lkml
Subsystem:
arm generic interrupt controller drivers, arm port, irqchip drivers, the rest · Maintainers:
Marc Zyngier, Russell King, Thomas Gleixner, Linus Torvalds
On 28/08/14 16:01, Russell King - ARM Linux wrote:
On Tue, Aug 19, 2014 at 07:12:07PM +0100, Daniel Thompson wrote:quoted
On 19/08/14 18:37, Russell King - ARM Linux wrote:quoted
On Tue, Aug 19, 2014 at 05:45:53PM +0100, Daniel Thompson wrote:quoted
+int register_fiq_nmi_notifier(struct notifier_block *nb) +{ + return atomic_notifier_chain_register(&fiq_nmi_chain, nb); +} + +asmlinkage void __exception_irq_entry fiq_nmi_handler(struct pt_regs *regs) +{ + struct pt_regs *old_regs = set_irq_regs(regs); + + nmi_enter(); + atomic_notifier_call_chain(&fiq_nmi_chain, (unsigned long)regs, NULL); + nmi_exit(); + set_irq_regs(old_regs); +}Really not happy with this. What happens if a FIQ occurs while we're inside register_fiq_nmi_notifier() - more specifically inside atomic_notifier_chain_register() ?Should depend on which side of the rcu update we're on.I just asked Paul McKenney, our RCU expert... essentially, yes, RCU stuff itself is safe in this context. However, RCU stuff can call into lockdep if lockdep is configured, and there are questions over lockdep. There's some things which can be done to reduce the lockdep exposure to it, such as ensuring that rcu_read_lock() is first called outside of FIQ context. There's concerns with whether either printk() in check_flags() could be reached too (flags there should always indicate that IRQs were disabled, so that reduces down to a question about just the first printk() there.) There's also the very_verbose() stuff for RCU lockdep classes which Paul says must not be enabled. Lastly, Paul isn't a lockdep expert, but he sees nothing that prevents lockdep doing the deadlock checking as a result of the above call. So... this coupled with my feeling that notifiers make it too easy for unreviewed code to be hooked into this path, I'm fairly sure that we don't want to be calling atomic notifier chains from FIQ context.
Having esablished (elsewhere in the thread) that RCU usage is safe from FIQ I have been working on the assumption that your feeling regarding unreviewed code is sufficient on its own to avoid using notifiers (and also to avoid a list of function pointers like on x86). Therefore I have made the changes requested and produced a before/after patch to show the impact of this. I will merge this into the FIQ patchset shortly (I need to run a few more build tests first). Personally I still favour using notifiers and think the coupling below is excessive. Nevertheless I've run a couple of basic tests on the code below and it works fine.
diff --git a/arch/arm/include/asm/fiq.h b/arch/arm/include/asm/fiq.h
index 175bfed..a25c952 100644
--- a/arch/arm/include/asm/fiq.h
+++ b/arch/arm/include/asm/fiq.h@@ -54,7 +54,6 @@ extern void disable_fiq(int fiq); extern int ack_fiq(int fiq); extern void eoi_fiq(int fiq); extern bool has_fiq(int fiq); -extern int register_fiq_nmi_notifier(struct notifier_block *nb); extern void fiq_register_mapping(int irq, struct fiq_chip *chip); /* helpers defined in fiqasm.S: */
diff --git a/arch/arm/include/asm/kgdb.h b/arch/arm/include/asm/kgdb.h
index 6563da0..cb5ccd6 100644
--- a/arch/arm/include/asm/kgdb.h
+++ b/arch/arm/include/asm/kgdb.h@@ -51,6 +51,7 @@ extern void kgdb_handle_bus_error(void); extern int kgdb_fault_expected; extern int kgdb_register_fiq(unsigned int fiq); +extern void kgdb_handle_fiq(struct pt_regs *regs); #endif /* !__ASSEMBLY__ */
diff --git a/arch/arm/kernel/fiq.c b/arch/arm/kernel/fiq.c
index b2bd1c7..7422b58 100644
--- a/arch/arm/kernel/fiq.c
+++ b/arch/arm/kernel/fiq.c@@ -43,12 +43,14 @@ #include <linux/irq.h> #include <linux/radix-tree.h> #include <linux/slab.h> +#include <linux/irqchip/arm-gic.h> #include <asm/cacheflush.h> #include <asm/cp15.h> #include <asm/exception.h> #include <asm/fiq.h> #include <asm/irq.h> +#include <asm/kgdb.h> #include <asm/traps.h> #define FIQ_OFFSET ({ \
@@ -65,7 +67,6 @@ static unsigned long no_fiq_insn; static int fiq_start = -1; static RADIX_TREE(fiq_data_tree, GFP_KERNEL); static DEFINE_MUTEX(fiq_data_mutex); -static ATOMIC_NOTIFIER_HEAD(fiq_nmi_chain); /* Default reacquire function * - we always relinquish FIQ control
@@ -218,17 +219,23 @@ bool has_fiq(int fiq) } EXPORT_SYMBOL(has_fiq); -int register_fiq_nmi_notifier(struct notifier_block *nb) -{ - return atomic_notifier_chain_register(&fiq_nmi_chain, nb); -} - asmlinkage void __exception_irq_entry fiq_nmi_handler(struct pt_regs *regs) { struct pt_regs *old_regs = set_irq_regs(regs); nmi_enter(); - atomic_notifier_call_chain(&fiq_nmi_chain, (unsigned long)regs, NULL); + + /* these callbacks deliberately avoid using a notifier chain in + * order to ensure code review happens (drivers cannot "secretly" + * employ FIQ without modifying this chain of calls). + */ +#ifdef CONFIG_KGDB_FIQ + kgdb_handle_fiq(regs); +#endif +#ifdef CONFIG_ARM_GIC + gic_handle_fiq_ipi(); +#endif + nmi_exit(); set_irq_regs(old_regs); }
diff --git a/arch/arm/kernel/kgdb.c b/arch/arm/kernel/kgdb.c
index b77b885..630a3ef 100644
--- a/arch/arm/kernel/kgdb.c
+++ b/arch/arm/kernel/kgdb.c@@ -312,12 +312,13 @@ struct kgdb_arch arch_kgdb_ops = { }; #ifdef CONFIG_KGDB_FIQ -static int kgdb_handle_fiq(struct notifier_block *nb, unsigned long arg, - void *data) +void kgdb_handle_fiq(struct pt_regs *regs) { - struct pt_regs *regs = (void *) arg; int actual; + if (!kgdb_fiq) + return; + if (!kgdb_nmicallback(raw_smp_processor_id(), regs)) return NOTIFY_OK;
@@ -333,11 +334,6 @@ static int kgdb_handle_fiq(struct notifier_block *nb, unsigned long arg, return NOTIFY_OK; } -static struct notifier_block kgdb_fiq_notifier = { - .notifier_call = kgdb_handle_fiq, - .priority = 100, -}; - int kgdb_register_fiq(unsigned int fiq) { static struct fiq_handler kgdb_fiq_desc = { .name = "kgdb", };
@@ -357,7 +353,6 @@ int kgdb_register_fiq(unsigned int fiq) } kgdb_fiq = fiq; - register_fiq_nmi_notifier(&kgdb_fiq_notifier); return 0; }
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index bda5a91..8821160 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c@@ -502,13 +502,17 @@ static void __init gic_init_fiq(struct gic_chip_data *gic, /* * Fully acknowledge (both ack and eoi) a FIQ-based IPI */ -static int gic_handle_fiq_ipi(struct notifier_block *nb, unsigned long regs, - void *data) +void gic_handle_fiq_ipi(void) { struct gic_chip_data *gic = &gic_data[0]; - void __iomem *cpu_base = gic_data_cpu_base(gic); + void __iomem *cpu_base; unsigned long irqstat, irqnr; + if (!gic || !gic->fiq_enable) + return; + + cpu_base = gic_data_cpu_base(gic); + if (WARN_ON(!in_nmi())) return NOTIFY_BAD;
@@ -525,13 +529,6 @@ static int gic_handle_fiq_ipi(struct notifier_block *nb, unsigned long regs, return NOTIFY_OK; } - -/* - * Notifier to ensure IPI FIQ is acknowledged correctly. - */ -static struct notifier_block gic_fiq_ipi_notifier = { - .notifier_call = gic_handle_fiq_ipi, -}; #else /* CONFIG_FIQ */ static inline void gic_set_group_irq(void __iomem *base, unsigned int hwirq, int group) {}
@@ -1250,10 +1247,6 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start, #ifdef CONFIG_SMP set_smp_cross_call(gic_raise_softirq); register_cpu_notifier(&gic_cpu_notifier); -#ifdef CONFIG_FIQ - if (gic_data_fiq_enable(gic)) - register_fiq_nmi_notifier(&gic_fiq_ipi_notifier); -#endif #endif set_handle_irq(gic_handle_irq); }
diff --git a/include/linux/irqchip/arm-gic.h b/include/linux/irqchip/arm-gic.h
index 45e2d8c..52a5676 100644
--- a/include/linux/irqchip/arm-gic.h
+++ b/include/linux/irqchip/arm-gic.h@@ -101,5 +101,8 @@ static inline void __init register_routable_domain_ops { gic_routable_irq_domain_ops = ops; } + +void gic_handle_fiq_ipi(void); + #endif /* __ASSEMBLY */ #endif