Skip to contentSkip to conversation

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

PATCH In review Auke Kok 5 participants 11 messages May 16, 2007

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Auke Kok

author
#0 lore raw

PATCH 1/1 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(-)
drivers/net/e1000/e1000_main.c +15 -17
index 637ae8f..b2c5b8e 100644
@@ -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;
 }
#1 lore raw
Quoted 8 lines · 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 <[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

author
#2 lore raw
Quoted 7 lines · Jeff Garzik
Quoted 6 lines · 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.

Quoted 6 lines
Quoted 2 lines
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

#3 lore raw
In reply to Jeff Garzik

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

#4 lore raw
Quoted 5 lines · 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_?

It already does -- in /proc/interrupts.

Quoted 3 lines

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

#5 lore raw
Quoted 10 lines · H. Peter Anvin
Quoted 5 lines · 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_?

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

#6 lore raw
Quoted 1 line · Rick Jones

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?

Quoted 1 line

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

#7 lore raw
Quoted 10 lines · Jeff Garzik
Quoted 2 lines · Rick Jones

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.

Quoted 7 lines
Quoted 3 lines

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.

Quoted 7 lines

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/

#8 lore raw
Quoted 1 line · Rick Jones

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

#9 lore raw
In reply to Rick Jones
Quoted 1 line · Rick Jones

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

maintainer
#10 lore raw
In reply to Rick Jones

From: Rick Jones <[email protected]>
Date: Wed, 16 May 2007 11:41:15 -0700

Quoted 7 lines

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.