Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 21 Jul 2022 10:46:27 -0500
From:   Bjorn Helgaas <helgaas@kernel.org>
To:     Jim Quinlan <james.quinlan@broadcom.com>
Cc:     Jim Quinlan <jim2101024@gmail.com>,
        "open list:PCI NATIVE HOST BRIDGE AND ENDPOINT DRIVERS" 
        <linux-pci@vger.kernel.org>,
        Nicolas Saenz Julienne <nsaenz@kernel.org>,
        Bjorn Helgaas <bhelgaas@google.com>,
        Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
        Cyril Brulebois <kibi@debian.org>,
        "maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE" 
        <bcm-kernel-feedback-list@broadcom.com>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Lorenzo Pieralisi <lpieralisi@kernel.org>,
        Rob Herring <robh@kernel.org>,
        Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= <kw@linux.com>,
        "moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE" 
        <linux-rpi-kernel@lists.infradead.org>,
        "moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE" 
        <linux-arm-kernel@lists.infradead.org>,
        open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 3/6] PCI: brcmstb: Add "refusal mode" to preclude
 PCIe-induced CPU aborts
Message-ID: <20220721154627.GA1722428@bhelgaas>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CA+-6iNyUB4es01xsSvsvS0gZyUuuttJJ4XSBVajtBrVNFT5dMw@mail.gmail.com>
Precedence: bulk

On Thu, Jul 21, 2022 at 10:53:54AM -0400, Jim Quinlan wrote:
> https://lore.kernel.org/linux-pci/20171215201434.GY30595@bhelgaas-glaptop.roam.corp.google.com/
> On Wed, Jul 20, 2022 at 6:06 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Sat, Jul 16, 2022 at 06:24:50PM -0400, Jim Quinlan wrote:
> > > Our PCIe RC HW has an atypical behavior: if it does not have PCIe link
> > > established between itself and downstream, any subsequent config space
> > > access causes a CPU abort.  This commit sets a "refusal mode" if the PCIe
> > > link-up fails, and this has our pci_ops map_bus function returning a NULL
> > > address, which in turn precludes the access from happening.
> > >
> > > Right now, "refusal mode" is window dressing.  It will become relevant
> > > in a future commit when brcm_pcie_start_link() is invoked during
> > > enumeration instead of before it.
> > >
> > > Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
> > > ---
> > >  drivers/pci/controller/pcie-brcmstb.c | 24 ++++++++++++++++++++++++
> > >  1 file changed, 24 insertions(+)
> > >
> > > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> > > index c026446d5830..72219a4f3964 100644
> > > --- a/drivers/pci/controller/pcie-brcmstb.c
> > > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > > @@ -255,6 +255,7 @@ struct brcm_pcie {
> > >       u32                     hw_rev;
> > >       void                    (*perst_set)(struct brcm_pcie *pcie, u32 val);
> > >       void                    (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
> > > +     bool                    refusal_mode;
> > >  };
> > >
> > >  static inline bool is_bmips(const struct brcm_pcie *pcie)
> > > @@ -687,6 +688,19 @@ static void __iomem *brcm_pcie_map_conf(struct pci_bus *bus, unsigned int devfn,
> > >       if (pci_is_root_bus(bus))
> > >               return PCI_SLOT(devfn) ? NULL : base + where;
> > >
> > > +     if (pcie->refusal_mode) {
> > > +             /*
> > > +              * At this point we do not have PCIe link-up.  If there is
> > > +              * a config read or write access besides those targeting
> > > +              * the host bridge, our PCIe HW throws a CPU abort.  To
> > > +              * prevent this we return the NULL address.  The calling
> > > +              * functions -- pci_generic_config_*() -- will notice this
> > > +              * and not perform the access, and if it is a read access,
> > > +              * 0xffffffff is returned.
> > > +              */
> > > +             return NULL;
> > > +     }
> >
> > Is this any different from all the other .map_bus() implementations
> > that return NULL when the link is down?
> 
> Not really, but long ago I submitted code that gated the config spec
> access based on link status and was advised not to do it  [1].
> I'll be happy to make it look like the others.
>
> [1] https://lore.kernel.org/linux-pci/20171215201434.GY30595@bhelgaas-glaptop.roam.corp.google.com/

My point there was that if you can deal with the abort cleanly, that's
the best approach.  Apparently brcmstb can't recover cleanly, so you
have to settle for the 99% solution.

The refusal_mode approach has the same race as checking
*_pcie_link_up(), since the link may go down between the time
brcm_pcie_start_link() sees that it is up and the time somebody does a
config access:

  brcm_pcie_start_link
    pcie->refusal_mode = false

  <link goes down>

  brcm_pcie_map_conf
    if (pcie->refusal_mode)            # still false

  <config access causes abort>

So there's no advantage in making the code look different.  Checking
for link-up in the config access path can never completely remove the
window, but it does make it smaller than using refusal_mode.

Bjorn