[PATCH] mips/kernel: Add missing of_node_get()

[PATCH] mips/kernel: Add missing of_node_get()

[Liang He]

In mips_cpc_default_phys_base(), we need to add of_node_get() before
of_find_compatible_node() which will decrease the refcount of its
first arg.

Signed-off-by: Liang He <windhl@126.com>
---
 arch/mips/kernel/mips-cpc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/mips/kernel/mips-cpc.c b/arch/mips/kernel/mips-cpc.c
index 3e386f7e1545..7ba0ae5df882 100644
--- a/arch/mips/kernel/mips-cpc.c
+++ b/arch/mips/kernel/mips-cpc.c
@@ -25,6 +25,7 @@ phys_addr_t __weak mips_cpc_default_phys_base(void)
 	struct resource res;
 	int err;
 
+	of_node_get(of_root);
 	cpc_node = of_find_compatible_node(of_root, NULL, "mti,mips-cpc");
 	if (cpc_node) {
 		err = of_address_to_resource(cpc_node, 0, &res);
-- 
2.25.1

Re: [PATCH] mips/kernel: Add missing of_node_get()

[Geert Uytterhoeven]

Hi Liang,

CC devicetree

On Wed, Jun 22, 2022 at 8:08 AM Liang He <windhl@126.com> wrote:
> In mips_cpc_default_phys_base(), we need to add of_node_get() before
> of_find_compatible_node() which will decrease the refcount of its
> first arg.
>
> Signed-off-by: Liang He <windhl@126.com>

Thanks for your patch!

> --- a/arch/mips/kernel/mips-cpc.c
> +++ b/arch/mips/kernel/mips-cpc.c
> @@ -25,6 +25,7 @@ phys_addr_t __weak mips_cpc_default_phys_base(void)
>         struct resource res;
>         int err;
>
> +       of_node_get(of_root);

Adding this looks strange to me...

However, of_find_compatible_node() indeed calls of_node_put(from),
so your patch is correct.

However, when passed NULL as the from pointer, __of_find_all_nodes()
(expanded from for_each_of_allnodes_from()) turns this into of_root.
As of_find_compatible_node() still has the original (NULL) from
pointer, of_node_put(from) becomes a no-op.

>         cpc_node = of_find_compatible_node(of_root, NULL, "mti,mips-cpc");

Hence I think it would be better to change the above to

    cpc_node = of_find_compatible_node(NULL, NULL, "mti,mips-cpc");

instead, i.e. get rid of the explicit of_root handling?

>         if (cpc_node) {
>                 err = of_address_to_resource(cpc_node, 0, &res);

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re:Re: [PATCH] mips/kernel: Add missing of_node_get()

[Liang He]

At 2022-06-22 14:56:32, "Geert Uytterhoeven" <geert@linux-m68k.org> wrote:
>Hi Liang,
>
>CC devicetree
>
>On Wed, Jun 22, 2022 at 8:08 AM Liang He <windhl@126.com> wrote:
>> In mips_cpc_default_phys_base(), we need to add of_node_get() before
>> of_find_compatible_node() which will decrease the refcount of its
>> first arg.
>>
>> Signed-off-by: Liang He <windhl@126.com>
>
>Thanks for your patch!
>
>> --- a/arch/mips/kernel/mips-cpc.c
>> +++ b/arch/mips/kernel/mips-cpc.c
>> @@ -25,6 +25,7 @@ phys_addr_t __weak mips_cpc_default_phys_base(void)
>>         struct resource res;
>>         int err;
>>
>> +       of_node_get(of_root);
>
>Adding this looks strange to me...
>

Hi, this is also strange to me.

In fact, I also don't like add a special of_node_get() before I call of_find_xx() each time.

In fact, I have learned this error-pattern based on the following bug-fix:

https://lore.kernel.org/all/20200720152806.443262648@linuxfoundation.org/


>However, of_find_compatible_node() indeed calls of_node_put(from),
>so your patch is correct.
>

Thanks.

>However, when passed NULL as the from pointer, __of_find_all_nodes()
>(expanded from for_each_of_allnodes_from()) turns this into of_root.
>As of_find_compatible_node() still has the original (NULL) from
>pointer, of_node_put(from) becomes a no-op.
>
>>         cpc_node = of_find_compatible_node(of_root, NULL, "mti,mips-cpc");
>
>Hence I think it would be better to change the above to
>
>    cpc_node = of_find_compatible_node(NULL, NULL, "mti,mips-cpc");
>
>instead, i.e. get rid of the explicit of_root handling?
>
>>         if (cpc_node) {
>>                 err = of_address_to_resource(cpc_node, 0, &res);
>


Sorry, Geert.

As I don't know anyother details of the code except the error-pattern code,
I cannot do anyother (more efficient) changes.

If it is a bug, please fix it in your professional way if my patch is not so well.

Thanks.

Liang.

>Gr{oetje,eeting}s,
>
>                        Geert
>
>--
>Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
>In personal conversations with technical people, I call myself a hacker. But
>when I'm talking to journalists I just say "programmer" or something like that.
>                                -- Linus Torvalds