[PATCH] genirq: Fix bad IRQ_ONSHOT in forced IRQ setting

[PATCH] genirq: Fix bad IRQ_ONSHOT in forced IRQ setting

[Kohji Okuno]

If handler and thread_fn have valid function pointers for each
in request_thread_irq(), IRQF_ONESHOT is set unnecessarily in
irq_setup_forces_threading() in case of threadirqs.
As this result, the IRQ handler will not be called.

Signed-off-by: Kohji Okuno <okuno.kohji@jp.panasonic.com>
---
 kernel/irq/manage.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index f9a59f6..759ce0f 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -991,12 +991,12 @@ static void irq_setup_forced_threading(struct irqaction *new)
 	if (new->flags & (IRQF_NO_THREAD | IRQF_PERCPU | IRQF_ONESHOT))
 		return;
 
-	new->flags |= IRQF_ONESHOT;
 
 	if (!new->thread_fn) {
 		set_bit(IRQTF_FORCED_THREAD, &new->thread_flags);
 		new->thread_fn = new->handler;
 		new->handler = irq_default_primary_handler;
+		new->flags |= IRQF_ONESHOT;
 	}
 }
 
-- 
1.9.1

Re: [PATCH] genirq: Fix bad IRQ_ONSHOT in forced IRQ setting

[Thomas Gleixner]

On Thu, 17 Sep 2015, Kohji Okuno wrote:
> If handler and thread_fn have valid function pointers for each
> in request_thread_irq(), IRQF_ONESHOT is set unnecessarily in
> irq_setup_forces_threading() in case of threadirqs.
> As this result, the IRQ handler will not be called.

That explanation does not make sense. Which handler is not called?

Thanks,

	tglx

Re: [PATCH] genirq: Fix bad IRQ_ONSHOT in forced IRQ setting

[Kohji Okuno]

> On Thu, 17 Sep 2015, Kohji Okuno wrote:
>> If handler and thread_fn have valid function pointers for each
>> in request_thread_irq(), IRQF_ONESHOT is set unnecessarily in
>> irq_setup_forces_threading() in case of threadirqs.
>> As this result, the IRQ handler will not be called.
> 
> That explanation does not make sense. Which handler is not called?
> 
> Thanks,
> 
> 	tglx

Hi tglx,

Please refer to drivers/mmc/host/sdhci.c. sdhci uses sdhci_irq and
sdhci_thread_irq as the followings.

3366         sdhci_init(host, 0);
3367 
3368         ret = request_threaded_irq(host->irq, sdhci_irq, sdhci_thread_irq,
3369                                    IRQF_SHARED, mmc_hostname(mmc), host);

When I tested SDIO card, sdhci_irq() was not called after 1st SDIO interrupt.
After I applied my patch, this worked good.

Best regards,
 Kohji Okuno

Re: [PATCH] genirq: Fix bad IRQ_ONSHOT in forced IRQ setting

[Kohji Okuno]

From: Kohji Okuno <okuno.kohji@jp.panasonic.com>
Date: Thu, 17 Sep 2015 17:21:08 +0900
>> On Thu, 17 Sep 2015, Kohji Okuno wrote:
>>> If handler and thread_fn have valid function pointers for each
>>> in request_thread_irq(), IRQF_ONESHOT is set unnecessarily in
>>> irq_setup_forces_threading() in case of threadirqs.
>>> As this result, the IRQ handler will not be called.
>> 
>> That explanation does not make sense. Which handler is not called?
>> 
>> Thanks,
>> 
>> 	tglx
> 
> Hi tglx,
> 
> Please refer to drivers/mmc/host/sdhci.c. sdhci uses sdhci_irq and
> sdhci_thread_irq as the followings.
> 
> 3366         sdhci_init(host, 0);
> 3367 
> 3368         ret = request_threaded_irq(host->irq, sdhci_irq, sdhci_thread_irq,
> 3369                                    IRQF_SHARED, mmc_hostname(mmc), host);
> 
> When I tested SDIO card, sdhci_irq() was not called after 1st SDIO interrupt.
> After I applied my patch, this worked good.
> 
> Best regards,
>  Kohji Okuno

Hi tglx,

When 1st sdio IRQ is happend, sdhci_irq() returns IRQ_WAKE_THREAD.
After this, sdhci_irq() is not called in case of threadirqs.

Best regards,
 Kohji Okuno

Re: [PATCH] genirq: Fix bad IRQ_ONSHOT in forced IRQ setting

[Thomas Gleixner]

On Thu, 17 Sep 2015, Kohji Okuno wrote:
>
> When 1st sdio IRQ is happend, sdhci_irq() returns IRQ_WAKE_THREAD.
> After this, sdhci_irq() is not called in case of threadirqs.

What kind of system is that?

Can you provide the output of /proc/interrupts please?

I think your patch is fine. I just want to understand why we don't see
any more interrupts.

Thanks,

	tglx

Re: [PATCH] genirq: Fix bad IRQ_ONSHOT in forced IRQ setting

[Kohji Okuno]

From: Thomas Gleixner <tglx@linutronix.de>
Date: Thu, 17 Sep 2015 23:10:02 +0200
> On Thu, 17 Sep 2015, Kohji Okuno wrote:
>>
>> When 1st sdio IRQ is happend, sdhci_irq() returns IRQ_WAKE_THREAD.
>> After this, sdhci_irq() is not called in case of threadirqs.
> 
> What kind of system is that?
> 
> Can you provide the output of /proc/interrupts please?
> 
> I think your patch is fine. I just want to understand why we don't see
> any more interrupts.
> 
> Thanks,
> 
> 	tglx

Hi tglx,

My system is arm, and I connect SDIO WiFi card.
In fact, I use kernel 3.18.11 base. But, I think sources concerned
with this are same. 

This is my "/proc/interrupts".

           CPU0       CPU1       CPU2       CPU3
 46:      20672          0          0          0       GIC  46  mmc1

In drivers/irqchip/irq-gic.c:gic_set_type(), irq46 is set as
IRQ_TYPE_LEVEL_HIGH.

Best regards,
 Kohji Okuno

Re: [PATCH] genirq: Fix bad IRQ_ONSHOT in forced IRQ setting

[Thomas Gleixner]

On Fri, 18 Sep 2015, Kohji Okuno wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
> Date: Thu, 17 Sep 2015 23:10:02 +0200
> > On Thu, 17 Sep 2015, Kohji Okuno wrote:
> >>
> >> When 1st sdio IRQ is happend, sdhci_irq() returns IRQ_WAKE_THREAD.
> >> After this, sdhci_irq() is not called in case of threadirqs.
> > 
> > What kind of system is that?
> > 
> > Can you provide the output of /proc/interrupts please?
> > 
> > I think your patch is fine. I just want to understand why we don't see
> > any more interrupts.
> > 
> > Thanks,
> > 
> > 	tglx
> 
> Hi tglx,
> 
> My system is arm, and I connect SDIO WiFi card.
> In fact, I use kernel 3.18.11 base. But, I think sources concerned
> with this are same. 
> 
> This is my "/proc/interrupts".
> 
>            CPU0       CPU1       CPU2       CPU3
>  46:      20672          0          0          0       GIC  46  mmc1
> 
> In drivers/irqchip/irq-gic.c:gic_set_type(), irq46 is set as
> IRQ_TYPE_LEVEL_HIGH.

That's weird. The flow is:

interrupt()
  mask()
  ret = primary_handler()
  if (ret == WAKE_THREAD)
    wake_thread()
  else
    unmask()

thread_handler()
  ....
  unmask()

So if an interrupt is triggered on the device while the interrupt is
masked it should be raised again immediately when the unmask happens
because its level type.

I'm wondering why that doesn't work.

Thanks,

	tglx

Re: [PATCH] genirq: Fix bad IRQ_ONSHOT in forced IRQ setting

[Kohji Okuno]

From: Thomas Gleixner <tglx@linutronix.de>
Date: Fri, 18 Sep 2015 11:04:23 +0200
> That's weird. The flow is:
> 
> interrupt()
>   mask()
>   ret = primary_handler()
>   if (ret == WAKE_THREAD)
>     wake_thread()
>   else
>     unmask()
> 
> thread_handler()
>   ....
>   unmask()
> 
> So if an interrupt is triggered on the device while the interrupt is
> masked it should be raised again immediately when the unmask happens
> because its level type.
> 
> I'm wondering why that doesn't work.

Yes. I think so. And, I have just found that sdhci_thread_irq() don't
finish in this case. I'm analyzing about this now. But, after I apply
my patch, sdhci_thread_irq() can finish. I will share the result with
you.

Best regards,
 Kohji Okuno

Re: [PATCH] genirq: Fix bad IRQ_ONSHOT in forced IRQ setting

[Marc Zyngier]

On Fri, 18 Sep 2015 18:22:07 +0900
Kohji Okuno <okuno.kohji@jp.panasonic.com> wrote:

> From: Thomas Gleixner <tglx@linutronix.de>
> Date: Fri, 18 Sep 2015 11:04:23 +0200
> > That's weird. The flow is:
> > 
> > interrupt()
> >   mask()
> >   ret = primary_handler()
> >   if (ret == WAKE_THREAD)
> >     wake_thread()
> >   else
> >     unmask()
> > 
> > thread_handler()
> >   ....
> >   unmask()
> > 
> > So if an interrupt is triggered on the device while the interrupt is
> > masked it should be raised again immediately when the unmask happens
> > because its level type.
> > 
> > I'm wondering why that doesn't work.
> 
> Yes. I think so. And, I have just found that sdhci_thread_irq() don't
> finish in this case. I'm analyzing about this now. But, after I apply
> my patch, sdhci_thread_irq() can finish. I will share the result with
> you.

What do you mean exactly by "don't finish"? Does it hang somewhere? Or
keeps processing data but never drains?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny.

Re: [PATCH] genirq: Fix bad IRQ_ONSHOT in forced IRQ setting

[Kohji Okuno]

From: Marc Zyngier <marc.zyngier@arm.com>
Date: Fri, 18 Sep 2015 11:03:12 +0100
> On Fri, 18 Sep 2015 18:22:07 +0900
> Kohji Okuno <okuno.kohji@jp.panasonic.com> wrote:
> 
>> From: Thomas Gleixner <tglx@linutronix.de>
>> Date: Fri, 18 Sep 2015 11:04:23 +0200
>> > That's weird. The flow is:
>> > 
>> > interrupt()
>> >   mask()
>> >   ret = primary_handler()
>> >   if (ret == WAKE_THREAD)
>> >     wake_thread()
>> >   else
>> >     unmask()
>> > 
>> > thread_handler()
>> >   ....
>> >   unmask()
>> > 
>> > So if an interrupt is triggered on the device while the interrupt is
>> > masked it should be raised again immediately when the unmask happens
>> > because its level type.
>> > 
>> > I'm wondering why that doesn't work.
>> 
>> Yes. I think so. And, I have just found that sdhci_thread_irq() don't
>> finish in this case. I'm analyzing about this now. But, after I apply
>> my patch, sdhci_thread_irq() can finish. I will share the result with
>> you.
> 
> What do you mean exactly by "don't finish"? Does it hang somewhere? Or
> keeps processing data but never drains?
> 
> Thanks,
> 
> 	M.

Hi Marc and tglx,

irq thread(runs sdhci_thread_irq()) is waiting on
drivers/mmc/core/core.c:mmc_wait_for_req_done() in order to access
a SDIO register. And, this thread shoud be woken up from
sdhci_irq() after the completion of the register access.
But, since the IRQ is masked, sdhci_irq() is not called and  irq
thread can not wake up. This is root cause, I think.
What do you think about this?

Best regards,
 Kohji Okuno

Re: [PATCH] genirq: Fix bad IRQ_ONSHOT in forced IRQ setting

[Thomas Gleixner]

On Fri, 18 Sep 2015, Kohji Okuno wrote:
> 
> irq thread(runs sdhci_thread_irq()) is waiting on
> drivers/mmc/core/core.c:mmc_wait_for_req_done() in order to access
> a SDIO register. And, this thread shoud be woken up from
> sdhci_irq() after the completion of the register access.
> But, since the IRQ is masked, sdhci_irq() is not called and  irq
> thread can not wake up. This is root cause, I think.
> What do you think about this?

Ah, that explains it. Let me think about it a bit.

Thanks,

	tglx

Re: [PATCH] genirq: Fix bad IRQ_ONSHOT in forced IRQ setting

[Kohji Okuno]

From: Thomas Gleixner <tglx@linutronix.de>
Date: Fri, 18 Sep 2015 16:46:24 +0200
> On Fri, 18 Sep 2015, Kohji Okuno wrote:
>> 
>> irq thread(runs sdhci_thread_irq()) is waiting on
>> drivers/mmc/core/core.c:mmc_wait_for_req_done() in order to access
>> a SDIO register. And, this thread shoud be woken up from
>> sdhci_irq() after the completion of the register access.
>> But, since the IRQ is masked, sdhci_irq() is not called and  irq
>> thread can not wake up. This is root cause, I think.
>> What do you think about this?
> 
> Ah, that explains it. Let me think about it a bit.
> 
> Thanks,
> 
> 	tglx

If something is not clear please let me know.
By the way, even if other issue exists, we should change
irq_setup_forced_threading() as my patch, I think.

Thanks,
 Kohji Okuno

Re: [PATCH] genirq: Fix bad IRQ_ONSHOT in forced IRQ setting

[Thomas Gleixner]

On Sun, 20 Sep 2015, Kohji Okuno wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
> Date: Fri, 18 Sep 2015 16:46:24 +0200
> > On Fri, 18 Sep 2015, Kohji Okuno wrote:
> >> 
> >> irq thread(runs sdhci_thread_irq()) is waiting on
> >> drivers/mmc/core/core.c:mmc_wait_for_req_done() in order to access
> >> a SDIO register. And, this thread shoud be woken up from
> >> sdhci_irq() after the completion of the register access.
> >> But, since the IRQ is masked, sdhci_irq() is not called and  irq
> >> thread can not wake up. This is root cause, I think.
> >> What do you think about this?
> > 
> > Ah, that explains it. Let me think about it a bit.
> 
> If something is not clear please let me know.
> By the way, even if other issue exists, we should change
> irq_setup_forced_threading() as my patch, I think.

No, we can't. We can only do that if the interrupt is not shared.

Assume the following scenario:

       request_irq(irq, handler, IRQF_SHARED, "devA", devidA);

In case of force threading that sets the oneshot flag for the irq
descriptor.

Now the sdhci driver is initialized

       request_thread_irq(irq, handler, thandler, IRQF_SHARED,
       			  "sdhci", devidSDHCI);

The oneshot flag sticks and you run into exactly the same issue as
now.

Shared interrupts are a major pain, but we have to deal with them.

I'm working on a solution for that issue.

Thanks,

	tglx

Re: [PATCH] genirq: Fix bad IRQ_ONSHOT in forced IRQ setting

[Kohji Okuno]

From: Thomas Gleixner <tglx@linutronix.de>
Date: Sat, 19 Sep 2015 22:24:15 +0200
> No, we can't. We can only do that if the interrupt is not shared.
> Assume the following scenario:
>        request_irq(irq, handler, IRQF_SHARED, "devA", devidA);
> In case of force threading that sets the oneshot flag for the irq
> descriptor.
> Now the sdhci driver is initialized
>        request_thread_irq(irq, handler, thandler, IRQF_SHARED,
>        			  "sdhci", devidSDHCI);
> The oneshot flag sticks and you run into exactly the same issue as
> now.
> Shared interrupts are a major pain, but we have to deal with them.
> I'm working on a solution for that issue.
> 
> Thanks,
> 	tglx

I understood. If you could prepare the patch, I can try it.

Thanks,
 Kohji Okuno