Skip to contentSkip to conversation

e1000e: enable/disable ASPM L0s and L1 and ERT according to hardware errata

PATCH Applied Jeff Kirsher 5 participants 8 messages Apr 27, 2010 – Apr 30, 2010

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Jeff Kirsher

author
#0 lore raw

PATCH 1/1 e1000e: enable/disable ASPM L0s and L1 and ERT according to hardware errata

From: Bruce Allan <[email protected]>

Prompted by a previous patch submitted by Matthew Garret <[email protected]>, further digging into errata documentation reveals the current enabling or disabling of ASPM L0s and L1 states for certain parts supported by this driver are incorrect. 82571 and 82572 should always disable L1. For standard frames, 82573/82574/82583 can enable L1 but L0s must be disabled, and for jumbo frames 82573/82574 must disable L1. This allows for some parts to enable L1 in certain configurations leading to better power savings.

Also according to the same errata, Early Receive (ERT) should be disabled on 82573 when using jumbo frames.

Cc Matthew Garret <[email protected]>
Signed-off-by Bruce Allan <[email protected]>
Signed-off-by Jeff Kirsher <[email protected]>

---

 drivers/net/e1000e/82571.c  |   20 ++++++------
 drivers/net/e1000e/e1000.h  |    5 ++-
 drivers/net/e1000e/netdev.c |   70 ++++++++++++++++++++++++++-----------------
 3 files changed, 55 insertions(+), 40 deletions(-)
drivers/net/e1000e/82571.c +9 -11
index 712ccc6..9015555 100644
@@ -336,7 +336,6 @@ static s32 e1000_get_variants_82571(struct e1000_adapter *adapter)
 	struct e1000_hw *hw = &adapter->hw;
 	static int global_quad_port_a; /* global port a indication */
 	struct pci_dev *pdev = adapter->pdev;
-	u16 eeprom_data = 0;
 	int is_port_b = er32(STATUS) & E1000_STATUS_FUNC_1;
 	s32 rc;
 

@@ -387,16 +386,15 @@ static s32 e1000_get_variants_82571(struct e1000_adapter *adapter)
 		if (pdev->device == E1000_DEV_ID_82571EB_SERDES_QUAD)
 			adapter->flags &= ~FLAG_HAS_WOL;
 		break;
-
 	case e1000_82573:
+	case e1000_82574:
+	case e1000_82583:
+		/* Disable ASPM L0s due to hardware errata */
+		e1000e_disable_aspm(adapter->pdev, PCIE_LINK_STATE_L0S);
+
 		if (pdev->device == E1000_DEV_ID_82573L) {
-			if (e1000_read_nvm(&adapter->hw, NVM_INIT_3GIO_3, 1,
-				       &eeprom_data) < 0)
-				break;
-			if (!(eeprom_data & NVM_WORD1A_ASPM_MASK)) {
-				adapter->flags |= FLAG_HAS_JUMBO_FRAMES;
-				adapter->max_hw_frame_size = DEFAULT_JUMBO;
-			}
+			adapter->flags |= FLAG_HAS_JUMBO_FRAMES;
+			adapter->max_hw_frame_size = DEFAULT_JUMBO;
 		}
 		break;
 	default:

@@ -1792,6 +1790,7 @@ struct e1000_info e1000_82571_info = {
 				  | FLAG_RESET_OVERWRITES_LAA /* errata */
 				  | FLAG_TARC_SPEED_MODE_BIT /* errata */
 				  | FLAG_APME_CHECK_PORT_B,
+	.flags2			= FLAG2_DISABLE_ASPM_L1, /* errata 13 */
 	.pba			= 38,
 	.max_hw_frame_size	= DEFAULT_JUMBO,
 	.get_variants		= e1000_get_variants_82571,

@@ -1809,6 +1808,7 @@ struct e1000_info e1000_82572_info = {
 				  | FLAG_RX_CSUM_ENABLED
 				  | FLAG_HAS_CTRLEXT_ON_LOAD
 				  | FLAG_TARC_SPEED_MODE_BIT, /* errata */
+	.flags2			= FLAG2_DISABLE_ASPM_L1, /* errata 13 */
 	.pba			= 38,
 	.max_hw_frame_size	= DEFAULT_JUMBO,
 	.get_variants		= e1000_get_variants_82571,

@@ -1820,13 +1820,11 @@ struct e1000_info e1000_82572_info = {
 struct e1000_info e1000_82573_info = {
 	.mac			= e1000_82573,
 	.flags			= FLAG_HAS_HW_VLAN_FILTER
-				  | FLAG_HAS_JUMBO_FRAMES
 				  | FLAG_HAS_WOL
 				  | FLAG_APME_IN_CTRL3
 				  | FLAG_RX_CSUM_ENABLED
 				  | FLAG_HAS_SMART_POWER_DOWN
 				  | FLAG_HAS_AMT
-				  | FLAG_HAS_ERT
 				  | FLAG_HAS_SWSM_ON_LOAD,
 	.pba			= 20,
 	.max_hw_frame_size	= ETH_FRAME_LEN + ETH_FCS_LEN,
drivers/net/e1000e/e1000.h +4 -1
index 118bdf4..ee32b9b 100644
@@ -37,6 +37,7 @@
 #include <linux/io.h>
 #include <linux/netdevice.h>
 #include <linux/pci.h>
+#include <linux/pci-aspm.h>
 
 #include "hw.h"
 

@@ -374,7 +375,7 @@ struct e1000_adapter {
 struct e1000_info {
 	enum e1000_mac_type	mac;
 	unsigned int		flags;
-	unsigned int            flags2;
+	unsigned int		flags2;
 	u32			pba;
 	u32			max_hw_frame_size;
 	s32			(*get_variants)(struct e1000_adapter *);

@@ -421,6 +422,7 @@ struct e1000_info {
 #define FLAG2_CRC_STRIPPING               (1 << 0)
 #define FLAG2_HAS_PHY_WAKEUP              (1 << 1)
 #define FLAG2_IS_DISCARDING               (1 << 2)
+#define FLAG2_DISABLE_ASPM_L1             (1 << 3)
 
 #define E1000_RX_DESC_PS(R, i)	    \
 	(&(((union e1000_rx_desc_packet_split *)((R).desc))[i]))

@@ -461,6 +463,7 @@ extern void e1000e_update_stats(struct e1000_adapter *adapter);
 extern bool e1000e_has_link(struct e1000_adapter *adapter);
 extern void e1000e_set_interrupt_capability(struct e1000_adapter *adapter);
 extern void e1000e_reset_interrupt_capability(struct e1000_adapter *adapter);
+extern void e1000e_disable_aspm(struct pci_dev *pdev, u16 state);
 
 extern unsigned int copybreak;
drivers/net/e1000e/netdev.c +42 -28

111 lines not on this page · raw message

#1 lore raw
Quoted 11 lines · Jeff Kirsher

From: Bruce Allan <[email protected]>

Prompted by a previous patch submitted by Matthew Garret <[email protected]>, further digging into errata documentation reveals the current enabling or disabling of ASPM L0s and L1 states for certain parts supported by this driver are incorrect. 82571 and 82572 should always disable L1. For standard frames, 82573/82574/82583 can enable L1 but L0s must be disabled, and for jumbo frames 82573/82574 must disable L1. This allows for some parts to enable L1 in certain configurations leading to better power savings.

Quoted 2 lines

Also according to the same errata, Early Receive (ERT) should be disabled on 82573 when using jumbo frames.

Looks good. Thanks for digging into this!

Signature · 1 line
Matthew Garrett | [email protected]
maintainer Applied
#2 lore raw
In reply to Jeff Kirsher

From: Jeff Kirsher <[email protected]>
Date: Tue, 27 Apr 2010 06:33:04 -0700

Quoted 11 lines

From: Bruce Allan <[email protected]>

Prompted by a previous patch submitted by Matthew Garret <[email protected]>, further digging into errata documentation reveals the current enabling or disabling of ASPM L0s and L1 states for certain parts supported by this driver are incorrect. 82571 and 82572 should always disable L1. For standard frames, 82573/82574/82583 can enable L1 but L0s must be disabled, and for jumbo frames 82573/82574 must disable L1. This allows for some parts to enable L1 in certain configurations leading to better power savings.

Quoted 6 lines

Also according to the same errata, Early Receive (ERT) should be disabled on 82573 when using jumbo frames.

Cc Matthew Garret <[email protected]>
Signed-off-by Bruce Allan <[email protected]>
Signed-off-by Jeff Kirsher <[email protected]>

Applied.

#3 lore raw
In reply to Jeff Kirsher

Hi,

Quoted 11 lines

From: Bruce Allan <[email protected]>

Prompted by a previous patch submitted by Matthew Garret <[email protected]>, further digging into errata documentation reveals the current enabling or disabling of ASPM L0s and L1 states for certain parts supported by this driver are incorrect. 82571 and 82572 should always disable L1. For standard frames, 82573/82574/82583 can enable L1 but L0s must be disabled, and for jumbo frames 82573/82574 must disable L1. This allows for some parts to enable L1 in certain configurations leading to better power savings.

Quoted 2 lines

Also according to the same errata, Early Receive (ERT) should be disabled on 82573 when using jumbo frames.

This oopses on one of my ppc64 boxes with a NULL pointer (0x4a):

Unable to handle kernel paging request for data at address 0x0000004a
Faulting instruction address: 0xc0000000004d2f1c
cpu 0xe: Vector: 300 (Data Access) at [c000000bec1833a0]
    pc: c0000000004d2f1c: .e1000e_disable_aspm+0xe0/0x150
    lr: c0000000004d2f0c: .e1000e_disable_aspm+0xd0/0x150

dar: 4a

[c000000bec1836d0] c00000000069b9d8 .e1000_probe+0x84/0xe8c [c000000bec1837b0] c000000000386d90 .local_pci_probe+0x4c/0x68 [c000000bec183840] c0000000003872ac .pci_device_probe+0xfc/0x148 [c000000bec183900] c000000000409e8c .driver_probe_device+0xe4/0x1d0 [c000000bec1839a0] c00000000040a024 .__driver_attach+0xac/0xf4 [c000000bec183a40] c000000000409124 .bus_for_each_dev+0x9c/0x10c [c000000bec183b00] c000000000409c1c .driver_attach+0x40/0x60 [c000000bec183b90] c0000000004085dc .bus_add_driver+0x150/0x328 [c000000bec183c40] c00000000040a58c .driver_register+0x100/0x1c4 [c000000bec183cf0] c00000000038764c .__pci_register_driver+0x78/0x128

Seems like pdev->bus->self == NULL. I haven't touched pci in a long time so I'm trying to remember what this means (no pcie bridge perhaps?)

The patch below fixes the oops for me.

Signed-off-by Anton Blanchard <[email protected]>

---

drivers/net/e1000e/netdev.c +3 -0
Index: linux-2.6.34-rc5/drivers/net/e1000e/netdev.c
===================================================================
@@ -4633,6 +4633,9 @@
 	reg16 &= ~state;
 	pci_write_config_word(pdev, pos + PCI_EXP_LNKCTL, reg16);
 
+	if (!pdev->bus->self)
+		return;
+
 	pos = pci_pcie_cap(pdev->bus->self);
 	pci_read_config_word(pdev->bus->self, pos + PCI_EXP_LNKCTL, &reg16);
 	reg16 &= ~state;
#4 lore raw
Quoted 34 lines · Anton Blanchard

This oopses on one of my ppc64 boxes with a NULL pointer (0x4a):

Unable to handle kernel paging request for data at address 0x0000004a
Faulting instruction address: 0xc0000000004d2f1c
cpu 0xe: Vector: 300 (Data Access) at [c000000bec1833a0]
    pc: c0000000004d2f1c: .e1000e_disable_aspm+0xe0/0x150
    lr: c0000000004d2f0c: .e1000e_disable_aspm+0xd0/0x150

dar: 4a

[c000000bec1836d0] c00000000069b9d8 .e1000_probe+0x84/0xe8c [c000000bec1837b0] c000000000386d90 .local_pci_probe+0x4c/0x68 [c000000bec183840] c0000000003872ac .pci_device_probe+0xfc/0x148 [c000000bec183900] c000000000409e8c .driver_probe_device+0xe4/0x1d0 [c000000bec1839a0] c00000000040a024 .__driver_attach+0xac/0xf4 [c000000bec183a40] c000000000409124 .bus_for_each_dev+0x9c/0x10c [c000000bec183b00] c000000000409c1c .driver_attach+0x40/0x60 [c000000bec183b90] c0000000004085dc .bus_add_driver+0x150/0x328 [c000000bec183c40] c00000000040a58c .driver_register+0x100/0x1c4 [c000000bec183cf0] c00000000038764c .__pci_register_driver+0x78/0x128

Seems like pdev->bus->self == NULL. I haven't touched pci in a long time
so I'm trying to remember what this means (no pcie bridge perhaps?)

The patch below fixes the oops for me.

Signed-off-by Anton Blanchard <[email protected]>

---

Index: linux-2.6.34-rc5/drivers/net/e1000e/netdev.c
===================================================================
--- linux-2.6.34-rc5.orig/drivers/net/e1000e/netdev.c 2010-04-29
00:10:58.000000000 -0500 +++
linux-2.6.34-rc5/drivers/net/e1000e/netdev.c 2010-04-29

drivers/net/e1000e/netdev.c line 4641

Quoted 9 lines
drivers/net/e1000e/netdev.c +3 -0
 	02:20:50.000000000 -0500 @@ -4633,6 +4633,9 @@ reg16 &= ~state;
 	pci_write_config_word(pdev, pos + PCI_EXP_LNKCTL, reg16);
 
+	if (!pdev->bus->self)
+		return;
+
 	pos = pci_pcie_cap(pdev->bus->self);
 	pci_read_config_word(pdev->bus->self, pos + PCI_EXP_LNKCTL, &reg16);
 	reg16 &= ~state;

Your patch is probably the correct thing to do but I'm not all that familiar with the ppc64 architecture. Would you please provide the output of 'lspci -t' and 'lspci -vvv -xxx'.

Thanks,
Bruce.

maintainer
#5 lore raw

From: "Allan, Bruce W" <[email protected]>
Date: Thu, 29 Apr 2010 10:19:56 -0700

Quoted 3 lines

Your patch is probably the correct thing to do but I'm not all that familiar with the ppc64 architecture. Would you please provide the output of 'lspci -t' and 'lspci -vvv -xxx'.

You're not guarenteed for there to be a pci_dev backing the top-level host controller, at the very least. Some platforms don't even implement the PCI config space for the host controller, whilst on others access to them is protected by the hypervisor.

So you can't go poking around the PCI host controller registers unconditionally.

The same OOPS probably would happen on Sparc64 in some configurations too. Although all of my PCI-E slots do have PCI-E express switch port nodes, so maybe it wouldn't trigger here.

Reviewed-by
#6 lore raw
Quoted 12 lines · David Miller

From: "Allan, Bruce W" <[email protected]>
Date: Thu, 29 Apr 2010 10:19:56 -0700

Quoted 3 lines

Your patch is probably the correct thing to do but I'm not all that familiar with the ppc64 architecture. Would you please provide the output of 'lspci -t' and 'lspci -vvv -xxx'.

You're not guarenteed for there to be a pci_dev backing the top-level host controller, at the very least. Some platforms don't even implement the PCI config space for the host controller, whilst on others access to them is protected by the hypervisor.

Quoted 6 lines

So you can't go poking around the PCI host controller registers unconditionally.

The same OOPS probably would happen on Sparc64 in some configurations too. Although all of my PCI-E slots do have PCI-E express switch port nodes, so maybe it wouldn't trigger here.

In that case, I agree with Anton's patch.

Reviewed-by Bruce Allan <[email protected]>

Thanks,
Bruce.