From: Jim Quinlan <jim2101024@gmail.com>
To: linux-pci <linux-pci@vger.kernel.org>,
Nicolas Saenz Julienne <nsaenz@kernel.org>,
Bjorn Helgaas <bhelgaas@google.com>,
James Dutton <james.dutton@gmail.com>,
Cyril Brulebois <kibi@debian.org>,
bcm-kernel-feedback-list <bcm-kernel-feedback-list@broadcom.com>,
Jim Quinlan <jim2101024@gmail.com>,
Jim Quinlan <james.quinlan@broadcom.com>,
Thorsten Leemhuis <regressions@leemhuis.info>
Cc: "Florian Fainelli" <f.fainelli@gmail.com>,
"Lorenzo Pieralisi" <lorenzo.pieralisi@arm.com>,
"Rob Herring" <robh@kernel.org>,
"Krzysztof Wilczyński" <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 1/1] PCI: brcmstb: Fix regression regarding missing PCIe linkup
Date: Sun, 29 May 2022 12:52:14 -0400 [thread overview]
Message-ID: <CANCKTBvyUOHJgh1=kFdyRFxWX5D4POnn0wmHYj-eNUQEP6esvA@mail.gmail.com> (raw)
In-Reply-To: <20220528224423.7017-2-jim2101024@gmail.com>
On Sat, May 28, 2022 at 6:44 PM Jim Quinlan <jim2101024@gmail.com> wrote:
>
> commit 93e41f3fca3d ("PCI: brcmstb: Add control of subdevice voltage regulators")
> Signed-off-by: Jim Quinlan <jim2101024@gmail.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@gmail.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
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2022-05-29 16:54 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-05-28 22:44 [PATCH v2 0/1] PCI: brcmstb: Fix regression regarding missing PCIe linkup Jim Quinlan
2022-05-28 22:44 ` [PATCH v2 1/1] " Jim Quinlan
2022-05-29 1:15 ` Cyril Brulebois
2022-05-29 2:11 ` Jim Quinlan
2022-05-30 10:12 ` Stefan Wahren
2022-06-02 19:17 ` Cyril Brulebois
2022-06-02 20:05 ` Bjorn Helgaas
2022-06-04 20:59 ` Jim Quinlan
2022-05-29 16:52 ` Jim Quinlan [this message]
2022-05-29 17:36 ` Thorsten Leemhuis
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CANCKTBvyUOHJgh1=kFdyRFxWX5D4POnn0wmHYj-eNUQEP6esvA@mail.gmail.com' \
--to=jim2101024@gmail.com \
--cc=bcm-kernel-feedback-list@broadcom.com \
--cc=bhelgaas@google.com \
--cc=f.fainelli@gmail.com \
--cc=james.dutton@gmail.com \
--cc=james.quinlan@broadcom.com \
--cc=kibi@debian.org \
--cc=kw@linux.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=linux-rpi-kernel@lists.infradead.org \
--cc=lorenzo.pieralisi@arm.com \
--cc=nsaenz@kernel.org \
--cc=regressions@leemhuis.info \
--cc=robh@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).