| Message ID | 20220622060807.4095040-1-windhl@126.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | mips/kernel: Add missing of_node_get() | expand |
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
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
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);
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(+)