[PATCH] irq: Add a new IRQF_ACK_BEFORE_UNMASK irq flag

[PATCH] irq: Add a new IRQF_ACK_BEFORE_UNMASK irq flag

[Hans de Goede]

In some cases we want to do an ack right before the unmask of an irq.

A typical example of such a case is having an i2c device which uses a level
interrupt. Such devices usually have an interrupt status register with
write to clear status bits. This means that the interrupt handler needs to
sleep to do i2c transfers to read / write the interrupt status register.

This means using a threaded interrupt handler and IRQF_ONESHOT so that the
interrupt does not re-trigger while the interrupt handler is waiting for the
i2c transfer.

Without ack-before-unmask the sequence of a single interrupt in this setup
looks like this:

i2c-device raises interrupt line
interrupt gets triggered
interrupt gets masked
interrupt gets acked
interrupt handling thread starts running
interrupt handling thread does its stuff, write-clears irq-status register
 in i2c-device
i2c-device lowers interrupt line (note this happens after the ack!)
interrupt handling thread is done
interrupt gets unmasked
interrupt immediately retriggers again, because the line has been high
 after the ack
interrupt gets masked
interrupt gets acked
interrupt handling thread starts running
interrupt handling thread reads irq-status register, has nothing todo
interrupt handling thread is done
interrupt gets unmasked

So in essence we get and handle 2 interrupts for what is 1 interrupt from
the i2c-device pov. By doing an ack before the unmask, the second interrupt
is avoided since at the point of the unmask the irq-thread has done its
job and cleared the interrupt source.

Note that things work fine without this patch, the purpose of this patch is
soley to avoid the second unneeded interrupt.

This patch uses an interrupt flag for this and not an irqchip flag because
the need for ack before unmask is based on the device and not on the irqchip.

Cc: Carlo Caione <carlo.caione@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 include/linux/interrupt.h | 2 ++
 kernel/irq/chip.c         | 4 ++++
 kernel/irq/internals.h    | 2 ++
 kernel/irq/manage.c       | 5 ++++-
 4 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index a2678d3..c252344 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -57,6 +57,7 @@
  * IRQF_NO_THREAD - Interrupt cannot be threaded
  * IRQF_EARLY_RESUME - Resume IRQ early during syscore instead of at device
  *                resume time.
+ * IRQF_ACK_BEFORE_UNMASK - IRQ must be acked before an unmask
  */
 #define IRQF_DISABLED		0x00000020
 #define IRQF_SHARED		0x00000080
@@ -70,6 +71,7 @@
 #define IRQF_FORCE_RESUME	0x00008000
 #define IRQF_NO_THREAD		0x00010000
 #define IRQF_EARLY_RESUME	0x00020000
+#define IRQF_ACK_BEFORE_UNMASK	0x00040000
 
 #define IRQF_TIMER		(__IRQF_TIMER | IRQF_NO_SUSPEND | IRQF_NO_THREAD)
 
diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index dc04c16..b6ed2fb 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -275,6 +275,10 @@ void mask_irq(struct irq_desc *desc)
 
 void unmask_irq(struct irq_desc *desc)
 {
+	if ((desc->istate & IRQS_ACK_BEFORE_UNMASK) &&
+	                desc->irq_data.chip->irq_ack)
+		desc->irq_data.chip->irq_ack(&desc->irq_data);
+
 	if (desc->irq_data.chip->irq_unmask) {
 		desc->irq_data.chip->irq_unmask(&desc->irq_data);
 		irq_state_clr_masked(desc);
diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h
index 001fa5b..cd68190 100644
--- a/kernel/irq/internals.h
+++ b/kernel/irq/internals.h
@@ -43,6 +43,7 @@ enum {
  * IRQS_WAITING			- irq is waiting
  * IRQS_PENDING			- irq is pending and replayed later
  * IRQS_SUSPENDED		- irq is suspended
+ * IRQS_ACK_BEFORE_UNMASK	- irq must be acked before an unmask
  */
 enum {
 	IRQS_AUTODETECT		= 0x00000001,
@@ -53,6 +54,7 @@ enum {
 	IRQS_WAITING		= 0x00000080,
 	IRQS_PENDING		= 0x00000200,
 	IRQS_SUSPENDED		= 0x00000800,
+	IRQS_ACK_BEFORE_UNMASK	= 0x00001000,
 };
 
 #include "debug.h"
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 481a13c..fd105ce 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1104,7 +1104,7 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 		}
 
 		desc->istate &= ~(IRQS_AUTODETECT | IRQS_SPURIOUS_DISABLED | \
-				  IRQS_ONESHOT | IRQS_WAITING);
+			IRQS_ONESHOT | IRQS_WAITING | IRQS_ACK_BEFORE_UNMASK);
 		irqd_clear(&desc->irq_data, IRQD_IRQ_INPROGRESS);
 
 		if (new->flags & IRQF_PERCPU) {
@@ -1140,6 +1140,9 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 				   irq, nmsk, omsk);
 	}
 
+	if (new->flags & IRQF_ACK_BEFORE_UNMASK)
+		desc->istate |= IRQS_ACK_BEFORE_UNMASK;
+
 	new->irq = irq;
 	*old_ptr = new;
 
-- 
1.8.4.2

Re: [PATCH] irq: Add a new IRQF_ACK_BEFORE_UNMASK irq flag

[Hans de Goede]

Hi All,

So after sleeping a night on this I'm not so sure anymore this patch
is such such a good idea. So self-nack.

Still I welcome feedback on this issue...

On 03/12/2014 12:13 AM, Hans de Goede wrote:
> In some cases we want to do an ack right before the unmask of an irq.
> 
> A typical example of such a case is having an i2c device which uses a level
> interrupt. Such devices usually have an interrupt status register with
> write to clear status bits. This means that the interrupt handler needs to
> sleep to do i2c transfers to read / write the interrupt status register.
> 
> This means using a threaded interrupt handler and IRQF_ONESHOT so that the
> interrupt does not re-trigger while the interrupt handler is waiting for the
> i2c transfer.
> 
> Without ack-before-unmask the sequence of a single interrupt in this setup
> looks like this:
> 
> i2c-device raises interrupt line
> interrupt gets triggered
> interrupt gets masked
> interrupt gets acked
> interrupt handling thread starts running
> interrupt handling thread does its stuff, write-clears irq-status register
>  in i2c-device
> i2c-device lowers interrupt line (note this happens after the ack!)
> interrupt handling thread is done
> interrupt gets unmasked
> interrupt immediately retriggers again, because the line has been high
>  after the ack

So this is the crux, it seems that for interrupt 0 (called an External NMI
by the datasheet, even though it can be masked just fine). When configured
for level interrupts the external interrupt line needs to become low (inactive)
before the ack. Otherwise the interrupt handler will be called a second time.

Comparing this to how normal level interrupts works, this seems a special
case for the ENMI on sun4i, AFAIK a normal level interrupt goes like this:

device raises interrupt line
interrupt gets triggered
interrupt gets masked
interrupt gets acked
interrupt handler runs, clears interrupt source
device lowers interrupt line
interrupt gets unmasked

And thats it, so normal level interrupts only get sensitive to an active
interrupt line after the unmask, but this ENMI stays sensitive the whole
time, and we need to do the ack after clearing the source, to avoid
a spurious second interrupt.

> interrupt gets masked
> interrupt gets acked
> interrupt handling thread starts running
> interrupt handling thread reads irq-status register, has nothing todo
> interrupt handling thread is done
> interrupt gets unmasked
> 
> So in essence we get and handle 2 interrupts for what is 1 interrupt from
> the i2c-device pov. By doing an ack before the unmask, the second interrupt
> is avoided since at the point of the unmask the irq-thread has done its
> job and cleared the interrupt source.
> 
> Note that things work fine without this patch, the purpose of this patch is
> soley to avoid the second unneeded interrupt.
> 
> This patch uses an interrupt flag for this and not an irqchip flag because
> the need for ack before unmask is based on the device and not on the irqchip.

So this is also wrong, this is an irqchip property, not a device property, and
more-over its an irqchip property only happening for one special interrupt, so
we cannot do this with an irqchip flag either. We will need to add some special
handling for this one interrupt inside the irq-sun4i.c driver.

I'll be working on my $dayjob for most of today, so I'm not sure if I'll have'
time for this today, but if I've some time this evening I'll try to whip up
a patch, give it a test-run and then send it out.

Regards,

Hans


> 
> Cc: Carlo Caione <carlo.caione@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  include/linux/interrupt.h | 2 ++
>  kernel/irq/chip.c         | 4 ++++
>  kernel/irq/internals.h    | 2 ++
>  kernel/irq/manage.c       | 5 ++++-
>  4 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
> index a2678d3..c252344 100644
> --- a/include/linux/interrupt.h
> +++ b/include/linux/interrupt.h
> @@ -57,6 +57,7 @@
>   * IRQF_NO_THREAD - Interrupt cannot be threaded
>   * IRQF_EARLY_RESUME - Resume IRQ early during syscore instead of at device
>   *                resume time.
> + * IRQF_ACK_BEFORE_UNMASK - IRQ must be acked before an unmask
>   */
>  #define IRQF_DISABLED		0x00000020
>  #define IRQF_SHARED		0x00000080
> @@ -70,6 +71,7 @@
>  #define IRQF_FORCE_RESUME	0x00008000
>  #define IRQF_NO_THREAD		0x00010000
>  #define IRQF_EARLY_RESUME	0x00020000
> +#define IRQF_ACK_BEFORE_UNMASK	0x00040000
>  
>  #define IRQF_TIMER		(__IRQF_TIMER | IRQF_NO_SUSPEND | IRQF_NO_THREAD)
>  
> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
> index dc04c16..b6ed2fb 100644
> --- a/kernel/irq/chip.c
> +++ b/kernel/irq/chip.c
> @@ -275,6 +275,10 @@ void mask_irq(struct irq_desc *desc)
>  
>  void unmask_irq(struct irq_desc *desc)
>  {
> +	if ((desc->istate & IRQS_ACK_BEFORE_UNMASK) &&
> +	                desc->irq_data.chip->irq_ack)
> +		desc->irq_data.chip->irq_ack(&desc->irq_data);
> +
>  	if (desc->irq_data.chip->irq_unmask) {
>  		desc->irq_data.chip->irq_unmask(&desc->irq_data);
>  		irq_state_clr_masked(desc);
> diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h
> index 001fa5b..cd68190 100644
> --- a/kernel/irq/internals.h
> +++ b/kernel/irq/internals.h
> @@ -43,6 +43,7 @@ enum {
>   * IRQS_WAITING			- irq is waiting
>   * IRQS_PENDING			- irq is pending and replayed later
>   * IRQS_SUSPENDED		- irq is suspended
> + * IRQS_ACK_BEFORE_UNMASK	- irq must be acked before an unmask
>   */
>  enum {
>  	IRQS_AUTODETECT		= 0x00000001,
> @@ -53,6 +54,7 @@ enum {
>  	IRQS_WAITING		= 0x00000080,
>  	IRQS_PENDING		= 0x00000200,
>  	IRQS_SUSPENDED		= 0x00000800,
> +	IRQS_ACK_BEFORE_UNMASK	= 0x00001000,
>  };
>  
>  #include "debug.h"
> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> index 481a13c..fd105ce 100644
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -1104,7 +1104,7 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
>  		}
>  
>  		desc->istate &= ~(IRQS_AUTODETECT | IRQS_SPURIOUS_DISABLED | \
> -				  IRQS_ONESHOT | IRQS_WAITING);
> +			IRQS_ONESHOT | IRQS_WAITING | IRQS_ACK_BEFORE_UNMASK);
>  		irqd_clear(&desc->irq_data, IRQD_IRQ_INPROGRESS);
>  
>  		if (new->flags & IRQF_PERCPU) {
> @@ -1140,6 +1140,9 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
>  				   irq, nmsk, omsk);
>  	}
>  
> +	if (new->flags & IRQF_ACK_BEFORE_UNMASK)
> +		desc->istate |= IRQS_ACK_BEFORE_UNMASK;
> +
>  	new->irq = irq;
>  	*old_ptr = new;
>  
> 

Re: [PATCH] irq: Add a new IRQF_ACK_BEFORE_UNMASK irq flag

[Thomas Gleixner]

On Wed, 12 Mar 2014, Hans de Goede wrote:

> In some cases we want to do an ack right before the unmask of an irq.
> 
> A typical example of such a case is having an i2c device which uses a level
> interrupt. Such devices usually have an interrupt status register with
> write to clear status bits. This means that the interrupt handler needs to
> sleep to do i2c transfers to read / write the interrupt status register.
> 
> This means using a threaded interrupt handler and IRQF_ONESHOT so that the
> interrupt does not re-trigger while the interrupt handler is waiting for the
> i2c transfer.
> 
> Without ack-before-unmask the sequence of a single interrupt in this setup
> looks like this:
> 
> i2c-device raises interrupt line
> interrupt gets triggered
> interrupt gets masked
> interrupt gets acked
> interrupt handling thread starts running
> interrupt handling thread does its stuff, write-clears irq-status register
>  in i2c-device
> i2c-device lowers interrupt line (note this happens after the ack!)
> interrupt handling thread is done
> interrupt gets unmasked
> interrupt immediately retriggers again, because the line has been high
>  after the ack
> interrupt gets masked
> interrupt gets acked
> interrupt handling thread starts running
> interrupt handling thread reads irq-status register, has nothing todo
> interrupt handling thread is done
> interrupt gets unmasked
> 
> So in essence we get and handle 2 interrupts for what is 1 interrupt from
> the i2c-device pov. By doing an ack before the unmask, the second interrupt
> is avoided since at the point of the unmask the irq-thread has done its
> job and cleared the interrupt source.

And how is that different from the non threaded case?

    mask()
    ack()	<-- irq line is still active
    handle()	<-- irq line goes inactive. So this happens
    		     after the ack as above
    unmask()

> Note that things work fine without this patch, the purpose of this patch is
> soley to avoid the second unneeded interrupt.
> 
> This patch uses an interrupt flag for this and not an irqchip flag because
> the need for ack before unmask is based on the device and not on the irqchip.
 
No, it's not a device property. Its an irq chip property. There are
interrupt chips which handle that case fine and making this a device
property might even break such interrupt chips. e.g. ioapic handles
that nicely but it would use ack_edge() on a level interrupt when the
device driver requests that. And the edge and level mode of the ioapic
are substantially different. Go figure.

The general policy is that device drivers should be oblivious of the
underlying interrupt controller implementation as much as possible.

If the interrupt chip has this behaviour then handle_level_irq as the
flow handler is the wrong thing to start with because it always acks
before calling the handler.

Due to the fact that we run the primary handler with interrupts
disabled, we should avoid the mask/unmask dance for the non threaded
case completely. handle_fasteoi_irq does that, except that it does not
provide the eoi() before unmask in the threaded case and it has an
extra eoi() even in the masked case which is pointless for interrupt
chips like the one you are dealing with.

Untested patch below.

Thanks,

	tglx

---------------->

Index: linux-2.6/include/linux/irq.h
===================================================================
--- linux-2.6.orig/include/linux/irq.h
+++ linux-2.6/include/linux/irq.h
@@ -349,6 +349,8 @@ 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_ONESHOT_SAFE:	One shot does not require mask/unmask
+ * IRQCHIP_EOI_THREADED:	Chip requires eoi() on unmask in threaded mode
  */
 enum {
 	IRQCHIP_SET_TYPE_MASKED		= (1 <<  0),
@@ -357,6 +359,7 @@ enum {
 	IRQCHIP_ONOFFLINE_ENABLED	= (1 <<  3),
 	IRQCHIP_SKIP_SET_WAKE		= (1 <<  4),
 	IRQCHIP_ONESHOT_SAFE		= (1 <<  5),
+	IRQCHIP_EOI_THREADED		= (1 <<  6),
 };
 
 /* This include will go away once we isolated irq_desc usage to core code */
Index: linux-2.6/kernel/irq/chip.c
===================================================================
--- linux-2.6.orig/kernel/irq/chip.c
+++ linux-2.6/kernel/irq/chip.c
@@ -281,6 +281,19 @@ void unmask_irq(struct irq_desc *desc)
 	}
 }
 
+void unmask_threaded_irq(struct irq_desc *desc)
+{
+	struct irq_chip *chip = desc->irq_data.chip;
+
+	if (chip->flags & IRQCHIP_EOI_THREADED)
+		chip->irq_eoi(&desc->irq_data);
+
+	if (chip->irq_unmask) {
+		desc->irq_data.chip->irq_unmask(&desc->irq_data);
+		irq_state_clr_masked(desc);
+	}
+}
+
 /*
  *	handle_nested_irq - Handle a nested irq from a irq thread
  *	@irq:	the interrupt number
@@ -487,6 +500,67 @@ out:
 	goto out_unlock;
 }
 
+static void cond_unmask_and_eoi_irq(struct irq_desc *desc)
+{
+	if (!(desc->istate & IRQS_ONESHOT)) {
+		desc->irq_data.chip->irq_eoi(&desc->irq_data);
+		return;
+	}
+	/*
+	 * We need to unmask in the following cases:
+	 * - Oneshot irq which did not wake the thread (caused by a
+	 *   spurious interrupt or a primary handler handling it
+	 *   completely).
+	 */
+	if (!irqd_irq_disabled(&desc->irq_data) &&
+	    irqd_irq_masked(&desc->irq_data) && !desc->threads_oneshot) {
+		desc->irq_data.chip->irq_eoi(&desc->irq_data);
+		unmask_irq(desc);
+	}
+}
+
+/**
+ *	handle_fasteoi_late_irq - irq handler for transparent controllers
+ *	@irq:	the interrupt number
+ *	@desc:	the interrupt description structure for this irq
+ *
+ *	Only a single callback will be issued to the chip: an ->eoi()
+ *	call when the interrupt has been serviced. Same as above, but
+ *	we avoid the eoi when the interrupt is masked due to a
+ *	threaded handler. The eoi will be issued right before the unmask.
+ */
+void handle_fasteoi_late_irq(unsigned int irq, struct irq_desc *desc)
+{
+	raw_spin_lock(&desc->lock);
+
+	if (unlikely(irqd_irq_inprogress(&desc->irq_data)))
+		if (!irq_check_poll(desc))
+			goto out;
+
+	desc->istate &= ~(IRQS_REPLAY | IRQS_WAITING);
+	kstat_incr_irqs_this_cpu(irq, desc);
+
+	/*
+	 * If its disabled or no action available
+	 * then mask it and get out of here:
+	 */
+	if (unlikely(!desc->action || irqd_irq_disabled(&desc->irq_data))) {
+		desc->istate |= IRQS_PENDING;
+		mask_irq(desc);
+		goto out;
+	}
+
+	if (desc->istate & IRQS_ONESHOT)
+		mask_irq(desc);
+
+	handle_irq_event(desc);
+
+	cond_unmask_and_eoi_irq(desc);
+out:
+	raw_spin_unlock(&desc->lock);
+	return;
+}
+
 /**
  *	handle_edge_irq - edge type IRQ handler
  *	@irq:	the interrupt number
Index: linux-2.6/kernel/irq/internals.h
===================================================================
--- linux-2.6.orig/kernel/irq/internals.h
+++ linux-2.6/kernel/irq/internals.h
@@ -73,6 +73,7 @@ extern void irq_percpu_enable(struct irq
 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 unmask_threaded_irq(struct irq_desc *desc);
 
 extern void init_kstat_irqs(struct irq_desc *desc, int node, int nr);
 
Index: linux-2.6/kernel/irq/manage.c
===================================================================
--- linux-2.6.orig/kernel/irq/manage.c
+++ linux-2.6/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);
+		unmask_threaded_irq(desc);
 
 out_unlock:
 	raw_spin_unlock_irq(&desc->lock);

Re: [PATCH] irq: Add a new IRQF_ACK_BEFORE_UNMASK irq flag

[Russell King - ARM Linux]

On Wed, Mar 12, 2014 at 11:38:24AM +0100, Thomas Gleixner wrote:
> If the interrupt chip has this behaviour then handle_level_irq as the
> flow handler is the wrong thing to start with because it always acks
> before calling the handler.

This sounds like the situation with the Dove PMC irqchip too, except
that it has the additional complication that "acking" any interrupt is
potentially distructive to other pending interrupts, so should be done
as infrequently as possible.

> +void unmask_threaded_irq(struct irq_desc *desc)
> +{
> +	struct irq_chip *chip = desc->irq_data.chip;
> +
> +	if (chip->flags & IRQCHIP_EOI_THREADED)
> +		chip->irq_eoi(&desc->irq_data);
> +
> +	if (chip->irq_unmask) {
> +		desc->irq_data.chip->irq_unmask(&desc->irq_data);

ITYM:
		chip->irq_unmask(&desc->irq_data);

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

Re: [PATCH] irq: Add a new IRQF_ACK_BEFORE_UNMASK irq flag

[Thomas Gleixner]

On Wed, 12 Mar 2014, Russell King - ARM Linux wrote:

> On Wed, Mar 12, 2014 at 11:38:24AM +0100, Thomas Gleixner wrote:
> > If the interrupt chip has this behaviour then handle_level_irq as the
> > flow handler is the wrong thing to start with because it always acks
> > before calling the handler.
> 
> This sounds like the situation with the Dove PMC irqchip too, except
> that it has the additional complication that "acking" any interrupt is
> potentially distructive to other pending interrupts, so should be done
> as infrequently as possible.

Right. I was wondering about that, but then reminded myself that the
"ack" on that chip is total clusterf*ck.
 
> > +void unmask_threaded_irq(struct irq_desc *desc)
> > +{
> > +	struct irq_chip *chip = desc->irq_data.chip;
> > +
> > +	if (chip->flags & IRQCHIP_EOI_THREADED)
> > +		chip->irq_eoi(&desc->irq_data);
> > +
> > +	if (chip->irq_unmask) {
> > +		desc->irq_data.chip->irq_unmask(&desc->irq_data);
> 
> ITYM:
> 		chip->irq_unmask(&desc->irq_data);

True.

Thanks,

	tglx

Re: [PATCH] irq: Add a new IRQF_ACK_BEFORE_UNMASK irq flag

[Hans de Goede]

Hi,

On 03/12/2014 11:38 AM, Thomas Gleixner wrote:
> On Wed, 12 Mar 2014, Hans de Goede wrote:

<snip>

> And how is that different from the non threaded case?
> 
>     mask()
>     ack()	<-- irq line is still active
>     handle()	<-- irq line goes inactive. So this happens
>     		     after the ack as above
>     unmask()
> 

Right as I already said in my own reply to my patch, my initial analysis
was wrong.

>> Note that things work fine without this patch, the purpose of this patch is
>> soley to avoid the second unneeded interrupt.
>>
>> This patch uses an interrupt flag for this and not an irqchip flag because
>> the need for ack before unmask is based on the device and not on the irqchip.
>  
> No, it's not a device property. Its an irq chip property.

Agreed.

<snip>

> If the interrupt chip has this behaviour then handle_level_irq as the
> flow handler is the wrong thing to start with because it always acks
> before calling the handler.
> 
> Due to the fact that we run the primary handler with interrupts
> disabled, we should avoid the mask/unmask dance for the non threaded
> case completely. handle_fasteoi_irq does that, except that it does not
> provide the eoi() before unmask in the threaded case and it has an
> extra eoi() even in the masked case which is pointless for interrupt
> chips like the one you are dealing with.
> 
> Untested patch below.

Thanks, I've just finished testing this, and it works as advertised.

This indeed seems the best solution. I'm going to send a v2
of my sun4i irq patch-set with this patch included with 2 fixes added:

1) Export the prototype for the new handle_fasteoi_late_irq function
2) Fix the minor issue Russell noticed

I've only one minor nitpick wrt this patch, for ONESHOT
to work properly the irq_chip for an irq_desc using
handle_fasteoi_late_irq must set the IRQCHIP_EOI_THREADED, but
this is not enforced anywhere.

Maybe add a BUG_ON to the irq mapping code checking this ?

Thanks & Regards,

Hans


> 
> Thanks,
> 
> 	tglx
> 
> ---------------->
> 
> Index: linux-2.6/include/linux/irq.h
> ===================================================================
> --- linux-2.6.orig/include/linux/irq.h
> +++ linux-2.6/include/linux/irq.h
> @@ -349,6 +349,8 @@ 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_ONESHOT_SAFE:	One shot does not require mask/unmask
> + * IRQCHIP_EOI_THREADED:	Chip requires eoi() on unmask in threaded mode
>   */
>  enum {
>  	IRQCHIP_SET_TYPE_MASKED		= (1 <<  0),
> @@ -357,6 +359,7 @@ enum {
>  	IRQCHIP_ONOFFLINE_ENABLED	= (1 <<  3),
>  	IRQCHIP_SKIP_SET_WAKE		= (1 <<  4),
>  	IRQCHIP_ONESHOT_SAFE		= (1 <<  5),
> +	IRQCHIP_EOI_THREADED		= (1 <<  6),
>  };
>  
>  /* This include will go away once we isolated irq_desc usage to core code */
> Index: linux-2.6/kernel/irq/chip.c
> ===================================================================
> --- linux-2.6.orig/kernel/irq/chip.c
> +++ linux-2.6/kernel/irq/chip.c
> @@ -281,6 +281,19 @@ void unmask_irq(struct irq_desc *desc)
>  	}
>  }
>  
> +void unmask_threaded_irq(struct irq_desc *desc)
> +{
> +	struct irq_chip *chip = desc->irq_data.chip;
> +
> +	if (chip->flags & IRQCHIP_EOI_THREADED)
> +		chip->irq_eoi(&desc->irq_data);
> +
> +	if (chip->irq_unmask) {
> +		desc->irq_data.chip->irq_unmask(&desc->irq_data);
> +		irq_state_clr_masked(desc);
> +	}
> +}
> +
>  /*
>   *	handle_nested_irq - Handle a nested irq from a irq thread
>   *	@irq:	the interrupt number
> @@ -487,6 +500,67 @@ out:
>  	goto out_unlock;
>  }
>  
> +static void cond_unmask_and_eoi_irq(struct irq_desc *desc)
> +{
> +	if (!(desc->istate & IRQS_ONESHOT)) {
> +		desc->irq_data.chip->irq_eoi(&desc->irq_data);
> +		return;
> +	}
> +	/*
> +	 * We need to unmask in the following cases:
> +	 * - Oneshot irq which did not wake the thread (caused by a
> +	 *   spurious interrupt or a primary handler handling it
> +	 *   completely).
> +	 */
> +	if (!irqd_irq_disabled(&desc->irq_data) &&
> +	    irqd_irq_masked(&desc->irq_data) && !desc->threads_oneshot) {
> +		desc->irq_data.chip->irq_eoi(&desc->irq_data);
> +		unmask_irq(desc);
> +	}
> +}
> +
> +/**
> + *	handle_fasteoi_late_irq - irq handler for transparent controllers
> + *	@irq:	the interrupt number
> + *	@desc:	the interrupt description structure for this irq
> + *
> + *	Only a single callback will be issued to the chip: an ->eoi()
> + *	call when the interrupt has been serviced. Same as above, but
> + *	we avoid the eoi when the interrupt is masked due to a
> + *	threaded handler. The eoi will be issued right before the unmask.
> + */
> +void handle_fasteoi_late_irq(unsigned int irq, struct irq_desc *desc)
> +{
> +	raw_spin_lock(&desc->lock);
> +
> +	if (unlikely(irqd_irq_inprogress(&desc->irq_data)))
> +		if (!irq_check_poll(desc))
> +			goto out;
> +
> +	desc->istate &= ~(IRQS_REPLAY | IRQS_WAITING);
> +	kstat_incr_irqs_this_cpu(irq, desc);
> +
> +	/*
> +	 * If its disabled or no action available
> +	 * then mask it and get out of here:
> +	 */
> +	if (unlikely(!desc->action || irqd_irq_disabled(&desc->irq_data))) {
> +		desc->istate |= IRQS_PENDING;
> +		mask_irq(desc);
> +		goto out;
> +	}
> +
> +	if (desc->istate & IRQS_ONESHOT)
> +		mask_irq(desc);
> +
> +	handle_irq_event(desc);
> +
> +	cond_unmask_and_eoi_irq(desc);
> +out:
> +	raw_spin_unlock(&desc->lock);
> +	return;
> +}
> +
>  /**
>   *	handle_edge_irq - edge type IRQ handler
>   *	@irq:	the interrupt number
> Index: linux-2.6/kernel/irq/internals.h
> ===================================================================
> --- linux-2.6.orig/kernel/irq/internals.h
> +++ linux-2.6/kernel/irq/internals.h
> @@ -73,6 +73,7 @@ extern void irq_percpu_enable(struct irq
>  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 unmask_threaded_irq(struct irq_desc *desc);
>  
>  extern void init_kstat_irqs(struct irq_desc *desc, int node, int nr);
>  
> Index: linux-2.6/kernel/irq/manage.c
> ===================================================================
> --- linux-2.6.orig/kernel/irq/manage.c
> +++ linux-2.6/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);
> +		unmask_threaded_irq(desc);
>  
>  out_unlock:
>  	raw_spin_unlock_irq(&desc->lock);
> 

Re: [PATCH] irq: Add a new IRQF_ACK_BEFORE_UNMASK irq flag

[Thomas Gleixner]

On Wed, 12 Mar 2014, Hans de Goede wrote:
> This indeed seems the best solution. I'm going to send a v2
> of my sun4i irq patch-set with this patch included with 2 fixes added:
> 
> 1) Export the prototype for the new handle_fasteoi_late_irq function
> 2) Fix the minor issue Russell noticed
> 
> I've only one minor nitpick wrt this patch, for ONESHOT
> to work properly the irq_chip for an irq_desc using
> handle_fasteoi_late_irq must set the IRQCHIP_EOI_THREADED, but
> this is not enforced anywhere.
> 
> Maybe add a BUG_ON to the irq mapping code checking this ?

We want some info, but a BUG_ON is probably overkill. I'll think about
a good place to do this.

Thanks,

	tglx

Re: [PATCH] irq: Add a new IRQF_ACK_BEFORE_UNMASK irq flag

[Sebastian Andrzej Siewior]

* Thomas Gleixner | 2014-03-12 11:38:24 [+0100]:

>Untested patch below.
Tested-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

on DA850 / OMAP-L138. Without:
  59:        560   cp_intc  ohci_hcd:usb2
with the patch
  59:        280   cp_intc  ohci_hcd:usb2

that makes a difference of 280 interrupts. Prior the patch each
interrupt was handled twice and returned once with IRQ_HANDLED and the
second time with IRQ_NONE. With the proper EOI ack I don't see the
IRQ_NONE anymore.

Sebastian

Re: [PATCH] irq: Add a new IRQF_ACK_BEFORE_UNMASK irq flag

[Thomas Gleixner]

On Thu, 13 Mar 2014, Sebastian Andrzej Siewior wrote:

> * Thomas Gleixner | 2014-03-12 11:38:24 [+0100]:
> 
> >Untested patch below.
> Tested-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> 
> on DA850 / OMAP-L138. Without:
>   59:        560   cp_intc  ohci_hcd:usb2
> with the patch
>   59:        280   cp_intc  ohci_hcd:usb2
> 
> that makes a difference of 280 interrupts. Prior the patch each
> interrupt was handled twice and returned once with IRQ_HANDLED and the
> second time with IRQ_NONE. With the proper EOI ack I don't see the
> IRQ_NONE anymore.

Right. The documentation of L138 clearly says, that you need to ack
the interrupt AFTER the handler has run. But the driver uses
handle_edge_irq, which acks BEFORE the handler has run.

Quality stuff.

Thanks,

	tglx

Re: [PATCH] irq: Add a new IRQF_ACK_BEFORE_UNMASK irq flag

[Sebastian Andrzej Siewior]

* Thomas Gleixner | 2014-03-12 11:38:24 [+0100]:

>Untested patch below.

I assumed that I forgot that piece during backporting but infact it was
not part of the original patch.

--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -388,6 +388,7 @@ extern int no_irq_affinity;
  */
 extern void handle_level_irq(unsigned int irq, struct irq_desc *desc);
 extern void handle_fasteoi_irq(unsigned int irq, struct irq_desc *desc);
+extern void handle_fasteoi_late_irq(unsigned int irq, struct irq_desc *desc);
 extern void handle_edge_irq(unsigned int irq, struct irq_desc *desc);
 extern void handle_edge_eoi_irq(unsigned int irq, struct irq_desc *desc);
 extern void handle_simple_irq(unsigned int irq, struct irq_desc *desc);


One side note: Since we need to specify IRQCHIP_EOI_THREADED and
handle_fasteoi_late_irq() as the handler, would it be easily doable to use
the handle_fasteoi_irq() handler and skip the EOI in the threaded mode?
But if it creates a mess then leave it as it is.

Sebastian

Re: [PATCH] irq: Add a new IRQF_ACK_BEFORE_UNMASK irq flagq

[Thomas Gleixner]

On Thu, 13 Mar 2014, Sebastian Andrzej Siewior wrote:
> One side note: Since we need to specify IRQCHIP_EOI_THREADED and
> handle_fasteoi_late_irq() as the handler, would it be easily doable to use
> the handle_fasteoi_irq() handler and skip the EOI in the threaded mode?
> But if it creates a mess then leave it as it is.

Yeah, I was looking into that already. Patch below.

To resemble the late version you need to add IRQCHIP_EOI_THREADED and
IRQCHIP_EOI_IF_HANDLED to your irq chip.

Thanks,

	tglx

Index: tip/include/linux/irq.h
===================================================================
--- tip.orig/include/linux/irq.h
+++ tip/include/linux/irq.h
@@ -355,6 +355,8 @@ 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_ONESHOT_SAFE:	One shot does not require mask/unmask
+ * IRQCHIP_EOI_THREADED:	Chip requires eoi() on unmask in threaded mode
  */
 enum {
 	IRQCHIP_SET_TYPE_MASKED		= (1 <<  0),
@@ -363,6 +365,7 @@ enum {
 	IRQCHIP_ONOFFLINE_ENABLED	= (1 <<  3),
 	IRQCHIP_SKIP_SET_WAKE		= (1 <<  4),
 	IRQCHIP_ONESHOT_SAFE		= (1 <<  5),
+	IRQCHIP_EOI_THREADED		= (1 <<  6),
 };
 
 /* This include will go away once we isolated irq_desc usage to core code */
Index: tip/kernel/irq/chip.c
===================================================================
--- tip.orig/kernel/irq/chip.c
+++ tip/kernel/irq/chip.c
@@ -281,6 +281,19 @@ void unmask_irq(struct irq_desc *desc)
 	}
 }
 
+void unmask_threaded_irq(struct irq_desc *desc)
+{
+	struct irq_chip *chip = desc->irq_data.chip;
+
+	if (chip->flags & IRQCHIP_EOI_THREADED)
+		chip->irq_eoi(&desc->irq_data);
+
+	if (chip->irq_unmask) {
+		chip->irq_unmask(&desc->irq_data);
+		irq_state_clr_masked(desc);
+	}
+}
+
 /*
  *	handle_nested_irq - Handle a nested irq from a irq thread
  *	@irq:	the interrupt number
@@ -435,6 +448,27 @@ static inline void preflow_handler(struc
 static inline void preflow_handler(struct irq_desc *desc) { }
 #endif
 
+static void cond_unmask_eoi_irq(struct irq_desc *desc, struct irq_chip *chip)
+{
+	if (!(desc->istate & IRQS_ONESHOT)) {
+		chip->irq_eoi(&desc->irq_data);
+		return;
+	}
+	/*
+	 * We need to unmask in the following cases:
+	 * - Oneshot irq which did not wake the thread (caused by a
+	 *   spurious interrupt or a primary handler handling it
+	 *   completely).
+	 */
+	if (!irqd_irq_disabled(&desc->irq_data) &&
+	    irqd_irq_masked(&desc->irq_data) && !desc->threads_oneshot) {
+		chip->irq_eoi(&desc->irq_data);
+		unmask_irq(desc);
+	} else if (!(chip->flags & IRQCHIP_EOI_THREADED)) {
+		chip->irq_eoi(&desc->irq_data);
+	}
+}
+
 /**
  *	handle_fasteoi_irq - irq handler for transparent controllers
  *	@irq:	the interrupt number
@@ -448,6 +482,8 @@ static inline void preflow_handler(struc
 void
 handle_fasteoi_irq(unsigned int irq, struct irq_desc *desc)
 {
+	struct irq_chip *chip = desc->irq_data.chip;
+
 	raw_spin_lock(&desc->lock);
 
 	if (unlikely(irqd_irq_inprogress(&desc->irq_data)))
@@ -473,18 +509,14 @@ handle_fasteoi_irq(unsigned int irq, str
 	preflow_handler(desc);
 	handle_irq_event(desc);
 
-	if (desc->istate & IRQS_ONESHOT)
-		cond_unmask_irq(desc);
+	cond_unmask_eoi_irq(desc, chip);
 
-out_eoi:
-	desc->irq_data.chip->irq_eoi(&desc->irq_data);
-out_unlock:
 	raw_spin_unlock(&desc->lock);
 	return;
 out:
-	if (!(desc->irq_data.chip->flags & IRQCHIP_EOI_IF_HANDLED))
-		goto out_eoi;
-	goto out_unlock;
+	if (!(chip->flags & IRQCHIP_EOI_IF_HANDLED))
+		chip->irq_eoi(&desc->irq_data);
+	raw_spin_unlock(&desc->lock);
 }
 
 /**
Index: tip/kernel/irq/internals.h
===================================================================
--- tip.orig/kernel/irq/internals.h
+++ tip/kernel/irq/internals.h
@@ -74,6 +74,7 @@ extern void irq_percpu_enable(struct irq
 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 unmask_threaded_irq(struct irq_desc *desc);
 
 extern void init_kstat_irqs(struct irq_desc *desc, int node, int nr);
 
Index: tip/kernel/irq/manage.c
===================================================================
--- tip.orig/kernel/irq/manage.c
+++ tip/kernel/irq/manage.c
@@ -748,7 +748,7 @@ again:
 
 	if (!desc->threads_oneshot && !irqd_irq_disabled(&desc->irq_data) &&
 	    irqd_irq_masked(&desc->irq_data))
-		unmask_irq(desc);
+		unmask_threaded_irq(desc);
 
 out_unlock:
 	raw_spin_unlock_irq(&desc->lock);

Re: [PATCH] irq: Add a new IRQF_ACK_BEFORE_UNMASK irq flagq

[Hans de Goede]

Hi,

On 03/13/2014 03:32 PM, Thomas Gleixner wrote:
> On Thu, 13 Mar 2014, Sebastian Andrzej Siewior wrote:
>> One side note: Since we need to specify IRQCHIP_EOI_THREADED and
>> handle_fasteoi_late_irq() as the handler, would it be easily doable to use
>> the handle_fasteoi_irq() handler and skip the EOI in the threaded mode?
>> But if it creates a mess then leave it as it is.
> 
> Yeah, I was looking into that already. Patch below.
> 
> To resemble the late version you need to add IRQCHIP_EOI_THREADED and
> IRQCHIP_EOI_IF_HANDLED to your irq chip.

Just gave this a review (in as far as I'm qualified to review core irq
code), looks good and I agree this is an improvement over the previous
version.

There is one behavioral change for irq-chips not setting the new
IRQCHIP_EOI_THREADED flag hiding in there.

Before if the conditional unmask conditions were true for the
ONESHOT case the code would do: "unmask; eoi", now it does
"eoi; unmask" I believe the new behavior is more correct, but, since
it is a behavior change I thought I should point this out.

I've also given this a test-run on sun4i and it works as advertised.

FWIW, this patch is:
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: Hans de Goede <hdegoede@redhat.com>

I'll include this in v3 of my irq-sun4i patch-set as it is a dependency
there.

Regards,

Hans

Re: [PATCH] irq: Add a new IRQF_ACK_BEFORE_UNMASK irq flagq

[Thomas Gleixner]

On Thu, 13 Mar 2014, Hans de Goede wrote:
> Hi,
> 
> On 03/13/2014 03:32 PM, Thomas Gleixner wrote:
> > On Thu, 13 Mar 2014, Sebastian Andrzej Siewior wrote:
> >> One side note: Since we need to specify IRQCHIP_EOI_THREADED and
> >> handle_fasteoi_late_irq() as the handler, would it be easily doable to use
> >> the handle_fasteoi_irq() handler and skip the EOI in the threaded mode?
> >> But if it creates a mess then leave it as it is.
> > 
> > Yeah, I was looking into that already. Patch below.
> > 
> > To resemble the late version you need to add IRQCHIP_EOI_THREADED and
> > IRQCHIP_EOI_IF_HANDLED to your irq chip.
> 
> Just gave this a review (in as far as I'm qualified to review core irq
> code), looks good and I agree this is an improvement over the previous
> version.
> 
> There is one behavioral change for irq-chips not setting the new
> IRQCHIP_EOI_THREADED flag hiding in there.
> 
> Before if the conditional unmask conditions were true for the
> ONESHOT case the code would do: "unmask; eoi", now it does
> "eoi; unmask" I believe the new behavior is more correct, but, since
> it is a behavior change I thought I should point this out.
> 
> I've also given this a test-run on sun4i and it works as advertised.
> 
> FWIW, this patch is:
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> Tested-by: Hans de Goede <hdegoede@redhat.com>
> 
> I'll include this in v3 of my irq-sun4i patch-set as it is a dependency
> there.

Sure, I split it up afterwards :)