lkml.org 
[lkml]   [2009]   [Jul]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH] Delete redundant IRQ_DISABLED check in irq_thread
From
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/

\
 
 \ /
  Last update: 2009-07-20 05:55    [W:0.089 / U:1.752 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site