[PATCH] add dispatch_i8259_irq() to i8259.c

[PATCH] add dispatch_i8259_irq() to i8259.c

[Jun Sun]

[-- Attachment #1: Type: text/plain, Size: 533 bytes --]


If a machine uses i8259 as part of its interrupt routing subsystem,
it typically has two ways to figure out which i8259 interrupt has 
happened.

1) through PCI interrupt ack cycle to directly read the IRQ number
2) read i8259 interrupt status registers and figure which one has
   happened.

This patch adds support for those sorry boards which only have
option 2) available.

While I am here, I also promoted do_IRQ() declaration to a 
header file, as it is needed by all C-level interrupt dispatching
code.

Any feedbacks? 

Jun

[-- Attachment #2: junk --]
[-- Type: text/plain, Size: 1281 bytes --]

--- ./arch/mips64/kernel/i8259.c.orig	Mon Aug  5 16:53:33 2002
+++ ./arch/mips64/kernel/i8259.c	Fri Dec 13 14:54:09 2002
@@ -14,6 +14,7 @@
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
 #include <linux/spinlock.h>
+#include <linux/bitops.h>
 
 #include <asm/io.h>
 
@@ -219,6 +220,26 @@
 	}
 }
 
+asmlinkage void dispatch_i8259_irq(struct pt_regs *regs)
+{
+	int isr, irq=-1;
+
+	isr = ~(int)inb(0x20) | cached_21;
+	if (isr != -1) 
+		irq = ffz (isr);
+	if (irq == 2) {
+		isr = ~(int)inb(0xa0) | cached_A1;
+		if (isr != -1) 
+			irq = 8 + ffz(isr);
+	}
+	if (irq == -1) {
+		printk("We got a spurious i8259 interrupt\n");
+		atomic_inc(&irq_err_count);
+	} else {
+		do_IRQ(irq,regs);
+	}
+}
+
 void __init init_8259A(int auto_eoi)
 {
 	unsigned long flags;
diff -Nru ./include/asm-mips64/irq.h.orig ./include/asm-mips/irq.h
--- ./include/asm-mips64/irq.h.orig	Fri Dec 13 10:51:25 2002
+++ ./include/asm-mips64/irq.h	Fri Dec 13 14:50:46 2002
@@ -12,6 +12,7 @@
 #define _ASM_IRQ_H
 
 #include <linux/config.h>
+#include <linux/linkage.h>
 
 #define NR_IRQS 128		/* Largest number of ints of all machines.  */
 
@@ -36,4 +37,6 @@
 
 extern void init_generic_irq(void);
 
+extern asmlinkage unsigned int do_IRQ(int irq, struct pt_regs *regs);
+
 #endif /* _ASM_IRQ_H */

Re: [PATCH] add dispatch_i8259_irq() to i8259.c

[Maciej W. Rozycki]

On Fri, 13 Dec 2002, Jun Sun wrote:

> This patch adds support for those sorry boards which only have
> option 2) available.
> 
> While I am here, I also promoted do_IRQ() declaration to a 
> header file, as it is needed by all C-level interrupt dispatching
> code.
> 
> Any feedbacks? 

 Yep -- a few random thoughts.

> +asmlinkage void dispatch_i8259_irq(struct pt_regs *regs)
> +{
> +	int isr, irq=-1;
> +
> +	isr = ~(int)inb(0x20) | cached_21;

 OCW3 defaults to IRR in our setup (as it does for the chip itself after
writing ICWs) -- you need to select ISR explicitly before reading and
reset it afterwards to avoid surprises.  Unless we change the default for
MIPS, which seems feasible -- we don't have to handle i386 oddities like
I/O APICs and weird chipset bugs.  And we avoid the need to grab a
spinlock here.  Alpha went this path. 

> +	if (isr != -1) 
> +		irq = ffz (isr);
> +	if (irq == 2) {
> +		isr = ~(int)inb(0xa0) | cached_A1;
> +		if (isr != -1) 
> +			irq = 8 + ffz(isr);
> +	}
> +	if (irq == -1) {
> +		printk("We got a spurious i8259 interrupt\n");

 The wording is odd -- how about following the one from
arch/i386/kernel/i8259.c? 

> +		atomic_inc(&irq_err_count);
> +	} else {
> +		do_IRQ(irq,regs);

 And how about using an offset passed from a user?  We're not on a PC --
i8259 IRQs do not have to start from 0.  E.g. I find cleaner allocating
CPU IRQs first if handled.

> --- ./include/asm-mips64/irq.h.orig	Fri Dec 13 10:51:25 2002
> +++ ./include/asm-mips64/irq.h	Fri Dec 13 14:50:46 2002
[...]
> +extern asmlinkage unsigned int do_IRQ(int irq, struct pt_regs *regs);

 Hmm, <asm/hw_irq.h> might be a better alternative.  I have no strong
preference, though.  Both get included by <linux/irq.h> so there's no
difference for a user.

  Maciej

-- 
+  Maciej W. Rozycki, Technical University of Gdansk, Poland   +
+--------------------------------------------------------------+
+        e-mail: macro@ds2.pg.gda.pl, PGP key available        +

Re: [PATCH] add dispatch_i8259_irq() to i8259.c

[Ralf Baechle]

On Sat, Dec 14, 2002 at 01:55:53AM +0100, Maciej W. Rozycki wrote:

>  OCW3 defaults to IRR in our setup (as it does for the chip itself after
> writing ICWs) -- you need to select ISR explicitly before reading and
> reset it afterwards to avoid surprises.  Unless we change the default for
> MIPS, which seems feasible -- we don't have to handle i386 oddities like
> I/O APICs and weird chipset bugs.  And we avoid the need to grab a
> spinlock here.  Alpha went this path. 

We don't have I/O APICs but there's a bunch of MIPS boxes that are based
on Intel chipsets plus glue logic so we may have to deal with some of the
same kind of bugs.  And I'd not be surprised if the 8259 VHDL are coming
from the same source as the x86 ones so because I didn't want to tickle
the dragon's tail so I simply recycled the x86 code.  Overly defensive?
Probably.

> > +		atomic_inc(&irq_err_count);
> > +	} else {
> > +		do_IRQ(irq,regs);
> 
>  And how about using an offset passed from a user?  We're not on a PC --
> i8259 IRQs do not have to start from 0.  E.g. I find cleaner allocating
> CPU IRQs first if handled.

There's still ISA drivers out there with hard coded interrupt numbers.
That's why we assume that ISA / i8259 interrupts are 0 ... 15.  Doesn't
legacy stuff suck ...

  Ralf

Re: [PATCH] add dispatch_i8259_irq() to i8259.c

[Maciej W. Rozycki]

On Sat, 14 Dec 2002, Ralf Baechle wrote:

> >  OCW3 defaults to IRR in our setup (as it does for the chip itself after
> > writing ICWs) -- you need to select ISR explicitly before reading and
> > reset it afterwards to avoid surprises.  Unless we change the default for
> > MIPS, which seems feasible -- we don't have to handle i386 oddities like
> > I/O APICs and weird chipset bugs.  And we avoid the need to grab a
> > spinlock here.  Alpha went this path. 
> 
> We don't have I/O APICs but there's a bunch of MIPS boxes that are based
> on Intel chipsets plus glue logic so we may have to deal with some of the
> same kind of bugs.  And I'd not be surprised if the 8259 VHDL are coming
> from the same source as the x86 ones so because I didn't want to tickle
> the dragon's tail so I simply recycled the x86 code.  Overly defensive?
> Probably.

 Definitely -- the only place the IRR is used is the Neptune (i82378IB/ZB
SIO, i82379AB SIO.A or i82374EB/SB ESC; one or more of them -- the note in
arch/i386/kernel/time.c isn't detailed enough) i8254 core latch
malfunction workaround.  This is needed for do_slow_gettimeoffset(), which
we do not need as we use the processor's internal timer for getting the
offset (or are there any R3k-class systems with an Intel-style chipset?). 
Even if we needed do_slow_gettimeoffset(), I don't think anyone uses any
of these chips in a MIPS system (please correct me if I'm wrong) and the
workaround isn't implemented. 

 Some Alphas do actually use the i82378ZB SIO component, but they use a
processor's internal timer, too so they don't use do_slow_gettimeoffset()
and don't implement the workaround either.

 Surprisingly, there are no known i8259 core implementation bugs. 

 BTW, the workaround probably need not use the IRR -- the Read-Back i8254
command can be used to get the output state.  It's even possible with the
read-back command the latch for the counter would work correctly, so no
workaround would be needed at all.  The code is ancient, though, and
changing it would be tough -- a tester with a buggy system would be
needed. 

> > > +		atomic_inc(&irq_err_count);
> > > +	} else {
> > > +		do_IRQ(irq,regs);
> > 
> >  And how about using an offset passed from a user?  We're not on a PC --
> > i8259 IRQs do not have to start from 0.  E.g. I find cleaner allocating
> > CPU IRQs first if handled.
> 
> There's still ISA drivers out there with hard coded interrupt numbers.
> That's why we assume that ISA / i8259 interrupts are 0 ... 15.  Doesn't
> legacy stuff suck ...

 Ah, I see.

 BTW, I thought on the code a bit and I discovered a few potential
problems due to races.  Handling them depends on the way acks are sent to
i8259s -- Jun, could you please elaborate?

  Maciej

-- 
+  Maciej W. Rozycki, Technical University of Gdansk, Poland   +
+--------------------------------------------------------------+
+        e-mail: macro@ds2.pg.gda.pl, PGP key available        +

Re: [PATCH] add dispatch_i8259_irq() to i8259.c

[Jun Sun]

On Mon, Dec 16, 2002 at 02:44:38PM +0100, Maciej W. Rozycki wrote:
> On Sat, 14 Dec 2002, Ralf Baechle wrote:
> 
> > >  OCW3 defaults to IRR in our setup (as it does for the chip itself after
> > > writing ICWs) -- you need to select ISR explicitly before reading and
> > > reset it afterwards to avoid surprises.  Unless we change the default for
> > > MIPS, which seems feasible -- we don't have to handle i386 oddities like
> > > I/O APICs and weird chipset bugs.  And we avoid the need to grab a
> > > spinlock here.  Alpha went this path. 
> >

I actually meant to read IRR.

I had the code for a while.  I remembered I was reading a i8259 programming 
guide which recommands this method (perhaps for that MIPS board, which should
generally apply to other MIPS boards.).  The idea is to read IRR to figure 
out which interrupt you want to service and directly ack it and mask it.

This style fits more or less what we are doing with the rest of IRQ handling.

> > We don't have I/O APICs but there's a bunch of MIPS boxes that are based
> > on Intel chipsets plus glue logic so we may have to deal with some of the
> > same kind of bugs.  And I'd not be surprised if the 8259 VHDL are coming
> > from the same source as the x86 ones so because I didn't want to tickle
> > the dragon's tail so I simply recycled the x86 code.  Overly defensive?
> > Probably.
> 
>  Definitely -- the only place the IRR is used is the Neptune (i82378IB/ZB
> SIO, i82379AB SIO.A or i82374EB/SB ESC; one or more of them -- the note in
> arch/i386/kernel/time.c isn't detailed enough) i8254 core latch
> malfunction workaround.  This is needed for do_slow_gettimeoffset(), which
> we do not need as we use the processor's internal timer for getting the
> offset (or are there any R3k-class systems with an Intel-style chipset?). 

I don't think so.

> Even if we needed do_slow_gettimeoffset(), 

No MIPS boards are using do_slow_gettimeoffset().  We really should get
rid of it.

>  BTW, I thought on the code a bit and I discovered a few potential
> problems due to races.  Handling them depends on the way acks are sent to
> i8259s -- Jun, could you please elaborate?
>

I am not sure which part you are referring to.  The dispatch_i8259_irq() itself
is called from the first-level interrupt handling routine (assembly or in C),
running with interrupt disabled.

When you decide to service an i8259 IRQ, you move on to call do_IRQ(), still
with interrupt disabled.  Pretty early in do_IRQ() we will do an ACK which will
clear the bit in IRR, before possibly later we turn on interrupt again.


Jun

Re: [PATCH] add dispatch_i8259_irq() to i8259.c

[Maciej W. Rozycki]

On Mon, 16 Dec 2002, Jun Sun wrote:

> > > >  OCW3 defaults to IRR in our setup (as it does for the chip itself after
> > > > writing ICWs) -- you need to select ISR explicitly before reading and
> > > > reset it afterwards to avoid surprises.  Unless we change the default for
> > > > MIPS, which seems feasible -- we don't have to handle i386 oddities like
> > > > I/O APICs and weird chipset bugs.  And we avoid the need to grab a
> > > > spinlock here.  Alpha went this path. 
> 
> I actually meant to read IRR.

 So the IRQ isn't ACKed within i8259As at this point -- I see.  Anyway you
named your variable "isr", so it's at least confusing. 

> I had the code for a while.  I remembered I was reading a i8259 programming 
> guide which recommands this method (perhaps for that MIPS board, which should
> generally apply to other MIPS boards.).  The idea is to read IRR to figure 
> out which interrupt you want to service and directly ack it and mask it.

 The idea is mostly OK, although I'd do it differently -- see below.  In
addition to derivatives I recommend reading the Intel's i8259A original
datasheet.

> This style fits more or less what we are doing with the rest of IRQ handling.

 But remember the i8259A is alien to MIPS systems -- it was designed with
i8080/i8086 processors in mind.  Specifically the way you handle it now
(no ACK for i8259As) breaks badly with edge-triggered interrupts. 

> > Even if we needed do_slow_gettimeoffset(), 
> 
> No MIPS boards are using do_slow_gettimeoffset().  We really should get
> rid of it.

 I know none does at the moment.  But are you sure there is no system that
would need it and might be supported one day?

> >  BTW, I thought on the code a bit and I discovered a few potential
> > problems due to races.  Handling them depends on the way acks are sent to
> > i8259s -- Jun, could you please elaborate?
> 
> I am not sure which part you are referring to.  The dispatch_i8259_irq() itself
> is called from the first-level interrupt handling routine (assembly or in C),
> running with interrupt disabled.

 I thought an IRQ is already ACKed within i8259As.  Then you could have a
race with interrupt masking if an IRQ was masked just at the moment it
would be ACKed.

> When you decide to service an i8259 IRQ, you move on to call do_IRQ(), still
> with interrupt disabled.  Pretty early in do_IRQ() we will do an ACK which will
> clear the bit in IRR, before possibly later we turn on interrupt again.

 The i8259A doesn't work this way.  With your proposed code the IRR is
never cleared (which is a problem for edge-triggered interrupts -- such an
interrupt gets signalled again once it's unmasked, until deasserted by a
device).  The i8259A only clears a bit in the IRR when it receives an ACK
(it then copies the bit to the corresponding bit of the ISR) or when an
interrupt goes away (a device deasserts it). 

 I understand you may be confused by the Linux nomenclature -- for Linux
an ACK is what for the i8259A an EOI is (and mask_and_ack_8259A() does
send EOIs to the chips under the assumption the causing IRQ is already
ACKed -- see the code).  There is no Linux name for what the i8259A calls
an ACK as Linux basically never sends ACKs to i8259As, because interrupts
arriving from them are ACKed by hardware by the means of INTA cycles
(which assert the INTA# lines of i8259As).  For the i386 there is actually
an exception for the NMI watchdog running in specific obscure
configurations involving an i82489DX discrete APIC where an explicit ACK
is sent by Linux to the master i8259A to deassert the NMI -- can you find
it? ;-) 

 Here is an example (untested) code that I would recommend.  It sends
explicit ACKs to the i8259As, which has the following advantages:

- the priority resolver of the i8259A is respected -- the settings of FNM,
EFNM (sort-of), rotating and SMM are obeyed,

- you get the IRQ number directly out of the i8259As,

- edge-triggered interrupts are handled properly,

- you don't have to mess with the mask.


asmlinkage void dispatch_i8259_irq(struct pt_regs *regs)
{
	int irq, real;

	spin_lock(&i8259A_lock);

	outb(0x0c, 0x20);
	iob();
	/* ACK the IRQ (master). */
	irq = inb(0x20);
	real = (irq & 0x80);
	irq &= 0x7;
	if (!real)
		goto spurious_master;

	if (irq == 2) {
		outb(0x0c, 0xa0);
		iob();
		/* ACK the IRQ (slave). */
		irq = inb(0xa0);
		real = (irq & 0x80);
		irq = (irq & 0x7) + 8;
		if (!real)
			goto spurious_slave;
	}

	spin_unlock(&i8259A_lock);

	do_IRQ(irq, regs);

	return;

spurious_slave:
	/* IRQ 2 was ACKed in the master; send an EOI to clear it. */
	outb(0x62, 0x20);
	iob();
spurious_master:

	spin_unlock(&i8259A_lock);

	printk("spurious 8259A interrupt: IRQ%d.\n", irq);
	atomic_inc(&irq_err_count);
}
	

 Does it work for you?  And what do you think of it?

  Maciej

-- 
+  Maciej W. Rozycki, Technical University of Gdansk, Poland   +
+--------------------------------------------------------------+
+        e-mail: macro@ds2.pg.gda.pl, PGP key available        +

Re: [PATCH] add dispatch_i8259_irq() to i8259.c

[Dominic Sweetman]

>  The i8259A doesn't work this way.  With your proposed code the IRR
> is never cleared (which is a problem for edge-triggered interrupts
> -- such an interrupt gets signalled again once it's unmasked, until
> deasserted by a device).  The i8259A only clears a bit in the IRR
> when it receives an ACK (it then copies the bit to the corresponding
> bit of the ISR) or when an interrupt goes away (a device deasserts
> it).

Just a few comments on the hardware:

As I recall, you can clear a stored edge-triggered interrupt using a
"specific EOI".  In the 8080 microprocessor for which the 8259 was
designed, this command was magically communicated to the 8259 when the
CPU ran its "return from interrupt" instruction.  I think even in the
8086 this had to be replaced with an explicit I/O cycle.

People not using x86 CPUs should consider putting the i8259 into
"special mask mode".  Then it behaves simply and predictably,
providing an interrupt on any active unmasked input.  You lose the
i8259 interrupt priority stuff, but this is only one of the
advantages.  You'd need to be reasonably knowledgeable about the Linux
interrupt system to make this clean and compatible with the x86
versions, but then these troubles would be over for ever and you'd be
a Hero, First Class.

Alternatively, many MIPS systems have a hardware feature which enables
them to generate imitation-x86 interrupt acknowledge cycles, so you
can keep the 8259s in complete ignorance that they're not being
controlled by an x86.  

--
Dominic Sweetman
MIPS Technologies
dom@mips.com

Re: [PATCH] add dispatch_i8259_irq() to i8259.c

[Ralf Baechle]

On Tue, Dec 17, 2002 at 02:35:22PM +0000, Dominic Sweetman wrote:

> Alternatively, many MIPS systems have a hardware feature which enables
> them to generate imitation-x86 interrupt acknowledge cycles, so you
> can keep the 8259s in complete ignorance that they're not being
> controlled by an x86.  

Unless the hardware is an RM200 where under special circumstances the
hardware acknowledge feature may result in loss of some or all i8259
interrupts until full-reinitialization of the i8259 ...

Bugs, bugs ...

  Ralf

Re: [PATCH] add dispatch_i8259_irq() to i8259.c

[Maciej W. Rozycki]

On Tue, 17 Dec 2002, Ralf Baechle wrote:

> > Alternatively, many MIPS systems have a hardware feature which enables
> > them to generate imitation-x86 interrupt acknowledge cycles, so you
> > can keep the 8259s in complete ignorance that they're not being
> > controlled by an x86.  
> 
> Unless the hardware is an RM200 where under special circumstances the
> hardware acknowledge feature may result in loss of some or all i8259
> interrupts until full-reinitialization of the i8259 ...

 Can the hardware acknowledge be disabled?  If so, my proposed code should
work just fine.

-- 
+  Maciej W. Rozycki, Technical University of Gdansk, Poland   +
+--------------------------------------------------------------+
+        e-mail: macro@ds2.pg.gda.pl, PGP key available        +

Re: [PATCH] add dispatch_i8259_irq() to i8259.c

[Maciej W. Rozycki]

On Tue, 17 Dec 2002, Dominic Sweetman wrote:

> >  The i8259A doesn't work this way.  With your proposed code the IRR
> > is never cleared (which is a problem for edge-triggered interrupts
> > -- such an interrupt gets signalled again once it's unmasked, until
> > deasserted by a device).  The i8259A only clears a bit in the IRR
> > when it receives an ACK (it then copies the bit to the corresponding
> > bit of the ISR) or when an interrupt goes away (a device deasserts
> > it).
> 
> Just a few comments on the hardware:
> 
> As I recall, you can clear a stored edge-triggered interrupt using a
> "specific EOI".  In the 8080 microprocessor for which the 8259 was
> designed, this command was magically communicated to the 8259 when the
> CPU ran its "return from interrupt" instruction.  I think even in the
> 8086 this had to be replaced with an explicit I/O cycle.

 An EOI command (be it the specific or the non-specific version) is used
to clear a bit in the ISR (unless the AEOI mode is activated; I'm not sure
why Linux doesn't use it as some code could be trimmed down -- probably
due to a historical reason) to permit further interrupts from the
respective input.  But a bit in the ISR is only copied from the IRR upon
an ACK.  Otherwise the i8259A still considers the IRQ pending.

 The EOI command has to arrive at the i8259A bus as a write cycle (i.e. 
with both CS# and WR# active) -- for i8080 and x86 systems a write cycle
to a system-specific location in the I/O address space of these processors
is typically the most convenient way, but memory-mapping is just fine,
too.  Also the i8080 has no "return from interrupt" instruction -- the
usual sequence is "ei; ret" (this is safe as "ei" defers enabling
interrupt acceptance until after the following instruction).  It's
possible there were systems that incorporated additional glue logic for
detecting this sequence of instructions (I haven't met any) and doing
writes to an i8259A, but I wouldn't believe they would be capable of
sending specific EOIs.  Non-specific ones -- certainly, but only for
systems with a single i8259A (you can have up to 9 i8259As per an
i8080/x86 system for 64 IRQ inputs without additional glue logic or
special arrangements).

> People not using x86 CPUs should consider putting the i8259 into
> "special mask mode".  Then it behaves simply and predictably,
> providing an interrupt on any active unmasked input.  You lose the
> i8259 interrupt priority stuff, but this is only one of the
> advantages.  You'd need to be reasonably knowledgeable about the Linux
> interrupt system to make this clean and compatible with the x86
> versions, but then these troubles would be over for ever and you'd be
> a Hero, First Class.

 It shouldn't be necessary for our handling model (but wouldn't hurt
either, even for the i386).  As we use almost unmodified code from the
i386 implementation, there is only a small window when interrupts are
marked "in service" by the i8259As -- mask_and_ack_8259A() sends an EOI to
the chips before dispatching handlers.  As a result there are no masked
interrupts with their ISR bits set.

> Alternatively, many MIPS systems have a hardware feature which enables
> them to generate imitation-x86 interrupt acknowledge cycles, so you
> can keep the 8259s in complete ignorance that they're not being
> controlled by an x86.  

 Do they ever need to "know"? 

  Maciej

-- 
+  Maciej W. Rozycki, Technical University of Gdansk, Poland   +
+--------------------------------------------------------------+
+        e-mail: macro@ds2.pg.gda.pl, PGP key available        +

Re: [PATCH] add dispatch_i8259_irq() to i8259.c

[Jun Sun]

On Tue, Dec 17, 2002 at 02:35:22PM +0000, Dominic Sweetman wrote:
> 
> >  The i8259A doesn't work this way.  With your proposed code the IRR
> > is never cleared (which is a problem for edge-triggered interrupts
> > -- such an interrupt gets signalled again once it's unmasked, until
> > deasserted by a device).  The i8259A only clears a bit in the IRR
> > when it receives an ACK (it then copies the bit to the corresponding
> > bit of the ISR) or when an interrupt goes away (a device deasserts
> > it).
> 
> Just a few comments on the hardware:
> 
> As I recall, you can clear a stored edge-triggered interrupt using a
> "specific EOI".  In the 8080 microprocessor for which the 8259 was
> designed, this command was magically communicated to the 8259 when the
> CPU ran its "return from interrupt" instruction.  I think even in the
> 8086 this had to be replaced with an explicit I/O cycle.
> 
> People not using x86 CPUs should consider putting the i8259 into
> "special mask mode".  Then it behaves simply and predictably,
> providing an interrupt on any active unmasked input.  You lose the
> i8259 interrupt priority stuff, but this is only one of the
> advantages.  

This sounds a lot like the doc I read when I did the programming.  Does
anybody know *the doc* I am talking about?  I can't seem to find it anymore.

Meanwhile I find myself cannot answer Maciej's question as to when IRR 
bit is cleared under edge triggering case.  Perhaps the hardware does it 
automatically when IRQ is generated?

I will probe further and reply to you later.

Jun

Re: [PATCH] add dispatch_i8259_irq() to i8259.c

[Jun Sun]

On Tue, Dec 17, 2002 at 02:39:21PM +0100, Maciej W. Rozycki wrote:
> > No MIPS boards are using do_slow_gettimeoffset().  We really should get
> > rid of it.
> 
>  I know none does at the moment.  But are you sure there is no system that
> would need it and might be supported one day?
>

I serisouly don't think so.  Moving forward every CPU will have a CPU counter,
which can be used for timeoffset purpose.  Even if it does not have one,
it will surely have some onboard high resolution timer, which can be used
to intra-jiffy offset purpose.

Anyway, this function is only defined in old-time.c, which will go away 
soon (hopefully). :-)

 
>  Here is an example (untested) code that I would recommend.  It sends
> explicit ACKs to the i8259As, which has the following advantages:
> 
<snip>

Cool.  This code works for me.

I studied it a little bit and I am convinced it is a better choice.
It should work for MIPS in general.

In my original code I did verify that the IRR bit is not cleared,
which apparently will be a problem in cases.

The only catch with your code is that we don't have iob() macro (which 
apparently is very useful).  Any suggestions on this?  Otherwise  
I will probably remove it.

Jun

Re: [PATCH] add dispatch_i8259_irq() to i8259.c

[Maciej W. Rozycki]

On Tue, 17 Dec 2002, Jun Sun wrote:

> > > No MIPS boards are using do_slow_gettimeoffset().  We really should get
> > > rid of it.
> > 
> >  I know none does at the moment.  But are you sure there is no system that
> > would need it and might be supported one day?
> 
> I serisouly don't think so.  Moving forward every CPU will have a CPU counter,
> which can be used for timeoffset purpose.  Even if it does not have one,
> it will surely have some onboard high resolution timer, which can be used
> to intra-jiffy offset purpose.

 Well, I do hope so, too, but you'll never know until you find out. ;-)

> >  Here is an example (untested) code that I would recommend.  It sends
> > explicit ACKs to the i8259As, which has the following advantages:
> > 
> <snip>
> 
> Cool.  This code works for me.

 Excellent.  I worked on the code a bit more and removed the spurious IRQ
stuff.  It's not really necessary -- mask_and_ack_8259A() will deal with
it anyway (a bit less precisely, but we don't care -- they are very rare
and drivers absolutely have to be able to deal with spurious interrupts)
and we want the low-level IRQ handling to be fast as it's performance
critical.  At this point the function became so compact it would be
unreasonable not to make it inline -- the generated code is 24
instructions on my system.  The positive side effect is the code won't be
compiled for systems that don't use it.

 Following is a patch that I consider a candidate for submission.  I
changed the interface a bit to permit greater flexibility.  I renamed the
function to reflect the new semantic.  Unless special handling is needed
you may simply call: 

do_IRQ(poll_8259A_irq(), regs);

> I studied it a little bit and I am convinced it is a better choice.
> It should work for MIPS in general.

 Actually for any system that doesn't send hardware INTAs to i8259A PICs.

> In my original code I did verify that the IRR bit is not cleared,
> which apparently will be a problem in cases.

 The real problem is sources of edge-triggered interrupts need not
deassert their IRQs until they want to trigger another interrupt.  The
i8254 is a notorius example. 

> The only catch with your code is that we don't have iob() macro (which 
> apparently is very useful).  Any suggestions on this?  Otherwise  
> I will probably remove it.

 iob() comes from <asm/system.h> -- do you miss it somehow?  Unfortunately
its drawback is it uses a memory clobber which for this specific example
costs 4 unnecessary instructions (or 20%) to reread mips_io_port_base
twice. 

 The following patch was successfully tested at the run time on a Malta
system (after replacing most of the junk from
mips/mips-boards/malta/malta_int.c with the single-liner shown above).
Apart from adding the function to ACK i8259A IRQs, it cleans up the i8259A
stuff a bit:

- private init_i8259_irqs() declarations scattered over the tree are
removed and files referring to it now include <asm/i8259.h>,

- declarations of obsolete, non-existent i8259A handling functions are
removed.

Note the i8259A_lock is now extern for poll_8259A_irq() but not declared
in <asm/i8259.h> publicly as it shouldn't be used elsewhere. 

 Ralf, is it OK to apply?

  Maciej

-- 
+  Maciej W. Rozycki, Technical University of Gdansk, Poland   +
+--------------------------------------------------------------+
+        e-mail: macro@ds2.pg.gda.pl, PGP key available        +

patch-mips-2.4.20-pre6-20021212-poll-8259A-4
diff -up --recursive --new-file linux-mips-2.4.20-pre6-20021212.macro/arch/mips/cobalt/irq.c linux-mips-2.4.20-pre6-20021212/arch/mips/cobalt/irq.c
--- linux-mips-2.4.20-pre6-20021212.macro/arch/mips/cobalt/irq.c	2002-12-04 03:56:24.000000000 +0000
+++ linux-mips-2.4.20-pre6-20021212/arch/mips/cobalt/irq.c	2002-12-18 00:50:36.000000000 +0000
@@ -17,6 +17,7 @@
 #include <linux/ioport.h>
 
 #include <asm/bootinfo.h>
+#include <asm/i8259.h>
 #include <asm/io.h>
 #include <asm/irq.h>
 #include <asm/mipsregs.h>
diff -up --recursive --new-file linux-mips-2.4.20-pre6-20021212.macro/arch/mips/ddb5xxx/ddb5074/irq.c linux-mips-2.4.20-pre6-20021212/arch/mips/ddb5xxx/ddb5074/irq.c
--- linux-mips-2.4.20-pre6-20021212.macro/arch/mips/ddb5xxx/ddb5074/irq.c	2002-08-06 02:57:07.000000000 +0000
+++ linux-mips-2.4.20-pre6-20021212/arch/mips/ddb5xxx/ddb5074/irq.c	2002-12-18 00:54:16.000000000 +0000
@@ -12,6 +12,7 @@
 #include <linux/interrupt.h>
 #include <linux/ioport.h>
 
+#include <asm/i8259.h>
 #include <asm/io.h>
 #include <asm/irq.h>
 #include <asm/irq_cpu.h>
@@ -21,13 +22,7 @@
 #include <asm/ddb5xxx/ddb5074.h>
 
 
-extern void __init i8259_init(void);
-extern void init_i8259_irqs (void);
-extern void i8259_disable_irq(unsigned int irq_nr);
-extern void i8259_enable_irq(unsigned int irq_nr);
-
 extern asmlinkage void ddbIRQ(void);
-extern asmlinkage void i8259_do_irq(int irq, struct pt_regs *regs);
 extern asmlinkage void do_IRQ(int irq, struct pt_regs *regs);
 
 static struct irqaction irq_cascade = { no_action, 0, 0, "cascade", NULL, NULL };
diff -up --recursive --new-file linux-mips-2.4.20-pre6-20021212.macro/arch/mips/ddb5xxx/ddb5476/irq.c linux-mips-2.4.20-pre6-20021212/arch/mips/ddb5xxx/ddb5476/irq.c
--- linux-mips-2.4.20-pre6-20021212.macro/arch/mips/ddb5xxx/ddb5476/irq.c	2002-08-06 02:57:07.000000000 +0000
+++ linux-mips-2.4.20-pre6-20021212/arch/mips/ddb5xxx/ddb5476/irq.c	2002-12-18 00:55:01.000000000 +0000
@@ -14,6 +14,7 @@
 #include <linux/types.h>
 #include <linux/interrupt.h>
 
+#include <asm/i8259.h>
 #include <asm/io.h>
 #include <asm/ptrace.h>
 
diff -up --recursive --new-file linux-mips-2.4.20-pre6-20021212.macro/arch/mips/ddb5xxx/ddb5477/irq.c linux-mips-2.4.20-pre6-20021212/arch/mips/ddb5xxx/ddb5477/irq.c
--- linux-mips-2.4.20-pre6-20021212.macro/arch/mips/ddb5xxx/ddb5477/irq.c	2002-12-04 03:56:25.000000000 +0000
+++ linux-mips-2.4.20-pre6-20021212/arch/mips/ddb5xxx/ddb5477/irq.c	2002-12-18 00:55:53.000000000 +0000
@@ -16,6 +16,7 @@
 #include <linux/types.h>
 #include <linux/ptrace.h>
 
+#include <asm/i8259.h>
 #include <asm/system.h>
 #include <asm/mipsregs.h>
 #include <asm/debug.h>
@@ -71,7 +72,6 @@ set_pci_int_attr(u32 pci, u32 intn, u32 
 	ddb_out32(pci, reg_value);
 }
 
-extern void init_i8259_irqs (void);
 extern void vrc5477_irq_init(u32 base);
 extern void mips_cpu_irq_init(u32 base);
 extern asmlinkage void ddb5477_handle_int(void);
diff -up --recursive --new-file linux-mips-2.4.20-pre6-20021212.macro/arch/mips/jazz/irq.c linux-mips-2.4.20-pre6-20021212/arch/mips/jazz/irq.c
--- linux-mips-2.4.20-pre6-20021212.macro/arch/mips/jazz/irq.c	2001-12-18 05:27:34.000000000 +0000
+++ linux-mips-2.4.20-pre6-20021212/arch/mips/jazz/irq.c	2002-12-18 00:56:22.000000000 +0000
@@ -12,6 +12,7 @@
 #include <linux/kernel.h>
 #include <linux/spinlock.h>
 
+#include <asm/i8259.h>
 #include <asm/io.h>
 #include <asm/jazz.h>
 
@@ -19,7 +20,7 @@ extern asmlinkage void jazz_handle_int(v
 
 /*
  * On systems with i8259-style interrupt controllers we assume for
- * driver compatibility reasons interrupts 0 - 15 to be the i8295
+ * driver compatibility reasons interrupts 0 - 15 to be the i8259
  * interrupts even if the hardware uses a different interrupt numbering.
  */
 void __init init_IRQ (void)
diff -up --recursive --new-file linux-mips-2.4.20-pre6-20021212.macro/arch/mips/kernel/i8259.c linux-mips-2.4.20-pre6-20021212/arch/mips/kernel/i8259.c
--- linux-mips-2.4.20-pre6-20021212.macro/arch/mips/kernel/i8259.c	2002-08-06 02:57:16.000000000 +0000
+++ linux-mips-2.4.20-pre6-20021212/arch/mips/kernel/i8259.c	2002-12-17 23:18:16.000000000 +0000
@@ -29,7 +29,7 @@ void disable_8259A_irq(unsigned int irq)
  * moves to arch independent land
  */
 
-static spinlock_t i8259A_lock = SPIN_LOCK_UNLOCKED;
+spinlock_t i8259A_lock = SPIN_LOCK_UNLOCKED;
 
 static void end_8259A_irq (unsigned int irq)
 {
diff -up --recursive --new-file linux-mips-2.4.20-pre6-20021212.macro/arch/mips/mips-boards/malta/malta_int.c linux-mips-2.4.20-pre6-20021212/arch/mips/mips-boards/malta/malta_int.c
--- linux-mips-2.4.20-pre6-20021212.macro/arch/mips/mips-boards/malta/malta_int.c	2002-12-12 03:56:34.000000000 +0000
+++ linux-mips-2.4.20-pre6-20021212/arch/mips/mips-boards/malta/malta_int.c	2002-12-18 00:57:12.000000000 +0000
@@ -29,6 +29,7 @@
 #include <linux/kernel_stat.h>
 #include <linux/random.h>
 
+#include <asm/i8259.h>
 #include <asm/irq.h>
 #include <asm/io.h>
 #include <asm/mips-boards/malta.h>
@@ -40,7 +41,6 @@
 
 extern asmlinkage void mipsIRQ(void);
 extern asmlinkage void do_IRQ(int irq, struct pt_regs *regs);
-extern void init_i8259_irqs (void);
 extern int mips_pcibios_iack(void);
 
 static spinlock_t mips_irq_lock = SPIN_LOCK_UNLOCKED;
diff -up --recursive --new-file linux-mips-2.4.20-pre6-20021212.macro/arch/mips/sni/irq.c linux-mips-2.4.20-pre6-20021212/arch/mips/sni/irq.c
--- linux-mips-2.4.20-pre6-20021212.macro/arch/mips/sni/irq.c	2001-12-18 05:27:36.000000000 +0000
+++ linux-mips-2.4.20-pre6-20021212/arch/mips/sni/irq.c	2002-12-18 00:58:13.000000000 +0000
@@ -12,6 +12,7 @@
 #include <linux/kernel.h>
 #include <linux/spinlock.h>
 
+#include <asm/i8259.h>
 #include <asm/io.h>
 #include <asm/sni.h>
 
diff -up --recursive --new-file linux-mips-2.4.20-pre6-20021212.macro/arch/mips64/kernel/i8259.c linux-mips-2.4.20-pre6-20021212/arch/mips64/kernel/i8259.c
--- linux-mips-2.4.20-pre6-20021212.macro/arch/mips64/kernel/i8259.c	2002-08-06 02:57:35.000000000 +0000
+++ linux-mips-2.4.20-pre6-20021212/arch/mips64/kernel/i8259.c	2002-12-18 01:03:29.000000000 +0000
@@ -29,7 +29,7 @@ void disable_8259A_irq(unsigned int irq)
  * moves to arch independent land
  */
 
-static spinlock_t i8259A_lock = SPIN_LOCK_UNLOCKED;
+spinlock_t i8259A_lock = SPIN_LOCK_UNLOCKED;
 
 static void end_8259A_irq (unsigned int irq)
 {
diff -up --recursive --new-file linux-mips-2.4.20-pre6-20021212.macro/include/asm-mips/i8259.h linux-mips-2.4.20-pre6-20021212/include/asm-mips/i8259.h
--- linux-mips-2.4.20-pre6-20021212.macro/include/asm-mips/i8259.h	1970-01-01 00:00:00.000000000 +0000
+++ linux-mips-2.4.20-pre6-20021212/include/asm-mips/i8259.h	2002-12-18 00:31:40.000000000 +0000
@@ -0,0 +1,54 @@
+/*
+ *	include/asm-mips/i8259.h
+ *
+ *	i8259A interrupt definitions.
+ *
+ *	Copyright (C) 2002  Maciej W. Rozycki
+ *
+ *	This program is free software; you can redistribute it and/or
+ *	modify it under the terms of the GNU General Public License
+ *	as published by the Free Software Foundation; either version
+ *	2 of the License, or (at your option) any later version.
+ */
+#ifndef __ASM_MIPS_I8259_H
+#define __ASM_MIPS_I8259_H
+
+#include <linux/spinlock.h>
+
+#include <asm/io.h>
+#include <asm/system.h>
+
+extern void init_i8259_irqs(void);
+
+/*
+ * Ack an i8259A IRQ for systems that don't use hardware INTA cycles.
+ *
+ * Note we intentionally ignore the "I" (valid) bit reported by the
+ * controllers and pass spurious IRQs as real ones, since they are
+ * rare so the overhead is small and they will be handled later by 
+ * mask_and_ack_8259A() anyway.
+ */
+static inline int poll_8259A_irq(void)
+{
+	extern spinlock_t i8259A_lock;
+	int irq;
+
+	spin_lock(&i8259A_lock);
+
+	outb(0x0c, 0x20);
+	iob();
+	/* Ack the IRQ in the master. */
+	irq = inb(0x20) & 7;
+	if (irq == 2) {
+		outb(0x0c, 0xa0);
+		iob();
+		/* Ditto for the slave. */
+		irq = (inb(0xa0) & 7) + 8;
+	}
+
+	spin_unlock(&i8259A_lock);
+
+	return irq;
+}
+
+#endif /* __ASM_MIPS_I8259_H */
diff -up --recursive --new-file linux-mips-2.4.20-pre6-20021212.macro/include/asm-mips/irq.h linux-mips-2.4.20-pre6-20021212/include/asm-mips/irq.h
--- linux-mips-2.4.20-pre6-20021212.macro/include/asm-mips/irq.h	2002-12-16 17:16:46.000000000 +0000
+++ linux-mips-2.4.20-pre6-20021212/include/asm-mips/irq.h	2002-12-18 15:58:52.000000000 +0000
@@ -24,9 +24,6 @@ static inline int irq_cannonicalize(int 
 #define irq_cannonicalize(irq) (irq)	/* Sane hardware, sane code ... */
 #endif
 
-struct irqaction;
-extern int i8259_setup_irq(int irq, struct irqaction * new);
-
 extern void disable_irq(unsigned int);
 extern void disable_irq_nosync(unsigned int);
 extern void enable_irq(unsigned int);
diff -up --recursive --new-file linux-mips-2.4.20-pre6-20021212.macro/include/asm-mips64/i8259.h linux-mips-2.4.20-pre6-20021212/include/asm-mips64/i8259.h
--- linux-mips-2.4.20-pre6-20021212.macro/include/asm-mips64/i8259.h	1970-01-01 00:00:00.000000000 +0000
+++ linux-mips-2.4.20-pre6-20021212/include/asm-mips64/i8259.h	2002-12-18 00:32:19.000000000 +0000
@@ -0,0 +1,54 @@
+/*
+ *	include/asm-mips/i8259.h
+ *
+ *	i8259A interrupt definitions.
+ *
+ *	Copyright (C) 2002  Maciej W. Rozycki
+ *
+ *	This program is free software; you can redistribute it and/or
+ *	modify it under the terms of the GNU General Public License
+ *	as published by the Free Software Foundation; either version
+ *	2 of the License, or (at your option) any later version.
+ */
+#ifndef __ASM_MIPS64_I8259_H
+#define __ASM_MIPS64_I8259_H
+
+#include <linux/spinlock.h>
+
+#include <asm/io.h>
+#include <asm/system.h>
+
+extern void init_i8259_irqs(void);
+
+/*
+ * Ack an i8259A IRQ for systems that don't use hardware INTA cycles.
+ *
+ * Note we intentionally ignore the "I" (valid) bit reported by the
+ * controllers and pass spurious IRQs as real ones, since they are
+ * rare so the overhead is small and they will be handled later by 
+ * mask_and_ack_8259A() anyway.
+ */
+static inline int poll_8259A_irq(void)
+{
+	extern spinlock_t i8259A_lock;
+	int irq;
+
+	spin_lock(&i8259A_lock);
+
+	outb(0x0c, 0x20);
+	iob();
+	/* Ack the IRQ in the master. */
+	irq = inb(0x20) & 7;
+	if (irq == 2) {
+		outb(0x0c, 0xa0);
+		iob();
+		/* Ditto for the slave. */
+		irq = (inb(0xa0) & 7) + 8;
+	}
+
+	spin_unlock(&i8259A_lock);
+
+	return irq;
+}
+
+#endif /* __ASM_MIPS64_I8259_H */
diff -up --recursive --new-file linux-mips-2.4.20-pre6-20021212.macro/include/asm-mips64/irq.h linux-mips-2.4.20-pre6-20021212/include/asm-mips64/irq.h
--- linux-mips-2.4.20-pre6-20021212.macro/include/asm-mips64/irq.h	2002-12-16 16:56:09.000000000 +0000
+++ linux-mips-2.4.20-pre6-20021212/include/asm-mips64/irq.h	2002-12-18 16:03:00.000000000 +0000
@@ -42,9 +42,6 @@ static inline int irq_cannonicalize(int 
 #define irq_cannonicalize(irq) (irq)	/* Sane hardware, sane code ... */
 #endif
 
-struct irqaction;
-extern int i8259_setup_irq(int irq, struct irqaction * new);
-
 extern void disable_irq(unsigned int);
 extern void disable_irq_nosync(unsigned int);
 extern void enable_irq(unsigned int);

Re: [PATCH] add dispatch_i8259_irq() to i8259.c

[Jun Sun]

On Wed, Dec 18, 2002 at 05:14:19PM +0100, Maciej W. Rozycki wrote:
> On Tue, 17 Dec 2002, Jun Sun wrote:
> 
> > > > No MIPS boards are using do_slow_gettimeoffset().  We really should get
> > > > rid of it.
> > > 
> > >  I know none does at the moment.  But are you sure there is no system that
> > > would need it and might be supported one day?
> > 
> > I serisouly don't think so.  Moving forward every CPU will have a CPU counter,
> > which can be used for timeoffset purpose.  Even if it does not have one,
> > it will surely have some onboard high resolution timer, which can be used
> > to intra-jiffy offset purpose.
> 
>  Well, I do hope so, too, but you'll never know until you find out. ;-)
> 
> > >  Here is an example (untested) code that I would recommend.  It sends
> > > explicit ACKs to the i8259As, which has the following advantages:
> > > 
> > <snip>
> > 
> > Cool.  This code works for me.
> 
>  Excellent.  I worked on the code a bit more and removed the spurious IRQ
> stuff.  It's not really necessary -- mask_and_ack_8259A() will deal with
> it anyway (a bit less precisely, but we don't care -- they are very rare
> and drivers absolutely have to be able to deal with spurious interrupts)
> and we want the low-level IRQ handling to be fast as it's performance
> critical.  At this point the function became so compact it would be
> unreasonable not to make it inline -- the generated code is 24
> instructions on my system.  The positive side effect is the code won't be
> compiled for systems that don't use it.
> 
>  Following is a patch that I consider a candidate for submission.  I
> changed the interface a bit to permit greater flexibility.  I renamed the
> function to reflect the new semantic.  Unless special handling is needed
> you may simply call: 
> 
> do_IRQ(poll_8259A_irq(), regs);
>

I actually don't like the new semantic.  The main drawback is that we can't
dispatch a 8259A interrupt from assemably code, which is often needed.

What is wrong with original way of dispatching?  The general interrupt 
dispatching flow is that you chase the routing path until you find the final
source and do a do_IRQ().  That seems fine with i8259A case here.

While there is certain urge to create asm/i8259a.h file, if in the end all there
is two function declarations (i8259_init() and dispatch_i8259_irq()), it is not
really worth it.

Jun

Re: [PATCH] add dispatch_i8259_irq() to i8259.c

[Maciej W. Rozycki]

On Wed, 18 Dec 2002, Jun Sun wrote:

> > do_IRQ(poll_8259A_irq(), regs);
> 
> I actually don't like the new semantic.  The main drawback is that we can't
> dispatch a 8259A interrupt from assemably code, which is often needed.

 You can't do that with your original code either, unless you arrange a
call to your dispatch_i8259_irq() C function.  This can still be done with
a trivial wrapper like:

asmlinkage void foo_dispatch_i8259_irq(struct pt_regs *regs)
{
	do_IRQ(poll_8259A_irq(), regs);
}

which results in code like you proposed.

> What is wrong with original way of dispatching?  The general interrupt 
> dispatching flow is that you chase the routing path until you find the final
> source and do a do_IRQ().  That seems fine with i8259A case here.

 It does too much and is therefore useful for a single specific case only. 
I focused on handling the chip only and the resulting function may be used
however desired, including your specific case.  Not all platforms need to
want to call do_IRQ() immediately after getting an IRQ number, including
code already in existence. 

> While there is certain urge to create asm/i8259a.h file, if in the end all there
> is two function declarations (i8259_init() and dispatch_i8259_irq()), it is not
> really worth it.

 The header issue is orthogonal -- for lone init_i8259_irqs() it should
exist.  Otherwise you'll be doomed upon the next interface change.

  Maciej

-- 
+  Maciej W. Rozycki, Technical University of Gdansk, Poland   +
+--------------------------------------------------------------+
+        e-mail: macro@ds2.pg.gda.pl, PGP key available        +

Re: [PATCH] add dispatch_i8259_irq() to i8259.c

[Jun Sun]

On Wed, Dec 18, 2002 at 07:14:20PM +0100, Maciej W. Rozycki wrote:
> On Wed, 18 Dec 2002, Jun Sun wrote:
> 
> > > do_IRQ(poll_8259A_irq(), regs);
> > 
> > I actually don't like the new semantic.  The main drawback is that we can't
> > dispatch a 8259A interrupt from assemably code, which is often needed.
> 
>  You can't do that with your original code either, 

What do you mean?  I could do a standard assembly intr dispatching code like
this;

	move	a0, sp
	jal	dispatch_i8259_irq
	j	ret_from_irq

Please cross-check all current assembly-level irq dispatching calls.  They
are all like this.

> unless you arrange a
> call to your dispatch_i8259_irq() C function.  This can still be done with
> a trivial wrapper like:
> 
> asmlinkage void foo_dispatch_i8259_irq(struct pt_regs *regs)
> {
> 	do_IRQ(poll_8259A_irq(), regs);
> }
>

This is essentially the same as adding dispatch_i8259_irq() to i8259.c file,
which in turn calls an inline function as its function body. 

Unless there is obvious another usage of poll_8259A_irq(), the inline function
might as well be not inlined.
 
> which results in code like you proposed.
>

Yes, that is why I liked my original function call. :-)
 
> > What is wrong with original way of dispatching?  The general interrupt 
> > dispatching flow is that you chase the routing path until you find the final
> > source and do a do_IRQ().  That seems fine with i8259A case here.
> 
>  It does too much and is therefore useful for a single specific case only. 
> I focused on handling the chip only and the resulting function may be used
> however desired, including your specific case.  Not all platforms need to
> want to call do_IRQ() immediately after getting an IRQ number, including
> code already in existence. 

Note those platforms don't read i8259 registers to get irq number either.

There is also a style issue.  Dispatching an interrupt is really part of
hw_interrupt_type structure.  You should implement it in the same file as the
rest functions.  However, if anybody feels it is not optimized enough they are
always free to lump all IRQ dispatching code together in one place, probably even
in assembly code.

I also disagree to have a header file with only one function declaration, but I 
agree there is orthognal issue, mostly a maintenance issue.  So if Ralf is ok with that,
I won't bitching about it.

Jun

Re: [PATCH] add dispatch_i8259_irq() to i8259.c

[Maciej W. Rozycki]

On Wed, 18 Dec 2002, Jun Sun wrote:

> > call to your dispatch_i8259_irq() C function.  This can still be done with
> > a trivial wrapper like:
> > 
> > asmlinkage void foo_dispatch_i8259_irq(struct pt_regs *regs)
> > {
> > 	do_IRQ(poll_8259A_irq(), regs);
> > }
> >
> 
> This is essentially the same as adding dispatch_i8259_irq() to i8259.c file,
> which in turn calls an inline function as its function body. 

 It's not the same (assuming you'd use a separate file as not all
platforms need dispatch_i8259_irq()) as for other uses you may need
slightly different duplicates.

> Unless there is obvious another usage of poll_8259A_irq(), the inline function
> might as well be not inlined.

 You lose several cycles for stack frame handling and the extraneous call
itself then. 

> There is also a style issue.  Dispatching an interrupt is really part of
> hw_interrupt_type structure.  You should implement it in the same file as the

 Not at all.  The hw_interrupt_type structure is about handling (or
receiving) interrupts.  The code we are talking about is about dispatching
(or sending) interrupts, which is sometimes done by hardware (e.g. 
probably always for ia32).  These activities are opposite to each other. 

> rest functions.  However, if anybody feels it is not optimized enough they are
> always free to lump all IRQ dispatching code together in one place, probably even
> in assembly code.

 Within sane code we keep dispatchers separate from handlers and that
makes a lot of sense.  Not only they are logically separated, but often
there are multiple unrelated handlers, e.g. three for most of DECstations,
and there is always a single dispatcher, i.e. the interrupt exception
handler.  Also dispatchers are platform-specific, depending on the wiring,
desired priorities, etc., while handlers are generic, depending on an
interrupt controller chip only. 

> I also disagree to have a header file with only one function declaration, but I 
> agree there is orthognal issue, mostly a maintenance issue.  So if Ralf is ok with that,
> I won't bitching about it.

 It's better to have a self-contained header for a single declaration than
a single header collecting various weakly related junk -- see
<linux/sched.h> (and keep a barf bag handy -- you have been warned).

-- 
+  Maciej W. Rozycki, Technical University of Gdansk, Poland   +
+--------------------------------------------------------------+
+        e-mail: macro@ds2.pg.gda.pl, PGP key available        +