[PATCH 2.6.11 4/8] tg3: Add msi test

[PATCH 2.6.11 4/8] tg3: Add msi test

[Michael Chan]

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

Add an MSI test to make sure MSI is working at the system level. If the test
fails, the driver will go back to INTx mode.

Signed-off-by: Michael Chan <mchan@broadcom.com>

[-- Attachment #2: tg3-4.patch --]
[-- Type: application/octet-stream, Size: 3950 bytes --]

diff -Nru 4/drivers/net/tg3.c 5/drivers/net/tg3.c
--- 4/drivers/net/tg3.c	2005-03-15 20:41:51.000000000 -0800
+++ 5/drivers/net/tg3.c	2005-03-17 17:15:40.000000000 -0800
@@ -2965,6 +2965,22 @@
 	return IRQ_RETVAL(handled);
 }
 
+/* ISR for interrupt test */
+static irqreturn_t tg3_test_isr(int irq, void *dev_id,
+		struct pt_regs *regs)
+{
+	struct net_device *dev = dev_id;
+	struct tg3 *tp = netdev_priv(dev);
+	struct tg3_hw_status *sblk = tp->hw_status;
+
+	if (sblk->status & SD_STATUS_UPDATED) {
+		tw32_mailbox(MAILBOX_INTERRUPT_0 + TG3_64BIT_REG_LOW,
+			     0x00000001);
+		return IRQ_RETVAL(1);
+	}
+	return IRQ_RETVAL(0);
+}
+
 static int tg3_init_hw(struct tg3 *);
 static int tg3_halt(struct tg3 *);
 
@@ -5718,6 +5734,113 @@
 	add_timer(&tp->timer);
 }
 
+static int tg3_test_interrupt(struct tg3 *tp)
+{
+	struct net_device *dev = tp->dev;
+	int err, i;
+	u32 int_mbox = 0;
+
+	tg3_disable_ints(tp);
+
+	free_irq(tp->pdev->irq, dev);
+
+	err = request_irq(tp->pdev->irq, tg3_test_isr,
+			  SA_SHIRQ, dev->name, dev);
+	if (err)
+		return err;
+
+	tg3_enable_ints(tp);
+
+	tw32_f(HOSTCC_MODE, tp->coalesce_mode | HOSTCC_MODE_ENABLE |
+	       HOSTCC_MODE_NOW);
+
+	for (i = 0; i < 5; i++) {
+		int_mbox = tr32(MAILBOX_INTERRUPT_0 + TG3_64BIT_REG_LOW);
+		if (int_mbox != 0)
+			break;
+		msleep(10);
+	}
+
+	tg3_disable_ints(tp);
+
+	free_irq(tp->pdev->irq, dev);
+	
+	if (tp->tg3_flags2 & TG3_FLG2_USING_MSI)
+		err = request_irq(tp->pdev->irq, tg3_msi,
+				  0, dev->name, dev);
+	else
+		err = request_irq(tp->pdev->irq, tg3_interrupt,
+				  SA_SHIRQ, dev->name, dev);
+
+	if (err)
+		return err;
+
+	if (int_mbox != 0)
+		return 0;
+
+	return -EIO;
+}
+
+/* Returns 0 if MSI test succeeds or MSI test fails and INTx mode is
+ * successfully restored
+ */
+static int tg3_test_msi(struct tg3 *tp)
+{
+	struct net_device *dev = tp->dev;
+	int err;
+	u16 pci_cmd;
+
+	if (!(tp->tg3_flags2 & TG3_FLG2_USING_MSI))
+		return 0;
+
+	/* Turn off SERR reporting in case MSI terminates with Master
+	 * Abort.
+	 */
+	pci_read_config_word(tp->pdev, PCI_COMMAND, &pci_cmd);
+	pci_write_config_word(tp->pdev, PCI_COMMAND,
+			      pci_cmd & ~PCI_COMMAND_SERR);
+
+	err = tg3_test_interrupt(tp);
+
+	pci_write_config_word(tp->pdev, PCI_COMMAND, pci_cmd);
+
+	if (!err)
+		return 0;
+
+	/* other failures */
+	if (err != -EIO)
+		return err;
+
+	/* MSI test failed, go back to INTx mode */
+	free_irq(tp->pdev->irq, dev);
+	pci_disable_msi(tp->pdev);
+
+	tp->tg3_flags2 &= ~TG3_FLG2_USING_MSI;
+
+	err = request_irq(tp->pdev->irq, tg3_interrupt,
+			  SA_SHIRQ, dev->name, dev);
+
+	if (err)
+		return err;
+
+	/* Need to reset the chip because the MSI cycle may have terminated
+	 * with Master Abort.
+	 */
+	spin_lock_irq(&tp->lock);
+	spin_lock(&tp->tx_lock);
+
+	tg3_halt(tp);
+	err = tg3_init_hw(tp);
+
+	spin_unlock(&tp->tx_lock);
+	spin_unlock_irq(&tp->lock);
+
+	if (err)
+		free_irq(tp->pdev->irq, dev);
+
+	return err;
+}
+
 static int tg3_open(struct net_device *dev)
 {
 	struct tg3 *tp = netdev_priv(dev);
@@ -5782,9 +5905,6 @@
 		tp->timer.expires = jiffies + tp->timer_offset;
 		tp->timer.data = (unsigned long) tp;
 		tp->timer.function = tg3_timer;
-		add_timer(&tp->timer);
-
-		tp->tg3_flags |= TG3_FLAG_INIT_COMPLETE;
 	}
 
 	spin_unlock(&tp->tx_lock);
@@ -5800,9 +5920,32 @@
 		return err;
 	}
 
+	if (tp->tg3_flags2 & TG3_FLG2_USING_MSI) {
+		err = tg3_test_msi(tp);
+		if (err) {
+			spin_lock_irq(&tp->lock);
+			spin_lock(&tp->tx_lock);
+
+			if (tp->tg3_flags2 & TG3_FLG2_USING_MSI) {
+				pci_disable_msi(tp->pdev);
+				tp->tg3_flags2 &= ~TG3_FLG2_USING_MSI;
+			}
+			tg3_halt(tp);
+			tg3_free_rings(tp);
+			tg3_free_consistent(tp);
+
+			spin_unlock(&tp->tx_lock);
+			spin_unlock_irq(&tp->lock);
+
+			return err;
+		}
+	}
+
 	spin_lock_irq(&tp->lock);
 	spin_lock(&tp->tx_lock);
 
+	add_timer(&tp->timer);
+	tp->tg3_flags |= TG3_FLAG_INIT_COMPLETE;
 	tg3_enable_ints(tp);
 
 	spin_unlock(&tp->tx_lock);

Re: [PATCH 2.6.11 4/8] tg3: Add msi test

[Jeff Garzik]

Michael Chan wrote:
> Add an MSI test to make sure MSI is working at the system level. If the test
> fails, the driver will go back to INTx mode.
> 
> Signed-off-by: Michael Chan <mchan@broadcom.com

In theory, this patch is not necessary.

pci_enable_msi() should fail, if MSI is not available at the system level.

What behavior are you seeing?

	Jeff

Re: [PATCH 2.6.11 4/8] tg3: Add msi test

[Michael Chan]

"Jeff Garzik" wrote:

> Michael Chan wrote:
> > Add an MSI test to make sure MSI is working at the system level. If 
> > the test fails, the driver will go back to INTx mode.
> > 
> > Signed-off-by: Michael Chan <mchan@broadcom.com
> 
> In theory, this patch is not necessary.
> 
> pci_enable_msi() should fail, if MSI is not available at the 
> system level.
> 
> What behavior are you seeing?
> 
> 	Jeff
> 
 
Some older chipsets/APICs in older PCI machines do not properly support MSI.
In some cases you will get no interrupts and in a few cases you can get
SERR/NMI because the MSI cycle terminates abnormally. The pci_msi_quirk does
not cover all problem chipsets out there. pci_enable_msi() can succeed on
these problem machines.

Most newer PCI Express machines have no problem with MSI, but we have at
least one machine in the lab that doesn't work with MSI.

The interrupt test code is very small and can conclusively determine if MSI
is working on the machine or not. The code can later be used as part of the
ethtool self test. Without the test, we'll have to resort of a parameter or a
config option to disable MSI in tg3.

Michael

Re: [PATCH 2.6.11 4/8] tg3: Add msi test

[Jeff Garzik]

Michael Chan wrote:
> Some older chipsets/APICs in older PCI machines do not properly support MSI.
> In some cases you will get no interrupts and in a few cases you can get
> SERR/NMI because the MSI cycle terminates abnormally. The pci_msi_quirk does
> not cover all problem chipsets out there. pci_enable_msi() can succeed on
> these problem machines.
> 
> Most newer PCI Express machines have no problem with MSI, but we have at
> least one machine in the lab that doesn't work with MSI.
> 
> The interrupt test code is very small and can conclusively determine if MSI
> is working on the machine or not. The code can later be used as part of the
> ethtool self test. Without the test, we'll have to resort of a parameter or a
> config option to disable MSI in tg3.


OK, so this is not a tg3-related problem at all.

It really sounds like you should update pci_msi_quirk() to cover the 
other affected cases.

I also disagree that a tg3 config option is needed, if the tg3 MSI test 
is not present.  An option to globally disable PCI MSI in the kernel is 
the preferred approach, if such an option doesn't already exist.

	Jeff

Re: [PATCH 2.6.11 4/8] tg3: Add msi test

[David S. Miller]

On Tue, 22 Mar 2005 17:14:58 -0500
Jeff Garzik <jgarzik@pobox.com> wrote:

> OK, so this is not a tg3-related problem at all.

Right.

> It really sounds like you should update pci_msi_quirk() to cover the 
> other affected cases.

That's not possible without either:

1) getting a lot of chipset vendors to reveal stuff they probably
   would rather keep secret (ie. that their chip in particular has
   MSI problems)

2) having a device on which to perform the MSI usability check

See #2?  That's the main issue from genericizing this, we need
a device on which to perform the usability check so that we can
handling a test interrupt and see if state got updated correctly.

So for the time being, Michael's change is OK until we find a
better solution that really does cover all the problem chipsets.

Re: [PATCH 2.6.11 4/8] tg3: Add msi test

[Jeff Garzik]

David S. Miller wrote:
> On Tue, 22 Mar 2005 17:14:58 -0500
> Jeff Garzik <jgarzik@pobox.com> wrote:
> 
> 
>>OK, so this is not a tg3-related problem at all.
> 
> 
> Right.
> 
> 
>>It really sounds like you should update pci_msi_quirk() to cover the 
>>other affected cases.
> 
> 
> That's not possible without either:
> 
> 1) getting a lot of chipset vendors to reveal stuff they probably
>    would rather keep secret (ie. that their chip in particular has
>    MSI problems)
> 
> 2) having a device on which to perform the MSI usability check
> 
> See #2?  That's the main issue from genericizing this, we need
> a device on which to perform the usability check so that we can
> handling a test interrupt and see if state got updated correctly.
> 
> So for the time being, Michael's change is OK until we find a
> better solution that really does cover all the problem chipsets.

I disagree: this will hide bugs.

We already have a standard process in Linux to handle this stuff:

* avoid platform tests in the driver
* when the driver fails, have the user try "pci=nomsi"
* if that works, add system to blacklist

In short, the tg3 driver -without- the MSI test is actually a better MSI 
test.

And as an added bonus, hardware setups with buggy MSI are easily 
highlighted.

MSI is going to hit big, real soon.  We don't need this sort of test in 
ever driver... that will just slow the deployment of a kernel that knows 
which systems are bad for MSI.

	Jeff, who just added MSI support to AHCI SATA driver

Re: [PATCH 2.6.11 4/8] tg3: Add msi test

[David S. Miller]

On Tue, 22 Mar 2005 17:26:44 -0500
Jeff Garzik <jgarzik@pobox.com> wrote:

> I disagree: this will hide bugs.

It actually spots them too. :-)  If you'd like it to print out
something like:

	TG3 MSI test failed on chipset ven[%x] devid[%x] please
	email this info to PCI maintainers at x@y.com

that's fine.

> We already have a standard process in Linux to handle this stuff:
> 
> * avoid platform tests in the driver
> * when the driver fails, have the user try "pci=nomsi"
> * if that works, add system to blacklist

Just because it's what we've been doing doesn't mean it's the
best solution in this case.

If you hope users will just report this stuff, most of them won't.
They'll say Linux doesn't work on my computer and I have real life
stuff to take care of so I'll install freebsd or windows or a different
networking card and get on with life.

Eventually, say over a year or so, the database of bad chipsets will
get built with your scheme, but to much hardship and pain to users.

Re: [PATCH 2.6.11 4/8] tg3: Add msi test

[Jeff Garzik]

David S. Miller wrote:
> Just because it's what we've been doing doesn't mean it's the
> best solution in this case.
> 
> If you hope users will just report this stuff, most of them won't.
> They'll say Linux doesn't work on my computer and I have real life
> stuff to take care of so I'll install freebsd or windows or a different
> networking card and get on with life.
> 
> Eventually, say over a year or so, the database of bad chipsets will
> get built with your scheme, but to much hardship and pain to users.


What do you think will happen for all the other MSI drivers/hardware out 
there?

Adding this test to tg3 simply means another piece of hardware doesn't 
work.  The problem disappears for -you-, but not the user.

You're just pushing the pain around, for the -few- -early- systems with 
problems.

Sorry, I still disagree.  We need to think about how to handle this 
situation for -all- drivers and -all- users, not just tg3.

	Jeff

Re: [PATCH 2.6.11 4/8] tg3: Add msi test

[David S. Miller]

On Tue, 22 Mar 2005 18:50:36 -0500
Jeff Garzik <jgarzik@pobox.com> wrote:

> Sorry, I still disagree.  We need to think about how to handle this 
> situation for -all- drivers and -all- users, not just tg3.

When you come up with something that doesn't require the users
to ask around and report stuff when MSI doesn't work, let me know.

But for now let's hold off on the tg3 MSI support, ok?

Re: [PATCH 2.6.11 4/8] tg3: Add msi test

[Michael Chan]

"David S. Miller" wrote:

> 
> On Tue, 22 Mar 2005 18:50:36 -0500
> Jeff Garzik <jgarzik@pobox.com> wrote:
> 
> > Sorry, I still disagree.  We need to think about how to handle this
> > situation for -all- drivers and -all- users, not just tg3.
> 
> When you come up with something that doesn't require the 
> users to ask around and report stuff when MSI doesn't work, 
> let me know.
> 
> But for now let's hold off on the tg3 MSI support, ok?
> 
> 

Jeff, I understand your point of view which is from the kernel development
perspective. For us, we have to consider the user's perspective when he is
trying to figure out why things are not working. Let's see if we can address
your concerns with the following:

#1. Keep the msi test but add a message telling the user to report to the PCI
maintainer when msi test fails, as David suggested.

#2. Do the msi test and if it fails, just print a message telling the user to
disable MSI some other way and report to PCI maintainer. This will at least
identify the potential problem to the user.

Michael