Hi all,
This is take #3 of the "add pci_{request,free}_irq" patch.
The following changes have been made since last proposal:
- add IRQF_SHARED systematically to the flags parameter. This is safe
even for MSI as seen here: http://lkml.org/lkml/2006/10/4/21
- remove IRQF_SHARED from tg3 and e1000
- s/ARCH_VALIDATE_PCI_IRQ/ARCH_VALIDATE_IRQ/ in
include/linux/interrupts.h, as the irq line may be tweaked outside
the pci subsystem in an arch specific way.
I'll send a follow-up patch showing the implied modifications for the
following drivers: aic7xxx, aic79xx, tg3 and e1000.
Please note that I'm not submitting the driver changes, they're there
only for illustration purposes. I'll CC the appropriate maintainers
when/if an API is agreed upon.
Regards,
Frederik
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index a544997..50b49ef 100644
drivers/pci/pci.c | 42 ++++++++++++++++++++++++++++++++++++++++++
include/linux/interrupt.h | 7 +++++++
include/linux/pci.h | 6 ++++++
3 files changed, 55 insertions(+)
Index: 2.6.18-mm3/drivers/pci/pci.c
===================================================================
--- 2.6.18-mm3.orig/drivers/pci/pci.c
+++ 2.6.18-mm3/drivers/pci/pci.c
@@ -15,6 +15,7 @@
#include <linux/pci.h>
#include <linux/module.h>
#include <linux/spinlock.h>
+#include <linux/interrupt.h>
#include <linux/string.h>
#include <asm/dma.h> /* isa_dma_bridge_buggy */
#include "pci.h"
@@ -810,6 +811,47 @@ err_out:
}
/**
+ * pci_request_irq - Reserve an IRQ for a PCI device
+ * @pdev: The PCI device whose irq is to be reserved
+ * @handler: The interrupt handler function,
+ * @flags: The flags to be passed to request_irq()
+ * @name: The name of the device to be associated with the irq
+ *
+ * Returns 0 on success, or a negative value on error. A warning
+ * message is also printed on failure.
+ * pci_get_drvdata(pdev) shall be passed as an argument to the @handler
+ * function
+ */
+int pci_request_irq(struct pci_dev *pdev,
+ irqreturn_t(*handler) (int, void *, struct pt_regs *),
+ unsigned long flags, const char *name)
+{
+ if (!is_irq_valid(pdev->irq)) {
+ dev_printk(KERN_ERR, &pdev->dev,
+ "No usable irq line was found (got #%d)\n",
+ pdev->irq);
+ return -EINVAL;
+ }
+
+ return request_irq(pdev->irq, handler, flags | IRQF_SHARED,
+ name ? name : pdev->driver->name,
+ pci_get_drvdata(pdev));
+}
+EXPORT_SYMBOL(pci_request_irq);
+
+/**
+ * pci_free_irq - Free an IRQ for a PCI device
+ *
+ * @pdev: the PCI device whose interrupt is to be freed
+ * pci_get_drvdata(pdev) is used as the device identifier
+ */
+void pci_free_irq(struct pci_dev *pdev)
+{
+ free_irq(pdev->irq, pci_get_drvdata(pdev));
+}
+EXPORT_SYMBOL(pci_free_irq);
+
+/**
* pci_set_master - enables bus-mastering for device dev
* @dev: the PCI device to enable
*
Index: 2.6.18-mm3/include/linux/interrupt.h
===================================================================
--- 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 */
+
extern irqreturn_t no_action(int cpl, void *dev_id, struct pt_regs *regs);
extern int request_irq(unsigned int,
irqreturn_t (*handler)(int, void *, struct pt_regs *),
Index: 2.6.18-mm3/include/linux/pci.h
===================================================================
--- 2.6.18-mm3.orig/include/linux/pci.h
+++ 2.6.18-mm3/include/linux/pci.h
@@ -52,6 +52,7 @@
#include <linux/compiler.h>
#include <linux/errno.h>
#include <linux/device.h>
+#include <linux/interrupt.h>
/* File state for mmap()s on /proc/bus/pci/X/Y */
enum pci_mmap_state {
@@ -532,6 +533,11 @@ void pci_release_regions(struct pci_dev
int __must_check pci_request_region(struct pci_dev *, int, const char *);
void pci_release_region(struct pci_dev *, int);
+int __must_check pci_request_irq(struct pci_dev *pdev,
+ irqreturn_t (*handler)(int, void *, struct pt_regs *),
+ unsigned long flags, const char *name);
+void pci_free_irq(struct pci_dev *pdev);
+
/* drivers/pci/bus.c */
int __must_check pci_bus_alloc_resource(struct pci_bus *bus,
struct resource *res, resource_size_t size,
Hi,
This proof-of-concept patch converts the aic7xxx driver to use the
pci_request_irq() function.
Please note that I'm not submitting the driver changes, they're there
only for illustration purposes. I'll CC the appropriate maintainers
when/if an API is agreed upon.
Regards,
Frederik
diff --git a/drivers/scsi/aic7xxx/aic7xxx_osm.h b/drivers/scsi/aic7xxx/aic7xxx_osm.h
index 86ea7ac..24f70bc 100644
drivers/scsi/aic7xxx/aic7xxx_osm.h | 1 -
drivers/scsi/aic7xxx/aic7xxx_osm_pci.c | 13 -------------
drivers/scsi/aic7xxx/aic7xxx_pci.c | 18 ++++++++++++------
3 files changed, 12 insertions(+), 20 deletions(-)
Index: 2.6.18-mm3/drivers/scsi/aic7xxx/aic7xxx_osm.h
===================================================================
--- 2.6.18-mm3.orig/drivers/scsi/aic7xxx/aic7xxx_osm.h
+++ 2.6.18-mm3/drivers/scsi/aic7xxx/aic7xxx_osm.h
@@ -561,7 +561,6 @@ static inline void ahc_linux_eisa_exit(v
int ahc_linux_pci_init(void);
void ahc_linux_pci_exit(void);
int ahc_pci_map_registers(struct ahc_softc *ahc);
-int ahc_pci_map_int(struct ahc_softc *ahc);
static __inline uint32_t ahc_pci_read_config(ahc_dev_softc_t pci,
int reg, int width);
Index: 2.6.18-mm3/drivers/scsi/aic7xxx/aic7xxx_osm_pci.c
===================================================================
--- 2.6.18-mm3.orig/drivers/scsi/aic7xxx/aic7xxx_osm_pci.c
+++ 2.6.18-mm3/drivers/scsi/aic7xxx/aic7xxx_osm_pci.c
@@ -368,16 +368,3 @@ ahc_pci_map_registers(struct ahc_softc *
return (error);
}
-int
-ahc_pci_map_int(struct ahc_softc *ahc)
-{
- int error;
-
- error = request_irq(ahc->dev_softc->irq, ahc_linux_isr,
- IRQF_SHARED, "aic7xxx", ahc);
- if (error == 0)
- ahc->platform_data->irq = ahc->dev_softc->irq;
-
- return (-error);
-}
-
Index: 2.6.18-mm3/drivers/scsi/aic7xxx/aic7xxx_pci.c
===================================================================
--- 2.6.18-mm3.orig/drivers/scsi/aic7xxx/aic7xxx_pci.c
+++ 2.6.18-mm3/drivers/scsi/aic7xxx/aic7xxx_pci.c
@@ -968,13 +968,19 @@ ahc_pci_config(struct ahc_softc *ahc, st
/*
* Allow interrupts now that we are completely setup.
- */
- error = ahc_pci_map_int(ahc);
- if (error != 0)
- return (error);
+ *
+ * Note: pci_request_irq return value is negated due to aic7xxx
+ * error handling style
+ */
+ error = -pci_request_irq(ahc->dev_softc, ahc_linux_isr,
+ 0, "aic7xxx");
+
+ if (!error) {
+ ahc->platform_data->irq = ahc->dev_softc->irq;
+ ahc->init_level++;
+ }
- ahc->init_level++;
- return (0);
+ return error;
}
/*
Hi,
This proof-of-concept patch converts the aic79xx driver to use the
pci_request_irq() function.
Please note that I'm not submitting the driver changes, they're there
only for illustration purposes. I'll CC the appropriate maintainers
when/if an API is agreed upon.
Regards,
Frederik
diff --git a/drivers/scsi/aic7xxx/aic79xx_osm.h b/drivers/scsi/aic7xxx/aic79xx_osm.h
index 448c39c..67897d4 100644
drivers/scsi/aic7xxx/aic79xx_osm.h | 1 -
drivers/scsi/aic7xxx/aic79xx_osm_pci.c | 13 -------------
drivers/scsi/aic7xxx/aic79xx_pci.c | 12 ++++++++++--
3 files changed, 10 insertions(+), 16 deletions(-)
Index: 2.6.18-mm3/drivers/scsi/aic7xxx/aic79xx_osm.h
===================================================================
--- 2.6.18-mm3.orig/drivers/scsi/aic7xxx/aic79xx_osm.h
+++ 2.6.18-mm3/drivers/scsi/aic7xxx/aic79xx_osm.h
@@ -594,7 +594,6 @@ void ahd_power_state_change(struct ahd_s
int ahd_linux_pci_init(void);
void ahd_linux_pci_exit(void);
int ahd_pci_map_registers(struct ahd_softc *ahd);
-int ahd_pci_map_int(struct ahd_softc *ahd);
static __inline uint32_t ahd_pci_read_config(ahd_dev_softc_t pci,
int reg, int width);
Index: 2.6.18-mm3/drivers/scsi/aic7xxx/aic79xx_osm_pci.c
===================================================================
--- 2.6.18-mm3.orig/drivers/scsi/aic7xxx/aic79xx_osm_pci.c
+++ 2.6.18-mm3/drivers/scsi/aic7xxx/aic79xx_osm_pci.c
@@ -336,19 +336,6 @@ ahd_pci_map_registers(struct ahd_softc *
return (error);
}
-int
-ahd_pci_map_int(struct ahd_softc *ahd)
-{
- int error;
-
- error = request_irq(ahd->dev_softc->irq, ahd_linux_isr,
- IRQF_SHARED, "aic79xx", ahd);
- if (!error)
- ahd->platform_data->irq = ahd->dev_softc->irq;
-
- return (-error);
-}
-
void
ahd_power_state_change(struct ahd_softc *ahd, ahd_power_state new_state)
{
Index: 2.6.18-mm3/drivers/scsi/aic7xxx/aic79xx_pci.c
===================================================================
--- 2.6.18-mm3.orig/drivers/scsi/aic7xxx/aic79xx_pci.c
+++ 2.6.18-mm3/drivers/scsi/aic7xxx/aic79xx_pci.c
@@ -376,10 +376,18 @@ ahd_pci_config(struct ahd_softc *ahd, st
/*
* Allow interrupts now that we are completely setup.
+ *
+ * Note: pci_request_irq return value is negated due to aic79xx
+ * error handling style
*/
- error = ahd_pci_map_int(ahd);
- if (!error)
+
+ error = -pci_request_irq(ahd->dev_softc, ahd_linux_isr,
+ 0, "aic79xx");
+ if (!error) {
+ ahd->platform_data->irq = ahd->dev_softc->irq;
ahd->init_level++;
+ }
+
return error;
}
Hi,
This proof-of-concept patch converts the tg3 driver to use the
pci_request_irq() function.
Please note that I'm not submitting the driver changes, they're there
only for illustration purposes. I'll CC the appropriate maintainers
when/if an API is agreed upon.
Regards,
Frederik
diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
index c25ba27..23660c6 100644
drivers/net/tg3.c | 22 +++++++++-------------
1 file changed, 9 insertions(+), 13 deletions(-)
Index: 2.6.18-mm3/drivers/net/tg3.c
===================================================================
--- 2.6.18-mm3.orig/drivers/net/tg3.c
+++ 2.6.18-mm3/drivers/net/tg3.c
@@ -6839,21 +6839,18 @@ restart_timer:
static int tg3_request_irq(struct tg3 *tp)
{
irqreturn_t (*fn)(int, void *, struct pt_regs *);
- unsigned long flags;
struct net_device *dev = tp->dev;
if (tp->tg3_flags2 & TG3_FLG2_USING_MSI) {
fn = tg3_msi;
if (tp->tg3_flags2 & TG3_FLG2_1SHOT_MSI)
fn = tg3_msi_1shot;
- flags = IRQF_SAMPLE_RANDOM;
} else {
fn = tg3_interrupt;
if (tp->tg3_flags & TG3_FLAG_TAGGED_STATUS)
fn = tg3_interrupt_tagged;
- flags = IRQF_SHARED | IRQF_SAMPLE_RANDOM;
}
- return (request_irq(tp->pdev->irq, fn, flags, dev->name, dev));
+ return pci_request_irq(tp->pdev, fn, IRQF_SAMPLE_RANDOM, dev->name);
}
static int tg3_test_interrupt(struct tg3 *tp)
@@ -6866,10 +6863,10 @@ static int tg3_test_interrupt(struct tg3
tg3_disable_ints(tp);
- free_irq(tp->pdev->irq, dev);
+ pci_free_irq(tp->pdev);
- err = request_irq(tp->pdev->irq, tg3_test_isr,
- IRQF_SHARED | IRQF_SAMPLE_RANDOM, dev->name, dev);
+ err = pci_request_irq(tp->pdev, tg3_test_isr,
+ IRQF_SAMPLE_RANDOM, dev->name);
if (err)
return err;
@@ -6897,7 +6894,7 @@ static int tg3_test_interrupt(struct tg3
tg3_disable_ints(tp);
- free_irq(tp->pdev->irq, dev);
+ pci_free_irq(tp->pdev);
err = tg3_request_irq(tp);
@@ -6915,7 +6912,6 @@ static int tg3_test_interrupt(struct tg3
*/
static int tg3_test_msi(struct tg3 *tp)
{
- struct net_device *dev = tp->dev;
int err;
u16 pci_cmd;
@@ -6946,7 +6942,7 @@ static int tg3_test_msi(struct tg3 *tp)
"the PCI maintainer and include system chipset information.\n",
tp->dev->name);
- free_irq(tp->pdev->irq, dev);
+ pci_free_irq(tp->pdev);
pci_disable_msi(tp->pdev);
tp->tg3_flags2 &= ~TG3_FLG2_USING_MSI;
@@ -6966,7 +6962,7 @@ static int tg3_test_msi(struct tg3 *tp)
tg3_full_unlock(tp);
if (err)
- free_irq(tp->pdev->irq, dev);
+ pci_free_irq(tp->pdev);
return err;
}
@@ -7051,7 +7047,7 @@ static int tg3_open(struct net_device *d
tg3_full_unlock(tp);
if (err) {
- free_irq(tp->pdev->irq, dev);
+ pci_free_irq(tp->pdev);
if (tp->tg3_flags2 & TG3_FLG2_USING_MSI) {
pci_disable_msi(tp->pdev);
tp->tg3_flags2 &= ~TG3_FLG2_USING_MSI;
@@ -7363,7 +7359,7 @@ static int tg3_close(struct net_device *
tg3_full_unlock(tp);
- free_irq(tp->pdev->irq, dev);
+ pci_free_irq(tp->pdev);
if (tp->tg3_flags2 & TG3_FLG2_USING_MSI) {
pci_disable_msi(tp->pdev);
tp->tg3_flags2 &= ~TG3_FLG2_USING_MSI;
Hi,
This proof-of-concept patch converts the e1000 driver to use the
pci_request_irq() function.
Please note that I'm not submitting the driver changes, they're there
only for illustration purposes. I'll CC the appropriate maintainers
when/if an API is agreed upon.
Regards,
Frederik
diff --git a/drivers/net/e1000/e1000_ethtool.c b/drivers/net/e1000/e1000_ethtool.c
index 778ede3..8b7be73 100644
drivers/net/e1000/e1000_ethtool.c | 11 +++++------
drivers/net/e1000/e1000_main.c | 15 +++------------
2 files changed, 8 insertions(+), 18 deletions(-)
Index: 2.6.18-mm3/drivers/net/e1000/e1000_ethtool.c
===================================================================
--- 2.6.18-mm3.orig/drivers/net/e1000/e1000_ethtool.c
+++ 2.6.18-mm3/drivers/net/e1000/e1000_ethtool.c
@@ -899,17 +899,16 @@ e1000_intr_test(struct e1000_adapter *ad
{
struct net_device *netdev = adapter->netdev;
uint32_t mask, i=0, shared_int = TRUE;
- uint32_t irq = adapter->pdev->irq;
*data = 0;
/* NOTE: we don't test MSI interrupts here, yet */
/* Hook up test interrupt handler just for this test */
- if (!request_irq(irq, &e1000_test_intr, IRQF_PROBE_SHARED,
- netdev->name, netdev))
+ if (!pci_request_irq(adapter->pdev, &e1000_test_intr, IRQF_PROBE_SHARED,
+ netdev->name))
shared_int = FALSE;
- else if (request_irq(irq, &e1000_test_intr, IRQF_SHARED,
- netdev->name, netdev)) {
+ else if (pci_request_irq(adapter->pdev, &e1000_test_intr, 0,
+ netdev->name)) {
*data = 1;
return -1;
}
@@ -986,7 +985,7 @@ e1000_intr_test(struct e1000_adapter *ad
msleep(10);
/* Unhook test interrupt handler */
- free_irq(irq, netdev);
+ pci_free_irq(adapter->pdev);
return *data;
}
Index: 2.6.18-mm3/drivers/net/e1000/e1000_main.c
===================================================================
--- 2.6.18-mm3.orig/drivers/net/e1000/e1000_main.c
+++ 2.6.18-mm3/drivers/net/e1000/e1000_main.c
@@ -281,9 +281,8 @@ module_exit(e1000_exit_module);
static int e1000_request_irq(struct e1000_adapter *adapter)
{
struct net_device *netdev = adapter->netdev;
- int flags, err = 0;
+ int err = 0;
- flags = IRQF_SHARED;
#ifdef CONFIG_PCI_MSI
if (adapter->hw.mac_type > e1000_82547_rev_2) {
adapter->have_msi = TRUE;
@@ -293,22 +292,14 @@ static int e1000_request_irq(struct e100
adapter->have_msi = FALSE;
}
}
- if (adapter->have_msi)
- flags &= ~IRQF_SHARED;
#endif
- if ((err = request_irq(adapter->pdev->irq, &e1000_intr, flags,
- netdev->name, netdev)))
- DPRINTK(PROBE, ERR,
- "Unable to allocate interrupt Error: %d\n", err);
-
+ err = pci_request_irq(adapter->pdev, &e1000_intr, 0, netdev->name);
return err;
}
static void e1000_free_irq(struct e1000_adapter *adapter)
{
- struct net_device *netdev = adapter->netdev;
-
- free_irq(adapter->pdev->irq, netdev);
+ pci_free_irq(adapter->pdev);
#ifdef CONFIG_PCI_MSI
if (adapter->have_msi)
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.
If a consensus of arch maintainers (i.e. not just willy) think it's
fine, then I'm overruled, otherwise...
Jeff
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);
On Wed, Oct 04, 2006 at 08:29:38PM +0000, Frederik Deweerdt wrote:
> I see. Just to be sure that I got the matter right, does the issue boils
> down to a choice between:
woah, woah, woah, you're getting yourself confused here.
You're looking at what the architectures do here. We're not concerned
with that, we're concerned with what the device drivers do with whatever
value the architecture has stuck in pdev->irq.
On Wed, Oct 04, 2006 at 02:33:11PM -0600, Matthew Wilcox wrote:
> On Wed, Oct 04, 2006 at 08:29:38PM +0000, Frederik Deweerdt wrote:
> > I see. Just to be sure that I got the matter right, does the issue boils
> > down to a choice between:
>
> woah, woah, woah, you're getting yourself confused here.
yep :), I clearly missed the point you made there:
http://lkml.org/lkml/2006/10/3/404
I've re-read it, hope I've got it right this time.
>
> You're looking at what the architectures do here. We're not concerned
> with that, we're concerned with what the device drivers do with whatever
> value the architecture has stuck in pdev->irq.
Not sure I get it still though. Is the issue more than just the location
of the irq validation code? If yes, could you explain what are the
differences between your proposal and Jeff's ?
Anyway, let me have another try at summing up the issue:
#1
- generic irq validation code in include/linux/pci.h
- arch specific irq validation code in include/asm/pci.h
- is_irq_valid() called by pci_request_irq()
BTW this is much like it's done for pci_resource_to_user()
#2
- generic irq validation code in include/linux/interrupt.h
- arch specific irq validation code in include/asm/interrupt.h
- include/linux/pci.h includes linux/interrupt.h
- is_irq_valid() called by pci_request_irq() - and maybe others? -
Thanks,
Frederik
On Wed, Oct 04, 2006 at 09:26:33PM +0000, Frederik Deweerdt wrote:
> On Wed, Oct 04, 2006 at 02:33:11PM -0600, Matthew Wilcox wrote:
> > On Wed, Oct 04, 2006 at 08:29:38PM +0000, Frederik Deweerdt wrote:
> > > I see. Just to be sure that I got the matter right, does the issue boils
> > > down to a choice between:
> >
> > woah, woah, woah, you're getting yourself confused here.
> yep :), I clearly missed the point you made there:
> http://lkml.org/lkml/2006/10/3/404
> I've re-read it, hope I've got it right this time.
> >
> > You're looking at what the architectures do here. We're not concerned
> > with that, we're concerned with what the device drivers do with whatever
> > value the architecture has stuck in pdev->irq.
> Not sure I get it still though. Is the issue more than just the location
> of the irq validation code? If yes, could you explain what are the
> differences between your proposal and Jeff's ?
>
> Anyway, let me have another try at summing up the issue:
>
> #1
> - generic irq validation code in include/linux/pci.h
> - arch specific irq validation code in include/asm/pci.h
> - is_irq_valid() called by pci_request_irq()
s/is_irq_valid/valid_irq/g methinks.
> BTW this is much like it's done for pci_resource_to_user()
>
> #2
> - generic irq validation code in include/linux/interrupt.h
> - arch specific irq validation code in include/asm/interrupt.h
> - include/linux/pci.h includes linux/interrupt.h
> - is_irq_valid() called by pci_request_irq() - and maybe others? -
On Thu, Oct 05, 2006 at 05:59:24PM +0400, Alexey Dobriyan wrote:
> On Wed, Oct 04, 2006 at 09:26:33PM +0000, Frederik Deweerdt wrote:
> > On Wed, Oct 04, 2006 at 02:33:11PM -0600, Matthew Wilcox wrote:
> > > On Wed, Oct 04, 2006 at 08:29:38PM +0000, Frederik Deweerdt wrote:
> > > > I see. Just to be sure that I got the matter right, does the issue boils
> > > > down to a choice between:
> > >
> > > woah, woah, woah, you're getting yourself confused here.
> > yep :), I clearly missed the point you made there:
> > http://lkml.org/lkml/2006/10/3/404
> > I've re-read it, hope I've got it right this time.
> > >
> > > You're looking at what the architectures do here. We're not concerned
> > > with that, we're concerned with what the device drivers do with whatever
> > > value the architecture has stuck in pdev->irq.
> > Not sure I get it still though. Is the issue more than just the location
> > of the irq validation code? If yes, could you explain what are the
> > differences between your proposal and Jeff's ?
> >
> > Anyway, let me have another try at summing up the issue:
> >
> > #1
> > - generic irq validation code in include/linux/pci.h
> > - arch specific irq validation code in include/asm/pci.h
> > - is_irq_valid() called by pci_request_irq()
>
> s/is_irq_valid/valid_irq/g methinks.
The point of the is_ prefix is to make it clear that we're returning 1
if it's true and 0 if it's false.
<checks thread on return values>
err... you said[1]:
> There are at least 3 idioms:
> [...]
> 2) return 1 on YES, 0 on NO.
> [...]
> #2 should only be used if condition in question is spelled nice:
Which I thought made sense, and that's why the is_ prefix is there now.
Am I missing something?
Regards,
Frederik
[1] http://lkml.org/lkml/2006/8/18/399
On Thu, Oct 05, 2006 at 02:36:07PM +0000, Frederik Deweerdt wrote:
> > > - is_irq_valid() called by pci_request_irq()
> >
> > s/is_irq_valid/valid_irq/g methinks.
> The point of the is_ prefix is to make it clear that we're returning 1
> if it's true and 0 if it's false.
> <checks thread on return values>
> err... you said[1]:
> > There are at least 3 idioms:
> > [...]
> > 2) return 1 on YES, 0 on NO.
> > [...]
> > #2 should only be used if condition in question is spelled nice:
> Which I thought made sense, and that's why the is_ prefix is there now.
> Am I missing something?
I think, looking at
if (irq_valid(irq))
one can be damn sure it follows common convention.
That "is_" prefix just beats my ears. If is irq valid.
On Fri, Oct 06, 2006 at 02:04:21PM +0400, Alexey Dobriyan wrote:
> On Thu, Oct 05, 2006 at 02:36:07PM +0000, Frederik Deweerdt wrote:
> > > > - is_irq_valid() called by pci_request_irq()
> > >
> > > s/is_irq_valid/valid_irq/g methinks.
> > The point of the is_ prefix is to make it clear that we're returning 1
> > if it's true and 0 if it's false.
> > <checks thread on return values>
> > err... you said[1]:
> > > There are at least 3 idioms:
> > > [...]
> > > 2) return 1 on YES, 0 on NO.
> > > [...]
> > > #2 should only be used if condition in question is spelled nice:
> > Which I thought made sense, and that's why the is_ prefix is there now.
> > Am I missing something?
>
> I think, looking at
>
> if (irq_valid(irq))
>
> one can be damn sure it follows common convention.
That maybe true, however the is_ prefix just rules out any ambiguity.
Using is/has/have/can for boolean functions whenever possible is a good
practice and I'd prefer to stick to it.
> That "is_" prefix just beats my ears. If is irq valid.
I understand your concerns on the "sound" issues though. Does
is_valid_irq() sound better to you?
Thanks,
Frederik