linux-kernel - Re: [PATCH v4 03/12] PCI: brcmstb: Use common error handling code in brcm_pcie

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: <20240726050423.GB2628@thinkpad>
Date: Fri, 26 Jul 2024 10:34:23 +0530
From: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
To: Jim Quinlan <james.quinlan@...adcom.com>
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>,
	Krzysztof Kozlowski <krzk@...nel.org>,
	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>,
	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 v4 03/12] PCI: brcmstb: Use common error handling code in
 brcm_pcie_probe()

On Thu, Jul 25, 2024 at 03:45:59PM -0400, Jim Quinlan wrote:
> On Thu, Jul 25, 2024 at 12:31 AM Manivannan Sadhasivam
> <manivannan.sadhasivam@...aro.org> wrote:
> >
> > On Tue, Jul 16, 2024 at 05:31:18PM -0400, Jim Quinlan wrote:
> > > o Move the clk_prepare_enable() below the resource allocations.
> > > o Add a jump target (clk_out) so that a bit of exception handling can be
> > >   better reused at the end of this function implementation.
> > >
> > > Signed-off-by: Jim Quinlan <james.quinlan@...adcom.com>
> > > Reviewed-by: Stanimir Varbanov <svarbanov@...e.de>
> > > Reviewed-by: Florian Fainelli <florian.fainelli@...adcom.com>
> > > ---
> > >  drivers/pci/controller/pcie-brcmstb.c | 29 +++++++++++++++------------
> > >  1 file changed, 16 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> > > index c08683febdd4..c257434edc08 100644
> > > --- a/drivers/pci/controller/pcie-brcmstb.c
> > > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > > @@ -1613,31 +1613,30 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> > >
> > >       pcie->ssc = of_property_read_bool(np, "brcm,enable-ssc");
> > >
> > > -     ret = clk_prepare_enable(pcie->clk);
> > > -     if (ret) {
> > > -             dev_err(&pdev->dev, "could not enable clock\n");
> > > -             return ret;
> > > -     }
> > >       pcie->rescal = devm_reset_control_get_optional_shared(&pdev->dev, "rescal");
> > > -     if (IS_ERR(pcie->rescal)) {
> > > -             clk_disable_unprepare(pcie->clk);
> > > +     if (IS_ERR(pcie->rescal))
> > >               return PTR_ERR(pcie->rescal);
> > > -     }
> > > +
> > >       pcie->perst_reset = devm_reset_control_get_optional_exclusive(&pdev->dev, "perst");
> > > -     if (IS_ERR(pcie->perst_reset)) {
> > > -             clk_disable_unprepare(pcie->clk);
> > > +     if (IS_ERR(pcie->perst_reset))
> > >               return PTR_ERR(pcie->perst_reset);
> > > +
> > > +     ret = clk_prepare_enable(pcie->clk);
> > > +     if (ret) {
> > > +             dev_err(&pdev->dev, "could not enable clock\n");
> > > +             return ret;
> > >       }
> > >
> > >       ret = reset_control_reset(pcie->rescal);
> > > -     if (ret)
> > > +     if (ret) {
> > >               dev_err(&pdev->dev, "failed to deassert 'rescal'\n");
> > > +             goto clk_out;
> >
> > Please use a descriptive name for the err labels. Here this err path disables
> > and unprepares the clk, so use 'clk_disable_unprepare'.
> ack
> >
> > > +     }
> > >
> > >       ret = brcm_phy_start(pcie);
> > >       if (ret) {
> > >               reset_control_rearm(pcie->rescal);
> > > -             clk_disable_unprepare(pcie->clk);
> > > -             return ret;
> > > +             goto clk_out;
> > >       }
> > >
> > >       ret = brcm_pcie_setup(pcie);
> > > @@ -1676,6 +1675,10 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> > >
> > >       return 0;
> > >
> > > +clk_out:
> > > +     clk_disable_unprepare(pcie->clk);
> > > +     return ret;
> > > +
> >
> > This is leaking the resources. Move this new label below 'fail'.
> What resources is it leaking?  At "clk_out" the return value will be negative
> and only managed resources have been allocated at that juncture.
> 

Right, but what about the err path below this one? If that path is taken, then
clks won't be released, right?

It is not a good design to return from each err labels. There should be only one
return for all err labels at the end and those labels need to be in reverse
order w.r.t the actual code.

- Mani

-- 
மணிவண்ணன் சதாசிவம்