| Message ID | 20210415073338.22287-1-dinghao.liu@zju.edu.cn (mailing list archive) |
|---|---|
| State | Awaiting Upstream, archived |
| Headers | show |
| Series | [v3] clk: renesas: rcar-usb2-clock-sel: Fix error handling in rcar_usb2_clock_sel_probe | expand |
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
> 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
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 --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 = {
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(-)