| Message ID | 1400853478-5824-2-git-send-email-daniel.thompson@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Fri, May 23, 2014 at 02:57:49PM +0100, Daniel Thompson wrote: > +void eoi_fiq(int fiq) > +{ > + struct irq_data *irq_data = irq_get_irq_data(fiq + fiq_start); > + struct irq_chip *chip = irq_data_get_irq_chip(irq_data); > + > + if (chip->irq_eoi) > + chip->irq_eoi(irq_data); > +} So what if the IRQ chip takes a spinlock? You can't take spinlocks in FIQ context... Who checks for that kind of stuff - just hoping that the IRQ driver doesn't take a spinlock sounds real fragile.
On 23/05/14 16:00, Russell King - ARM Linux wrote: > On Fri, May 23, 2014 at 02:57:49PM +0100, Daniel Thompson wrote: >> +void eoi_fiq(int fiq) >> +{ >> + struct irq_data *irq_data = irq_get_irq_data(fiq + fiq_start); >> + struct irq_chip *chip = irq_data_get_irq_chip(irq_data); >> + >> + if (chip->irq_eoi) >> + chip->irq_eoi(irq_data); >> +} > > So what if the IRQ chip takes a spinlock? You can't take spinlocks in > FIQ context... Who checks for that kind of stuff - just hoping that > the IRQ driver doesn't take a spinlock sounds real fragile. I'll certainly do a better code review before pushing on. I certainly did overlook a spinlock in the GIC code which (I think) is actually reachable on a Tegra platform. I'll have to do something about that. I've spent a bit of time this week seeing whether lock dep can be provoked into triggering if it sees a spin lock in this code but that isn't turning out very well (it needs CONFIG_LOCKDEP_DEBUG, relies on lockdep believing the FIQ handling state isn't a hardirq and still doesn't work properly). Fortunately architectures using EOI appear to be pretty rare (I count five in drivers/irqchip/ and three in arch/arm/) so traditional code review and yelling might be sufficient. Daniel.
diff --git a/arch/arm/include/asm/fiq.h b/arch/arm/include/asm/fiq.h index d493d0b..5a2a9b9 100644 --- a/arch/arm/include/asm/fiq.h +++ b/arch/arm/include/asm/fiq.h @@ -38,6 +38,7 @@ extern void release_fiq(struct fiq_handler *f); extern void set_fiq_handler(void *start, unsigned int length); extern void enable_fiq(int fiq); extern void disable_fiq(int fiq); +extern void eoi_fiq(int fiq); /* helpers defined in fiqasm.S: */ extern void __set_fiq_regs(unsigned long const *regs); diff --git a/arch/arm/kernel/fiq.c b/arch/arm/kernel/fiq.c index 918875d..defbe85 100644 --- a/arch/arm/kernel/fiq.c +++ b/arch/arm/kernel/fiq.c @@ -40,6 +40,7 @@ #include <linux/init.h> #include <linux/interrupt.h> #include <linux/seq_file.h> +#include <linux/irq.h> #include <asm/cacheflush.h> #include <asm/cp15.h> @@ -139,6 +140,15 @@ void disable_fiq(int fiq) disable_irq(fiq + fiq_start); } +void eoi_fiq(int fiq) +{ + struct irq_data *irq_data = irq_get_irq_data(fiq + fiq_start); + struct irq_chip *chip = irq_data_get_irq_chip(irq_data); + + if (chip->irq_eoi) + chip->irq_eoi(irq_data); +} + EXPORT_SYMBOL(set_fiq_handler); EXPORT_SYMBOL(__set_fiq_regs); /* defined in fiqasm.S */ EXPORT_SYMBOL(__get_fiq_regs); /* defined in fiqasm.S */ @@ -146,6 +156,7 @@ EXPORT_SYMBOL(claim_fiq); EXPORT_SYMBOL(release_fiq); EXPORT_SYMBOL(enable_fiq); EXPORT_SYMBOL(disable_fiq); +EXPORT_SYMBOL(eoi_fiq); void __init init_FIQ(int start) {
Modern ARM systems require an EOI to be sent to the interrupt controller on completion of both IRQ and FIQ. The FIQ code currently does not provide any API to perform this. This patch provides this API, implemented by hooking into main irq driver in a similar way to the existing enable_fiq()/disable_fiq(). Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org> Cc: Russell King <linux@arm.linux.org.uk> Cc: Fabio Estevam <festevam@gmail.com> Cc: Nicolas Pitre <nico@linaro.org> --- arch/arm/include/asm/fiq.h | 1 + arch/arm/kernel/fiq.c | 11 +++++++++++ 2 files changed, 12 insertions(+)