Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 27 Nov 2018 09:08:30 -0700
From:   Alex Williamson <alex.williamson@redhat.com>
To:     Bjorn Helgaas <helgaas@kernel.org>
Cc:     Bharat Bhushan <bharat.bhushan@nxp.com>,
        "linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "bharatb.yadav@gmail.com" <bharatb.yadav@gmail.com>,
        David Daney <david.daney@cavium.com>,
        Jan Glauber <jglauber@cavium.com>,
        Maik Broemme <mbroemme@libmpq.org>,
        Chris Blake <chrisrblake93@gmail.com>
Subject: Re: [PATCH] PCI: Mark NXP LS1088 to avoid bus reset bus
Message-ID: <20181127090830.084fedf1@x1.home>
In-Reply-To: <20181127153356.GA112381@google.com>
References: <20181127083454.26560-1-Bharat.Bhushan@nxp.com>
        <20181127153356.GA112381@google.com>
Organization: Red Hat
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, 27 Nov 2018 09:33:56 -0600
Bjorn Helgaas <helgaas@kernel.org> wrote:

> [+cc David, Jan, Alex, Maik, Chris]
> 
> On Tue, Nov 27, 2018 at 08:46:33AM +0000, Bharat Bhushan wrote:
> > NXP (Freescale Vendor ID) LS1088 chips do not behave correctly after
> > bus reset with e1000e. Link state of device does not comes UP and so
> > config space never accessible again.  
> 
> Previous similar commits:
> 
>   822155100e58 ("PCI: Mark Cavium CN8xxx to avoid bus reset")
>   8e2e03179923 ("PCI: Mark Atheros AR9580 to avoid bus reset")
>   9ac0108c2bac ("PCI: Mark Atheros AR9485 and QCA9882 to avoid bus reset")
>   c3e59ee4e766 ("PCI: Mark Atheros AR93xx to avoid bus reset")
> 
> 1) Please make your subject match (remove the spurious "bus" at the
> end)
> 
> 2) This should probably be marked for stable (v3.14 and later, since
> the quirk itself appeared in v3.19 and marked for v3.14 and later
> stable kernels).  Maybe even mark it as "Fixes: c3e59ee4e766..." to
> connect it.
> 
> 3) The 1957:80c0 PCI ID doesn't appear in https://pci-ids.ucw.cz/; can
> you add it?
> 
> 4) Is there a hardware erratum for this?  If so, please include the
> URL here.
> 
> 5) Can you reproduce the problem using the same endpoint (e1000e) on a
> different system with a different bridge?
> 
> 6) Have you looked at this with a PCIe analyzer?  It would be very
> interesting to compare the boot-time or system reboot path with the
> individual bus reset path you're fixing.
> 
> Since there are several similar reports and they sometimes involve the
> same devices (both your patch and 822155100e58 mention e1000e), I'm a
> little suspicious that we're doing something wrong in the bus reset
> path.

I agree, entirely excluding bus resets is not something to be taken
lightly.  It's less than ideal for an endpoint and a fairly major
functional gap for a downstream port.  It should really be considered
a last resort.

> I think bus reset uses Secondary Bus Reset in the Bridge Control
> register.  That's a generic mechanism that I would expect to be pretty
> well-tested.  I suspect the BIOS probably uses it in the reboot path,
> and the device probably works after that.
> 
> So I wonder if the Linux delay isn't quite long enough, or our first
> access to the device isn't quite right, e.g., maybe there's some issue
> with the bus/device number capture (PCIe r4.0, sec 2.2.6.2).

Tweaking the delay would be a reasonable solution, though we are seeing
some issues where users with lots of assigned devices that require bus
resets experience long delays as vfio file descriptors are closed
sequentially on exit.  So perhaps we could flag downstream ports
requiring an extra delay, if that becomes a solution.  Your mention of
the bus/device number also reminds me of the issue we saw on
Threadripper where there were patches proposed to re-write the
secondary and subordinate bus numbers after reset.  AMD was able to
resolve that in a firmware update, but there could be something similar
occurring here. Thanks,

Alex

> > Signed-off-by: Bharat Bhushan <Bharat.Bhushan@nxp.com>
> > ---
> >  drivers/pci/quirks.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > index 4700d24e5d55..b9ae4e9f101a 100644
> > --- a/drivers/pci/quirks.c
> > +++ b/drivers/pci/quirks.c
> > @@ -3391,6 +3391,13 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_ATHEROS, 0x0033, quirk_no_bus_reset);
> >   */
> >  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_CAVIUM, 0xa100, quirk_no_bus_reset);
> >  
> > +/*
> > + * NXP (Freescale Vendor ID) LS1088 chips do not behave correctly after
> > + * bus reset. Link state of device does not comes UP and so config space
> > + * never accessible again.
> > + */
> > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_FREESCALE, 0x80c0, quirk_no_bus_reset);
> > +
> >  static void quirk_no_pm_reset(struct pci_dev *dev)
> >  {
> >  	/*
> > -- 
> > 2.19.1
> >