First steps towards making NO_IRQ a generic concept

First steps towards making NO_IRQ a generic concept

[Matthew Wilcox]

Hi Linus,

This series of four patches are the first step towards making NO_IRQ a
pervasive concept.  It's bundled up in a git tree for your convenience
(unless I bodged it up, in which case it's your inconvenience).

git://git.kernel.org/pub/scm/linux/kernel/git/willy/misc-2.6.git no_irq

 - Check the irq number is within bounds in the functions which weren't
   already checking.
 - Introduce PCI_NO_IRQ and pci_valid_irq()
   Explicitly initialise pci_dev->irq with PCI_NO_IRQ, allowing us to change
   the value of PCI_NO_IRQ when all drivers have been audited.
 - Use pci_valid_irq() instead of a custom NO_IRQ definition.
   It probably didn't work on half a dozen architectures.
 - Move the definition of NO_IRQ from asm directories to <linux/hardirq.h>.
   Individual architectures can still override it if they want to, but all
   existing definitions were -1.

 drivers/pci/probe.c       |    7 +++++--
 drivers/pcmcia/pd6729.c   |    6 +-----
 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   |   10 ++++++++++
 include/linux/pci.h       |    9 +++++++++
 kernel/irq/manage.c       |   15 +++++++++++++++
 10 files changed, 40 insertions(+), 31 deletions(-)

I'll follow this mail with the patches for other peoples benefits.  They
were previously posted to linux-arch with no responses.

Re: First steps towards making NO_IRQ a generic concept

[Matthew Wilcox]

[PATCH] Check the irq number is within bounds in the functions which weren't
already checking.

Signed-off-by: Matthew Wilcox <matthew@wil.cx>

---

 kernel/irq/manage.c |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

applies-to: d49992521230a4e302c6d4bef9191e885220b82e
2bcbc18c7e2148e9a1588e607a63f51e53d53810
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 1cfdb08..4190d9e 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -35,6 +35,9 @@ void synchronize_irq(unsigned int irq)
 {
 	struct irq_desc *desc = irq_desc + irq;
 
+	if (irq >= NR_IRQS)
+		return;
+
 	while (desc->status & IRQ_INPROGRESS)
 		cpu_relax();
 }
@@ -59,6 +62,9 @@ void disable_irq_nosync(unsigned int irq
 	irq_desc_t *desc = irq_desc + irq;
 	unsigned long flags;
 
+	if (irq >= NR_IRQS)
+		return;
+
 	spin_lock_irqsave(&desc->lock, flags);
 	if (!desc->depth++) {
 		desc->status |= IRQ_DISABLED;
@@ -85,6 +91,9 @@ void disable_irq(unsigned int irq)
 {
 	irq_desc_t *desc = irq_desc + irq;
 
+	if (irq >= NR_IRQS)
+		return;
+
 	disable_irq_nosync(irq);
 	if (desc->action)
 		synchronize_irq(irq);
@@ -107,6 +116,9 @@ void enable_irq(unsigned int irq)
 	irq_desc_t *desc = irq_desc + irq;
 	unsigned long flags;
 
+	if (irq >= NR_IRQS)
+		return;
+
 	spin_lock_irqsave(&desc->lock, flags);
 	switch (desc->depth) {
 	case 0:
@@ -162,6 +174,9 @@ int setup_irq(unsigned int irq, struct i
 	unsigned long flags;
 	int shared = 0;
 
+	if (irq >= NR_IRQS)
+		return -EINVAL;
+
 	if (desc->handler == &no_irq_type)
 		return -ENOSYS;
 	/*
---
0.99.8.GIT

Re: First steps towards making NO_IRQ a generic concept

[Ingo Molnar]

* Matthew Wilcox <matthew@wil.cx> wrote:

> [PATCH] Check the irq number is within bounds in the functions which 
> weren't already checking.

> +	if (irq >= NR_IRQS)
> +		return;

hm, why not start with the -1 value for PCI_NO_IRQ, instead of 0:

> +#define PCI_NO_IRQ             0

and be done with it.

also:

> - Move the definition of NO_IRQ from asm directories to 
>   <linux/hardirq.h>. Individual architectures can still override it if 
>   they want to, but all existing definitions were -1.

we shouldnt make it overridable just for the sake of it. If all arches 
were fine with -1, it should be the generic value and there's no 
override.

plus, shouldnt this go into -mm first, since it clearly affects some 
drivers? Why into Linus' tree immediately?

the patch series looks good to me otherwise.

	Ingo

Re: First steps towards making NO_IRQ a generic concept

[Matthew Wilcox]

On Thu, Nov 03, 2005 at 04:44:39PM +0100, Ingo Molnar wrote:
> > +	if (irq >= NR_IRQS)
> > +		return;
> 
> hm, why not start with the -1 value for PCI_NO_IRQ, instead of 0:
> 
> > +#define PCI_NO_IRQ             0
> 
> and be done with it.

There's a number of drivers which check "if (!irq) ...".  For example:

drivers/net/3c523.c:            if ((irq && irq != dev->irq) || 
drivers/net/atarilance.c:               if (!irq) {
drivers/net/cs89x0.c:           if (!dev->irq)
drivers/net/depca.c:                    if (!dev->irq) {
drivers/net/eexpress.c: if (!dev->irq || !irqrmap[dev->irq])
drivers/net/ewrk3.c:                            if (!dev->irq) {
drivers/net/ibmlana.c:          return (base != 0 || irq != 0) ? -ENXIO
: -ENODE
drivers/net/lasi_82596.c:       if (!dev->irq) {
drivers/net/ne-h8300.c: if (! dev->irq) {
drivers/net/ne.c:       if (! dev->irq) {
drivers/net/ni52.c:             if(!dev->irq)
drivers/net/ni65.c:                     if(!dev->irq)
drivers/net/pcnet32.c:  if (!dev->irq) {

... and that's just drivers/net, and that doesn't include other ways
for checking if irq is not 0, and doesn't include irqs referred to
under different names not including the string 'irq'.  Against that,
I know not all of these are PCI drivers.  So we need to spend some time
checking drivers for this assumption.

We also need to figure out what to do with non-PCI drivers.  Some of
them need more work than others to work with a -1 NO_IRQ.  There's also
plenty of janitorial work with people misusing the probe_irq_off()
interface.

> > - Move the definition of NO_IRQ from asm directories to 
> >   <linux/hardirq.h>. Individual architectures can still override it if 
> >   they want to, but all existing definitions were -1.
> 
> we shouldnt make it overridable just for the sake of it. If all arches 
> were fine with -1, it should be the generic value and there's no 
> override.

Fine with me.  I can take out the ifndef.

> plus, shouldnt this go into -mm first, since it clearly affects some 
> drivers? Why into Linus' tree immediately?

With the way I'm staging it, it shouldn't affect drivers.  The only
exception was the pcmcia driver that defined its own NO_IRQ macro.  So I
converted that one to the new preferred way to check the irq is unset.

Re: First steps towards making NO_IRQ a generic concept

[Ingo Molnar]

* Matthew Wilcox <matthew@wil.cx> wrote:

> On Thu, Nov 03, 2005 at 04:44:39PM +0100, Ingo Molnar wrote:
> > > +	if (irq >= NR_IRQS)
> > > +		return;
> > 
> > hm, why not start with the -1 value for PCI_NO_IRQ, instead of 0:
> > 
> > > +#define PCI_NO_IRQ             0
> > 
> > and be done with it.
> 
> There's a number of drivers which check "if (!irq) ...".  For example:
> 
> drivers/net/3c523.c:            if ((irq && irq != dev->irq) || 
> drivers/net/atarilance.c:               if (!irq) {
> drivers/net/cs89x0.c:           if (!dev->irq)
> drivers/net/depca.c:                    if (!dev->irq) {
> drivers/net/eexpress.c: if (!dev->irq || !irqrmap[dev->irq])
> drivers/net/ewrk3.c:                            if (!dev->irq) {
> drivers/net/ibmlana.c:          return (base != 0 || irq != 0) ? -ENXIO
> : -ENODE
> drivers/net/lasi_82596.c:       if (!dev->irq) {
> drivers/net/ne-h8300.c: if (! dev->irq) {
> drivers/net/ne.c:       if (! dev->irq) {
> drivers/net/ni52.c:             if(!dev->irq)
> drivers/net/ni65.c:                     if(!dev->irq)
> drivers/net/pcnet32.c:  if (!dev->irq) {
> 
> ... and that's just drivers/net, and that doesn't include other ways
> for checking if irq is not 0, and doesn't include irqs referred to
> under different names not including the string 'irq'.  Against that,
> I know not all of these are PCI drivers.  So we need to spend some time
> checking drivers for this assumption.
> 
> We also need to figure out what to do with non-PCI drivers.  Some of 
> them need more work than others to work with a -1 NO_IRQ.  There's 
> also plenty of janitorial work with people misusing the 
> probe_irq_off() interface.

ok, understood. I'm wondering, why is there any need to do a PCI_NO_IRQ?  
Why not just a generic NO_IRQ. It's not like we can or want to make them 
different in the future. The interrupt vector number is a generic thing 
that attaches to the platform via request_irq() - there is nothing 'PCI' 
about it. So the PCI layer shouldnt pretend it has its own IRQ 
abstraction - the two are forcibly joined. The same goes for 
pci_valid_irq() - we should only have valid_irq(). Am i missing 
anything?

> > plus, shouldnt this go into -mm first, since it clearly affects some 
> > drivers? Why into Linus' tree immediately?
> 
> With the way I'm staging it, it shouldn't affect drivers.  The only 
> exception was the pcmcia driver that defined its own NO_IRQ macro.  So 
> I converted that one to the new preferred way to check the irq is 
> unset.

ok. Your transition path looks safe to me.

	Ingo

Re: First steps towards making NO_IRQ a generic concept

[Matthew Wilcox]

On Thu, Nov 03, 2005 at 05:20:59PM +0100, Ingo Molnar wrote:
> ok, understood. I'm wondering, why is there any need to do a PCI_NO_IRQ?  
> Why not just a generic NO_IRQ. It's not like we can or want to make them 
> different in the future. The interrupt vector number is a generic thing 
> that attaches to the platform via request_irq() - there is nothing 'PCI' 
> about it. So the PCI layer shouldnt pretend it has its own IRQ 
> abstraction - the two are forcibly joined. The same goes for 
> pci_valid_irq() - we should only have valid_irq(). Am i missing 
> anything?

The last patch in this vein will delete PCI_NO_IRQ, replacing it with
NO_IRQ.  To make that final patch small, I wanted to introduce an
abstraction that PCI drivers could use.  Possibly it's not well thought
out.  Do you think we should put in the explicit compares against
PCI_NO_IRQ as we find drivers that care and then do a big sweep when we
think we've found them all?

Re: First steps towards making NO_IRQ a generic concept

[Ingo Molnar]

* Matthew Wilcox <matthew@wil.cx> wrote:

> On Thu, Nov 03, 2005 at 05:20:59PM +0100, Ingo Molnar wrote:
> > ok, understood. I'm wondering, why is there any need to do a PCI_NO_IRQ?  
> > Why not just a generic NO_IRQ. It's not like we can or want to make them 
> > different in the future. The interrupt vector number is a generic thing 
> > that attaches to the platform via request_irq() - there is nothing 'PCI' 
> > about it. So the PCI layer shouldnt pretend it has its own IRQ 
> > abstraction - the two are forcibly joined. The same goes for 
> > pci_valid_irq() - we should only have valid_irq(). Am i missing 
> > anything?
> 
> The last patch in this vein will delete PCI_NO_IRQ, replacing it with 
> NO_IRQ.  To make that final patch small, I wanted to introduce an 
> abstraction that PCI drivers could use.  Possibly it's not well 
> thought out.  Do you think we should put in the explicit compares 
> against PCI_NO_IRQ as we find drivers that care and then do a big 
> sweep when we think we've found them all?

i missed the detail that we want to have PCI_NO_IRQ at 0, while keeping 
NO_IRQ at -1 - so the namespaces have to be separate, temporarily. So 
your approach is fine.

	Ingo

Re: First steps towards making NO_IRQ a generic concept

[Matthew Wilcox]

Introduce PCI_NO_IRQ and pci_valid_irq()
Explicitly initialise pci_dev->irq with PCI_NO_IRQ, allowing us to change
the value of PCI_NO_IRQ when all drivers have been audited.

Signed-off-by: Matthew Wilcox <matthew@wil.cx>

---

 drivers/pci/probe.c |    7 +++++--
 include/linux/pci.h |    9 +++++++++
 2 files changed, 14 insertions(+), 2 deletions(-)

applies-to: 734b9ec7d20002b1acdc9f4f7142efa2e5340e0d
bc63ebe08e89a7be876a0538e2c563e2cb1d9617
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index fce2cb2..35ba70b 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -571,9 +571,12 @@ static void pci_read_irq(struct pci_dev 
 	unsigned char irq;
 
 	pci_read_config_byte(dev, PCI_INTERRUPT_PIN, &irq);
-	if (irq)
+	if (irq) {
 		pci_read_config_byte(dev, PCI_INTERRUPT_LINE, &irq);
-	dev->irq = irq;
+		dev->irq = irq;
+	} else {
+		dev->irq = PCI_NO_IRQ;
+	}
 }
 
 /**
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 3596ac9..4e009e2 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -140,6 +140,15 @@ struct pci_dev {
 	struct bin_attribute *res_attr[DEVICE_COUNT_RESOURCE]; /* sysfs file for resources */
 };
 
+/*
+ * The PCI subsystem has traditionally filled in 0 when no interrupt has been
+ * assigned.  While we should move to using NO_IRQ instead, many drivers
+ * remain to be converted.  Once all drivers are using PCI_NO_IRQ, switching
+ * over should be a simple search-and-replace.
+ */
+#define PCI_NO_IRQ		0
+#define pci_valid_irq(irq)	(irq != PCI_NO_IRQ)
+
 #define pci_dev_g(n) list_entry(n, struct pci_dev, global_list)
 #define pci_dev_b(n) list_entry(n, struct pci_dev, bus_list)
 #define	to_pci_dev(n) container_of(n, struct pci_dev, dev)
---
0.99.8.GIT

Re: First steps towards making NO_IRQ a generic concept

[Arjan van de Ven]

On Thu, 2005-11-03 at 07:51 -0700, Matthew Wilcox wrote:
> Introduce PCI_NO_IRQ and pci_valid_irq()
> Explicitly initialise pci_dev->irq with PCI_NO_IRQ, allowing us to change
> the value of PCI_NO_IRQ when all drivers have been audited.


can pci_valid_irq() accept a device instead of a number?
that allows a lot more checks later on (like "is it enabled") and other
such checks. 

Re: First steps towards making NO_IRQ a generic concept

[Matthew Wilcox]

Use pci_valid_irq() instead of a custom NO_IRQ definition.
It probably didn't work on half a dozen architectures.

Signed-off-by: Matthew Wilcox <matthew@wil.cx>

---

 drivers/pcmcia/pd6729.c |    6 +-----
 1 files changed, 1 insertions(+), 5 deletions(-)

applies-to: 1081da18698662c172c682d7e8b882e38ee57eee
220fd6c4becfe49c606b7bcbb7df6262691459b4
diff --git a/drivers/pcmcia/pd6729.c b/drivers/pcmcia/pd6729.c
index 20642f0..72720f2 100644
--- a/drivers/pcmcia/pd6729.c
+++ b/drivers/pcmcia/pd6729.c
@@ -39,10 +39,6 @@ MODULE_AUTHOR("Jun Komuro <komurojun-mbn
  */
 #define to_cycles(ns)	((ns)/120)
 
-#ifndef NO_IRQ
-#define NO_IRQ	((unsigned int)(0))
-#endif
-
 /*
  * PARAMETERS
  *  irq_mode=n
@@ -733,7 +729,7 @@ static int __devinit pd6729_pci_probe(st
 		goto err_out_disable;
 	}
 
-	if (dev->irq == NO_IRQ)
+	if (!pci_valid_irq(dev->irq))
 		irq_mode = 0;	/* fall back to ISA interrupt mode */
 
 	mask = pd6729_isa_scan();
---
0.99.8.GIT

Re: First steps towards making NO_IRQ a generic concept

[Matthew Wilcox]

Move the definition of NO_IRQ from asm directories to <linux/hardirq.h>.
Individual architectures can still override it if they want to, but all
existing definitions were -1.

Signed-off-by: Matthew Wilcox <matthew@wil.cx>

---

 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   |   10 ++++++++++
 6 files changed, 10 insertions(+), 24 deletions(-)

applies-to: 378fab44adde8e150b890a6a45ca755d5910bbe7
8b323aaed3682ec4890966757fb407a46356fb05
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 f876bdf..037ef9f 100644
--- a/include/asm-parisc/irq.h
+++ b/include/asm-parisc/irq.h
@@ -10,8 +10,6 @@
 #include <linux/config.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 c7c3f91..b5720cf 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 5912874..a8b3f8e 100644
--- a/include/linux/hardirq.h
+++ b/include/linux/hardirq.h
@@ -87,6 +87,16 @@ 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.
+ */
+#ifndef NO_IRQ
+#define NO_IRQ			((unsigned int)(-1))
+#endif
+
 #define nmi_enter()		irq_enter()
 #define nmi_exit()		sub_preempt_count(HARDIRQ_OFFSET)
 
---
0.99.8.GIT