Re: [PATCH v1 2/6] PCI: brcmstb: Fix error path upon call of regulator_bulk_get()

On Thu, Feb 06, 2025 at 01:22:54PM -0500, Jim Quinlan wrote:

On Thu, Feb 6, 2025 at 12:29 PM Bjorn Helgaas [off-list ref] wrote:

quoted

On Wed, Feb 05, 2025 at 02:12:02PM -0500, Jim Quinlan wrote:

quoted

If regulator_bulk_get() returns an error, no regulators are
created and we need to set their number to zero.  If we do
not do this and the PCIe link-up fails, regulator_bulk_free()
will be invoked and effect a panic.

Also print out the error value, as we cannot return an error
upwards as Linux will WARN on an error from add_bus().

quoted

quoted

Fixes: 9e6be018b263 ("PCI: brcmstb: Enable child bus device regulators from DT")
Signed-off-by: Jim Quinlan <redacted>
---
 drivers/pci/controller/pcie-brcmstb.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index f8fc3d620ee2..bf919467cbcd 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c

@@ -1417,7 +1417,8 @@ static int brcm_pcie_add_bus(struct pci_bus *bus)

              ret = regulator_bulk_get(dev, sr->num_supplies, sr->supplies);
              if (ret) {
-                     dev_info(dev, "No regulators for downstream device\n");
+                     dev_info(dev, "Did not get regulators; err=%d\n", ret);
+                     sr->num_supplies = 0;
                      goto no_regulators;

I think it might have been better if we could do the
regulator_bulk_get() separately, before pci_host_probe(), so that if
this error happens, we can deal with it more easily.

Keep in mind that brcm_pcie_add_bus() cannot return a non-zero error
code, as it will get a WARN() from the caller if it does.

Would you  accept deallocating the "sr" array and setting it to NULL
like the following error condition does?  In that way we would not be
invoking any regulator_bulk_xxxx() functions with nr==0.  I'm really
wary of moving things around...

Yeah, don't move anything around right now.  My wondering is just
about whether the alloc and bulk_get() could be done earlier, leaving
only the bulk_enable() to be done in brcm_pcie_add_bus().

The alloc and bulk_get() depend on DT things and it's nice to catch
those errors earlier.

Bjorn