diff mbox

ARM: OMAP2+: Grab reference to device nodes where needed

Message ID 1486853948-20978-1-git-send-email-linux@roeck-us.net (mailing list archive)
State New, archived
Headers show

Commit Message

Guenter Roeck Feb. 11, 2017, 10:59 p.m. UTC
After commit 'of: fix of_node leak caused in of_find_node_opts_by_path',
the following error may be reported when running omap images.

OF: ERROR: Bad of_node_put() on /ocp@68000000
CPU: 0 PID: 0 Comm: swapper Not tainted 4.10.0-rc7-next-20170210 #1
Hardware name: Generic OMAP3-GP (Flattened Device Tree)
[<c0310604>] (unwind_backtrace) from [<c030bbf4>] (show_stack+0x10/0x14)
[<c030bbf4>] (show_stack) from [<c05add8c>] (dump_stack+0x98/0xac)
[<c05add8c>] (dump_stack) from [<c05af1b0>] (kobject_release+0x48/0x7c)
[<c05af1b0>] (kobject_release)
	from [<c0ad1aa4>] (of_find_node_by_name+0x74/0x94)
[<c0ad1aa4>] (of_find_node_by_name)
	from [<c1215bd4>] (omap3xxx_hwmod_is_hs_ip_block_usable+0x24/0x2c)
[<c1215bd4>] (omap3xxx_hwmod_is_hs_ip_block_usable) from
[<c1215d5c>] (omap3xxx_hwmod_init+0x180/0x274)
[<c1215d5c>] (omap3xxx_hwmod_init)
	from [<c120faa8>] (omap3_init_early+0xa0/0x11c)
[<c120faa8>] (omap3_init_early)
	from [<c120fb2c>] (omap3430_init_early+0x8/0x30)
[<c120fb2c>] (omap3430_init_early)
	from [<c1204710>] (setup_arch+0xc04/0xc34)
[<c1204710>] (setup_arch) from [<c1200948>] (start_kernel+0x68/0x38c)
[<c1200948>] (start_kernel) from [<8020807c>] (0x8020807c)

of_find_node_by_name() drops the reference to the passed device node,
so it is necessary to grab an extra one if it is still needed after the
call.

Cc: Qi Hou <qi.hou@windriver.com>
Cc: Peter Rosin <peda@axentia.se>
Cc: Rob Herring <robh@kernel.org>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 arch/arm/mach-omap2/omap_hwmod_3xxx_data.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Comments

Guenter Roeck Feb. 27, 2017, 11:34 p.m. UTC | #1
ping ... this problem is now seen in mainline.

Guenter

On Sat, Feb 11, 2017 at 02:59:08PM -0800, Guenter Roeck wrote:
> After commit 'of: fix of_node leak caused in of_find_node_opts_by_path',
> the following error may be reported when running omap images.
> 
> OF: ERROR: Bad of_node_put() on /ocp@68000000
> CPU: 0 PID: 0 Comm: swapper Not tainted 4.10.0-rc7-next-20170210 #1
> Hardware name: Generic OMAP3-GP (Flattened Device Tree)
> [<c0310604>] (unwind_backtrace) from [<c030bbf4>] (show_stack+0x10/0x14)
> [<c030bbf4>] (show_stack) from [<c05add8c>] (dump_stack+0x98/0xac)
> [<c05add8c>] (dump_stack) from [<c05af1b0>] (kobject_release+0x48/0x7c)
> [<c05af1b0>] (kobject_release)
> 	from [<c0ad1aa4>] (of_find_node_by_name+0x74/0x94)
> [<c0ad1aa4>] (of_find_node_by_name)
> 	from [<c1215bd4>] (omap3xxx_hwmod_is_hs_ip_block_usable+0x24/0x2c)
> [<c1215bd4>] (omap3xxx_hwmod_is_hs_ip_block_usable) from
> [<c1215d5c>] (omap3xxx_hwmod_init+0x180/0x274)
> [<c1215d5c>] (omap3xxx_hwmod_init)
> 	from [<c120faa8>] (omap3_init_early+0xa0/0x11c)
> [<c120faa8>] (omap3_init_early)
> 	from [<c120fb2c>] (omap3430_init_early+0x8/0x30)
> [<c120fb2c>] (omap3430_init_early)
> 	from [<c1204710>] (setup_arch+0xc04/0xc34)
> [<c1204710>] (setup_arch) from [<c1200948>] (start_kernel+0x68/0x38c)
> [<c1200948>] (start_kernel) from [<8020807c>] (0x8020807c)
> 
> of_find_node_by_name() drops the reference to the passed device node,
> so it is necessary to grab an extra one if it is still needed after the
> call.
> 
> Cc: Qi Hou <qi.hou@windriver.com>
> Cc: Peter Rosin <peda@axentia.se>
> Cc: Rob Herring <robh@kernel.org>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  arch/arm/mach-omap2/omap_hwmod_3xxx_data.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
> index 56f917ec8621..d4ca1e3964c3 100644
> --- a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
> +++ b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
> @@ -3117,6 +3117,7 @@ static int __init omap3xxx_hwmod_is_hs_ip_block_usable(struct device_node *bus,
>  	if (!bus)
>  		return (omap_type() == OMAP2_DEVICE_TYPE_GP) ? 1 : 0;
>  
> +	of_node_get(bus);
>  	if (of_device_is_available(of_find_node_by_name(bus, dev_name)))
>  		return 1;
>  
> @@ -3189,15 +3190,20 @@ int __init omap3xxx_hwmod_init(void)
>  
>  	if (h_sham && omap3xxx_hwmod_is_hs_ip_block_usable(bus, "sham")) {
>  		r = omap_hwmod_register_links(h_sham);
> -		if (r < 0)
> +		if (r < 0) {
> +			of_node_put(bus);
>  			return r;
> +		}
>  	}
>  
>  	if (h_aes && omap3xxx_hwmod_is_hs_ip_block_usable(bus, "aes")) {
>  		r = omap_hwmod_register_links(h_aes);
> -		if (r < 0)
> +		if (r < 0) {
> +			of_node_put(bus);
>  			return r;
> +		}
>  	}
> +	of_node_put(bus);
>  
>  	/*
>  	 * Register hwmod links specific to certain ES levels of a
Tony Lindgren Feb. 28, 2017, 4:23 p.m. UTC | #2
* Guenter Roeck <linux@roeck-us.net> [170227 15:36]:
> ping ... this problem is now seen in mainline.

Oops sorry I had accidentally tagged this with "next" instead of
"fixes".

Looking at the patch though, can't we make of_node_get/put local to
omap3xxx_hwmod_is_hs_ip_block_usable()?

Regards,

Tony
Guenter Roeck Feb. 28, 2017, 5:26 p.m. UTC | #3
On Tue, Feb 28, 2017 at 08:23:16AM -0800, Tony Lindgren wrote:
> * Guenter Roeck <linux@roeck-us.net> [170227 15:36]:
> > ping ... this problem is now seen in mainline.
> 
> Oops sorry I had accidentally tagged this with "next" instead of
> "fixes".
> 
> Looking at the patch though, can't we make of_node_get/put local to
> omap3xxx_hwmod_is_hs_ip_block_usable()?
> 
Not really.

Strictly speaking the patch fixes two problems:
1) of_find_node_by_name() gets a reference on "bus", thus of_node_put()
   is necessary to release it. This is the change in omap3xxx_hwmod_init().
2) of_find_node_by_name() drops a reference to the passed device node,
   thus it is necessary to call of_node_get(bus) in
   omap3xxx_hwmod_is_hs_ip_block_usable().

I should have stated that more clearly in the description.

We could ignore 1) to get rid of the warning, but that would leave us with
a dangling reference to the bus node. I could also improve the description,
or split the patch in two. Let me know what you prefer.

Another possible change would be to use of_get_child_by_name() instead of
of_find_node_by_name(). That would, however, only search immediate children
of the 'ocp' node, not all nodes. I  think that is what is actually intended
here, but I am not entirely sure. Thoughts ?

Either case, turns out I'll have to re-submit the patch because, as mentioned
above, of_find_node_by_name() gets a reference to the device node it returns,
so a put on that node is necessary in omap3xxx_hwmod_is_hs_ip_block_usable().

Guenter
Tony Lindgren Feb. 28, 2017, 5:52 p.m. UTC | #4
* Guenter Roeck <linux@roeck-us.net> [170228 09:28]:
> On Tue, Feb 28, 2017 at 08:23:16AM -0800, Tony Lindgren wrote:
> > * Guenter Roeck <linux@roeck-us.net> [170227 15:36]:
> > > ping ... this problem is now seen in mainline.
> > 
> > Oops sorry I had accidentally tagged this with "next" instead of
> > "fixes".
> > 
> > Looking at the patch though, can't we make of_node_get/put local to
> > omap3xxx_hwmod_is_hs_ip_block_usable()?
> > 
> Not really.
> 
> Strictly speaking the patch fixes two problems:
> 1) of_find_node_by_name() gets a reference on "bus", thus of_node_put()
>    is necessary to release it. This is the change in omap3xxx_hwmod_init().
> 2) of_find_node_by_name() drops a reference to the passed device node,
>    thus it is necessary to call of_node_get(bus) in
>    omap3xxx_hwmod_is_hs_ip_block_usable().
> 
> I should have stated that more clearly in the description.

OK

> We could ignore 1) to get rid of the warning, but that would leave us with
> a dangling reference to the bus node. I could also improve the description,
> or split the patch in two. Let me know what you prefer.

Once patch is just fine with me.

> Another possible change would be to use of_get_child_by_name() instead of
> of_find_node_by_name(). That would, however, only search immediate children
> of the 'ocp' node, not all nodes. I  think that is what is actually intended
> here, but I am not entirely sure. Thoughts ?

Yes that would be better.

> Either case, turns out I'll have to re-submit the patch because, as mentioned
> above, of_find_node_by_name() gets a reference to the device node it returns,
> so a put on that node is necessary in omap3xxx_hwmod_is_hs_ip_block_usable().

OK

Thanks,

Tony
diff mbox

Patch

diff --git a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
index 56f917ec8621..d4ca1e3964c3 100644
--- a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
@@ -3117,6 +3117,7 @@  static int __init omap3xxx_hwmod_is_hs_ip_block_usable(struct device_node *bus,
 	if (!bus)
 		return (omap_type() == OMAP2_DEVICE_TYPE_GP) ? 1 : 0;
 
+	of_node_get(bus);
 	if (of_device_is_available(of_find_node_by_name(bus, dev_name)))
 		return 1;
 
@@ -3189,15 +3190,20 @@  int __init omap3xxx_hwmod_init(void)
 
 	if (h_sham && omap3xxx_hwmod_is_hs_ip_block_usable(bus, "sham")) {
 		r = omap_hwmod_register_links(h_sham);
-		if (r < 0)
+		if (r < 0) {
+			of_node_put(bus);
 			return r;
+		}
 	}
 
 	if (h_aes && omap3xxx_hwmod_is_hs_ip_block_usable(bus, "aes")) {
 		r = omap_hwmod_register_links(h_aes);
-		if (r < 0)
+		if (r < 0) {
+			of_node_put(bus);
 			return r;
+		}
 	}
+	of_node_put(bus);
 
 	/*
 	 * Register hwmod links specific to certain ES levels of a