xirc2ps_cs irq return fix
A message that is not in the archive (sent off-list or not yet received): [email protected]
drivers/net/pcmcia/xirc2ps_cs.c
Quoted 6 lines · Linux Kernel Mailing List
diff -Nru
a/drivers/net/pcmcia/xirc2ps_cs.cb/drivers/net/pcmcia/xirc2ps_cs.cdrivers/net/pcmcia/xirc2ps_cs.c +1
-1if (!netif_device_present (dev)) -return IRQ_NONE ; +return IRQ_HANDLED ;
As I mentioned in the thread, this piece of code is obviously wrong.
Think about how scalable this fix is?? Do you really want to crap up all pcmcia drivers with this silly -- and wrong -- check?
IIRC the pcmcia layer or new irqreturn_t was blamed for the problem. Come on. Linux mantra is -against- papering over bugs.
Jeff
Quoted 4 lines · Jeff Garzik
As I mentioned in the thread, this piece of code is obviously wrong.
Think about how scalable this fix is?? Do you really want to crap up all pcmcia drivers with this silly -- and wrong -- check?
My interpretation of it is the PCMCIA controller was triggering interrupts on exit and the link handler for the card was still installed even after the netdevice was down.
Quoted 2 lines
IIRC the pcmcia layer or new
irqreturn_twas blamed for the problem. Come on. Linux mantra is -against- papering over bugs.
I have to take responsibility for that little mess :(
Zwane
Signature · 1 line
function.linuxpower.ca
Quoted 3 lines · Zwane Mwaikambo
My interpretation of it is the PCMCIA controller was triggering interrupts on exit and the link handler for the card was still installed even after the netdevice was down.
This is exactly what will happen all the time on PCMCIA devices. The edge triggered interrupt will cause an IRQ to float around every remove of the device on most hardware
The fix is basically correct, although the odd floating IRQ ought to be cleaned up by the heuristics being fixed in the core IRQ disaster detector
Quoted 5 lines · Alan Cox
Quoted 1 line · Zwane Mwaikambo
My interpretation of it is the PCMCIA controller was triggering interrupts on exit and the link handler for the card was still installed even after the netdevice was down.
Quoted 7 lines
This is exactly what will happen all the time on PCMCIA devices. The edge triggered interrupt will cause an IRQ to float around every remove of the device on most hardware
The fix is basically correct, although the odd floating IRQ ought to be cleaned up by the heuristics being fixed in the core IRQ disaster detector
huh?
If the fix is correct, we need to do a bombing run that adds the following code to each driver,
if (!netif_device_present(dev)) return IRQ_HANDLED;
but of course the irq _wasn't_ handled by the driver. Silly, isn't it?
I still maintain it needs to be fixed elsewhere. Touching every driver for this is just not scalable, in addition to pointing the finger at the pcmcia core instead of individual drivers.
Jeff
Review
Quoted 6 lines · Jeff Garzik
If the fix is correct, we need to do a bombing run that adds the
following code to each driver,(quoted diff) +0
-0if (!netif_device_present(dev)) return IRQ_HANDLED;
no... What we need to do is to kill the printk in handle_IRQ_event().
It should only be turned on for special situations, where someone is trying to hunt down a reproducible lockup. These are situations in which the odd false positive just doesn't matter. And we know that there will always be false positives due to apic delivery latency (at least).
I think the time is right to do this. Add CONFIG_DEBUG_IRQ and get on with fixing real stuff.
Quoted 7 lines · Andrew Morton
It should only be turned on for special situations, where someone is trying to hunt down a reproducible lockup. These are situations in which the odd false positive just doesn't matter. And we know that there will always be false positives due to apic delivery latency (at least).
I think the time is right to do this. Add
CONFIG_DEBUG_IRQand get on with fixing real stuff.
I not 100% convinced. I think it should be left by default but only if you get something like a million unhandled interrupts in a row. Thats the "your box has died" case where the info is useful anyway.
Maybe the number in a row is what you want to tweak in config or /proc/sys ?
Quoted 7 lines · Alan Cox
Quoted 5 lines · Andrew Morton
It should only be turned on for special situations, where someone is trying to hunt down a reproducible lockup. These are situations in which the odd false positive just doesn't matter. And we know that there will always be false positives due to apic delivery latency (at least).
Quoted 9 lines
Quoted 2 lines
I think the time is right to do this. Add
CONFIG_DEBUG_IRQand get on with fixing real stuff.I not 100% convinced. I think it should be left by default but only if you get something like a million unhandled interrupts in a row. Thats the "your box has died" case where the info is useful anyway.
Maybe the number in a row is what you want to tweak in config or
/proc/sys?
Maybe.
The below patch has been in -mm for some time. It was supposed to kill the IRQ if 750 of the previous 1000 IRQs weren't handled.
I disabled the killing code because it was triggering on someone's works-just-fine setup.
There will be pain involved in getting all this to work right. Do you really think there's much value in it?
Attempt to do something intelligent with IRQ handlers which don't return IRQ_HANDLED.
- If they return neither IRQ_HANDLED nor IRQ_NONE, complain.
- If they return IRQ_NONE more than 750 times in 1000 interrupts, complain and disable the IRQ.
arch/i386/kernel/irq.c | 96 +++++++++++++++++++++++++++++++++++-------------- include/linux/irq.h | 2 + 2 files changed, 72 insertions(+), 26 deletions(-)
arch/i386/kernel/irq.c renamed from arch/i386/kernel/irq.c~irq-check-rate-limit +70 -26
@@ -66,8 +66,12 @@/* * Controller mappings for all interrupt sources: */ -irq_desc_t irq_desc[NR_IRQS] __cacheline_aligned = - { [0 ...NR_IRQS -1 ] = {0 , &no_irq_type,NULL ,0 ,SPIN_LOCK_UNLOCKED }}; +irq_desc_t irq_desc[NR_IRQS] __cacheline_aligned = { + [0 ...NR_IRQS -1 ] = { + .handler = &no_irq_type, + .lock =SPIN_LOCK_UNLOCKED + } +};static void register_irq_proc (unsigned int irq);
@@ -209,7 +213,6 @@ int handle_IRQ_event(unsigned int irq, {int status =1 ;/* Force the "do bottom halves" bit */ int retval =0 ; -struct irqaction *first_action = action;if (!(action->flags &SA_INTERRUPT ))local_irq_enable ();
@@ -222,30 +225,69 @@ int handle_IRQ_event(unsigned int irq,if (status &SA_SAMPLE_RANDOM )add_interrupt_randomness (irq);local_irq_disable (); -if (retval !=1 ) { -static int count =100 ; -if (count) { - count--; -if (retval) { -printk ("irq event %d: bogus retval mask %x\n" , - irq, retval); - }else { -printk ("irq %d: nobody cared!\n" , irq); - } -dump_stack (); -printk ("handlers:\n" ); - action = first_action; -do { -printk ("[<%p>]" , action->handler); -print_symbol (" (%s)" , - (unsigned long )action->handler); -printk ("\n" ); - action = action->next; - }while (action); +return retval; +} + +static void report_bad_irq (int irq,irq_desc_t *desc,irqreturn_t action_ret) +{ +static int count =100 ; +struct irqaction *action; + +if (count) { + count--; +if (action_ret !=IRQ_HANDLED && action_ret !=IRQ_NONE ) { +printk ("irq event %d: bogus return value %x\n" , + irq, action_ret); + }else { +printk ("irq %d: nobody cared!\n" , irq); } +dump_stack (); +printk ("handlers:\n" ); + action = desc->action; +do { +printk ("[<%p>]" , action->handler); +print_symbol (" (%s)" , + (unsigned long )action->handler); +printk ("\n" ); + action = action->next; + }while (action); } +} -return status; +/* +* If 750 of the previous 1000 interrupts have not been handled then assume +* that the IRQ is stuck in some manner. Drop a diagnostic and try to turn the +* IRQ off. +* +* Called under desc->lock +*/ +static void note_interrupt (int irq,irq_desc_t *desc,irqreturn_t action_ret) +{ +if (action_ret !=IRQ_HANDLED ) { + desc->irqs_unhandled++; +if (action_ret !=IRQ_NONE ) +report_bad_irq (irq, desc, action_ret); + } + + desc->irq_count++; +if (desc->irq_count <1000 ) +return ; + + desc->irq_count =0 ; +if (desc->irqs_unhandled >750 ) { +/* +* The interrupt is stuck +*/ +report_bad_irq (irq, desc, action_ret); +/* +* Now kill the IRQ +*/ +#if 0 + desc->status |=IRQ_DISABLED ; + desc->handler->disable (irq); +#endif + } + desc->irqs_unhandled =0 ; }/*
@@ -418,10 +460,12 @@ asmlinkage unsigned int do_IRQ(struct pt* SMP environment. */ for (;;) { +irqreturn_t action_ret; +spin_unlock (&desc->lock); -handle_IRQ_event (irq, ®s, action); + action_ret =handle_IRQ_event (irq, ®s, action);spin_lock (&desc->lock); - +note_interrupt (irq, desc, action_ret);if (likely (!(desc->status &IRQ_PENDING )))break ; desc->status &= ~IRQ_PENDING ;
include/linux/irq.h renamed from include/linux/irq.h~irq-check-rate-limit +2 -0
@@ -61,6 +61,8 @@ typedef struct {hw_irq_controller *handler;struct irqaction *action;/* IRQ action list */ unsigned int depth;/* nested irq disables */ +unsigned int irq_count;/* For detecting broken interrupts */ +unsigned int irqs_unhandled;spinlock_t lock; }____cacheline_aligned irq_desc_t;
_
Quoted 8 lines · Andrew Morton
The below patch has been in -mm for some time. It was supposed to kill the IRQ if 750 of the previous 1000 IRQs weren't handled.
I disabled the killing code because it was triggering on someone's works-just-fine setup.
There will be pain involved in getting all this to work right. Do you really think there's much value in it?
Being able to at least turn it on at run time is valuable when you are debugging a box operated by someone who doesnt habitually rebuild kernels. The 750 of 1000 thing doesnt work because it can happen to be timing triggered by blocks of IRQ's from a chip being folded together. The "million in a row" should be a stuck IRQ, maybe 50,000 in a row even but just "zillions in a row"
Quoted 8 lines · Alan Cox
Quoted 6 lines · Andrew Morton
The below patch has been in -mm for some time. It was supposed to kill the IRQ if 750 of the previous 1000 IRQs weren't handled.
I disabled the killing code because it was triggering on someone's works-just-fine setup.
Quoted 9 lines
Quoted 2 lines
There will be pain involved in getting all this to work right. Do you really think there's much value in it?
Being able to at least turn it on at run time is valuable when you are debugging a box operated by someone who doesnt habitually rebuild kernels. The 750 of 1000 thing doesnt work because it can happen to be timing triggered by blocks of IRQ's from a chip being folded together. The "million in a row" should be a stuck IRQ, maybe 50,000 in a row even but just "zillions in a row"
The problem with zillions-in-a-row is that the babbling device could be sharing the IRQ with a non-babbling device. So the legitimate interrupts from the non-babbler will cause the detection state machine to reset itself. It will never go off, and the box remains locked up.
Maybe the 750/1000 should be 100/100,000. Who knows...