[PATCH] Disable INTx when enabling MSI in forcedeth

[PATCH] Disable INTx when enabling MSI in forcedeth

[Daniel Barkalow]

My nVidia ethernet card doesn't disable its own INTx when MSI is
enabled. This causes a steady stream of spurious interrupts that
eventually kills my SATA IRQ if MSI is used with forcedeth, which is
true by default. Simply disabling the INTx interrupt takes care of it.

This is against -stable, and would be suitable once someone who knows the 
code verifies that it's correct.

Works for me, but I've obviously only tested the codepath where using
MSI succeeds.

Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>
---

(cc:s to people who were talking about the existance of devices 
that misbehave like this)

The device in question is:
00:07.0 Bridge: nVidia Corporation MCP61 Ethernet (rev a2)
        Subsystem: ASUSTeK Computer Inc. Unknown device 8234
        Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B-
        Status: Cap+ 66MHz+ UDF- FastB2B+ ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR-
        Latency: 0 (250ns min, 5000ns max)
        Interrupt: pin A routed to IRQ 209
        Region 0: Memory at dff7d000 (32-bit, non-prefetchable) [size=4K]
        Region 1: I/O ports at e480 [size=8]
        Capabilities: [44] Power Management version 2
                Flags: PMEClk- DSI- D1+ D2+ AuxCurrent=0mA PME(D0+,D1+,D2+,D3hot+,D3cold+)
                Status: D0 PME-Enable+ DSel=0 DScale=0 PME-
        Capabilities: [50] Message Signalled Interrupts: 64bit+ Queue=0/3 Enable-
                Address: 00000000fee00000  Data: 4032
        Capabilities: [6c] HyperTransport: MSI Mapping

 drivers/net/forcedeth.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/net/forcedeth.c b/drivers/net/forcedeth.c
index 11b8f1b..97087d2 100644
--- a/drivers/net/forcedeth.c
+++ b/drivers/net/forcedeth.c
@@ -2691,12 +2691,14 @@ static int nv_request_irq(struct net_dev
 		}
 	}
 	if (ret != 0 && np->msi_flags & NV_MSI_CAPABLE) {
+		pci_intx(np->pci_dev, 0);
 		if ((ret = pci_enable_msi(np->pci_dev)) == 0) {
 			np->msi_flags |= NV_MSI_ENABLED;
 			if ((!intr_test && request_irq(np->pci_dev->irq, &nv_nic_irq, IRQF_SHARED, dev->name, dev) != 0) ||
 			    (intr_test && request_irq(np->pci_dev->irq, &nv_nic_irq_test, IRQF_SHARED, dev->name, dev) != 0)) {
 				printk(KERN_INFO "forcedeth: request_irq failed %d\n", ret);
 				pci_disable_msi(np->pci_dev);
+				pci_intx(np->pci_dev, 1);
 				np->msi_flags &= ~NV_MSI_ENABLED;
 				goto out_err;
 			}
@@ -2706,6 +2708,8 @@ static int nv_request_irq(struct net_dev
 			writel(0, base + NvRegMSIMap1);
 			/* enable msi vector 0 */
 			writel(NVREG_MSI_VECTOR_0_ENABLED, base + NvRegMSIIrqMask);
+		} else {
+			pci_intx(np->pci_dev, 1);
 		}
 	}
 	if (ret != 0) {
-- 
1.4.2.4

Re: [PATCH] Disable INTx when enabling MSI in forcedeth

[Linus Torvalds]

On Tue, 21 Nov 2006, Daniel Barkalow wrote:
>
> My nVidia ethernet card doesn't disable its own INTx when MSI is
> enabled. This causes a steady stream of spurious interrupts that
> eventually kills my SATA IRQ if MSI is used with forcedeth, which is
> true by default. Simply disabling the INTx interrupt takes care of it.
> 
> This is against -stable, and would be suitable once someone who knows the 
> code verifies that it's correct.

I _really_ think that we should do this in pci_msi_enable().

Screw cards that are not PCI-2.3 compliant - just make the rule be that if 
you use MSI, you _have_ to allow us to set the disable-INTx bit. It's then 
up to the drivers to decide if they can use MSI or not.

(Even a number of cards that are not PCI-2.3 may simply not _implement_ 
the disable-INTx bit, and in that case, they can use MSI if they disable 
INTx automatically - the ).

Comments?

		Linus

Re: [PATCH] Disable INTx when enabling MSI in forcedeth

[Daniel Barkalow]

On Tue, 21 Nov 2006, Linus Torvalds wrote:

> On Tue, 21 Nov 2006, Daniel Barkalow wrote:
> >
> > My nVidia ethernet card doesn't disable its own INTx when MSI is
> > enabled. This causes a steady stream of spurious interrupts that
> > eventually kills my SATA IRQ if MSI is used with forcedeth, which is
> > true by default. Simply disabling the INTx interrupt takes care of it.
> > 
> > This is against -stable, and would be suitable once someone who knows the 
> > code verifies that it's correct.
> 
> I _really_ think that we should do this in pci_msi_enable().
>
> Screw cards that are not PCI-2.3 compliant - just make the rule be that if 
> you use MSI, you _have_ to allow us to set the disable-INTx bit. It's then 
> up to the drivers to decide if they can use MSI or not.
> 
> (Even a number of cards that are not PCI-2.3 may simply not _implement_ 
> the disable-INTx bit, and in that case, they can use MSI if they disable 
> INTx automatically - the ).
> 
> Comments?

I think that's the right thing to do, but I bet it'll break systems until 
the drivers are up to date. So I'd wait until 2.6.20 to do it that way, 
but definitely do it that way then.

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH] Disable INTx when enabling MSI in forcedeth

[Jeff Garzik]

[-- Attachment #1: Type: text/plain, Size: 1086 bytes --]

Linus Torvalds wrote:
> 
> On Tue, 21 Nov 2006, Daniel Barkalow wrote:
>> My nVidia ethernet card doesn't disable its own INTx when MSI is
>> enabled. This causes a steady stream of spurious interrupts that
>> eventually kills my SATA IRQ if MSI is used with forcedeth, which is
>> true by default. Simply disabling the INTx interrupt takes care of it.
>>
>> This is against -stable, and would be suitable once someone who knows the 
>> code verifies that it's correct.
> 
> I _really_ think that we should do this in pci_msi_enable().
> 
> Screw cards that are not PCI-2.3 compliant - just make the rule be that if 
> you use MSI, you _have_ to allow us to set the disable-INTx bit. It's then 
> up to the drivers to decide if they can use MSI or not.
> 
> (Even a number of cards that are not PCI-2.3 may simply not _implement_ 
> the disable-INTx bit, and in that case, they can use MSI if they disable 
> INTx automatically - the ).
> 
> Comments?

I agree.  And it's just a simple matter of remove the PCI-Express 
brackets AFAICS, like in the attached patch (untested).

	Jeff




[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 940 bytes --]

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 9fc9a34..c2828a3 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -255,10 +255,8 @@ static void enable_msi_mode(struct pci_d
 		pci_write_config_word(dev, msi_control_reg(pos), control);
 		dev->msix_enabled = 1;
 	}
-    	if (pci_find_capability(dev, PCI_CAP_ID_EXP)) {
-		/* PCI Express Endpoint device detected */
-		pci_intx(dev, 0);  /* disable intx */
-	}
+
+	pci_intx(dev, 0);  /* disable intx */
 }
 
 void disable_msi_mode(struct pci_dev *dev, int pos, int type)
@@ -276,10 +274,8 @@ void disable_msi_mode(struct pci_dev *de
 		pci_write_config_word(dev, msi_control_reg(pos), control);
 		dev->msix_enabled = 0;
 	}
-    	if (pci_find_capability(dev, PCI_CAP_ID_EXP)) {
-		/* PCI Express Endpoint device detected */
-		pci_intx(dev, 1);  /* enable intx */
-	}
+
+	pci_intx(dev, 1);  /* enable intx */
 }
 
 static int msi_lookup_irq(struct pci_dev *dev, int type)

Re: [PATCH] Disable INTx when enabling MSI in forcedeth

[Benjamin Herrenschmidt]

On Tue, 2006-11-21 at 18:42 -0800, Linus Torvalds wrote:
> 
> On Tue, 21 Nov 2006, Daniel Barkalow wrote:
> >
> > My nVidia ethernet card doesn't disable its own INTx when MSI is
> > enabled. This causes a steady stream of spurious interrupts that
> > eventually kills my SATA IRQ if MSI is used with forcedeth, which is
> > true by default. Simply disabling the INTx interrupt takes care of it.
> > 
> > This is against -stable, and would be suitable once someone who knows the 
> > code verifies that it's correct.
> 
> I _really_ think that we should do this in pci_msi_enable().

Agreed.

> Screw cards that are not PCI-2.3 compliant - just make the rule be that if 
> you use MSI, you _have_ to allow us to set the disable-INTx bit. It's then 
> up to the drivers to decide if they can use MSI or not.
> 
> (Even a number of cards that are not PCI-2.3 may simply not _implement_ 
> the disable-INTx bit, and in that case, they can use MSI if they disable 
> INTx automatically - the ).
> 
> Comments?

Do we know of any card that wants MSIs _and_ uses that bit for some
non-compliant purpose ?

Ben.

Re: [PATCH] Disable INTx when enabling MSI in forcedeth

[Yinghai Lu]

On 11/21/06, Jeff Garzik <jeff@garzik.org> wrote:
> I agree.  And it's just a simple matter of remove the PCI-Express
> brackets AFAICS, like in the attached patch (untested).

How about if the ioapic is not inited for that card?

For example, I didn't initialize irq routing/ioapci for nvidia LAN.

YH

Re: [PATCH] Disable INTx when enabling MSI in forcedeth

[Chris Wright]

* Linus Torvalds (torvalds@osdl.org) wrote:
> On Tue, 21 Nov 2006, Daniel Barkalow wrote:
> >
> > My nVidia ethernet card doesn't disable its own INTx when MSI is
> > enabled. This causes a steady stream of spurious interrupts that
> > eventually kills my SATA IRQ if MSI is used with forcedeth, which is
> > true by default. Simply disabling the INTx interrupt takes care of it.
> > 
> > This is against -stable, and would be suitable once someone who knows the 
> > code verifies that it's correct.
> 
> I _really_ think that we should do this in pci_msi_enable().
> 
> Screw cards that are not PCI-2.3 compliant - just make the rule be that if 
> you use MSI, you _have_ to allow us to set the disable-INTx bit. It's then 
> up to the drivers to decide if they can use MSI or not.
> 
> (Even a number of cards that are not PCI-2.3 may simply not _implement_ 
> the disable-INTx bit, and in that case, they can use MSI if they disable 
> INTx automatically - the ).

Hmm, what do you recommend we do in the meantime, since it's a real
problem and the -stable tree has no fix?  Only issue I had with Daniel's
patch is that it's only half-tested.

thanks,
-chris