[net-next PATCH 1/2] PCI: Add PCI quirk to disable L0s ASPM state for 82575 and 82598

[net-next PATCH 1/2] PCI: Add PCI quirk to disable L0s ASPM state for 82575 and 82598

[Jeff Kirsher]

From: Alexander Duyck <alexander.h.duyck@intel.com>

This patch is intended to disable L0s ASPM link state for 82598 (ixgbe)
parts due to the fact that it is possible to corrupt TX data when coming
back out of L0s on some systems.  The workaround had been added for 82575
(igb) previously, but did not use the ASPM api.  This quirk uses the ASPM
api to prevent the ASPM subsystem from re-enabling the L0s state.

Instead of adding the fix in igb to the ixgbe driver as well it was
decided to move it into a pci quirk.  It is necessary to move the fix out
of the driver and into a pci quirk in order to prevent the issue from
occuring prior to driver load to handle the possibility of the device being
passed to a VM via direct assignment.

Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
CC: linux-pci <linux-pci@vger.kernel.org>
CC: Matthew Wilcox <willy@linux.intel.com>
CC: Jesse Barnes <jbarnes@virtuousgeek.org>
---

 drivers/pci/quirks.c |   25 +++++++++++++++++++++++++
 1 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index f20d553..1a7c48d 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -23,6 +23,7 @@
 #include <linux/acpi.h>
 #include <linux/kallsyms.h>
 #include <linux/dmi.h>
+#include <linux/pci-aspm.h>
 #include "pci.h"
 
 int isa_dma_bridge_buggy;
@@ -1749,6 +1750,30 @@ static void __devinit quirk_e100_interrupt(struct pci_dev *dev)
 }
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_ANY_ID, quirk_e100_interrupt);
 
+/*
+ * The 82575 and 82598 may experience data corruption issues when transitioning
+ * out of L0S.  To prevent this we need to disable L0S on the pci-e link
+ */
+static void __devinit quirk_disable_aspm_l0s(struct pci_dev *dev)
+{
+	dev_info(&dev->dev, "Disabling L0s\n");
+	pci_disable_link_state(dev, PCIE_LINK_STATE_L0S);
+}
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x10a7, quirk_disable_aspm_l0s);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x10a9, quirk_disable_aspm_l0s);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x10b6, quirk_disable_aspm_l0s);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x10c6, quirk_disable_aspm_l0s);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x10c7, quirk_disable_aspm_l0s);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x10c8, quirk_disable_aspm_l0s);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x10d6, quirk_disable_aspm_l0s);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x10db, quirk_disable_aspm_l0s);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x10dd, quirk_disable_aspm_l0s);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x10e1, quirk_disable_aspm_l0s);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x10ec, quirk_disable_aspm_l0s);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x10f1, quirk_disable_aspm_l0s);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x10f4, quirk_disable_aspm_l0s);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1508, quirk_disable_aspm_l0s);
+
 static void __devinit fixup_rev1_53c810(struct pci_dev* dev)
 {
 	/* rev 1 ncr53c810 chips don't set the class at all which means

[net-next PATCH 2/2] igb: remove ASPM L0s workaround

[Jeff Kirsher]

From: Alexander Duyck <alexander.h.duyck@intel.com>

The L0s workaround should be moved into a pci quirk and so it is not
necessary in the driver.  This update removes the L0s workaround from the
igb driver.

Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---

 drivers/net/igb/igb_main.c |   26 ++------------------------
 1 files changed, 2 insertions(+), 24 deletions(-)

diff --git a/drivers/net/igb/igb_main.c b/drivers/net/igb/igb_main.c
index 78558f8..1adc7d2 100644
--- a/drivers/net/igb/igb_main.c
+++ b/drivers/net/igb/igb_main.c
@@ -1126,11 +1126,10 @@ static int __devinit igb_probe(struct pci_dev *pdev,
 	struct net_device *netdev;
 	struct igb_adapter *adapter;
 	struct e1000_hw *hw;
-	struct pci_dev *us_dev;
 	const struct e1000_info *ei = igb_info_tbl[ent->driver_data];
 	unsigned long mmio_start, mmio_len;
-	int err, pci_using_dac, pos;
-	u16 eeprom_data = 0, state = 0;
+	int err, pci_using_dac;
+	u16 eeprom_data = 0;
 	u16 eeprom_apme_mask = IGB_EEPROM_APME;
 	u32 part_num;
 
@@ -1156,27 +1155,6 @@ static int __devinit igb_probe(struct pci_dev *pdev,
 		}
 	}
 
-	/* 82575 requires that the pci-e link partner disable the L0s state */
-	switch (pdev->device) {
-	case E1000_DEV_ID_82575EB_COPPER:
-	case E1000_DEV_ID_82575EB_FIBER_SERDES:
-	case E1000_DEV_ID_82575GB_QUAD_COPPER:
-		us_dev = pdev->bus->self;
-		pos = pci_find_capability(us_dev, PCI_CAP_ID_EXP);
-		if (pos) {
-			pci_read_config_word(us_dev, pos + PCI_EXP_LNKCTL,
-			                     &state);
-			state &= ~PCIE_LINK_STATE_L0S;
-			pci_write_config_word(us_dev, pos + PCI_EXP_LNKCTL,
-			                      state);
-			dev_info(&pdev->dev,
-				 "Disabling ASPM L0s upstream switch port %s\n",
-				 pci_name(us_dev));
-		}
-	default:
-		break;
-	}
-
 	err = pci_request_selected_regions(pdev, pci_select_bars(pdev,
 	                                   IORESOURCE_MEM),
 	                                   igb_driver_name);

Re: [net-next PATCH 1/2] PCI: Add PCI quirk to disable L0s ASPM state for 82575 and 82598

[Matthew Wilcox]

On Tue, Mar 03, 2009 at 06:03:09PM -0800, Jeff Kirsher wrote:
> From: Alexander Duyck <alexander.h.duyck@intel.com>
> 
> This patch is intended to disable L0s ASPM link state for 82598 (ixgbe)
> parts due to the fact that it is possible to corrupt TX data when coming
> back out of L0s on some systems.  The workaround had been added for 82575
> (igb) previously, but did not use the ASPM api.  This quirk uses the ASPM
> api to prevent the ASPM subsystem from re-enabling the L0s state.
> 
> Instead of adding the fix in igb to the ixgbe driver as well it was
> decided to move it into a pci quirk.  It is necessary to move the fix out
> of the driver and into a pci quirk in order to prevent the issue from
> occuring prior to driver load to handle the possibility of the device being
> passed to a VM via direct assignment.

Thanks for the explanation, it handles my immediate question of "why do
it in a quirk?"  It does, however, raise the very interesting question
about what to do about other devices which have issues that are currently
handled in the driver that can't be handled by the driver in a VM.  
I don't have a good example to hand right now, but I bet I can spot one
when I'm less tired.

> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> CC: linux-pci <linux-pci@vger.kernel.org>
> CC: Matthew Wilcox <willy@linux.intel.com>
> CC: Jesse Barnes <jbarnes@virtuousgeek.org>

I'm not going to ack this patch at this point, let's just give it a day.
FWIW, Jesse is not going to be able to review patches this week.

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [net-next PATCH 1/2] PCI: Add PCI quirk to disable L0s ASPM state for 82575 and 82598

[David Miller]

From: Matthew Wilcox <matthew@wil.cx>
Date: Tue, 3 Mar 2009 21:05:26 -0700

> I'm not going to ack this patch at this point, let's just give it a day.

More than a day has passed, how long are we going to let this
patch sit and rot?

Do we have to wait until Jesse gets back?  That doesn't sound
reasonable to me.

Re: [net-next PATCH 1/2] PCI: Add PCI quirk to disable L0s ASPM state for 82575 and 82598

[Matthew Wilcox]

On Thu, Mar 05, 2009 at 04:31:08PM -0800, David Miller wrote:
> From: Matthew Wilcox <matthew@wil.cx>
> Date: Tue, 3 Mar 2009 21:05:26 -0700
> 
> > I'm not going to ack this patch at this point, let's just give it a day.
> 
> More than a day has passed, how long are we going to let this
> patch sit and rot?
> 
> Do we have to wait until Jesse gets back?  That doesn't sound
> reasonable to me.

A lot of the discussion on this happened face-to-face; Jeff, Linus and I
were all at the same conference this week.  I have put it in a pci fixes
tree, which I now have to pull apart again because one of the earlier
patches needs to be dropped.  I'm getting on a plane tonight and flying
home, so I'll be pushing this fix to Linus tomorrow.

Sorry for not keeping you in the loop on this one.

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [net-next PATCH 1/2] PCI: Add PCI quirk to disable L0s ASPM state for 82575 and 82598

[David Miller]

From: Matthew Wilcox <matthew@wil.cx>
Date: Thu, 5 Mar 2009 18:02:07 -0700

> On Thu, Mar 05, 2009 at 04:31:08PM -0800, David Miller wrote:
> > From: Matthew Wilcox <matthew@wil.cx>
> > Date: Tue, 3 Mar 2009 21:05:26 -0700
> > 
> > > I'm not going to ack this patch at this point, let's just give it a day.
> > 
> > More than a day has passed, how long are we going to let this
> > patch sit and rot?
> > 
> > Do we have to wait until Jesse gets back?  That doesn't sound
> > reasonable to me.
> 
> A lot of the discussion on this happened face-to-face; Jeff, Linus and I
> were all at the same conference this week.  I have put it in a pci fixes
> tree, which I now have to pull apart again because one of the earlier
> patches needs to be dropped.  I'm getting on a plane tonight and flying
> home, so I'll be pushing this fix to Linus tomorrow.
> 
> Sorry for not keeping you in the loop on this one.

Ok, sounds good to me.