Messages in this thread | | | Date | Tue, 2 Sep 2014 07:23:14 -0700 | From | "Paul E. McKenney" <> | Subject | Re: [PATCH v10 03/19] arm: fiq: Replace default FIQ handler |
| |
On Tue, Sep 02, 2014 at 12:49:16PM +0100, Daniel Thompson wrote: > On 28/08/14 16:01, Russell King - ARM Linux wrote: > > On Tue, Aug 19, 2014 at 07:12:07PM +0100, Daniel Thompson wrote: > >> On 19/08/14 18:37, Russell King - ARM Linux wrote: > >>> On Tue, Aug 19, 2014 at 05:45:53PM +0100, Daniel Thompson wrote: > >>>> +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).
There was a later clarification from a lockdep expert showing that the code was in fact safe, so the notifier approach should be just fine.
Thanx, Paul
> 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 > >
| |