Message-ID: <CANCKTBvyUOHJgh1=kFdyRFxWX5D4POnn0wmHYj-eNUQEP6esvA@mail.gmail.com>
Date:   Sun, 29 May 2022 12:52:14 -0400
From:   Jim Quinlan <jim2101024@...il.com>
To:     linux-pci <linux-pci@...r.kernel.org>,
        Nicolas Saenz Julienne <nsaenz@...nel.org>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        James Dutton <james.dutton@...il.com>,
        Cyril Brulebois <kibi@...ian.org>,
        bcm-kernel-feedback-list <bcm-kernel-feedback-list@...adcom.com>,
        Jim Quinlan <jim2101024@...il.com>,
        Jim Quinlan <james.quinlan@...adcom.com>,
        Thorsten Leemhuis <regressions@...mhuis.info>
Cc:     Florian Fainelli <f.fainelli@...il.com>,
        Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        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 1/1] PCI: brcmstb: Fix regression regarding missing
 PCIe linkup

On Sat, May 28, 2022 at 6:44 PM Jim Quinlan <jim2101024@...il.com> wrote:
>
> commit 93e41f3fca3d ("PCI: brcmstb: Add control of subdevice voltage regulators")

> Signed-off-by: Jim Quinlan <jim2101024@...il.com>
> introduced a regression on the PCIe RPi4 Compute Module.  If the PCIe
> root port DT node described in [2] was missing, no linkup would be attempted,
> and subsequent accesses would cause a panic because this particular PCIe HW
> causes a CPU abort on illegal accesses (instead of returning 0xffffffff).
>
> We fix this by allowing the DT root port node to be missing, as it behaved
> before the original patchset messed things up.
>
> In addition, two small changes are made:
>
>   1. Having pci_subdev_regulators_remove_bus() call
>      regulator_bulk_free() in addtion to regulator_bulk_disable().
>   2. Having brcm_pcie_add_bus() return 0 if there is an
>      error in calling pci_subdev_regulators_add_bus().
>      Instead, we dev_err() and turn on our refusal mode instead.
>
> It would be best if this commit were tested by someone with a Rpi CM4
> platform, as that is how the regression was found.  I have only emulated
> the problem and fix on different platform.
>
> Note that a bisection identified
>
> commit 830aa6f29f07 ("PCI: brcmstb: Split brcm_pcie_setup() into two funcs")
>
> as the first failing commit.  This commit is a regression, but is unrelated
> and was fixed by a subsequent commit in the original patchset.
>
> [1] https://bugzilla.kernel.org/show_bug.cgi?id=215925
> [2] Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
>
> Fixes: 93e41f3fca3d ("PCI: brcmstb: Add control of subdevice voltage regulators")
> Fixes: 830aa6f29f07 ("PCI: brcmstb: Split brcm_pcie_setup() into two funcs")
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=215925
Thorston -- I forgot to replace the bugzilla link; I'll get it on V3.  -- Jim

> Signed-off-by: Jim Quinlan <jim2101024@...il.com>
> ---
>  drivers/pci/controller/pcie-brcmstb.c | 43 +++++++++++++++++++--------
>  1 file changed, 30 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index ba5c120816b2..0839325f79ab 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -540,29 +540,42 @@ static int pci_subdev_regulators_add_bus(struct pci_bus *bus)
>
>  static int brcm_pcie_add_bus(struct pci_bus *bus)
>  {
> -       struct device *dev = &bus->dev;
> -       struct brcm_pcie *pcie = (struct brcm_pcie *) bus->sysdata;
> +       struct brcm_pcie *pcie;
>         int ret;
>
> -       if (!dev->of_node || !bus->parent || !pci_is_root_bus(bus->parent))
> +       /*
> +        * Right now we only alloc/enable regulators and initiate pcie link
> +        * when under the root port bus of the current domain.  In the
> +        * future we may want to alloc/enable regulators under any port
> +        * device (e.g. a switch).
> +        */
> +       if (!bus->parent || !pci_is_root_bus(bus->parent))
>                 return 0;
>
>         ret = pci_subdev_regulators_add_bus(bus);
> -       if (ret)
> -               return ret;
> +       if (ret) {
> +               dev_err(pcie->dev, "failed to alloc/enable regulators\n");
> +               goto err;
> +       }
>
> -       /* Grab the regulators for suspend/resume */
> +       /* Save the regulators for RC suspend/resume */
> +       pcie = (struct brcm_pcie *) bus->sysdata;
>         pcie->sr = bus->dev.driver_data;
>
> +       /* Attempt PCIe link-up */
> +       if (brcm_pcie_linkup(pcie) == 0)
> +               return 0;
> +err:
>         /*
> -        * If we have failed linkup there is no point to return an error as
> -        * currently it will cause a WARNING() from pci_alloc_child_bus().
> -        * We return 0 and turn on the "refusal_mode" so that any further
> -        * accesses to the pci_dev just get 0xffffffff
> +        * If we have failed linkup or have an error when turning on
> +        * regulators, there is no point to return an error value to the
> +        * caller (pci_alloc_child_bus()) as it will summarily execute a
> +        * WARNING().  Instead, we turn on our "refusal_mode" and return 0
> +        * so that any further PCIe accesses succeed but do nothing (reads
> +        * return 0xffffffff).  If we do not turn on refusal mode, our
> +        * unforgiving PCIe HW will signal a CPU abort.
>          */
> -       if (brcm_pcie_linkup(pcie) != 0)
> -               pcie->refusal_mode = true;
> -
> +       pcie->refusal_mode = true;
>         return 0;
>  }
>
> @@ -570,13 +583,17 @@ static void pci_subdev_regulators_remove_bus(struct pci_bus *bus)
>  {
>         struct device *dev = &bus->dev;
>         struct subdev_regulators *sr = dev->driver_data;
> +       struct brcm_pcie *pcie;
>
>         if (!sr || !bus->parent || !pci_is_root_bus(bus->parent))
>                 return;
>
>         if (regulator_bulk_disable(sr->num_supplies, sr->supplies))
>                 dev_err(dev, "failed to disable regulators for downstream device\n");
> +       regulator_bulk_free(sr->num_supplies, sr->supplies);
>         dev->driver_data = NULL;
> +       pcie = (struct brcm_pcie *) bus->sysdata;
> +       pcie->sr = NULL;
>  }
>
>  /* Limits operation to a specific generation (1, 2, or 3) */
> --
> 2.17.1
>

Powered by blists - more mailing lists