Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1736145 > unrolled thread

[PATCH 1/2] powerpc/pseries: fix "OF: ERROR: Bad of_node_put() on /cpus" during DLPAR

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 1/2] powerpc/pseries: fix "OF: ERROR: Bad of_node_put() on /cpus" during DLPAR Tyrel Datwyler <tyreld@linux.vnet.ibm.com> - 2017-09-20 23:10 +0200
    Re: [PATCH 1/2] powerpc/pseries: fix "OF: ERROR: Bad of_node_put() on /cpus" during DLPAR Michael Ellerman <mpe@ellerman.id.au> - 2017-09-21 12:00 +0200
      Re: [PATCH 1/2] powerpc/pseries: fix "OF: ERROR: Bad of_node_put() on  /cpus" during DLPAR Tyrel Datwyler <tyreld@linux.vnet.ibm.com> - 2017-09-21 20:50 +0200
    Re: [1/2] powerpc/pseries: fix "OF: ERROR: Bad of_node_put() on /cpus" during DLPAR Michael Ellerman <patch-notifications@ellerman.id.au> - 2017-09-22 03:10 +0200
      Re: [1/2] powerpc/pseries: fix "OF: ERROR: Bad of_node_put() on  /cpus" during DLPAR Abdul Haleem <abdhalee@linux.vnet.ibm.com> - 2017-09-22 12:10 +0200

#1736145 — [PATCH 1/2] powerpc/pseries: fix "OF: ERROR: Bad of_node_put() on /cpus" during DLPAR

Commit 215ee763f8cb ("powerpc: pseries: remove dlpar_attach_node dependency on
full path") reworked dlpar_attach_node() to no longer look up the parent
node "/cpus", but instead to have the parent node passed by the caller in the
function parameter list. As a result dlpar_attach_node() is no longer
responsible for freeing the reference to the parent node. However,
commit 215ee763f8cb failed to remove the of_node_put(parent) call in
dlpar_attach_node(), or to take into account that the reference to the
parent in the caller dlpar_cpu_add() needs to be held until after
dlpar_attach_node() returns. As a result doing repeated cpu add/remove dlpar
operations will eventually result in the following error:

OF: ERROR: Bad of_node_put() on /cpus
CPU: 0 PID: 10896 Comm: drmgr Not tainted 4.13.0-autotest #1
Call Trace:
[c00000026ecdf810] [c00000000278a2a4] dump_stack+0x15c/0x1f8
(unreliable)
[c00000026ecdf850] [c0000000025371a4] of_node_release+0x1a4/0x1c0
[c00000026ecdf8e0] [c0000000027948c8] kobject_put+0x1a8/0x310
[c00000026ecdf960] [c000000002794bdc] kobject_del+0xbc/0xf0
[c00000026ecdf990] [c000000002535ff4] __of_detach_node_sysfs+0x144/0x210
[c00000026ecdf9d0] [c000000002536f70] of_detach_node+0xf0/0x180
[c00000026ecdfa40] [c0000000016ed494] dlpar_detach_node+0xc4/0x120
[c00000026ecdfa80] [c0000000016f47d0] dlpar_cpu_remove+0x280/0x560
[c00000026ecdfb60] [c0000000016f4d9c] dlpar_cpu_release+0xbc/0x1b0
[c00000026ecdfbb0] [c00000000161279c] arch_cpu_release+0x6c/0xb0
[c00000026ecdfbe0] [c00000000218ebf0] cpu_release_store+0xa0/0x100
[c00000026ecdfc20] [c000000002178388] dev_attr_store+0x68/0xa0
[c00000026ecdfc50] [c000000001bfaae8] sysfs_kf_write+0xa8/0xf0
[c00000026ecdfc80] [c000000001bf8a3c] kernfs_fop_write+0x2cc/0x400
[c00000026ecdfce0] [c000000001ad33fc] __vfs_write+0x5c/0x340
[c00000026ecdfd80] [c000000001ad89e8] vfs_write+0x1a8/0x3d0
[c00000026ecdfdd0] [c000000001ad9178] SyS_write+0xa8/0x1a0
[c00000026ecdfe30] [c0000000015eb8e0] system_call+0x58/0x6c

Fix the issue by removing the of_node_put(parent) call from
dlpar_attach_node(), and ensuring that the reference to the parent node
is properly held and released by the caller dlpar_cpu_add().

Cc: stable@vger.kernel.org # v4.13+
Fixes: 215ee763f8cb ("powerpc: pseries: remove dlpar_attach_node dependency on full path")
Signed-off-by: Tyrel Datwyler <tyreld@linux.vnet.ibm.com>
Reported-by: Abdul Haleem <abdhalee@linux.vnet.ibm.com>
---
 arch/powerpc/platforms/pseries/dlpar.c       | 1 -
 arch/powerpc/platforms/pseries/hotplug-cpu.c | 4 +++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/dlpar.c b/arch/powerpc/platforms/pseries/dlpar.c
index 783f363..e45b5f1 100644
--- a/arch/powerpc/platforms/pseries/dlpar.c
+++ b/arch/powerpc/platforms/pseries/dlpar.c
@@ -266,7 +266,6 @@ int dlpar_attach_node(struct device_node *dn, struct device_node *parent)
 		return rc;
 	}
 
-	of_node_put(dn->parent);
 	return 0;
 }
 
diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
index fc0d8f9..473d817 100644
--- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
+++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
@@ -462,15 +462,17 @@ static ssize_t dlpar_cpu_add(u32 drc_index)
 	}
 
 	dn = dlpar_configure_connector(cpu_to_be32(drc_index), parent);
-	of_node_put(parent);
 	if (!dn) {
 		pr_warn("Failed call to configure-connector, drc index: %x\n",
 			drc_index);
 		dlpar_release_drc(drc_index);
+		of_node_put(parent);
 		return -EINVAL;
 	}
 
 	rc = dlpar_attach_node(dn, parent);
+	of_node_put(parent);
+
 	if (rc) {
 		saved_rc = rc;
 		pr_warn("Failed to attach node %s, rc: %d, drc index: %x\n",
-- 
1.8.3.1

[toc] | [next] | [standalone]

#1736512

Hi Tyrel,

Thanks for jumping on this.

Tyrel Datwyler <tyreld@linux.vnet.ibm.com> writes:
> Commit 215ee763f8cb ("powerpc: pseries: remove dlpar_attach_node dependency on
> full path") reworked dlpar_attach_node() to no longer look up the parent
> node "/cpus", but instead to have the parent node passed by the caller in the
> function parameter list. As a result dlpar_attach_node() is no longer
> responsible for freeing the reference to the parent node. However,
> commit 215ee763f8cb failed to remove the of_node_put(parent) call in
> dlpar_attach_node(), or to take into account that the reference to the
> parent in the caller dlpar_cpu_add() needs to be held until after
> dlpar_attach_node() returns. As a result doing repeated cpu add/remove dlpar
> operations will eventually result in the following error:
>
> OF: ERROR: Bad of_node_put() on /cpus
> CPU: 0 PID: 10896 Comm: drmgr Not tainted 4.13.0-autotest #1
> Call Trace:
> [c00000026ecdf810] [c00000000278a2a4] dump_stack+0x15c/0x1f8
> (unreliable)
> [c00000026ecdf850] [c0000000025371a4] of_node_release+0x1a4/0x1c0
> [c00000026ecdf8e0] [c0000000027948c8] kobject_put+0x1a8/0x310
> [c00000026ecdf960] [c000000002794bdc] kobject_del+0xbc/0xf0
> [c00000026ecdf990] [c000000002535ff4] __of_detach_node_sysfs+0x144/0x210
> [c00000026ecdf9d0] [c000000002536f70] of_detach_node+0xf0/0x180
> [c00000026ecdfa40] [c0000000016ed494] dlpar_detach_node+0xc4/0x120
> [c00000026ecdfa80] [c0000000016f47d0] dlpar_cpu_remove+0x280/0x560
> [c00000026ecdfb60] [c0000000016f4d9c] dlpar_cpu_release+0xbc/0x1b0
> [c00000026ecdfbb0] [c00000000161279c] arch_cpu_release+0x6c/0xb0
> [c00000026ecdfbe0] [c00000000218ebf0] cpu_release_store+0xa0/0x100
> [c00000026ecdfc20] [c000000002178388] dev_attr_store+0x68/0xa0
> [c00000026ecdfc50] [c000000001bfaae8] sysfs_kf_write+0xa8/0xf0
> [c00000026ecdfc80] [c000000001bf8a3c] kernfs_fop_write+0x2cc/0x400
> [c00000026ecdfce0] [c000000001ad33fc] __vfs_write+0x5c/0x340
> [c00000026ecdfd80] [c000000001ad89e8] vfs_write+0x1a8/0x3d0
> [c00000026ecdfdd0] [c000000001ad9178] SyS_write+0xa8/0x1a0
> [c00000026ecdfe30] [c0000000015eb8e0] system_call+0x58/0x6c

I usually omit all the addresses in the change log, they don't add much
for future readers. I fixed it up myself.

> Fix the issue by removing the of_node_put(parent) call from
> dlpar_attach_node(), and ensuring that the reference to the parent node
> is properly held and released by the caller dlpar_cpu_add().
>
> Fixes: 215ee763f8cb ("powerpc: pseries: remove dlpar_attach_node dependency on full path")
> Cc: stable@vger.kernel.org # v4.13+

It doesn't need to go to stable, v4.13 doesn't have that commit, it's
only in v4.14-rc1.

I dropped the stable tag.

> diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> index fc0d8f9..473d817 100644
> --- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
> +++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> @@ -462,15 +462,17 @@ static ssize_t dlpar_cpu_add(u32 drc_index)
>  	}
>  
>  	dn = dlpar_configure_connector(cpu_to_be32(drc_index), parent);
> -	of_node_put(parent);
>  	if (!dn) {
>  		pr_warn("Failed call to configure-connector, drc index: %x\n",
>  			drc_index);
>  		dlpar_release_drc(drc_index);
> +		of_node_put(parent);
>  		return -EINVAL;
>  	}
>  
>  	rc = dlpar_attach_node(dn, parent);

I added a comment here that I had in my version:

+       /* Regardless we are done with parent now */

> +	of_node_put(parent);
> +
>  	if (rc) {
>  		saved_rc = rc;
>  		pr_warn("Failed to attach node %s, rc: %d, drc index: %x\n",

cheers

[toc] | [prev] | [next] | [standalone]

#1737008 — Re: [PATCH 1/2] powerpc/pseries: fix "OF: ERROR: Bad of_node_put() on /cpus" during DLPAR

On 09/21/2017 02:54 AM, Michael Ellerman wrote:
> Hi Tyrel,
> 
> Thanks for jumping on this.
> 
> Tyrel Datwyler <tyreld@linux.vnet.ibm.com> writes:
>> Commit 215ee763f8cb ("powerpc: pseries: remove dlpar_attach_node dependency on
>> full path") reworked dlpar_attach_node() to no longer look up the parent
>> node "/cpus", but instead to have the parent node passed by the caller in the
>> function parameter list. As a result dlpar_attach_node() is no longer
>> responsible for freeing the reference to the parent node. However,
>> commit 215ee763f8cb failed to remove the of_node_put(parent) call in
>> dlpar_attach_node(), or to take into account that the reference to the
>> parent in the caller dlpar_cpu_add() needs to be held until after
>> dlpar_attach_node() returns. As a result doing repeated cpu add/remove dlpar
>> operations will eventually result in the following error:
>>
>> OF: ERROR: Bad of_node_put() on /cpus
>> CPU: 0 PID: 10896 Comm: drmgr Not tainted 4.13.0-autotest #1
>> Call Trace:
>> [c00000026ecdf810] [c00000000278a2a4] dump_stack+0x15c/0x1f8
>> (unreliable)
>> [c00000026ecdf850] [c0000000025371a4] of_node_release+0x1a4/0x1c0
>> [c00000026ecdf8e0] [c0000000027948c8] kobject_put+0x1a8/0x310
>> [c00000026ecdf960] [c000000002794bdc] kobject_del+0xbc/0xf0
>> [c00000026ecdf990] [c000000002535ff4] __of_detach_node_sysfs+0x144/0x210
>> [c00000026ecdf9d0] [c000000002536f70] of_detach_node+0xf0/0x180
>> [c00000026ecdfa40] [c0000000016ed494] dlpar_detach_node+0xc4/0x120
>> [c00000026ecdfa80] [c0000000016f47d0] dlpar_cpu_remove+0x280/0x560
>> [c00000026ecdfb60] [c0000000016f4d9c] dlpar_cpu_release+0xbc/0x1b0
>> [c00000026ecdfbb0] [c00000000161279c] arch_cpu_release+0x6c/0xb0
>> [c00000026ecdfbe0] [c00000000218ebf0] cpu_release_store+0xa0/0x100
>> [c00000026ecdfc20] [c000000002178388] dev_attr_store+0x68/0xa0
>> [c00000026ecdfc50] [c000000001bfaae8] sysfs_kf_write+0xa8/0xf0
>> [c00000026ecdfc80] [c000000001bf8a3c] kernfs_fop_write+0x2cc/0x400
>> [c00000026ecdfce0] [c000000001ad33fc] __vfs_write+0x5c/0x340
>> [c00000026ecdfd80] [c000000001ad89e8] vfs_write+0x1a8/0x3d0
>> [c00000026ecdfdd0] [c000000001ad9178] SyS_write+0xa8/0x1a0
>> [c00000026ecdfe30] [c0000000015eb8e0] system_call+0x58/0x6c
> 
> I usually omit all the addresses in the change log, they don't add much
> for future readers. I fixed it up myself.

Ok, good point. I'll be sure to try and remember for future patches.

> 
>> Fix the issue by removing the of_node_put(parent) call from
>> dlpar_attach_node(), and ensuring that the reference to the parent node
>> is properly held and released by the caller dlpar_cpu_add().
>>
>> Fixes: 215ee763f8cb ("powerpc: pseries: remove dlpar_attach_node dependency on full path")
>> Cc: stable@vger.kernel.org # v4.13+
> 
> It doesn't need to go to stable, v4.13 doesn't have that commit, it's
> only in v4.14-rc1.

My bad, Abdul mentioned 4.13.0 in his email, but sure enough you are correct.

[root@ltcalpine2-lp2 linux]# git tag --contains 215ee763f8cb
v4.14-rc1

Thanks for catching that.

> 
> I dropped the stable tag.
> 
>> diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
>> index fc0d8f9..473d817 100644
>> --- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
>> +++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
>> @@ -462,15 +462,17 @@ static ssize_t dlpar_cpu_add(u32 drc_index)
>>  	}
>>  
>>  	dn = dlpar_configure_connector(cpu_to_be32(drc_index), parent);
>> -	of_node_put(parent);
>>  	if (!dn) {
>>  		pr_warn("Failed call to configure-connector, drc index: %x\n",
>>  			drc_index);
>>  		dlpar_release_drc(drc_index);
>> +		of_node_put(parent);
>>  		return -EINVAL;
>>  	}
>>  
>>  	rc = dlpar_attach_node(dn, parent);
> 
> I added a comment here that I had in my version:
Sure np.

-Tyrel

> 
> +       /* Regardless we are done with parent now */
> 
>> +	of_node_put(parent);
>> +
>>  	if (rc) {
>>  		saved_rc = rc;
>>  		pr_warn("Failed to attach node %s, rc: %d, drc index: %x\n",
> 
> cheers
> 

[toc] | [prev] | [next] | [standalone]

#1737117 — Re: [1/2] powerpc/pseries: fix "OF: ERROR: Bad of_node_put() on /cpus" during DLPAR

On Wed, 2017-09-20 at 21:02:51 UTC, Tyrel Datwyler wrote:
> Commit 215ee763f8cb ("powerpc: pseries: remove dlpar_attach_node dependency on
> full path") reworked dlpar_attach_node() to no longer look up the parent
> node "/cpus", but instead to have the parent node passed by the caller in the
> function parameter list. As a result dlpar_attach_node() is no longer
> responsible for freeing the reference to the parent node. However,
> commit 215ee763f8cb failed to remove the of_node_put(parent) call in
> dlpar_attach_node(), or to take into account that the reference to the
> parent in the caller dlpar_cpu_add() needs to be held until after
> dlpar_attach_node() returns. As a result doing repeated cpu add/remove dlpar
> operations will eventually result in the following error:
> 
> OF: ERROR: Bad of_node_put() on /cpus
> CPU: 0 PID: 10896 Comm: drmgr Not tainted 4.13.0-autotest #1
> Call Trace:
> [c00000026ecdf810] [c00000000278a2a4] dump_stack+0x15c/0x1f8
> (unreliable)
> [c00000026ecdf850] [c0000000025371a4] of_node_release+0x1a4/0x1c0
> [c00000026ecdf8e0] [c0000000027948c8] kobject_put+0x1a8/0x310
> [c00000026ecdf960] [c000000002794bdc] kobject_del+0xbc/0xf0
> [c00000026ecdf990] [c000000002535ff4] __of_detach_node_sysfs+0x144/0x210
> [c00000026ecdf9d0] [c000000002536f70] of_detach_node+0xf0/0x180
> [c00000026ecdfa40] [c0000000016ed494] dlpar_detach_node+0xc4/0x120
> [c00000026ecdfa80] [c0000000016f47d0] dlpar_cpu_remove+0x280/0x560
> [c00000026ecdfb60] [c0000000016f4d9c] dlpar_cpu_release+0xbc/0x1b0
> [c00000026ecdfbb0] [c00000000161279c] arch_cpu_release+0x6c/0xb0
> [c00000026ecdfbe0] [c00000000218ebf0] cpu_release_store+0xa0/0x100
> [c00000026ecdfc20] [c000000002178388] dev_attr_store+0x68/0xa0
> [c00000026ecdfc50] [c000000001bfaae8] sysfs_kf_write+0xa8/0xf0
> [c00000026ecdfc80] [c000000001bf8a3c] kernfs_fop_write+0x2cc/0x400
> [c00000026ecdfce0] [c000000001ad33fc] __vfs_write+0x5c/0x340
> [c00000026ecdfd80] [c000000001ad89e8] vfs_write+0x1a8/0x3d0
> [c00000026ecdfdd0] [c000000001ad9178] SyS_write+0xa8/0x1a0
> [c00000026ecdfe30] [c0000000015eb8e0] system_call+0x58/0x6c
> 
> Fix the issue by removing the of_node_put(parent) call from
> dlpar_attach_node(), and ensuring that the reference to the parent node
> is properly held and released by the caller dlpar_cpu_add().
> 
> Cc: stable@vger.kernel.org # v4.13+
> Fixes: 215ee763f8cb ("powerpc: pseries: remove dlpar_attach_node dependency on full path")
> Signed-off-by: Tyrel Datwyler <tyreld@linux.vnet.ibm.com>
> Reported-by: Abdul Haleem <abdhalee@linux.vnet.ibm.com>

Series applied to powerpc fixes, thanks.

https://git.kernel.org/powerpc/c/087ff6a5ae3052bb2835e191094b79

cheers

[toc] | [prev] | [next] | [standalone]

#1737365 — Re: [1/2] powerpc/pseries: fix "OF: ERROR: Bad of_node_put() on /cpus" during DLPAR

On Fri, 2017-09-22 at 11:03 +1000, Michael Ellerman wrote:
> On Wed, 2017-09-20 at 21:02:51 UTC, Tyrel Datwyler wrote:
> > Commit 215ee763f8cb ("powerpc: pseries: remove dlpar_attach_node dependency on
> > full path") reworked dlpar_attach_node() to no longer look up the parent
> > node "/cpus", but instead to have the parent node passed by the caller in the
> > function parameter list. As a result dlpar_attach_node() is no longer
> > responsible for freeing the reference to the parent node. However,
> > commit 215ee763f8cb failed to remove the of_node_put(parent) call in
> > dlpar_attach_node(), or to take into account that the reference to the
> > parent in the caller dlpar_cpu_add() needs to be held until after
> > dlpar_attach_node() returns. As a result doing repeated cpu add/remove dlpar
> > operations will eventually result in the following error:
> > 
> > OF: ERROR: Bad of_node_put() on /cpus
> > CPU: 0 PID: 10896 Comm: drmgr Not tainted 4.13.0-autotest #1
> > Call Trace:
> > [c00000026ecdf810] [c00000000278a2a4] dump_stack+0x15c/0x1f8
> > (unreliable)
> > [c00000026ecdf850] [c0000000025371a4] of_node_release+0x1a4/0x1c0
> > [c00000026ecdf8e0] [c0000000027948c8] kobject_put+0x1a8/0x310
> > [c00000026ecdf960] [c000000002794bdc] kobject_del+0xbc/0xf0
> > [c00000026ecdf990] [c000000002535ff4] __of_detach_node_sysfs+0x144/0x210
> > [c00000026ecdf9d0] [c000000002536f70] of_detach_node+0xf0/0x180
> > [c00000026ecdfa40] [c0000000016ed494] dlpar_detach_node+0xc4/0x120
> > [c00000026ecdfa80] [c0000000016f47d0] dlpar_cpu_remove+0x280/0x560
> > [c00000026ecdfb60] [c0000000016f4d9c] dlpar_cpu_release+0xbc/0x1b0
> > [c00000026ecdfbb0] [c00000000161279c] arch_cpu_release+0x6c/0xb0
> > [c00000026ecdfbe0] [c00000000218ebf0] cpu_release_store+0xa0/0x100
> > [c00000026ecdfc20] [c000000002178388] dev_attr_store+0x68/0xa0
> > [c00000026ecdfc50] [c000000001bfaae8] sysfs_kf_write+0xa8/0xf0
> > [c00000026ecdfc80] [c000000001bf8a3c] kernfs_fop_write+0x2cc/0x400
> > [c00000026ecdfce0] [c000000001ad33fc] __vfs_write+0x5c/0x340
> > [c00000026ecdfd80] [c000000001ad89e8] vfs_write+0x1a8/0x3d0
> > [c00000026ecdfdd0] [c000000001ad9178] SyS_write+0xa8/0x1a0
> > [c00000026ecdfe30] [c0000000015eb8e0] system_call+0x58/0x6c
> > 
> > Fix the issue by removing the of_node_put(parent) call from
> > dlpar_attach_node(), and ensuring that the reference to the parent node
> > is properly held and released by the caller dlpar_cpu_add().
> > 
> > Cc: stable@vger.kernel.org # v4.13+
> > Fixes: 215ee763f8cb ("powerpc: pseries: remove dlpar_attach_node dependency on full path")
> > Signed-off-by: Tyrel Datwyler <tyreld@linux.vnet.ibm.com>
> > Reported-by: Abdul Haleem <abdhalee@linux.vnet.ibm.com>
> 
> Series applied to powerpc fixes, thanks.
> 
> https://git.kernel.org/powerpc/c/087ff6a5ae3052bb2835e191094b79

The patch fixes the problem, No warnings seen for 100 iterations of
DLPAR CPU add/remove operation.

Thanks Tyrel and Michael.

Tested-by: Abdul Haleem <abdhalee@linux.vnet.ibm.com>

-- 
Regard's

Abdul Haleem
IBM Linux Technology Centre

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web