2006-11-22 02:28:43

by Daniel Barkalow

[permalink] [raw]
Subject: [PATCH] Disable INTx when enabling MSI in forcedeth

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 <[email protected]>
---

(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


2006-11-22 02:43:55

by Linus Torvalds

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



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

2006-11-22 02:53:05

by Daniel Barkalow

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

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*

2006-11-22 03:43:57

by Jeff Garzik

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

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)


Attachments:
patch (940.00 B)

2006-11-22 07:09:27

by Benjamin Herrenschmidt

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

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.

2006-11-22 07:36:49

by Lu, Yinghai

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

On 11/21/06, Jeff Garzik <[email protected]> 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

2006-12-01 01:13:41

by Chris Wright

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

* Linus Torvalds ([email protected]) 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