diff mbox series

[v3] clk: renesas: rcar-usb2-clock-sel: Fix error handling in rcar_usb2_clock_sel_probe

Message ID 20210415073338.22287-1-dinghao.liu@zju.edu.cn (mailing list archive)
State Under Review
Delegated to: Geert Uytterhoeven
Headers show
Series [v3] clk: renesas: rcar-usb2-clock-sel: Fix error handling in rcar_usb2_clock_sel_probe | expand

Commit Message

Dinghao Liu April 15, 2021, 7:33 a.m. UTC
The error handling paths after pm_runtime_get_sync() has no
refcount decrement, which leads to refcount leak.

Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn>
---

Changelog:

v2: - Move the position of pm_runtime_enable,_get_sync().
      Use devm_clk_register() to simplify error handling.

v2: - Use devm_clk_hw_register() instead of devm_clk_register().
---
 drivers/clk/renesas/rcar-usb2-clock-sel.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

Comments

Geert Uytterhoeven April 23, 2021, 7:46 a.m. UTC | #1
Hi Dinghao,

On Thu, Apr 15, 2021 at 9:33 AM Dinghao Liu <dinghao.liu@zju.edu.cn> wrote:
> The error handling paths after pm_runtime_get_sync() has no
> refcount decrement, which leads to refcount leak.
>
> Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn>
> ---
>
> Changelog:
>
> v2: - Move the position of pm_runtime_enable,_get_sync().
>       Use devm_clk_register() to simplify error handling.

Thanks for the update!

> --- a/drivers/clk/renesas/rcar-usb2-clock-sel.c
> +++ b/drivers/clk/renesas/rcar-usb2-clock-sel.c
> @@ -131,7 +131,6 @@ static int rcar_usb2_clock_sel_remove(struct platform_device *pdev)
>         struct usb2_clock_sel_priv *priv = platform_get_drvdata(pdev);

    warning: unused variable ‘priv’ [-Wunused-variable]

Have you compiled this?

>
>         of_clk_del_provider(dev->of_node);
> -       clk_hw_unregister(&priv->hw);
>         pm_runtime_put(dev);
>         pm_runtime_disable(dev);
>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
i.e. will queue in renesas-clk-for-v5.14, with the above fixed.

Gr{oetje,eeting}s,

                        Geert
Dinghao Liu April 23, 2021, 8:07 a.m. UTC | #2
> Hi Dinghao,
> 
> On Thu, Apr 15, 2021 at 9:33 AM Dinghao Liu <dinghao.liu@zju.edu.cn> wrote:
> > The error handling paths after pm_runtime_get_sync() has no
> > refcount decrement, which leads to refcount leak.
> >
> > Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn>
> > ---
> >
> > Changelog:
> >
> > v2: - Move the position of pm_runtime_enable,_get_sync().
> >       Use devm_clk_register() to simplify error handling.
> 
> Thanks for the update!
> 
> > --- a/drivers/clk/renesas/rcar-usb2-clock-sel.c
> > +++ b/drivers/clk/renesas/rcar-usb2-clock-sel.c
> > @@ -131,7 +131,6 @@ static int rcar_usb2_clock_sel_remove(struct platform_device *pdev)
> >         struct usb2_clock_sel_priv *priv = platform_get_drvdata(pdev);
> 
>     warning: unused variable ‘priv’ [-Wunused-variable]
> 
> Have you compiled this?
> 

This is my carelessness, thanks for pointing out this. When we use 
devm_clk_hw_register(), we will not need to unregister priv->hw in 
rcar_usb2_clock_sel_remove(). So I think it's okay to remove 
platform_get_drvdata() in it to eliminate this warning.

Do you need a new version of patch to fix this warning?

Regards,
Dinghao
Geert Uytterhoeven April 23, 2021, 8:15 a.m. UTC | #3
Hi Dinghao,

On Fri, Apr 23, 2021 at 10:08 AM <dinghao.liu@zju.edu.cn> wrote:
> > On Thu, Apr 15, 2021 at 9:33 AM Dinghao Liu <dinghao.liu@zju.edu.cn> wrote:
> > > The error handling paths after pm_runtime_get_sync() has no
> > > refcount decrement, which leads to refcount leak.
> > >
> > > Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn>
> > > ---
> > >
> > > Changelog:
> > >
> > > v2: - Move the position of pm_runtime_enable,_get_sync().
> > >       Use devm_clk_register() to simplify error handling.
> >
> > Thanks for the update!
> >
> > > --- a/drivers/clk/renesas/rcar-usb2-clock-sel.c
> > > +++ b/drivers/clk/renesas/rcar-usb2-clock-sel.c
> > > @@ -131,7 +131,6 @@ static int rcar_usb2_clock_sel_remove(struct platform_device *pdev)
> > >         struct usb2_clock_sel_priv *priv = platform_get_drvdata(pdev);
> >
> >     warning: unused variable ‘priv’ [-Wunused-variable]
> >
> > Have you compiled this?
> >
>
> This is my carelessness, thanks for pointing out this. When we use
> devm_clk_hw_register(), we will not need to unregister priv->hw in
> rcar_usb2_clock_sel_remove(). So I think it's okay to remove
> platform_get_drvdata() in it to eliminate this warning.
>
> Do you need a new version of patch to fix this warning?

I'll remove that line while applying, so no need to fix and resend.

Gr{oetje,eeting}s,

                        Geert
diff mbox series

Patch

diff --git a/drivers/clk/renesas/rcar-usb2-clock-sel.c b/drivers/clk/renesas/rcar-usb2-clock-sel.c
index 3abafd78f7c8..a6f82a5a6335 100644
--- a/drivers/clk/renesas/rcar-usb2-clock-sel.c
+++ b/drivers/clk/renesas/rcar-usb2-clock-sel.c
@@ -131,7 +131,6 @@  static int rcar_usb2_clock_sel_remove(struct platform_device *pdev)
 	struct usb2_clock_sel_priv *priv = platform_get_drvdata(pdev);
 
 	of_clk_del_provider(dev->of_node);
-	clk_hw_unregister(&priv->hw);
 	pm_runtime_put(dev);
 	pm_runtime_disable(dev);
 
@@ -164,9 +163,6 @@  static int rcar_usb2_clock_sel_probe(struct platform_device *pdev)
 	if (IS_ERR(priv->rsts))
 		return PTR_ERR(priv->rsts);
 
-	pm_runtime_enable(dev);
-	pm_runtime_get_sync(dev);
-
 	clk = devm_clk_get(dev, "usb_extal");
 	if (!IS_ERR(clk) && !clk_prepare_enable(clk)) {
 		priv->extal = !!clk_get_rate(clk);
@@ -183,6 +179,8 @@  static int rcar_usb2_clock_sel_probe(struct platform_device *pdev)
 		return -ENOENT;
 	}
 
+	pm_runtime_enable(dev);
+	pm_runtime_get_sync(dev);
 	platform_set_drvdata(pdev, priv);
 	dev_set_drvdata(dev, priv);
 
@@ -193,11 +191,20 @@  static int rcar_usb2_clock_sel_probe(struct platform_device *pdev)
 	init.num_parents = 0;
 	priv->hw.init = &init;
 
-	clk = clk_register(NULL, &priv->hw);
-	if (IS_ERR(clk))
-		return PTR_ERR(clk);
+	ret = devm_clk_hw_register(NULL, &priv->hw);
+	if (ret)
+		goto pm_put;
+
+	ret = of_clk_add_hw_provider(np, of_clk_hw_simple_get, &priv->hw);
+	if (ret)
+		goto pm_put;
+
+	return 0;
 
-	return of_clk_add_hw_provider(np, of_clk_hw_simple_get, &priv->hw);
+pm_put:
+	pm_runtime_put(dev);
+	pm_runtime_disable(dev);
+	return ret;
 }
 
 static const struct dev_pm_ops rcar_usb2_clock_sel_pm_ops = {