| Message ID | 553FA94D.2000608@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | f32393c943e297b8ae180c8f83d81a156c7d0412 |
| Delegated to: | Michael Ellerman |
| Headers | show |
Subject should be "powerpc/pseries: ..." please. On Tue, 2015-04-28 at 10:37 -0500, Nathan Fontenot wrote: > The incorrect ordering of operations during cpu dlpar causes the affinity > of cpus being added to be invalid. Phyp does not assign affinity information > for a cpu until the rtas set-indicator calls are made to set the isolation > and allocation state. In the current code we call rtas configure-connector > before making the set-indicator calls which results in invalid data in the > ibm,associativity property for the cpu we're adding. Invalid and benign? Or invalid and causes an oops or ..? > This patch corrects the order of operations to make the set-indicator > calls (done in acquire_drc) before calling configure-connector. Which commit added the code and/or caused it to be wrong? https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches#n187 While looking at the code I notice it looks like we leak a reference if dlpar_configure_connector() fails: parent = of_find_node_by_path("/cpus"); if (!parent) return -ENODEV; dn = dlpar_configure_connector(cpu_to_be32(drc_index), parent); if (!dn) return -EINVAL; of_node_put(parent); Please send a separate patch to fix that. cheers
On 04/28/2015 11:53 PM, Michael Ellerman wrote: > Subject should be "powerpc/pseries: ..." please. ok > > On Tue, 2015-04-28 at 10:37 -0500, Nathan Fontenot wrote: >> The incorrect ordering of operations during cpu dlpar causes the affinity >> of cpus being added to be invalid. Phyp does not assign affinity information >> for a cpu until the rtas set-indicator calls are made to set the isolation >> and allocation state. In the current code we call rtas configure-connector >> before making the set-indicator calls which results in invalid data in the >> ibm,associativity property for the cpu we're adding. > > Invalid and benign? Or invalid and causes an oops or ..? > Invalid and benign. I'll add this to the description. >> This patch corrects the order of operations to make the set-indicator >> calls (done in acquire_drc) before calling configure-connector. > > Which commit added the code and/or caused it to be wrong? > > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches#n187 > And the commit that this fixes.. > > While looking at the code I notice it looks like we leak a reference if > dlpar_configure_connector() fails: > > parent = of_find_node_by_path("/cpus"); > if (!parent) > return -ENODEV; > > dn = dlpar_configure_connector(cpu_to_be32(drc_index), parent); > if (!dn) > return -EINVAL; > > of_node_put(parent); > > > Please send a separate patch to fix that. Yep, saw that too. New patches on their way. Thanks for the feedback, -Nathan
diff --git a/arch/powerpc/platforms/pseries/dlpar.c b/arch/powerpc/platforms/pseries/dlpar.c index b4b1109..019d34a 100644 --- a/arch/powerpc/platforms/pseries/dlpar.c +++ b/arch/powerpc/platforms/pseries/dlpar.c @@ -412,6 +412,10 @@ static ssize_t dlpar_cpu_probe(const char *buf, size_t count) if (rc) return -EINVAL; + rc = dlpar_acquire_drc(drc_index); + if (rc) + return -EINVAL; + parent = of_find_node_by_path("/cpus"); if (!parent) return -ENODEV; @@ -422,12 +426,6 @@ static ssize_t dlpar_cpu_probe(const char *buf, size_t count) of_node_put(parent); - rc = dlpar_acquire_drc(drc_index); - if (rc) { - dlpar_free_cc_nodes(dn); - return -EINVAL; - } - rc = dlpar_attach_node(dn); if (rc) { dlpar_release_drc(drc_index);
The incorrect ordering of operations during cpu dlpar causes the affinity of cpus being added to be invalid. Phyp does not assign affinity information for a cpu until the rtas set-indicator calls are made to set the isolation and allocation state. In the current code we call rtas configure-connector before making the set-indicator calls which results in invalid data in the ibm,associativity property for the cpu we're adding. This patch corrects the order of operations to make the set-indicator calls (done in acquire_drc) before calling configure-connector. Signed-off-by: Nathan Fontenot <nfont@linux.vnet.ibm.com> --- arch/powerpc/platforms/pseries/dlpar.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)