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

[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 no=
t
>> > >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 f=
inding
>> > >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 =3D pci_find_capability(dev, PCI_CAP_ID_AF);
>> > >>>+        if (pos) {
>> > >>>+                u8 cap;
>> > >>>+                pci_read_config_byte(dev, pos + PCI_AF_CAP, &cap);
>> > >>>+                cap =3D 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=3Dmultiple&id=3D966840
>>
>> 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 =E2=80=902" 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 5/5] PCI: disable FLR for 82579 device

[Neftin, Sasha]

DQpTaW5jZSBJIHdvcmtlZCB3aXRoIFNhc2hhIG9uIHRoaXMgSSB3aWxsIHByb3ZpZGUgYSBiaXQg
b2YgaW5mb3JtYXRpb24gZnJvbSB3aGF0IEkgdW5kZXJzdGFuZCBvZiB0aGlzIGJ1ZyBhcyB3ZWxs
Lg0KDQpPbiBUdWUsIFNlcCAyNywgMjAxNiBhdCAxMjoxMyBQTSwgQWxleCBXaWxsaWFtc29uIDxh
bGV4LndpbGxpYW1zb25AcmVkaGF0LmNvbT4gd3JvdGU6DQo+IE9uIFR1ZSwgMjcgU2VwIDIwMTYg
MTM6MTc6MDIgLTA1MDANCj4gQmpvcm4gSGVsZ2FhcyA8aGVsZ2Fhc0BrZXJuZWwub3JnPiB3cm90
ZToNCj4NCj4+IE9uIFN1biwgU2VwIDI1LCAyMDE2IGF0IDEwOjAyOjQzQU0gKzAzMDAsIE5lZnRp
biwgU2FzaGEgd3JvdGU6DQo+PiA+IE9uIDkvMjQvMjAxNiAxMjowNSBBTSwgSmVmZiBLaXJzaGVy
IHdyb3RlOg0KPj4gPiA+T24gRnJpLCAyMDE2LTA5LTIzIGF0IDA5OjAxIC0wNTAwLCBCam9ybiBI
ZWxnYWFzIHdyb3RlOg0KPj4gPiA+Pk9uIFRodSwgU2VwIDIyLCAyMDE2IGF0IDExOjM5OjAxUE0g
LTA3MDAsIEplZmYgS2lyc2hlciB3cm90ZToNCj4+ID4gPj4+RnJvbTogU2FzaGEgTmVmdGluIDxz
YXNoYS5uZWZ0aW5AaW50ZWwuY29tPg0KPj4gPiA+Pj4NCj4+ID4gPj4+ODI1NzkgaGFzIGEgcHJv
YmxlbSByZWF0dGFjaGluZyBpdHNlbGYgYWZ0ZXIgdGhlIGRldmljZSBpcyBkZXRhY2hlZC4NCj4+
ID4gPj4+VGhlIGJ1ZyB3YXMgcmVwb3J0ZWQgYnkgUmVkaGF0LiBUaGUgc3VnZ2VzdGVkIGZpeCBp
cyB0byBkaXNhYmxlIA0KPj4gPiA+Pj5GTFIgY2FwYWJpbGl0eSBpbiBQQ0llIGNvbmZpZ3VyYXRp
b24gc3BhY2UuDQo+PiA+ID4+Pg0KPj4gPiA+Pj5SZXByb2R1Y3Rpb246DQo+PiA+ID4+PkF0dGFj
aCB0aGUgZGV2aWNlIHRvIGEgVk0sIHRoZW4gZGV0YWNoIGFuZCB0cnkgdG8gYXR0YWNoIGFnYWlu
Lg0KPj4gPiA+Pj4NCj4+ID4gPj4+Rml4Og0KPj4gPiA+Pj5EaXNhYmxlIEZMUiBjYXBhYmlsaXR5
IHRvIHByZXZlbnQgdGhlIDgyNTc5IGZyb20gaGFuZ2luZy4NCj4+ID4gPj5JcyB0aGVyZSBhIGJ1
Z3ppbGxhIG9yIG90aGVyIHJlZmVyZW5jZSBVUkwgdG8gaW5jbHVkZSBoZXJlPyAgDQo+PiA+ID4+
U2hvdWxkIHRoaXMgYmUgbWFya2VkIGZvciBzdGFibGU/DQo+PiA+ID5TbyB0aGUgYXV0aG9yIGlz
IGluIElzcmFlbCwgbWVhbmluZyBpdCBpcyB0aGVpciB3ZWVrZW5kIG5vdy4gIEkgZG8gDQo+PiA+
ID5ub3QgYmVsaWV2ZSBTYXNoYSBtb25pdG9ycyBlbWFpbCBvdmVyIHRoZSB3ZWVrZW5kLCBzbyBh
IHJlc3BvbnNlIA0KPj4gPiA+dG8geW91ciBxdWVzdGlvbnMgd29uJ3QgaGFwcGVuIGZvciBhIGZl
dyBkYXlzLg0KPj4gPiA+DQo+PiA+ID5JIHRyaWVkIHNlYXJjaGluZyBteSBhcmNoaXZlcyBmb3Ig
bW9yZSBpbmZvcm1hdGlvbiwgYnV0IGhhZCBubyANCj4+ID4gPmx1Y2sgZmluZGluZyBhbnkgYWRk
aXRpb25hbCBpbmZvcm1hdGlvbi4NCj4+ID4gPg0KDQpJIGFncmVlIHRoYXQgd2UgZG8gcHJvYmFi
bHkgbmVlZCB0byB1cGRhdGUgdGhlIHBhdGNoIGRlc2NyaXB0aW9uIHNpbmNlIGl0IGlzbid0IGV4
YWN0bHkgY2xlYXIgd2hhdCB0aGlzIGlzIGZpeGluZyBvciB3aGF0IHdhcyBhY3R1YWxseSBicm9r
ZW4uDQoNCj4+ID4gPj4+U2lnbmVkLW9mZi1ieTogU2FzaGEgTmVmdGluIDxzYXNoYS5uZWZ0aW5A
aW50ZWwuY29tPg0KPj4gPiA+Pj5UZXN0ZWQtYnk6IEFhcm9uIEJyb3duIDxhYXJvbi5mLmJyb3du
QGludGVsLmNvbT4NCj4+ID4gPj4+U2lnbmVkLW9mZi1ieTogSmVmZiBLaXJzaGVyIDxqZWZmcmV5
LnQua2lyc2hlckBpbnRlbC5jb20+DQo+PiA+ID4+Pi0tLQ0KPj4gPiA+Pj4gIGRyaXZlcnMvcGNp
L3F1aXJrcy5jIHwgMjEgKysrKysrKysrKysrKysrKysrKysrDQo+PiA+ID4+PiAgMSBmaWxlIGNo
YW5nZWQsIDIxIGluc2VydGlvbnMoKykNCj4+ID4gPj4+DQo+PiA+ID4+PmRpZmYgLS1naXQgYS9k
cml2ZXJzL3BjaS9xdWlya3MuYyBiL2RyaXZlcnMvcGNpL3F1aXJrcy5jIGluZGV4IA0KPj4gPiA+
Pj40NGUwZmYzLi41OWZiYTZlIDEwMDY0NA0KPj4gPiA+Pj4tLS0gYS9kcml2ZXJzL3BjaS9xdWly
a3MuYw0KPj4gPiA+Pj4rKysgYi9kcml2ZXJzL3BjaS9xdWlya3MuYw0KPj4gPiA+Pj5AQCAtNDQz
MSwzICs0NDMxLDI0IEBAIHN0YXRpYyB2b2lkIHF1aXJrX2ludGVsX3FhdF92Zl9jYXAoc3RydWN0
IA0KPj4gPiA+Pj5wY2lfZGV2ICpwZGV2KQ0KPj4gPiA+Pj4gICAgICAgICB9DQo+PiA+ID4+PiAg
fQ0KPj4gPiA+Pj4gIERFQ0xBUkVfUENJX0ZJWFVQX0VBUkxZKFBDSV9WRU5ET1JfSURfSU5URUws
IDB4NDQzLCANCj4+ID4gPj4+cXVpcmtfaW50ZWxfcWF0X3ZmX2NhcCk7DQo+PiA+ID4+PisvKg0K
Pj4gPiA+Pj4rICogV29ya2Fyb3VuZCBGTFIgaXNzdWVzIGZvciA4MjU3OQ0KPj4gPiA+Pj4rICog
VGhpcyBjb2RlIGRpc2FibGVzIHRoZSBGTFIgKEZ1bmN0aW9uIExldmVsIFJlc2V0KSB2aWEgUENJ
ZSwgDQo+PiA+ID4+Pitpbg0KPj4gPiA+Pj5vcmRlcg0KPj4gPiA+Pj4rICogdG8gd29ya2Fyb3Vu
ZCBhIGJ1ZyBmb3VuZCB3aGlsZSB1c2luZyBkZXZpY2UgcGFzc3Rocm91Z2gsIA0KPj4gPiA+Pj4r
IHdoZXJlIHRoZQ0KPj4gPiA+Pj4rICogaW50ZXJmYWNlIHdvdWxkIGJlY29tZSBub24tcmVzcG9u
c2l2ZS4NCj4+ID4gPj4+KyAqIE5PVEU6IHRoZSBGTFIgYml0IGlzIFJlYWQvV3JpdGUgT25jZSAo
UldPKSBpbiBjb25maWcgc3BhY2UsIA0KPj4gPiA+Pj4rIHNvIGlmDQo+PiA+ID4+PisgKiB0aGUg
QklPUyBvciBrZXJuZWwgd3JpdGVzIHRoaXMgcmVnaXN0ZXIgKiB0aGVuIHRoaXMgDQo+PiA+ID4+
Pisgd29ya2Fyb3VuZCB3aWxsDQo+PiA+ID4+PisgKiBub3Qgd29yay4NCj4+ID4gPj5UaGlzIGRv
ZXNuJ3Qgc291bmQgbGlrZSBhIHJvb3QgY2F1c2UuICBJcyB0aGUgaXNzdWUgYSBoYXJkd2FyZSAN
Cj4+ID4gPj5lcnJhdHVtPyAgTGludXggUENJIGNvcmUgYnVnPyAgVkZJTyBidWc/ICBEZXZpY2Ug
ZmlybXdhcmUgYnVnPw0KPj4gPiA+Pg0KPj4gPiA+PlRoZSBjaGFuZ2Vsb2cgc3VnZ2VzdHMgdGhh
dCB0aGUgcHJvYmxlbSBvbmx5IGFmZmVjdHMgcGFzc3Rocm91Z2gsIA0KPj4gPiA+PndoaWNoIHN1
Z2dlc3RzIHNvbWUgc29ydCBvZiBrZXJuZWwgYnVnIHJlbGF0ZWQgdG8gaG93IHBhc3N0aHJvdWdo
IA0KPj4gPiA+PmlzIGltcGxlbWVudGVkLg0KPj4NCj4+IElmIHRoaXMgYnVnIGFmZmVjdHMgYWxs
IHNjZW5hcmlvcywgbm90IGp1c3QgcGFzc3Rocm91Z2gsIHRoZSANCj4+IGNoYW5nZWxvZyBzaG91
bGQgbm90IG1lbnRpb24gcGFzc3Rocm91Z2guDQo+Pg0KPj4gPiA+Pj4rICovDQo+PiA+ID4+Pitz
dGF0aWMgdm9pZCBxdWlya19pbnRlbF9mbHJfY2FwX2RpcyhzdHJ1Y3QgcGNpX2RldiAqZGV2KSB7
DQo+PiA+ID4+PisgICAgICAgIGludCBwb3MgPSBwY2lfZmluZF9jYXBhYmlsaXR5KGRldiwgUENJ
X0NBUF9JRF9BRik7DQo+PiA+ID4+PisgICAgICAgIGlmIChwb3MpIHsNCj4+ID4gPj4+KyAgICAg
ICAgICAgICAgICB1OCBjYXA7DQo+PiA+ID4+PisgICAgICAgICAgICAgICAgcGNpX3JlYWRfY29u
ZmlnX2J5dGUoZGV2LCBwb3MgKyBQQ0lfQUZfQ0FQLCAmY2FwKTsNCj4+ID4gPj4+KyAgICAgICAg
ICAgICAgICBjYXAgPSBjYXAgJiAoflBDSV9BRl9DQVBfRkxSKTsNCj4+ID4gPj4+KyAgICAgICAg
ICAgICAgICBwY2lfd3JpdGVfY29uZmlnX2J5dGUoZGV2LCBwb3MgKyBQQ0lfQUZfQ0FQLCBjYXAp
Ow0KPj4gPiA+Pj4rICAgICAgICB9DQo+PiA+ID4+Pit9DQo+PiA+ID4+PitERUNMQVJFX1BDSV9G
SVhVUF9FQVJMWShQQ0lfVkVORE9SX0lEX0lOVEVMLCAweDE1MDIsDQo+PiA+ID4+PnF1aXJrX2lu
dGVsX2Zscl9jYXBfZGlzKTsNCj4+ID4gPj4+K0RFQ0xBUkVfUENJX0ZJWFVQX0VBUkxZKFBDSV9W
RU5ET1JfSURfSU5URUwsIDB4MTUwMywNCj4+ID4gPj4+cXVpcmtfaW50ZWxfZmxyX2NhcF9kaXMp
Ow0KPj4gPiA+Pj4tLQ0KPj4gPiA+Pj4yLjcuNA0KPj4gPiA+Pj4NCj4+ID4gPj4+LS0NCj4+ID4g
Pj4+VG8gdW5zdWJzY3JpYmUgZnJvbSB0aGlzIGxpc3Q6IHNlbmQgdGhlIGxpbmUgInVuc3Vic2Ny
aWJlIA0KPj4gPiA+Pj5saW51eC1wY2kiIGluIHRoZSBib2R5IG9mIGEgbWVzc2FnZSB0byBtYWpv
cmRvbW9Admdlci5rZXJuZWwub3JnIA0KPj4gPiA+Pj5Nb3JlIG1ham9yZG9tbyBpbmZvIGF0ICAN
Cj4+ID4gPj4+aHR0cDovL3ZnZXIua2VybmVsLm9yZy9tYWpvcmRvbW8taW5mby5odG1sDQo+PiA+
DQo+PiA+IEhlbGxvLA0KPj4gPg0KPj4gPiBPcmlnaW5hbCBidWd6aWxsYSB0aHJlYWQgY291bGQg
YmUgZm91bmQgaGVyZToNCj4+ID4gaHR0cHM6Ly9idWd6aWxsYS5yZWRoYXQuY29tL3Nob3dfYnVn
LmNnaT9mb3JtYXQ9bXVsdGlwbGUmaWQ9OTY2ODQwDQo+Pg0KPj4gVGhhdCBidWd6aWxsYSBpcyBw
cml2YXRlIGFuZCBJIGNhbid0IHJlYWQgaXQuDQo+DQo+IEhtbSwgSSBjYW4sIGJ1dCBJIGRvbid0
IHNlZSBhbnl0aGluZyBpbiBpdCB0aGF0IHN1cHBvcnRzIHRoaXMuICBJcyANCj4gdGhhdCByZWFs
bHkgdGhlIHJpZ2h0IGJ6PyAgSXQncyB0aGUgcmlnaHQgaGFyZHdhcmUsIGJ1dCBoYXMgYWxsIHNv
cnRzIA0KPiBvZiBGVUQgYWJvdXQgdGhlIHZlcnNpb24gb2YgdmFyaW91cyBvdGhlciBjb21wb25l
bnRzIGluIHRoZSBzdGFjay4NCg0KSXQgbG9va3MgbGlrZSB3ZSBoYWQgYSBsb2NhbCBjb3B5IG9m
IHRoZSBidWd6aWxsYSBzYXZlZCBoZXJlLCB0aG91Z2ggaXQgb25seSBnb2VzIHVwIHRvIGNvbW1l
bnQgMTMgd2hpY2ggaXMgd2hlcmUgSSB0aGluayB3ZSBzdGFydGVkIHdvcmtpbmcgdGhpcyBvbiBv
dXIgc2lkZS4gIEkgYmVsaWV2ZSB3aGF0IHRoaXMgcGF0Y2ggaXMgYXR0ZW1wdGluZyB0byByZXNv
bHZlIGlzIHJlbGF0ZWQgdG8gY29tbWVudCA4IHdoZXJlIHRoZSBkcml2ZXIgcmV0dXJuZWQgInBy
b2JlIG9mDQowMDAwOjAwOjE5LjAgZmFpbGVkIHdpdGggZXJyb3Ig4oCQMiIgaW5zdGVhZCBvZiBj
b3JyZWN0bHkgcHJvYmluZyB0aGUgaW50ZXJmYWNlLg0KDQpTbyB0aGUgYnVnIGFzIHJlcG9ydGVk
IHdhcyB0aGF0IGUxMDAwZSBoYWQgYSBwcm9ibGVtIHJlYXR0YWNoaW5nIGl0c2VsZiB0byB0aGUg
UEhZIGFmdGVyIGl0IHdhcyBhdHRhY2hlZCB0byBhIFZNLiAgU2FzaGEsIHBsZWFzZSBmZWVsIGZy
ZWUgdG8gY29ycmVjdCBtZSBpZiBJIGhhdmUgdGhpcyBiaXQgd3JvbmcuICBJIGhhZCBiZWVuIHRv
bGQgdGhlIHByb2JsZW0gd2FzIHRoZSBGTFIgZnVuY3Rpb25hbGl0eSB3YXNuJ3QgZnVsbHkgaW1w
bGVtZW50ZWQgc28gdGhhdCB3YXMgd2h5IHRoZXkgd2VyZSB3YW50aW5nIHRvIGRlZmVhdHVyZSBp
dC4gIEkgaGFkIGFzc3VtZWQgdGhhdCB0aGVyZSB3YXMgc3VwcG9ydCBmb3IgYSByZXNldCBvbiBE
MC0+RDMtPkQwLCBidXQgSSBqdXN0IHJlYWxpemVkIHRoYXQgcHJvYmFibHkgaXNuJ3QgdGhlIGNh
c2Ugc2luY2UgaXQgbG9va3MgbGlrZSBteSBsb2NhbCBzeXN0ZW0gc2V0cyB0aGUgTm9Tb2Z0UlNU
IGJpdC4NCg0KPj4gPiBUaGlzIGlzIG91ciBIVyBidWcsIGV4aXN0IG9ubHkgaW4gODI1NzkgZGV2
aWNlcy4gTW9yZSBuZXcgZGV2aWNlcyANCj4+ID4gaGF2ZSBubyBzdWNoIHByb2JsZW0uIFdlIGhh
dmUgZm91bmQgcm9vdCBjYXVzZSBhbmQgc3VnZ2VzdGVkIHRoaXMgDQo+PiA+IHNvbHV0aW9uLg0K
Pj4NCj4+IElzIHRoZXJlIGFuIGVycmF0dW0geW91IGNhbiByZWZlcmVuY2U/DQo+Pg0KPj4gPiBU
aGlzIHNvbHV0aW9uIHNob3VsZCB3b3JrIGZvciBhIDk1JSBvZiBjYXNlcywgc28gSSBkbyBub3Qg
dGhpbmsgDQo+PiA+IHRoYXQgdGhpcyBpcyBmcmFnaWxlLiBGb3IgYW5vdGhlciBjYXNlcyBwb3Nz
aWJsZSBzb2x1dGlvbiBpcyBnZXQgdXAgDQo+PiA+IHdvcmtpbmcgc3lzdGVtIGFuZCBtYW51YWxs
eSBkaXNhYmxlIEZMUiwgYmVmb3JlIFZNIHN0YXJ0IHVzZSBvdXIgDQo+PiA+IGFkYXB0ZXIuDQo+
Pg0KPj4gSSBkb24ndCB0aGluayBhIDk1JSBzb2x1dGlvbiBpcyBzdWZmaWNpZW50LiAgQ2FuIHlv
dSB1c2UgdGhlDQo+PiBwY2lfZGV2X3NwZWNpZmljX3Jlc2V0KCkgZnJhbWV3b3JrIHRvIG1ha2Ug
YSAxMDAlIHNvbHV0aW9uPw0KDQpJIGNhbiB0cnkgd29ya2luZyB3aXRoIFNhc2hhIG9uIHRoaXMg
dG8gc2VlIHdoYXQgd2UgY2FuIGRvLg0KDQo+IFJpZ2h0LCBwbHVzIHdoZW4gdGhpcyBkb2VzIHdv
cmsgSSBzdXNwZWN0IGl0IHJlbW92ZXMgdGhlIG9uZSBtZWNoYW5pc20gDQo+IHdlIGhhdmUgdG8g
cmVzZXQgdGhlIGRldmljZSwgd2hpY2ggZGVwZW5kaW5nIG9uIGhvdyBvYnNjdXJlIHRoZSANCj4g
ZmFpbHVyZSBzY2VuYXJpbyBpcywgaXNuJ3QgYSBjbGVhciBjdXQgaW1wcm92ZW1lbnQgZm9yIGRl
dmljZSBhc3NpZ25tZW50Lg0KPiBUaGFua3MsDQo+DQo+IEFsZXgNCg0KSSdsbCB3b3JrIHdpdGgg
U2FzaGEgdG8gc2VlIHdoYXQgd2UgY2FuIGRvLiAgT2RkcyBhcmUgdGhlcmUgaXMgc29tZSBzb3J0
IG9mIHByb2JsZW0gYmV0d2VlbiB0aGUgTUFDL1BIWSB0aGF0IG5lZWRzIHRvIGJlIHJlc29sdmVk
IHdoZW4gd2UgcGVyZm9ybSB0aGUgZnVuY3Rpb24gbGV2ZWwgcmVzZXQgc28gd2Ugd2lsbCBwcm9i
YWJseSBuZWVkIHRvIGFkZCBjb2RlIHNvIHRoYXQgd2UgcmVzZXQgdGhlIHBhcnQgYW5kIHJlLWVz
dGFibGlzaCB0aGUgbGluayB3aXRoIHRoZSBQSFkgYWZ0ZXIgdGhlIHJlc2V0Lg0KDQotIEFsZXgN
Cg0KDQpIZWxsbywNCkkgd291bGQgbGlrZSBjbGFyaWZ5IGEgZmV3IHBvaW50cy4NCg0KMS4gODI1
NzkgY2xpZW50IGRldmljZXMgZG8gbm90IHN1cHBvcnQgZnVuY3Rpb25hbCBsZXZlbCByZXNldCAo
RkxSKS4gVW5mb3J0dW5hdGVseSwgaXQgYWR2ZXJ0aXNlcyBGTFIgY2FwYWJpbGl0eSAtICB0aGlz
IGlzIGEgYnVnLiBTbywgd2hlbiBWTSBzb2Z0d2FyZSBldmVuIGFjY2lkZW50YWxseSB0cmllcyB0
byBhY2Nlc3MgRkxSLCB0aGF0IGNhdXNlcyBvdXIgZGV2aWNlIHRvIGhhbmcuIFRvIGVsaW1pbmF0
ZSB0aGlzIHByb2JsZW0gd2UgZGVjaWRlZCB0byBkaXNhYmxlIEZMUiB2aWEgZHJpdmVycy9wY2kv
cXVpcmtzLmMgDQpJbiBwcmV2aW91cyBlLW1haWwgSSB3cm90ZSB0aGF0IHRoZSBzb2x1dGlvbiBz
aG91bGQgd29yayBmb3IgYSA5NSUgb2YgY2FzZXMuIENhc2VzIHdoZXJlIGl0IG1pZ2h0IG5vdCB3
b3JrIGlmIEJJT1Mgd3JpdGVzIEZMUiBjYXBhYmlsaXR5IGJlZm9yZSB0aGUgT1MgKEluIDgyNTc5
IGRldmljZSBGTFIgY2FwYWJpbGl0eSBpcyBSV08gLSByZWFkIHdyaXRlIG9uY2UpLiBCdXQgdGhp
cyBpcyBhIHZlcnkgd2VpcmQgc2NlbmFyaW8sIHNvIHByb2JhYmx5IHRoZSB3b3JrYXJvdW5kIHdp
bGwgd29yayBjbG9zZSB0byAxMDAlLiBBbHNvLCBJIHdvdWxkIGxpa2UgdG8gYWRkIHRoYXQgd2Ug
aGF2ZSBydW4gbG90cyBvZiB0ZXN0cyBvdmVyIDgyNTc5IGluIG91ciBsYWIgYW5kIGVuc3VyZWQg
dGhhdCBhbGwgZmxvd3Mgd29yayBwcm9wZXJseS4NCg0KMi4gRDAtPkQzLUQwIGZsb3cgaXMgbm90
IGFmZmVjdGVkIGFuZCBpcyBpbiBmYWN0IHN1cHBvcnRlZCBieSA4MjU3OS4NCg0KMy4gSSB3aWxs
IGxvb2sgZm9yIGFuIGVycmF0dW0gZXhwbGFpbmluZyB0aGlzLg0KDQpUaGFua3MsDQpTYXNoYQ0K
DQoNCg0KDQoNCg==

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
> 
> 
> 
> 
>