Message-ID: <20220720220558.GA1661469@bhelgaas>
Date:   Wed, 20 Jul 2022 17:05:58 -0500
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     Jim Quinlan <jim2101024@...il.com>
Cc:     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>,
        bcm-kernel-feedback-list@...adcom.com, james.quinlan@...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

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?

  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

Powered by blists - more mailing lists