Re: [PATCH] powerpc/pseries: Fix of_node_put() underflow during pseries remove

[Tyrel Datwyler]

On 07/24/2017 09:47 PM, Michael Ellerman wrote:
> Tyrel Datwyler  writes:
> 
>> On 07/24/2017 03:42 AM, Michael Ellerman wrote:
>>> Laurent Vivier  writes:
>>>
 As for commit 68baf692c435 ("powerpc/pseries: Fix of_node_put()
 underflow during DLPAR remove"), the call to of_node_put()
 must be removed from pSeries_reconfig_remove_node().

 dlpar_detach_node() and pSeries_reconfig_remove_node() call
 of_detach_node(), and thus the node should not be released
 in this case too.

 Signed-off-by: Laurent Vivier 
 ---
  arch/powerpc/platforms/pseries/reconfig.c | 1 -
  1 file changed, 1 deletion(-)
>>>
>>> Thanks. I'll spare you the swearing about why we have the same bug in
>>> two places.
>>
>> That's probably my bad. I must have failed to test with older powerpc-util 
>> tooling where
>> drmgr uses the /proc/ofdt interface for device tree modification.
> 
> OK. Really we should have automated tests of the various cases, I've
> just never had time to write any.

Agreed, some better CI is warranted.

> 
> Mainly the thing that bugs me is that we still have the two separate
> paths. Or if we must maintain both they could at least share more code,
> the two functions do basically the same thing AFAICS.

Yeah, I think that is where I dropped the ball. I wrongly assumed by not 
looking close
enough that code was shared in those two paths. Definitely some code 
de-duplication work
that can be done.

-Tyrel

> 
> cheers
> 

Re: [PATCH] powerpc/pseries: Fix of_node_put() underflow during pseries remove

[Tyrel Datwyler]

On 07/24/2017 09:47 PM, Michael Ellerman wrote:
> Tyrel Datwyler  writes:
> 
>> On 07/24/2017 03:42 AM, Michael Ellerman wrote:
>>> Laurent Vivier  writes:
>>>
 As for commit 68baf692c435 ("powerpc/pseries: Fix of_node_put()
 underflow during DLPAR remove"), the call to of_node_put()
 must be removed from pSeries_reconfig_remove_node().

 dlpar_detach_node() and pSeries_reconfig_remove_node() call
 of_detach_node(), and thus the node should not be released
 in this case too.

 Signed-off-by: Laurent Vivier 
 ---
  arch/powerpc/platforms/pseries/reconfig.c | 1 -
  1 file changed, 1 deletion(-)
>>>
>>> Thanks. I'll spare you the swearing about why we have the same bug in
>>> two places.
>>
>> That's probably my bad. I must have failed to test with older powerpc-util 
>> tooling where
>> drmgr uses the /proc/ofdt interface for device tree modification.
> 
> OK. Really we should have automated tests of the various cases, I've
> just never had time to write any.

Agreed, some better CI is warranted.

> 
> Mainly the thing that bugs me is that we still have the two separate
> paths. Or if we must maintain both they could at least share more code,
> the two functions do basically the same thing AFAICS.

Yeah, I think that is where I dropped the ball. I wrongly assumed by not 
looking close
enough that code was shared in those two paths. Definitely some code 
de-duplication work
that can be done.

-Tyrel

> 
> cheers
> 

Re: [PATCH] powerpc/pseries: Fix of_node_put() underflow during pseries remove

[Michael Ellerman]

Tyrel Datwyler  writes:

> On 07/24/2017 03:42 AM, Michael Ellerman wrote:
>> Laurent Vivier  writes:
>> 
>>> As for commit 68baf692c435 ("powerpc/pseries: Fix of_node_put()
>>> underflow during DLPAR remove"), the call to of_node_put()
>>> must be removed from pSeries_reconfig_remove_node().
>>>
>>> dlpar_detach_node() and pSeries_reconfig_remove_node() call
>>> of_detach_node(), and thus the node should not be released
>>> in this case too.
>>>
>>> Signed-off-by: Laurent Vivier 
>>> ---
>>>  arch/powerpc/platforms/pseries/reconfig.c | 1 -
>>>  1 file changed, 1 deletion(-)
>> 
>> Thanks. I'll spare you the swearing about why we have the same bug in
>> two places.
>
> That's probably my bad. I must have failed to test with older powerpc-util 
> tooling where
> drmgr uses the /proc/ofdt interface for device tree modification.

OK. Really we should have automated tests of the various cases, I've
just never had time to write any.

Mainly the thing that bugs me is that we still have the two separate
paths. Or if we must maintain both they could at least share more code,
the two functions do basically the same thing AFAICS.

cheers

Re: [PATCH] powerpc/pseries: Fix of_node_put() underflow during pseries remove

[Michael Ellerman]

Tyrel Datwyler  writes:

> On 07/24/2017 03:42 AM, Michael Ellerman wrote:
>> Laurent Vivier  writes:
>> 
>>> As for commit 68baf692c435 ("powerpc/pseries: Fix of_node_put()
>>> underflow during DLPAR remove"), the call to of_node_put()
>>> must be removed from pSeries_reconfig_remove_node().
>>>
>>> dlpar_detach_node() and pSeries_reconfig_remove_node() call
>>> of_detach_node(), and thus the node should not be released
>>> in this case too.
>>>
>>> Signed-off-by: Laurent Vivier 
>>> ---
>>>  arch/powerpc/platforms/pseries/reconfig.c | 1 -
>>>  1 file changed, 1 deletion(-)
>> 
>> Thanks. I'll spare you the swearing about why we have the same bug in
>> two places.
>
> That's probably my bad. I must have failed to test with older powerpc-util 
> tooling where
> drmgr uses the /proc/ofdt interface for device tree modification.

OK. Really we should have automated tests of the various cases, I've
just never had time to write any.

Mainly the thing that bugs me is that we still have the two separate
paths. Or if we must maintain both they could at least share more code,
the two functions do basically the same thing AFAICS.

cheers

Re: [PATCH] powerpc/pseries: Fix of_node_put() underflow during pseries remove

[Tyrel Datwyler]

On 07/24/2017 03:42 AM, Michael Ellerman wrote:
> Laurent Vivier  writes:
> 
>> As for commit 68baf692c435 ("powerpc/pseries: Fix of_node_put()
>> underflow during DLPAR remove"), the call to of_node_put()
>> must be removed from pSeries_reconfig_remove_node().
>>
>> dlpar_detach_node() and pSeries_reconfig_remove_node() call
>> of_detach_node(), and thus the node should not be released
>> in this case too.
>>
>> Signed-off-by: Laurent Vivier 
>> ---
>>  arch/powerpc/platforms/pseries/reconfig.c | 1 -
>>  1 file changed, 1 deletion(-)
> 
> Thanks. I'll spare you the swearing about why we have the same bug in
> two places.

That's probably my bad. I must have failed to test with older powerpc-util 
tooling where
drmgr uses the /proc/ofdt interface for device tree modification.

-Tyrel

> 
> As for the other fix, I'll add:
> 
> Fixes: 0829f6d1f69e ("of: device_node kobject lifecycle fixes")
> Cc: sta...@vger.kernel.org # v3.15+
> 
> cheers
> 
>> diff --git a/arch/powerpc/platforms/pseries/reconfig.c 
>> b/arch/powerpc/platforms/pseries/reconfig.c
>> index e5bf1e8..011ef21 100644
>> --- a/arch/powerpc/platforms/pseries/reconfig.c
>> +++ b/arch/powerpc/platforms/pseries/reconfig.c
>> @@ -82,7 +82,6 @@ static int pSeries_reconfig_remove_node(struct device_node 
>> *np)
>>  
>>  of_detach_node(np);
>>  of_node_put(parent);
>> -of_node_put(np); /* Must decrement the refcount */
>>  return 0;
>>  }
>>  
>> -- 
>> 2.9.4

Re: [PATCH] powerpc/pseries: Fix of_node_put() underflow during pseries remove

[Tyrel Datwyler]

On 07/24/2017 03:42 AM, Michael Ellerman wrote:
> Laurent Vivier  writes:
> 
>> As for commit 68baf692c435 ("powerpc/pseries: Fix of_node_put()
>> underflow during DLPAR remove"), the call to of_node_put()
>> must be removed from pSeries_reconfig_remove_node().
>>
>> dlpar_detach_node() and pSeries_reconfig_remove_node() call
>> of_detach_node(), and thus the node should not be released
>> in this case too.
>>
>> Signed-off-by: Laurent Vivier 
>> ---
>>  arch/powerpc/platforms/pseries/reconfig.c | 1 -
>>  1 file changed, 1 deletion(-)
> 
> Thanks. I'll spare you the swearing about why we have the same bug in
> two places.

That's probably my bad. I must have failed to test with older powerpc-util 
tooling where
drmgr uses the /proc/ofdt interface for device tree modification.

-Tyrel

> 
> As for the other fix, I'll add:
> 
> Fixes: 0829f6d1f69e ("of: device_node kobject lifecycle fixes")
> Cc: sta...@vger.kernel.org # v3.15+
> 
> cheers
> 
>> diff --git a/arch/powerpc/platforms/pseries/reconfig.c 
>> b/arch/powerpc/platforms/pseries/reconfig.c
>> index e5bf1e8..011ef21 100644
>> --- a/arch/powerpc/platforms/pseries/reconfig.c
>> +++ b/arch/powerpc/platforms/pseries/reconfig.c
>> @@ -82,7 +82,6 @@ static int pSeries_reconfig_remove_node(struct device_node 
>> *np)
>>  
>>  of_detach_node(np);
>>  of_node_put(parent);
>> -of_node_put(np); /* Must decrement the refcount */
>>  return 0;
>>  }
>>  
>> -- 
>> 2.9.4

Re: [PATCH] powerpc/pseries: Fix of_node_put() underflow during pseries remove

[Michael Ellerman]

Laurent Vivier  writes:

> As for commit 68baf692c435 ("powerpc/pseries: Fix of_node_put()
> underflow during DLPAR remove"), the call to of_node_put()
> must be removed from pSeries_reconfig_remove_node().
>
> dlpar_detach_node() and pSeries_reconfig_remove_node() call
> of_detach_node(), and thus the node should not be released
> in this case too.
>
> Signed-off-by: Laurent Vivier 
> ---
>  arch/powerpc/platforms/pseries/reconfig.c | 1 -
>  1 file changed, 1 deletion(-)

Thanks. I'll spare you the swearing about why we have the same bug in
two places.

As for the other fix, I'll add:

Fixes: 0829f6d1f69e ("of: device_node kobject lifecycle fixes")
Cc: sta...@vger.kernel.org # v3.15+

cheers

> diff --git a/arch/powerpc/platforms/pseries/reconfig.c 
> b/arch/powerpc/platforms/pseries/reconfig.c
> index e5bf1e8..011ef21 100644
> --- a/arch/powerpc/platforms/pseries/reconfig.c
> +++ b/arch/powerpc/platforms/pseries/reconfig.c
> @@ -82,7 +82,6 @@ static int pSeries_reconfig_remove_node(struct device_node 
> *np)
>  
>   of_detach_node(np);
>   of_node_put(parent);
> - of_node_put(np); /* Must decrement the refcount */
>   return 0;
>  }
>  
> -- 
> 2.9.4

Re: [PATCH] powerpc/pseries: Fix of_node_put() underflow during pseries remove

[Michael Ellerman]

Laurent Vivier  writes:

> As for commit 68baf692c435 ("powerpc/pseries: Fix of_node_put()
> underflow during DLPAR remove"), the call to of_node_put()
> must be removed from pSeries_reconfig_remove_node().
>
> dlpar_detach_node() and pSeries_reconfig_remove_node() call
> of_detach_node(), and thus the node should not be released
> in this case too.
>
> Signed-off-by: Laurent Vivier 
> ---
>  arch/powerpc/platforms/pseries/reconfig.c | 1 -
>  1 file changed, 1 deletion(-)

Thanks. I'll spare you the swearing about why we have the same bug in
two places.

As for the other fix, I'll add:

Fixes: 0829f6d1f69e ("of: device_node kobject lifecycle fixes")
Cc: sta...@vger.kernel.org # v3.15+

cheers

> diff --git a/arch/powerpc/platforms/pseries/reconfig.c 
> b/arch/powerpc/platforms/pseries/reconfig.c
> index e5bf1e8..011ef21 100644
> --- a/arch/powerpc/platforms/pseries/reconfig.c
> +++ b/arch/powerpc/platforms/pseries/reconfig.c
> @@ -82,7 +82,6 @@ static int pSeries_reconfig_remove_node(struct device_node 
> *np)
>  
>   of_detach_node(np);
>   of_node_put(parent);
> - of_node_put(np); /* Must decrement the refcount */
>   return 0;
>  }
>  
> -- 
> 2.9.4

Re: [PATCH] powerpc/pseries: Fix of_node_put() underflow during pseries remove

[David Gibson]

On Fri, Jul 21, 2017 at 04:51:39PM +0200, Laurent Vivier wrote:
> As for commit 68baf692c435 ("powerpc/pseries: Fix of_node_put()
> underflow during DLPAR remove"), the call to of_node_put()
> must be removed from pSeries_reconfig_remove_node().
> 
> dlpar_detach_node() and pSeries_reconfig_remove_node() call
> of_detach_node(), and thus the node should not be released
> in this case too.
> 
> Signed-off-by: Laurent Vivier 

Reviewed-by: David Gibson 

> ---
>  arch/powerpc/platforms/pseries/reconfig.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/arch/powerpc/platforms/pseries/reconfig.c 
> b/arch/powerpc/platforms/pseries/reconfig.c
> index e5bf1e8..011ef21 100644
> --- a/arch/powerpc/platforms/pseries/reconfig.c
> +++ b/arch/powerpc/platforms/pseries/reconfig.c
> @@ -82,7 +82,6 @@ static int pSeries_reconfig_remove_node(struct device_node 
> *np)
>  
>   of_detach_node(np);
>   of_node_put(parent);
> - of_node_put(np); /* Must decrement the refcount */
>   return 0;
>  }
>  

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [PATCH] powerpc/pseries: Fix of_node_put() underflow during pseries remove

[David Gibson]

On Fri, Jul 21, 2017 at 04:51:39PM +0200, Laurent Vivier wrote:
> As for commit 68baf692c435 ("powerpc/pseries: Fix of_node_put()
> underflow during DLPAR remove"), the call to of_node_put()
> must be removed from pSeries_reconfig_remove_node().
> 
> dlpar_detach_node() and pSeries_reconfig_remove_node() call
> of_detach_node(), and thus the node should not be released
> in this case too.
> 
> Signed-off-by: Laurent Vivier 

Reviewed-by: David Gibson 

> ---
>  arch/powerpc/platforms/pseries/reconfig.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/arch/powerpc/platforms/pseries/reconfig.c 
> b/arch/powerpc/platforms/pseries/reconfig.c
> index e5bf1e8..011ef21 100644
> --- a/arch/powerpc/platforms/pseries/reconfig.c
> +++ b/arch/powerpc/platforms/pseries/reconfig.c
> @@ -82,7 +82,6 @@ static int pSeries_reconfig_remove_node(struct device_node 
> *np)
>  
>   of_detach_node(np);
>   of_node_put(parent);
> - of_node_put(np); /* Must decrement the refcount */
>   return 0;
>  }
>  

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

[PATCH] powerpc/pseries: Fix of_node_put() underflow during pseries remove

[Laurent Vivier]

As for commit 68baf692c435 ("powerpc/pseries: Fix of_node_put()
underflow during DLPAR remove"), the call to of_node_put()
must be removed from pSeries_reconfig_remove_node().

dlpar_detach_node() and pSeries_reconfig_remove_node() call
of_detach_node(), and thus the node should not be released
in this case too.

Signed-off-by: Laurent Vivier 
---
 arch/powerpc/platforms/pseries/reconfig.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/powerpc/platforms/pseries/reconfig.c 
b/arch/powerpc/platforms/pseries/reconfig.c
index e5bf1e8..011ef21 100644
--- a/arch/powerpc/platforms/pseries/reconfig.c
+++ b/arch/powerpc/platforms/pseries/reconfig.c
@@ -82,7 +82,6 @@ static int pSeries_reconfig_remove_node(struct device_node 
*np)
 
of_detach_node(np);
of_node_put(parent);
-   of_node_put(np); /* Must decrement the refcount */
return 0;
 }
 
-- 
2.9.4

[PATCH] powerpc/pseries: Fix of_node_put() underflow during pseries remove

[Laurent Vivier]

As for commit 68baf692c435 ("powerpc/pseries: Fix of_node_put()
underflow during DLPAR remove"), the call to of_node_put()
must be removed from pSeries_reconfig_remove_node().

dlpar_detach_node() and pSeries_reconfig_remove_node() call
of_detach_node(), and thus the node should not be released
in this case too.

Signed-off-by: Laurent Vivier 
---
 arch/powerpc/platforms/pseries/reconfig.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/powerpc/platforms/pseries/reconfig.c 
b/arch/powerpc/platforms/pseries/reconfig.c
index e5bf1e8..011ef21 100644
--- a/arch/powerpc/platforms/pseries/reconfig.c
+++ b/arch/powerpc/platforms/pseries/reconfig.c
@@ -82,7 +82,6 @@ static int pSeries_reconfig_remove_node(struct device_node 
*np)
 
of_detach_node(np);
of_node_put(parent);
-   of_node_put(np); /* Must decrement the refcount */
return 0;
 }
 
-- 
2.9.4