| Message ID | 20240716213131.6036-4-james.quinlan@broadcom.com |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: brcnstb: Enable STB 7712 SOC | expand |
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@broadcom.com> > Reviewed-by: Stanimir Varbanov <svarbanov@suse.de> > Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.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'. > + } > > 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'. - Mani
On Thu, Jul 25, 2024 at 12:31 AM Manivannan Sadhasivam <manivannan.sadhasivam@linaro.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@broadcom.com> > > Reviewed-by: Stanimir Varbanov <svarbanov@suse.de> > > Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.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. Regards, Jim Quinlan Broadcom STB/CM > > - Mani > > -- > மணிவண்ணன் சதாசிவம்
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@linaro.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@broadcom.com> > > > Reviewed-by: Stanimir Varbanov <svarbanov@suse.de> > > > Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.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
On Fri, Jul 26, 2024 at 1:04 AM Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> wrote: > > 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@linaro.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@broadcom.com> > > > > Reviewed-by: Stanimir Varbanov <svarbanov@suse.de> > > > > Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.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? No, that is the same situation. The clock is originally allocated with "devm_clk_get_optional()", i.e. it is a managed resource. If the probe fails, and it does in both of these error paths, Linux deallocates the newly formed device structure and all of its resources. Perhaps I am missing something? > > 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. Agreed. Regards, Jim Quinlan Broadcom STB/CM > > - Mani > > -- > மணிவண்ணன் சதாசிவம்
On Fri, Jul 26, 2024 at 02:34:54PM -0400, Jim Quinlan wrote: > On Fri, Jul 26, 2024 at 1:04 AM Manivannan Sadhasivam > <manivannan.sadhasivam@linaro.org> wrote: > > > > 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@linaro.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@broadcom.com> > > > > > Reviewed-by: Stanimir Varbanov <svarbanov@suse.de> > > > > > Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.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? > No, that is the same situation. The clock is originally allocated > with "devm_clk_get_optional()", i.e. it is a managed resource. > If the probe fails, and it does in both of these error paths, > Linux deallocates the newly formed device structure and all of its resources. > Perhaps I am missing something? > No, I missed the fact that __brcm_pcie_remove() is freeing all resources. But grouping all release functions in a single helper and using it in multiple err paths even when the err path need not release everything the helper is releasing, warrants trouble. - Mani
On Sat, Jul 27, 2024 at 2:40 AM Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> wrote: > > On Fri, Jul 26, 2024 at 02:34:54PM -0400, Jim Quinlan wrote: > > On Fri, Jul 26, 2024 at 1:04 AM Manivannan Sadhasivam > > <manivannan.sadhasivam@linaro.org> wrote: > > > > > > 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@linaro.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@broadcom.com> > > > > > > Reviewed-by: Stanimir Varbanov <svarbanov@suse.de> > > > > > > Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.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? > > No, that is the same situation. The clock is originally allocated > > with "devm_clk_get_optional()", i.e. it is a managed resource. > > If the probe fails, and it does in both of these error paths, > > Linux deallocates the newly formed device structure and all of its resources. > > Perhaps I am missing something? > > > > No, I missed the fact that __brcm_pcie_remove() is freeing all resources. But > grouping all release functions in a single helper and using it in multiple err > paths even when the err path need not release everything the helper is > releasing, warrants trouble. Got it, I will address this. Regards, Jim Quinlan Broadcom STB/CM > > - Mani > > -- > மணிவண்ணன் சதாசிவம்
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; + } 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; + fail: __brcm_pcie_remove(pcie); return ret;