[net 0/2][pull request] Intel Wired LAN Driver Updates

All of lore.kernel.org
 help / color / mirror / Atom feed

* [net 0/2][pull request] Intel Wired LAN Driver Updates
@ 2012-04-25  5:55 Jeff Kirsher
  2012-04-25  5:55 ` [net 1/2] e1000e: MSI interrupt test failed, using legacy interrupt Jeff Kirsher
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Jeff Kirsher @ 2012-04-25  5:55 UTC (permalink / raw)
  To: davem; +Cc: Jeff Kirsher, netdev, gospo, sassmann

This series of patches contains fixes for e1000e only.

The following are changes since commit 2a5809499e35b53a6044fd34e72b242688b7a862:
  asix: Fix tx transfer padding for full-speed USB
and are available in the git repository at:
  git://git.kernel.org/pub/scm/linux/kernel/git/jkirsher/net master

Jeff Kirsher (1):
  e1000e: Fix default interrupt throttle rate not set in NIC HW

Prasanna S Panchamukhi (1):
  e1000e: MSI interrupt test failed, using legacy interrupt

 drivers/net/ethernet/intel/e1000e/netdev.c |    2 +-
 drivers/net/ethernet/intel/e1000e/param.c  |   99 +++++++++++++++-------------
 2 files changed, 54 insertions(+), 47 deletions(-)

-- 
1.7.7.6

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [net 1/2] e1000e: MSI interrupt test failed, using legacy interrupt
  2012-04-25  5:55 [net 0/2][pull request] Intel Wired LAN Driver Updates Jeff Kirsher
@ 2012-04-25  5:55 ` Jeff Kirsher
  2012-04-25 15:59   ` Ben Hutchings
  2012-04-25  5:55 ` [net 2/2] e1000e: Fix default interrupt throttle rate not set in NIC HW Jeff Kirsher
  2012-04-26  9:11 ` [net 0/2][pull request] Intel Wired LAN Driver Updates David Miller
  2 siblings, 1 reply; 6+ messages in thread
From: Jeff Kirsher @ 2012-04-25  5:55 UTC (permalink / raw)
  To: davem; +Cc: Prasanna S Panchamukhi, netdev, gospo, sassmann, Jeff Kirsher

From: Prasanna S Panchamukhi <ppanchamukhi@riverbed.com>

Following logs where seen on Systems with multiple NICs,
while using MSI interrupts as shown below:

Feb 16 15:09:32 (none) user.notice kernel: 0000:00:0d.0: lan0_0: NIC Link is Up
1000 Mbps Full Duplex, Flow Control: RX/TX
Feb 16 15:09:32 (none) user.notice kernel: 0000:40:0d.0: wan0_1: NIC Link is Up
1000 Mbps Full Duplex, Flow Control: RX/TX
Feb 16 15:09:32 (none) user.notice kernel: 0000:40:0d.0: lan0_1: NIC Link is Up
1000 Mbps Full Duplex, Flow Control: RX/TX
Feb 16 15:09:32 (none) user.warn kernel: 0000:40:0e.0: wan4_0: MSI interrupt
test failed, using legacy interrupt.
Feb 16 15:09:32 (none) user.notice kernel: 0000:00:0e.0: wan1_0: NIC Link is Up
1000 Mbps Full Duplex, Flow Control: RX/TX
Feb 16 15:09:33 (none) user.notice kernel: 0000:00:0e.0: lan1_0: NIC Link is Up
1000 Mbps Full Duplex, Flow Control: RX/TX
Feb 16 15:09:33 (none) user.notice kernel: 0000:00:0f.0: wan2_0: NIC Link is Up
1000 Mbps Full Duplex, Flow Control: RX/TX
Feb 16 15:09:33 (none) user.notice kernel: 0000:00:0f.0: lan2_0: NIC Link is Up
1000 Mbps Full Duplex, Flow Control: RX/TX
Feb 16 15:09:33 (none) user.notice kernel: 0000:40:0a.0: wan3_0: NIC Link is Up
1000 Mbps Full Duplex, Flow Control: RX/TX
Feb 16 15:09:33 (none) user.notice kernel: 0000:40:0a.0: lan3_0: NIC Link is Up
1000 Mbps Full Duplex, Flow Control: RX/TX
Feb 16 15:09:34 (none) user.notice kernel: 0000:40:0e.0: lan4_0: NIC Link is Up
1000 Mbps Full Duplex, Flow Control: RX/TX
Feb 16 15:09:34 (none) user.notice kernel: 0000:40:0f.0: wan5_0: NIC Link is Up
1000 Mbps Full Duplex, Flow Control: RX/TX
Feb 16 15:09:34 (none) user.notice kernel: 0000:40:0f.0: lan5_0: NIC Link is Up
1000 Mbps Full Duplex, Flow Control: RX/TX

This patch fixes this problem by increasing the msleep from 50 to 100.

Signed-off-by: Prasanna S Panchamukhi <ppanchamukhi@riverbed.com>
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/net/ethernet/intel/e1000e/netdev.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index 19ab215..9520a6a 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -3799,7 +3799,7 @@ static int e1000_test_msi_interrupt(struct e1000_adapter *adapter)
 	/* fire an unusual interrupt on the test handler */
 	ew32(ICS, E1000_ICS_RXSEQ);
 	e1e_flush();
-	msleep(50);
+	msleep(100);

 	e1000_irq_disable(adapter);

-- 
1.7.7.6

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [net 1/2] e1000e: MSI interrupt test failed, using legacy interrupt
  2012-04-25  5:55 ` [net 1/2] e1000e: MSI interrupt test failed, using legacy interrupt Jeff Kirsher
@ 2012-04-25 15:59   ` Ben Hutchings
  2012-04-25 22:46     ` Prasanna Panchamukhi
  0 siblings, 1 reply; 6+ messages in thread
From: Ben Hutchings @ 2012-04-25 15:59 UTC (permalink / raw)
  To: Jeff Kirsher; +Cc: davem, Prasanna S Panchamukhi, netdev, gospo, sassmann

On Tue, 2012-04-24 at 22:55 -0700, Jeff Kirsher wrote:
> From: Prasanna S Panchamukhi <ppanchamukhi@riverbed.com>
> 
> Following logs where seen on Systems with multiple NICs,
> while using MSI interrupts as shown below:
[...]
> This patch fixes this problem by increasing the msleep from 50 to 100.
[...]

It probably doesn't, in general.  The out-of-tree version of the sfc
driver used to perform an interrupt test during probe, and after fixing
it up repeatedly I eventually had to remove it completely as there are
so many ways to get a false negative.  Also see the related commit
93e5dfa59b0e26a145a8adce5c9edf50bdaef4c7 ('sfc: Raise self-test
timeouts').

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [net 1/2] e1000e: MSI interrupt test failed, using legacy interrupt
  2012-04-25 15:59   ` Ben Hutchings
@ 2012-04-25 22:46     ` Prasanna Panchamukhi
  0 siblings, 0 replies; 6+ messages in thread
From: Prasanna Panchamukhi @ 2012-04-25 22:46 UTC (permalink / raw)
  To: Ben Hutchings; +Cc: Jeff Kirsher, davem, netdev, gospo, sassmann

On 04/25/2012 08:59 AM, Ben Hutchings wrote:
> On Tue, 2012-04-24 at 22:55 -0700, Jeff Kirsher wrote:
>> From: Prasanna S Panchamukhi<ppanchamukhi@riverbed.com>
>>
>> Following logs where seen on Systems with multiple NICs,
>> while using MSI interrupts as shown below:
> [...]
>> This patch fixes this problem by increasing the msleep from 50 to 100.
> [...]
>
> It probably doesn't, in general.  The out-of-tree version of the sfc
> driver used to perform an interrupt test during probe, and after fixing
> it up repeatedly I eventually had to remove it completely as there are
> so many ways to get a false negative.  Also see the related commit
> 93e5dfa59b0e26a145a8adce5c9edf50bdaef4c7 ('sfc: Raise self-test
> timeouts').
>
Thanks Ben for your review.
In general Ben's approach looks good fix to avoid false negatives. To 
give a little
bit of background, this issues is seen mostly with Intel 82571 Dual/quad 
port Gigabit ethernet controller add-on NICs and not seen with other 
Intel ethernet controller NICs. If Ben suggested
approach is the best way to go, I will provide a patch  soon.

Thanks,
Prasanna

> Ben.
>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [net 2/2] e1000e: Fix default interrupt throttle rate not set in NIC HW
  2012-04-25  5:55 [net 0/2][pull request] Intel Wired LAN Driver Updates Jeff Kirsher
  2012-04-25  5:55 ` [net 1/2] e1000e: MSI interrupt test failed, using legacy interrupt Jeff Kirsher
@ 2012-04-25  5:55 ` Jeff Kirsher
  2012-04-26  9:11 ` [net 0/2][pull request] Intel Wired LAN Driver Updates David Miller
  2 siblings, 0 replies; 6+ messages in thread
From: Jeff Kirsher @ 2012-04-25  5:55 UTC (permalink / raw)
  To: davem
  Cc: Jeff Kirsher, netdev, gospo, sassmann, Ying Cai, David Decotigny,
	Jeff Kirsher

Based on the original patch from  Ying Cai <ycai@google.com>
This change ensures that the itr/itr_setting adjustment logic is used,
even for the default/compiled-in value.

Context:
  When we changed the default InterruptThrottleRate value from default
  (3 = dynamic mode) to 8000 for example, only adapter->itr_setting
  (which controls interrupt coalescing mode) was set to 8000, but
  adapter->itr (which controls the value set in NIC register) was not
  updated accordingly. So from ethtool, it seemed the interrupt
  throttling is enabled at 8000 intr/s, but the NIC actually was
  running in dynamic mode which has lower CPU efficiency especially
  when throughput is not high.

CC: Ying Cai <ycai@google.com>
CC: David Decotigny <david.decotigny@google.com>
Signed-off-by: Jeff Kirsher <jeffrey.kirsher@intel.com>
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
---
 drivers/net/ethernet/intel/e1000e/param.c |   99 +++++++++++++++-------------
 1 files changed, 53 insertions(+), 46 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/param.c b/drivers/net/ethernet/intel/e1000e/param.c
index ff796e4..16adeb9 100644
--- a/drivers/net/ethernet/intel/e1000e/param.c
+++ b/drivers/net/ethernet/intel/e1000e/param.c
@@ -106,7 +106,7 @@ E1000_PARAM(RxAbsIntDelay, "Receive Absolute Interrupt Delay");
 /*
  * Interrupt Throttle Rate (interrupts/sec)
  *
- * Valid Range: 100-100000 (0=off, 1=dynamic, 3=dynamic conservative)
+ * Valid Range: 100-100000 or one of: 0=off, 1=dynamic, 3=dynamic conservative
  */
 E1000_PARAM(InterruptThrottleRate, "Interrupt Throttling Rate");
 #define DEFAULT_ITR 3
@@ -344,53 +344,60 @@ void __devinit e1000e_check_options(struct e1000_adapter *adapter)
 
 		if (num_InterruptThrottleRate > bd) {
 			adapter->itr = InterruptThrottleRate[bd];
-			switch (adapter->itr) {
-			case 0:
-				e_info("%s turned off\n", opt.name);
-				break;
-			case 1:
-				e_info("%s set to dynamic mode\n", opt.name);
-				adapter->itr_setting = adapter->itr;
-				adapter->itr = 20000;
-				break;
-			case 3:
-				e_info("%s set to dynamic conservative mode\n",
-					opt.name);
-				adapter->itr_setting = adapter->itr;
-				adapter->itr = 20000;
-				break;
-			case 4:
-				e_info("%s set to simplified (2000-8000 ints) "
-				       "mode\n", opt.name);
-				adapter->itr_setting = 4;
-				break;
-			default:
-				/*
-				 * Save the setting, because the dynamic bits
-				 * change itr.
-				 */
-				if (e1000_validate_option(&adapter->itr, &opt,
-							  adapter) &&
-				    (adapter->itr == 3)) {
-					/*
-					 * In case of invalid user value,
-					 * default to conservative mode.
-					 */
-					adapter->itr_setting = adapter->itr;
-					adapter->itr = 20000;
-				} else {
-					/*
-					 * Clear the lower two bits because
-					 * they are used as control.
-					 */
-					adapter->itr_setting =
-						adapter->itr & ~3;
-				}
-				break;
-			}
+
+			/*
+			 * Make sure a message is printed for non-special
+			 * values.  And in case of an invalid option, display
+			 * warning, use default and got through itr/itr_setting
+			 * adjustment logic below
+			 */
+			if ((adapter->itr > 4) &&
+			    e1000_validate_option(&adapter->itr, &opt, adapter))
+				adapter->itr = opt.def;
 		} else {
-			adapter->itr_setting = opt.def;
+			/*
+			 * If no option specified, use default value and go
+			 * through the logic below to adjust itr/itr_setting
+			 */
+			adapter->itr = opt.def;
+
+			/*
+			 * Make sure a message is printed for non-special
+			 * default values
+			 */
+			if (adapter->itr > 40)
+				e_info("%s set to default %d\n", opt.name,
+				       adapter->itr);
+		}
+
+		adapter->itr_setting = adapter->itr;
+		switch (adapter->itr) {
+		case 0:
+			e_info("%s turned off\n", opt.name);
+			break;
+		case 1:
+			e_info("%s set to dynamic mode\n", opt.name);
+			adapter->itr = 20000;
+			break;
+		case 3:
+			e_info("%s set to dynamic conservative mode\n",
+			       opt.name);
 			adapter->itr = 20000;
+			break;
+		case 4:
+			e_info("%s set to simplified (2000-8000 ints) mode\n",
+			       opt.name);
+			break;
+		default:
+			/*
+			 * Save the setting, because the dynamic bits
+			 * change itr.
+			 *
+			 * Clear the lower two bits because
+			 * they are used as control.
+			 */
+			adapter->itr_setting &= ~3;
+			break;
 		}
 	}
 	{ /* Interrupt Mode */
-- 
1.7.7.6

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [net 0/2][pull request] Intel Wired LAN Driver Updates
  2012-04-25  5:55 [net 0/2][pull request] Intel Wired LAN Driver Updates Jeff Kirsher
  2012-04-25  5:55 ` [net 1/2] e1000e: MSI interrupt test failed, using legacy interrupt Jeff Kirsher
  2012-04-25  5:55 ` [net 2/2] e1000e: Fix default interrupt throttle rate not set in NIC HW Jeff Kirsher
@ 2012-04-26  9:11 ` David Miller
  2 siblings, 0 replies; 6+ messages in thread
From: David Miller @ 2012-04-26  9:11 UTC (permalink / raw)
  To: jeffrey.t.kirsher; +Cc: netdev, gospo, sassmann

From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Date: Tue, 24 Apr 2012 22:55:12 -0700

> This series of patches contains fixes for e1000e only.
> 
> The following are changes since commit 2a5809499e35b53a6044fd34e72b242688b7a862:
>   asix: Fix tx transfer padding for full-speed USB
> and are available in the git repository at:
>   git://git.kernel.org/pub/scm/linux/kernel/git/jkirsher/net master
> 
> Jeff Kirsher (1):
>   e1000e: Fix default interrupt throttle rate not set in NIC HW
> 
> Prasanna S Panchamukhi (1):
>   e1000e: MSI interrupt test failed, using legacy interrupt

Pulled, but longer term you guys might want to take Ben's suggestion
and just take the interrupt self-test out altogether.

Thanks.

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2012-04-26  9:11 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-04-25  5:55 [net 0/2][pull request] Intel Wired LAN Driver Updates Jeff Kirsher
2012-04-25  5:55 ` [net 1/2] e1000e: MSI interrupt test failed, using legacy interrupt Jeff Kirsher
2012-04-25 15:59   ` Ben Hutchings
2012-04-25 22:46     ` Prasanna Panchamukhi
2012-04-25  5:55 ` [net 2/2] e1000e: Fix default interrupt throttle rate not set in NIC HW Jeff Kirsher
2012-04-26  9:11 ` [net 0/2][pull request] Intel Wired LAN Driver Updates David Miller

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.