[PATCH] irq: Add new flag to ack level-triggered interrupts before unmasking

[PATCH] irq: Add new flag to ack level-triggered interrupts before unmasking

[Carlo Caione]

Several irqchip drivers require the level-triggered interrupt to be
acked before unmasking to avoid that a second interrupt is immediately
triggered. This small patch introduces a new irqchip flags that is used
to ack the IRQ line before it is unmasked.

Signed-off-by: Carlo Caione <carlo@caione.org>
---
 include/linux/irq.h    |  2 ++
 kernel/irq/chip.c      | 10 ++++++++++
 kernel/irq/internals.h |  1 +
 kernel/irq/manage.c    |  2 +-
 4 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/include/linux/irq.h b/include/linux/irq.h
index 7dc1003..2cbe8d1 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -349,6 +349,7 @@ struct irq_chip {
  * IRQCHIP_ONOFFLINE_ENABLED:	Only call irq_on/off_line callbacks
  *				when irq enabled
  * IRQCHIP_SKIP_SET_WAKE:	Skip chip.irq_set_wake(), for this irq chip
+ * IRQCHIP_ACK_ON_UNMASK:	Ack level interrupts right before unmask
  */
 enum {
 	IRQCHIP_SET_TYPE_MASKED		= (1 <<  0),
@@ -357,6 +358,7 @@ enum {
 	IRQCHIP_ONOFFLINE_ENABLED	= (1 <<  3),
 	IRQCHIP_SKIP_SET_WAKE		= (1 <<  4),
 	IRQCHIP_ONESHOT_SAFE		= (1 <<  5),
+	IRQCHIP_ACK_ON_UNMASK		= (1 <<  6),
 };
 
 /* This include will go away once we isolated irq_desc usage to core code */
diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index dc04c16..9d2d2e2 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -265,6 +265,16 @@ static inline void mask_ack_irq(struct irq_desc *desc)
 	irq_state_set_masked(desc);
 }
 
+void ack_unmask_irq(struct irq_desc *desc)
+{
+	if ((desc->irq_data.chip->flags & IRQCHIP_ACK_ON_UNMASK) &&
+	    (irqd_get_trigger_type(&desc->irq_data) & IRQ_TYPE_LEVEL_MASK) &&
+	    desc->irq_data.chip->irq_ack)
+			desc->irq_data.chip->irq_ack(&desc->irq_data);
+
+	unmask_irq(desc);
+}
+
 void mask_irq(struct irq_desc *desc)
 {
 	if (desc->irq_data.chip->irq_mask) {
diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h
index 001fa5b..06ff850 100644
--- a/kernel/irq/internals.h
+++ b/kernel/irq/internals.h
@@ -73,6 +73,7 @@ extern void irq_percpu_enable(struct irq_desc *desc, unsigned int cpu);
 extern void irq_percpu_disable(struct irq_desc *desc, unsigned int cpu);
 extern void mask_irq(struct irq_desc *desc);
 extern void unmask_irq(struct irq_desc *desc);
+extern void ack_unmask_irq(struct irq_desc *desc);
 
 extern void init_kstat_irqs(struct irq_desc *desc, int node, int nr);
 
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 481a13c..39505d8 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -718,7 +718,7 @@ again:
 
 	if (!desc->threads_oneshot && !irqd_irq_disabled(&desc->irq_data) &&
 	    irqd_irq_masked(&desc->irq_data))
-		unmask_irq(desc);
+		ack_unmask_irq(desc);
 
 out_unlock:
 	raw_spin_unlock_irq(&desc->lock);
-- 
1.8.5.3

Re: [PATCH] irq: Add new flag to ack level-triggered interrupts before unmasking

[Thomas Gleixner]

On Thu, 6 Feb 2014, Carlo Caione wrote:

> Several irqchip drivers require the level-triggered interrupt to be
> acked before unmasking to avoid that a second interrupt is immediately
> triggered. This small patch introduces a new irqchip flags that is used
> to ack the IRQ line before it is unmasked.

And why are you not doing this in the unmask function of the affected
chip in the first place?
 
Thanks,

	tglx

Re: [linux-sunxi] Re: [PATCH] irq: Add new flag to ack level-triggered interrupts before unmasking

[Carlo Caione]

On Thu, Feb 6, 2014 at 10:14 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Thu, 6 Feb 2014, Carlo Caione wrote:
>
>> Several irqchip drivers require the level-triggered interrupt to be
>> acked before unmasking to avoid that a second interrupt is immediately
>> triggered. This small patch introduces a new irqchip flags that is used
>> to ack the IRQ line before it is unmasked.
>
> And why are you not doing this in the unmask function of the affected
> chip in the first place?

Because this is a common behavior of several irqchips (sunxi NMI
controller,  exynos, etc...) so I think it should be useful to have it
in the core instead of replicating the same code structure in all the
irqchip drivers.

Thanks,

-- 
Carlo Caione

Re: [linux-sunxi] Re: [PATCH] irq: Add new flag to ack level-triggered interrupts before unmasking

[Thomas Gleixner]

On Thu, 6 Feb 2014, Carlo Caione wrote:
> On Thu, Feb 6, 2014 at 10:14 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > On Thu, 6 Feb 2014, Carlo Caione wrote:
> >
> >> Several irqchip drivers require the level-triggered interrupt to be
> >> acked before unmasking to avoid that a second interrupt is immediately
> >> triggered. This small patch introduces a new irqchip flags that is used
> >> to ack the IRQ line before it is unmasked.
> >
> > And why are you not doing this in the unmask function of the affected
> > chip in the first place?
> 
> Because this is a common behavior of several irqchips (sunxi NMI
> controller,  exynos, etc...) so I think it should be useful to have it
> in the core instead of replicating the same code structure in all the
> irqchip drivers.

I'm all for making stuff generic, but you introduce this gem

+void ack_unmask_irq(struct irq_desc *desc)
+{
+       if ((desc->irq_data.chip->flags & IRQCHIP_ACK_ON_UNMASK) &&
+           (irqd_get_trigger_type(&desc->irq_data) & IRQ_TYPE_LEVEL_MASK) &&
+           desc->irq_data.chip->irq_ack)

This is totally backwards.

1) If level and edge are handled differently then you should provide
   different chips.

2) If a chip has IRQCHIP_ACK_ON_UNMASK set, then it better provides an
   irq_ack callback.

So why do you need this complex conditional?

+                       desc->irq_data.chip->irq_ack(&desc->irq_data);
+
+       unmask_irq(desc);
+}

Now the even more confusing part is a single call site in
irq_finalize_oneshot()

        if (!desc->threads_oneshot && !irqd_irq_disabled(&desc->irq_data) &&
            irqd_irq_masked(&desc->irq_data))
-               unmask_irq(desc);
+               ack_unmask_irq(desc);


But you completely fail to explain the rationale. 

- Why is this only an issue for the threaded irq case? 

- Why are other sites where interrupts are masked/unmasked not
  affected?

  IOW, why is the handle_level_irq() logic for a non threaded
  interrupt different from the threaded case?

  In the non threaded case we do:

  interrupt();
  mask_ack();
  handle_irq();
  unmask();
  reti();

  In the threaded case:

  interrupt();
  mask_ack();
  wake_thread();
  reti();
  run_thread();
  handle_irq();
  unmask();

  The difference between those scenarios is:

  1) The timing is different

  2) In the threaded case we return from the exception with the irq
     line masked and reenable it later after the threaded handler has
     run.

Do you have any sensible explanation for that requirement to ack
before unmask while you already acked on mask? And why this is only an
issue in the threaded case?

What's the context of the problem you are trying to solve?

Thanks,

	tglx

Re: [linux-sunxi] Re: [PATCH] irq: Add new flag to ack level-triggered interrupts before unmasking

[Carlo Caione]

On Thu, Feb 6, 2014 at 10:54 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Thu, 6 Feb 2014, Carlo Caione wrote:
>> On Thu, Feb 6, 2014 at 10:14 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
>> > On Thu, 6 Feb 2014, Carlo Caione wrote:
>> >
>> >> Several irqchip drivers require the level-triggered interrupt to be
>> >> acked before unmasking to avoid that a second interrupt is immediately
>> >> triggered. This small patch introduces a new irqchip flags that is used
>> >> to ack the IRQ line before it is unmasked.
>> >
>> > And why are you not doing this in the unmask function of the affected
>> > chip in the first place?
>>
>> Because this is a common behavior of several irqchips (sunxi NMI
>> controller,  exynos, etc...) so I think it should be useful to have it
>> in the core instead of replicating the same code structure in all the
>> irqchip drivers.
>
> I'm all for making stuff generic, but you introduce this gem
>
> +void ack_unmask_irq(struct irq_desc *desc)
> +{
> +       if ((desc->irq_data.chip->flags & IRQCHIP_ACK_ON_UNMASK) &&
> +           (irqd_get_trigger_type(&desc->irq_data) & IRQ_TYPE_LEVEL_MASK) &&
> +           desc->irq_data.chip->irq_ack)
>
> This is totally backwards.
>
> 1) If level and edge are handled differently then you should provide
>    different chips.

Agree. I was trying to use the flag to trigger a different behavior
since AFAIK the problem of the IRQ retrigger is specific for
level-triggered interrupts.

> 2) If a chip has IRQCHIP_ACK_ON_UNMASK set, then it better provides an
>    irq_ack callback.

The check on irq_ack is done to be sure that the irq_ack callback is
really defined by the irqchip driver before calling it.

> So why do you need this complex conditional?

I wanted to be sure to be in the right case: level-triggered interrupt
with the flag defined in the driver (and the ack callback correctly
defined).
To the best of my knowledge this is the only case in which a flag like
 IRQCHIP_ACK_ON_UNMASK could make sense.

> +                       desc->irq_data.chip->irq_ack(&desc->irq_data);
> +
> +       unmask_irq(desc);
> +}
>
> Now the even more confusing part is a single call site in
> irq_finalize_oneshot()
>
>         if (!desc->threads_oneshot && !irqd_irq_disabled(&desc->irq_data) &&
>             irqd_irq_masked(&desc->irq_data))
> -               unmask_irq(desc);
> +               ack_unmask_irq(desc);
>
>
> But you completely fail to explain the rationale.

You are right. I'll try to clarify it,

> - Why is this only an issue for the threaded irq case?
>
> - Why are other sites where interrupts are masked/unmasked not
>   affected?

<snip>

> Do you have any sensible explanation for that requirement to ack
> before unmask while you already acked on mask? And why this is only an
> issue in the threaded case?
>
> What's the context of the problem you are trying to solve?

The context and the rationale is fully explained in this thread
http://www.spinics.net/lists/arm-kernel/msg299952.html and some
answers are already given along the thread especially by Hans here
http://www.spinics.net/lists/arm-kernel/msg303542.html
Shortly, the double interrupt problem as seen on sunxi NMI controller
(and other chips AFAIK) is specific for level-triggered interrupts
when the hard interrupt handler is not able to ACK the interrupts on
the originating device since this device can only be accessed via a
bus (in our case it was a PMIC on I2C).
This explains why my patch was specific for the threaded case and why
the ACK on mask is not really effective in actually acking the IRQs
(so that I need a second ACK before unmasking the line). In fact in
our case (PMIC connected via I2C with an interrupt line on a special
NMI controller) the implicit ACK done by the unmask is ignored if the
NMI line is still high, and to have the line going down we have to ACK
all the IRQs on the device accessing to it by I2C in the thread and
then ACK the NMI controller with the second ACK before the unmasking
callback.
I hope this clarifies a bit.

Thank you,

-- 
Carlo Caione

Re: [linux-sunxi] Re: [PATCH] irq: Add new flag to ack level-triggered interrupts before unmasking

[Thomas Gleixner]

On Fri, 7 Feb 2014, Carlo Caione wrote:
> The context and the rationale is fully explained in this thread
> http://www.spinics.net/lists/arm-kernel/msg299952.html and some
> answers are already given along the thread especially by Hans here
> http://www.spinics.net/lists/arm-kernel/msg303542.html
> Shortly, the double interrupt problem as seen on sunxi NMI controller
> (and other chips AFAIK) is specific for level-triggered interrupts
> when the hard interrupt handler is not able to ACK the interrupts on
> the originating device since this device can only be accessed via a
> bus (in our case it was a PMIC on I2C).
> This explains why my patch was specific for the threaded case and why
> the ACK on mask is not really effective in actually acking the IRQs
> (so that I need a second ACK before unmasking the line). In fact in
> our case (PMIC connected via I2C with an interrupt line on a special
> NMI controller) the implicit ACK done by the unmask is ignored if the
> NMI line is still high, and to have the line going down we have to ACK
> all the IRQs on the device accessing to it by I2C in the thread and
> then ACK the NMI controller with the second ACK before the unmasking
> callback.

I can't see why it would be specific for the threaded case.

The explanation says that the NMI chip is ignoring the ack on mask,
which is basically the entry of the interrupt handler. Now it does not
matter whether you are threaded or not. The interrupt line at the NMI
controller is asserted in both cases. So the same issue should be
visible for a non threaded interrupt, if the NMI controller really
needs an ack on unmask. But there is another detail:

	sunxi_sc_nmi_handle_irq()
	chained_irq_enter()
		NOP, because GIC uses EOI

	generic_handle_irq();
		nmi->mask();		      
		dev_handler();
		nmi->unmask();

	chained_irq_exit()
		gic->eoi();

In the threaded case this looks like:

	sunxi_sc_nmi_handle_irq()
	chained_irq_enter()
		NOP, because GIC uses EOI

	generic_handle_irq();
		nmi->mask();
		wake_thread();

	chained_irq_exit()
		gic->eoi();

	run_thread()
		dev_handler();
		nmi->unmask();

So the difference is that in the non threaded case the gic->eoi is
called after the device interrupt has been cleared and the
nmi->interrupt has been unmasked. And in the threaded case its the
other way round. I have no idea how that stuff is connected internaly,
but I suspect that the gic->eoi is related to this as it might
actually ack the NMI chip, which of course only works in the non
threaded case.

Now back to your patch.

I'm not against having a flag, but this should be done less convoluted
and have proper names which make the use case clear along with a good
technical explanation of the flag in the comment.

unmask_and_ack() plus IRQCHIP_ACK_ON_UNMASK are not really telling
what the heck is going on. You can restrict it to level irqs in the
core, but please use the proper functions and not some opencoded
hackery.

Thanks,

	tglx

Re: [linux-sunxi] Re: [PATCH] irq: Add new flag to ack level-triggered interrupts before unmasking

[Carlo Caione]

On Fri, Feb 7, 2014 at 12:51 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> I can't see why it would be specific for the threaded case.
>
> The explanation says that the NMI chip is ignoring the ack on mask,
> which is basically the entry of the interrupt handler. Now it does not
> matter whether you are threaded or not. The interrupt line at the NMI
> controller is asserted in both cases. So the same issue should be
> visible for a non threaded interrupt, if the NMI controller really
> needs an ack on unmask.

Yes, you are right. I was focusing on the threaded case because it was
the only case I have experienced with the hardware I have.
The problem should be there also for the non threaded case (even
though I don't know how to double check this).
At this point I suspect that here the problem is that ACKing the NMI
controller without ACKing ahead the device connected always fails (or
at least is useless and the pending flag is not cleared), so when I
unmask later the irqchip I have a spurious interrupt. That's why I
need another ACK for the NMI controller _after_ having ACKed at device
level.

> But there is another detail:
>
>         sunxi_sc_nmi_handle_irq()
>         chained_irq_enter()
>                 NOP, because GIC uses EOI
>
>         generic_handle_irq();
>                 nmi->mask();
>                 dev_handler();
>                 nmi->unmask();
>
>         chained_irq_exit()
>                 gic->eoi();
>
> In the threaded case this looks like:
>
>         sunxi_sc_nmi_handle_irq()
>         chained_irq_enter()
>                 NOP, because GIC uses EOI
>
>         generic_handle_irq();
>                 nmi->mask();
>                 wake_thread();
>
>         chained_irq_exit()
>                 gic->eoi();
>
>         run_thread()
>                 dev_handler();
>                 nmi->unmask();
>
> So the difference is that in the non threaded case the gic->eoi is
> called after the device interrupt has been cleared and the
> nmi->interrupt has been unmasked. And in the threaded case its the
> other way round. I have no idea how that stuff is connected internaly,
> but I suspect that the gic->eoi is related to this as it might
> actually ack the NMI chip, which of course only works in the non
> threaded case.

Yeah, no really difference between threaded and non threaded.
For the record, from a mail exchange with Allwinner's engineers: "the
NMI module is a signal conversion module. It catches the NMI pin's
state and generates irq to GIC", so GIC does not really ACK anything.
BTW being a dummy "signal conversion module" this is probably why I
still need to clear the pending status even though my IRQ line has
already been cleared.

> Now back to your patch.
>
> I'm not against having a flag, but this should be done less convoluted
> and have proper names which make the use case clear along with a good
> technical explanation of the flag in the comment.

Ok, at this point do you think that a patch in the core could be
useful or is it better to stick with modifying the unmask callback?

> unmask_and_ack() plus IRQCHIP_ACK_ON_UNMASK are not really telling
> what the heck is going on. You can restrict it to level irqs in the
> core, but please use the proper functions and not some opencoded
> hackery.

I'll do in case of v2.

Thanks,

-- 
Carlo Caione

Re: [linux-sunxi] Re: [PATCH] irq: Add new flag to ack level-triggered interrupts before unmasking

[Thomas Gleixner]

On Fri, 7 Feb 2014, Carlo Caione wrote:
> Yeah, no really difference between threaded and non threaded.
> For the record, from a mail exchange with Allwinner's engineers: "the
> NMI module is a signal conversion module. It catches the NMI pin's
> state and generates irq to GIC", so GIC does not really ACK anything.
> BTW being a dummy "signal conversion module" this is probably why I
> still need to clear the pending status even though my IRQ line has
> already been cleared.

A pretty useless signal conversion module it seems creating a big mess
for a single interrupt line :)

> > I'm not against having a flag, but this should be done less convoluted
> > and have proper names which make the use case clear along with a good
> > technical explanation of the flag in the comment.
> 
> Ok, at this point do you think that a patch in the core could be
> useful or is it better to stick with modifying the unmask callback?

Not sure, really, but I tend to a core patch. Though we really want to
know whether the issue is threaded only or not. If it's a general
issue then this wants to go into unmask_irq() itself and not into an
extra unmask_threaded_irq() function.

Thanks,

	tglx

Re: [linux-sunxi] Re: [PATCH] irq: Add new flag to ack level-triggered interrupts before unmasking

[Carlo Caione]

On Fri, Feb 7, 2014 at 6:24 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> Not sure, really, but I tend to a core patch. Though we really want to
> know whether the issue is threaded only or not. If it's a general
> issue then this wants to go into unmask_irq() itself and not into an
> extra unmask_threaded_irq() function.

Unfortunately I cannot be sure whether the problem is still present
for the non threaded case since in the hardware I have the NMI
controller is only connected to the PMIC.
Considering that I have no way of checking what do you suggest?

-- 
Carlo Caione