[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CA+-6iNyUB4es01xsSvsvS0gZyUuuttJJ4XSBVajtBrVNFT5dMw@mail.gmail.com>
Date: Thu, 21 Jul 2022 10:53:54 -0400
From: Jim Quinlan <james.quinlan@...adcom.com>
To: Bjorn Helgaas <helgaas@...nel.org>
Cc: Jim Quinlan <jim2101024@...il.com>,
"open list:PCI NATIVE HOST BRIDGE AND ENDPOINT DRIVERS"
<linux-pci@...r.kernel.org>,
Nicolas Saenz Julienne <nsaenz@...nel.org>,
Bjorn Helgaas <bhelgaas@...gle.com>,
Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
Cyril Brulebois <kibi@...ian.org>,
"maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE"
<bcm-kernel-feedback-list@...adcom.com>,
Florian Fainelli <f.fainelli@...il.com>,
Lorenzo Pieralisi <lpieralisi@...nel.org>,
Rob Herring <robh@...nel.org>,
Krzysztof WilczyĆski <kw@...ux.com>,
"moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE"
<linux-rpi-kernel@...ts.infradead.org>,
"moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE"
<linux-arm-kernel@...ts.infradead.org>,
open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 3/6] PCI: brcmstb: Add "refusal mode" to preclude
PCIe-induced CPU aborts
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@...nel.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@...il.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.
Regards,
Jim Quinlan
Broadcom STB
[1] https://lore.kernel.org/linux-pci/20171215201434.GY30595@bhelgaas-glaptop.roam.corp.google.com/
>
> cdns_pci_map_bus()
> dw_pcie_other_conf_map_bus()
> nwl_pcie_map_bus() (see nwl_pcie_valid_device())
> xilinx_pcie_map_bus() (see xilinx_pcie_valid_device())
>
> If you can implement this the same way, i.e., using
> brcm_pcie_link_up(), it would be nice.
>
> > /* For devices, write to the config space index register */
> > idx = PCIE_ECAM_OFFSET(bus->number, devfn, 0);
> > writel(idx, pcie->base + PCIE_EXT_CFG_INDEX);
> > @@ -704,6 +718,11 @@ static void __iomem *brcm_pcie_map_conf32(struct pci_bus *bus, unsigned int devf
> > if (pci_is_root_bus(bus))
> > return PCI_SLOT(devfn) ? NULL : base + (where & ~0x3);
> >
> > + if (pcie->refusal_mode) {
> > + /* See note above in brcm_pcie_map_conf() */
> > + return NULL;
> > + }
> > +
> > /* For devices, write to the config space index register */
> > idx = PCIE_ECAM_OFFSET(bus->number, devfn, (where & ~3));
> > writel(idx, base + IDX_ADDR(pcie));
> > @@ -989,6 +1008,7 @@ static int brcm_pcie_start_link(struct brcm_pcie *pcie)
> > dev_err(dev, "link down\n");
> > return -ENODEV;
> > }
> > + pcie->refusal_mode = false;
> >
> > if (!brcm_pcie_rc_mode(pcie)) {
> > dev_err(dev, "PCIe misconfigured; is in EP mode\n");
> > @@ -1134,6 +1154,8 @@ static void brcm_pcie_turn_off(struct brcm_pcie *pcie)
> > void __iomem *base = pcie->base;
> > int tmp;
> >
> > + pcie->refusal_mode = true;
> > +
> > if (brcm_pcie_link_up(pcie))
> > brcm_pcie_enter_l23(pcie);
> > /* Assert fundamental reset */
> > @@ -1185,6 +1207,7 @@ static int brcm_pcie_resume(struct device *dev)
> > u32 tmp;
> > int ret;
> >
> > + pcie->refusal_mode = true;
> > base = pcie->base;
> > ret = clk_prepare_enable(pcie->clk);
> > if (ret)
> > @@ -1361,6 +1384,7 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> > pcie->type = data->type;
> > pcie->perst_set = data->perst_set;
> > pcie->bridge_sw_init_set = data->bridge_sw_init_set;
> > + pcie->refusal_mode = true;
> >
> > pcie->base = devm_platform_ioremap_resource(pdev, 0);
> > if (IS_ERR(pcie->base))
> > --
> > 2.17.1
> >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@...ts.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Download attachment "smime.p7s" of type "application/pkcs7-signature" (4210 bytes)
Powered by blists - more mailing lists