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

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

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.

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
>>