RE: [PATCH] genirq: Add the IRQS_ONESHOT support for edge interrupt

RE: [PATCH] genirq: Add the IRQS_ONESHOT support for edge interrupt

[Liu, Chuansheng]

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

> Just curios about the problem you are facing without this code?
The issue I meet is in my calling request_threaded_irq()[edge interrupt], even with IRQS_ONESHOT,
When irq thread is handling, the interrupt is still coming, and the primary handler is called,
It cause possible spin recursive locks if irq handler and irq thread use the same spin lock.

> -----Original Message-----
> From: anish singh [mailto:anish198519851985@gmail.com]
> Sent: Tuesday, September 18, 2012 12:31 PM
> To: Liu, Chuansheng
> Subject: Re: [PATCH] genirq: Add the IRQS_ONESHOT support for edge interrupt
> 
> On Tue, Sep 18, 2012 at 6:24 PM, Chuansheng Liu
> <chuansheng.liu@intel.com> wrote:
> > In handle_edge_irq(), currently do not care about the flag IRQS_ONESHOT,
> > but there are many edge interrupt handler with irq thread need it indeed,
> > so implement here.
> Just curios about the problem you are facing without this code?
> >
> > 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 eebd6d5..8e4e49a 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();
> > +               goto out_unlock;
> > +       } else
> > +               desc->irq_data.chip->irq_ack(&desc->irq_data);
> >
> >         do {
> >                 if (unlikely(!desc->action)) {
> > --
> > 1.7.0.4
> >
> >
> >
> > --
> > 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: Add the IRQS_ONESHOT support for edge interrupt

[anish singh]

On Tue, Sep 18, 2012 at 10:21 AM, Liu, Chuansheng
<chuansheng.liu@intel.com> wrote:
>> Just curios about the problem you are facing without this code?
> The issue I meet is in my calling request_threaded_irq()[edge interrupt], even with IRQS_ONESHOT,
> When irq thread is handling, the interrupt is still coming, and the primary handler is called,
> It cause possible spin recursive locks if irq handler and irq thread use the same spin lock.

Are you sure you have not returned from the irq_thread and how do you
know that primary handler is called in between when your irq_thread is running?

This comment might help.
kernel/irq/manage.c

    } else if (new->handler == irq_default_primary_handler) {

        /*
         * The interrupt was requested with handler = NULL, so
         * we use the default primary handler for it. But it
         * does not have the oneshot flag set. In combination
         * with level interrupts this is deadly, because the
         * default primary handler just wakes the thread, then
         * the irq lines is reenabled, but the device still
         * has the level irq asserted. Rinse and repeat....
         *
         * While this works for edge type interrupts, we play
         * it safe and reject unconditionally because we can't
         * say for sure which type this interrupt really
         * has. The type flags are unreliable as the
         * underlying chip implementation can override them.
         */

Though I am not an expert on interrupt handling from the code
it looks like this scenario is already taken care of.

Just one more query from someone who knows this code:
Looks to me to be a spelling problem.I don't want to change the code
but just wants to know.

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

>
>> -----Original Message-----
>> From: anish singh [mailto:anish198519851985@gmail.com]
>> Sent: Tuesday, September 18, 2012 12:31 PM
>> To: Liu, Chuansheng
>> Subject: Re: [PATCH] genirq: Add the IRQS_ONESHOT support for edge interrupt
>>
>> On Tue, Sep 18, 2012 at 6:24 PM, Chuansheng Liu
>> <chuansheng.liu@intel.com> wrote:
>> > In handle_edge_irq(), currently do not care about the flag IRQS_ONESHOT,
>> > but there are many edge interrupt handler with irq thread need it indeed,
>> > so implement here.
>> Just curios about the problem you are facing without this code?
>> >
>> > 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 eebd6d5..8e4e49a 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();
>> > +               goto out_unlock;
>> > +       } else
>> > +               desc->irq_data.chip->irq_ack(&desc->irq_data);
>> >
>> >         do {
>> >                 if (unlikely(!desc->action)) {
>> > --
>> > 1.7.0.4
>> >
>> >
>> >
>> > --
>> > 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: Add the IRQS_ONESHOT support for edge interrupt

[Liu, Chuansheng]

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

> Are you sure you have not returned from the irq_thread and how do you
> know that primary handler is called in between when your irq_thread is
> running?

I am sure because the spin recursive locks has been printed with call stack,
further more, with IRQS_ONESHOT, I have printed the value of irqd_irq_masked(&desc->irq_data),
it is 0, that means when with IRQS_ONESHOT, and the irq thread is called but the irq is not masked during this time.
ÿôèº{.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: Add the IRQS_ONESHOT support for edge interrupt

[Liu, Chuansheng]

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

> This comment might help.
> kernel/irq/manage.c
> 
>     } else if (new->handler == irq_default_primary_handler) {
> 
>         /*
>          * The interrupt was requested with handler = NULL, so
>          * we use the default primary handler for it. But it
>          * does not have the oneshot flag set. In combination
>          * with level interrupts this is deadly, because the
>          * default primary handler just wakes the thread, then
>          * the irq lines is reenabled, but the device still
>          * has the level irq asserted. Rinse and repeat....
>          *
>          * While this works for edge type interrupts, we play
>          * it safe and reject unconditionally because we can't
>          * say for sure which type this interrupt really
>          * has. The type flags are unreliable as the
>          * underlying chip implementation can override them.
>          */

This is not my case. My request_irq is as below:
request_threaded_irq(drv_data->irq,
					  func1,
					  func2,
					  IRQF_SHARED|IRQS_ONESHOT ,
					  "AAAA",
					  drv_data);
ÿôèº{.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: Add the IRQS_ONESHOT support for edge interrupt

[anish singh]

On Tue, Sep 18, 2012 at 12:51 PM, Liu, Chuansheng
<chuansheng.liu@intel.com> wrote:
>> This comment might help.
>> kernel/irq/manage.c
>>
>>     } else if (new->handler == irq_default_primary_handler) {
>>
>>         /*
>>          * The interrupt was requested with handler = NULL, so
>>          * we use the default primary handler for it. But it
>>          * does not have the oneshot flag set. In combination
>>          * with level interrupts this is deadly, because the
>>          * default primary handler just wakes the thread, then
>>          * the irq lines is reenabled, but the device still
>>          * has the level irq asserted. Rinse and repeat....
>>          *
>>          * While this works for edge type interrupts, we play
>>          * it safe and reject unconditionally because we can't
>>          * say for sure which type this interrupt really
>>          * has. The type flags are unreliable as the
>>          * underlying chip implementation can override them.
>>          */
>
> This is not my case. My request_irq is as below:
> request_threaded_irq(drv_data->irq,
>                                           func1,
>                                           func2,
>                                           IRQF_SHARED|IRQS_ONESHOT ,
>                                           "AAAA",
>                                           drv_data);

I hope you are disabling the interrupt in your primary handler and returning
IRQ_WAKE_THREAD.
If yes then someone with experience in this interrupt handling code
can help you.

[PATCH] genirq: Add the IRQS_ONESHOT support for edge interrupt

[Chuansheng Liu]

In handle_edge_irq(), currently do not care about the flag IRQS_ONESHOT,
but there are many edge interrupt handler with irq thread need it indeed,
so implement here.

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 eebd6d5..8e4e49a 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();
+		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: Add the IRQS_ONESHOT support for edge interrupt

[Chuansheng Liu]

Sorry, update the patch.

On Tue, 2012-09-18 at 20:54 +0800, Chuansheng Liu wrote:
> In handle_edge_irq(), currently do not care about the flag IRQS_ONESHOT,
> but there are many edge interrupt handler with irq thread need it indeed,
> so implement here.
> 
> Signed-off-by: liu chuansheng <chuansheng.liu@intel.com>
> ---
>  kernel/irq/chip.c |    8 +++++++-
>  1 files changed, 7 insertions(+), 1 deletions(-)
> 

In handle_edge_irq(), currently do not care about the flag IRQS_ONESHOT,
but there are many edge interrupt handler with irq thread need it indeed,
so implement here.

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

diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
old mode 100644
new mode 100755
index eebd6d5..04da0d7
--- 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