genirq vs. fastack

genirq vs. fastack

[Benjamin Herrenschmidt]

Hi Thomas, Ingo !

There is one bit in genirq that I don't get (and doesn't work for me),
it's the "fastack" handler. It does:

out:
	if (!(desc->status & IRQ_DISABLED))
		desc->chip->ack(irq);
	else
		desc->chip->mask(irq);

Which doesn't at all match the needs of things like XICS or MPIC and
thus I wonder if it does also make sense for controllers for which you
intend it. It should just be:

	desc->chip->end(irq);

Basically, those controllers will have 1) already acke'd the interrupt
by the time you get the vector (the act of getting the vector does the
ack), 2) will use a processor priority mecanism to handle non-reentrency
of an interrupt thus mask/unmask is completely orthogonal to handling of
interrupts and thus there is no need to do anything about mask/unmask in
the handler, 3) all we need is to do an "EOI" (end of interrupt) at the
end of the handling, which is what is done logically in the end()
handler.

Thus this proposed patch:

Index: linux-work/kernel/irq/chip.c
===================================================================
--- linux-work.orig/kernel/irq/chip.c	2006-05-31 11:26:45.000000000 +1000
+++ linux-work/kernel/irq/chip.c	2006-05-31 11:48:19.000000000 +1000
@@ -325,10 +325,7 @@ handle_fastack_irq(unsigned int irq, str
 	spin_lock(&desc->lock);
 	desc->status &= ~IRQ_INPROGRESS;
 out:
-	if (!(desc->status & IRQ_DISABLED))
-		desc->chip->ack(irq);
-	else
-		desc->chip->mask(irq);
+	desc->chip->end(irq);
 
 	spin_unlock(&desc->lock);
 }

Re: genirq vs. fastack

[Thomas Gleixner]

Hi Ben,

On Wed, 2006-05-31 at 11:52 +1000, Benjamin Herrenschmidt wrote:
> Hi Thomas, Ingo !
> 
> There is one bit in genirq that I don't get (and doesn't work for me),
> it's the "fastack" handler. It does:
> 
> out:
> 	if (!(desc->status & IRQ_DISABLED))
> 		desc->chip->ack(irq);
> 	else
> 		desc->chip->mask(irq);
> 
> Which doesn't at all match the needs of things like XICS or MPIC and
> thus I wonder if it does also make sense for controllers for which you
> intend it. It should just be:
> 
> 	desc->chip->end(irq);
> 
> Basically, those controllers will have 1) already acke'd the interrupt
> by the time you get the vector (the act of getting the vector does the
> ack), 2) will use a processor priority mecanism to handle non-reentrency
> of an interrupt thus mask/unmask is completely orthogonal to handling of
> interrupts and thus there is no need to do anything about mask/unmask in
> the handler, 3) all we need is to do an "EOI" (end of interrupt) at the
> end of the handling, which is what is done logically in the end()
> handler.

I see your point, but isn't EOI the chip specific implementation of
chip->ack() in that case ? 

The intention was to get down to the chip primitives and away from the
flow type chip->functions. Your patch would actually force the flow
control part (if (!(desc->status & IRQ_DISABLED))) back into the end()
function for Ingo's x86 implementation. So the intended seperation of
low level chip functions and flow control would be moot.

	tglx

Re: genirq vs. fastack

[Benjamin Herrenschmidt]

> I see your point, but isn't EOI the chip specific implementation of
> chip->ack() in that case ? 

Not ack, end :)

The ack is implicit when retreiving the irq number (when the irq
happens), the register is even called the ack register :) the EOI isn't
ack'ing, it's really "end of interrupt", that is end of handling by the
processor, thus the CPU local priority can be lowered to what it was
when the interrupt happened.
 
> The intention was to get down to the chip primitives and away from the
> flow type chip->functions. Your patch would actually force the flow
> control part (if (!(desc->status & IRQ_DISABLED))) back into the end()

No. end() for mpic and xics will be the exact same one-liner. The whole
point is that on those chips, mask/unmask are completely disconnected
from the interrupt flow. Mask should happen when disable_irq() is called
wether the irq is in progress or not, independently, and thus the
handler shouldn't mask. Beside, eoi _must_ be called wether it was
masked in between or not or the controller will lose track.

> function for Ingo's x86 implementation. So the intended seperation of
> low level chip functions and flow control would be moot.

Nope. Both MPIC and xics will mask/unmask in ... mask() and unmask()
_and_ have a end() handler that does a EOI without testing if the irq
was disabled. There is no flow handling. That's the whole point of the
handler for "smart" controllers. Non smart controllers can use normal
flow handlers as far as I'm concerned.

Ben.

[patch, -rc5-mm1] genirq: add chip->eoi(), fastack -> fasteoi

[Ingo Molnar]

* Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> > I see your point, but isn't EOI the chip specific implementation of 
> > chip->ack() in that case ?
> 
> Not ack, end :)
> 
> The ack is implicit when retreiving the irq number (when the irq 
> happens), the register is even called the ack register :) the EOI 
> isn't ack'ing, it's really "end of interrupt", that is end of handling 
> by the processor, thus the CPU local priority can be lowered to what 
> it was when the interrupt happened.

yeah. That's quite similar to how the APIC works on x86.

this is mostly a naming issue, and i agree it should be cleaned up some 
more . Right now the 'fastack' handler just cheats a bit and pretends 
that ->ack() is the EOI. But you are right and we could as well 
introduce a separate ->eoi() chip method that clearly separates an 
ack+mask+unmask based chip from an ->eoi chip. [I'd not reuse the 
->end() method because that really is a flow thing in many places and 
the conversion is cleaner. Having one more entry in the chip operations 
structure is no big issue.]

> [...] Beside, eoi _must_ be called wether it was masked in between or 
> not or the controller will lose track.

correct - and that must happen for x86 APICs too. This is a plain bug in 
the fastack handler.

sounds like a plan? The patch below works fine for me.

	Ingo

----------------
Subject: genirq: add chip->eoi(), fastack -> fasteoi
From: Ingo Molnar <mingo@elte.hu>

clean up the fastack concept by turning it into fasteoi and
introducing the ->eoi() method for chips.

this also allows the cleanup of an i386 EOI quirk - now the
quirk is cleanly separated from the pure ACK implementation.

the patch also enables the fasteoi handler for i386 level-triggered
IO-APIC irqs.

tested on i386 and x86_64 with MSI on and off as well.

Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/i386/kernel/io_apic.c   |   28 +++++++++++++++++++---------
 arch/x86_64/kernel/io_apic.c |    9 +++++----
 include/linux/irq.h          |    6 ++++--
 kernel/irq/chip.c            |   25 ++++++++++++-------------
 4 files changed, 40 insertions(+), 28 deletions(-)

Index: linux/arch/i386/kernel/io_apic.c
===================================================================
--- linux.orig/arch/i386/kernel/io_apic.c
+++ linux/arch/i386/kernel/io_apic.c
@@ -1214,7 +1214,7 @@ static void ioapic_register_intr(int irq
 	if ((trigger == IOAPIC_AUTO && IO_APIC_irq_trigger(irq)) ||
 			trigger == IOAPIC_LEVEL)
 		set_irq_chip_and_handler(idx, &ioapic_chip,
-					 handle_level_irq);
+					 handle_fasteoi_irq);
 	else
 		set_irq_chip_and_handler(idx, &ioapic_chip,
 					 handle_edge_irq);
@@ -1946,6 +1946,12 @@ static unsigned int startup_ioapic_irq(u
 	return was_pending;
 }
 
+static void ack_ioapic_irq(unsigned int irq)
+{
+	move_irq(irq);
+	ack_APIC_irq();
+}
+
 static void ack_ioapic_quirk_irq(unsigned int irq)
 {
 	unsigned long v;
@@ -1971,11 +1977,6 @@ static void ack_ioapic_quirk_irq(unsigne
  * operation to prevent an edge-triggered interrupt escaping meanwhile.
  * The idea is from Manfred Spraul.  --macro
  */
-	if (irq_desc[irq].handle_irq == handle_edge_irq) {
-		ack_APIC_irq();
-		return;
-	}
-
 	i = IO_APIC_VECTOR(irq);
 
 	v = apic_read(APIC_TMR + ((i & ~0x1f) >> 1));
@@ -1998,6 +1999,14 @@ static unsigned int startup_ioapic_vecto
 	return startup_ioapic_irq(irq);
 }
 
+static void ack_ioapic_vector(unsigned int vector)
+{
+	int irq = vector_to_irq(vector);
+
+	move_native_irq(vector);
+	ack_ioapic_irq(irq);
+}
+
 static void ack_ioapic_quirk_vector(unsigned int vector)
 {
 	int irq = vector_to_irq(vector);
@@ -2050,7 +2059,8 @@ static struct irq_chip ioapic_chip __rea
 	.startup 	= startup_ioapic_vector,
 	.mask	 	= mask_IO_APIC_vector,
 	.unmask	 	= unmask_IO_APIC_vector,
-	.ack 		= ack_ioapic_quirk_vector,
+	.ack 		= ack_ioapic_vector,
+	.eoi 		= ack_ioapic_quirk_vector,
 #ifdef CONFIG_SMP
 	.set_affinity 	= set_ioapic_affinity,
 #endif
@@ -2124,7 +2134,7 @@ static struct irq_chip lapic_chip __read
 	.name		= "local-APIC-edge",
 	.mask		= mask_lapic_irq,
 	.unmask		= unmask_lapic_irq,
-	.ack		= ack_apic,
+	.eoi		= ack_apic,
 };
 
 static void setup_nmi (void)
@@ -2305,7 +2315,7 @@ static inline void check_timer(void)
 	printk(KERN_INFO "...trying to set up timer as Virtual Wire IRQ...");
 
 	disable_8259A_irq(0);
-	set_irq_chip_and_handler(0, &lapic_chip, handle_fastack_irq);
+	set_irq_chip_and_handler(0, &lapic_chip, handle_fasteoi_irq);
 	apic_write_around(APIC_LVT0, APIC_DM_FIXED | vector);	/* Fixed mode */
 	enable_8259A_irq(0);
 
Index: linux/arch/x86_64/kernel/io_apic.c
===================================================================
--- linux.orig/arch/x86_64/kernel/io_apic.c
+++ linux/arch/x86_64/kernel/io_apic.c
@@ -865,7 +865,7 @@ static void ioapic_register_intr(int irq
 	if ((trigger == IOAPIC_AUTO && IO_APIC_irq_trigger(irq)) ||
 			trigger == IOAPIC_LEVEL)
 		set_irq_chip_and_handler(idx, &ioapic_chip,
-					 handle_fastack_irq);
+					 handle_fasteoi_irq);
 	else
 		set_irq_chip_and_handler(idx, &ioapic_chip,
 					 handle_edge_irq);
@@ -970,7 +970,7 @@ static void __init setup_ExtINT_IRQ0_pin
 	 * The timer IRQ doesn't have to know that behind the
 	 * scene we have a 8259A-master in AEOI mode ...
 	 */
-	set_irq_chip_and_handler(0, &ioapic_chip, handle_fastack_irq);
+	set_irq_chip_and_handler(0, &ioapic_chip, handle_fasteoi_irq);
 
 	/*
 	 * Add it to the IO-APIC irq-routing table:
@@ -1563,6 +1563,7 @@ static struct irq_chip ioapic_chip __rea
 	.mask	 	= mask_ioapic_vector,
 	.unmask	 	= unmask_ioapic_vector,
 	.ack 		= ack_apic,
+	.eoi 		= ack_apic,
 #ifdef CONFIG_SMP
 	.set_affinity 	= set_ioapic_affinity_vector,
 #endif
@@ -1626,7 +1627,7 @@ static struct irq_chip lapic_chip __read
 	.name		= "local-APIC-edge",
 	.mask		= mask_lapic_irq,
 	.unmask		= unmask_lapic_irq,
-	.ack		= ack_apic,
+	.eoi		= ack_apic,
 };
 
 static void setup_nmi (void)
@@ -1808,7 +1809,7 @@ static inline void check_timer(void)
 	apic_printk(APIC_VERBOSE, KERN_INFO "...trying to set up timer as Virtual Wire IRQ...");
 
 	disable_8259A_irq(0);
-	set_irq_chip_and_handler(0, &lapic_chip, handle_fastack_irq);
+	set_irq_chip_and_handler(0, &lapic_chip, handle_fasteoi_irq);
 	apic_write(APIC_LVT0, APIC_DM_FIXED | vector);	/* Fixed mode */
 	enable_8259A_irq(0);
 
Index: linux/include/linux/irq.h
===================================================================
--- linux.orig/include/linux/irq.h
+++ linux/include/linux/irq.h
@@ -73,7 +73,8 @@ struct proc_dir_entry;
  * @mask:		mask an interrupt source
  * @mask_ack:		ack and mask an interrupt source
  * @unmask:		unmask an interrupt source
- * @end:		end of interrupt
+ * @eoi:		end of interrupt - chip level
+ * @end:		end of interrupt - flow level
  * @set_affinity:	set the CPU affinity on SMP machines
  * @retrigger:		resend an IRQ to the CPU
  * @set_type:		set the flow type (IRQ_TYPE_LEVEL/etc.) of an IRQ
@@ -93,6 +94,7 @@ struct irq_chip {
 	void		(*mask)(unsigned int irq);
 	void		(*mask_ack)(unsigned int irq);
 	void		(*unmask)(unsigned int irq);
+	void		(*eoi)(unsigned int irq);
 
 	void		(*end)(unsigned int irq);
 	void		(*set_affinity)(unsigned int irq, cpumask_t dest);
@@ -286,7 +288,7 @@ extern int handle_IRQ_event(unsigned int
 extern void fastcall
 handle_level_irq(unsigned int irq, struct irq_desc *desc, struct pt_regs *regs);
 extern void fastcall
-handle_fastack_irq(unsigned int irq, struct irq_desc *desc,
+handle_fasteoi_irq(unsigned int irq, struct irq_desc *desc,
 			 struct pt_regs *regs);
 extern void fastcall
 handle_edge_irq(unsigned int irq, struct irq_desc *desc, struct pt_regs *regs);
Index: linux/kernel/irq/chip.c
===================================================================
--- linux.orig/kernel/irq/chip.c
+++ linux/kernel/irq/chip.c
@@ -280,18 +280,18 @@ out:
 }
 
 /**
- *	handle_fastack_irq - irq handler for transparent controllers
+ *	handle_fasteoi_irq - irq handler for transparent controllers
  *	@irq:	the interrupt number
  *	@desc:	the interrupt description structure for this irq
  *	@regs:	pointer to a register structure
  *
- *	Only a single callback will be issued to the chip: an ->ack()
+ *	Only a single callback will be issued to the chip: an ->eoi()
  *	call when the interrupt has been serviced. This enables support
  *	for modern forms of interrupt handlers, which handle the flow
  *	details in hardware, transparently.
  */
 void fastcall
-handle_fastack_irq(unsigned int irq, struct irq_desc *desc,
+handle_fasteoi_irq(unsigned int irq, struct irq_desc *desc,
 		   struct pt_regs *regs)
 {
 	unsigned int cpu = smp_processor_id();
@@ -324,10 +324,9 @@ handle_fastack_irq(unsigned int irq, str
 	spin_lock(&desc->lock);
 	desc->status &= ~IRQ_INPROGRESS;
 out:
-	if (!(desc->status & IRQ_DISABLED))
-		desc->chip->ack(irq);
-	else
+	if (unlikely(desc->status & IRQ_DISABLED))
 		desc->chip->mask(irq);
+	desc->chip->eoi(irq);
 
 	spin_unlock(&desc->lock);
 }
@@ -507,19 +506,19 @@ handle_irq_name(void fastcall (*handle)(
 					struct pt_regs *))
 {
 	if (handle == handle_level_irq)
-		return "level ";
-	if (handle == handle_fastack_irq)
-		return "level ";
+		return "level  ";
+	if (handle == handle_fasteoi_irq)
+		return "fasteoi";
 	if (handle == handle_edge_irq)
-		return "edge  ";
+		return "edge   ";
 	if (handle == handle_simple_irq)
-		return "simple";
+		return "simple ";
 #ifdef CONFIG_SMP
 	if (handle == handle_percpu_irq)
-		return "percpu";
+		return "percpu ";
 #endif
 	if (handle == handle_bad_irq)
-		return "bad   ";
+		return "bad    ";
 
 	return NULL;
 }

Re: [patch, -rc5-mm1] genirq: add chip->eoi(), fastack -> fasteoi

[Benjamin Herrenschmidt]

> this is mostly a naming issue, and i agree it should be cleaned up some 
> more . Right now the 'fastack' handler just cheats a bit and pretends 
> that ->ack() is the EOI. But you are right and we could as well 
> introduce a separate ->eoi() chip method that clearly separates an 
> ack+mask+unmask based chip from an ->eoi chip. [I'd not reuse the 
> ->end() method because that really is a flow thing in many places and 
> the conversion is cleaner. Having one more entry in the chip operations 
> structure is no big issue.]

Hrm... ok. Not sure I agree with adding one more callback but it doesn't
matter much.

Thing is, end() isn't used anymore at all now. Thus it's just basically
renaming end() to eoi() except that end() is still there for whoever
uses __do_IRQ() and ... handle_percpu_irq(). Doesn't make that much
sense to me. So I suppose you should also change handle_percpu_irq() to
use eoi() then and consider end() to be "legacy" (to be used only by
__do_IRQ) ?

> > [...] Beside, eoi _must_ be called wether it was masked in between or 
> > not or the controller will lose track.
> 
> correct - and that must happen for x86 APICs too. This is a plain bug in 
> the fastack handler.
> 
> sounds like a plan? The patch below works fine for me.

The patch is _almost_ right to me :) I don't need the

	if (unlikely(desc->status & IRQ_DISABLED))
 		desc->chip->mask(irq);

At all. I suppose it won't harm, but it shouldn't be necessary for me and I'm not sure why it's
necessary on IO_APIC neither (but then I don't know those very well).

Ben.

> 
> 	Ingo
> 
> ----------------
> Subject: genirq: add chip->eoi(), fastack -> fasteoi
> From: Ingo Molnar <mingo@elte.hu>
> 
> clean up the fastack concept by turning it into fasteoi and
> introducing the ->eoi() method for chips.
> 
> this also allows the cleanup of an i386 EOI quirk - now the
> quirk is cleanly separated from the pure ACK implementation.
> 
> the patch also enables the fasteoi handler for i386 level-triggered
> IO-APIC irqs.
> 
> tested on i386 and x86_64 with MSI on and off as well.
> 
> Signed-off-by: Ingo Molnar <mingo@elte.hu>
> ---
>  arch/i386/kernel/io_apic.c   |   28 +++++++++++++++++++---------
>  arch/x86_64/kernel/io_apic.c |    9 +++++----
>  include/linux/irq.h          |    6 ++++--
>  kernel/irq/chip.c            |   25 ++++++++++++-------------
>  4 files changed, 40 insertions(+), 28 deletions(-)
> 
> Index: linux/arch/i386/kernel/io_apic.c
> ===================================================================
> --- linux.orig/arch/i386/kernel/io_apic.c
> +++ linux/arch/i386/kernel/io_apic.c
> @@ -1214,7 +1214,7 @@ static void ioapic_register_intr(int irq
>  	if ((trigger == IOAPIC_AUTO && IO_APIC_irq_trigger(irq)) ||
>  			trigger == IOAPIC_LEVEL)
>  		set_irq_chip_and_handler(idx, &ioapic_chip,
> -					 handle_level_irq);
> +					 handle_fasteoi_irq);
>  	else
>  		set_irq_chip_and_handler(idx, &ioapic_chip,
>  					 handle_edge_irq);
> @@ -1946,6 +1946,12 @@ static unsigned int startup_ioapic_irq(u
>  	return was_pending;
>  }
>  
> +static void ack_ioapic_irq(unsigned int irq)
> +{
> +	move_irq(irq);
> +	ack_APIC_irq();
> +}
> +
>  static void ack_ioapic_quirk_irq(unsigned int irq)
>  {
>  	unsigned long v;
> @@ -1971,11 +1977,6 @@ static void ack_ioapic_quirk_irq(unsigne
>   * operation to prevent an edge-triggered interrupt escaping meanwhile.
>   * The idea is from Manfred Spraul.  --macro
>   */
> -	if (irq_desc[irq].handle_irq == handle_edge_irq) {
> -		ack_APIC_irq();
> -		return;
> -	}
> -
>  	i = IO_APIC_VECTOR(irq);
>  
>  	v = apic_read(APIC_TMR + ((i & ~0x1f) >> 1));
> @@ -1998,6 +1999,14 @@ static unsigned int startup_ioapic_vecto
>  	return startup_ioapic_irq(irq);
>  }
>  
> +static void ack_ioapic_vector(unsigned int vector)
> +{
> +	int irq = vector_to_irq(vector);
> +
> +	move_native_irq(vector);
> +	ack_ioapic_irq(irq);
> +}
> +
>  static void ack_ioapic_quirk_vector(unsigned int vector)
>  {
>  	int irq = vector_to_irq(vector);
> @@ -2050,7 +2059,8 @@ static struct irq_chip ioapic_chip __rea
>  	.startup 	= startup_ioapic_vector,
>  	.mask	 	= mask_IO_APIC_vector,
>  	.unmask	 	= unmask_IO_APIC_vector,
> -	.ack 		= ack_ioapic_quirk_vector,
> +	.ack 		= ack_ioapic_vector,
> +	.eoi 		= ack_ioapic_quirk_vector,
>  #ifdef CONFIG_SMP
>  	.set_affinity 	= set_ioapic_affinity,
>  #endif
> @@ -2124,7 +2134,7 @@ static struct irq_chip lapic_chip __read
>  	.name		= "local-APIC-edge",
>  	.mask		= mask_lapic_irq,
>  	.unmask		= unmask_lapic_irq,
> -	.ack		= ack_apic,
> +	.eoi		= ack_apic,
>  };
>  
>  static void setup_nmi (void)
> @@ -2305,7 +2315,7 @@ static inline void check_timer(void)
>  	printk(KERN_INFO "...trying to set up timer as Virtual Wire IRQ...");
>  
>  	disable_8259A_irq(0);
> -	set_irq_chip_and_handler(0, &lapic_chip, handle_fastack_irq);
> +	set_irq_chip_and_handler(0, &lapic_chip, handle_fasteoi_irq);
>  	apic_write_around(APIC_LVT0, APIC_DM_FIXED | vector);	/* Fixed mode */
>  	enable_8259A_irq(0);
>  
> Index: linux/arch/x86_64/kernel/io_apic.c
> ===================================================================
> --- linux.orig/arch/x86_64/kernel/io_apic.c
> +++ linux/arch/x86_64/kernel/io_apic.c
> @@ -865,7 +865,7 @@ static void ioapic_register_intr(int irq
>  	if ((trigger == IOAPIC_AUTO && IO_APIC_irq_trigger(irq)) ||
>  			trigger == IOAPIC_LEVEL)
>  		set_irq_chip_and_handler(idx, &ioapic_chip,
> -					 handle_fastack_irq);
> +					 handle_fasteoi_irq);
>  	else
>  		set_irq_chip_and_handler(idx, &ioapic_chip,
>  					 handle_edge_irq);
> @@ -970,7 +970,7 @@ static void __init setup_ExtINT_IRQ0_pin
>  	 * The timer IRQ doesn't have to know that behind the
>  	 * scene we have a 8259A-master in AEOI mode ...
>  	 */
> -	set_irq_chip_and_handler(0, &ioapic_chip, handle_fastack_irq);
> +	set_irq_chip_and_handler(0, &ioapic_chip, handle_fasteoi_irq);
>  
>  	/*
>  	 * Add it to the IO-APIC irq-routing table:
> @@ -1563,6 +1563,7 @@ static struct irq_chip ioapic_chip __rea
>  	.mask	 	= mask_ioapic_vector,
>  	.unmask	 	= unmask_ioapic_vector,
>  	.ack 		= ack_apic,
> +	.eoi 		= ack_apic,
>  #ifdef CONFIG_SMP
>  	.set_affinity 	= set_ioapic_affinity_vector,
>  #endif
> @@ -1626,7 +1627,7 @@ static struct irq_chip lapic_chip __read
>  	.name		= "local-APIC-edge",
>  	.mask		= mask_lapic_irq,
>  	.unmask		= unmask_lapic_irq,
> -	.ack		= ack_apic,
> +	.eoi		= ack_apic,
>  };
>  
>  static void setup_nmi (void)
> @@ -1808,7 +1809,7 @@ static inline void check_timer(void)
>  	apic_printk(APIC_VERBOSE, KERN_INFO "...trying to set up timer as Virtual Wire IRQ...");
>  
>  	disable_8259A_irq(0);
> -	set_irq_chip_and_handler(0, &lapic_chip, handle_fastack_irq);
> +	set_irq_chip_and_handler(0, &lapic_chip, handle_fasteoi_irq);
>  	apic_write(APIC_LVT0, APIC_DM_FIXED | vector);	/* Fixed mode */
>  	enable_8259A_irq(0);
>  
> Index: linux/include/linux/irq.h
> ===================================================================
> --- linux.orig/include/linux/irq.h
> +++ linux/include/linux/irq.h
> @@ -73,7 +73,8 @@ struct proc_dir_entry;
>   * @mask:		mask an interrupt source
>   * @mask_ack:		ack and mask an interrupt source
>   * @unmask:		unmask an interrupt source
> - * @end:		end of interrupt
> + * @eoi:		end of interrupt - chip level
> + * @end:		end of interrupt - flow level
>   * @set_affinity:	set the CPU affinity on SMP machines
>   * @retrigger:		resend an IRQ to the CPU
>   * @set_type:		set the flow type (IRQ_TYPE_LEVEL/etc.) of an IRQ
> @@ -93,6 +94,7 @@ struct irq_chip {
>  	void		(*mask)(unsigned int irq);
>  	void		(*mask_ack)(unsigned int irq);
>  	void		(*unmask)(unsigned int irq);
> +	void		(*eoi)(unsigned int irq);
>  
>  	void		(*end)(unsigned int irq);
>  	void		(*set_affinity)(unsigned int irq, cpumask_t dest);
> @@ -286,7 +288,7 @@ extern int handle_IRQ_event(unsigned int
>  extern void fastcall
>  handle_level_irq(unsigned int irq, struct irq_desc *desc, struct pt_regs *regs);
>  extern void fastcall
> -handle_fastack_irq(unsigned int irq, struct irq_desc *desc,
> +handle_fasteoi_irq(unsigned int irq, struct irq_desc *desc,
>  			 struct pt_regs *regs);
>  extern void fastcall
>  handle_edge_irq(unsigned int irq, struct irq_desc *desc, struct pt_regs *regs);
> Index: linux/kernel/irq/chip.c
> ===================================================================
> --- linux.orig/kernel/irq/chip.c
> +++ linux/kernel/irq/chip.c
> @@ -280,18 +280,18 @@ out:
>  }
>  
>  /**
> - *	handle_fastack_irq - irq handler for transparent controllers
> + *	handle_fasteoi_irq - irq handler for transparent controllers
>   *	@irq:	the interrupt number
>   *	@desc:	the interrupt description structure for this irq
>   *	@regs:	pointer to a register structure
>   *
> - *	Only a single callback will be issued to the chip: an ->ack()
> + *	Only a single callback will be issued to the chip: an ->eoi()
>   *	call when the interrupt has been serviced. This enables support
>   *	for modern forms of interrupt handlers, which handle the flow
>   *	details in hardware, transparently.
>   */
>  void fastcall
> -handle_fastack_irq(unsigned int irq, struct irq_desc *desc,
> +handle_fasteoi_irq(unsigned int irq, struct irq_desc *desc,
>  		   struct pt_regs *regs)
>  {
>  	unsigned int cpu = smp_processor_id();
> @@ -324,10 +324,9 @@ handle_fastack_irq(unsigned int irq, str
>  	spin_lock(&desc->lock);
>  	desc->status &= ~IRQ_INPROGRESS;
>  out:
> -	if (!(desc->status & IRQ_DISABLED))
> -		desc->chip->ack(irq);
> -	else
> +	if (unlikely(desc->status & IRQ_DISABLED))
>  		desc->chip->mask(irq);
> +	desc->chip->eoi(irq);
>  
>  	spin_unlock(&desc->lock);
>  }
> @@ -507,19 +506,19 @@ handle_irq_name(void fastcall (*handle)(
>  					struct pt_regs *))
>  {
>  	if (handle == handle_level_irq)
> -		return "level ";
> -	if (handle == handle_fastack_irq)
> -		return "level ";
> +		return "level  ";
> +	if (handle == handle_fasteoi_irq)
> +		return "fasteoi";
>  	if (handle == handle_edge_irq)
> -		return "edge  ";
> +		return "edge   ";
>  	if (handle == handle_simple_irq)
> -		return "simple";
> +		return "simple ";
>  #ifdef CONFIG_SMP
>  	if (handle == handle_percpu_irq)
> -		return "percpu";
> +		return "percpu ";
>  #endif
>  	if (handle == handle_bad_irq)
> -		return "bad   ";
> +		return "bad    ";
>  
>  	return NULL;
>  }

Re: [patch, -rc5-mm1] genirq: add chip->eoi(), fastack -> fasteoi

[Ingo Molnar]

* Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> Hrm... ok. Not sure I agree with adding one more callback but it 
> doesn't matter much.
> 
> Thing is, end() isn't used anymore at all now. Thus it's just 
> basically renaming end() to eoi() except that end() is still there for 
> whoever uses __do_IRQ() and ... handle_percpu_irq(). Doesn't make that 
> much sense to me. So I suppose you should also change 
> handle_percpu_irq() to use eoi() then and consider end() to be 
> "legacy" (to be used only by __do_IRQ) ?

ok, that works with me. I did not want to reuse ->end() just to have a 
clean migration path. ->eoi() is in fact quite descriptive as well, so 
i'm not worried about the name.

> > sounds like a plan? The patch below works fine for me.
> 
> The patch is _almost_ right to me :) I don't need the
> 
> 	if (unlikely(desc->status & IRQ_DISABLED))
>  		desc->chip->mask(irq);
> 
> At all. I suppose it won't harm, but it shouldn't be necessary for me 
> and I'm not sure why it's necessary on IO_APIC neither (but then I 
> don't know those very well).

hm, i dont think it's necessary either. I'll run a few experiments. 
Thomas, do you remember why we have that masking there?

	Ingo

Re: [patch, -rc5-mm1] genirq: add chip->eoi(), fastack -> fasteoi

[Benjamin Herrenschmidt]

On Wed, 2006-05-31 at 23:30 +0200, Ingo Molnar wrote:
> * Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> 
> > Hrm... ok. Not sure I agree with adding one more callback but it 
> > doesn't matter much.
> > 
> > Thing is, end() isn't used anymore at all now. Thus it's just 
> > basically renaming end() to eoi() except that end() is still there for 
> > whoever uses __do_IRQ() and ... handle_percpu_irq(). Doesn't make that 
> > much sense to me. So I suppose you should also change 
> > handle_percpu_irq() to use eoi() then and consider end() to be 
> > "legacy" (to be used only by __do_IRQ) ?
> 
> ok, that works with me. I did not want to reuse ->end() just to have a 
> clean migration path. ->eoi() is in fact quite descriptive as well, so 
> i'm not worried about the name.

Ok, I'll send a patch changing percpu to also use eoi() later from work
unless you beat me to it.

> > > sounds like a plan? The patch below works fine for me.
> > 
> > The patch is _almost_ right to me :) I don't need the
> > 
> > 	if (unlikely(desc->status & IRQ_DISABLED))
> >  		desc->chip->mask(irq);
> > 
> > At all. I suppose it won't harm, but it shouldn't be necessary for me 
> > and I'm not sure why it's necessary on IO_APIC neither (but then I 
> > don't know those very well).
> 
> hm, i dont think it's necessary either. I'll run a few experiments. 
> Thomas, do you remember why we have that masking there?

Ben.