[PATCH] genirq: for edge interrupt IRQS_ONESHOT support with irq thread

[PATCH] genirq: for edge interrupt IRQS_ONESHOT support with irq thread

[Chuansheng Liu]

In our system, there is one edge interrupt, and we want it to be
irq thread with IRQS_ONESHOT, and found in handle_edge_irq(),
even with IRQS_ONESHOT, the irq is still unmasked without care of
flag IRQS_ONESHOT.

It causes IRQS_ONESHOT can not work well for edge interrupt, but also
after the irq thread finished with flag IRQS_ONESHOT, the irq will be
possible to be unmasked again, it should be messing mask/unmask logic.

Signed-off-by: liu chuansheng <chuansheng.liu@intel.com>
---
 kernel/irq/chip.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index 57d86d0..f23f524 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -497,7 +497,13 @@ handle_edge_irq(unsigned int irq, struct irq_desc *desc)
 	kstat_incr_irqs_this_cpu(irq, desc);
 
 	/* Start handling the irq */
-	desc->irq_data.chip->irq_ack(&desc->irq_data);
+	if (desc->istate & IRQS_ONESHOT) {
+		mask_ack_irq(desc);
+		handle_irq_event(desc);
+		cond_unmask_irq(desc);
+		goto out_unlock;
+	} else
+		desc->irq_data.chip->irq_ack(&desc->irq_data);
 
 	do {
 		if (unlikely(!desc->action)) {
-- 
1.7.0.4

Re: [PATCH] genirq: for edge interrupt IRQS_ONESHOT support with irq thread

[Thomas Gleixner]

On Fri, 12 Oct 2012, Chuansheng Liu wrote:
> 
> In our system, there is one edge interrupt, and we want it to be
> irq thread with IRQS_ONESHOT, and found in handle_edge_irq(),
> even with IRQS_ONESHOT, the irq is still unmasked without care of
> flag IRQS_ONESHOT.
> 
> It causes IRQS_ONESHOT can not work well for edge interrupt, but also
> after the irq thread finished with flag IRQS_ONESHOT, the irq will be
> possible to be unmasked again, it should be messing mask/unmask logic.

This is just wrong. By masking edge interrupts you will run into
situations where you will lose interrupts.

Can you please explain, why you want to mask your edge interrupt?

Thanks,

	tglx

 
> Signed-off-by: liu chuansheng <chuansheng.liu@intel.com>
> ---
>  kernel/irq/chip.c |    8 +++++++-
>  1 files changed, 7 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
> index 57d86d0..f23f524 100644
> --- a/kernel/irq/chip.c
> +++ b/kernel/irq/chip.c
> @@ -497,7 +497,13 @@ handle_edge_irq(unsigned int irq, struct irq_desc *desc)
>  	kstat_incr_irqs_this_cpu(irq, desc);
>  
>  	/* Start handling the irq */
> -	desc->irq_data.chip->irq_ack(&desc->irq_data);
> +	if (desc->istate & IRQS_ONESHOT) {
> +		mask_ack_irq(desc);
> +		handle_irq_event(desc);
> +		cond_unmask_irq(desc);
> +		goto out_unlock;
> +	} else
> +		desc->irq_data.chip->irq_ack(&desc->irq_data);
>  
>  	do {
>  		if (unlikely(!desc->action)) {
> -- 
> 1.7.0.4
> 
> 
> 
> 

RE: [PATCH] genirq: for edge interrupt IRQS_ONESHOT support with irq thread

[Liu, Chuansheng]

> -----Original Message-----
> From: Thomas Gleixner [mailto:tglx@linutronix.de]
> Sent: Friday, October 12, 2012 8:32 PM
> To: Liu, Chuansheng
> Cc: linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] genirq: for edge interrupt IRQS_ONESHOT support with irq
> thread
> 
> On Fri, 12 Oct 2012, Chuansheng Liu wrote:
> >
> > In our system, there is one edge interrupt, and we want it to be
> > irq thread with IRQS_ONESHOT, and found in handle_edge_irq(),
> > even with IRQS_ONESHOT, the irq is still unmasked without care of
> > flag IRQS_ONESHOT.
> >
> > It causes IRQS_ONESHOT can not work well for edge interrupt, but also
> > after the irq thread finished with flag IRQS_ONESHOT, the irq will be
> > possible to be unmasked again, it should be messing mask/unmask logic.
> 
> This is just wrong. By masking edge interrupts you will run into
> situations where you will lose interrupts.
> 
> Can you please explain, why you want to mask your edge interrupt?
When I request_irq with irq thread handler and flag IRQS_ONESHOT, if do not mask the edge interrupt,
the primary handler and irq thread maybe run at the same time, and in my real case it causes spin deadlock.

You means it is not right with IRQS_ONESHOT for edge interrupt?
> 
> Thanks,
> 
> 	tglx
> 
> 
> > Signed-off-by: liu chuansheng <chuansheng.liu@intel.com>
> > ---
> >  kernel/irq/chip.c |    8 +++++++-
> >  1 files changed, 7 insertions(+), 1 deletions(-)
> >
> > diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
> > index 57d86d0..f23f524 100644
> > --- a/kernel/irq/chip.c
> > +++ b/kernel/irq/chip.c
> > @@ -497,7 +497,13 @@ handle_edge_irq(unsigned int irq, struct irq_desc
> *desc)
> >  	kstat_incr_irqs_this_cpu(irq, desc);
> >
> >  	/* Start handling the irq */
> > -	desc->irq_data.chip->irq_ack(&desc->irq_data);
> > +	if (desc->istate & IRQS_ONESHOT) {
> > +		mask_ack_irq(desc);
> > +		handle_irq_event(desc);
> > +		cond_unmask_irq(desc);
> > +		goto out_unlock;
> > +	} else
> > +		desc->irq_data.chip->irq_ack(&desc->irq_data);
> >
> >  	do {
> >  		if (unlikely(!desc->action)) {
> > --
> > 1.7.0.4
> >
> >
> >
> >

RE: [PATCH] genirq: for edge interrupt IRQS_ONESHOT support with irq thread

[Thomas Gleixner]

On Fri, 12 Oct 2012, Liu, Chuansheng wrote:
> > -----Original Message-----
> > From: Thomas Gleixner [mailto:tglx@linutronix.de]
> > Sent: Friday, October 12, 2012 8:32 PM
> > To: Liu, Chuansheng
> > Cc: linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH] genirq: for edge interrupt IRQS_ONESHOT support with irq
> > thread
> > 
> > On Fri, 12 Oct 2012, Chuansheng Liu wrote:
> > >
> > > In our system, there is one edge interrupt, and we want it to be
> > > irq thread with IRQS_ONESHOT, and found in handle_edge_irq(),
> > > even with IRQS_ONESHOT, the irq is still unmasked without care of
> > > flag IRQS_ONESHOT.
> > >
> > > It causes IRQS_ONESHOT can not work well for edge interrupt, but also
> > > after the irq thread finished with flag IRQS_ONESHOT, the irq will be
> > > possible to be unmasked again, it should be messing mask/unmask logic.
> > 
> > This is just wrong. By masking edge interrupts you will run into
> > situations where you will lose interrupts.
> > 
> > Can you please explain, why you want to mask your edge interrupt?

> When I request_irq with irq thread handler and flag IRQS_ONESHOT, if
> do not mask the edge interrupt, the primary handler and irq thread
> maybe run at the same time, and in my real case it causes spin
> deadlock.

Then your code is simply wrong and you need to fix it instead of hacking a
workaround into the core code. Locking is not that hard.

> You means it is not right with IRQS_ONESHOT for edge interrupt?

It's wrong. Simnply because you can lose interrupts.

     interrupt raised
     	 handle_edge_irq()
	    mask_ack_irq()
	    handle_event()
	        wake irq thread
     reti

     irq thread runs
     	 handle device interrupt()
				   <--- device issues edge irq
	 unmask_irq()

This interrupt is not delivered. So your device stops working. Not
what you want, right?

Thanks,

	tglx

RE: [PATCH] genirq: for edge interrupt IRQS_ONESHOT support with irq thread

[Liu, Chuansheng]

> -----Original Message-----
> From: Thomas Gleixner [mailto:tglx@linutronix.de]
> Sent: Friday, October 12, 2012 8:46 PM
> To: Liu, Chuansheng
> Cc: linux-kernel@vger.kernel.org
> Subject: RE: [PATCH] genirq: for edge interrupt IRQS_ONESHOT support with irq
> thread
> 
> On Fri, 12 Oct 2012, Liu, Chuansheng wrote:
> > > -----Original Message-----
> > > From: Thomas Gleixner [mailto:tglx@linutronix.de]
> > > Sent: Friday, October 12, 2012 8:32 PM
> > > To: Liu, Chuansheng
> > > Cc: linux-kernel@vger.kernel.org
> > > Subject: Re: [PATCH] genirq: for edge interrupt IRQS_ONESHOT support
> with irq
> > > thread
> > >
> > > On Fri, 12 Oct 2012, Chuansheng Liu wrote:
> > > >
> > > > In our system, there is one edge interrupt, and we want it to be
> > > > irq thread with IRQS_ONESHOT, and found in handle_edge_irq(),
> > > > even with IRQS_ONESHOT, the irq is still unmasked without care of
> > > > flag IRQS_ONESHOT.
> > > >
> > > > It causes IRQS_ONESHOT can not work well for edge interrupt, but also
> > > > after the irq thread finished with flag IRQS_ONESHOT, the irq will be
> > > > possible to be unmasked again, it should be messing mask/unmask logic.
> > >
> > > This is just wrong. By masking edge interrupts you will run into
> > > situations where you will lose interrupts.
> > >
> > > Can you please explain, why you want to mask your edge interrupt?
> 
> > When I request_irq with irq thread handler and flag IRQS_ONESHOT, if
> > do not mask the edge interrupt, the primary handler and irq thread
> > maybe run at the same time, and in my real case it causes spin
> > deadlock.
> 
> Then your code is simply wrong and you need to fix it instead of hacking a
> workaround into the core code. Locking is not that hard.
I got means, so I want to use flag IRQS_ONESHOT to avoid the case that two
handlers running at the same time. Is it right direction?
But IRQS_ONESHOT does not work well for edge interrupt.
And pasting the IRQS_ONESHOT description:
* IRQS_ONESHOT			- irq is not unmasked in primary handler

And I need irq handler is because some heavy work is needed, it can avoid local
irq disabling time thru irq handler.

> 
> > You means it is not right with IRQS_ONESHOT for edge interrupt?
> 
> It's wrong. Simnply because you can lose interrupts.
> 
>      interrupt raised
>      	 handle_edge_irq()
> 	    mask_ack_irq()
> 	    handle_event()
> 	        wake irq thread
>      reti
> 
>      irq thread runs
>      	 handle device interrupt()
> 				   <--- device issues edge irq
> 	 unmask_irq()
> 
> This interrupt is not delivered. So your device stops working. Not
> what you want, right?
Device should not stop:) And even in current handle_edge_irq(), it is possible that losting
Interrupt if primary handler need some time and the irq is quick enough. I says the below
code, it just avoid one time lost.

                        desc->istate |= IRQS_PENDING;
                        mask_ack_irq(desc);


> 
> Thanks,
> 
> 	tglx

RE: [PATCH] genirq: for edge interrupt IRQS_ONESHOT support with irq thread

[Thomas Gleixner]

On Fri, 12 Oct 2012, Liu, Chuansheng wrote:
> > -----Original Message-----
> > From: Thomas Gleixner [mailto:tglx@linutronix.de]
> > Sent: Friday, October 12, 2012 8:46 PM
> > To: Liu, Chuansheng
> > Cc: linux-kernel@vger.kernel.org
> > Subject: RE: [PATCH] genirq: for edge interrupt IRQS_ONESHOT support with irq
> > thread
> > 
> > On Fri, 12 Oct 2012, Liu, Chuansheng wrote:
> > > > -----Original Message-----
> > > > From: Thomas Gleixner [mailto:tglx@linutronix.de]
> > > > Sent: Friday, October 12, 2012 8:32 PM
> > > > To: Liu, Chuansheng
> > > > Cc: linux-kernel@vger.kernel.org
> > > > Subject: Re: [PATCH] genirq: for edge interrupt IRQS_ONESHOT support
> > with irq
> > > > thread
> > > >
> > > > On Fri, 12 Oct 2012, Chuansheng Liu wrote:
> > > > >
> > > > > In our system, there is one edge interrupt, and we want it to be
> > > > > irq thread with IRQS_ONESHOT, and found in handle_edge_irq(),
> > > > > even with IRQS_ONESHOT, the irq is still unmasked without care of
> > > > > flag IRQS_ONESHOT.
> > > > >
> > > > > It causes IRQS_ONESHOT can not work well for edge interrupt, but also
> > > > > after the irq thread finished with flag IRQS_ONESHOT, the irq will be
> > > > > possible to be unmasked again, it should be messing mask/unmask logic.
> > > >
> > > > This is just wrong. By masking edge interrupts you will run into
> > > > situations where you will lose interrupts.
> > > >
> > > > Can you please explain, why you want to mask your edge interrupt?
> > 
> > > When I request_irq with irq thread handler and flag IRQS_ONESHOT, if
> > > do not mask the edge interrupt, the primary handler and irq thread
> > > maybe run at the same time, and in my real case it causes spin
> > > deadlock.
> > 
> > Then your code is simply wrong and you need to fix it instead of hacking a
> > workaround into the core code. Locking is not that hard.
>
> I got means, so I want to use flag IRQS_ONESHOT to avoid the case that two
> handlers running at the same time. Is it right direction?

Again, this does not work for edge type interrupts. Period. And we are
not adding something which is known to be broken.

Also there is no problem when a hard interrupt comes in while the
thread handler is running. It's just a matter of proper code and
proper locking.

> But IRQS_ONESHOT does not work well for edge interrupt.
> And pasting the IRQS_ONESHOT description:
> * IRQS_ONESHOT			- irq is not unmasked in primary handler

Right, and edge type interrupts doe not support it.
 
> And I need irq handler is because some heavy work is needed, it can avoid local
> irq disabling time thru irq handler.

Disable the interrupt at the device level in your primary handler, but
do not try to impose something to the core code which is fundamentaly
wrong.
 
> > 
> > > You means it is not right with IRQS_ONESHOT for edge interrupt?
> > 
> > It's wrong. Simnply because you can lose interrupts.
> > 
> >      interrupt raised
> >      	 handle_edge_irq()
> > 	    mask_ack_irq()
> > 	    handle_event()
> > 	        wake irq thread
> >      reti
> > 
> >      irq thread runs
> >      	 handle device interrupt()
> > 				   <--- device issues edge irq
> > 	 unmask_irq()
> > 
> > This interrupt is not delivered. So your device stops working. Not
> > what you want, right?

> Device should not stop:) And even in current handle_edge_irq(), it
> is possible that losting Interrupt if primary handler need some time
> and the irq is quick enough. I says the below code, it just avoid

The code flow is:

   Interrupt
	ack()
	handler()
   RETI

After the ack another interrupt can come in. It's raised in the CPU,
but it cannot be delivered because the CPU is running that very
interrupt at this point with interupts disabled. After RETI this
interrupt is delivered and runs the edge handler again. That's on UP.

On SMP an interrupt which is raised after the ack() again before the
handler finishes, can invoke another delivery on a different CPU,
which then sees the IRQ_INPROGESS flag, masks it and flags it
PENDING. When the primary handler on the first CPU returns, it sees
the PENDING flag, unmasks and invokes the handler another time.

So nothing gets lost. Now you mask it and if you look at the flow I
showed in my last mail, then your device will be stuck. Simply because
the interrupt was delivered while the line was masked at the irq chip
level which causes a drop. That's a property of edge type interrupts
and we have proper code to deal with it. No way to change that just
that you can avoid to fix your broken driver design.

Thanks,

	tglx

RE: [PATCH] genirq: for edge interrupt IRQS_ONESHOT support with irq thread

[Liu, Chuansheng]

Thanks your beautiful explain for edge interrupt handler, still has one confusing for unmask_irq in
irq_finalize_oneshot().

Could you see below comments? Thanks.

> -----Original Message-----
> From: Thomas Gleixner [mailto:tglx@linutronix.de]
> Sent: Friday, October 12, 2012 9:48 PM
> To: Liu, Chuansheng
> Cc: linux-kernel@vger.kernel.org
> Subject: RE: [PATCH] genirq: for edge interrupt IRQS_ONESHOT support with irq
> thread
> 
> On Fri, 12 Oct 2012, Liu, Chuansheng wrote:
> > > -----Original Message-----
> > > From: Thomas Gleixner [mailto:tglx@linutronix.de]
> > > Sent: Friday, October 12, 2012 8:46 PM
> > > To: Liu, Chuansheng
> > > Cc: linux-kernel@vger.kernel.org
> > > Subject: RE: [PATCH] genirq: for edge interrupt IRQS_ONESHOT support
> with irq
> > > thread
> > >
> > > On Fri, 12 Oct 2012, Liu, Chuansheng wrote:
> > > > > -----Original Message-----
> > > > > From: Thomas Gleixner [mailto:tglx@linutronix.de]
> > > > > Sent: Friday, October 12, 2012 8:32 PM
> > > > > To: Liu, Chuansheng
> > > > > Cc: linux-kernel@vger.kernel.org
> > > > > Subject: Re: [PATCH] genirq: for edge interrupt IRQS_ONESHOT support
> > > with irq
> > > > > thread
> > > > >
> > > > > On Fri, 12 Oct 2012, Chuansheng Liu wrote:
> > > > > >
> > > > > > In our system, there is one edge interrupt, and we want it to be
> > > > > > irq thread with IRQS_ONESHOT, and found in handle_edge_irq(),
> > > > > > even with IRQS_ONESHOT, the irq is still unmasked without care of
> > > > > > flag IRQS_ONESHOT.
> > > > > >
> > > > > > It causes IRQS_ONESHOT can not work well for edge interrupt, but
> also
> > > > > > after the irq thread finished with flag IRQS_ONESHOT, the irq will be
> > > > > > possible to be unmasked again, it should be messing mask/unmask
> logic.
> > > > >
> > > > > This is just wrong. By masking edge interrupts you will run into
> > > > > situations where you will lose interrupts.
> > > > >
> > > > > Can you please explain, why you want to mask your edge interrupt?
> > >
> > > > When I request_irq with irq thread handler and flag IRQS_ONESHOT, if
> > > > do not mask the edge interrupt, the primary handler and irq thread
> > > > maybe run at the same time, and in my real case it causes spin
> > > > deadlock.
> > >
> > > Then your code is simply wrong and you need to fix it instead of hacking a
> > > workaround into the core code. Locking is not that hard.
> >
> > I got means, so I want to use flag IRQS_ONESHOT to avoid the case that two
> > handlers running at the same time. Is it right direction?
> 
> Again, this does not work for edge type interrupts. Period. And we are
> not adding something which is known to be broken.
> 
> Also there is no problem when a hard interrupt comes in while the
> thread handler is running. It's just a matter of proper code and
> proper locking.
> 
> > But IRQS_ONESHOT does not work well for edge interrupt.
> > And pasting the IRQS_ONESHOT description:
> > * IRQS_ONESHOT			- irq is not unmasked in primary handler
> 
> Right, and edge type interrupts doe not support it.
Can we do something? Thanks your sharing.
In request_thread_irq() case with FLAG IRQS_ONESHOT, for edge interrupt,
in function irq_finalize_oneshot():
	if (!desc->threads_oneshot && !irqd_irq_disabled(&desc->irq_data) &&
	    irqd_irq_masked(&desc->irq_data))
		unmask_irq(desc);

It is possible unmask_irq() is called, but the below code is just aiming for masking action
in irq handler, so I guess if I called the mask_irq() in non-core code, when irq_finalize_oneshot
is called, the unmask_irq is called, and it is not we wanted, right? Do not test this case:)
> 
> > And I need irq handler is because some heavy work is needed, it can avoid
> local
> > irq disabling time thru irq handler.
> 
> Disable the interrupt at the device level in your primary handler, but
> do not try to impose something to the core code which is fundamentaly
> wrong.
> 
> > >
> > > > You means it is not right with IRQS_ONESHOT for edge interrupt?
> > >
> > > It's wrong. Simnply because you can lose interrupts.
> > >
> > >      interrupt raised
> > >      	 handle_edge_irq()
> > > 	    mask_ack_irq()
> > > 	    handle_event()
> > > 	        wake irq thread
> > >      reti
> > >
> > >      irq thread runs
> > >      	 handle device interrupt()
> > > 				   <--- device issues edge irq
> > > 	 unmask_irq()
> > >
> > > This interrupt is not delivered. So your device stops working. Not
> > > what you want, right?
> 
> > Device should not stop:) And even in current handle_edge_irq(), it
> > is possible that losting Interrupt if primary handler need some time
> > and the irq is quick enough. I says the below code, it just avoid
> 
> The code flow is:
> 
>    Interrupt
> 	ack()
> 	handler()
>    RETI
> 
> After the ack another interrupt can come in. It's raised in the CPU,
> but it cannot be delivered because the CPU is running that very
> interrupt at this point with interupts disabled. After RETI this
> interrupt is delivered and runs the edge handler again. That's on UP.
> 
> On SMP an interrupt which is raised after the ack() again before the
> handler finishes, can invoke another delivery on a different CPU,
> which then sees the IRQ_INPROGESS flag, masks it and flags it
> PENDING. When the primary handler on the first CPU returns, it sees
> the PENDING flag, unmasks and invokes the handler another time.
> 
Got it. Thanks your clear explain.

> So nothing gets lost. Now you mask it and if you look at the flow I
> showed in my last mail, then your device will be stuck. Simply because
> the interrupt was delivered while the line was masked at the irq chip
> level which causes a drop. That's a property of edge type interrupts
> and we have proper code to deal with it. No way to change that just
> that you can avoid to fix your broken driver design.
> 
> Thanks,
> 
> 	tglx
> 

RE: [PATCH] genirq: for edge interrupt IRQS_ONESHOT support with irq thread

[Thomas Gleixner]

On Fri, 12 Oct 2012, Liu, Chuansheng wrote:
> > > But IRQS_ONESHOT does not work well for edge interrupt.
> > > And pasting the IRQS_ONESHOT description:
> > > * IRQS_ONESHOT			- irq is not unmasked in primary handler
> > 
> > Right, and edge type interrupts doe not support it.
>
> Can we do something? Thanks your sharing.

No, we cannot do anything. The edge handler is not going to
change. End of story.

> In request_thread_irq() case with FLAG IRQS_ONESHOT, for edge interrupt,
> in function irq_finalize_oneshot():
> 	if (!desc->threads_oneshot && !irqd_irq_disabled(&desc->irq_data) &&
> 	    irqd_irq_masked(&desc->irq_data))
> 		unmask_irq(desc);
> 
> It is possible unmask_irq() is called, but the below code is just
> aiming for masking action in irq handler, so I guess if I called the
> mask_irq() in non-core code, when irq_finalize_oneshot is called,
> the unmask_irq is called, and it is not we wanted, right? Do not
> test this case:)

You're guessing wrong again. Non core code CANNOT call mask_irq()
except via irq_disable(). No have a look at irq_disable() and then
read the above condition again.

Thanks,

	tglx

RE: [PATCH] genirq: for edge interrupt IRQS_ONESHOT support with irq thread

[Liu, Chuansheng]

> On SMP an interrupt which is raised after the ack() again before the
> handler finishes, can invoke another delivery on a different CPU,
> which then sees the IRQ_INPROGESS flag, masks it and flags it
> PENDING. When the primary handler on the first CPU returns, it sees
> the PENDING flag, unmasks and invokes the handler another time.
In this case, when IRQ_INPROGRESS flag is set, on another CPU, it will
mask and ack it, if before the primary handler on the first CPU returns,
the edge interrupt is raised again, it will be lost, right?
So I think set PENDING just confirm one time, it just depends on primary handler
execution time and irq frequency.

RE: [PATCH] genirq: for edge interrupt IRQS_ONESHOT support with irq thread

[anish kumar]

On Fri, 2012-10-12 at 14:57 +0000, Liu, Chuansheng wrote:
> > On SMP an interrupt which is raised after the ack() again before the
> > handler finishes, can invoke another delivery on a different CPU,
> > which then sees the IRQ_INPROGESS flag, masks it and flags it
> > PENDING. When the primary handler on the first CPU returns, it sees
> > the PENDING flag, unmasks and invokes the handler another time.
> In this case, when IRQ_INPROGRESS flag is set, on another CPU, it will
> mask and ack it, if before the primary handler on the first CPU returns,
> the edge interrupt is raised again, it will be lost, right?
Why will the interrupt be raised again?Is not it masked?I read tglx
statement as this:if the interrupt is being handled on one core, then
the delivery of new interrupt can be on the second core and in that case
it will see IRQ_INPROGRESS flag and it will *mask* it and set the flag
as pending.So there is no chance of any new interrupt.
> So I think set PENDING just confirm one time, it just depends on primary handler
> execution time and irq frequency.
> --
> 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/

RE: [PATCH] genirq: for edge interrupt IRQS_ONESHOT support with irq thread

[Liu, Chuansheng]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1938 bytes --]



> -----Original Message-----
> From: anish kumar [mailto:anish198519851985@gmail.com]
> Sent: Friday, October 12, 2012 11:25 PM
> To: Liu, Chuansheng
> Cc: Thomas Gleixner; linux-kernel@vger.kernel.org
> Subject: RE: [PATCH] genirq: for edge interrupt IRQS_ONESHOT support with irq
> thread
> 
> On Fri, 2012-10-12 at 14:57 +0000, Liu, Chuansheng wrote:
> > > On SMP an interrupt which is raised after the ack() again before the
> > > handler finishes, can invoke another delivery on a different CPU,
> > > which then sees the IRQ_INPROGESS flag, masks it and flags it
> > > PENDING. When the primary handler on the first CPU returns, it sees
> > > the PENDING flag, unmasks and invokes the handler another time.
> > In this case, when IRQ_INPROGRESS flag is set, on another CPU, it will
> > mask and ack it, if before the primary handler on the first CPU returns,
> > the edge interrupt is raised again, it will be lost, right?
> Why will the interrupt be raised again?Is not it masked?I read tglx
I means because it is masked, if at this time device issues edge irq,
It will not be delivered and lost.

> statement as this:if the interrupt is being handled on one core, then
> the delivery of new interrupt can be on the second core and in that case
> it will see IRQ_INPROGRESS flag and it will *mask* it and set the flag
> as pending.So there is no chance of any new interrupt.
> > So I think set PENDING just confirm one time, it just depends on primary
> handler
> > execution time and irq frequency.
> > --
> > 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/
> 

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCH] genirq: for edge interrupt IRQS_ONESHOT support with irq thread

[anish kumar]

On Fri, 2012-10-12 at 15:29 +0000, Liu, Chuansheng wrote:
> 
> > -----Original Message-----
> > From: anish kumar [mailto:anish198519851985@gmail.com]
> > Sent: Friday, October 12, 2012 11:25 PM
> > To: Liu, Chuansheng
> > Cc: Thomas Gleixner; linux-kernel@vger.kernel.org
> > Subject: RE: [PATCH] genirq: for edge interrupt IRQS_ONESHOT support with irq
> > thread
> > 
> > On Fri, 2012-10-12 at 14:57 +0000, Liu, Chuansheng wrote:
> > > > On SMP an interrupt which is raised after the ack() again before the
> > > > handler finishes, can invoke another delivery on a different CPU,
> > > > which then sees the IRQ_INPROGESS flag, masks it and flags it
> > > > PENDING. When the primary handler on the first CPU returns, it sees
> > > > the PENDING flag, unmasks and invokes the handler another time.
> > > In this case, when IRQ_INPROGRESS flag is set, on another CPU, it will
> > > mask and ack it, if before the primary handler on the first CPU returns,
> > > the edge interrupt is raised again, it will be lost, right?
> > Why will the interrupt be raised again?Is not it masked?I read tglx
> I means because it is masked, if at this time device issues edge irq,
> It will not be delivered and lost.
That depends on the hardware, doesn't it?If if the frequency of
interrupt is too much that device is not able to cope up with that then
don't you think that the interrupt controller is faulty?

Well I am not a expert on the hardware but it looks to me that the
hardware should be at a fault here.
> 
> > statement as this:if the interrupt is being handled on one core, then
> > the delivery of new interrupt can be on the second core and in that case
> > it will see IRQ_INPROGRESS flag and it will *mask* it and set the flag
> > as pending.So there is no chance of any new interrupt.
> > > So I think set PENDING just confirm one time, it just depends on primary
> > handler
> > > execution time and irq frequency.
> > > --
> > > 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/
> > 
> 

RE: [PATCH] genirq: for edge interrupt IRQS_ONESHOT support with irq thread

[Thomas Gleixner]

On Fri, 12 Oct 2012, Liu, Chuansheng wrote:
> > -----Original Message-----
> > From: anish kumar [mailto:anish198519851985@gmail.com]
> > Sent: Friday, October 12, 2012 11:25 PM
> > To: Liu, Chuansheng
> > Cc: Thomas Gleixner; linux-kernel@vger.kernel.org
> > Subject: RE: [PATCH] genirq: for edge interrupt IRQS_ONESHOT support with irq
> > thread
> > 
> > On Fri, 2012-10-12 at 14:57 +0000, Liu, Chuansheng wrote:
> > > > On SMP an interrupt which is raised after the ack() again before the
> > > > handler finishes, can invoke another delivery on a different CPU,
> > > > which then sees the IRQ_INPROGESS flag, masks it and flags it
> > > > PENDING. When the primary handler on the first CPU returns, it sees
> > > > the PENDING flag, unmasks and invokes the handler another time.
> > > In this case, when IRQ_INPROGRESS flag is set, on another CPU, it will
> > > mask and ack it, if before the primary handler on the first CPU returns,
> > > the edge interrupt is raised again, it will be lost, right?
> > Why will the interrupt be raised again?Is not it masked?I read tglx
> I means because it is masked, if at this time device issues edge irq,
> It will not be delivered and lost.

No, it is NOT lost. The irq is marked PENDING already, so we invoke
the handler again and handle it. And before we invoke the handler
another time we unmask it.

It does not matter at all whether the interrupt has been sent five
times while it was masked. What matters is that we recorded the first
one and set the PENDING flag. That way we invoke the interrupt handler
again and keep stuff rolling.

Thanks,

	tglx

RE: [PATCH] genirq: for edge interrupt IRQS_ONESHOT support with irq thread

[anish kumar]

On Fri, 2012-10-12 at 22:52 +0200, Thomas Gleixner wrote:
> On Fri, 12 Oct 2012, Liu, Chuansheng wrote:
> > > -----Original Message-----
> > > From: anish kumar [mailto:anish198519851985@gmail.com]
> > > Sent: Friday, October 12, 2012 11:25 PM
> > > To: Liu, Chuansheng
> > > Cc: Thomas Gleixner; linux-kernel@vger.kernel.org
> > > Subject: RE: [PATCH] genirq: for edge interrupt IRQS_ONESHOT support with irq
> > > thread
> > > 
> > > On Fri, 2012-10-12 at 14:57 +0000, Liu, Chuansheng wrote:
> > > > > On SMP an interrupt which is raised after the ack() again before the
> > > > > handler finishes, can invoke another delivery on a different CPU,
> > > > > which then sees the IRQ_INPROGESS flag, masks it and flags it
> > > > > PENDING. When the primary handler on the first CPU returns, it sees
> > > > > the PENDING flag, unmasks and invokes the handler another time.
> > > > In this case, when IRQ_INPROGRESS flag is set, on another CPU, it will
> > > > mask and ack it, if before the primary handler on the first CPU returns,
> > > > the edge interrupt is raised again, it will be lost, right?
> > > Why will the interrupt be raised again?Is not it masked?I read tglx
> > I means because it is masked, if at this time device issues edge irq,
> > It will not be delivered and lost.
> 
> No, it is NOT lost. The irq is marked PENDING already, so we invoke
It is fairly easy for an edge triggered interrupt to be missed - for
example if interrupts have to be masked for a period - and unless there
is some type of hardware latch that records the event it is impossible
to recover.
tglx, explanation will only work if we have a hardware latch which when
unmasked sends all those edge interrupts again (which had come when it
was masked while the CPU was handling the same interrupts).

PS:http://kernel.org/doc/htmldocs/genericirq.html
> the handler again and handle it. And before we invoke the handler
> another time we unmask it.
> 
> It does not matter at all whether the interrupt has been sent five
> times while it was masked. What matters is that we recorded the first
> one and set the PENDING flag. That way we invoke the interrupt handler
> again and keep stuff rolling.
> 
> Thanks,
> 
> 	tglx

RE: [PATCH] genirq: for edge interrupt IRQS_ONESHOT support with irq thread

[anish kumar]

On Sat, 2012-10-13 at 14:21 +0900, anish kumar wrote:
> On Fri, 2012-10-12 at 22:52 +0200, Thomas Gleixner wrote:
> > On Fri, 12 Oct 2012, Liu, Chuansheng wrote:
> > > > -----Original Message-----
> > > > From: anish kumar [mailto:anish198519851985@gmail.com]
> > > > Sent: Friday, October 12, 2012 11:25 PM
> > > > To: Liu, Chuansheng
> > > > Cc: Thomas Gleixner; linux-kernel@vger.kernel.org
> > > > Subject: RE: [PATCH] genirq: for edge interrupt IRQS_ONESHOT support with irq
> > > > thread
> > > > 
> > > > On Fri, 2012-10-12 at 14:57 +0000, Liu, Chuansheng wrote:
> > > > > > On SMP an interrupt which is raised after the ack() again before the
> > > > > > handler finishes, can invoke another delivery on a different CPU,
> > > > > > which then sees the IRQ_INPROGESS flag, masks it and flags it
> > > > > > PENDING. When the primary handler on the first CPU returns, it sees
> > > > > > the PENDING flag, unmasks and invokes the handler another time.
> > > > > In this case, when IRQ_INPROGRESS flag is set, on another CPU, it will
> > > > > mask and ack it, if before the primary handler on the first CPU returns,
> > > > > the edge interrupt is raised again, it will be lost, right?
> > > > Why will the interrupt be raised again?Is not it masked?I read tglx
> > > I means because it is masked, if at this time device issues edge irq,
> > > It will not be delivered and lost.
> > 
> > No, it is NOT lost. The irq is marked PENDING already, so we invoke
> It is fairly easy for an edge triggered interrupt to be missed - for
> example if interrupts have to be masked for a period - and unless there
> is some type of hardware latch that records the event it is impossible
> to recover.
> tglx, explanation will only work if we have a hardware latch which when
> unmasked sends all those edge interrupts again (which had come when it
> was masked while the CPU was handling the same interrupts).
> 
> PS:http://kernel.org/doc/htmldocs/genericirq.html
Hello tglx,
Does this explanation makes sense?
> > the handler again and handle it. And before we invoke the handler
> > another time we unmask it.
> > 
> > It does not matter at all whether the interrupt has been sent five
> > times while it was masked. What matters is that we recorded the first
> > one and set the PENDING flag. That way we invoke the interrupt handler
> > again and keep stuff rolling.
> > 
> > Thanks,
> > 
> > 	tglx
> 

RE: [PATCH] genirq: for edge interrupt IRQS_ONESHOT support with irq thread

[Thomas Gleixner]

On Fri, 12 Oct 2012, Liu, Chuansheng wrote:

> > On SMP an interrupt which is raised after the ack() again before the
> > handler finishes, can invoke another delivery on a different CPU,
> > which then sees the IRQ_INPROGESS flag, masks it and flags it
> > PENDING. When the primary handler on the first CPU returns, it sees
> > the PENDING flag, unmasks and invokes the handler another time.
> In this case, when IRQ_INPROGRESS flag is set, on another CPU, it will
> mask and ack it, if before the primary handler on the first CPU returns,
> the edge interrupt is raised again, it will be lost, right?
> So I think set PENDING just confirm one time, it just depends on primary handler
> execution time and irq frequency.

It does NOT matter. We marked it PENDING. And that guarantees that we
invoke the handler again after unmasking. And that keeps stuff rolling.

We would lose when we masked and did not notice that there was one,
but we notice and store that information with the PENDING flag.

RE: [PATCH] genirq: for edge interrupt IRQS_ONESHOT support with irq thread

[Liu, Chuansheng]

FLAG IRQS_ONESHOT does not work well for edge interrupt.
* IRQS_ONESHOT			- irq is not unmasked in primary handler

Is it possible give some notification or remind? Thanks. I took some time
to know it:)

irq/manage.c wrong comment( ? )

[anish kumar]

Hello tglx,

I just found the below inconsistency while going through the code.


kernel/irq/manage.c

    if (new->flags & IRQF_ONESHOT) {
        /*
         * The thread_mask for the action is or'ed to
         * desc->thread_active to indicate that the
         * IRQF_ONESHOT thread handler has been woken, but not
         * yet finished. The bit is cleared when a thread
         * completes. When all threads of a shared interr

Shouldn't this "desc->thread_active" be desc->threads_active ??

Re: irq/manage.c wrong comment( ? )

[anish kumar]

ping...
On Sat, 2012-10-13 at 00:32 +0900, anish kumar wrote:?
> Hello tglx,
> 
> I just found the below inconsistency while going through the code.
> 
> 
> kernel/irq/manage.c
> 
>     if (new->flags & IRQF_ONESHOT) {
>         /*
>          * The thread_mask for the action is or'ed to
>          * desc->thread_active to indicate that the
>          * IRQF_ONESHOT thread handler has been woken, but not
>          * yet finished. The bit is cleared when a thread
>          * completes. When all threads of a shared interr
> 
> Shouldn't this "desc->thread_active" be desc->threads_active ??

Re: irq/manage.c wrong comment( ? )

[Thomas Gleixner]

On Sun, 21 Oct 2012, anish kumar wrote:

> ping...

Oh well.

> On Sat, 2012-10-13 at 00:32 +0900, anish kumar wrote:?
> > Hello tglx,
> > 
> > I just found the below inconsistency while going through the code.
> > 
> > 
> > kernel/irq/manage.c
> > 
> >     if (new->flags & IRQF_ONESHOT) {
> >         /*
> >          * The thread_mask for the action is or'ed to
> >          * desc->thread_active to indicate that the
> >          * IRQF_ONESHOT thread handler has been woken, but not
> >          * yet finished. The bit is cleared when a thread
> >          * completes. When all threads of a shared interr
> > 
> > Shouldn't this "desc->thread_active" be desc->threads_active ??

That's obvious, right? Please send a proper patch if you think it's
really that important.

Thanks,

	tglx