Re:Re: Re: [PATCH] clk/sprd: Hold reference returned by of_get_parent()

["Liang He" <windhl@126.com>]

At 2022-06-26 11:32:40, "Orson Zhai" <orsonzhai@gmail.com> wrote:
>Hi Liang,
>
>On Sat, Jun 25, 2022 at 11:34 PM Liang He <windhl@126.com> wrote:
>>
>>
>> At 2022-06-25 16:36:04, "Orson Zhai" <orsonzhai@gmail.com> wrote:
>> >Hi Liang,
>> >
>> >On Fri, Jun 24, 2022 at 6:39 PM Liang He <windhl@126.com> wrote:
>> >>
>> >> We should hold the reference returned by of_get_parent() and use it
>> >> to call of_node_out() for refcount balance.
>> >>
>> >> Fixes: f95e8c7923d1 ("clk: sprd: support to get regmap from parent node")
>> >> Signed-off-by: Liang He <windhl@126.com>
>> >> ---
>> >>  these bugs are found in 5.19rc2
>> >>  these bugs are confirmed not fixed by lore.kernel.org
>> >>  these bugs are compiled tested in 5.19rc2 with at91_dt_defconfig
>> >>
>> >>  drivers/clk/sprd/common.c | 16 +++++++++++-----
>> >>  1 file changed, 11 insertions(+), 5 deletions(-)
>> >>
>> >> diff --git a/drivers/clk/sprd/common.c b/drivers/clk/sprd/common.c
>> >> index d620bbbcdfc8..08c1d7a9ec8b 100644
>> >> --- a/drivers/clk/sprd/common.c
>> >> +++ b/drivers/clk/sprd/common.c
>> >> @@ -41,35 +41,41 @@ int sprd_clk_regmap_init(struct platform_device *pdev,
>> >>  {
>> >>         void __iomem *base;
>> >>         struct device *dev = &pdev->dev;
>> >> -       struct device_node *node = dev->of_node;
>> >> +       struct device_node *node = dev->of_node, *np;
>> >>         struct regmap *regmap;
>> >>
>> >> +       np = of_get_parent(dev->of_node);
>> >>         if (of_find_property(node, "sprd,syscon", NULL)) {
>> >>                 regmap = syscon_regmap_lookup_by_phandle(node, "sprd,syscon");
>> >
>> >Why not to call of_node_put() in this function?
>> >He returns an error and it is his responsibility to clean up
>> >everything he used, right?
>> >
>> >-Orson
>> >
>> Hi, Orson,
>>
>> I only know we should use the pair of functions (of_get_parent(), of_node_put)
>> to keep refcount balance.
>>
>> However, as I do not really know the semantic of other functions, so I cannot
>> give other kinds of patch.
>
>Sorry, I didn't catch your point before.
>You're right on this unbalance. But it has no direct relation to the
>function here.

Thanks for your understanding. And yes, it has no direct relation
 to the function here.

>I think it is better to minimize the effective range of of_get_parent()
>
>Could you refer to Pantelis's answer to the last question at [1].
>
>Thanks.
>-Orson
>
>[1] https://www.spinics.net/lists/devicetree/msg122555.html
>

Thanks, Orson.

Yes, I also wanted to make a better patch solution as the bug is only caused 
by the 'else if' branch.

However, I cannot find a better solution to fix the bug in the middle of nested 
'if-else if-else' brances unless we try to rewrite them, which is more difficult for me, now.

But if you think it is (semantic) safe to rewrite these branches, I can try it.

Thanks again for your relpy.

Liang 

>>
>> By the way, if we chose to put of_node_put() in this function, I think it also need
>> to put of_node_put() into 'device_node_to_regmap' as it also return error code in
>> following path.
>>
>> Anyway, thanks very much for your reply and review on my patch code.
>>
>> Liang
>>
>>
>> >>                 if (IS_ERR(regmap)) {
>> >>                         pr_err("%s: failed to get syscon regmap\n", __func__);
>> >> +                       of_node_put(np);
>> >>                         return PTR_ERR(regmap);
>> >>                 }
>> >> -       } else if (of_device_is_compatible(of_get_parent(dev->of_node),
>> >> -                          "syscon")) {
>> >> -               regmap = device_node_to_regmap(of_get_parent(dev->of_node));
>> >> +       } else if (of_device_is_compatible(np, "syscon")) {
>> >> +               regmap = device_node_to_regmap(np);
>> >>                 if (IS_ERR(regmap)) {
>> >> +                       of_node_put(np);
>> >>                         dev_err(dev, "failed to get regmap from its parent.\n");
>> >>                         return PTR_ERR(regmap);
>> >>                 }
>> >>         } else {
>> >>                 base = devm_platform_ioremap_resource(pdev, 0);
>> >> -               if (IS_ERR(base))
>> >> +               if (IS_ERR(base)) {
>> >> +                       of_node_put(np);
>> >>                         return PTR_ERR(base);
>> >> +               }
>> >>
>> >>                 regmap = devm_regmap_init_mmio(&pdev->dev, base,
>> >>                                                &sprdclk_regmap_config);
>> >>                 if (IS_ERR(regmap)) {
>> >> +                       of_node_put(np);
>> >>                         pr_err("failed to init regmap\n");
>> >>                         return PTR_ERR(regmap);
>> >>                 }
>> >>         }
>> >>
>> >> +       of_node_put(np);
>> >>         sprd_clk_set_regmap(desc, regmap);
>> >>
>> >>         return 0;
>> >> --
>> >> 2.25.1
>> >>