linux-kernel - Re: [PATCH v1 2/6] PCI: brcmstb: Fix error path upon call of regulator_bulk

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CA+-6iNwws3On9-VJ-vF55o5LwKYpOHtfRYLtoQ5=bh7uGTfYkg@mail.gmail.com>
Date: Thu, 6 Feb 2025 13:22:54 -0500
From: Jim Quinlan <james.quinlan@...adcom.com>
To: Bjorn Helgaas <helgaas@...nel.org>
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>, Stanimir Varbanov <svarbanov@...e.de>, 
	bcm-kernel-feedback-list@...adcom.com, jim2101024@...il.com, 
	Florian Fainelli <florian.fainelli@...adcom.com>, Lorenzo Pieralisi <lpieralisi@...nel.org>, 
	Krzysztof Wilczyński <kw@...ux.com>, 
	Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>, Rob Herring <robh@...nel.org>, 
	"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 v1 2/6] PCI: brcmstb: Fix error path upon call of regulator_bulk_get()

On Thu, Feb 6, 2025 at 12:29 PM Bjorn Helgaas <helgaas@...nel.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@...adcom.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

Download attachment "smime.p7s" of type "application/pkcs7-signature" (4210 bytes)