[PATCH] Delete redundant IRQ_DISABLED check in irq_thread

[PATCH] Delete redundant IRQ_DISABLED check in irq_thread

[Barry Song]

Signed-off-by: Barry Song <21cnbao@gmail.com>
I think it is completely redundant to check IRQ_DISABLED to decide whether thread_fn will be called.

At first, the irq_thread is waken up by HARDIRQ handler, if IRQ_DISABLED is true, HARDIRQ will have no chance to run, then irq_thread will not run. So there is only a situation that both HARDIRQ can run and IRQ_DISABLED is set.
The case is that the flag is set in the interval of HARDIRQ enter and irq_thread is scheduled to run. I think there is nobody which is interested to disable irq in the interval except HARDIRQ handler itself. Then that causes the second problem I will explain.

Secondly, checking the flag causes some problems in fact. We often call disable_irq_nosync to diable irq in HARDIRQ to avoid flooding irq to follow. But the disable_irq_nosync will set IRQ_DISABLED flag, then that will prevent the execution of thread_fn. That's not the original idea to call disable_irq_nosync in HARDIRQ.

So I guess deleting the check is maybe better and more compact.

-Barry
---
 kernel/irq/manage.c |   17 +----------------
 1 files changed, 1 insertions(+), 16 deletions(-)

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 50da676..ff4e05d 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -460,22 +460,7 @@ static int irq_thread(void *data)
 
 		atomic_inc(&desc->threads_active);
 
-		spin_lock_irq(&desc->lock);
-		if (unlikely(desc->status & IRQ_DISABLED)) {
-			/*
-			 * CHECKME: We might need a dedicated
-			 * IRQ_THREAD_PENDING flag here, which
-			 * retriggers the thread in check_irq_resend()
-			 * but AFAICT IRQ_PENDING should be fine as it
-			 * retriggers the interrupt itself --- tglx
-			 */
-			desc->status |= IRQ_PENDING;
-			spin_unlock_irq(&desc->lock);
-		} else {
-			spin_unlock_irq(&desc->lock);
-
-			action->thread_fn(action->irq, action->dev_id);
-		}
+		action->thread_fn(action->irq, action->dev_id);
 
 		wake = atomic_dec_and_test(&desc->threads_active);
 
-- 
1.5.6.3

Re: [PATCH] Delete redundant IRQ_DISABLED check in irq_thread

[Thomas Gleixner]

On Fri, 17 Jul 2009, Barry Song wrote:
> Signed-off-by: Barry Song <21cnbao@gmail.com>

> I think it is completely redundant to check IRQ_DISABLED to decide
> whether thread_fn will be called.

I do not :)

> At first, the irq_thread is waken up by HARDIRQ handler, if
> IRQ_DISABLED is true, HARDIRQ will have no chance to run, then
> irq_thread will not run. So there is only a situation that both
> HARDIRQ can run and IRQ_DISABLED is set.
>
> The case is that the flag is set in the interval of HARDIRQ enter
> and irq_thread is scheduled to run. I think there is nobody which is
> interested to disable irq in the interval except HARDIRQ handler
> itself. Then that causes the second problem I will explain.

Err. It's not a question whether somebody is interested or not. 

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.

We would need more complex accounting when we want to cover the full
chain from hardirq->wakeup->thread to avoid the above scenario, but
that'd be a nightmare as we would have to deal with accounting wakeups
of an already running irq thread as well.

We optimize for the normal and fast case so the current logic is
correct and stays that way.

> Secondly, checking the flag causes some problems in fact. We often
> call disable_irq_nosync to diable irq in HARDIRQ to avoid flooding
> irq to follow. But the disable_irq_nosync will set IRQ_DISABLED
> flag, then that will prevent the execution of thread_fn. That's not
> the original idea to call disable_irq_nosync in HARDIRQ.

Calling disable_irq_nosync in the hard irq handler is wrong with
threaded interrupts and I even consider it wrong with non threaded irq
handlers. 

The hard interrupt handler of a threaded irq needs to check whether
the interrupt originated from the device and if that's the case it
needs to disable the irq in the device rather than disabling the
interrupt line completely. Simply because it would disable shared
interrupts on the same irq line until the thread reenables them.

The correct thing to do is disabling it at the device level if you
need to prevent interrupt storms.

Thanks,

	tglx

Re: [PATCH] Delete redundant IRQ_DISABLED check in irq_thread

[Barry Song]

2009/7/17 Thomas Gleixner <tglx@linutronix.de>:
> On Fri, 17 Jul 2009, Barry Song wrote:
>> Signed-off-by: Barry Song <21cnbao@gmail.com>
>
>> I think it is completely redundant to check IRQ_DISABLED to decide
>> whether thread_fn will be called.
>
> I do not :)
>
>> At first, the irq_thread is waken up by HARDIRQ handler, if
>> IRQ_DISABLED is true, HARDIRQ will have no chance to run, then
>> irq_thread will not run. So there is only a situation that both
>> HARDIRQ can run and IRQ_DISABLED is set.
>>
>> The case is that the flag is set in the interval of HARDIRQ enter
>> and irq_thread is scheduled to run. I think there is nobody which is
>> interested to disable irq in the interval except HARDIRQ handler
>> itself. Then that causes the second problem I will explain.
>
> Err. It's not a question whether somebody is interested or not.
>
> 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.
>
> We would need more complex accounting when we want to cover the full
> chain from hardirq->wakeup->thread to avoid the above scenario, but
> that'd be a nightmare as we would have to deal with accounting wakeups
> of an already running irq thread as well.
>
> We optimize for the normal and fast case so the current logic is
> correct and stays that way.
>
>> Secondly, checking the flag causes some problems in fact. We often
>> call disable_irq_nosync to diable irq in HARDIRQ to avoid flooding
>> irq to follow. But the disable_irq_nosync will set IRQ_DISABLED
>> flag, then that will prevent the execution of thread_fn. That's not
>> the original idea to call disable_irq_nosync in HARDIRQ.
>
> Calling disable_irq_nosync in the hard irq handler is wrong with
> threaded interrupts and I even consider it wrong with non threaded irq
> handlers.
>
> The hard interrupt handler of a threaded irq needs to check whether
> the interrupt originated from the device and if that's the case it
> needs to disable the irq in the device rather than disabling the
> interrupt line completely. Simply because it would disable shared
> interrupts on the same irq line until the thread reenables them.
>
> 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?
>
> Thanks,
>
>        tglx
>

Re: [PATCH] Delete redundant IRQ_DISABLED check in irq_thread

[Thomas Gleixner]

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

Re: [PATCH] Delete redundant IRQ_DISABLED check in irq_thread

[Barry Song]

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

Re: [PATCH] Delete redundant IRQ_DISABLED check in irq_thread

[Thomas Gleixner]

On Sat, 18 Jul 2009, Barry Song wrote:
> 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.

The disable blocks both. After the disable no hardirq comes in
anymore. 

The disabled check in the thread is just to take care of the case when
the disable comes in between the hardirq and the thread handler.

That way we avoid doing the full tracking of the hardirq / thread
chain simply because it is complex and would produce significant
overhead in the fast path for no gain. It could be done, but it's not
worth the trouble.

Thanks,

	tglx

Re: [PATCH] Delete redundant IRQ_DISABLED check in irq_thread

[Barry Song]

2009/7/18, Thomas Gleixner <tglx@linutronix.de>:
> On Sat, 18 Jul 2009, Barry Song wrote:
>> 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.
>
> The disable blocks both. After the disable no hardirq comes in
> anymore.
>
I know disable_irq will block the future hardirq and bottom-half. Here
I means the current hardirq has been executed, but bottom-half will
not be executed because of the disable_irq. So the disable_irq only
blocks a half for the special case and the work of the current IRQ is
not finished fully in fact. But anyway, it should not cause trouble,
but just give the semantic of disable_irq a little confused.
Anyway, I agree it is not necessary to fulfill a complicated
check/sync for thread IRQ.

And thanks for your review and providing a good solution for my problem!

> The disabled check in the thread is just to take care of the case when
> the disable comes in between the hardirq and the thread handler.
>
> That way we avoid doing the full tracking of the hardirq / thread
> chain simply because it is complex and would produce significant
> overhead in the fast path for no gain. It could be done, but it's not
> worth the trouble.
>
> Thanks,
>
> 	tglx
>
>

Thanks,
Barry

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

Re: [PATCH] Delete redundant IRQ_DISABLED check in irq_thread

[Thomas Gleixner]

On Mon, 20 Jul 2009, Barry Song wrote:

Can you please stop top posting and keeping the whole previous mail
for no value ?

> 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.

Sorry, I do not understand your question. Can you please explain ?

Thanks,

	tglx