Re: [PATCH v4 03/12] PCI: brcmstb: Use common error handling code in brcm_pcie_probe()

[Manivannan Sadhasivam]

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@xxxxxxxxxx> 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@xxxxxxxxxxxx>
> > > Reviewed-by: Stanimir Varbanov <svarbanov@xxxxxxx>
> > > Reviewed-by: Florian Fainelli <florian.fainelli@xxxxxxxxxxxx>
> > > ---
> > >  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

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