Date:   Tue, 27 Sep 2016 17:26:50 -0700
From:   Alexander Duyck <alexander.duyck@...il.com>
To:     Alex Williamson <alex.williamson@...hat.com>
Cc:     Bjorn Helgaas <helgaas@...nel.org>,
        "Neftin, Sasha" <sasha.neftin@...el.com>,
        Jeff Kirsher <jeffrey.t.kirsher@...el.com>,
        "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
        David Miller <davem@...emloft.net>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Netdev <netdev@...r.kernel.org>,
        Neil Horman <nhorman@...hat.com>, sassmann@...hat.com,
        John Greene <jogreene@...hat.com>, guru.anbalagane@...cle.com
Subject: Re: [net-next 5/5] PCI: disable FLR for 82579 device

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@...hat.com> wrote:
> On Tue, 27 Sep 2016 13:17:02 -0500
> Bjorn Helgaas <helgaas@...nel.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@...el.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@...el.com>
>> > >>>Tested-by: Aaron Brown <aaron.f.brown@...el.com>
>> > >>>Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@...el.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@...r.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

Powered by blists - more mailing lists