| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20061004202938.GF352@slug>
Date: Wed, 4 Oct 2006 20:29:38 +0000
From: Frederik Deweerdt <deweerdt@...e.fr>
To: Jeff Garzik <jeff@...zik.org>
Cc: linux-kernel@...r.kernel.org, arjan@...radead.org, matthew@....cx,
alan@...rguk.ukuu.org.uk, akpm@...l.org, rdunlap@...otime.net,
gregkh@...e.de
Subject: Re: [RFC PATCH] add pci_{request,free}_irq take #3
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);
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists