Re: [PATCH] e1000: Fix msi enable leak on error, don't print error message, cleanup
Auke Kok wrote: pci_enable_msi failure is a normal event so we should not print any error. Going over the code I spotted a missing pci_disable_msi() leak when irq allocation fails. The whole code also needed a cleanup, so I combined the two different calls to pci_request_irq into a single call making this look a lot better. All #ifdef CONFIG_PCI_MSI's have been removed. Compile tested with both CONFIG_PCI_MSI enabled and disabled. Signed-off-by: Auke Kok [EMAIL PROTECTED] Cc: H. Peter Anvin [EMAIL PROTECTED] --- drivers/net/e1000/e1000.h |4 +--- drivers/net/e1000/e1000_main.c | 39 ++- 2 files changed, 15 insertions(+), 28 deletions(-) applied - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] e1000: Fix msi enable leak on error, don't print error message, cleanup
pci_enable_msi failure is a normal event so we should not print any error.
Going over the code I spotted a missing pci_disable_msi() leak when irq
allocation fails. The whole code also needed a cleanup, so I combined the
two different calls to pci_request_irq into a single call making this
look a lot better.
Signed-off-by: Auke Kok [EMAIL PROTECTED]
Cc: H. Peter Anvin [EMAIL PROTECTED]
---
drivers/net/e1000/e1000_main.c | 32 +++-
1 files changed, 15 insertions(+), 17 deletions(-)
diff --git a/drivers/net/e1000/e1000_main.c b/drivers/net/e1000/e1000_main.c
index 637ae8f..b2c5b8e 100644
--- a/drivers/net/e1000/e1000_main.c
+++ b/drivers/net/e1000/e1000_main.c
@@ -300,31 +300,29 @@ module_exit(e1000_exit_module);
static int e1000_request_irq(struct e1000_adapter *adapter)
{
struct net_device *netdev = adapter-netdev;
- int flags, err = 0;
+ void (*handler) = e1000_intr;
+ int irq_flags = IRQF_SHARED;
+ int err;
- flags = IRQF_SHARED;
#ifdef CONFIG_PCI_MSI
if (adapter-hw.mac_type = e1000_82571) {
- adapter-have_msi = TRUE;
- if ((err = pci_enable_msi(adapter-pdev))) {
- DPRINTK(PROBE, ERR,
-Unable to allocate MSI interrupt Error: %d\n, err);
- adapter-have_msi = FALSE;
+ adapter-have_msi = !pci_enable_msi(adapter-pdev);
+ if (adapter-have_msi) {
+ handler = e1000_intr_msi;
+ irq_flags = 0;
}
}
- if (adapter-have_msi) {
- flags = ~IRQF_SHARED;
- err = request_irq(adapter-pdev-irq, e1000_intr_msi, flags,
- netdev-name, netdev);
- if (err)
- DPRINTK(PROBE, ERR,
- Unable to allocate interrupt Error: %d\n, err);
- } else
#endif
- if ((err = request_irq(adapter-pdev-irq, e1000_intr, flags,
- netdev-name, netdev)))
+ err = request_irq(adapter-pdev-irq, handler, irq_flags, netdev-name,
+ netdev);
+ if (err) {
+#ifdef CONFIG_PCI_MSI
+ if (adapter-have_msi)
+ pci_disable_msi(adapter-pdev);
+#endif
DPRINTK(PROBE, ERR,
Unable to allocate interrupt Error: %d\n, err);
+ }
return err;
}
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] e1000: Fix msi enable leak on error, don't print error message, cleanup
Auke Kok wrote: pci_enable_msi failure is a normal event so we should not print any error. Going over the code I spotted a missing pci_disable_msi() leak when irq allocation fails. The whole code also needed a cleanup, so I combined the two different calls to pci_request_irq into a single call making this look a lot better. Signed-off-by: Auke Kok [EMAIL PROTECTED] Cc: H. Peter Anvin [EMAIL PROTECTED] Looks OK, but compounds (and highlights) another problem: you shouldn't be adding CONFIG_PCI_MSI ifdefs to the code. MSI support is properly set up in the headers to enable working code even if CONFIG_PCI_MSI is disabled. Revise your patch to remove CONFIG_PCI_MSI tests, and I'll include it straightaway. Jeff - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] e1000: Fix msi enable leak on error, don't print error message, cleanup
Jeff Garzik wrote: Auke Kok wrote: pci_enable_msi failure is a normal event so we should not print any error. Going over the code I spotted a missing pci_disable_msi() leak when irq allocation fails. The whole code also needed a cleanup, so I combined the two different calls to pci_request_irq into a single call making this look a lot better. Signed-off-by: Auke Kok [EMAIL PROTECTED] Cc: H. Peter Anvin [EMAIL PROTECTED] Looks OK, but compounds (and highlights) another problem: you shouldn't be adding CONFIG_PCI_MSI ifdefs to the code. MSI support is properly set up in the headers to enable working code even if CONFIG_PCI_MSI is disabled. Revise your patch to remove CONFIG_PCI_MSI tests, and I'll include it straightaway. OK, on its way. Thanks. Auke - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] e1000: Fix msi enable leak on error, don't print error message, cleanup
pci_enable_msi failure is a normal event so we should not print any error.
Going over the code I spotted a missing pci_disable_msi() leak when irq
allocation fails. The whole code also needed a cleanup, so I combined the
two different calls to pci_request_irq into a single call making this
look a lot better. All #ifdef CONFIG_PCI_MSI's have been removed.
Compile tested with both CONFIG_PCI_MSI enabled and disabled.
Signed-off-by: Auke Kok [EMAIL PROTECTED]
Cc: H. Peter Anvin [EMAIL PROTECTED]
---
drivers/net/e1000/e1000.h |4 +---
drivers/net/e1000/e1000_main.c | 39 ++-
2 files changed, 15 insertions(+), 28 deletions(-)
diff --git a/drivers/net/e1000/e1000.h b/drivers/net/e1000/e1000.h
index a9ea67e..16a6edf 100644
--- a/drivers/net/e1000/e1000.h
+++ b/drivers/net/e1000/e1000.h
@@ -333,11 +333,9 @@ struct e1000_adapter {
struct e1000_tx_ring test_tx_ring;
struct e1000_rx_ring test_rx_ring;
-
int msg_enable;
-#ifdef CONFIG_PCI_MSI
boolean_t have_msi;
-#endif
+
/* to not mess up cache alignment, always add to the bottom */
boolean_t tso_force;
boolean_t smart_power_down; /* phy smart power down */
diff --git a/drivers/net/e1000/e1000_main.c b/drivers/net/e1000/e1000_main.c
index 637ae8f..49be393 100644
--- a/drivers/net/e1000/e1000_main.c
+++ b/drivers/net/e1000/e1000_main.c
@@ -158,9 +158,7 @@ static struct net_device_stats * e1000_get_stats(struct
net_device *netdev);
static int e1000_change_mtu(struct net_device *netdev, int new_mtu);
static int e1000_set_mac(struct net_device *netdev, void *p);
static irqreturn_t e1000_intr(int irq, void *data);
-#ifdef CONFIG_PCI_MSI
static irqreturn_t e1000_intr_msi(int irq, void *data);
-#endif
static boolean_t e1000_clean_tx_irq(struct e1000_adapter *adapter,
struct e1000_tx_ring *tx_ring);
#ifdef CONFIG_E1000_NAPI
@@ -300,31 +298,26 @@ module_exit(e1000_exit_module);
static int e1000_request_irq(struct e1000_adapter *adapter)
{
struct net_device *netdev = adapter-netdev;
- int flags, err = 0;
+ void (*handler) = e1000_intr;
+ int irq_flags = IRQF_SHARED;
+ int err;
- flags = IRQF_SHARED;
-#ifdef CONFIG_PCI_MSI
if (adapter-hw.mac_type = e1000_82571) {
- adapter-have_msi = TRUE;
- if ((err = pci_enable_msi(adapter-pdev))) {
- DPRINTK(PROBE, ERR,
-Unable to allocate MSI interrupt Error: %d\n, err);
- adapter-have_msi = FALSE;
+ adapter-have_msi = !pci_enable_msi(adapter-pdev);
+ if (adapter-have_msi) {
+ handler = e1000_intr_msi;
+ irq_flags = 0;
}
}
- if (adapter-have_msi) {
- flags = ~IRQF_SHARED;
- err = request_irq(adapter-pdev-irq, e1000_intr_msi, flags,
- netdev-name, netdev);
- if (err)
- DPRINTK(PROBE, ERR,
- Unable to allocate interrupt Error: %d\n, err);
- } else
-#endif
- if ((err = request_irq(adapter-pdev-irq, e1000_intr, flags,
- netdev-name, netdev)))
+
+ err = request_irq(adapter-pdev-irq, handler, irq_flags, netdev-name,
+ netdev);
+ if (err) {
+ if (adapter-have_msi)
+ pci_disable_msi(adapter-pdev);
DPRINTK(PROBE, ERR,
Unable to allocate interrupt Error: %d\n, err);
+ }
return err;
}
@@ -335,10 +328,8 @@ static void e1000_free_irq(struct e1000_adapter *adapter)
free_irq(adapter-pdev-irq, netdev);
-#ifdef CONFIG_PCI_MSI
if (adapter-have_msi)
pci_disable_msi(adapter-pdev);
-#endif
}
/**
@@ -3744,7 +3735,6 @@ e1000_update_stats(struct e1000_adapter *adapter)
spin_unlock_irqrestore(adapter-stats_lock, flags);
}
-#ifdef CONFIG_PCI_MSI
/**
* e1000_intr_msi - Interrupt Handler
@@ -3810,7 +3800,6 @@ e1000_intr_msi(int irq, void *data)
return IRQ_HANDLED;
}
-#endif
/**
* e1000_intr - Interrupt Handler
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] e1000: Fix msi enable leak on error, don't print error message, cleanup
Some more of my paranoid questions :) So, if a driver tries to enable MSI and that is unsuccessful (I'll try to avoid using the possibly loaded term fails) shouldn't that show-up _somewhere_? Just how normal is an attempt to enable MSI not succeding going to remain over time and aren't there times when it does indeed mean that someone should be looking into it? rick jones - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] e1000: Fix msi enable leak on error, don't print error message, cleanup
Rick Jones wrote: Some more of my paranoid questions :) So, if a driver tries to enable MSI and that is unsuccessful (I'll try to avoid using the possibly loaded term fails) shouldn't that show-up _somewhere_? It already does -- in /proc/interrupts. Just how normal is an attempt to enable MSI not succeding going to remain over time and aren't there times when it does indeed mean that someone should be looking into it? If we ever want a message, that should go in the general code and not in each driver. -hpa - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] e1000: Fix msi enable leak on error, don't print error message, cleanup
Rick Jones wrote: So, if a driver tries to enable MSI and that is unsuccessful (I'll try to avoid using the possibly loaded term fails) shouldn't that show-up _somewhere_? Just how normal is an attempt to enable MSI not succeding going to remain over time and aren't there times when it does indeed mean that someone should be looking into it? If there is an MSI problem, it is the job of the PCI core to complain. Jeff - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] e1000: Fix msi enable leak on error, don't print error message, cleanup
From: Rick Jones [EMAIL PROTECTED] Date: Wed, 16 May 2007 11:41:15 -0700 Some more of my paranoid questions :) So, if a driver tries to enable MSI and that is unsuccessful (I'll try to avoid using the possibly loaded term fails) shouldn't that show-up _somewhere_? Just how normal is an attempt to enable MSI not succeding going to remain over time and aren't there times when it does indeed mean that someone should be looking into it? We have whole platforms where pci_enable_msi() is going to fail on every call, it doesn't help to mention this to be honest. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] e1000: Fix msi enable leak on error, don't print error message, cleanup
Jeff Garzik wrote: Rick Jones wrote: But that is rather incidental isn't it? Would some sort of system health monitor be likely to be checking that for interrupt flavors? And Well, that's where the information is exported in a standard way. I hope you're not suggesting that a system health monitor should be parsing random, driver-specific printk messages to obtain the same information? No, I wouldn't. The only system health monitor I would expect to be parsing that sort of thing would be a human. Perhaps I'm just backing-into the meta question via the specifics of this driver patch. just looking at /proc/interrupts, while it tells you what sort of interrupt is being used, it doesn't (IIRC) say anything about what sort of interrupt the driver _tried_ to use. True. In the context of this thread, it could be any number of reasons: MSI isn't compiled in. MSI was disabled at runtime via kernel command line. MSI was disabled by BIOS quirk. MSI enable was attempted, but failed for some reason. None of those reasons are really driver-specific, or need driver-specific complaint messages. Agreed. But is the PCI (?) subsystem doing something in that regard or is this a hole? rick jones - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] e1000: Fix msi enable leak on error, don't print error message, cleanup
Rick Jones wrote: Agreed. But is the PCI (?) subsystem doing something in that regard or is this a hole? Depends on your platform, your BIOS, your kernel command line, ... :) Jeff - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
