[PATCH 4/5] Centralise NO_IRQ definition

[PATCH 4/5] Centralise NO_IRQ definition

[Matthew Wilcox]

Move the definition of NO_IRQ from asm directories to <linux/hardirq.h>.
All existing definitions were already -1.

Signed-off-by: Matthew Wilcox <matthew@wil.cx>
Acked-by: Ingo Molnar <mingo@elte.hu>

---

 include/asm-arm/irq.h     |    8 --------
 include/asm-arm26/irq.h   |    8 --------
 include/asm-frv/irq.h     |    3 ---
 include/asm-parisc/irq.h  |    2 --
 include/asm-powerpc/irq.h |    3 ---
 include/linux/hardirq.h   |    8 ++++++++
 6 files changed, 8 insertions(+), 24 deletions(-)

applies-to: c2fd3b28a74be2ae6274c85c7a9538c7bf1dd949
604c0538936822cbb1c0958f0f835cbbaf723530
diff --git a/include/asm-arm/irq.h b/include/asm-arm/irq.h
index 59975ee..0545e36 100644
--- a/include/asm-arm/irq.h
+++ b/include/asm-arm/irq.h
@@ -11,14 +11,6 @@
 #define NR_IRQS	128
 #endif
 
-/*
- * Use this value to indicate lack of interrupt
- * capability
- */
-#ifndef NO_IRQ
-#define NO_IRQ	((unsigned int)(-1))
-#endif
-
 struct irqaction;
 
 extern void disable_irq_nosync(unsigned int);
diff --git a/include/asm-arm26/irq.h b/include/asm-arm26/irq.h
index 06bd5a5..7957d4e 100644
--- a/include/asm-arm26/irq.h
+++ b/include/asm-arm26/irq.h
@@ -14,14 +14,6 @@
 #endif
 
 
-/*
- * Use this value to indicate lack of interrupt
- * capability
- */
-#ifndef NO_IRQ
-#define NO_IRQ	((unsigned int)(-1))
-#endif
-
 struct irqaction;
 
 #define disable_irq_nosync(i) disable_irq(i)
diff --git a/include/asm-frv/irq.h b/include/asm-frv/irq.h
index 2c16d8d..fbc5bd7 100644
--- a/include/asm-frv/irq.h
+++ b/include/asm-frv/irq.h
@@ -20,9 +20,6 @@
  * drivers
  */
 
-/* this number is used when no interrupt has been assigned */
-#define NO_IRQ				(-1)
-
 #define NR_IRQ_LOG2_ACTIONS_PER_GROUP	5
 #define NR_IRQ_ACTIONS_PER_GROUP	(1 << NR_IRQ_LOG2_ACTIONS_PER_GROUP)
 #define NR_IRQ_GROUPS			4
diff --git a/include/asm-parisc/irq.h b/include/asm-parisc/irq.h
index b0a30e2..26ab3be 100644
--- a/include/asm-parisc/irq.h
+++ b/include/asm-parisc/irq.h
@@ -11,8 +11,6 @@
 #include <linux/cpumask.h>
 #include <asm/types.h>
 
-#define NO_IRQ		(-1)
-
 #ifdef CONFIG_GSC
 #define GSC_IRQ_BASE	16
 #define GSC_IRQ_MAX	63
diff --git a/include/asm-powerpc/irq.h b/include/asm-powerpc/irq.h
index 8eb7e85..1b41206 100644
--- a/include/asm-powerpc/irq.h
+++ b/include/asm-powerpc/irq.h
@@ -15,9 +15,6 @@
 #include <asm/types.h>
 #include <asm/atomic.h>
 
-/* this number is used when no interrupt has been assigned */
-#define NO_IRQ			(-1)
-
 /*
  * These constants are used for passing information about interrupt
  * signal polarity and level/edge sensing to the low-level PIC chip
diff --git a/include/linux/hardirq.h b/include/linux/hardirq.h
index 71d2b8a..9ee4946 100644
--- a/include/linux/hardirq.h
+++ b/include/linux/hardirq.h
@@ -87,6 +87,14 @@ extern void synchronize_irq(unsigned int
 # define synchronize_irq(irq)	barrier()
 #endif
 
+/*
+ * This value means "Device has no interrupt".  For most pieces of code,
+ * any value above NR_IRQS would do, but -1 is traditional.  The PCI
+ * subsystem currently uses 0, but that's a legal IRQ number on some
+ * architectures.
+ */
+#define NO_IRQ			((unsigned int)(-1))
+
 #define nmi_enter()		irq_enter()
 #define nmi_exit()		sub_preempt_count(HARDIRQ_OFFSET)
 
---
0.99.8.GIT

Re: [PATCH 4/5] Centralise NO_IRQ definition

[David Howells]

Matthew Wilcox <matthew@wil.cx> wrote:

> +#define NO_IRQ			((unsigned int)(-1))

Should this be wrapped with #ifndef?

David

Re: [PATCH 4/5] Centralise NO_IRQ definition

[Matthew Wilcox]

On Mon, Nov 21, 2005 at 11:12:36AM +0000, David Howells wrote:
> Matthew Wilcox <matthew@wil.cx> wrote:
> 
> > +#define NO_IRQ			((unsigned int)(-1))
> 
> Should this be wrapped with #ifndef?

*sigh*.  The one piece of feedback I got on the last series was from
Ingo, and he asked that I *not* wrap it with ifndef.  So, no.

Re: [PATCH 4/5] Centralise NO_IRQ definition

[Linus Torvalds]

On Mon, 21 Nov 2005, Matthew Wilcox wrote:
>
> On Mon, Nov 21, 2005 at 11:12:36AM +0000, David Howells wrote:
> > Matthew Wilcox <matthew@wil.cx> wrote:
> > 
> > > +#define NO_IRQ			((unsigned int)(-1))
> > 
> > Should this be wrapped with #ifndef?
> 
> *sigh*.  The one piece of feedback I got on the last series was from
> Ingo, and he asked that I *not* wrap it with ifndef.  So, no.

Quite frankly, if we change [PCI_]NO_IRQ to -1, there's almost certainly 
going to be a lot of drivers breaking.

On x86, 0 has been the lack of IRQ since basically forever, in one form or 
another (for devices, that is - there _is_ a timer irq 0, but that's set 
up separately).

Which means that I'd _much_ prefer to other architectures to just follow 
suit.

So for an architecture where irq0 is a valid physical interrupt for a 
device, why not just translate that "real irq 0" into some other thing? 
Much less likely to break anything.

In fact, it could be as simple as setting the high bit for any valid 
interrupt, and then just masking it out in request_irq() and friends. Do 
something like

	#define PCI_IRQ_VALID_MASK	(1u << 31)

and then if the physical irq is 0, just or in that VALID mask, turning the 
interrupt into a non-zero, and then when registering it, just and it away 
again..

This is not theory: a _lot_ of real-life PCI devices very much think that 
irq 0 means "disabled". Not even just in drivers - in actual _hardware_. 
When you write 0 to the irq number for irq routers, they disable that 
line. So the "zero as NO_IRQ" is more than just a "several drivers think 
that is how it is", it's how a lot of hardware actually works.

		Linus

Re: [PATCH 4/5] Centralise NO_IRQ definition

[Matthew Wilcox]

On Mon, Nov 21, 2005 at 10:55:24AM -0800, Linus Torvalds wrote:
> Quite frankly, if we change [PCI_]NO_IRQ to -1, there's almost certainly 
> going to be a lot of drivers breaking.

There's only one driver using NO_IRQ today (outside of architectures
which define NO_IRQ to -1, that is).  So *this* series of patches should
break nothing.  The next series of patches should find all the PCI
drivers assuming dev->irq == 0 means no interrupt, and then we try to
break drivers by getting rid of PCI_NO_IRQ.

> This is not theory: a _lot_ of real-life PCI devices very much think that 
> irq 0 means "disabled". Not even just in drivers - in actual _hardware_. 
> When you write 0 to the irq number for irq routers, they disable that 
> line. So the "zero as NO_IRQ" is more than just a "several drivers think 
> that is how it is", it's how a lot of hardware actually works.

That's a common misreading of the PCI spec -- it actually says the
opposite.  Interrupt Pin works as you've described.  But what we're
concerned with is Interrupt Line, and PCI 2.3 has the following to say
in a footnote:

	43 For x86 based PCs, the values in this register correspond to
	IRQ numbers (0-15) of the standard dual 8259 configuration. The
	value 255 is defined as meaning "unknown" or "no connection" to
	the interrupt controller. Values between 15 and 254 are reserved.

It's just that Linux doesn't bother to read Interrupt Line if Interrupt
Pin is 0, and thus leaves the interrupt value at 0 (since the struct was
memset).

Really, this patch brings x86 back into compliance with what the
BIOS does ;-)

Re: [PATCH 4/5] Centralise NO_IRQ definition

[Linus Torvalds]

On Mon, 21 Nov 2005, Matthew Wilcox wrote:
>
> On Mon, Nov 21, 2005 at 10:55:24AM -0800, Linus Torvalds wrote:
> > Quite frankly, if we change [PCI_]NO_IRQ to -1, there's almost certainly 
> > going to be a lot of drivers breaking.
> 
> There's only one driver using NO_IRQ today (outside of architectures
> which define NO_IRQ to -1, that is).  So *this* series of patches should
> break nothing.

Right. But the point is that most drivers will do something like

	if (!dev->irq)
		return;

(whatever, made up). And that having NO_IRQ be anything but 0 is thus 
fundamentally broken.

Because the fact is, NO_IRQ _is_ zero. And exactly because it's zero, and 
that is encoded everywhere, nobody uses it.

> That's a common misreading of the PCI spec -- it actually says the
> opposite.

I'm NOT talking about PCI specs.

I'm talking about real hardware.

Read pretty much _any_ data-sheet for an interrupt router, and you'll see 
that the bit pattern 0000 means _disabled_. 

In other words, I'm talking about HARD REALITY.

I know, it's a bitch.

		Linus

Re: [PATCH 4/5] Centralise NO_IRQ definition

[Matthew Wilcox]

On Mon, Nov 21, 2005 at 11:27:05AM -0800, Linus Torvalds wrote:
> On Mon, 21 Nov 2005, Matthew Wilcox wrote:
> >
> > On Mon, Nov 21, 2005 at 10:55:24AM -0800, Linus Torvalds wrote:
> > > Quite frankly, if we change [PCI_]NO_IRQ to -1, there's almost certainly 
> > > going to be a lot of drivers breaking.
> > 
> > There's only one driver using NO_IRQ today (outside of architectures
> > which define NO_IRQ to -1, that is).  So *this* series of patches should
> > break nothing.
> 
> Right. But the point is that most drivers will do something like
> 
> 	if (!dev->irq)
> 		return;
> 
> (whatever, made up). And that having NO_IRQ be anything but 0 is thus 
> fundamentally broken.

The idea was to give them something better to use instead of this.
Whether that be if (has_irq(dev)) return; or some other similar
construct, I'm not terribly fussed.

> I'm NOT talking about PCI specs.
> 
> I'm talking about real hardware.
> 
> Read pretty much _any_ data-sheet for an interrupt router, and you'll see 
> that the bit pattern 0000 means _disabled_. 

The only relevant thing I found with google was
http://www.microsoft.com/whdc/archive/pciirq.mspx

Where it talks about 0 meaning disabled, it says:

	Link Value for INTn#:A value of zero means this interrupt pin is
	not connected to any other interrupt pins and is not connected
	to any of the Interrupt Router's interrupt pins.

which is a different bit from where it talks about the AT-compatible
IRQ numbers.

Everything else I find seems to be talking about Arcnet hardware (!)

Re: [PATCH 4/5] Centralise NO_IRQ definition

[Linus Torvalds]

On Mon, 21 Nov 2005, Matthew Wilcox wrote:
> > 
> > 	if (!dev->irq)
> > 		return;
> > 
> > (whatever, made up). And that having NO_IRQ be anything but 0 is thus 
> > fundamentally broken.
> 
> The idea was to give them something better to use instead of this.

Why?

The above is obvious even to a non-programmer. There _is_ no better thing 
from a clarity standpoint.

> The only relevant thing I found with google was
> http://www.microsoft.com/whdc/archive/pciirq.mspx

Look at, for example, any cardbus controller. The way to disable 
generation of the legacy interrupt? Write an irq value of 0 to the 
interrupt control register.

Or check out the irq routers in arch/i386/pci/irq.c. Every _single_ one of 
them does the same. Zero means disabled.

The point I'm trying to make is that there already _is_ a special bit 
pattern that means "no irq", and it's what

 - the kernel has used for the last 14 years
 - is the easiest and most logical one to test for
 - real hardware uses

so why not just use it?

Anybody who says that

	if (!dev->irq)
		return;

isn't logical is just on drugs. It's logical _and_ readable. More so than 
any alternative, especially as any alternative will always have the 
problem that some people will write the above _anyway_.

If you make NO_IRQ be something else than 0, for safety you should also 
make the irq thing be a structure so that the test against zero (which is 
special, as _defined_ by the C language) won't work.

At which point you might as well just do something like

	struct interrupt_descriptor {
		unsigned int nr:31;
		unsigned int valid:1;
	};

and then people can just say

	if (!dev->irq.valid)
		return;

instead, which is also readable, and where you simply cannot do the old 
"if (!dev->irq)" at all.

The fact is, 0 _is_ special. Not just for hardware, but because 0 has a 
magical meaning as "false" in the C language.

			Linus

Re: [PATCH 4/5] Centralise NO_IRQ definition

[Ingo Molnar]

* Linus Torvalds <torvalds@osdl.org> wrote:

> At which point you might as well just do something like
> 
> 	struct interrupt_descriptor {
> 		unsigned int nr:31;
> 		unsigned int valid:1;
> 	};
> 
> and then people can just say
> 
> 	if (!dev->irq.valid)
> 		return;
> 
> instead, which is also readable, and where you simply cannot do the old 
> "if (!dev->irq)" at all.
> 
> The fact is, 0 _is_ special. Not just for hardware, but because 0 has 
> a magical meaning as "false" in the C language.

yeah, i wanted to suggest this originally, but got distracted by the x86 
quirk that 'IRQ#0' is often the i8253 timer interrupt.

is there any architecture where irq 0 is a legitimate setting that could 
occur in drivers, and which would make NO_IRQ define of 0 non-practical?  
If not (which i think is the case) then we should indeed standardize on 
0. (in one way or another) It's not like any real driver will ever have 
IRQ#0 even on a PC: the timer IRQ is 'known' to be routed to 0, and we 
do a platform-specific setup_irq() on it. Not a good reason to abstract 
the notion of 'no irq' away into a define.

in fact we dont even have to do the irq.valid thing, !dev->irq is 
obviously readable.

	Ingo

Re: [PATCH 4/5] Centralise NO_IRQ definition

[Paul Mackerras]

Ingo Molnar writes:

> is there any architecture where irq 0 is a legitimate setting that could 
> occur in drivers, and which would make NO_IRQ define of 0 non-practical?  

Yes, G5 powermacs have the SATA controller on irq 0.  So if we can't
use irq 0, I can't get to my hard disk. :)  Other powermacs also use
irq 0 for various things, as do embedded PPC machines.

Paul.

Re: [PATCH 4/5] Centralise NO_IRQ definition

[Ingo Molnar]

* Paul Mackerras <paulus@samba.org> wrote:

> Ingo Molnar writes:
> 
> > is there any architecture where irq 0 is a legitimate setting that could 
> > occur in drivers, and which would make NO_IRQ define of 0 non-practical?  
> 
> Yes, G5 powermacs have the SATA controller on irq 0.  So if we can't 
> use irq 0, I can't get to my hard disk. :) Other powermacs also use 
> irq 0 for various things, as do embedded PPC machines.

oh well [*]. Then it's gotta be the !dev->irq.valid thing i guess. OTOH 
that has some disadvantages too: any normal access to dev->irq.nr will 
mean implicit 0xffffffff (or 0xffffffffffffffff) masking generated by 
the compiler. Also, unless there's some compiler trick, tons of drivers 
will be affected - because dev->irq isnt valid anymore. A quick grep 
suggests 5381 lines of code affected, spread out in 917 files. Quite 
impractical.  So we are back to square one and Matthew's patch(es).

	Ingo

[*] is there any weird architecture that hardcodes IRQ -1 to some
    device? ;-)

Re: [PATCH 4/5] Centralise NO_IRQ definition

[Linus Torvalds]

On Mon, 21 Nov 2005, Ingo Molnar wrote:
> 
> oh well [*]. Then it's gotta be the !dev->irq.valid thing i guess. 

No it's not.

The ppc PCI probing could trivilly just turn a 0 into 256 (or equivalent), 
and mask off the low 7 bits when installing the handler.  They know the 
interrupt is _really_ 0 from other sources (ie they have a different 
firmware, with explicit callbacks, and/or hardcoded knowledge).

		Linus

Re: [PATCH 4/5] Centralise NO_IRQ definition

[Benjamin Herrenschmidt]

On Mon, 2005-11-21 at 13:51 -0800, Linus Torvalds wrote:
> 
> On Mon, 21 Nov 2005, Ingo Molnar wrote:
> > 
> > oh well [*]. Then it's gotta be the !dev->irq.valid thing i guess. 
> 
> No it's not.
> 
> The ppc PCI probing could trivilly just turn a 0 into 256 (or equivalent), 
> and mask off the low 7 bits when installing the handler.  They know the 
> interrupt is _really_ 0 from other sources (ie they have a different 
> firmware, with explicit callbacks, and/or hardcoded knowledge).

The ppc irq handling is more complex than that due to the wide range of
different hardware. We parse the irq tree from OF and assign them to
ranges of numbers allocated per controller. Adding some remapping of
some numbers would add complexity and possible bugs.

Ben.

Re: [PATCH 4/5] Centralise NO_IRQ definition

[Linus Torvalds]

On Tue, 22 Nov 2005, Benjamin Herrenschmidt wrote:
> 
> The ppc irq handling is more complex than that due to the wide range of
> different hardware. We parse the irq tree from OF and assign them to
> ranges of numbers allocated per controller. Adding some remapping of
> some numbers would add complexity and possible bugs.

Let me rephrase that as: "On ppc we already _do_ irq mapping, but we made 
a mistake to map to 0, so to cover up that mistake we now want everybody 
else to change how they have done things since day 1".

See? You argue that there might be "possible bugs". Yet you totally ignore 
the fact that I can absolutely point to code that we _know_ depends on 
"zero means no irq". So when you argue for changing away from that, I 
GUARANTEE you that it has a higher likelihood of bugs AND that it affects 
a hell of a lot more code than the alternative I propose.

Anyway, let's just stop this discussion. Let's just leave PCI_NO_IRQ as 0 
on x86, and let PPC has it as -1. Problem solved. That way we are 
guaranteed to not introduce any new bugs. It works today, and we can just 
ignore this whole patch-series, since centralizing NO_IRQ is clearly the 
wrong thing to do.

By centralizing NO_IRQ, you either have to break a lot of existing PC 
setups, or you have to potentially break (far fewer) PowerPC setups. So 
let's not do it at all.

		Linus

Re: [PATCH 4/5] Centralise NO_IRQ definition

[Benjamin Herrenschmidt]

> By centralizing NO_IRQ, you either have to break a lot of existing PC 
> setups, or you have to potentially break (far fewer) PowerPC setups. So 
> let's not do it at all.

I have no strong feeling vs. centralized or not centralized NO_IRQ
value. All I want is NO_IRQ to exist on all archs so I can fix the few
drivers that assume that 0 is no irq.

Ben.

Re: [PATCH 4/5] Centralise NO_IRQ definition

[Linus Torvalds]

On Tue, 22 Nov 2005, Paul Mackerras wrote:
> 
> Yes, G5 powermacs have the SATA controller on irq 0.  So if we can't
> use irq 0, I can't get to my hard disk. :)  Other powermacs also use
> irq 0 for various things, as do embedded PPC machines.

That doesn't change any of the logic. There already is no "1:1" mapping of 
PCI interrupts to what the machine does.

On all PC hardware, having a zero in the PCI irq register basically means 
that no irq is enabled. That's a _fact_. It's a fact however much you may 
not like it. It's how the hardware comes up, and it's how the BIOS leaves 
it. So "0" absolutely does mean "not allocated". 

Now, the second part of the story is that when it comes to PCI, it doesn't 
matter what Apple, Sun, or pretty much anybody else has done.  The reason 
PCI has a separate MMIO and IO space is that it comes from a PC 
background, and the reason Apple and others use PCI is that through that, 
there are thousands of controller cards that are sold for PC's that also 
happen to work on non-PC's.

So PC usage really is a defining part of PCI. It's what defines basically 
_all_ of the testing, even under Linux. 

So let's face those facts:
 - we have a 8-bit register (0-255) for firmware telling the kernel what 
   the pre-allocated interrupt is.
 - all of those 256 numbers _may_ in fact be valid on some piece of 
   hardware.
 - only one of those numbers (0) is de-facto the "no irq line set up" 
   value.
 - pretty much all drivers have been tested mainly with 0 being the "no 
   irq" value.

Those are FACTS. Denying them is a sign of stupidity.

I'd suggest that if some architecture can't live with those facts, it 
either:

 - define it's own PCI_NO_IRQ value, and face the fact that it will have 
   to test the drivers and hope they work (and that a lot of them simply 
   will _not_ work). 

   This is what we have today. It mostly works. Maybe we shouldn't change 
   it.

 - realize that 0 is special, and use another number for when firmware 
   tells it 0 _is_ actually a valid irq (maybe 256. Maybe "1u<<31". I 
   don't care.)

And I won't apply the "turn PCI_NO_IRQ" into -1 generally, because I 
consider it to be strictly _worse_ than what we have now.

Comprende?

		Linus

Re: [PATCH 4/5] Centralise NO_IRQ definition

[Benjamin Herrenschmidt]

> On all PC hardware, having a zero in the PCI irq register basically means 
> that no irq is enabled. That's a _fact_. It's a fact however much you may 
> not like it. It's how the hardware comes up, and it's how the BIOS leaves 
> it. So "0" absolutely does mean "not allocated". 

It's not the case on various non-x86 architectures, not the case in the
PCI spec neither.

> Now, the second part of the story is that when it comes to PCI, it doesn't 
> matter what Apple, Sun, or pretty much anybody else has done.  The reason 
> PCI has a separate MMIO and IO space is that it comes from a PC 
> background, and the reason Apple and others use PCI is that through that, 
> there are thousands of controller cards that are sold for PC's that also 
> happen to work on non-PC's.

And ?

> So PC usage really is a defining part of PCI. It's what defines basically 
> _all_ of the testing, even under Linux. 
> 
> So let's face those facts:
>  - we have a 8-bit register (0-255) for firmware telling the kernel what 
>    the pre-allocated interrupt is.

That register is mostly useless on a wide range of architectures tho :)

>  - all of those 256 numbers _may_ in fact be valid on some piece of 
>    hardware.
>  - only one of those numbers (0) is de-facto the "no irq line set up" 
>    value.

No, it's not.

>  - pretty much all drivers have been tested mainly with 0 being the "no 
>    irq" value.
> 
> Those are FACTS. Denying them is a sign of stupidity.

I must have lost a lot of neurons in the past few years then.

> I'd suggest that if some architecture can't live with those facts, it 
> either:
> 
>  - define it's own PCI_NO_IRQ value, and face the fact that it will have 
>    to test the drivers and hope they work (and that a lot of them simply 
>    will _not_ work). 

Very few driver bother at all about the irq value. Most don't test it,
they just use it and be done with it, and that's the way it works. Some
drivers, mostly ISA stuff or legacy stuff or drivers that deal with old
weird chipsets actually look at the irq value, and those can be easily
fixed to use NO_IRQ. I really don't understand why you are making such a
fuss about this...

Re: [PATCH 4/5] Centralise NO_IRQ definition

[Linus Torvalds]

On Tue, 22 Nov 2005, Benjamin Herrenschmidt wrote:
> 
> > On all PC hardware, having a zero in the PCI irq register basically means 
> > that no irq is enabled. That's a _fact_. It's a fact however much you may 
> > not like it. It's how the hardware comes up, and it's how the BIOS leaves 
> > it. So "0" absolutely does mean "not allocated". 
> 
> It's not the case on various non-x86 architectures, not the case in the
> PCI spec neither.

So?

The point is, we have to handle the 0 on a PC architecture _anyway_.

The PCI spec does NOT MATTER. The 0 case is a _fact_. 

If the PCI spec said that the world was flat, would that make a 
difference? No. 

The fact is, we need to pick some value for "No irq", and that value needs 
translation. I claim that 0 is superior, because
 - we know it works on the biggest hardware base
 - it's de facto what pretty much all of the current source code _does_ 
   anyway
 - it's easier to test for, and more importantly it allows the most 
   natural source code syntax with fewer mistakes.
 - it requires no more translation than any other value, including -1.

These are all just undeniable facts. 

In contrast, the PCI spec gives us a 8-bit value that is _known_ to not be 
sufficient anyway: (a) it has no "undefined" value at all, although one is 
clearly needed (and on PC's, 0 _is_ that value) and (b) it's really too 
small to cover the possible interrupt sources anyway (ie big machines 
already tend to have more than 256 interrupts).

So you could just make the ppc PCI probing do

	dev->irq = PCI_IRQ_BASE + node->intrs[0].line;

and suddenly 0 works equally well for you as it does on a regular PC. 

Notice? Magic. Suddenly "0" means "No irq" on ppc too.

		Linus

Re: [PATCH 4/5] Centralise NO_IRQ definition

[Benjamin Herrenschmidt]

> So you could just make the ppc PCI probing do
> 
> 	dev->irq = PCI_IRQ_BASE + node->intrs[0].line;
> 
> and suddenly 0 works equally well for you as it does on a regular PC. 
> 
> Notice? Magic. Suddenly "0" means "No irq" on ppc too.

Not really, we also need to fix the interrupt controller code among
others...

Ben.

Re: [PATCH 4/5] Centralise NO_IRQ definition

[Paul Mackerras]

Linus Torvalds writes:

> On all PC hardware, having a zero in the PCI irq register basically means 
> that no irq is enabled. That's a _fact_. It's a fact however much you may 
> not like it. It's how the hardware comes up, and it's how the BIOS leaves 
> it. So "0" absolutely does mean "not allocated". 

First, are you talking about the interrupt pin register or the
interrupt line register?

Secondly, I would say that any driver that looks at either of those
registers is broken.  Drivers should be looking at dev->irq, which is
set up by platform code, and may be quite different from what is in
the interrupt line register.

So the question of what PCI devices do with the value in the interrupt
line register is pretty much irrelevant.  Those few devices that can
control their interrupt routing (such as cardbus bridges) don't use
the interrupt line register for that AFAIK.

I think all we need is for the convention for drivers to be that they
test for whether the device has an interrupt is to test (dev->irq !=
NO_IRQ).  Then x86 can have NO_IRQ = 0 and PPC can have NO_IRQ = -1
and I think everyone is happy (well, nearly everyone, anyway :).

Paul.

Re: [PATCH 4/5] Centralise NO_IRQ definition

[Linus Torvalds]

On Tue, 22 Nov 2005, Paul Mackerras wrote:
> 
> First, are you talking about the interrupt pin register or the
> interrupt line register?

Interrupt line. The interrupt pin is totally separate.

> Secondly, I would say that any driver that looks at either of those
> registers is broken.  Drivers should be looking at dev->irq, which is
> set up by platform code, and may be quite different from what is in
> the interrupt line register.

But that's part of the POINT, Paul.

The platform code needs to set up something in dev->irq. And we have 
_always_ had "dev->irq == 0" meaning "no irq".

So if PCI irq (the interrupt line register or whatever) 0 means something 
for you on PPC, then BY DEFINITION you should not have translated it into 
"dev->irq". But PPC did. Tough. Don't blame that mistake on me, or try to 
force that mistake on other architectures.

The fact that PPC screwed that up is a PPC problem, and it's a PPC problem 
from the very beginning, because the "dev->irq" value doesn't have to 
match the PCI irq at all.

On sparc, for example, "dev->irq" used to be some random cookie, if I 
remember correctly. But "0" still meant "unallocated".

So face it. PPC screwed up, and if it had just followed what the 
"dev->irq" meant on the regular x86 platforms, it wouldn't have needed 
that NO_IRQ in the first place.

The whole notion of needing "NO_IRQ" is broken. The way to test for not 
having an irq is "!dev->irq". Any architecture that uses NO_IRQ is just a 
bug waiting to happen, for any number of drivers. And for no good reason.

		Linus

Re: [PATCH 4/5] Centralise NO_IRQ definition

[Matthew Wilcox]

On Mon, Nov 21, 2005 at 01:49:45PM -0800, Linus Torvalds wrote:
> On all PC hardware, having a zero in the PCI irq register basically means 
> that no irq is enabled. That's a _fact_. It's a fact however much you may 
> not like it. It's how the hardware comes up, and it's how the BIOS leaves 
> it. So "0" absolutely does mean "not allocated". 

Actually, no.  Here's my x86 laptop's config space ...

0000:02:06.2 System peripheral: Texas Instruments PCI1620 Firmware Loading Function (rev 01)
        Subsystem: Hewlett-Packard Company: Unknown device 08b0
        Flags: bus master, medium devsel, latency 64
        I/O ports at 4000 [size=64]
        Capabilities: [44] Power Management version 2
00: 4c 10 01 82 07 01 10 02 01 00 80 08 08 40 00 00
10: 01 40 00 00 00 00 00 00 00 00 00 00 00 00 00 00
20: 00 00 00 00 00 00 00 00 00 00 00 00 3c 10 b0 08
30: 00 00 00 00 44 00 00 00 00 00 00 00 ff 00 07 04

Interrupt Pin is 0x3d and Interrupt Line is 0x3c.  As I explained earlier,
Linux simply never reads Interrupt Line if Interrupt Pin is 0.


Anyway, this doesn't matter too much.  I don't personally care about
NO_IRQ being 0 or -1; that was something benh tried to make me care
about ;-)

So, how about this for a new patch series:

 - Patch 1, same as this series (it just makes sense to bounds-check in
   the irq management functions).
 - Patch 2, add #ifndef NO_IRQ #define NO_IRQ 0 #endif to linux/hardirq.h
 - Patch 3, set dev->irq to NO_IRQ in drivers/pci/probe.c
 - Patch 4, remove custom definition of NO_IRQ from pd6729
 - Patch 5, use NO_IRQ in serial_core

I'll start work on that now, since that fixes the problems I care about
and doesn't negatively affect people with problems I don't care about ;-)

Re: [PATCH 4/5] Centralise NO_IRQ definition

[Benjamin Herrenschmidt]

On Tue, 2005-11-22 at 08:25 +1100, Paul Mackerras wrote:
> Ingo Molnar writes:
> 
> > is there any architecture where irq 0 is a legitimate setting that could 
> > occur in drivers, and which would make NO_IRQ define of 0 non-practical?  
> 
> Yes, G5 powermacs have the SATA controller on irq 0.  So if we can't
> use irq 0, I can't get to my hard disk. :)  Other powermacs also use
> irq 0 for various things, as do embedded PPC machines.

And other non-ppc embedded things I've seen in the past... I think it's
quite common outside of the x86 world

Ben.

Re: [PATCH 4/5] Centralise NO_IRQ definition

[Alan Cox]

On Maw, 2005-11-22 at 08:25 +1100, Paul Mackerras wrote:
> Ingo Molnar writes:
> 
> > is there any architecture where irq 0 is a legitimate setting that could 
> > occur in drivers, and which would make NO_IRQ define of 0 non-practical?  
> 
> Yes, G5 powermacs have the SATA controller on irq 0.  So if we can't
> use irq 0, I can't get to my hard disk. :)  Other powermacs also use
> irq 0 for various things, as do embedded PPC machines.

G5 powermacs have the SATA controller on physical IRQ value 0. Linux IRQ
values don't need to exactly map. One of the x86 ports handles 'real IRQ
0' exactly this way. Its a cookie. Sure would benefit from a function
for turning an IRQ into a description as a cleanup.

Alan

Re: [PATCH 4/5] Centralise NO_IRQ definition

[David Woodhouse]

On Mon, 2005-11-21 at 22:20 +0000, Alan Cox wrote:
> > Yes, G5 powermacs have the SATA controller on irq 0.  So if we can't
> > use irq 0, I can't get to my hard disk. :)  Other powermacs also use
> > irq 0 for various things, as do embedded PPC machines.
> 
> G5 powermacs have the SATA controller on physical IRQ value 0. Linux IRQ
> values don't need to exactly map. One of the x86 ports handles 'real IRQ
> 0' exactly this way. Its a cookie. Sure would benefit from a function
> for turning an IRQ into a description as a cleanup.

Remapping in that way sounds like a half-arsed hack to work around the
problem which Matthew is trying to fix properly by using NO_IRQ == -1.

Yes, there are drivers which are currently broken and assume irq 0 is
'no irq'. They are broken. Let's just fix them and not continue the
brain-damage.

-- 
dwmw2

Re: [PATCH 4/5] Centralise NO_IRQ definition

[Alan Cox]

On Maw, 2005-11-22 at 11:13 +0000, David Woodhouse wrote:
> Yes, there are drivers which are currently broken and assume irq 0 is
> 'no irq'. They are broken. Let's just fix them and not continue the
> brain-damage.

0 in the Linux kernel has always meant 'no IRQ' and it makes it natural
to express in C (and on some cpus more efficient too).

What if my hardware has an IRQ -1 ;)

Re: [PATCH 4/5] Centralise NO_IRQ definition

[Matthew Wilcox]

On Tue, Nov 22, 2005 at 02:15:39PM +0000, Alan Cox wrote:
> On Maw, 2005-11-22 at 11:13 +0000, David Woodhouse wrote:
> > Yes, there are drivers which are currently broken and assume irq 0 is
> > 'no irq'. They are broken. Let's just fix them and not continue the
> > brain-damage.
> 
> 0 in the Linux kernel has always meant 'no IRQ' and it makes it natural
> to express in C (and on some cpus more efficient too).
> 
> What if my hardware has an IRQ -1 ;)

Then it falls off the bottom of the irq_desc array.  Already tried that,
hence patch 1/5.

That was a great one to debug, btw.  "The machine hangs when I select
processor type PA8000 but works with processor type PA7200".  Why?
synchronize_irq() spins waiting for a bit to become clear ... for some
reason that bit was always set with PA8000 and always clear with PA7200.
http://lists.parisc-linux.org/pipermail/parisc-linux/2005-October/027485.html

Re: [PATCH 4/5] Centralise NO_IRQ definition

[Linus Torvalds]

On Tue, 22 Nov 2005, Alan Cox wrote:
>
> On Maw, 2005-11-22 at 11:13 +0000, David Woodhouse wrote:
> > Yes, there are drivers which are currently broken and assume irq 0 is
> > 'no irq'. They are broken. Let's just fix them and not continue the
> > brain-damage.
> 
> 0 in the Linux kernel has always meant 'no IRQ' and it makes it natural
> to express in C (and on some cpus more efficient too).

Ahh, a voice of sanity!

> What if my hardware has an IRQ -1 ;)

And "-1" isn't actually a valid value in the first place.

The struct pci_device definition is:

	unsigned int irq;

and anybody who uses -1 is just a total idiot and nincompoop. It's going 
to be a major pain in the ass (others use "int irq", others use "unsigned 
long irq").

Using (~0u) would be more correct, but still insane.

The fact is, 0 _is_ "no interrupt". Always has been. And anybody who says 
that "-1" is "correct" is just totally wrong. Making it -1 would be 
guaranteed to generate tons of breakage, and most people won't ever even 
notice, because in most cases the irq _is_ actually there, and you never 
hit the path. Which just makes the breakage EVEN WORSE.

So David Woodhouse, you're just wrong. But hey, you seem to (sadly) not be 
alone.

In short: NO_IRQ _is_ 0. Always has been. It's the only sane value. And 
btw, there is no need for that #define at all, exactly because the way you 
test for "is this no irq" is by doing "!dev->irq".

Anybody who does anything else is a bug waiting to happen.

		Linus

Re: [PATCH 4/5] Centralise NO_IRQ definition

[Matthew Wilcox]

On Tue, Nov 22, 2005 at 09:03:12AM -0800, Linus Torvalds wrote:
> In short: NO_IRQ _is_ 0. Always has been. It's the only sane value. And 
> btw, there is no need for that #define at all, exactly because the way you 
> test for "is this no irq" is by doing "!dev->irq".

Could you at least take the first patch that checks that we don't go
outside the bounds of the irq_desc array?

Re: [PATCH 4/5] Centralise NO_IRQ definition

[David Howells]

Linus Torvalds <torvalds@osdl.org> wrote:

> The fact is, 0 _is_ "no interrupt". Always has been.

The fact that it always has been doesn't make it correct; and the fact that
the C particularly likes zeros is only optimisable on some hardware. On RISC
processors where you can't test memory directly, it seems to be the case that
any small positive or negative integer is as good as zero; for instance, in
FRV:

	ldi	@(gr8,#0),gr4		# doesn't set the condition codes
	subicc	gr4,#-1,gr0,icc0	# cmp to -1, result to immutable gr0
	beq	icc0,#0,it_was_unset

I suspect a lot of RISC archs will be like this. It's only certain ones like
M68K and x86 where you can test memory directly:

	testl	(%eax)
	je	it_was_unset

that it's usefully optimisable, and in those cases it might be possible to do
this sort of thing:

	cmpl	$-1,(%eax)
	je	it_was_unset

though the instruction will be longer.

> In short: NO_IRQ _is_ 0. Always has been.

So what? That hasn't stopped you imposing a blanket change before.

> It's the only sane value.

Has anyone ever accused you of being sane? :-)

> Anybody who does anything else is a bug waiting to happen.

My three main concerns are this:

 (1) Changing the no-irq value away from zero is going to cause problems in
     certain drivers that assume they can do !dev->irq. I'd like my drivers to
     work without me having to do anything to them, but there's a lot of
     rubbish drivers out there, even allowing for this. I suspect this is a
     tiny part of the problem, and easily fixed in the drivers in the kernel.

 (2) 0 is a valid IRQ in lots of places, including x86. IIRC it is permissible
     for ISAPNP and PNP-BIOS (and presumably ACPI) to indicate something is
     attached to IRQ 0 (usually only the timer is there though, but it can be
     possible to reconfigure that).
     
     Fortunately, for the FRV arch IRQ 0 is not used - level 0 in the interrupt
     mask register permits all interrupts; and for the AM33 arch, whilst there
     is an IRQ 0, that's the NMI interrupt and so has to be handled specially
     anyway.

     The only reason NO_IRQ on FRV is -1 is that I copied the code from
     elsewhere. It could easily be changed to 0.

 (3) Having to translate a cookie for a specific IRQ means that the IRQ
     handling code will be slower and more complex, or is going to avoid the
     issue and be naughty and not deal with irq == NO_IRQ properly:

     The x86 PIC reports it as IRQ 0 having happened. In which case, by your
     argument, you _have_ to translate it: you're not allowed to pass NO_IRQ to
     setup_irq(), and you're not allowed to pass it to the interrupt handler -
     in this case timer_interrupt(). Doing otherwise is wrong, insane, etc...

     It may even be possible to simplify the x86/x86_64 arch code by making IRQ
     0 a normal IRQ instead of something special.

     The only argument for not doing so is that it's hidden inside the arch
     where it can't be seen... apart from by those looking for examples of good
     code to copy (the i386 arch is used as a model for a lot of things).


I'd like to see dev->irq as a pointer to a structure. As you say, the number is
a cookie, but it's also very much dependent on the bus, so why shouldn't it be
associated with the bus in the same way I/O ports and memory ranges are? I'd
also like to see it arranged as a tree: with FRV, I can add extra PIC's into
the tree, thus expanding the IRQ space available dynamically.

However, as far as the current issue goes, I've no concerns for FRV or AM33
should NO_IRQ become 0.

Whatever you decide to do *please* document this in Documentation/ somewhere!
Then you have a "standard" at which to point and say "so it is written". At the
moment it's documented in the code, and that is inconsistent, perhaps
reasonably.

David

Re: [PATCH 4/5] Centralise NO_IRQ definition

[David Woodhouse]

On Tue, 2005-11-22 at 18:37 +0000, David Howells wrote:
> 
>  (3) Having to translate a cookie for a specific IRQ means that the IRQ
>      handling code will be slower and more complex, or is going to avoid the
>      issue and be naughty and not deal with irq == NO_IRQ properly:
> 
>      The x86 PIC reports it as IRQ 0 having happened. In which case, by your
>      argument, you _have_ to translate it: you're not allowed to pass NO_IRQ to
>      setup_irq(), and you're not allowed to pass it to the interrupt handler -
>      in this case timer_interrupt(). Doing otherwise is wrong, insane, etc...

This is true. If we're suddenly going to start pretending that IRQ 0
isn't a valid interrupt merely on the basis that "x86 doesn't use it"¹,
then we can't really go making an exception to allow us to use IRQ 0 on
i386.

-- 
dwmw2
¹ ...despite the fact that even that isn't true on legacy-free machines.

Re: [PATCH 4/5] Centralise NO_IRQ definition

[Linus Torvalds]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 655 bytes --]



On Tue, 22 Nov 2005, David Woodhouse wrote:
> 
> This is true. If we're suddenly going to start pretending that IRQ 0
> isn't a valid interrupt merely on the basis that "x86 doesn't use it"¹,
> then we can't really go making an exception to allow us to use IRQ 0 on
> i386.

Of _course_ "irq0" is a valid irq. On PC's, it's usually the timer 
interrupt.

It's the "dev->irq" _cookie_ zero that means it is does not have an irq.

If you have a physical "irq 0" that is bound to a device, it needs a 
cookie, and that cookie can't be 0, because that means the device has no 
interrupt.

How hard is that to understand? Why do people mix these up?

		Linus

Re: [PATCH 4/5] Centralise NO_IRQ definition

[David Woodhouse]

On Tue, 2005-11-22 at 11:21 -0800, Linus Torvalds wrote:
> Of _course_ "irq0" is a valid irq. On PC's, it's usually the timer 
> interrupt.
> 
> It's the "dev->irq" _cookie_ zero that means it is does not have an irq.
> 
> If you have a physical "irq 0" that is bound to a device, it needs a 
> cookie, and that cookie can't be 0, because that means the device has no 
> interrupt.
> 
> How hard is that to understand? Why do people mix these up?

Mostly I'd suggest that people mix them up because you've only just made
up this 'cookie' nonsense, and the number we store in dev->irq has
always been just the irq number -- at least on all but SPARC, which was
always 'special'. That's precisely _why_ we have a mix of 'int' and
'long' and other types in which we store it, as you've said. It's never
been considered a 'cookie'.

But turning it into a cookie makes enough sense, I suppose, as long as
we stop confusing it with irq 'numbers' -- and that includes banning
anyone from printing it with '%d'.

I'm inclined to agree with David's suggestion of making it a pointer.
Then it can point to a structure which is part a proper _tree_ of
interrupts which actually resembles reality. And SPARC can stop being a
special (and probably saner) case. And all the mess which platforms have
with assigning interrupt 'number' ranges for various PICs can go away.
And hotplug interrupt controllers (it happens, on CardBus or with
hotplug PCI) can work sanely. But Matthew still can't have his pony.

Yes, you still need a mapping at _setup_ time from stuff like isapnp and
OpenFirmware interrupt numbers to the appropriate 'cookie', but that's
not so hard.

-- 
dwmw2

Re: [PATCH 4/5] Centralise NO_IRQ definition

[Linus Torvalds]

On Tue, 22 Nov 2005, David Howells wrote:
> 
> > In short: NO_IRQ _is_ 0. Always has been.
> 
> So what? That hasn't stopped you imposing a blanket change before.
> 
> > It's the only sane value.
> 
> Has anyone ever accused you of being sane? :-)

Good arguments, but usually when I'm insane I do blanket changes that I at 
least personally agree with.

So my insanity is a "self-consistent" one, although part of it is that I 
do tend to change my mind (ie I may be self-consistent at any particular 
point in time, but I reserve the right to be inconsistent over the span of 
a day, week, or a year).

> > Anybody who does anything else is a bug waiting to happen.
> 
> My three main concerns are this:
> 
>  (1) Changing the no-irq value away from zero is going to cause problems in
>      certain drivers that assume they can do !dev->irq. I'd like my drivers to
>      work without me having to do anything to them, but there's a lot of
>      rubbish drivers out there, even allowing for this. I suspect this is a
>      tiny part of the problem, and easily fixed in the drivers in the kernel.

Drivers really are the bulk of the kernel. They are a huge problem spot in 
general, because quite often driver writers are pretty limited in their 
understanding of the big picture and the rest of the kernel, which 
together with the fact that drivers are often hard to write _anyway_ (bad 
hw docs if you have any at all, coupled with often the hardware itself 
having strange "features") means that drivers often have the lowest 
quality of code.

To make matters worse, most drivers will have very limited testing anyway, 
because they have a limited audience. There's a few _very_ common drivers, 
but there's a lot of drivers that are relevant to only a small percentage 
of kernel users.

Finally, in this particular case, the notion of NO_IRQ is usually an issue 
only for a small percentage of that small percentage. I bet there are tons 
of drivers that don't even bother to check, because they simply don't need 
to: they always have an interrupt available, or the machine would be so 
broken that it wouldn't ever get used anyway.

This is part of the reason why I'm so adamant that _only_ PC's matter in 
this space. For all other architectures, the small percentage of a small 
percentage of a small percentage basically means that the driver is 
totally irrelevant and has never had any users and absolutely zero 
testing.

So when it comes to drivers, other architectures simply _have_ to look 
like a PC, because if they don't, they'll be sorry. They'll _never_ get 
the kind of testing overhead that PC's get. And even on bog-standard 
Linux/x86, we have several times had an issue of a driver being broken for 
over a _year_ or more, without anybody noticing, and only then being 
fixed (because the users were so few that it took a long time before they 
upgraded).

And this is why I absolutely _hate_ to make changes that you can only test 
by actually having the hardware. Sometimes we have to do it (the big 
interrupt handling changes for SMP), and for all I know we _still_ have 
drivers that simply don't even compile on SMP because they depend on the 
old global "cli/sti" behaviour.

And in this case, I'm 100% convinced we do not want to change that old PC 
behaviour. The fact is, -1 really _is_ technically worse than 0. As 
already mentioned "unsigned int" is actually the canonical form of NO_IRQ, 
so -1 really ends up being "~0u", and that coupled with the fact that some 
people use "int", others use "unsigned long" is just clearly not good.

So my suggested (very _strongly_ suggested) solution is for people to just 
consider "irq" to be a cookie, with the magical value 0 meaning "not 
there" but no inherent meaning otherwise. That just solves all the 
fundamentally hard problems, and has no fundamental problems of its own.

The thing is, we'll have to do that _anyway_ over time.  We _know_ that 
even PCI interrupts are changing. We're clearly moving towards a world 
where an interrupt is _literally_ a cookie from the device, and this very 
much includes PCI. What do you think MSI (message signalled interrupts) 
are all about?

So thinking about "dev->irq" as a cookie is literally the right thing to 
do. It is _not_ an index, and it certainly has nothing to do with things 
like the pci interrupt line register or what the actual hardware 
connection to the CPU is. It's a cookie. And for historical reasons we 
have value 0 being the "no cookie" value.

There are actually some good reasons to think of interrupts as kernel 
_pointers_ (and they'd point to the "irq descriptor"), but quite frankly, 
right now that's just too big of a change. But if we ever do that change, 
the value 0 would _still_ be the "no cookie" value, and you'd _still_ just 
use "if (!dev->irq)" to test whether you had an irq or not.

(If somebody wants to try to make "irq" into a pointer, I'd actually be a 
lot more supportive of it than of this "NO_IRQ" thing. I suspect it's a 
bigger patch than we really want, for not a lot of gain, but at least it 
would result in compile warnings for drivers, which is a way to control 
the damage. That's what we did for the SMP irq disable/enable changes too)

> I'd like to see dev->irq as a pointer to a structure.

Yes, indeed. We already even probably have the right structure (ie likely 
the right thing to do is to just use the current "irq_desc_t *").

If somebody wants to do that, I'll happily accept it. I just suspect it's 
a _lot_ of work.

		Linus

Re: [PATCH 4/5] Centralise NO_IRQ definition

[David Howells]

Linus Torvalds <torvalds@osdl.org> wrote:

> bad hw docs if you have any at all

The same could be said of Linux. The docs there could be a *lot* better.

I wonder... might it be worth creating a Wiki or something on kernel.org in
which kernel docs can be maintained?

David

Re: [PATCH 4/5] Centralise NO_IRQ definition

[Linus Torvalds]

On Tue, 22 Nov 2005, David Howells wrote:
> 
> The same could be said of Linux. The docs there could be a *lot* better.
> 
> I wonder... might it be worth creating a Wiki or something on kernel.org in
> which kernel docs can be maintained?

I'm not sure a wiki will save us, but I don't think we'd have anything to 
lose from trying. 

Whether kernel.org wants to have a wiki and the related infrastructure is 
another matter. Right now I think everything is set up so that the public 
machines are pure mirrors of the master machine, and the master machine 
doesn't allow any public interaction at all.

So the wiki would have to be a totally separate setup (probably a 
dedicated public wiki machine at "wiki.kernel.org" or something). 

IOW, I suspect it would be best set up the same way "vger.kernel.org" is, 
rather than necessarily any of the current core kernel.org machines. So 
maybe somebody can just try to set it up, and ask for the DNS magic to be 
done?

		Linus

Re: [PATCH 4/5] Centralise NO_IRQ definition

[Pavel Machek]

Hi!

> > and then people can just say
> > 
> > 	if (!dev->irq.valid)
> > 		return;
> > 
> > instead, which is also readable, and where you simply cannot do the old 
> > "if (!dev->irq)" at all.
> > 
> > The fact is, 0 _is_ special. Not just for hardware, but because 0 has 
> > a magical meaning as "false" in the C language.
> 
> yeah, i wanted to suggest this originally, but got distracted by the x86 
> quirk that 'IRQ#0' is often the i8253 timer interrupt.
> 
> is there any architecture where irq 0 is a legitimate setting that could 
> occur in drivers, and which would make NO_IRQ define of 0 non-practical?  
> If not (which i think is the case) then we should indeed standardize on 
> 0. (in one way or another) It's not like any real driver will ever have 
> IRQ#0 even on a PC: the timer IRQ is 'known' to be routed to 0, and we 

Well, I still may want for example disk driver (with broken interrupt)
to hook onto irq#0 (timer). Better than no interrupts at all....

								Pavel
-- 
Thanks, Sharp!

Re: [PATCH 4/5] Centralise NO_IRQ definition

[Benjamin Herrenschmidt]

> The fact is, 0 _is_ special. Not just for hardware, but because 0 has a 
> magical meaning as "false" in the C language.

I don't agree, irq 0 has been a valid irq on a number of platforms for
ages (including your own G5, at least some of them have the SATA irq at
0 :) and this didn't cause any problem for most drivers. The few ones
that have done broken assumption have been easily fixed using NO_IRQ.

"Translating" it means some ugly translation work all over the place,
which means overhead in the interrupt path (ok, not that much but
still), plus finding some magic number to put 0 on, which makes things
much more complicated for archs that have interrupts sorted in nice
blocks of power of two, etc... a significant burden on arch/PIC code for
no good reason imho.

I hate arbitrary "translations" when they aren't strictly necessary.
It's common to have a constant for a "not valid" number in spaces where
"0" is a valid value, I don't think that "looking simpler" to dump
driver writers is worth it in this case.

Ben.

Re: [PATCH 4/5] Centralise NO_IRQ definition

[Linus Torvalds]

On Tue, 22 Nov 2005, Benjamin Herrenschmidt wrote:
> 
> > The fact is, 0 _is_ special. Not just for hardware, but because 0 has a 
> > magical meaning as "false" in the C language.
> 
> I don't agree, irq 0 has been a valid irq on a number of platforms for
> ages

The point is, it's _not_ a valid irq for 99.9% of all machines and drivers 
that have ever been tested.

Also, if you don't agree that 0 is special in the C language, then you're 
just strictly _wrong_. It's an undeniable fact that zero _is_ special.

		Linus

Re: [PATCH 4/5] Centralise NO_IRQ definition

[Benjamin Herrenschmidt]

On Mon, 2005-11-21 at 13:38 -0800, Linus Torvalds wrote:

> The point is, it's _not_ a valid irq for 99.9% of all machines and drivers 
> that have ever been tested.

It is a valid irq for a wide range of embedded devices.

> Also, if you don't agree that 0 is special in the C language, then you're 
> just strictly _wrong_. It's an undeniable fact that zero _is_ special.

And ? I really don't agree that just because 0 "looks kewl", we should
enforce that and add some dodgy remapping all over the place.

It's not like it was difficult to fix the few drivers that make bogus
assumptions and use NO_IRQ instead ...

Ben.

Re: [PATCH 4/5] Centralise NO_IRQ definition

[Linus Torvalds]

On Tue, 22 Nov 2005, Benjamin Herrenschmidt wrote:
> 
> And ? I really don't agree that just because 0 "looks kewl", we should
> enforce that and add some dodgy remapping all over the place.

That's not at all what I'm saying.

THE "BAD IRQ" MAPPING IS REQUIRED REGARDLESS.

If we make PCI_NO_IRQ be -1, then PC's need to remap 0 to that value. In 
pretty much _exactly_ the same places that I suggest that the ppc code 
should do it.

And there are several thousand times more PC's than there are other 
things.

Got it?

Everybody who argues for PCI_NO_IRQ being -1 is arguing for all the same 
things I argue that the ppc port should do, except they _also_ argue that 
we should break now-working setups.

Is that so hard to understand? -1 is no different from 0, except it is in 
many way sprovably _inferior_. Both need some mapping. But the -1 case 
needs more of it, _and_ will result in a inferior end result (because the 
nice "!dev->pci" thing suddenly doesn't work).

See? You're arguing for a technically inferior solution. 

		Linus

Re: [PATCH 4/5] Centralise NO_IRQ definition

[Benjamin Herrenschmidt]

> If we make PCI_NO_IRQ be -1, then PC's need to remap 0 to that value. In 
> pretty much _exactly_ the same places that I suggest that the ppc code 
> should do it.

No. Remapping a valid number to something else means remapping at
runtime for all things that manipulate an irq : issuing it,
enable/disable callbacks, etc...

"Fixing" the definition of "no irq" on x86 means only changing the boot
code that sets up irq numbers, whatever it is.

Thus what solution is technically superior can be argued based on the
fact that your solution introduce overhead to code path that are hot
during normal kernel operations.

Ben.