| Message ID | 20250205191213.29202-3-james.quinlan@broadcom.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: brcmstb: Misc small tweaks and fixes | expand |
On Wed, Feb 05, 2025 at 02:12:02PM -0500, Jim Quinlan wrote: > 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(). Wrap all these commit logs to fill 75 columns. No point in leaving unused space when most things are formatted to fill 80ish columns or more. > Fixes: 9e6be018b263 ("PCI: brcmstb: Enable child bus device regulators from DT") > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com> > --- > 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. Setting num_supplies = 0 is an unusual way of handling this error, and if this pattern of managing PCIe regulators spreads to other drivers, we might trip over this again. Not asking for a redesign here, and maybe it wouldn't even be possible, but it kind of fits with thinking about splitting Root Port support from the Root Complex/host bridge support. Bjorn
On Thu, Feb 6, 2025 at 12:29 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Wed, Feb 05, 2025 at 02:12:02PM -0500, Jim Quinlan wrote: > > 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(). > > Wrap all these commit logs to fill 75 columns. No point in leaving > unused space when most things are formatted to fill 80ish columns or > more. ack > > > Fixes: 9e6be018b263 ("PCI: brcmstb: Enable child bus device regulators from DT") > > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com> > > --- > > 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. Hi Bjorn, 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... > > Setting num_supplies = 0 is an unusual way of handling this error, and > if this pattern of managing PCIe regulators spreads to other drivers, > we might trip over this again. Understood. > > Not asking for a redesign here, and maybe it wouldn't even be > possible, but it kind of fits with thinking about splitting Root Port > support from the Root Complex/host bridge support. IIRC there was a submission having custom/modified port drivers owning regulators, but I don't think it got accepted. FWIW, we were considering switching to that. Regards, Jim Quinlan Broadcom STB/CM > > Bjorn
On Thu, Feb 06, 2025 at 01:22:54PM -0500, Jim Quinlan wrote: > On Thu, Feb 6, 2025 at 12:29 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Wed, Feb 05, 2025 at 02:12:02PM -0500, Jim Quinlan wrote: > > > 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(). > > > Fixes: 9e6be018b263 ("PCI: brcmstb: Enable child bus device regulators from DT") > > > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com> > > > --- > > > 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
On Thu, Feb 6, 2025 at 1:34 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Thu, Feb 06, 2025 at 01:22:54PM -0500, Jim Quinlan wrote: > > On Thu, Feb 6, 2025 at 12:29 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > > On Wed, Feb 05, 2025 at 02:12:02PM -0500, Jim Quinlan wrote: > > > > 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(). > > > > > Fixes: 9e6be018b263 ("PCI: brcmstb: Enable child bus device regulators from DT") > > > > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com> > > > > --- > > > > 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. IIRC, I believe the reason that this code is in brcm_pcie_{add,remove}_bus() is because the reviewers wanted the code outside of the RC driver, as it was the port driver that "owned' the regulator. I also vaguely recall one of my submissions that had generic pci_subdev_regulators_{add,remove}_bus() calls in pcie/bus.c, but was later redirected back to the brcmstb RC driver. Regards, Jim Quinlan Broadcom STB/CM > > Bjorn
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; }
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(). Fixes: 9e6be018b263 ("PCI: brcmstb: Enable child bus device regulators from DT") Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com> --- drivers/pci/controller/pcie-brcmstb.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)