Hi all,
This is take #2 of the "add pci_{request,free}_irq" patch.
The following changes have been made since last proposal:
- fix broken kerneldoc (Randy Dunlap)
- change warning message (Alan Cox)
- taken into account the various comments made by Matthew Wilcox
- remove the IRQF_SHARED flag from the request_irq() call: not all
drivers (eg. tg3) set it (Arjan van de Ven suggested dropping it
before the call, but this may not suit the different usage patterns,
tg3 in particular)
I'll send a follow-up patch showing the implied modifications for the
following - semi-randomly chosen :) - drivers: aic7xxx, aic79xx, tg3
and e1000 (Dropped the drm one, which was NACKed by the maintainer).
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
PS: trimmed the CC list a bit, this must be noise to linux-scsi
PPS: used quilt to manage patches, I should have done it long ago :)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index a544997..50b49ef 100644
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,
+ 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_PCI_IRQ
+static inline int is_irq_valid(unsigned int irq)
+{
+ return irq ? 1 : 0;
+}
+#endif /* ARCH_VALIDATE_PCI_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
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,
+ IRQF_SHARED, "aic7xxx");
+
+ if (!error) {
+ ahc->platform_data->irq = ahc->dev_softc->irq;
+ ahc->init_level++;
+ }
- ahc->init_level++;
- return (0);
+ return error;
}
/*
Frederik Deweerdt wrote:
> +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,
> + name ? name : pdev->driver->name,
> + pci_get_drvdata(pdev));
> +}
> +EXPORT_SYMBOL(pci_request_irq);
> +void pci_free_irq(struct pci_dev *pdev)
> +{
> + free_irq(pdev->irq, pci_get_drvdata(pdev));
> +}
> +EXPORT_SYMBOL(pci_free_irq);
ACK these parts
> 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_PCI_IRQ
> +static inline int is_irq_valid(unsigned int irq)
> +{
> + return irq ? 1 : 0;
> +}
> +#endif /* ARCH_VALIDATE_PCI_IRQ */
It's not appropriate to have PCI IRQ stuff in linux/interrupt.h.
This is precisely why I passed 'struct pci_dev *' to a PCI-specific irq
validation function, and prototyped it in linux/pci.h.
Jeff
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
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,
+ IRQF_SHARED, "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
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
@@ -6853,7 +6853,7 @@ static int tg3_request_irq(struct tg3 *t
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, flags, dev->name);
}
static int tg3_test_interrupt(struct tg3 *tp)
@@ -6866,10 +6866,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_SHARED | IRQF_SAMPLE_RANDOM, dev->name);
if (err)
return err;
@@ -6897,7 +6897,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 +6915,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 +6945,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 +6965,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 +7050,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 +7362,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
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, IRQF_SHARED,
+ 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
@@ -296,19 +296,13 @@ static int e1000_request_irq(struct e100
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, flags, 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)
> >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_PCI_IRQ
> >+static inline int is_irq_valid(unsigned int irq)
> >+{
> >+ return irq ? 1 : 0;
> >+}
> >+#endif /* ARCH_VALIDATE_PCI_IRQ */
>
> It's not appropriate to have PCI IRQ stuff in linux/interrupt.h.
>
> This is precisely why I passed 'struct pci_dev *' to a PCI-specific irq validation function, and prototyped it in linux/pci.h.
>
My bad, I've mixed your proposal and Matthew's, isn't this just a
matter of:
s/ARCH_VALIDATE_PCI_IRQ/ARCH_VALIDATE_IRQ/ ?
I'll look if there's some non-PCI code that might check the irq's value
and thus might benefit from this.
Regards,
Frederik
Frederik Deweerdt wrote:
> + else if (pci_request_irq(adapter->pdev, &e1000_test_intr, IRQF_SHARED,
> + netdev->name)) {
doesn't need IRQF_SHARED flag anymore
Frederik Deweerdt wrote:
> 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
> 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
> @@ -6853,7 +6853,7 @@ static int tg3_request_irq(struct tg3 *t
> 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, flags, dev->name);
> }
>
> static int tg3_test_interrupt(struct tg3 *tp)
> @@ -6866,10 +6866,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_SHARED | IRQF_SAMPLE_RANDOM, dev->name);
IRQF_SHARED flags are still left hanging around...
Frederik Deweerdt wrote:
> My bad, I've mixed your proposal and Matthew's, isn't this just a
> matter of:
> s/ARCH_VALIDATE_PCI_IRQ/ARCH_VALIDATE_IRQ/ ?
>
> I'll look if there's some non-PCI code that might check the irq's value
> and thus might benefit from this.
The irq value comes from the PCI subsystem... The PCI subsystem should
validate it.
Jeff
On Tue, Oct 03, 2006 at 06:37:43PM -0400, Jeff Garzik wrote:
> Frederik Deweerdt wrote:
> >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
> >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
> >@@ -6853,7 +6853,7 @@ static int tg3_request_irq(struct tg3 *t
> > 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, flags, dev->name);
> > }
> > static int tg3_test_interrupt(struct tg3 *tp)
> >@@ -6866,10 +6866,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_SHARED | IRQF_SAMPLE_RANDOM, dev->name);
>
> IRQF_SHARED flags are still left hanging around...
I did it on purpose (see parent post): some parts of tg3 for example
don't pass IRQF_SHARED, so I though it wasn't a good idea to enforce
IRQF_SHARED in all cases. Did I miss something?
Frederik
On Tue, Oct 03, 2006 at 06:37:12PM -0400, Jeff Garzik wrote:
> Frederik Deweerdt wrote:
> >My bad, I've mixed your proposal and Matthew's, isn't this just a
> >matter of:
> >s/ARCH_VALIDATE_PCI_IRQ/ARCH_VALIDATE_IRQ/ ?
> >
> >I'll look if there's some non-PCI code that might check the irq's value
> >and thus might benefit from this.
>
> The irq value comes from the PCI subsystem... The PCI subsystem should
> validate it.
That's not true. The value in the pci_dev->irq field has been changed
by the architecture. See, for example, pci_read_irq_line() in
arch/powerpc/kernel/pci_32.c. It's a Linux IRQ number, not a PCI IRQ
number.
Frederik Deweerdt wrote:
> On Tue, Oct 03, 2006 at 06:37:43PM -0400, Jeff Garzik wrote:
>> Frederik Deweerdt wrote:
>>> 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
>>> 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
>>> @@ -6853,7 +6853,7 @@ static int tg3_request_irq(struct tg3 *t
>>> 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, flags, dev->name);
>>> }
>>> static int tg3_test_interrupt(struct tg3 *tp)
>>> @@ -6866,10 +6866,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_SHARED | IRQF_SAMPLE_RANDOM, dev->name);
>> IRQF_SHARED flags are still left hanging around...
> I did it on purpose (see parent post): some parts of tg3 for example
> don't pass IRQF_SHARED, so I though it wasn't a good idea to enforce
> IRQF_SHARED in all cases. Did I miss something?
When it's hardcoded into the definition of pci_request_irq(), then
setting the flag in the above code is logically superfluous.
As for why the flag may be missing -- PCI MSI interrupts are never
shared. However, it won't _hurt_ anything to set the flag needlessly,
AFAIK.
Jeff
From: Jeff Garzik <[email protected]>
Date: Wed, 04 Oct 2006 01:38:31 -0400
> As for why the flag may be missing -- PCI MSI interrupts are never
> shared. However, it won't _hurt_ anything to set the flag needlessly,
> AFAIK.
That's right.
On Tue, Oct 03, 2006 at 11:27:48PM -0700, David Miller wrote:
> From: Jeff Garzik <[email protected]>
> Date: Wed, 04 Oct 2006 01:38:31 -0400
>
> > As for why the flag may be missing -- PCI MSI interrupts are never
> > shared. However, it won't _hurt_ anything to set the flag needlessly,
> > AFAIK.
>
> That's right.
Just to make sure I understood: does this means that the code at lines
296 and 297 in e1000_main.c can be safely removed?
296 if (adapter->have_msi)
297 flags &= ~IRQF_SHARED;
298 #endif
299 if ((err = request_irq(adapter->pdev->irq, &e1000_intr, flags,
300 netdev->name, netdev)))
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>