forcedeth: msi interrupts

PATCH In review Ayaz Abdulla 3 participants 15 messages Jun 3, 2008 – Jun 8, 2008

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Ayaz Abdulla

author

2008-06-03 23:56 UTC #0 lore raw

PATCH 1/1 forcedeth: msi interrupts

This patch adds a workaround for lost MSI interrupts. There is a race condition in the HW in which future interrupts could be missed. The workaround is to toggle the MSI irq mask.

Signed-off-by Ayaz Abdulla <[email protected]>

drivers/net/forcedeth.c +20 -0

@@ -3277,6 +3277,20 @@
 	dprintk(KERN_DEBUG "%s: link change notification done.\n", dev->name);
 }
 
+static inline void nv_msi_workaround(struct net_device *dev)
+{
+	struct fe_priv *np = netdev_priv(dev);
+	u8 __iomem *base = get_hwbase(dev);
+
+	/* Need to toggle the msi irq mask within the ethernet device,
+	 * otherwise, future interrupts will not be detected.
+	 */
+	if (np->msi_flags & NV_MSI_ENABLED) {
+		writel(0, base + NvRegMSIIrqMask);
+		writel(NVREG_MSI_VECTOR_0_ENABLED, base + NvRegMSIIrqMask);
+	}
+}
+
 static irqreturn_t nv_nic_irq(int foo, void *data)
 {
 	struct net_device *dev = (struct net_device *) data;

@@ -3299,6 +3313,8 @@
 		if (!(events & np->irqmask))
 			break;
 
+		nv_msi_workaround(dev);
+
 		spin_lock(&np->lock);
 		nv_tx_done(dev);
 		spin_unlock(&np->lock);

@@ -3414,6 +3430,8 @@
 		if (!(events & np->irqmask))
 			break;
 
+		nv_msi_workaround(dev);
+
 		spin_lock(&np->lock);
 		nv_tx_done_optimized(dev, TX_WORK_PER_LOOP);
 		spin_unlock(&np->lock);

@@ -3754,6 +3772,8 @@
 	if (!(events & NVREG_IRQ_TIMER))
 		return IRQ_RETVAL(0);
 
+	nv_msi_workaround(dev);
+
 	spin_lock(&np->lock);
 	np->intr_test = 1;
 	spin_unlock(&np->lock);

Now, if the (np->msi_flags & NV_MSI_ENABLED) test is usually false then there might be some advantage in inlining the whole thing. Or just inlining the NV_MSI_ENABLED test, but those two writel()s probably aren't worth the fuss of uninlining.

Semi-relatedly, the driver does an awful lot of this:

	struct fe_priv *np = get_nvpriv(dev);
	u8 __iomem *base = get_hwbase(dev);

but get_hwbase() re-evaluates netdev_priv(). It would be more efficient to pass an fe_priv* into get_hwbase(). Or, better, just remove get_hwbase() and open-code `np->base' everywhere.

Karen Shaeffer

2008-06-04 23:37 UTC #2 lore raw

Quoted 9 lines · Andrew Morton

Quoted 4 lines · On Tue, 03 Jun 2008 16:51:46 -0400 Ayaz Abdulla <[email protected]> wrote:
This patch adds a workaround for lost MSI interrupts. There is a race condition in the HW in which future interrupts could be missed. The workaround is to toggle the MSI irq mask.
Do you think this is a 2.6.26 thing?

Or does it go back even further, maybe even as far as 2.6.18? I've observed the CK48 NICs hang on 2.6.18 - 2.6.24 kernels. Just wondering if this is relevant.

Thanks,
Karen

Signature · 3 lines

 Karen Shaeffer
 Neuralscape, Palo Alto, Ca. 94306
 [email protected]  http://www.neuralscape.com

Ayaz Abdulla

author

2008-06-06 20:15 UTC #3 lore raw

In reply to Andrew Morton

Quoted 9 lines · Andrew Morton

Quoted 4 lines · On Tue, 03 Jun 2008 16:51:46 -0400 Ayaz Abdulla <[email protected]> wrote:
This patch adds a workaround for lost MSI interrupts. There is a race condition in the HW in which future interrupts could be missed. The workaround is to toggle the MSI irq mask.
Do you think this is a 2.6.26 thing?

It is by HW design, not related to the kernel.

Quoted 119 lines

drivers/net/forcedeth.c 2008-06-03 16:31:44.000000000 -0400 line 3277

Quoted 6 lines

[patch-forcedeth-msi-irq text/plain (1.3KB)]
drivers/net/forcedeth.c 2008-06-03 16:31:44.000000000 -0400 renamed from drivers/net/forcedeth.c 2008-06-03 16:16:26.000000000 -0400 +0 -0
@@ -3277,6 +3277,20 @@
      dprintk(KERN_DEBUG "%s: link change notification done.\n", 

dev->name);

Quoted 39 lines

drivers/net/forcedeth.c 2008-06-03 16:31:44.000000000 -0400 renamed from drivers/net/forcedeth.c 2008-06-03 16:16:26.000000000 -0400 +18 -0

 }
 
+static inline void nv_msi_workaround(struct net_device *dev)
+{
+     struct fe_priv *np = netdev_priv(dev);
+     u8 __iomem *base = get_hwbase(dev);
+
+     /* Need to toggle the msi irq mask within the ethernet device,
+      * otherwise, future interrupts will not be detected.
+      */
+     if (np->msi_flags & NV_MSI_ENABLED) {
+             writel(0, base + NvRegMSIIrqMask);
+             writel(NVREG_MSI_VECTOR_0_ENABLED, base + NvRegMSIIrqMask);
+     }
+}
+
 static irqreturn_t nv_nic_irq(int foo, void *data)
 {
      struct net_device *dev = (struct net_device *) data;

@@ -3299,6 +3313,8 @@
              if (!(events & np->irqmask))
                      break;
 
+             nv_msi_workaround(dev);
+
              spin_lock(&np->lock);
              nv_tx_done(dev);
              spin_unlock(&np->lock);

@@ -3414,6 +3430,8 @@
              if (!(events & np->irqmask))
                      break;
 
+             nv_msi_workaround(dev);
+
              spin_lock(&np->lock);
              nv_tx_done_optimized(dev, TX_WORK_PER_LOOP);
              spin_unlock(&np->lock);

drivers/net/forcedeth.c 2008-06-03 16:31:44.000000000 -0400 line 3779

Quoted 8 lines

drivers/net/forcedeth.c 2008-06-03 16:31:44.000000000 -0400 renamed from drivers/net/forcedeth.c 2008-06-03 16:16:26.000000000 -0400 +2 -0
@@ -3754,6 +3772,8 @@
      if (!(events & NVREG_IRQ_TIMER))
              return IRQ_RETVAL(0);
 
+     nv_msi_workaround(dev);
+
      spin_lock(&np->lock);
      np->intr_test = 1;
      spin_unlock(&np->lock);

I'm not loving the implementation.

- That `inline' adds 35 bytes more text to the driver, and we expect that this often yields slower code.

- Every caller of nv_msi_workaround() already has the fe_priv* in a local variable, so why not pass that in and save the additional pointer calculation?

So this:

diff -puN drivers/net/forcedeth.c~forcedeth-msi-interrupts-uninlining drivers/net/forcedeth.c

drivers/net/forcedeth.c renamed from drivers/net/forcedeth.c~forcedeth-msi-interrupts-uninlining +3 -4

@@ -3277,15 +3277,14 @@ static void nv_link_irq(struct net_devic
        dprintk(KERN_DEBUG "%s: link change notification done.\n", 
 dev->name);
 }
 
-static inline void nv_msi_workaround(struct net_device *dev)
+static void nv_msi_workaround(struct fe_priv *np)
 {
-       struct fe_priv *np = netdev_priv(dev);
-       u8 __iomem *base = get_hwbase(dev);
-
        /* Need to toggle the msi irq mask within the ethernet device,
         * otherwise, future interrupts will not be detected.
         */
        if (np->msi_flags & NV_MSI_ENABLED) {
+               u8 __iomem *base = np->base;
+
                writel(0, base + NvRegMSIIrqMask);
                writel(NVREG_MSI_VECTOR_0_ENABLED, base + NvRegMSIIrqMask);

drivers/net/forcedeth.c renamed from drivers/net/forcedeth.c~forcedeth-msi-interrupts-uninlining +3 -3

@@ -3313,7 +3312,7 @@ static irqreturn_t nv_nic_irq(int foo, v
                if (!(events & np->irqmask))
                        break;
 
-               nv_msi_workaround(dev);
+               nv_msi_workaround(np);
 
                spin_lock(&np->lock);
                nv_tx_done(dev);

@@ -3430,7 +3429,7 @@ static irqreturn_t nv_nic_irq_optimized(
                if (!(events & np->irqmask))
                        break;
 
-               nv_msi_workaround(dev);
+               nv_msi_workaround(np);
 
                spin_lock(&np->lock);
                nv_tx_done_optimized(dev, TX_WORK_PER_LOOP);

@@ -3772,7 +3771,7 @@ static irqreturn_t nv_nic_irq_test(int f
        if (!(events & NVREG_IRQ_TIMER))
                return IRQ_RETVAL(0);
 
-       nv_msi_workaround(dev);
+       nv_msi_workaround(np);
 
        spin_lock(&np->lock);
        np->intr_test = 1;

Quoted 8 lines

save 42 bytes of text.
Now, if the (np->msi_flags & NV_MSI_ENABLED) test is usually false then there might be some advantage in inlining the whole thing. Or just inlining the NV_MSI_ENABLED test, but those two writel()s probably aren't worth the fuss of uninlining.

Sure, thats fine.

Quoted 12 lines

Semi-relatedly, the driver does an awful lot of this:
        struct fe_priv *np = get_nvpriv(dev);
        u8 __iomem *base = get_hwbase(dev);
but get_hwbase() re-evaluates netdev_priv(). It would be more efficient to pass an fe_priv* into get_hwbase(). Or, better, just remove get_hwbase() and open-code `np->base' everywhere.

-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

Andrew Morton

2008-06-06 20:20 UTC #4 lore raw

Quoted 11 lines · On Fri, 06 Jun 2008 13:15:32 -0400 Ayaz Abdulla <[email protected]> wrote:

Quoted 9 lines · Andrew Morton
Quoted 4 lines · On Tue, 03 Jun 2008 16:51:46 -0400 Ayaz Abdulla <[email protected]> wrote:
This patch adds a workaround for lost MSI interrupts. There is a race condition in the HW in which future interrupts could be missed. The workaround is to toggle the MSI irq mask.
Do you think this is a 2.6.26 thing?
It is by HW design, not related to the kernel.

Sorry, what I meant was: do you believe that this patch should be in 2.6.26?

Ayaz Abdulla

author

2008-06-06 21:04 UTC #5 lore raw

Quoted 14 lines · Andrew Morton

Quoted 10 lines · On Fri, 06 Jun 2008 13:15:32 -0400 Ayaz Abdulla <[email protected]> wrote:
Quoted 8 lines · Andrew Morton
Quoted 4 lines · On Tue, 03 Jun 2008 16:51:46 -0400 Ayaz Abdulla <[email protected]> wrote:
This patch adds a workaround for lost MSI interrupts. There is a race condition in the HW in which future interrupts could be missed. The workaround is to toggle the MSI irq mask.

Quoted 7 lines

Quoted 3 lines
Quoted 1 line
Do you think this is a 2.6.26 thing?
It is by HW design, not related to the kernel.
Sorry, what I meant was: do you believe that this patch should be in 2.6.26?

Yes, it should be treated as a critical fix.

Andrew Morton

2008-06-06 21:11 UTC #6 lore raw

Quoted 17 lines · On Fri, 06 Jun 2008 14:04:05 -0400 Ayaz Abdulla <[email protected]> wrote:

Quoted 14 lines · Andrew Morton
Quoted 10 lines · On Fri, 06 Jun 2008 13:15:32 -0400 Ayaz Abdulla <[email protected]> wrote:
Quoted 8 lines · Andrew Morton
Quoted 4 lines · On Tue, 03 Jun 2008 16:51:46 -0400 Ayaz Abdulla <[email protected]> wrote:
This patch adds a workaround for lost MSI interrupts. There is a race condition in the HW in which future interrupts could be missed. The workaround is to toggle the MSI irq mask.

Quoted 8 lines

Quoted 7 lines
Quoted 3 lines
Quoted 1 line
Do you think this is a 2.6.26 thing?
It is by HW design, not related to the kernel.
Sorry, what I meant was: do you believe that this patch should be in 2.6.26?
Yes, it should be treated as a critical fix.

So should it also be backported into 2.6.25.x?

Bear in mind that $major_distros are apparently basing product on 2.6.25.

Ayaz Abdulla

author

2008-06-06 21:23 UTC #7 lore raw

Yes, that would be great!

-----Original Message-----
From: Andrew Morton [mailto:[email protected]]
Sent: Friday, June 06, 2008 2:11 PM
To: Ayaz Abdulla
Cc: [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH] forcedeth: msi interrupts

Quoted 7 lines · On Fri, 06 Jun 2008 14:04:05 -0400 Ayaz Abdulla <[email protected]> wrote:

Quoted 4 lines · Andrew Morton
On Fri, 06 Jun 2008 13:15:32 -0400
Ayaz Abdulla <[email protected]> wrote:

Quoted 6 lines

Quoted 6 lines
Quoted 6 lines
Quoted 4 lines · Andrew Morton
Quoted 1 line · On Tue, 03 Jun 2008 16:51:46 -0400 Ayaz Abdulla <[email protected]> wrote:
This patch adds a workaround for lost MSI interrupts. There is a

race

Quoted 1 line

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
condition in the HW in which future interrupts could be missed.

The

Quoted 11 lines

Quoted 10 lines
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
workaround is to toggle the MSI irq mask.
Do you think this is a 2.6.26 thing?
It is by HW design, not related to the kernel.
Sorry, what I meant was: do you believe that this patch should be in 2.6.26?
Yes, it should be treated as a critical fix.

So should it also be backported into 2.6.25.x?

Bear in mind that $major_distros are apparently basing product on 2.6.25.
-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

Karen Shaeffer

2008-06-06 21:40 UTC #8 lore raw

Quoted 1 line · Ayaz Abdulla

Yes, that would be great!

Hi Ayaz,
How far back should it be back ported? Do you know? Thanks,
Karen

Quoted 42 lines

-----Original Message-----
From: Andrew Morton [mailto:[email protected]]
Sent: Friday, June 06, 2008 2:11 PM
To: Ayaz Abdulla
Cc: [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH] forcedeth: msi interrupts
Quoted 7 lines · On Fri, 06 Jun 2008 14:04:05 -0400 Ayaz Abdulla <[email protected]> wrote:
Quoted 4 lines · Andrew Morton
On Fri, 06 Jun 2008 13:15:32 -0400
Ayaz Abdulla <[email protected]> wrote:
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 4 lines · Andrew Morton
Quoted 1 line · On Tue, 03 Jun 2008 16:51:46 -0400 Ayaz Abdulla <[email protected]> wrote:
This patch adds a workaround for lost MSI interrupts. There is a
race
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
condition in the HW in which future interrupts could be missed.
The
Quoted 11 lines
Quoted 10 lines
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
workaround is to toggle the MSI irq mask.
Do you think this is a 2.6.26 thing?
It is by HW design, not related to the kernel.
Sorry, what I meant was: do you believe that this patch should be in 2.6.26?
Yes, it should be treated as a critical fix.
So should it also be backported into 2.6.25.x?

Quoted 12 lines

Bear in mind that $major_distros are apparently basing product on 2.6.25.
-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

---end quoted text---

Signature · 3 lines

 Karen Shaeffer
 Neuralscape, Palo Alto, Ca. 94306
 [email protected]  http://www.neuralscape.com

Karen Shaeffer

2008-06-07 18:31 UTC #9 lore raw

Quoted 5 lines · Karen Shaeffer

Quoted 1 line · Ayaz Abdulla
Yes, that would be great!
Hi Ayaz,
How far back should it be back ported? Do you know?

Hello,
Let me explain and maybe get some advice. I have recently done work with the Sun Netra X4200 M2 Server that uses the Nvidia CK48 chipset and the forcedeth driver. You can see an architecture overview here:
http://www.sun.com/servers/netra/x4200/wp.pdf

The Nvidia NIC that is integrated into the Nvidia 2200 chip, has a failure mode for the following linux kernels 2.6.21.x
2.6.23.x
2.6.24.x
RHEL 2.6.18*

The NIC will hang under specific conditions for all these kernels. First, you must run the NIC in 100 Mb mode with autoneg enabled, then it will always link in a mismatch with the switch. The switch will link at 100 Mb full duplex, while the Nvidia NIC will link at 100 Mb half duplex. This was shown with both Cisco managed switches and HP managed switches, and I suspect it will happen with any switch. (You can force the NIC to 100 Mb full, but autoneg will always result in the link mismatch.)

Once this link mismatch is in effect, then, if you run it long enough, the NIC will eventually hang and become completely disabled. (I know you shouldn't run a NIC in link mismatch, but end users in the field sometimes don't realize it has happened.) It could take days or weeks under reasonably heavy load, but it will always hang in the end. Continually rebooting the server will result in the hang in a matter of hours, where the link negotiation results in the hang. No packets are ever transmitted in these cases. Because it is reproducable in a matter of hours, this is the preferred way to reproduce the failure mode.

The ethtool online test will pass. The ethtool offline test will fail. The driver does TX register dumps into the logs and reports TX busy errors. I provided all this information to Ayaz in real time, but never got any response or comment from him.

Even a soft reboot will not clear this failure. This initially lead me to conclude this is a hardware failure, but it isn't 100% certain to be the case. This is because the NIC is known to hang at boot time during the link negotiation, where no packets are ever transmitted. I didn't have time to fully understand this failure mode, but it could be that a soft reboot does clear the failure. And then at boot time link negotiation, it fails immediately, giving the appearance of a HW failure sustained across a soft reboot. I did not investigate enough to conclude with certainty it is a HW failure.

I did determine that a double hard reboot, where the second reboot is executed while the Netra is in the BIOS POST will always clear the NIC failure. This lead me to conclude with reasonable certainty this is a hardware failure that can occur at 100 Mb mode with a link mismatch. But I am not certain as stated above. Nvidia never did provide a resolution to this problem, despite the fact they were provided substantial information characterizing the failures and clear instructions on how to reproduce it within a few hours.

I've always known there may be a driver workaround for this failure. And if there is a driver workaround it would likely be related to interrupts. So, that was my motivation to ask the original question here. In the future, I will likely just dump all the data into bugzilla, as it seems like the preferred response to such a set of circumtances.

Thanks,
Karen

Quoted 43 lines

Quoted 43 lines
-----Original Message-----
From: Andrew Morton [mailto:[email protected]]
Sent: Friday, June 06, 2008 2:11 PM
To: Ayaz Abdulla
Cc: [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH] forcedeth: msi interrupts
Quoted 7 lines · On Fri, 06 Jun 2008 14:04:05 -0400 Ayaz Abdulla <[email protected]> wrote:
Quoted 4 lines · Andrew Morton
On Fri, 06 Jun 2008 13:15:32 -0400
Ayaz Abdulla <[email protected]> wrote:
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 4 lines · Andrew Morton
Quoted 1 line · On Tue, 03 Jun 2008 16:51:46 -0400 Ayaz Abdulla <[email protected]> wrote:
This patch adds a workaround for lost MSI interrupts. There is a
race
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
condition in the HW in which future interrupts could be missed.
The
Quoted 11 lines
Quoted 10 lines
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
workaround is to toggle the MSI irq mask.
Do you think this is a 2.6.26 thing?
It is by HW design, not related to the kernel.
Sorry, what I meant was: do you believe that this patch should be in 2.6.26?
Yes, it should be treated as a critical fix.
So should it also be backported into 2.6.25.x?
Bear in mind that $major_distros are apparently basing product on 2.6.25.

Signature · 3 lines

 Karen Shaeffer
 Neuralscape, Palo Alto, Ca. 94306
 [email protected]  http://www.neuralscape.com

Ayaz Abdulla

author

2008-06-07 19:29 UTC #10 lore raw

Karen,

Is the switch in forced mode? That would explain the mismatch. I can look into a fix to workaround the hang.

Please open a bugzilla bug as you recommend. Emails get deleted after a couple of weeks and it is better to have a permanent location to track issues like this.

Thanks,
Ayaz

-----Original Message-----
From: Karen Shaeffer [mailto:[email protected]]
Sent: Saturday, June 07, 2008 11:31 AM
To: Ayaz Abdulla
Cc: Andrew Morton; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH] forcedeth: msi interrupts

Quoted 5 lines · Karen Shaeffer

Quoted 1 line · Ayaz Abdulla
Yes, that would be great!
Hi Ayaz,
How far back should it be back ported? Do you know?

The Nvidia NIC that is integrated into the Nvidia 2200 chip, has a failure mode for the following linux kernels 2.6.21.x 2.6.23.x 2.6.24.x RHEL 2.6.18*

The NIC will hang under specific conditions for all these kernels. First, you must run the NIC in 100 Mb mode with autoneg enabled, then it will always link in a mismatch with the switch.
The switch will link at 100 Mb full duplex, while the Nvidia NIC will link at 100 Mb half duplex. This was shown with both Cisco managed switches and HP managed switches, and I suspect it will happen with any switch. (You can force the NIC to 100 Mb full, but autoneg will always result in the link mismatch.)

Thanks,
Karen

Quoted 21 lines

Quoted 21 lines
-----Original Message-----
From: Andrew Morton [mailto:[email protected]]
Sent: Friday, June 06, 2008 2:11 PM
To: Ayaz Abdulla
Cc: [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH] forcedeth: msi interrupts
Quoted 9 lines · On Fri, 06 Jun 2008 14:04:05 -0400 Ayaz Abdulla <[email protected]> wrote:
Quoted 6 lines · Andrew Morton
Quoted 2 lines · On Fri, 06 Jun 2008 13:15:32 -0400 Ayaz Abdulla <[email protected]> wrote:
Andrew Morton wrote:

Quoted 7 lines

Quoted 7 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 2 lines · On Tue, 03 Jun 2008 16:51:46 -0400 Ayaz Abdulla <[email protected]> wrote:
This patch adds a workaround for lost MSI interrupts. There is a
race
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
condition in the HW in which future interrupts could be

missed.

Quoted 9 lines

Quoted 9 lines
The
Quoted 8 lines
Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
workaround is to toggle the MSI irq mask.
Do you think this is a 2.6.26 thing?
It is by HW design, not related to the kernel.

Quoted 8 lines

Quoted 8 lines
Quoted 3 lines
Quoted 2 lines
Sorry, what I meant was: do you believe that this patch should be in 2.6.26?
Yes, it should be treated as a critical fix.
So should it also be backported into 2.6.25.x?
Bear in mind that $major_distros are apparently basing product on 2.6.25.

--
Karen Shaeffer
Neuralscape, Palo Alto, Ca. 94306
[email protected] http://www.neuralscape.com
-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

Karen Shaeffer

2008-06-08 01:34 UTC #11 lore raw

Quoted 11 lines · Ayaz Abdulla

Karen,
Is the switch in forced mode? That would explain the mismatch. I can look into a fix to workaround the hang.
Please open a bugzilla bug as you recommend. Emails get deleted after a couple of weeks and it is better to have a permanent location to track issues like this.
Thanks,
Ayaz

Hi Ayaz,
Thank you for responding.
No, the switch vlan port is configured for 100 Mb autoneg on. The NIC is configured with a boot time script using ethtool for 100 Mb autoneg on. The result is always the same. The switch port links at 100 Mb full duplex, and the NIC links at 100 Mb half duplex.

One can configure the NIC for 100 Mb autoneg off full duplex. And then the link mismatch does not occur. And I have never seen the failure in this case. But end users in the field sometimes just don't pay attention and end up with the mismatch, so the failure was first discovered by an end user.

At 1000 Mb, the link mismatch never happens. I've only seen this failure at 100 Mb.

The bug has been opened with ID 10885. I don't work with that project anymore, but I'll definitely follow up with the appropriate folks, when this issue is resolved. I am sure they will be very interested in a resolution.

Thanks,
Karen

Signature · 3 lines

 Karen Shaeffer
 Neuralscape, Palo Alto, Ca. 94306
 [email protected]  http://www.neuralscape.com

Karen Shaeffer

2008-06-08 02:01 UTC #12 lore raw

Quoted 26 lines · Karen Shaeffer

Quoted 11 lines · Ayaz Abdulla
Karen,
Is the switch in forced mode? That would explain the mismatch. I can look into a fix to workaround the hang.
Please open a bugzilla bug as you recommend. Emails get deleted after a couple of weeks and it is better to have a permanent location to track issues like this.
Thanks,
Ayaz
Hi Ayaz,
Thank you for responding.
No, the switch vlan port is configured for 100 Mb autoneg on. The NIC is configured with a boot time script using ethtool for 100 Mb autoneg on. The result is always the same. The switch port links at 100 Mb full duplex, and the NIC links at 100 Mb half duplex.
One can configure the NIC for 100 Mb autoneg off full duplex. And then the link mismatch does not occur. And I have never seen the failure in this case. But end users in the field sometimes just don't pay attention and end up with the mismatch, so the failure was first discovered by an end user.

Quoted 6 lines

At 1000 Mb, the link mismatch never happens. I've only seen this failure at 100 Mb.
The bug has been opened with ID 10885. I don't work with that project anymore, but I'll definitely follow up with the appropriate folks, when this issue is resolved. I am sure they will be very interested in a resolution.

Hi,
One more detail. I personally reproduced the NIC mismatch and the NIC TX failure many times in the lab on 5 different Netra servers, using quite a few different kernel.org and RHEL kernels. It happened on every kernel I tested, with 32 and 64 bit compiles. And it was produced in a data center using the 2.6.22.10 kernel several times completely independent of me. For more details, please see the bug ID 10885.

And I need to clarify that the last kernel I tested this on was actually linux-2.6.24-rc8-git6. I mistated in the bug that I observed this failure on the 2.6.25.4 kernel. That is inaccurate. I don't know, if it exist in the 2.6.25.4 kernel, because I never tested that kernel. My error.

Thanks,
Karen

Signature · 3 lines

 Karen Shaeffer
 Neuralscape, Palo Alto, Ca. 94306
 [email protected]  http://www.neuralscape.com

Ayaz Abdulla

author

2008-06-07 19:24 UTC #13 lore raw

In reply to Karen Shaeffer

Karen,

It is a bug fix that affects MSI interrupts. Any kernel that is accepting critical fixes should add the patch.

Thanks,
Ayaz

-----Original Message-----
From: Karen Shaeffer [mailto:[email protected]]
Sent: Friday, June 06, 2008 2:40 PM
To: Ayaz Abdulla
Cc: Andrew Morton; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH] forcedeth: msi interrupts

Quoted 1 line · Ayaz Abdulla

Yes, that would be great!

Hi Ayaz,
How far back should it be back ported? Do you know? Thanks,
Karen

Quoted 22 lines

-----Original Message-----
From: Andrew Morton [mailto:[email protected]]
Sent: Friday, June 06, 2008 2:11 PM
To: Ayaz Abdulla
Cc: [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH] forcedeth: msi interrupts
Quoted 9 lines · On Fri, 06 Jun 2008 14:04:05 -0400 Ayaz Abdulla <[email protected]> wrote:
Quoted 6 lines · Andrew Morton
Quoted 2 lines · On Fri, 06 Jun 2008 13:15:32 -0400 Ayaz Abdulla <[email protected]> wrote:
Andrew Morton wrote:

Quoted 8 lines

Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 3 lines · On Tue, 03 Jun 2008 16:51:46 -0400 Ayaz Abdulla <[email protected]> wrote:
This patch adds a workaround for lost MSI interrupts. There is
a
race
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
condition in the HW in which future interrupts could be

missed.

Quoted 29 lines

The
Quoted 11 lines
Quoted 10 lines
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
workaround is to toggle the MSI irq mask.
Do you think this is a 2.6.26 thing?
It is by HW design, not related to the kernel.
Sorry, what I meant was: do you believe that this patch should be in 2.6.26?
Yes, it should be treated as a critical fix.
So should it also be backported into 2.6.25.x?
Bear in mind that $major_distros are apparently basing product on 2.6.25.
----------------------------------------------------------------------
------------- This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message.
----------------------------------------------------------------------
-------------
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected] More majordomo info

Quoted 1 line

at http://vger.kernel.org/majordomo-info.html

---end quoted text---

--
Karen Shaeffer
Neuralscape, Palo Alto, Ca. 94306 [email protected] http://www.neuralscape.com

Ayaz Abdulla

author

2008-06-06 21:03 UTC #14 lore raw

PATCH 1/1 forcedeth: msi interrupts

In reply to Ayaz Abdulla

This patch adds a workaround for lost MSI interrupts. There is a race condition in the HW in which future interrupts could be missed. The workaround is to toggle the MSI irq mask.

Added cleanup based on comments from Andrew Morton.

Signed-off-by Ayaz Abdulla <[email protected]>

drivers/net/forcedeth.c +20 -0

@@ -3277,6 +3277,20 @@
 	dprintk(KERN_DEBUG "%s: link change notification done.\n", dev->name);
 }
 
+static void nv_msi_workaround(struct fe_priv *np)
+{
+
+	/* Need to toggle the msi irq mask within the ethernet device,
+	 * otherwise, future interrupts will not be detected.
+	 */
+	if (np->msi_flags & NV_MSI_ENABLED) {
+		u8 __iomem *base = np->base;
+
+		writel(0, base + NvRegMSIIrqMask);
+		writel(NVREG_MSI_VECTOR_0_ENABLED, base + NvRegMSIIrqMask);
+	}
+}
+
 static irqreturn_t nv_nic_irq(int foo, void *data)
 {
 	struct net_device *dev = (struct net_device *) data;

@@ -3299,6 +3313,8 @@
 		if (!(events & np->irqmask))
 			break;
 
+		nv_msi_workaround(np);
+
 		spin_lock(&np->lock);
 		nv_tx_done(dev);
 		spin_unlock(&np->lock);

@@ -3414,6 +3430,8 @@
 		if (!(events & np->irqmask))
 			break;
 
+		nv_msi_workaround(np);
+
 		spin_lock(&np->lock);
 		nv_tx_done_optimized(dev, TX_WORK_PER_LOOP);
 		spin_unlock(&np->lock);

@@ -3754,6 +3772,8 @@
 	if (!(events & NVREG_IRQ_TIMER))
 		return IRQ_RETVAL(0);
 
+	nv_msi_workaround(np);
+
 	spin_lock(&np->lock);
 	np->intr_test = 1;
 	spin_unlock(&np->lock);