[net-next 0/5][pull request] 1GbE Intel Wired LAN Driver Updates 2016-09-22

[net-next 0/5][pull request] 1GbE Intel Wired LAN Driver Updates 2016-09-22

[Jeff Kirsher]

This series contains updates to igb, igbvf and PCI quirks.

Wei Yongjun makes a function static to shut up sparse.

Todd bumps the igb and igbvf version, which is long overdue.

Jake fixes an issue where the PPS SYS_WRAP interrupt was not re-enabled
after a reset, which resulted in disabling of the PPS signaling.

Sasha adds a quirk for 82579 devices, which was reported by Red Hat.
The issue is that 82579 has a problem reattaching itself after being
detached, so disable the FLR capability in PCIe configuration space.

The following are changes since commit cdd0766d7da19085e88df86d1e5e21d9fe3d374f:
  Merge branch 'ftgmac100-ast2500-support'
and are available in the git repository at:
  git://git.kernel.org/pub/scm/linux/kernel/git/jkirsher/next-queue 1GbE

Jacob Keller (1):
  igb: restore PPS signal on igb_ptp_reset

Sasha Neftin (1):
  PCI: disable FLR for 82579 device

Todd Fujinaka (2):
  igbvf: bump version to igbvf-2.4.0
  igb: bump version to igb-5.4.0

Wei Yongjun (1):
  igb: fix non static symbol warning

 drivers/net/ethernet/intel/igb/igb.h         |  1 +
 drivers/net/ethernet/intel/igb/igb_ethtool.c |  4 ++--
 drivers/net/ethernet/intel/igb/igb_main.c    |  2 +-
 drivers/net/ethernet/intel/igb/igb_ptp.c     |  5 ++++-
 drivers/net/ethernet/intel/igbvf/netdev.c    |  2 +-
 drivers/pci/quirks.c                         | 21 +++++++++++++++++++++
 6 files changed, 30 insertions(+), 5 deletions(-)

-- 
2.7.4

[net-next 1/5] igb: fix non static symbol warning

[Jeff Kirsher]

From: Wei Yongjun <weiyongjun1@huawei.com>

Fixes the following sparse warning:

drivers/net/ethernet/intel/igb/igb_ethtool.c:2707:5: warning:
 symbol 'igb_rxnfc_write_vlan_prio_filter' was not declared. Should it be static?

Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/net/ethernet/intel/igb/igb_ethtool.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c b/drivers/net/ethernet/intel/igb/igb_ethtool.c
index 0c33eca..737b664 100644
--- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
+++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
@@ -2704,8 +2704,8 @@ static int igb_rxnfc_write_etype_filter(struct igb_adapter *adapter,
 	return 0;
 }
 
-int igb_rxnfc_write_vlan_prio_filter(struct igb_adapter *adapter,
-				     struct igb_nfc_filter *input)
+static int igb_rxnfc_write_vlan_prio_filter(struct igb_adapter *adapter,
+					    struct igb_nfc_filter *input)
 {
 	struct e1000_hw *hw = &adapter->hw;
 	u8 vlan_priority;
-- 
2.7.4

[net-next 2/5] igbvf: bump version to igbvf-2.4.0

[Jeff Kirsher]

From: Todd Fujinaka <todd.fujinaka@intel.com>

Bump version to match other igbvf drivers.

Signed-off-by: Todd Fujinaka <todd.fujinaka@intel.com>
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/net/ethernet/intel/igbvf/netdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/igbvf/netdev.c b/drivers/net/ethernet/intel/igbvf/netdev.c
index b0778ba..12bb877 100644
--- a/drivers/net/ethernet/intel/igbvf/netdev.c
+++ b/drivers/net/ethernet/intel/igbvf/netdev.c
@@ -47,7 +47,7 @@
 
 #include "igbvf.h"
 
-#define DRV_VERSION "2.0.2-k"
+#define DRV_VERSION "2.4.0-k"
 char igbvf_driver_name[] = "igbvf";
 const char igbvf_driver_version[] = DRV_VERSION;
 static const char igbvf_driver_string[] =
-- 
2.7.4

[net-next 3/5] igb: bump version to igb-5.4.0

[Jeff Kirsher]

From: Todd Fujinaka <todd.fujinaka@intel.com>

Bump igb version to match other igb drivers.

Signed-off-by: Todd Fujinaka <todd.fujinaka@intel.com>
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/net/ethernet/intel/igb/igb_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index af75eac..e120d9b 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -58,7 +58,7 @@
 #include "igb.h"
 
 #define MAJ 5
-#define MIN 3
+#define MIN 4
 #define BUILD 0
 #define DRV_VERSION __stringify(MAJ) "." __stringify(MIN) "." \
 __stringify(BUILD) "-k"
-- 
2.7.4

[net-next 4/5] igb: restore PPS signal on igb_ptp_reset

[Jeff Kirsher]

From: Jacob Keller <jacob.e.keller@intel.com>

When a reset occurs, the PPS SYS_WRAP interrupt was not re-enabled which
resulted in disabling of the PPS signaling. Fix this by recording when
the interrupt is on and ensuring that we re-enable it every time we
reset.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/net/ethernet/intel/igb/igb.h     | 1 +
 drivers/net/ethernet/intel/igb/igb_ptp.c | 5 ++++-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h
index 03fbe4b..d11093d 100644
--- a/drivers/net/ethernet/intel/igb/igb.h
+++ b/drivers/net/ethernet/intel/igb/igb.h
@@ -489,6 +489,7 @@ struct igb_adapter {
 	struct timecounter tc;
 	u32 tx_hwtstamp_timeouts;
 	u32 rx_hwtstamp_cleared;
+	bool pps_sys_wrap_on;
 
 	struct ptp_pin_desc sdp_config[IGB_N_SDP];
 	struct {
diff --git a/drivers/net/ethernet/intel/igb/igb_ptp.c b/drivers/net/ethernet/intel/igb/igb_ptp.c
index 1dd14e1..a7895c4 100644
--- a/drivers/net/ethernet/intel/igb/igb_ptp.c
+++ b/drivers/net/ethernet/intel/igb/igb_ptp.c
@@ -591,6 +591,7 @@ static int igb_ptp_feature_enable_i210(struct ptp_clock_info *ptp,
 			tsim |= TSINTR_SYS_WRAP;
 		else
 			tsim &= ~TSINTR_SYS_WRAP;
+		igb->pps_sys_wrap_on = !!on;
 		wr32(E1000_TSIM, tsim);
 		spin_unlock_irqrestore(&igb->tmreg_lock, flags);
 		return 0;
@@ -1235,7 +1236,9 @@ void igb_ptp_reset(struct igb_adapter *adapter)
 	case e1000_i211:
 		wr32(E1000_TSAUXC, 0x0);
 		wr32(E1000_TSSDP, 0x0);
-		wr32(E1000_TSIM, TSYNC_INTERRUPTS);
+		wr32(E1000_TSIM,
+		     TSYNC_INTERRUPTS |
+		     (adapter->pps_sys_wrap_on ? TSINTR_SYS_WRAP : 0));
 		wr32(E1000_IMS, E1000_IMS_TS);
 		break;
 	default:
-- 
2.7.4

[net-next 5/5] PCI: disable FLR for 82579 device

[Jeff Kirsher]

From: Sasha Neftin <sasha.neftin@intel.com>

82579 has a problem reattaching itself after the device is detached.
The bug was reported by Redhat. The suggested fix is to disable
FLR capability in PCIe configuration space.

Reproduction:
Attach the device to a VM, then detach and try to attach again.

Fix:
Disable FLR capability to prevent the 82579 from hanging.

Signed-off-by: Sasha Neftin <sasha.neftin@intel.com>
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/pci/quirks.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 44e0ff3..59fba6e 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -4431,3 +4431,24 @@ static void quirk_intel_qat_vf_cap(struct pci_dev *pdev)
 	}
 }
 DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x443, quirk_intel_qat_vf_cap);
+/*
+ * Workaround FLR issues for 82579
+ * This code disables the FLR (Function Level Reset) via PCIe, in order
+ * to workaround a bug found while using device passthrough, where the
+ * interface would become non-responsive.
+ * NOTE: the FLR bit is Read/Write Once (RWO) in config space, so if
+ * the BIOS or kernel writes this register * then this workaround will
+ * not work.
+ */
+static void quirk_intel_flr_cap_dis(struct pci_dev *dev)
+{
+	int pos = pci_find_capability(dev, PCI_CAP_ID_AF);
+	if (pos) {
+		u8 cap;
+		pci_read_config_byte(dev, pos + PCI_AF_CAP, &cap);
+		cap = cap & (~PCI_AF_CAP_FLR);
+		pci_write_config_byte(dev, pos + PCI_AF_CAP, cap);
+	}
+}
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1502, quirk_intel_flr_cap_dis);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1503, quirk_intel_flr_cap_dis);
-- 
2.7.4

Re: [net-next 5/5] PCI: disable FLR for 82579 device

[Sergei Shtylyov]

Hello.

On 9/23/2016 9:39 AM, Jeff Kirsher wrote:

> From: Sasha Neftin <sasha.neftin@intel.com>
>
> 82579 has a problem reattaching itself after the device is detached.
> The bug was reported by Redhat. The suggested fix is to disable
> FLR capability in PCIe configuration space.
>
> Reproduction:
> Attach the device to a VM, then detach and try to attach again.
>
> Fix:
> Disable FLR capability to prevent the 82579 from hanging.
>
> Signed-off-by: Sasha Neftin <sasha.neftin@intel.com>
> Tested-by: Aaron Brown <aaron.f.brown@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> ---
>  drivers/pci/quirks.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 44e0ff3..59fba6e 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -4431,3 +4431,24 @@ static void quirk_intel_qat_vf_cap(struct pci_dev *pdev)
>  	}
>  }
>  DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x443, quirk_intel_qat_vf_cap);
> +/*
> + * Workaround FLR issues for 82579
> + * This code disables the FLR (Function Level Reset) via PCIe, in order
> + * to workaround a bug found while using device passthrough, where the
> + * interface would become non-responsive.
> + * NOTE: the FLR bit is Read/Write Once (RWO) in config space, so if
> + * the BIOS or kernel writes this register * then this workaround will
                                               ^
    That asterisk shouldn't be there.

> + * not work.
> + */
> +static void quirk_intel_flr_cap_dis(struct pci_dev *dev)
> +{
> +	int pos = pci_find_capability(dev, PCI_CAP_ID_AF);

    Should be an empty line here...

> +	if (pos) {
> +		u8 cap;

    And here...

> +		pci_read_config_byte(dev, pos + PCI_AF_CAP, &cap);
> +		cap = cap & (~PCI_AF_CAP_FLR);

    () not needed.

> +		pci_write_config_byte(dev, pos + PCI_AF_CAP, cap);
> +	}
> +}
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1502, quirk_intel_flr_cap_dis);
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1503, quirk_intel_flr_cap_dis);

MBR, Sergei

Re: [net-next 5/5] PCI: disable FLR for 82579 device

[Alex Williamson]

On Thu, 22 Sep 2016 23:39:01 -0700
Jeff Kirsher <jeffrey.t.kirsher@intel.com> wrote:

> From: Sasha Neftin <sasha.neftin@intel.com>
> 
> 82579 has a problem reattaching itself after the device is detached.
> The bug was reported by Redhat. The suggested fix is to disable
> FLR capability in PCIe configuration space.
> 
> Reproduction:
> Attach the device to a VM, then detach and try to attach again.
> 
> Fix:
> Disable FLR capability to prevent the 82579 from hanging.
> 
> Signed-off-by: Sasha Neftin <sasha.neftin@intel.com>
> Tested-by: Aaron Brown <aaron.f.brown@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> ---
>  drivers/pci/quirks.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 44e0ff3..59fba6e 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -4431,3 +4431,24 @@ static void quirk_intel_qat_vf_cap(struct pci_dev *pdev)
>  	}
>  }
>  DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x443, quirk_intel_qat_vf_cap);
> +/*
> + * Workaround FLR issues for 82579
> + * This code disables the FLR (Function Level Reset) via PCIe, in order
> + * to workaround a bug found while using device passthrough, where the
> + * interface would become non-responsive.
> + * NOTE: the FLR bit is Read/Write Once (RWO) in config space, so if
> + * the BIOS or kernel writes this register * then this workaround will
> + * not work.
> + */
> +static void quirk_intel_flr_cap_dis(struct pci_dev *dev)
> +{
> +	int pos = pci_find_capability(dev, PCI_CAP_ID_AF);
> +	if (pos) {
> +		u8 cap;
> +		pci_read_config_byte(dev, pos + PCI_AF_CAP, &cap);
> +		cap = cap & (~PCI_AF_CAP_FLR);
> +		pci_write_config_byte(dev, pos + PCI_AF_CAP, cap);
> +	}
> +}
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1502, quirk_intel_flr_cap_dis);
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1503, quirk_intel_flr_cap_dis);

This seems like a pretty fragile quirk since we're just hoping that the
BIOS hasn't already written this byte.  Should we at least re-read and
warn if the write didn't take?  What about using dev_flags or a device
specific reset to make this less fragile?  A device specific reset
could pick the best reset mechanism for the device, ignoring AF FLR.
Thanks,

Alex

Re: [net-next 5/5] PCI: disable FLR for 82579 device

[Bjorn Helgaas]

On Thu, Sep 22, 2016 at 11:39:01PM -0700, Jeff Kirsher wrote:
> From: Sasha Neftin <sasha.neftin@intel.com>
> 
> 82579 has a problem reattaching itself after the device is detached.
> The bug was reported by Redhat. The suggested fix is to disable
> FLR capability in PCIe configuration space.
> 
> Reproduction:
> Attach the device to a VM, then detach and try to attach again.
> 
> Fix:
> Disable FLR capability to prevent the 82579 from hanging.

Is there a bugzilla or other reference URL to include here?  Should
this be marked for stable?

> Signed-off-by: Sasha Neftin <sasha.neftin@intel.com>
> Tested-by: Aaron Brown <aaron.f.brown@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> ---
>  drivers/pci/quirks.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 44e0ff3..59fba6e 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -4431,3 +4431,24 @@ static void quirk_intel_qat_vf_cap(struct pci_dev *pdev)
>  	}
>  }
>  DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x443, quirk_intel_qat_vf_cap);
> +/*
> + * Workaround FLR issues for 82579
> + * This code disables the FLR (Function Level Reset) via PCIe, in order
> + * to workaround a bug found while using device passthrough, where the
> + * interface would become non-responsive.
> + * NOTE: the FLR bit is Read/Write Once (RWO) in config space, so if
> + * the BIOS or kernel writes this register * then this workaround will
> + * not work.

This doesn't sound like a root cause.  Is the issue a hardware
erratum?  Linux PCI core bug?  VFIO bug?  Device firmware bug?

The changelog suggests that the problem only affects passthrough,
which suggests some sort of kernel bug related to how passthrough is
implemented.

> + */
> +static void quirk_intel_flr_cap_dis(struct pci_dev *dev)
> +{
> +	int pos = pci_find_capability(dev, PCI_CAP_ID_AF);
> +	if (pos) {
> +		u8 cap;
> +		pci_read_config_byte(dev, pos + PCI_AF_CAP, &cap);
> +		cap = cap & (~PCI_AF_CAP_FLR);
> +		pci_write_config_byte(dev, pos + PCI_AF_CAP, cap);
> +	}
> +}
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1502, quirk_intel_flr_cap_dis);
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1503, quirk_intel_flr_cap_dis);
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [net-next 5/5] PCI: disable FLR for 82579 device

[Jeff Kirsher]

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

On Fri, 2016-09-23 at 09:01 -0500, Bjorn Helgaas wrote:
> On Thu, Sep 22, 2016 at 11:39:01PM -0700, Jeff Kirsher wrote:
> > 
> > From: Sasha Neftin <sasha.neftin@intel.com>
> > 
> > 82579 has a problem reattaching itself after the device is detached.
> > The bug was reported by Redhat. The suggested fix is to disable
> > FLR capability in PCIe configuration space.
> > 
> > Reproduction:
> > Attach the device to a VM, then detach and try to attach again.
> > 
> > Fix:
> > Disable FLR capability to prevent the 82579 from hanging.
> 
> Is there a bugzilla or other reference URL to include here?  Should
> this be marked for stable?

So the author is in Israel, meaning it is their weekend now.  I do not
believe Sasha monitors email over the weekend, so a response to your
questions won't happen for a few days.

I tried searching my archives for more information, but had no luck finding
any additional information.

> > Signed-off-by: Sasha Neftin <sasha.neftin@intel.com>
> > Tested-by: Aaron Brown <aaron.f.brown@intel.com>
> > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> > ---
> >  drivers/pci/quirks.c | 21 +++++++++++++++++++++
> >  1 file changed, 21 insertions(+)
> > 
> > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > index 44e0ff3..59fba6e 100644
> > --- a/drivers/pci/quirks.c
> > +++ b/drivers/pci/quirks.c
> > @@ -4431,3 +4431,24 @@ static void quirk_intel_qat_vf_cap(struct
> > pci_dev *pdev)
> >  	}
> >  }
> >  DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x443,
> > quirk_intel_qat_vf_cap);
> > +/*
> > + * Workaround FLR issues for 82579
> > + * This code disables the FLR (Function Level Reset) via PCIe, in
> > order
> > + * to workaround a bug found while using device passthrough, where the
> > + * interface would become non-responsive.
> > + * NOTE: the FLR bit is Read/Write Once (RWO) in config space, so if
> > + * the BIOS or kernel writes this register * then this workaround will
> > + * not work.
> 
> This doesn't sound like a root cause.  Is the issue a hardware
> erratum?  Linux PCI core bug?  VFIO bug?  Device firmware bug?
> 
> The changelog suggests that the problem only affects passthrough,
> which suggests some sort of kernel bug related to how passthrough is
> implemented.
> 
> > 
> > + */
> > +static void quirk_intel_flr_cap_dis(struct pci_dev *dev)
> > +{
> > +	int pos = pci_find_capability(dev, PCI_CAP_ID_AF);
> > +	if (pos) {
> > +		u8 cap;
> > +		pci_read_config_byte(dev, pos + PCI_AF_CAP, &cap);
> > +		cap = cap & (~PCI_AF_CAP_FLR);
> > +		pci_write_config_byte(dev, pos + PCI_AF_CAP, cap);
> > +	}
> > +}
> > +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1502,
> > quirk_intel_flr_cap_dis);
> > +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1503,
> > quirk_intel_flr_cap_dis);
> > -- 
> > 2.7.4
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [net-next 5/5] PCI: disable FLR for 82579 device

[Neftin, Sasha]

On 9/24/2016 12:05 AM, Jeff Kirsher wrote:
> On Fri, 2016-09-23 at 09:01 -0500, Bjorn Helgaas wrote:
>> On Thu, Sep 22, 2016 at 11:39:01PM -0700, Jeff Kirsher wrote:
>>> From: Sasha Neftin <sasha.neftin@intel.com>
>>>
>>> 82579 has a problem reattaching itself after the device is detached.
>>> The bug was reported by Redhat. The suggested fix is to disable
>>> FLR capability in PCIe configuration space.
>>>
>>> Reproduction:
>>> Attach the device to a VM, then detach and try to attach again.
>>>
>>> Fix:
>>> Disable FLR capability to prevent the 82579 from hanging.
>> Is there a bugzilla or other reference URL to include here?  Should
>> this be marked for stable?
> So the author is in Israel, meaning it is their weekend now.  I do not
> believe Sasha monitors email over the weekend, so a response to your
> questions won't happen for a few days.
>
> I tried searching my archives for more information, but had no luck finding
> any additional information.
>
>>> Signed-off-by: Sasha Neftin <sasha.neftin@intel.com>
>>> Tested-by: Aaron Brown <aaron.f.brown@intel.com>
>>> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
>>> ---
>>>   drivers/pci/quirks.c | 21 +++++++++++++++++++++
>>>   1 file changed, 21 insertions(+)
>>>
>>> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
>>> index 44e0ff3..59fba6e 100644
>>> --- a/drivers/pci/quirks.c
>>> +++ b/drivers/pci/quirks.c
>>> @@ -4431,3 +4431,24 @@ static void quirk_intel_qat_vf_cap(struct
>>> pci_dev *pdev)
>>>   	}
>>>   }
>>>   DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x443,
>>> quirk_intel_qat_vf_cap);
>>> +/*
>>> + * Workaround FLR issues for 82579
>>> + * This code disables the FLR (Function Level Reset) via PCIe, in
>>> order
>>> + * to workaround a bug found while using device passthrough, where the
>>> + * interface would become non-responsive.
>>> + * NOTE: the FLR bit is Read/Write Once (RWO) in config space, so if
>>> + * the BIOS or kernel writes this register * then this workaround will
>>> + * not work.
>> This doesn't sound like a root cause.  Is the issue a hardware
>> erratum?  Linux PCI core bug?  VFIO bug?  Device firmware bug?
>>
>> The changelog suggests that the problem only affects passthrough,
>> which suggests some sort of kernel bug related to how passthrough is
>> implemented.
>>
>>> + */
>>> +static void quirk_intel_flr_cap_dis(struct pci_dev *dev)
>>> +{
>>> +	int pos = pci_find_capability(dev, PCI_CAP_ID_AF);
>>> +	if (pos) {
>>> +		u8 cap;
>>> +		pci_read_config_byte(dev, pos + PCI_AF_CAP, &cap);
>>> +		cap = cap & (~PCI_AF_CAP_FLR);
>>> +		pci_write_config_byte(dev, pos + PCI_AF_CAP, cap);
>>> +	}
>>> +}
>>> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1502,
>>> quirk_intel_flr_cap_dis);
>>> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1503,
>>> quirk_intel_flr_cap_dis);
>>> -- 
>>> 2.7.4
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hello,

Original bugzilla thread could be found here: 
https://bugzilla.redhat.com/show_bug.cgi?format=multiple&id=966840

This is our HW bug, exist only in 82579 devices. More new devices have 
no such problem. We have found root cause and suggested this solution. 
This solution should work for a 95% of cases, so I do not think that 
this is fragile. For another cases possible solution is get up working 
system and manually disable FLR, before VM start use our adapter.

Thanks,

Sasha

Re: [net-next 5/5] PCI: disable FLR for 82579 device

[Bjorn Helgaas]

On Sun, Sep 25, 2016 at 10:02:43AM +0300, Neftin, Sasha wrote:
> On 9/24/2016 12:05 AM, Jeff Kirsher wrote:
> >On Fri, 2016-09-23 at 09:01 -0500, Bjorn Helgaas wrote:
> >>On Thu, Sep 22, 2016 at 11:39:01PM -0700, Jeff Kirsher wrote:
> >>>From: Sasha Neftin <sasha.neftin@intel.com>
> >>>
> >>>82579 has a problem reattaching itself after the device is detached.
> >>>The bug was reported by Redhat. The suggested fix is to disable
> >>>FLR capability in PCIe configuration space.
> >>>
> >>>Reproduction:
> >>>Attach the device to a VM, then detach and try to attach again.
> >>>
> >>>Fix:
> >>>Disable FLR capability to prevent the 82579 from hanging.
> >>Is there a bugzilla or other reference URL to include here?  Should
> >>this be marked for stable?
> >So the author is in Israel, meaning it is their weekend now.  I do not
> >believe Sasha monitors email over the weekend, so a response to your
> >questions won't happen for a few days.
> >
> >I tried searching my archives for more information, but had no luck finding
> >any additional information.
> >
> >>>Signed-off-by: Sasha Neftin <sasha.neftin@intel.com>
> >>>Tested-by: Aaron Brown <aaron.f.brown@intel.com>
> >>>Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> >>>---
> >>>  drivers/pci/quirks.c | 21 +++++++++++++++++++++
> >>>  1 file changed, 21 insertions(+)
> >>>
> >>>diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> >>>index 44e0ff3..59fba6e 100644
> >>>--- a/drivers/pci/quirks.c
> >>>+++ b/drivers/pci/quirks.c
> >>>@@ -4431,3 +4431,24 @@ static void quirk_intel_qat_vf_cap(struct
> >>>pci_dev *pdev)
> >>>  	}
> >>>  }
> >>>  DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x443,
> >>>quirk_intel_qat_vf_cap);
> >>>+/*
> >>>+ * Workaround FLR issues for 82579
> >>>+ * This code disables the FLR (Function Level Reset) via PCIe, in
> >>>order
> >>>+ * to workaround a bug found while using device passthrough, where the
> >>>+ * interface would become non-responsive.
> >>>+ * NOTE: the FLR bit is Read/Write Once (RWO) in config space, so if
> >>>+ * the BIOS or kernel writes this register * then this workaround will
> >>>+ * not work.
> >>This doesn't sound like a root cause.  Is the issue a hardware
> >>erratum?  Linux PCI core bug?  VFIO bug?  Device firmware bug?
> >>
> >>The changelog suggests that the problem only affects passthrough,
> >>which suggests some sort of kernel bug related to how passthrough is
> >>implemented.

If this bug affects all scenarios, not just passthrough, the changelog
should not mention passthrough.

> >>>+ */
> >>>+static void quirk_intel_flr_cap_dis(struct pci_dev *dev)
> >>>+{
> >>>+	int pos = pci_find_capability(dev, PCI_CAP_ID_AF);
> >>>+	if (pos) {
> >>>+		u8 cap;
> >>>+		pci_read_config_byte(dev, pos + PCI_AF_CAP, &cap);
> >>>+		cap = cap & (~PCI_AF_CAP_FLR);
> >>>+		pci_write_config_byte(dev, pos + PCI_AF_CAP, cap);
> >>>+	}
> >>>+}
> >>>+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1502,
> >>>quirk_intel_flr_cap_dis);
> >>>+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1503,
> >>>quirk_intel_flr_cap_dis);
> >>>-- 
> >>>2.7.4
> >>>
> >>>--
> >>>To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> >>>the body of a message to majordomo@vger.kernel.org
> >>>More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> Hello,
> 
> Original bugzilla thread could be found here:
> https://bugzilla.redhat.com/show_bug.cgi?format=multiple&id=966840

That bugzilla is private and I can't read it.

> This is our HW bug, exist only in 82579 devices. More new devices
> have no such problem. We have found root cause and suggested this
> solution. 

Is there an erratum you can reference?

> This solution should work for a 95% of cases, so I do not
> think that this is fragile. For another cases possible solution is
> get up working system and manually disable FLR, before VM start use
> our adapter.

I don't think a 95% solution is sufficient.  Can you use the
pci_dev_specific_reset() framework to make a 100% solution?

Bjorn

Re: [net-next 5/5] PCI: disable FLR for 82579 device

[Alex Williamson]

On Tue, 27 Sep 2016 13:17:02 -0500
Bjorn Helgaas <helgaas@kernel.org> wrote:

> On Sun, Sep 25, 2016 at 10:02:43AM +0300, Neftin, Sasha wrote:
> > On 9/24/2016 12:05 AM, Jeff Kirsher wrote:  
> > >On Fri, 2016-09-23 at 09:01 -0500, Bjorn Helgaas wrote:  
> > >>On Thu, Sep 22, 2016 at 11:39:01PM -0700, Jeff Kirsher wrote:  
> > >>>From: Sasha Neftin <sasha.neftin@intel.com>
> > >>>
> > >>>82579 has a problem reattaching itself after the device is detached.
> > >>>The bug was reported by Redhat. The suggested fix is to disable
> > >>>FLR capability in PCIe configuration space.
> > >>>
> > >>>Reproduction:
> > >>>Attach the device to a VM, then detach and try to attach again.
> > >>>
> > >>>Fix:
> > >>>Disable FLR capability to prevent the 82579 from hanging.  
> > >>Is there a bugzilla or other reference URL to include here?  Should
> > >>this be marked for stable?  
> > >So the author is in Israel, meaning it is their weekend now.  I do not
> > >believe Sasha monitors email over the weekend, so a response to your
> > >questions won't happen for a few days.
> > >
> > >I tried searching my archives for more information, but had no luck finding
> > >any additional information.
> > >  
> > >>>Signed-off-by: Sasha Neftin <sasha.neftin@intel.com>
> > >>>Tested-by: Aaron Brown <aaron.f.brown@intel.com>
> > >>>Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> > >>>---
> > >>>  drivers/pci/quirks.c | 21 +++++++++++++++++++++
> > >>>  1 file changed, 21 insertions(+)
> > >>>
> > >>>diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > >>>index 44e0ff3..59fba6e 100644
> > >>>--- a/drivers/pci/quirks.c
> > >>>+++ b/drivers/pci/quirks.c
> > >>>@@ -4431,3 +4431,24 @@ static void quirk_intel_qat_vf_cap(struct
> > >>>pci_dev *pdev)
> > >>>  	}
> > >>>  }
> > >>>  DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x443,
> > >>>quirk_intel_qat_vf_cap);
> > >>>+/*
> > >>>+ * Workaround FLR issues for 82579
> > >>>+ * This code disables the FLR (Function Level Reset) via PCIe, in
> > >>>order
> > >>>+ * to workaround a bug found while using device passthrough, where the
> > >>>+ * interface would become non-responsive.
> > >>>+ * NOTE: the FLR bit is Read/Write Once (RWO) in config space, so if
> > >>>+ * the BIOS or kernel writes this register * then this workaround will
> > >>>+ * not work.  
> > >>This doesn't sound like a root cause.  Is the issue a hardware
> > >>erratum?  Linux PCI core bug?  VFIO bug?  Device firmware bug?
> > >>
> > >>The changelog suggests that the problem only affects passthrough,
> > >>which suggests some sort of kernel bug related to how passthrough is
> > >>implemented.  
> 
> If this bug affects all scenarios, not just passthrough, the changelog
> should not mention passthrough.
> 
> > >>>+ */
> > >>>+static void quirk_intel_flr_cap_dis(struct pci_dev *dev)
> > >>>+{
> > >>>+	int pos = pci_find_capability(dev, PCI_CAP_ID_AF);
> > >>>+	if (pos) {
> > >>>+		u8 cap;
> > >>>+		pci_read_config_byte(dev, pos + PCI_AF_CAP, &cap);
> > >>>+		cap = cap & (~PCI_AF_CAP_FLR);
> > >>>+		pci_write_config_byte(dev, pos + PCI_AF_CAP, cap);
> > >>>+	}
> > >>>+}
> > >>>+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1502,
> > >>>quirk_intel_flr_cap_dis);
> > >>>+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1503,
> > >>>quirk_intel_flr_cap_dis);
> > >>>-- 
> > >>>2.7.4
> > >>>
> > >>>--
> > >>>To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> > >>>the body of a message to majordomo@vger.kernel.org
> > >>>More majordomo info at  http://vger.kernel.org/majordomo-info.html  
> > 
> > Hello,
> > 
> > Original bugzilla thread could be found here:
> > https://bugzilla.redhat.com/show_bug.cgi?format=multiple&id=966840  
> 
> That bugzilla is private and I can't read it.

Hmm, I can, but I don't see anything in it that supports this.  Is that
really the right bz?  It's the right hardware, but has all sorts of FUD
about the version of various other components in the stack.
 
> > This is our HW bug, exist only in 82579 devices. More new devices
> > have no such problem. We have found root cause and suggested this
> > solution.   
> 
> Is there an erratum you can reference?
> 
> > This solution should work for a 95% of cases, so I do not
> > think that this is fragile. For another cases possible solution is
> > get up working system and manually disable FLR, before VM start use
> > our adapter.  
> 
> I don't think a 95% solution is sufficient.  Can you use the
> pci_dev_specific_reset() framework to make a 100% solution?

Right, plus when this does work I suspect it removes the one mechanism
we have to reset the device, which depending on how obscure the failure
scenario is, isn't a clear cut improvement for device assignment.
Thanks,

Alex

Re: [net-next 5/5] PCI: disable FLR for 82579 device

[Alexander Duyck]

Since I worked with Sasha on this I will provide a bit of information
from what I understand of this bug as well.

On Tue, Sep 27, 2016 at 12:13 PM, Alex Williamson
<alex.williamson@redhat.com> wrote:
> On Tue, 27 Sep 2016 13:17:02 -0500
> Bjorn Helgaas <helgaas@kernel.org> wrote:
>
>> On Sun, Sep 25, 2016 at 10:02:43AM +0300, Neftin, Sasha wrote:
>> > On 9/24/2016 12:05 AM, Jeff Kirsher wrote:
>> > >On Fri, 2016-09-23 at 09:01 -0500, Bjorn Helgaas wrote:
>> > >>On Thu, Sep 22, 2016 at 11:39:01PM -0700, Jeff Kirsher wrote:
>> > >>>From: Sasha Neftin <sasha.neftin@intel.com>
>> > >>>
>> > >>>82579 has a problem reattaching itself after the device is detached.
>> > >>>The bug was reported by Redhat. The suggested fix is to disable
>> > >>>FLR capability in PCIe configuration space.
>> > >>>
>> > >>>Reproduction:
>> > >>>Attach the device to a VM, then detach and try to attach again.
>> > >>>
>> > >>>Fix:
>> > >>>Disable FLR capability to prevent the 82579 from hanging.
>> > >>Is there a bugzilla or other reference URL to include here?  Should
>> > >>this be marked for stable?
>> > >So the author is in Israel, meaning it is their weekend now.  I do not
>> > >believe Sasha monitors email over the weekend, so a response to your
>> > >questions won't happen for a few days.
>> > >
>> > >I tried searching my archives for more information, but had no luck finding
>> > >any additional information.
>> > >

I agree that we do probably need to update the patch description since
it isn't exactly clear what this is fixing or what was actually
broken.

>> > >>>Signed-off-by: Sasha Neftin <sasha.neftin@intel.com>
>> > >>>Tested-by: Aaron Brown <aaron.f.brown@intel.com>
>> > >>>Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
>> > >>>---
>> > >>>  drivers/pci/quirks.c | 21 +++++++++++++++++++++
>> > >>>  1 file changed, 21 insertions(+)
>> > >>>
>> > >>>diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
>> > >>>index 44e0ff3..59fba6e 100644
>> > >>>--- a/drivers/pci/quirks.c
>> > >>>+++ b/drivers/pci/quirks.c
>> > >>>@@ -4431,3 +4431,24 @@ static void quirk_intel_qat_vf_cap(struct
>> > >>>pci_dev *pdev)
>> > >>>         }
>> > >>>  }
>> > >>>  DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x443,
>> > >>>quirk_intel_qat_vf_cap);
>> > >>>+/*
>> > >>>+ * Workaround FLR issues for 82579
>> > >>>+ * This code disables the FLR (Function Level Reset) via PCIe, in
>> > >>>order
>> > >>>+ * to workaround a bug found while using device passthrough, where the
>> > >>>+ * interface would become non-responsive.
>> > >>>+ * NOTE: the FLR bit is Read/Write Once (RWO) in config space, so if
>> > >>>+ * the BIOS or kernel writes this register * then this workaround will
>> > >>>+ * not work.
>> > >>This doesn't sound like a root cause.  Is the issue a hardware
>> > >>erratum?  Linux PCI core bug?  VFIO bug?  Device firmware bug?
>> > >>
>> > >>The changelog suggests that the problem only affects passthrough,
>> > >>which suggests some sort of kernel bug related to how passthrough is
>> > >>implemented.
>>
>> If this bug affects all scenarios, not just passthrough, the changelog
>> should not mention passthrough.
>>
>> > >>>+ */
>> > >>>+static void quirk_intel_flr_cap_dis(struct pci_dev *dev)
>> > >>>+{
>> > >>>+        int pos = pci_find_capability(dev, PCI_CAP_ID_AF);
>> > >>>+        if (pos) {
>> > >>>+                u8 cap;
>> > >>>+                pci_read_config_byte(dev, pos + PCI_AF_CAP, &cap);
>> > >>>+                cap = cap & (~PCI_AF_CAP_FLR);
>> > >>>+                pci_write_config_byte(dev, pos + PCI_AF_CAP, cap);
>> > >>>+        }
>> > >>>+}
>> > >>>+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1502,
>> > >>>quirk_intel_flr_cap_dis);
>> > >>>+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1503,
>> > >>>quirk_intel_flr_cap_dis);
>> > >>>--
>> > >>>2.7.4
>> > >>>
>> > >>>--
>> > >>>To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>> > >>>the body of a message to majordomo@vger.kernel.org
>> > >>>More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> >
>> > Hello,
>> >
>> > Original bugzilla thread could be found here:
>> > https://bugzilla.redhat.com/show_bug.cgi?format=multiple&id=966840
>>
>> That bugzilla is private and I can't read it.
>
> Hmm, I can, but I don't see anything in it that supports this.  Is that
> really the right bz?  It's the right hardware, but has all sorts of FUD
> about the version of various other components in the stack.

It looks like we had a local copy of the bugzilla saved here, though
it only goes up to comment 13 which is where I think we started
working this on our side.  I believe what this patch is attempting to
resolve is related to comment 8 where the driver returned "probe of
0000:00:19.0 failed with error ‐2" instead of correctly probing the
interface.

So the bug as reported was that e1000e had a problem reattaching
itself to the PHY after it was attached to a VM.  Sasha, please feel
free to correct me if I have this bit wrong.  I had been told the
problem was the FLR functionality wasn't fully implemented so that was
why they were wanting to defeature it.  I had assumed that there was
support for a reset on D0->D3->D0, but I just realized that probably
isn't the case since it looks like my local system sets the NoSoftRST
bit.

>> > This is our HW bug, exist only in 82579 devices. More new devices
>> > have no such problem. We have found root cause and suggested this
>> > solution.
>>
>> Is there an erratum you can reference?
>>
>> > This solution should work for a 95% of cases, so I do not
>> > think that this is fragile. For another cases possible solution is
>> > get up working system and manually disable FLR, before VM start use
>> > our adapter.
>>
>> I don't think a 95% solution is sufficient.  Can you use the
>> pci_dev_specific_reset() framework to make a 100% solution?

I can try working with Sasha on this to see what we can do.

> Right, plus when this does work I suspect it removes the one mechanism
> we have to reset the device, which depending on how obscure the failure
> scenario is, isn't a clear cut improvement for device assignment.
> Thanks,
>
> Alex

I'll work with Sasha to see what we can do.  Odds are there is some
sort of problem between the MAC/PHY that needs to be resolved when we
perform the function level reset so we will probably need to add code
so that we reset the part and re-establish the link with the PHY after
the reset.

- Alex

Re: [net-next 0/5][pull request] 1GbE Intel Wired LAN Driver Updates 2016-09-22

[Jeff Kirsher]

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

On Thu, 2016-09-22 at 23:38 -0700, Jeff Kirsher wrote:
> This series contains updates to igb, igbvf and PCI quirks.
> 
> Wei Yongjun makes a function static to shut up sparse.
> 
> Todd bumps the igb and igbvf version, which is long overdue.
> 
> Jake fixes an issue where the PPS SYS_WRAP interrupt was not re-enabled
> after a reset, which resulted in disabling of the PPS signaling.
> 
> Sasha adds a quirk for 82579 devices, which was reported by Red Hat.
> The issue is that 82579 has a problem reattaching itself after being
> detached, so disable the FLR capability in PCIe configuration space.

Based on feedback, I will drop patch 5 in the series and re-submit the
series, while Alex and Sasha re-work the PCI quirk patch.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

RE: [net-next 5/5] PCI: disable FLR for 82579 device

[Neftin, Sasha]

Since I worked with Sasha on this I will provide a bit of information from what I understand of this bug as well.

On Tue, Sep 27, 2016 at 12:13 PM, Alex Williamson <alex.williamson@redhat.com> wrote:
> On Tue, 27 Sep 2016 13:17:02 -0500
> Bjorn Helgaas <helgaas@kernel.org> wrote:
>
>> On Sun, Sep 25, 2016 at 10:02:43AM +0300, Neftin, Sasha wrote:
>> > On 9/24/2016 12:05 AM, Jeff Kirsher wrote:
>> > >On Fri, 2016-09-23 at 09:01 -0500, Bjorn Helgaas wrote:
>> > >>On Thu, Sep 22, 2016 at 11:39:01PM -0700, Jeff Kirsher wrote:
>> > >>>From: Sasha Neftin <sasha.neftin@intel.com>
>> > >>>
>> > >>>82579 has a problem reattaching itself after the device is detached.
>> > >>>The bug was reported by Redhat. The suggested fix is to disable 
>> > >>>FLR capability in PCIe configuration space.
>> > >>>
>> > >>>Reproduction:
>> > >>>Attach the device to a VM, then detach and try to attach again.
>> > >>>
>> > >>>Fix:
>> > >>>Disable FLR capability to prevent the 82579 from hanging.
>> > >>Is there a bugzilla or other reference URL to include here?  
>> > >>Should this be marked for stable?
>> > >So the author is in Israel, meaning it is their weekend now.  I do 
>> > >not believe Sasha monitors email over the weekend, so a response 
>> > >to your questions won't happen for a few days.
>> > >
>> > >I tried searching my archives for more information, but had no 
>> > >luck finding any additional information.
>> > >

I agree that we do probably need to update the patch description since it isn't exactly clear what this is fixing or what was actually broken.

>> > >>>Signed-off-by: Sasha Neftin <sasha.neftin@intel.com>
>> > >>>Tested-by: Aaron Brown <aaron.f.brown@intel.com>
>> > >>>Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
>> > >>>---
>> > >>>  drivers/pci/quirks.c | 21 +++++++++++++++++++++
>> > >>>  1 file changed, 21 insertions(+)
>> > >>>
>> > >>>diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c index 
>> > >>>44e0ff3..59fba6e 100644
>> > >>>--- a/drivers/pci/quirks.c
>> > >>>+++ b/drivers/pci/quirks.c
>> > >>>@@ -4431,3 +4431,24 @@ static void quirk_intel_qat_vf_cap(struct 
>> > >>>pci_dev *pdev)
>> > >>>         }
>> > >>>  }
>> > >>>  DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x443, 
>> > >>>quirk_intel_qat_vf_cap);
>> > >>>+/*
>> > >>>+ * Workaround FLR issues for 82579
>> > >>>+ * This code disables the FLR (Function Level Reset) via PCIe, 
>> > >>>+in
>> > >>>order
>> > >>>+ * to workaround a bug found while using device passthrough, 
>> > >>>+ where the
>> > >>>+ * interface would become non-responsive.
>> > >>>+ * NOTE: the FLR bit is Read/Write Once (RWO) in config space, 
>> > >>>+ so if
>> > >>>+ * the BIOS or kernel writes this register * then this 
>> > >>>+ workaround will
>> > >>>+ * not work.
>> > >>This doesn't sound like a root cause.  Is the issue a hardware 
>> > >>erratum?  Linux PCI core bug?  VFIO bug?  Device firmware bug?
>> > >>
>> > >>The changelog suggests that the problem only affects passthrough, 
>> > >>which suggests some sort of kernel bug related to how passthrough 
>> > >>is implemented.
>>
>> If this bug affects all scenarios, not just passthrough, the 
>> changelog should not mention passthrough.
>>
>> > >>>+ */
>> > >>>+static void quirk_intel_flr_cap_dis(struct pci_dev *dev) {
>> > >>>+        int pos = pci_find_capability(dev, PCI_CAP_ID_AF);
>> > >>>+        if (pos) {
>> > >>>+                u8 cap;
>> > >>>+                pci_read_config_byte(dev, pos + PCI_AF_CAP, &cap);
>> > >>>+                cap = cap & (~PCI_AF_CAP_FLR);
>> > >>>+                pci_write_config_byte(dev, pos + PCI_AF_CAP, cap);
>> > >>>+        }
>> > >>>+}
>> > >>>+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1502,
>> > >>>quirk_intel_flr_cap_dis);
>> > >>>+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1503,
>> > >>>quirk_intel_flr_cap_dis);
>> > >>>--
>> > >>>2.7.4
>> > >>>
>> > >>>--
>> > >>>To unsubscribe from this list: send the line "unsubscribe 
>> > >>>linux-pci" in the body of a message to majordomo@vger.kernel.org 
>> > >>>More majordomo info at  
>> > >>>http://vger.kernel.org/majordomo-info.html
>> >
>> > Hello,
>> >
>> > Original bugzilla thread could be found here:
>> > https://bugzilla.redhat.com/show_bug.cgi?format=multiple&id=966840
>>
>> That bugzilla is private and I can't read it.
>
> Hmm, I can, but I don't see anything in it that supports this.  Is 
> that really the right bz?  It's the right hardware, but has all sorts 
> of FUD about the version of various other components in the stack.

It looks like we had a local copy of the bugzilla saved here, though it only goes up to comment 13 which is where I think we started working this on our side.  I believe what this patch is attempting to resolve is related to comment 8 where the driver returned "probe of
0000:00:19.0 failed with error ‐2" instead of correctly probing the interface.

So the bug as reported was that e1000e had a problem reattaching itself to the PHY after it was attached to a VM.  Sasha, please feel free to correct me if I have this bit wrong.  I had been told the problem was the FLR functionality wasn't fully implemented so that was why they were wanting to defeature it.  I had assumed that there was support for a reset on D0->D3->D0, but I just realized that probably isn't the case since it looks like my local system sets the NoSoftRST bit.

>> > This is our HW bug, exist only in 82579 devices. More new devices 
>> > have no such problem. We have found root cause and suggested this 
>> > solution.
>>
>> Is there an erratum you can reference?
>>
>> > This solution should work for a 95% of cases, so I do not think 
>> > that this is fragile. For another cases possible solution is get up 
>> > working system and manually disable FLR, before VM start use our 
>> > adapter.
>>
>> I don't think a 95% solution is sufficient.  Can you use the
>> pci_dev_specific_reset() framework to make a 100% solution?

I can try working with Sasha on this to see what we can do.

> Right, plus when this does work I suspect it removes the one mechanism 
> we have to reset the device, which depending on how obscure the 
> failure scenario is, isn't a clear cut improvement for device assignment.
> Thanks,
>
> Alex

I'll work with Sasha to see what we can do.  Odds are there is some sort of problem between the MAC/PHY that needs to be resolved when we perform the function level reset so we will probably need to add code so that we reset the part and re-establish the link with the PHY after the reset.

- Alex


Hello,
I would like clarify a few points.

1. 82579 client devices do not support functional level reset (FLR). Unfortunately, it advertises FLR capability -  this is a bug. So, when VM software even accidentally tries to access FLR, that causes our device to hang. To eliminate this problem we decided to disable FLR via drivers/pci/quirks.c 
In previous e-mail I wrote that the solution should work for a 95% of cases. Cases where it might not work if BIOS writes FLR capability before the OS (In 82579 device FLR capability is RWO - read write once). But this is a very weird scenario, so probably the workaround will work close to 100%. Also, I would like to add that we have run lots of tests over 82579 in our lab and ensured that all flows work properly.

2. D0->D3-D0 flow is not affected and is in fact supported by 82579.

3. I will look for an erratum explaining this.

Thanks,
Sasha

Re: [net-next 5/5] PCI: disable FLR for 82579 device

[Bjorn Helgaas]

On Wed, Sep 28, 2016 at 03:33:52PM +0000, Neftin, Sasha wrote:
> 
> Since I worked with Sasha on this I will provide a bit of information from what I understand of this bug as well.
> 
> On Tue, Sep 27, 2016 at 12:13 PM, Alex Williamson <alex.williamson@redhat.com> wrote:
> > On Tue, 27 Sep 2016 13:17:02 -0500
> > Bjorn Helgaas <helgaas@kernel.org> wrote:
> >
> >> On Sun, Sep 25, 2016 at 10:02:43AM +0300, Neftin, Sasha wrote:
> >> > On 9/24/2016 12:05 AM, Jeff Kirsher wrote:
> >> > >On Fri, 2016-09-23 at 09:01 -0500, Bjorn Helgaas wrote:
> >> > >>On Thu, Sep 22, 2016 at 11:39:01PM -0700, Jeff Kirsher wrote:
> >> > >>>From: Sasha Neftin <sasha.neftin@intel.com>
> >> > >>>
> >> > >>>82579 has a problem reattaching itself after the device is detached.
> >> > >>>The bug was reported by Redhat. The suggested fix is to disable 
> >> > >>>FLR capability in PCIe configuration space.
> >> > >>>
> >> > >>>Reproduction:
> >> > >>>Attach the device to a VM, then detach and try to attach again.
> >> > >>>
> >> > >>>Fix:
> >> > >>>Disable FLR capability to prevent the 82579 from hanging.
> >> > >>Is there a bugzilla or other reference URL to include here?  
> >> > >>Should this be marked for stable?
> >> > >So the author is in Israel, meaning it is their weekend now.  I do 
> >> > >not believe Sasha monitors email over the weekend, so a response 
> >> > >to your questions won't happen for a few days.
> >> > >
> >> > >I tried searching my archives for more information, but had no 
> >> > >luck finding any additional information.
> >> > >
> 
> I agree that we do probably need to update the patch description since it isn't exactly clear what this is fixing or what was actually broken.
> 
> >> > >>>Signed-off-by: Sasha Neftin <sasha.neftin@intel.com>
> >> > >>>Tested-by: Aaron Brown <aaron.f.brown@intel.com>
> >> > >>>Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> >> > >>>---
> >> > >>>  drivers/pci/quirks.c | 21 +++++++++++++++++++++
> >> > >>>  1 file changed, 21 insertions(+)
> >> > >>>
> >> > >>>diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c index 
> >> > >>>44e0ff3..59fba6e 100644
> >> > >>>--- a/drivers/pci/quirks.c
> >> > >>>+++ b/drivers/pci/quirks.c
> >> > >>>@@ -4431,3 +4431,24 @@ static void quirk_intel_qat_vf_cap(struct 
> >> > >>>pci_dev *pdev)
> >> > >>>         }
> >> > >>>  }
> >> > >>>  DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x443, 
> >> > >>>quirk_intel_qat_vf_cap);
> >> > >>>+/*
> >> > >>>+ * Workaround FLR issues for 82579
> >> > >>>+ * This code disables the FLR (Function Level Reset) via PCIe, 
> >> > >>>+in
> >> > >>>order
> >> > >>>+ * to workaround a bug found while using device passthrough, 
> >> > >>>+ where the
> >> > >>>+ * interface would become non-responsive.
> >> > >>>+ * NOTE: the FLR bit is Read/Write Once (RWO) in config space, 
> >> > >>>+ so if
> >> > >>>+ * the BIOS or kernel writes this register * then this 
> >> > >>>+ workaround will
> >> > >>>+ * not work.
> >> > >>This doesn't sound like a root cause.  Is the issue a hardware 
> >> > >>erratum?  Linux PCI core bug?  VFIO bug?  Device firmware bug?
> >> > >>
> >> > >>The changelog suggests that the problem only affects passthrough, 
> >> > >>which suggests some sort of kernel bug related to how passthrough 
> >> > >>is implemented.
> >>
> >> If this bug affects all scenarios, not just passthrough, the 
> >> changelog should not mention passthrough.
> >>
> >> > >>>+ */
> >> > >>>+static void quirk_intel_flr_cap_dis(struct pci_dev *dev) {
> >> > >>>+        int pos = pci_find_capability(dev, PCI_CAP_ID_AF);
> >> > >>>+        if (pos) {
> >> > >>>+                u8 cap;
> >> > >>>+                pci_read_config_byte(dev, pos + PCI_AF_CAP, &cap);
> >> > >>>+                cap = cap & (~PCI_AF_CAP_FLR);
> >> > >>>+                pci_write_config_byte(dev, pos + PCI_AF_CAP, cap);
> >> > >>>+        }
> >> > >>>+}
> >> > >>>+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1502,
> >> > >>>quirk_intel_flr_cap_dis);
> >> > >>>+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1503,
> >> > >>>quirk_intel_flr_cap_dis);
> >> > >>>--
> >> > >>>2.7.4
> >> > >>>
> >> > >>>--
> >> > >>>To unsubscribe from this list: send the line "unsubscribe 
> >> > >>>linux-pci" in the body of a message to majordomo@vger.kernel.org 
> >> > >>>More majordomo info at  
> >> > >>>http://vger.kernel.org/majordomo-info.html
> >> >
> >> > Hello,
> >> >
> >> > Original bugzilla thread could be found here:
> >> > https://bugzilla.redhat.com/show_bug.cgi?format=multiple&id=966840
> >>
> >> That bugzilla is private and I can't read it.
> >
> > Hmm, I can, but I don't see anything in it that supports this.  Is 
> > that really the right bz?  It's the right hardware, but has all sorts 
> > of FUD about the version of various other components in the stack.
> 
> It looks like we had a local copy of the bugzilla saved here, though it only goes up to comment 13 which is where I think we started working this on our side.  I believe what this patch is attempting to resolve is related to comment 8 where the driver returned "probe of
> 0000:00:19.0 failed with error ‐2" instead of correctly probing the interface.
> 
> So the bug as reported was that e1000e had a problem reattaching itself to the PHY after it was attached to a VM.  Sasha, please feel free to correct me if I have this bit wrong.  I had been told the problem was the FLR functionality wasn't fully implemented so that was why they were wanting to defeature it.  I had assumed that there was support for a reset on D0->D3->D0, but I just realized that probably isn't the case since it looks like my local system sets the NoSoftRST bit.
> 
> >> > This is our HW bug, exist only in 82579 devices. More new devices 
> >> > have no such problem. We have found root cause and suggested this 
> >> > solution.
> >>
> >> Is there an erratum you can reference?
> >>
> >> > This solution should work for a 95% of cases, so I do not think 
> >> > that this is fragile. For another cases possible solution is get up 
> >> > working system and manually disable FLR, before VM start use our 
> >> > adapter.
> >>
> >> I don't think a 95% solution is sufficient.  Can you use the
> >> pci_dev_specific_reset() framework to make a 100% solution?
> 
> I can try working with Sasha on this to see what we can do.
> 
> > Right, plus when this does work I suspect it removes the one mechanism 
> > we have to reset the device, which depending on how obscure the 
> > failure scenario is, isn't a clear cut improvement for device assignment.
> > Thanks,
> >
> > Alex
> 
> I'll work with Sasha to see what we can do.  Odds are there is some sort of problem between the MAC/PHY that needs to be resolved when we perform the function level reset so we will probably need to add code so that we reset the part and re-establish the link with the PHY after the reset.
> 
> - Alex
> 
> 
> Hello,
> I would like clarify a few points.
> 
> 1. 82579 client devices do not support functional level reset (FLR).
> Unfortunately, it advertises FLR capability -  this is a bug. So,
> when VM software even accidentally tries to access FLR, that causes
> our device to hang. To eliminate this problem we decided to disable
> FLR via drivers/pci/quirks.c 

That makes sense.  To me, the problem is that writing the AF
capability to disable FLR is unreliable.  We can't tell whether BIOS
has already written it, and there's no way to debug the corner cases
where BIOS *has* written it and the device doesn't reset correctly.

Maybe it is in fact weird for a BIOS to write that, and the corner
case will never arise.  But I have no way of knowing that, and I don't
like making assumptions about what a BIOS will or won't do.

Here are two ideas:

  1) Implement a device-specific reset quirk.  The problem here is
  that you'd have to duplicate the body of __pci_dev_reset(), omitting
  the pcie_flr() and pci_af_flr() calls.  This might be a maintenance
  issue because future changes to __pci_dev_reset() should also be
  made to the quirk.

  2) Add some sort of "broken_flr_reset" bit in struct pci_dev, set it
  with a quirk, and change pci_af_flr() to return -ENOTTY when it is
  set.

> In previous e-mail I wrote that the solution should work for a 95% of cases. Cases where it might not work if BIOS writes FLR capability before the OS (In 82579 device FLR capability is RWO - read write once). But this is a very weird scenario, so probably the workaround will work close to 100%. Also, I would like to add that we have run lots of tests over 82579 in our lab and ensured that all flows work properly.
> 
> 2. D0->D3-D0 flow is not affected and is in fact supported by 82579.
> 
> 3. I will look for an erratum explaining this.
> 
> Thanks,
> Sasha
> 
> 
> 
> 
>