Messages in this thread Patch in this message | | | Date | Mon, 28 Apr 2008 18:10:30 +0200 (CEST) | From | Thomas Gleixner <> | Subject | Re: [PATCH] let setup_irq reenable a shared irq |
| |
On Mon, 28 Apr 2008, Uwe Kleine-König wrote: > > Oh no. There is lots of code in drivers, which does: > > > > disable_irq(); > > do_some_protected_stuff(); > > enable_irq(); > > > > So when the second driver is loaded on another CPU it would see the > > IRQ_DISABLED bit set and unconditionally reenable the interrupt. > > > > This unprotects the protected operation and definitely triggers the > > WARN_ON() in enable_irq() where we check for desc->depth == 0. > mmpf. > > It's not nice to use disable_irq()/enable_irq() in a driver, is it?
Well, it's not nice, but it's there (in rather large quantities)
> > Waht kind of scenario/devices do you have which trigger this ? > > It's a ns9215 SoC. The rtc component has a clock flag that I need to > set before accessing the address space. After enabling the clock flag > I get an irq if the rtc is up. The rtc itself uses the same irq. As I > want to handle the clk enabling in platform code and the actual rtc in a > driver I need a shared irq. > Now the problem is that the rtc remembers its alarm flags over > reboots[1] and starts asserting the irq when the platform code has > requested the irq. > > I currently see two ways to handle that: > 1) find an alternative patch similar to the one I sent that don't break > driver code; or > 2) let the platform code disable the rtc's irqs. > > for 1) you need to remember the reason for disabling the irq (or fix all > drivers not to use disable_irq()). And 2) is ugly because I have to > ioremap then.
1) makes a lot of sense, as we can see those stupid device/bios keeps irq asserted problems once in a while. Giving the irq a new chance when a new driver arrives is definitely not a bad idea. See patch below.
Reworking all the drivers would be a nice but definitely non trivial project and it needs the acceptance of all the affected subsystem maintainers.
2) ioremap is not a really big deal, so that's not a good argument not to put hardware back into the state which was found when we started the box, especially when we know that it does stupid things eventually.
Thanks, tglx
------------>
Subject: genirq: reenable a nobody cared disabled irq when a new driver arrives From: Thomas Gleixner <tglx@linutronix.de> Date: Mon, 28 Apr 2008 17:01:56 +0200
Uwe Kleine-Koenig has some strange hardware where one of the shared interrupts can be asserted during boot before the appropriate driver loads. Requesting the shared irq line from another driver results in a spurious interrupt storm which finally disables the interrupt line.
I have seen similar behaviour on resume before (the hardware does not work anymore so I can not verify) and this spurious irq issue is raised on a regular base in bugreports.
Change the spurious disable logic to increment the disable depth and mark the interrupt with an extra flag which allows us to reenable the interrupt when a new driver arrives which requests the same irq line. In the worst case this will disable the irq again via the spurious trap, but there is a decent chance that the new driver is the one which can handle the already asserted interrupt and makes the box usable again.
Signed-off-by: Thomas Gleixner <tglx@linutronix.de> Acked-by: Ingo Molnar <mingo@elte.hu> --- include/linux/irq.h | 1 + kernel/irq/manage.c | 49 ++++++++++++++++++++++++++++++++----------------- kernel/irq/spurious.c | 4 ++-- 3 files changed, 35 insertions(+), 19 deletions(-)
Index: linux-2.6/include/linux/irq.h =================================================================== --- linux-2.6.orig/include/linux/irq.h +++ linux-2.6/include/linux/irq.h @@ -61,6 +61,7 @@ typedef void (*irq_flow_handler_t)(unsig #define IRQ_WAKEUP 0x00100000 /* IRQ triggers system wakeup */ #define IRQ_MOVE_PENDING 0x00200000 /* need to re-target IRQ destination */ #define IRQ_NO_BALANCING 0x00400000 /* IRQ is excluded from balancing */ +#define IRQ_SPURIOUS_DISABLED 0x00400000 /* IRQ was disabled by the spurious trap */ #ifdef CONFIG_IRQ_PER_CPU # define CHECK_IRQ_PER_CPU(var) ((var) & IRQ_PER_CPU) Index: linux-2.6/kernel/irq/manage.c =================================================================== --- linux-2.6.orig/kernel/irq/manage.c +++ linux-2.6/kernel/irq/manage.c @@ -149,6 +149,26 @@ void disable_irq(unsigned int irq) } EXPORT_SYMBOL(disable_irq); +static void __enable_irq(struct irq_desc *desc, unsigned int irq) +{ + switch (desc->depth) { + case 0: + printk(KERN_WARNING "Unbalanced enable for IRQ %d\n", irq); + WARN_ON(1); + break; + case 1: { + unsigned int status = desc->status & ~IRQ_DISABLED; + + /* Prevent probing on this irq: */ + desc->status = status | IRQ_NOPROBE; + check_irq_resend(desc, irq); + /* fall-through */ + } + default: + desc->depth--; + } +} + /** * enable_irq - enable handling of an irq * @irq: Interrupt to enable @@ -168,22 +188,7 @@ void enable_irq(unsigned int irq) return; spin_lock_irqsave(&desc->lock, flags); - switch (desc->depth) { - case 0: - printk(KERN_WARNING "Unbalanced enable for IRQ %d\n", irq); - WARN_ON(1); - break; - case 1: { - unsigned int status = desc->status & ~IRQ_DISABLED; - - /* Prevent probing on this irq: */ - desc->status = status | IRQ_NOPROBE; - check_irq_resend(desc, irq); - /* fall-through */ - } - default: - desc->depth--; - } + __enable_irq(desc, irq); spin_unlock_irqrestore(&desc->lock, flags); } EXPORT_SYMBOL(enable_irq); @@ -364,7 +369,7 @@ int setup_irq(unsigned int irq, struct i compat_irq_chip_set_default_handler(desc); desc->status &= ~(IRQ_AUTODETECT | IRQ_WAITING | - IRQ_INPROGRESS); + IRQ_INPROGRESS | IRQ_SPURIOUS_DISABLED); if (!(desc->status & IRQ_NOAUTOEN)) { desc->depth = 0; @@ -380,6 +385,16 @@ int setup_irq(unsigned int irq, struct i /* Reset broken irq detection when installing new handler */ desc->irq_count = 0; desc->irqs_unhandled = 0; + + /* + * Check whether we disabled the irq via the spurious handler + * before. Reenable it and give it another chance. + */ + if (shared && (desc->status & IRQ_SPURIOUS_DISABLED)) { + desc->status &= ~IRQ_SPURIOUS_DISABLED; + __enable_irq(desc, irq); + } + spin_unlock_irqrestore(&desc->lock, flags); new->irq = irq; Index: linux-2.6/kernel/irq/spurious.c =================================================================== --- linux-2.6.orig/kernel/irq/spurious.c +++ linux-2.6/kernel/irq/spurious.c @@ -209,8 +209,8 @@ void note_interrupt(unsigned int irq, st * Now kill the IRQ */ printk(KERN_EMERG "Disabling IRQ #%d\n", irq); - desc->status |= IRQ_DISABLED; - desc->depth = 1; + desc->status |= IRQ_DISABLED | IRQ_SPURIOUS_DISABLED; + desc->depth++; desc->chip->disable(irq); } desc->irqs_unhandled = 0; | |