Re: [RFC PATCH] add pci_{request,free}_irq take #3

[Frederik Deweerdt <deweerdt@free.fr>]

On Wed, Oct 04, 2006 at 03:50:04PM -0400, Jeff Garzik wrote:
> Frederik Deweerdt wrote:
> >--- 2.6.18-mm3.orig/include/linux/interrupt.h
> >+++ 2.6.18-mm3/include/linux/interrupt.h
> >@@ -75,6 +75,13 @@ struct irqaction {
> > 	struct proc_dir_entry *dir;
> > };
> > +#ifndef ARCH_VALIDATE_IRQ
> >+static inline int is_irq_valid(unsigned int irq)
> >+{
> >+	return irq ? 1 : 0;
> >+}
> >+#endif /* ARCH_VALIDATE_IRQ */
> 
> 
> I ACK everything except the above snippet.  I just don't think it's
> linux/interrupt.h material, sorry.
I see. Just to be sure that I got the matter right, does the issue boils
down to a choice between:

#
# 1: is_pci_irq_valid in include/linux/pci.h
#
diff --git a/arch/powerpc/kernel/pci_32.c b/arch/powerpc/kernel/pci_32.c
index 9b49f86..9d1bb1d 100644
--- a/arch/powerpc/kernel/pci_32.c
+++ b/arch/powerpc/kernel/pci_32.c
@@ -1445,7 +1445,7 @@ int pci_read_irq_line(struct pci_dev *pc
 		DBG(" -> no map ! Using irq line %d from PCI config\n", line);
 
 		virq = irq_create_mapping(NULL, line);
-		if (virq != NO_IRQ)
+		if (is_pci_irq_valid(virq))
 			set_irq_type(virq, IRQ_TYPE_LEVEL_LOW);
 	} else {
 		DBG(" -> got one, spec %d cells (0x%08x...) on %s\n",
@@ -1454,7 +1454,7 @@ int pci_read_irq_line(struct pci_dev *pc
 		virq = irq_create_of_mapping(oirq.controller, oirq.specifier,
 					     oirq.size);
 	}
-	if(virq == NO_IRQ) {
+	if(!is_pci_irq_valid(virq)) {
 		DBG(" -> failed to map !\n");
 		return -1;
 	}

and 


#
# 2: is_irq_valid in include/linux/interrupt.h
#
diff --git a/arch/powerpc/kernel/pci_32.c b/arch/powerpc/kernel/pci_32.c
index 9b49f86..68bd1fa 100644
--- a/arch/powerpc/kernel/pci_32.c
+++ b/arch/powerpc/kernel/pci_32.c
@@ -12,6 +12,7 @@ #include <linux/sched.h>
 #include <linux/errno.h>
 #include <linux/bootmem.h>
 #include <linux/irq.h>
+#include <linux/interrupt.h>
 
 #include <asm/processor.h>
 #include <asm/io.h>
@@ -1445,7 +1446,7 @@ int pci_read_irq_line(struct pci_dev *pc
 		DBG(" -> no map ! Using irq line %d from PCI config\n", line);
 
 		virq = irq_create_mapping(NULL, line);
-		if (virq != NO_IRQ)
+		if (is_irq_valid(virq))
 			set_irq_type(virq, IRQ_TYPE_LEVEL_LOW);
 	} else {
 		DBG(" -> got one, spec %d cells (0x%08x...) on %s\n",
@@ -1454,7 +1455,7 @@ int pci_read_irq_line(struct pci_dev *pc
 		virq = irq_create_of_mapping(oirq.controller, oirq.specifier,
 					     oirq.size);
 	}
-	if(virq == NO_IRQ) {
+	if(!is_irq_valid(virq)) {
 		DBG(" -> failed to map !\n");
 		return -1;
 	}

Which in turn boils down to:
- which naming does make more sense
- which include file should contain the code
 (this point is IMO a 1:1 mapping to the first one: is_pci_irq_valid()
 should go to pci.h and is_irq_valid() should go to interrupt.h)

I'm personally inclined for the second solution is_irq_valid() because
(a) there is some irq setting code outside the pci subsystem[1], (b)
the function name is easier to remember. But I'd happily concede that
these aren't that strong points. Also, only two arches seem to tweak the
irq behind the pci subsystem, so "a consensus of arch maintainers" may
be hard to get :).

Regards,
Frederik

[1] $ grep -r 'read.*PCI_INTERRUPT_LINE' *
arch/alpha/kernel/sys_dp264.c:  pci_read_config_byte(dev, PCI_INTERRUPT_LINE, &irq8);
arch/alpha/kernel/sys_eiger.c:  pci_read_config_byte(dev, PCI_INTERRUPT_LINE, &irq_orig);
arch/alpha/kernel/sys_marvel.c: pci_read_config_byte(dev, PCI_INTERRUPT_LINE, &intline);
arch/alpha/kernel/sys_marvel.c:         pci_read_config_byte(dev, PCI_INTERRUPT_LINE, &intline);
arch/alpha/kernel/sys_nautilus.c:       pci_read_config_byte(dev, PCI_INTERRUPT_LINE, &irq);
arch/alpha/kernel/sys_titan.c:  pci_read_config_byte(dev, PCI_INTERRUPT_LINE, &intline);
arch/arm/mach-footbridge/personal-pci.c:        pci_read_config_byte(dev, PCI_INTERRUPT_LINE, &line);
arch/frv/mb93090-mb00/pci-irq.c:                pci_read_config_byte(dev, PCI_INTERRUPT_LINE, &line);
arch/i386/pci/fixup.c:  pci_read_config_byte(dev, PCI_INTERRUPT_LINE, (u8 *)&dev->irq);
arch/powerpc/kernel/pci_32.c:           if (pci_read_config_byte(pci_dev, PCI_INTERRUPT_LINE, &line) ||
arch/powerpc/kernel/pci_64.c:           if (pci_read_config_byte(pci_dev, PCI_INTERRUPT_LINE, &line) ||
drivers/ide/pci/pdc202xx_old.c:         pci_read_config_byte(dev, PCI_INTERRUPT_LINE, &irq);
drivers/ide/pci/pdc202xx_old.c:         pci_read_config_byte(dev, (PCI_INTERRUPT_LINE)|0x80, &irq2);
drivers/macintosh/via-pmu.c:            pci_read_config_word(pd, PCI_INTERRUPT_LINE, &ps->intr);
drivers/macintosh/via-pmu68k.c:         pci_read_config_word(pd, PCI_INTERRUPT_LINE, &ps->intr);
drivers/pci/hotplug/cpqphp_core.c:      pci_read_config_byte(pdev, PCI_INTERRUPT_LINE, &ctrl->cfgspc_irq);
drivers/pci/probe.c:            pci_read_config_byte(dev, PCI_INTERRUPT_LINE, &irq);
drivers/pci/quirks.c:   pci_read_config_byte(dev, PCI_INTERRUPT_LINE, &irq);
drivers/ata/sata_via.c: pci_read_config_byte(pdev, PCI_INTERRUPT_LINE, &tmp8);