Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1226717 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH] genirq: Fix bad IRQ_ONSHOT in forced IRQ setting Kohji Okuno <okuno.kohji@jp.panasonic.com> - 2015-09-17 09:40 +0200
    Re: [PATCH] genirq: Fix bad IRQ_ONSHOT in forced IRQ setting Thomas Gleixner <tglx@linutronix.de> - 2015-09-17 10:10 +0200
      Re: [PATCH] genirq: Fix bad IRQ_ONSHOT in forced IRQ setting Kohji Okuno <okuno.kohji@jp.panasonic.com> - 2015-09-17 10:30 +0200
        Re: [PATCH] genirq: Fix bad IRQ_ONSHOT in forced IRQ setting Kohji Okuno <okuno.kohji@jp.panasonic.com> - 2015-09-17 11:50 +0200
          Re: [PATCH] genirq: Fix bad IRQ_ONSHOT in forced IRQ setting Thomas Gleixner <tglx@linutronix.de> - 2015-09-17 23:20 +0200
            Re: [PATCH] genirq: Fix bad IRQ_ONSHOT in forced IRQ setting Kohji Okuno <okuno.kohji@jp.panasonic.com> - 2015-09-18 02:40 +0200
              Re: [PATCH] genirq: Fix bad IRQ_ONSHOT in forced IRQ setting Thomas Gleixner <tglx@linutronix.de> - 2015-09-18 11:10 +0200
                Re: [PATCH] genirq: Fix bad IRQ_ONSHOT in forced IRQ setting Kohji Okuno <okuno.kohji@jp.panasonic.com> - 2015-09-18 11:30 +0200
                  Re: [PATCH] genirq: Fix bad IRQ_ONSHOT in forced IRQ setting Marc Zyngier <marc.zyngier@arm.com> - 2015-09-18 12:10 +0200
                    Re: [PATCH] genirq: Fix bad IRQ_ONSHOT in forced IRQ setting Kohji Okuno <okuno.kohji@jp.panasonic.com> - 2015-09-18 13:00 +0200
                      Re: [PATCH] genirq: Fix bad IRQ_ONSHOT in forced IRQ setting Thomas Gleixner <tglx@linutronix.de> - 2015-09-18 16:50 +0200
                        Re: [PATCH] genirq: Fix bad IRQ_ONSHOT in forced IRQ setting Kohji Okuno <okuno.kohji@jp.panasonic.com> - 2015-09-19 17:40 +0200
                          Re: [PATCH] genirq: Fix bad IRQ_ONSHOT in forced IRQ setting Thomas Gleixner <tglx@linutronix.de> - 2015-09-19 22:30 +0200
                            Re: [PATCH] genirq: Fix bad IRQ_ONSHOT in forced IRQ setting Kohji Okuno <okuno.kohji@jp.panasonic.com> - 2015-09-20 02:30 +0200

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

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

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

[toc] | [next] | [standalone]

#1226732

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

[toc] | [prev] | [next] | [standalone]

#1226748

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

[toc] | [prev] | [next] | [standalone]

#1226818

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

[toc] | [prev] | [next] | [standalone]

#1227402

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

[toc] | [prev] | [next] | [standalone]

#1227500

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

[toc] | [prev] | [next] | [standalone]

#1227691

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


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

[toc] | [prev] | [next] | [standalone]

#1227710

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

[toc] | [prev] | [next] | [standalone]

#1227738

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

[toc] | [prev] | [next] | [standalone]

#1227767

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

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

[toc] | [prev] | [next] | [standalone]

#1227949

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

[toc] | [prev] | [next] | [standalone]

#1228463

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

[toc] | [prev] | [next] | [standalone]

#1228748

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

[toc] | [prev] | [next] | [standalone]

#1228789

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web