lkml.org 
[lkml]   [2022]   [Jul]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2 3/6] PCI: brcmstb: Add "refusal mode" to preclude PCIe-induced CPU aborts
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

\
 
 \ /
  Last update: 2022-07-21 17:48    [W:0.092 / U:0.420 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site