[PATCH] clocksource: Do not drop unheld reference on device node

[PATCH] clocksource: Do not drop unheld reference on device node

[Thierry Reding]

When booting a recent kernel on ARM with OF_DYNAMIC enabled, the kernel
warns about the following:

	[    0.000000] ERROR: Bad of_node_put() on /timer@50004600
	[    0.000000] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 3.12.0-rc5-next-20131017-00077-gedfd827-dirty #406
	[    0.000000] [<c0015b68>] (unwind_backtrace+0x0/0xf4) from [<c00117e4>] (show_stack+0x10/0x14)
	[    0.000000] [<c00117e4>] (show_stack+0x10/0x14) from [<c055f734>] (dump_stack+0x9c/0xc8)
	[    0.000000] [<c055f734>] (dump_stack+0x9c/0xc8) from [<c03b47d4>] (of_node_release+0x90/0x9c)
	[    0.000000] [<c03b47d4>] (of_node_release+0x90/0x9c) from [<c03b5084>] (of_find_matching_node_and_match+0x78/0xb4)
	[    0.000000] [<c03b5084>] (of_find_matching_node_and_match+0x78/0xb4) from [<c07887c8>] (clocksource_of_init+0x60/0x70)
	[    0.000000] [<c07887c8>] (clocksource_of_init+0x60/0x70) from [<c076e99c>] (start_kernel+0x1f4/0x33c)
	[    0.000000] [<c076e99c>] (start_kernel+0x1f4/0x33c) from [<80008074>] (0x80008074)

This is caused by clocksource_of_init() dropping a reference on the
device node that it never took. The reference taken by the loop is
implicitly dropped on subsequent iterations. See the implementation of
and the comment on top of the of_find_matching_node_and_match()
function for reference (no pun intended).

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/clocksource/clksrc-of.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/clocksource/clksrc-of.c b/drivers/clocksource/clksrc-of.c
index 35639cf4..b9ddd9e 100644
--- a/drivers/clocksource/clksrc-of.c
+++ b/drivers/clocksource/clksrc-of.c
@@ -35,6 +35,5 @@ void __init clocksource_of_init(void)
 
 		init_func = match->data;
 		init_func(np);
-		of_node_put(np);
 	}
 }
-- 
1.8.4

Re: [PATCH] clocksource: Do not drop unheld reference on device node

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1978 bytes --]

On Sat, Oct 19, 2013 at 12:49:48AM +0200, Thierry Reding wrote:
> When booting a recent kernel on ARM with OF_DYNAMIC enabled, the kernel
> warns about the following:
> 
> 	[    0.000000] ERROR: Bad of_node_put() on /timer@50004600
> 	[    0.000000] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 3.12.0-rc5-next-20131017-00077-gedfd827-dirty #406
> 	[    0.000000] [<c0015b68>] (unwind_backtrace+0x0/0xf4) from [<c00117e4>] (show_stack+0x10/0x14)
> 	[    0.000000] [<c00117e4>] (show_stack+0x10/0x14) from [<c055f734>] (dump_stack+0x9c/0xc8)
> 	[    0.000000] [<c055f734>] (dump_stack+0x9c/0xc8) from [<c03b47d4>] (of_node_release+0x90/0x9c)
> 	[    0.000000] [<c03b47d4>] (of_node_release+0x90/0x9c) from [<c03b5084>] (of_find_matching_node_and_match+0x78/0xb4)
> 	[    0.000000] [<c03b5084>] (of_find_matching_node_and_match+0x78/0xb4) from [<c07887c8>] (clocksource_of_init+0x60/0x70)
> 	[    0.000000] [<c07887c8>] (clocksource_of_init+0x60/0x70) from [<c076e99c>] (start_kernel+0x1f4/0x33c)
> 	[    0.000000] [<c076e99c>] (start_kernel+0x1f4/0x33c) from [<80008074>] (0x80008074)
> 
> This is caused by clocksource_of_init() dropping a reference on the
> device node that it never took. The reference taken by the loop is
> implicitly dropped on subsequent iterations. See the implementation of
> and the comment on top of the of_find_matching_node_and_match()
> function for reference (no pun intended).
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/clocksource/clksrc-of.c | 1 -
>  1 file changed, 1 deletion(-)

Can someone look at this?

Thierry

> diff --git a/drivers/clocksource/clksrc-of.c b/drivers/clocksource/clksrc-of.c
> index 35639cf4..b9ddd9e 100644
> --- a/drivers/clocksource/clksrc-of.c
> +++ b/drivers/clocksource/clksrc-of.c
> @@ -35,6 +35,5 @@ void __init clocksource_of_init(void)
>  
>  		init_func = match->data;
>  		init_func(np);
> -		of_node_put(np);
>  	}
>  }
> -- 
> 1.8.4
> 

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] clocksource: Do not drop unheld reference on device node

[Daniel Lezcano]

On 11/22/2013 05:16 PM, Thierry Reding wrote:
> On Sat, Oct 19, 2013 at 12:49:48AM +0200, Thierry Reding wrote:
>> When booting a recent kernel on ARM with OF_DYNAMIC enabled, the kernel
>> warns about the following:
>>
>> 	[    0.000000] ERROR: Bad of_node_put() on /timer@50004600
>> 	[    0.000000] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 3.12.0-rc5-next-20131017-00077-gedfd827-dirty #406
>> 	[    0.000000] [<c0015b68>] (unwind_backtrace+0x0/0xf4) from [<c00117e4>] (show_stack+0x10/0x14)
>> 	[    0.000000] [<c00117e4>] (show_stack+0x10/0x14) from [<c055f734>] (dump_stack+0x9c/0xc8)
>> 	[    0.000000] [<c055f734>] (dump_stack+0x9c/0xc8) from [<c03b47d4>] (of_node_release+0x90/0x9c)
>> 	[    0.000000] [<c03b47d4>] (of_node_release+0x90/0x9c) from [<c03b5084>] (of_find_matching_node_and_match+0x78/0xb4)
>> 	[    0.000000] [<c03b5084>] (of_find_matching_node_and_match+0x78/0xb4) from [<c07887c8>] (clocksource_of_init+0x60/0x70)
>> 	[    0.000000] [<c07887c8>] (clocksource_of_init+0x60/0x70) from [<c076e99c>] (start_kernel+0x1f4/0x33c)
>> 	[    0.000000] [<c076e99c>] (start_kernel+0x1f4/0x33c) from [<80008074>] (0x80008074)
>>
>> This is caused by clocksource_of_init() dropping a reference on the
>> device node that it never took. The reference taken by the loop is
>> implicitly dropped on subsequent iterations. See the implementation of
>> and the comment on top of the of_find_matching_node_and_match()
>> function for reference (no pun intended).
>>
>> Signed-off-by: Thierry Reding <treding@nvidia.com>
>> ---
>>   drivers/clocksource/clksrc-of.c | 1 -
>>   1 file changed, 1 deletion(-)
>
> Can someone look at this?

Yes. Sounds like I missed it.

This regression has been introduced by:

commit 326e31eebe61dc838e031ea16968b2cfb43443e3
Author: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Date:   Tue Oct 1 11:00:53 2013 +0200

     clocksource: Put nodes passed to CLOCKSOURCE_OF_DECLARE callbacks 
centrally

     Instead of letting each driver call of_node_put do it centrally in the
     loop that also calls the CLOCKSOURCE_OF_DECLARE callbacks. This is less
     prone to error and also moves getting and putting the references 
into the
     same function.

     Consequently all respective of_node_put calls in drivers are removed.

     Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
     Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
     Acked-by: David Brown <davidb@codeaurora.org>


>> diff --git a/drivers/clocksource/clksrc-of.c b/drivers/clocksource/clksrc-of.c
>> index 35639cf4..b9ddd9e 100644
>> --- a/drivers/clocksource/clksrc-of.c
>> +++ b/drivers/clocksource/clksrc-of.c
>> @@ -35,6 +35,5 @@ void __init clocksource_of_init(void)
>>
>>   		init_func = match->data;
>>   		init_func(np);
>> -		of_node_put(np);
>>   	}
>>   }
>> --
>> 1.8.4
>>


-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH] clocksource: Do not drop unheld reference on device node

[Uwe Kleine-König]

On Fri, Nov 22, 2013 at 05:31:46PM +0100, Daniel Lezcano wrote:
> On 11/22/2013 05:16 PM, Thierry Reding wrote:
> >On Sat, Oct 19, 2013 at 12:49:48AM +0200, Thierry Reding wrote:
> >>When booting a recent kernel on ARM with OF_DYNAMIC enabled, the kernel
> >>warns about the following:
> >>
> >>	[    0.000000] ERROR: Bad of_node_put() on /timer@50004600
> >>	[    0.000000] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 3.12.0-rc5-next-20131017-00077-gedfd827-dirty #406
> >>	[    0.000000] [<c0015b68>] (unwind_backtrace+0x0/0xf4) from [<c00117e4>] (show_stack+0x10/0x14)
> >>	[    0.000000] [<c00117e4>] (show_stack+0x10/0x14) from [<c055f734>] (dump_stack+0x9c/0xc8)
> >>	[    0.000000] [<c055f734>] (dump_stack+0x9c/0xc8) from [<c03b47d4>] (of_node_release+0x90/0x9c)
> >>	[    0.000000] [<c03b47d4>] (of_node_release+0x90/0x9c) from [<c03b5084>] (of_find_matching_node_and_match+0x78/0xb4)
> >>	[    0.000000] [<c03b5084>] (of_find_matching_node_and_match+0x78/0xb4) from [<c07887c8>] (clocksource_of_init+0x60/0x70)
> >>	[    0.000000] [<c07887c8>] (clocksource_of_init+0x60/0x70) from [<c076e99c>] (start_kernel+0x1f4/0x33c)
> >>	[    0.000000] [<c076e99c>] (start_kernel+0x1f4/0x33c) from [<80008074>] (0x80008074)
> >>
> >>This is caused by clocksource_of_init() dropping a reference on the
> >>device node that it never took. The reference taken by the loop is
> >>implicitly dropped on subsequent iterations. See the implementation of
> >>and the comment on top of the of_find_matching_node_and_match()
> >>function for reference (no pun intended).
I only saw the end of the comment saying:

	Returns a node pointer with refcount incremented, use
	of_node_put() on it when done.

Maybe that could be improved to something like:

/**
 * of_find_matching_node_and_match - Find a node based on an of_device_id
 *                                   match table
 * @from: The node to start searching from or NULL, the node you pass
 *        will not be searched, only the next one will; typically, you
 *        pass what the previous call returned.
 * @matches: ...
 * @match: ...
 *
 * Reference counting: Puts a reference on @from and increases the
 * reference count of the returned node.
 */

> >>
> >>Signed-off-by: Thierry Reding <treding@nvidia.com>
> >>---
> >>  drivers/clocksource/clksrc-of.c | 1 -
> >>  1 file changed, 1 deletion(-)
> >
> >Can someone look at this?
> 
> Yes. Sounds like I missed it.
> 
> This regression has been introduced by:
> 
> commit 326e31eebe61dc838e031ea16968b2cfb43443e3
> Author: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Date:   Tue Oct 1 11:00:53 2013 +0200
> 
>     clocksource: Put nodes passed to CLOCKSOURCE_OF_DECLARE
> callbacks centrally
> 
>     Instead of letting each driver call of_node_put do it centrally in the
>     loop that also calls the CLOCKSOURCE_OF_DECLARE callbacks. This is less
>     prone to error and also moves getting and putting the references
> into the
>     same function.
> 
>     Consequently all respective of_node_put calls in drivers are removed.
> 
>     Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>     Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>     Acked-by: David Brown <davidb@codeaurora.org>
Still all but the hook in clocksource_of_init of this commit was
correct, right? (Well, but this buggy hunk makes the commit log wrong.)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] clocksource: Do not drop unheld reference on device node

[Daniel Lezcano]

On 11/22/2013 08:22 PM, Uwe Kleine-König wrote:
> On Fri, Nov 22, 2013 at 05:31:46PM +0100, Daniel Lezcano wrote:
>> On 11/22/2013 05:16 PM, Thierry Reding wrote:
>>> On Sat, Oct 19, 2013 at 12:49:48AM +0200, Thierry Reding wrote:
>>>> When booting a recent kernel on ARM with OF_DYNAMIC enabled, the kernel
>>>> warns about the following:
>>>>
>>>> 	[    0.000000] ERROR: Bad of_node_put() on /timer@50004600
>>>> 	[    0.000000] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 3.12.0-rc5-next-20131017-00077-gedfd827-dirty #406
>>>> 	[    0.000000] [<c0015b68>] (unwind_backtrace+0x0/0xf4) from [<c00117e4>] (show_stack+0x10/0x14)
>>>> 	[    0.000000] [<c00117e4>] (show_stack+0x10/0x14) from [<c055f734>] (dump_stack+0x9c/0xc8)
>>>> 	[    0.000000] [<c055f734>] (dump_stack+0x9c/0xc8) from [<c03b47d4>] (of_node_release+0x90/0x9c)
>>>> 	[    0.000000] [<c03b47d4>] (of_node_release+0x90/0x9c) from [<c03b5084>] (of_find_matching_node_and_match+0x78/0xb4)
>>>> 	[    0.000000] [<c03b5084>] (of_find_matching_node_and_match+0x78/0xb4) from [<c07887c8>] (clocksource_of_init+0x60/0x70)
>>>> 	[    0.000000] [<c07887c8>] (clocksource_of_init+0x60/0x70) from [<c076e99c>] (start_kernel+0x1f4/0x33c)
>>>> 	[    0.000000] [<c076e99c>] (start_kernel+0x1f4/0x33c) from [<80008074>] (0x80008074)
>>>>
>>>> This is caused by clocksource_of_init() dropping a reference on the
>>>> device node that it never took. The reference taken by the loop is
>>>> implicitly dropped on subsequent iterations. See the implementation of
>>>> and the comment on top of the of_find_matching_node_and_match()
>>>> function for reference (no pun intended).
> I only saw the end of the comment saying:
>
> 	Returns a node pointer with refcount incremented, use
> 	of_node_put() on it when done.
>
> Maybe that could be improved to something like:
>
> /**
>   * of_find_matching_node_and_match - Find a node based on an of_device_id
>   *                                   match table
>   * @from: The node to start searching from or NULL, the node you pass
>   *        will not be searched, only the next one will; typically, you
>   *        pass what the previous call returned.
>   * @matches: ...
>   * @match: ...
>   *
>   * Reference counting: Puts a reference on @from and increases the
>   * reference count of the returned node.
>   */
>
>>>>
>>>> Signed-off-by: Thierry Reding <treding@nvidia.com>
>>>> ---
>>>>   drivers/clocksource/clksrc-of.c | 1 -
>>>>   1 file changed, 1 deletion(-)
>>>
>>> Can someone look at this?
>>
>> Yes. Sounds like I missed it.
>>
>> This regression has been introduced by:
>>
>> commit 326e31eebe61dc838e031ea16968b2cfb43443e3
>> Author: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>> Date:   Tue Oct 1 11:00:53 2013 +0200
>>
>>      clocksource: Put nodes passed to CLOCKSOURCE_OF_DECLARE
>> callbacks centrally
>>
>>      Instead of letting each driver call of_node_put do it centrally in the
>>      loop that also calls the CLOCKSOURCE_OF_DECLARE callbacks. This is less
>>      prone to error and also moves getting and putting the references
>> into the
>>      same function.
>>
>>      Consequently all respective of_node_put calls in drivers are removed.
>>
>>      Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>>      Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>      Acked-by: David Brown <davidb@codeaurora.org>
> Still all but the hook in clocksource_of_init of this commit was
> correct, right? (Well, but this buggy hunk makes the commit log wrong.)

I don't understand your comment, can you elaborate ?

Thanks
   -- Daniel

-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH] clocksource: Do not drop unheld reference on device node

[Uwe Kleine-König]

Hello Daniel,

On Sun, Nov 24, 2013 at 10:28:15PM +0100, Daniel Lezcano wrote:
> On 11/22/2013 08:22 PM, Uwe Kleine-König wrote:
> >On Fri, Nov 22, 2013 at 05:31:46PM +0100, Daniel Lezcano wrote:
> >>On 11/22/2013 05:16 PM, Thierry Reding wrote:
> >>>On Sat, Oct 19, 2013 at 12:49:48AM +0200, Thierry Reding wrote:
> >>Yes. Sounds like I missed it.
> >>
> >>This regression has been introduced by:
> >>
> >>commit 326e31eebe61dc838e031ea16968b2cfb43443e3
> >>Author: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> >>Date:   Tue Oct 1 11:00:53 2013 +0200
> >>
> >>     clocksource: Put nodes passed to CLOCKSOURCE_OF_DECLARE
> >>callbacks centrally
> >>
> >>     Instead of letting each driver call of_node_put do it centrally in the
> >>     loop that also calls the CLOCKSOURCE_OF_DECLARE callbacks. This is less
> >>     prone to error and also moves getting and putting the references
> >>into the
> >>     same function.
> >>
> >>     Consequently all respective of_node_put calls in drivers are removed.
> >>
> >>     Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> >>     Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> >>     Acked-by: David Brown <davidb@codeaurora.org>
> >Still all but the hook in clocksource_of_init of this commit was
> >correct, right? (Well, but this buggy hunk makes the commit log wrong.)
> 
> I don't understand your comment, can you elaborate ?
My patch added an of_node_put in clocksource_of_init and dropped several
of_node_puts in drivers. This thread is about the first being wrong. My
question was if dropping the others was correct.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] clocksource: Do not drop unheld reference on device node

[Daniel Lezcano]

On 11/25/2013 08:29 AM, Uwe Kleine-König wrote:
> Hello Daniel,
>
> On Sun, Nov 24, 2013 at 10:28:15PM +0100, Daniel Lezcano wrote:
>> On 11/22/2013 08:22 PM, Uwe Kleine-König wrote:
>>> On Fri, Nov 22, 2013 at 05:31:46PM +0100, Daniel Lezcano wrote:
>>>> On 11/22/2013 05:16 PM, Thierry Reding wrote:
>>>>> On Sat, Oct 19, 2013 at 12:49:48AM +0200, Thierry Reding wrote:
>>>> Yes. Sounds like I missed it.
>>>>
>>>> This regression has been introduced by:
>>>>
>>>> commit 326e31eebe61dc838e031ea16968b2cfb43443e3
>>>> Author: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>>>> Date:   Tue Oct 1 11:00:53 2013 +0200
>>>>
>>>>      clocksource: Put nodes passed to CLOCKSOURCE_OF_DECLARE
>>>> callbacks centrally
>>>>
>>>>      Instead of letting each driver call of_node_put do it centrally in the
>>>>      loop that also calls the CLOCKSOURCE_OF_DECLARE callbacks. This is less
>>>>      prone to error and also moves getting and putting the references
>>>> into the
>>>>      same function.
>>>>
>>>>      Consequently all respective of_node_put calls in drivers are removed.
>>>>
>>>>      Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>>>>      Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>>>      Acked-by: David Brown <davidb@codeaurora.org>
>>> Still all but the hook in clocksource_of_init of this commit was
>>> correct, right? (Well, but this buggy hunk makes the commit log wrong.)
>>
>> I don't understand your comment, can you elaborate ?
> My patch added an of_node_put in clocksource_of_init and dropped several
> of_node_puts in drivers. This thread is about the first being wrong. My
> question was if dropping the others was correct.

Yes, Thierry's patch removes the of_node_puts but I am also wondering if 
we shouldn't just revert the patch 326e31eebe61dc838e instead.


-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH] clocksource: Do not drop unheld reference on device node

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2253 bytes --]

On Mon, Nov 25, 2013 at 09:57:39AM +0100, Daniel Lezcano wrote:
> On 11/25/2013 08:29 AM, Uwe Kleine-König wrote:
> >Hello Daniel,
> >
> >On Sun, Nov 24, 2013 at 10:28:15PM +0100, Daniel Lezcano wrote:
> >>On 11/22/2013 08:22 PM, Uwe Kleine-König wrote:
> >>>On Fri, Nov 22, 2013 at 05:31:46PM +0100, Daniel Lezcano wrote:
> >>>>On 11/22/2013 05:16 PM, Thierry Reding wrote:
> >>>>>On Sat, Oct 19, 2013 at 12:49:48AM +0200, Thierry Reding wrote:
> >>>>Yes. Sounds like I missed it.
> >>>>
> >>>>This regression has been introduced by:
> >>>>
> >>>>commit 326e31eebe61dc838e031ea16968b2cfb43443e3
> >>>>Author: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> >>>>Date:   Tue Oct 1 11:00:53 2013 +0200
> >>>>
> >>>>     clocksource: Put nodes passed to CLOCKSOURCE_OF_DECLARE
> >>>>callbacks centrally
> >>>>
> >>>>     Instead of letting each driver call of_node_put do it centrally in the
> >>>>     loop that also calls the CLOCKSOURCE_OF_DECLARE callbacks. This is less
> >>>>     prone to error and also moves getting and putting the references
> >>>>into the
> >>>>     same function.
> >>>>
> >>>>     Consequently all respective of_node_put calls in drivers are removed.
> >>>>
> >>>>     Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> >>>>     Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> >>>>     Acked-by: David Brown <davidb@codeaurora.org>
> >>>Still all but the hook in clocksource_of_init of this commit was
> >>>correct, right? (Well, but this buggy hunk makes the commit log wrong.)
> >>
> >>I don't understand your comment, can you elaborate ?
> >My patch added an of_node_put in clocksource_of_init and dropped several
> >of_node_puts in drivers. This thread is about the first being wrong. My
> >question was if dropping the others was correct.
> 
> Yes, Thierry's patch removes the of_node_puts but I am also wondering if we
> shouldn't just revert the patch 326e31eebe61dc838e instead.

I don't think that's necessary. Dropping the other of_node_put()s looks
like the right thing to do. They drop a reference that they haven't
taken themselves, which is usually not correct. I can't find anything
wrong with the rest of that commit.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] clocksource: Do not drop unheld reference on device node

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2161 bytes --]

On Mon, Nov 25, 2013 at 08:29:23AM +0100, Uwe Kleine-König wrote:
> Hello Daniel,
> 
> On Sun, Nov 24, 2013 at 10:28:15PM +0100, Daniel Lezcano wrote:
> > On 11/22/2013 08:22 PM, Uwe Kleine-König wrote:
> > >On Fri, Nov 22, 2013 at 05:31:46PM +0100, Daniel Lezcano wrote:
> > >>On 11/22/2013 05:16 PM, Thierry Reding wrote:
> > >>>On Sat, Oct 19, 2013 at 12:49:48AM +0200, Thierry Reding wrote:
> > >>Yes. Sounds like I missed it.
> > >>
> > >>This regression has been introduced by:
> > >>
> > >>commit 326e31eebe61dc838e031ea16968b2cfb43443e3
> > >>Author: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > >>Date:   Tue Oct 1 11:00:53 2013 +0200
> > >>
> > >>     clocksource: Put nodes passed to CLOCKSOURCE_OF_DECLARE
> > >>callbacks centrally
> > >>
> > >>     Instead of letting each driver call of_node_put do it centrally in the
> > >>     loop that also calls the CLOCKSOURCE_OF_DECLARE callbacks. This is less
> > >>     prone to error and also moves getting and putting the references
> > >>into the
> > >>     same function.
> > >>
> > >>     Consequently all respective of_node_put calls in drivers are removed.
> > >>
> > >>     Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > >>     Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> > >>     Acked-by: David Brown <davidb@codeaurora.org>
> > >Still all but the hook in clocksource_of_init of this commit was
> > >correct, right? (Well, but this buggy hunk makes the commit log wrong.)
> > 
> > I don't understand your comment, can you elaborate ?
> My patch added an of_node_put in clocksource_of_init and dropped several
> of_node_puts in drivers. This thread is about the first being wrong. My
> question was if dropping the others was correct.

Yes, it looks to me like the others were correct. A reference on np is
taken by the for_each_matching_node_and_match() in clocksource_of_init()
and released when through the loop, or rather on the next iteration. So
it would be an error to release the reference within the driver-specific
initializations, since they don't take a reference of their own.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] clocksource: Do not drop unheld reference on device node

[Daniel Lezcano]

On 10/19/2013 12:49 AM, Thierry Reding wrote:
> When booting a recent kernel on ARM with OF_DYNAMIC enabled, the kernel
> warns about the following:
>
> 	[    0.000000] ERROR: Bad of_node_put() on /timer@50004600
> 	[    0.000000] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 3.12.0-rc5-next-20131017-00077-gedfd827-dirty #406
> 	[    0.000000] [<c0015b68>] (unwind_backtrace+0x0/0xf4) from [<c00117e4>] (show_stack+0x10/0x14)
> 	[    0.000000] [<c00117e4>] (show_stack+0x10/0x14) from [<c055f734>] (dump_stack+0x9c/0xc8)
> 	[    0.000000] [<c055f734>] (dump_stack+0x9c/0xc8) from [<c03b47d4>] (of_node_release+0x90/0x9c)
> 	[    0.000000] [<c03b47d4>] (of_node_release+0x90/0x9c) from [<c03b5084>] (of_find_matching_node_and_match+0x78/0xb4)
> 	[    0.000000] [<c03b5084>] (of_find_matching_node_and_match+0x78/0xb4) from [<c07887c8>] (clocksource_of_init+0x60/0x70)
> 	[    0.000000] [<c07887c8>] (clocksource_of_init+0x60/0x70) from [<c076e99c>] (start_kernel+0x1f4/0x33c)
> 	[    0.000000] [<c076e99c>] (start_kernel+0x1f4/0x33c) from [<80008074>] (0x80008074)
>
> This is caused by clocksource_of_init() dropping a reference on the
> device node that it never took. The reference taken by the loop is
> implicitly dropped on subsequent iterations. See the implementation of
> and the comment on top of the of_find_matching_node_and_match()
> function for reference (no pun intended).
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>   drivers/clocksource/clksrc-of.c | 1 -
>   1 file changed, 1 deletion(-)
>
> diff --git a/drivers/clocksource/clksrc-of.c b/drivers/clocksource/clksrc-of.c
> index 35639cf4..b9ddd9e 100644
> --- a/drivers/clocksource/clksrc-of.c
> +++ b/drivers/clocksource/clksrc-of.c
> @@ -35,6 +35,5 @@ void __init clocksource_of_init(void)
>
>   		init_func = match->data;
>   		init_func(np);
> -		of_node_put(np);
>   	}
>   }

Applied to my tree for 3.13 fixes.


-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog