Messages in this thread | | | Date | Mon, 20 Jul 2009 11:52:28 +0800 | Subject | Re: [PATCH] Delete redundant IRQ_DISABLED check in irq_thread | From | Barry Song <> |
| |
Another question, is it possible the irq thread run from a different CPU with the hardirq? If so, even in your patch, the desc->thread_eoi(action->irq, desc) run on another CPU, maybe it can't unmask the original CPU interrupt. Then the device will not work later.
2009/7/18 Thomas Gleixner <tglx@linutronix.de>: > On Fri, 17 Jul 2009, Barry Song wrote: >> 2009/7/17 Thomas Gleixner <tglx@linutronix.de>: >> > The correct thing to do is disabling it at the device level if you >> > need to prevent interrupt storms. >> >> For some devices which use level trigger and buses like spi, i2c. >> Checking the interrupt source and clear IRQ probably need to >> read/write the registers of devices by i2c,spi. But i2c, spi access >> maybe causes schedule based on their driver framework. So the checking >> operation and clear operation can only be done in process switch. So >> the only way to avoid interrupt storms is disabling the irq line at >> hardirq, after clearing irq in the bottom-half by work-queue or >> thread, then enable it again. >> For example, a spi touchscreen is touched, then it gives a high level >> in a GPIO irq line to CPU. CPU enter hardirq, but to clear the high >> level, it must write the touchscreen by spi, how could it avoid the >> interrupt storms betweem hardirq and bottom-half if it doesn't disable >> the irq line since it can't write the spi in hardirq? > > Sigh, I probably don't need to understand why hardware designers come > up with such crap. Using a level triggered interrupt for a device > which cannot be shut up in the hard interrupt routine is simply > moronic. But yeah, we have to live with that. :( > > Removing the disabled check to fix that stupidity and replacing it by > a complex accounting mechanism is not an option. > > The patently untested patch below should solve your problem without > adding horrible complexity. > > I think that's a straight forward solution to the problem and reflects > the semantics of your use case very clearly while it does not touch > the disabled logic. You really do not want to disable the interrupt, > you want to keep it masked until you can unmask it again. That makes a > huge difference. > > All you need to do is to set the irq flow handler of your GPIO pin to > handle_level_oneshot_irq and the generic code will take care of it. > > Thanks, > > tglx > ------ > diff --git a/include/linux/irq.h b/include/linux/irq.h > index cb2e77a..5f22436 100644 > --- a/include/linux/irq.h > +++ b/include/linux/irq.h > @@ -194,6 +194,8 @@ struct irq_desc { > #endif > atomic_t threads_active; > wait_queue_head_t wait_for_threads; > + void (*thread_eoi)(unsigned int irq, > + struct irq_desc *desc); > #ifdef CONFIG_PROC_FS > struct proc_dir_entry *dir; > #endif > @@ -284,6 +286,7 @@ extern irqreturn_t handle_IRQ_event(unsigned int irq, struct irqaction *action); > * callable via desc->chip->handle_irq() > */ > extern void handle_level_irq(unsigned int irq, struct irq_desc *desc); > +extern void handle_level_oneshot_irq(unsigned int irq, struct irq_desc *desc); > extern void handle_fasteoi_irq(unsigned int irq, struct irq_desc *desc); > extern void handle_edge_irq(unsigned int irq, struct irq_desc *desc); > extern void handle_simple_irq(unsigned int irq, struct irq_desc *desc); > diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c > index 13c68e7..2aa072c 100644 > --- a/kernel/irq/chip.c > +++ b/kernel/irq/chip.c > @@ -341,27 +341,15 @@ out_unlock: > spin_unlock(&desc->lock); > } > > -/** > - * handle_level_irq - Level type irq handler > - * @irq: the interrupt number > - * @desc: the interrupt description structure for this irq > - * > - * Level type interrupts are active as long as the hardware line has > - * the active level. This may require to mask the interrupt and unmask > - * it after the associated handler has acknowledged the device, so the > - * interrupt line is back to inactive. > - */ > -void > -handle_level_irq(unsigned int irq, struct irq_desc *desc) > +static void __handle_level_irq(unsigned int irq, struct irq_desc *desc) > { > struct irqaction *action; > irqreturn_t action_ret; > > - spin_lock(&desc->lock); > mask_ack_irq(desc, irq); > > if (unlikely(desc->status & IRQ_INPROGRESS)) > - goto out_unlock; > + return; > desc->status &= ~(IRQ_REPLAY | IRQ_WAITING); > kstat_incr_irqs_this_cpu(irq, desc); > > @@ -371,7 +359,7 @@ handle_level_irq(unsigned int irq, struct irq_desc *desc) > */ > action = desc->action; > if (unlikely(!action || (desc->status & IRQ_DISABLED))) > - goto out_unlock; > + return; > > desc->status |= IRQ_INPROGRESS; > spin_unlock(&desc->lock); > @@ -382,14 +370,69 @@ handle_level_irq(unsigned int irq, struct irq_desc *desc) > > spin_lock(&desc->lock); > desc->status &= ~IRQ_INPROGRESS; > +} > + > +/** > + * handle_level_irq - Level type irq handler > + * @irq: the interrupt number > + * @desc: the interrupt description structure for this irq > + * > + * Level type interrupts are active as long as the hardware line has > + * the active level. This may require to mask the interrupt and unmask > + * it after the associated handler has acknowledged the device, so the > + * interrupt line is back to inactive. > + */ > +void > +handle_level_irq(unsigned int irq, struct irq_desc *desc) > +{ > + spin_lock(&desc->lock); > + __handle_level_irq(irq, desc); > if (!(desc->status & IRQ_DISABLED) && desc->chip->unmask) > desc->chip->unmask(irq); > -out_unlock: > spin_unlock(&desc->lock); > } > EXPORT_SYMBOL_GPL(handle_level_irq); > > /** > + * handle_level_oneshot_irq - Level type oneshot irq handler > + * @irq: the interrupt number > + * @desc: the interrupt description structure for this irq > + * > + * Level type interrupts are active as long as the hardware line has > + * the active level. This may require to mask the interrupt and unmask > + * it after the associated handler has acknowledged the device, so the > + * interrupt line is back to inactive. The oneshot variant keeps the > + * interrupt masked on return. This allows to use it with devices > + * where the interrupt can not be disabled or acknowledged on the > + * device level in the hard interrupt context (device is on i2c, spi..) > + * The unmask is done when the threaded interrupt has processed the > + * interrupt. > + */ > +void > +handle_level_oneshot_irq(unsigned int irq, struct irq_desc *desc) > +{ > + spin_lock(&desc->lock); > + __handle_level_irq(irq, desc); > + desc->status |= IRQ_MASKED; > + spin_unlock(&desc->lock); > +} > +EXPORT_SYMBOL_GPL(handle_level_oneshot_irq); > + > +/* > + * One shot mode handlers do not unmask the irq line in the hard > + * interrupt context. Unmask when the handler has finished. > + */ > +static void unmask_oneshot_irq(unsigned int irq, struct irq_desc *desc) > +{ > + spin_lock_irq(&desc->lock); > + if (!(desc->status & IRQ_DISABLED) && (desc->status & IRQ_MASKED)) { > + desc->status &= ~IRQ_MASKED; > + desc->chip->unmask(irq); > + } > + spin_unlock_irq(&desc->lock); > +} > + > +/** > * handle_fasteoi_irq - irq handler for transparent controllers > * @irq: the interrupt number > * @desc: the interrupt description structure for this irq > @@ -584,6 +627,9 @@ __set_irq_handler(unsigned int irq, irq_flow_handler_t handle, int is_chained, > desc->handle_irq = handle; > desc->name = name; > > + if (handle == handle_level_oneshot_irq) > + desc->thread_eoi = unmask_oneshot_irq; > + > if (handle != handle_bad_irq && is_chained) { > desc->status &= ~IRQ_DISABLED; > desc->status |= IRQ_NOREQUEST | IRQ_NOPROBE; > diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c > index 50da676..d0115d4 100644 > --- a/kernel/irq/manage.c > +++ b/kernel/irq/manage.c > @@ -475,6 +475,8 @@ static int irq_thread(void *data) > spin_unlock_irq(&desc->lock); > > action->thread_fn(action->irq, action->dev_id); > + if (desc->thread_eoi) > + desc->thread_eoi(action->irq, desc); > } > > wake = atomic_dec_and_test(&desc->threads_active); > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |