Re: [PATCH] Delete redundant IRQ_DISABLED check in irq_thread

[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@xxxxxxxxxxxxx>:
> On Fri, 17 Jul 2009, Barry Song wrote:
>> 2009/7/17 Thomas Gleixner <tglx@xxxxxxxxxxxxx>:
>> > 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/