lkml.org 
[lkml]   [2009]   [Jul]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
SubjectRe: [PATCH] Delete redundant IRQ_DISABLED check in irq_thread
From
Yes. The patch looks great and very intelligible. I think it is
acceptable and valid. But why not the case you said:

> Fact is that the interrupt can be disabled between the thread wake up
> and the handler thread calling the handler function. So it's a
> question of correctness not to call the handler when the irq is
> disabled for the following reason:

CPU 0 CPU 1
hard irq
-> thread is woken
disable_irq()
synchronize_irq() returns because
there is no thread running the handler

thread runs code which assumes that no irq handler
-> calls handler can run continues.

> That would happen with your patch applied.

be changed to:
CPU 0 CPU 1
hard irq
-> thread is woken
disable_irq()

thread runs
-> calls handler

synchronize_irq() returns because both hardirq
and thread is finished
run continues.

and

CPU 0 CPU 1
hard irq
-> thread is woken

thread runs
-> calls handler
|
| disable_irq()
|
thread fn return
synchronize_irq() returns because both hardirq
and thread is finished.
run continues.

Or synchronize_irq() still returns even thread is not executed or
running, but add an interface like flush_scheduled_work() for
work-queue to wait for the return of thread_fn like:
CPU 0 CPU 1
hard irq
-> thread is woken
disable_irq()
synchronize_irq() returns because
there is no thread running the handler

thread runs
-> calls handler
synchronize_threaded_irq()
run continues.

I guess people call disable_irq to disable the whole execution,
specially to disable the hardirq(then both top and bottom are
blocked). It's really strange the disable operation only blocks the
bottom-half.

And an interrupt is generally a trigger source to trigger other
executions. And other executions often sync according to the
interrupt, and seldom control the interrupt enable/disable
asynchronously. So an asynchronous and unexpected disable_irq() is
generically called by close/remove/release and so on. Why not let it
wait for the finish of top and bottom?


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);
>


\
 
 \ /
  Last update: 2009-07-18 01:29    [W:2.185 / U:0.324 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site