[PATCH] e1000: Fix msi enable leak on error, don't print error message, cleanup

[PATCH] e1000: Fix msi enable leak on error, don't print error message, cleanup

[Auke Kok]

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 <auke-jan.h.kok@intel.com>
Cc: H. Peter Anvin <hpa@zytor.com>
---

 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;
 }

[PATCH] e1000: Fix msi enable leak on error, don't print error message, cleanup

[Auke Kok]

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 <auke-jan.h.kok@intel.com>
Cc: H. Peter Anvin <hpa@zytor.com>
---

 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

Re: [PATCH] e1000: Fix msi enable leak on error, don't print error message, cleanup

[Jeff Garzik]

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 <auke-jan.h.kok@intel.com>
> Cc: H. Peter Anvin <hpa@zytor.com>

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

Re: [PATCH] e1000: Fix msi enable leak on error, don't print error message, cleanup

[Kok, Auke]

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 <auke-jan.h.kok@intel.com>
>> Cc: H. Peter Anvin <hpa@zytor.com>
> 
> 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

Re: [PATCH] e1000: Fix msi enable leak on error, don't print error message, cleanup

[Rick Jones]

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

Re: [PATCH] e1000: Fix msi enable leak on error, don't print error message, cleanup

[H. Peter Anvin]

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

Re: [PATCH] e1000: Fix msi enable leak on error, don't print error message, cleanup

[Jeff Garzik]

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

Re: [PATCH] e1000: Fix msi enable leak on error, don't print error message, cleanup

[Rick Jones]

H. Peter Anvin wrote:
> 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.

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 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.

rick jones

Re: [PATCH] e1000: Fix msi enable leak on error, don't print error message, cleanup

[Jeff Garzik]

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?


> 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.

	Jeff

Re: [PATCH] e1000: Fix msi enable leak on error, don't print error message, cleanup

[David Miller]

From: Rick Jones <rick.jones2@hp.com>
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.

Re: [PATCH] e1000: Fix msi enable leak on error, don't print error message, cleanup

[Rick Jones]

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

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [PATCH] e1000: Fix msi enable leak on error, don't print error message, cleanup

[Jeff Garzik]

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

Re: [PATCH] e1000: Fix msi enable leak on error, don't print error message, cleanup

[Jeff Garzik]

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 <auke-jan.h.kok@intel.com>
> Cc: H. Peter Anvin <hpa@zytor.com>
> ---
> 
>  drivers/net/e1000/e1000.h      |    4 +---
>  drivers/net/e1000/e1000_main.c |   39 ++++++++++++++-------------------------
>  2 files changed, 15 insertions(+), 28 deletions(-)

applied