[PATCH 0/3] powerpc/pseries: Update affinity index during memory dlpar

[PATCH 0/3] powerpc/pseries: Update affinity index during memory dlpar

[Nathan Fontenot]

When adding or removing a LMB the associativity index for the LMB
needs to be updated in the device tree property ibm,dynamic-memory.

Without updating the associativity index for a LMB before adding the
LMB it could be added with the incorrect affinity. For LMBs that are
not present at boot the associativity index in the device tree property
is set to 0xffffffff, the result being that the LMB is added to the
first online node. For an LMB that was present at boot but was later
DLPAR removed, the LMB associativity may be stale causing the LMB
to be added to the incorrect node.

This set of patches updates the memory DLPAR add path to discover the
associativity index for a LMB and update the device tree property prior
to adding the memory for that LMB. This also updates the DLPAR remove
path to set the associativity back to 0xffffffff when a LMB is removed.

Patch 1/3 refactors the dlpar LMB add code to make further updates easier,
there are no functional changes.

Patch 2/3 updates the add and remove paths to modify the associativity
index for the LMB being added or removed.

Patch 3/3 removes some no longer needed conversions of the device tree
property from BE to cpu format.

-Nathan

 hotplug-memory.c |  306 ++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 214 insertions(+), 92 deletions(-)

[PATCH 1/3] powerpc/pseries: Refactor dlpar_add_lmb() code

[Nathan Fontenot]

Re-factor dlpar_lmb_add() routine by moving the validation of the lmb
flags and the acquireing of the DRC to a wrapper around the work to add
the memory to the system. This is done to make handling of errors
during the addition of the memory easier and to facilitate the upcoming
addition of updating the lmb's affinity prior to adding the memory.

Signed-off-by: Nathan Fontenot <nfont@linux.vnet.ibm.com>
---
 arch/powerpc/platforms/pseries/hotplug-memory.c |   34 ++++++++++++++---------
 1 file changed, 20 insertions(+), 14 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
index e9ff44c..294acfd 100644
--- a/arch/powerpc/platforms/pseries/hotplug-memory.c
+++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
@@ -384,43 +384,32 @@ static int dlpar_memory_remove_by_index(u32 drc_index, struct property *prop)
 
 #endif /* CONFIG_MEMORY_HOTREMOVE */
 
-static int dlpar_add_lmb(struct of_drconf_cell *lmb)
+static int dlpar_add_lmb_memory(struct of_drconf_cell *lmb)
 {
 	struct memory_block *mem_block;
 	unsigned long block_sz;
 	int nid, rc;
 
-	if (lmb->flags & DRCONF_MEM_ASSIGNED)
-		return -EINVAL;
-
 	block_sz = memory_block_size_bytes();
 
-	rc = dlpar_acquire_drc(lmb->drc_index);
-	if (rc)
-		return rc;
-
 	/* Find the node id for this address */
 	nid = memory_add_physaddr_to_nid(lmb->base_addr);
 
 	/* Add the memory */
 	rc = add_memory(nid, lmb->base_addr, block_sz);
-	if (rc) {
-		dlpar_release_drc(lmb->drc_index);
+	if (rc)
 		return rc;
-	}
 
 	/* Register this block of memory */
 	rc = memblock_add(lmb->base_addr, block_sz);
 	if (rc) {
 		remove_memory(nid, lmb->base_addr, block_sz);
-		dlpar_release_drc(lmb->drc_index);
 		return rc;
 	}
 
 	mem_block = lmb_to_memblock(lmb);
 	if (!mem_block) {
 		remove_memory(nid, lmb->base_addr, block_sz);
-		dlpar_release_drc(lmb->drc_index);
 		return -EINVAL;
 	}
 
@@ -428,7 +417,6 @@ static int dlpar_add_lmb(struct of_drconf_cell *lmb)
 	put_device(&mem_block->dev);
 	if (rc) {
 		remove_memory(nid, lmb->base_addr, block_sz);
-		dlpar_release_drc(lmb->drc_index);
 		return rc;
 	}
 
@@ -436,6 +424,24 @@ static int dlpar_add_lmb(struct of_drconf_cell *lmb)
 	return 0;
 }
 
+static int dlpar_add_lmb(struct of_drconf_cell *lmb)
+{
+	int rc;
+
+	if (lmb->flags & DRCONF_MEM_ASSIGNED)
+		return -EINVAL;
+
+	rc = dlpar_acquire_drc(lmb->drc_index);
+	if (rc)
+		return rc;
+
+	rc = dlpar_add_lmb_memory(lmb);
+	if (rc)
+		dlpar_release_drc(lmb->drc_index);
+
+	return rc;
+}
+
 static int dlpar_memory_add_by_count(u32 lmbs_to_add, struct property *prop)
 {
 	struct of_drconf_cell *lmbs;

Re: [1/3] powerpc/pseries: Refactor dlpar_add_lmb() code

[Michael Ellerman]

On Wed, 2016-10-02 at 17:10:44 UTC, Nathan Fontenot wrote:
> Re-factor dlpar_lmb_add() routine by moving the validation of the lmb
> flags and the acquireing of the DRC to a wrapper around the work to add
> the memory to the system. This is done to make handling of errors
> during the addition of the memory easier and to facilitate the upcoming
> addition of updating the lmb's affinity prior to adding the memory.
> 
> Signed-off-by: Nathan Fontenot <nfont@linux.vnet.ibm.com>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/4a4bdfea7cb75b5c8b22293279

cheers

[PATCH 2/3] powerpc/pseries: Update LMB associativity index during DLPAR add/remove

[Nathan Fontenot]

The associativity array index specified for a LMB in the device tree,
/ibm,dynamic-reconfiguration-memory/ibm,dynamic-memory, needs to be updated
prior to DLPAR adding a LMB and after DLPAR removing a LMB.

Without doing this step in the DLPAR add process a LMB could be configured
with the incorrect affinity. For a LMB that was not present at boot the
affinity index is set to 0xffffffff, which defaults to adding the LMB to
the first online node since the index is not a valid value. Or, the
affinity index could contain a stale value if the LMB was present at boot
but later DLPAR removed and is being DLPAR added back to the system.

This patch adds a step in the DLPAR add flow to look up the associativity
index for a LMB prior to adding a LMB and setting the associativity to
0xffffffff when a LMB is removed.

This patch also modifies the DLPAR add/remove flow to no longer do a single
update of the device tree property after all of the requested DLPAR
operations are complete and now does a property update during the add
or remove of each LMB.

Signed-off-by: Nathan Fontenot <nfont@linux.vnet.ibm.com>
---
 arch/powerpc/platforms/pseries/hotplug-memory.c |  193 +++++++++++++++++++----
 1 file changed, 162 insertions(+), 31 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
index 294acfd..2ce1385 100644
--- a/arch/powerpc/platforms/pseries/hotplug-memory.c
+++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
@@ -116,6 +116,155 @@ static struct property *dlpar_clone_drconf_property(struct device_node *dn)
 	return new_prop;
 }
 
+static void dlpar_update_drconf_property(struct device_node *dn,
+					 struct property *prop)
+{
+	struct of_drconf_cell *lmbs;
+	u32 num_lmbs, *p;
+	int i;
+
+	/* Convert the property back to BE */
+	p = prop->value;
+	num_lmbs = *p;
+	*p = cpu_to_be32(*p);
+	p++;
+
+	lmbs = (struct of_drconf_cell *)p;
+	for (i = 0; i < num_lmbs; i++) {
+		lmbs[i].base_addr = cpu_to_be64(lmbs[i].base_addr);
+		lmbs[i].drc_index = cpu_to_be32(lmbs[i].drc_index);
+		lmbs[i].flags = cpu_to_be32(lmbs[i].flags);
+	}
+
+	rtas_hp_event = true;
+	of_update_property(dn, prop);
+	rtas_hp_event = false;
+}
+
+static int dlpar_update_device_tree_lmb(struct of_drconf_cell *lmb)
+{
+	struct device_node *dn;
+	struct property *prop;
+	struct of_drconf_cell *lmbs;
+	u32 *p, num_lmbs;
+	int i;
+
+	dn = of_find_node_by_path("/ibm,dynamic-reconfiguration-memory");
+	if (!dn)
+		return -ENODEV;
+
+	prop = dlpar_clone_drconf_property(dn);
+	if (!prop) {
+		of_node_put(dn);
+		return -ENODEV;
+	}
+
+	p = prop->value;
+	num_lmbs = *p++;
+	lmbs = (struct of_drconf_cell *)p;
+
+	for (i = 0; i < num_lmbs; i++) {
+		if (lmbs[i].drc_index == lmb->drc_index) {
+			lmbs[i].flags = lmb->flags;
+			lmbs[i].aa_index = lmb->aa_index;
+
+			dlpar_update_drconf_property(dn, prop);
+			break;
+		}
+	}
+
+	of_node_put(dn);
+	return 0;
+}
+
+static u32 lookup_lmb_associativity_index(struct of_drconf_cell *lmb)
+{
+	struct device_node *parent, *lmb_node, *dr_node;
+	const u32 *lmb_assoc;
+	const u32 *assoc_arrays;
+	u32 aa_index;
+	int aa_arrays, aa_array_entries, aa_array_sz;
+	int i;
+
+	parent = of_find_node_by_path("/");
+	if (!parent)
+		return -ENODEV;
+
+	lmb_node = dlpar_configure_connector(cpu_to_be32(lmb->drc_index),
+					     parent);
+	of_node_put(parent);
+	if (!lmb_node)
+		return -EINVAL;
+
+	lmb_assoc = of_get_property(lmb_node, "ibm,associativity", NULL);
+	if (!lmb_assoc) {
+		dlpar_free_cc_nodes(lmb_node);
+		return -ENODEV;
+	}
+
+	dr_node = of_find_node_by_path("/ibm,dynamic-reconfiguration-memory");
+	if (!dr_node) {
+		dlpar_free_cc_nodes(lmb_node);
+		return -ENODEV;
+	}
+
+	assoc_arrays = of_get_property(dr_node,
+				       "ibm,associativity-lookup-arrays",
+				       NULL);
+	of_node_put(dr_node);
+	if (!assoc_arrays) {
+		dlpar_free_cc_nodes(lmb_node);
+		return -ENODEV;
+	}
+
+	/* The ibm,associativity-lookup-arrays property is defined to be
+	 * a 32-bit value specifying the number of associativity arrays
+	 * followed by a 32-bitvalue specifying the number of entries per
+	 * array, followed by the associativity arrays.
+	 */
+	aa_arrays = be32_to_cpu(assoc_arrays[0]);
+	aa_array_entries = be32_to_cpu(assoc_arrays[1]);
+	aa_array_sz = aa_array_entries * sizeof(u32);
+
+	aa_index = -1;
+	for (i = 0; i < aa_arrays; i++) {
+		int indx = (i * aa_array_entries) + 2;
+
+		if (memcmp(&assoc_arrays[indx], &lmb_assoc[1], aa_array_sz))
+			continue;
+
+		aa_index = i;
+		break;
+	}
+
+	dlpar_free_cc_nodes(lmb_node);
+	return aa_index;
+}
+
+static int dlpar_add_device_tree_lmb(struct of_drconf_cell *lmb)
+{
+	int aa_index;
+
+	lmb->flags |= DRCONF_MEM_ASSIGNED;
+
+	aa_index = lookup_lmb_associativity_index(lmb);
+	if (aa_index < 0) {
+		pr_err("Couldn't find associativity index for drc index %x\n",
+		       lmb->drc_index);
+		return aa_index;
+	}
+
+	lmb->aa_index = aa_index;
+	return dlpar_update_device_tree_lmb(lmb);
+}
+
+static int dlpar_remove_device_tree_lmb(struct of_drconf_cell *lmb)
+{
+	lmb->flags &= ~DRCONF_MEM_ASSIGNED;
+	lmb->aa_index = 0xffffffff;
+	return dlpar_update_device_tree_lmb(lmb);
+}
+
 static struct memory_block *lmb_to_memblock(struct of_drconf_cell *lmb)
 {
 	unsigned long section_nr;
@@ -243,8 +392,8 @@ static int dlpar_remove_lmb(struct of_drconf_cell *lmb)
 	memblock_remove(lmb->base_addr, block_sz);
 
 	dlpar_release_drc(lmb->drc_index);
+	dlpar_remove_device_tree_lmb(lmb);
 
-	lmb->flags &= ~DRCONF_MEM_ASSIGNED;
 	return 0;
 }
 
@@ -435,9 +584,19 @@ static int dlpar_add_lmb(struct of_drconf_cell *lmb)
 	if (rc)
 		return rc;
 
+	rc = dlpar_add_device_tree_lmb(lmb);
+	if (rc) {
+		pr_err("Couldn't update device tree for drc index %x\n",
+		       lmb->drc_index);
+		dlpar_release_drc(lmb->drc_index);
+		return rc;
+	}
+
 	rc = dlpar_add_lmb_memory(lmb);
-	if (rc)
+	if (rc) {
+		dlpar_remove_device_tree_lmb(lmb);
 		dlpar_release_drc(lmb->drc_index);
+	}
 
 	return rc;
 }
@@ -542,31 +701,6 @@ static int dlpar_memory_add_by_index(u32 drc_index, struct property *prop)
 	return rc;
 }
 
-static void dlpar_update_drconf_property(struct device_node *dn,
-					 struct property *prop)
-{
-	struct of_drconf_cell *lmbs;
-	u32 num_lmbs, *p;
-	int i;
-
-	/* Convert the property back to BE */
-	p = prop->value;
-	num_lmbs = *p;
-	*p = cpu_to_be32(*p);
-	p++;
-
-	lmbs = (struct of_drconf_cell *)p;
-	for (i = 0; i < num_lmbs; i++) {
-		lmbs[i].base_addr = cpu_to_be64(lmbs[i].base_addr);
-		lmbs[i].drc_index = cpu_to_be32(lmbs[i].drc_index);
-		lmbs[i].flags = cpu_to_be32(lmbs[i].flags);
-	}
-
-	rtas_hp_event = true;
-	of_update_property(dn, prop);
-	rtas_hp_event = false;
-}
-
 int dlpar_memory(struct pseries_hp_errorlog *hp_elog)
 {
 	struct device_node *dn;
@@ -614,10 +748,7 @@ int dlpar_memory(struct pseries_hp_errorlog *hp_elog)
 		break;
 	}
 
-	if (rc)
-		dlpar_free_drconf_property(prop);
-	else
-		dlpar_update_drconf_property(dn, prop);
+	dlpar_free_drconf_property(prop);
 
 dlpar_memory_out:
 	of_node_put(dn);

Re: [2/3] powerpc/pseries: Update LMB associativity index during DLPAR add/remove

[Michael Ellerman]

On Wed, 2016-10-02 at 17:12:13 UTC, Nathan Fontenot wrote:
> The associativity array index specified for a LMB in the device tree,
> /ibm,dynamic-reconfiguration-memory/ibm,dynamic-memory, needs to be updated
> prior to DLPAR adding a LMB and after DLPAR removing a LMB.
> 
> Without doing this step in the DLPAR add process a LMB could be configured
> with the incorrect affinity. For a LMB that was not present at boot the
> affinity index is set to 0xffffffff, which defaults to adding the LMB to
> the first online node since the index is not a valid value. Or, the
> affinity index could contain a stale value if the LMB was present at boot
> but later DLPAR removed and is being DLPAR added back to the system.
> 
> This patch adds a step in the DLPAR add flow to look up the associativity
> index for a LMB prior to adding a LMB and setting the associativity to
> 0xffffffff when a LMB is removed.
> 
> This patch also modifies the DLPAR add/remove flow to no longer do a single
> update of the device tree property after all of the requested DLPAR
> operations are complete and now does a property update during the add
> or remove of each LMB.
> 
> Signed-off-by: Nathan Fontenot <nfont@linux.vnet.ibm.com>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/bdf5fc6338047cfeba98d2ab62

cheers

[PATCH 3/3] powerpc/pseries: Cleanup property cloning in memory dlpar

[Nathan Fontenot]

Now that the DLPAR add/remove flow updates the ibm,dynamic-memory device
tree property each time we add or remove a LMB the work needed to clone
this property can be reduced.

Prior to performing any memory DLPAR operation we now clone the device
tree property once and convert it to cpu format. This copy is then used
to walk through LMBs as we process them and is thrown away when we
are finished. There is no longer a need to convert the entire property to
cpu format and then back to BE every time we update it, we can just parse
it in its native BE format and update the one LMB we need to modify
before updating the property.

This patch removes the BE => cpu conversion step in the clone routine and
creates a drconf_property_to_cpu() routine to make this conversion for the
one time we need to convert the entire property. This then allows us
to remove dlpar_update_drconf_property() since we can now do everything
in dlpar_update_device_tree_lmb().

Signed-off-by: Nathan Fontenot <nfont@linux.vnet.ibm.com>
---
 arch/powerpc/platforms/pseries/hotplug-memory.c |   79 +++++++++--------------
 1 file changed, 32 insertions(+), 47 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
index 2ce1385..4e0cde9 100644
--- a/arch/powerpc/platforms/pseries/hotplug-memory.c
+++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
@@ -79,9 +79,6 @@ static void dlpar_free_drconf_property(struct property *prop)
 static struct property *dlpar_clone_drconf_property(struct device_node *dn)
 {
 	struct property *prop, *new_prop;
-	struct of_drconf_cell *lmbs;
-	u32 num_lmbs, *p;
-	int i;
 
 	prop = of_find_property(dn, "ibm,dynamic-memory", NULL);
 	if (!prop)
@@ -99,48 +96,9 @@ static struct property *dlpar_clone_drconf_property(struct device_node *dn)
 	}
 
 	new_prop->length = prop->length;
-
-	/* Convert the property to cpu endian-ness */
-	p = new_prop->value;
-	*p = be32_to_cpu(*p);
-
-	num_lmbs = *p++;
-	lmbs = (struct of_drconf_cell *)p;
-
-	for (i = 0; i < num_lmbs; i++) {
-		lmbs[i].base_addr = be64_to_cpu(lmbs[i].base_addr);
-		lmbs[i].drc_index = be32_to_cpu(lmbs[i].drc_index);
-		lmbs[i].flags = be32_to_cpu(lmbs[i].flags);
-	}
-
 	return new_prop;
 }
 
-static void dlpar_update_drconf_property(struct device_node *dn,
-					 struct property *prop)
-{
-	struct of_drconf_cell *lmbs;
-	u32 num_lmbs, *p;
-	int i;
-
-	/* Convert the property back to BE */
-	p = prop->value;
-	num_lmbs = *p;
-	*p = cpu_to_be32(*p);
-	p++;
-
-	lmbs = (struct of_drconf_cell *)p;
-	for (i = 0; i < num_lmbs; i++) {
-		lmbs[i].base_addr = cpu_to_be64(lmbs[i].base_addr);
-		lmbs[i].drc_index = cpu_to_be32(lmbs[i].drc_index);
-		lmbs[i].flags = cpu_to_be32(lmbs[i].flags);
-	}
-
-	rtas_hp_event = true;
-	of_update_property(dn, prop);
-	rtas_hp_event = false;
-}
-
 static int dlpar_update_device_tree_lmb(struct of_drconf_cell *lmb)
 {
 	struct device_node *dn;
@@ -160,19 +118,24 @@ static int dlpar_update_device_tree_lmb(struct of_drconf_cell *lmb)
 	}
 
 	p = prop->value;
-	num_lmbs = *p++;
+	num_lmbs = be32_to_cpu(*p);
+	p++;
 	lmbs = (struct of_drconf_cell *)p;
 
 	for (i = 0; i < num_lmbs; i++) {
-		if (lmbs[i].drc_index == lmb->drc_index) {
-			lmbs[i].flags = lmb->flags;
-			lmbs[i].aa_index = lmb->aa_index;
+		u32 this_drc_index = be32_to_cpu(lmbs[i].drc_index);
 
-			dlpar_update_drconf_property(dn, prop);
+		if (this_drc_index == lmb->drc_index) {
+			lmbs[i].flags = cpu_to_be32(lmb->flags);
+			lmbs[i].aa_index = cpu_to_be32(lmb->aa_index);
 			break;
 		}
 	}
 
+	rtas_hp_event = true;
+	of_update_property(dn, prop);
+	rtas_hp_event = false;
+
 	of_node_put(dn);
 	return 0;
 }
@@ -701,6 +664,26 @@ static int dlpar_memory_add_by_index(u32 drc_index, struct property *prop)
 	return rc;
 }
 
+static void drconf_property_to_cpu(struct property *prop)
+{
+	struct of_drconf_cell *lmbs;
+	int i, num_lmbs;
+	u32 *p;
+
+	/* Convert the property to cpu endian-ness */
+	p = prop->value;
+	*p = be32_to_cpu(*p);
+
+	num_lmbs = *p++;
+	lmbs = (struct of_drconf_cell *)p;
+
+	for (i = 0; i < num_lmbs; i++) {
+		lmbs[i].base_addr = be64_to_cpu(lmbs[i].base_addr);
+		lmbs[i].drc_index = be32_to_cpu(lmbs[i].drc_index);
+		lmbs[i].flags = be32_to_cpu(lmbs[i].flags);
+	}
+}
+
 int dlpar_memory(struct pseries_hp_errorlog *hp_elog)
 {
 	struct device_node *dn;
@@ -725,6 +708,8 @@ int dlpar_memory(struct pseries_hp_errorlog *hp_elog)
 		goto dlpar_memory_out;
 	}
 
+	drconf_property_to_cpu(prop);
+
 	switch (hp_elog->action) {
 	case PSERIES_HP_ELOG_ACTION_ADD:
 		if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_COUNT)

Re: [3/3] powerpc/pseries: Cleanup property cloning in memory dlpar

[Michael Ellerman]

On Wed, 2016-10-02 at 17:13:29 UTC, Nathan Fontenot wrote:
> Now that the DLPAR add/remove flow updates the ibm,dynamic-memory device
> tree property each time we add or remove a LMB the work needed to clone
> this property can be reduced.
> 
> Prior to performing any memory DLPAR operation we now clone the device
> tree property once and convert it to cpu format. This copy is then used
> to walk through LMBs as we process them and is thrown away when we
> are finished. There is no longer a need to convert the entire property to
> cpu format and then back to BE every time we update it, we can just parse
> it in its native BE format and update the one LMB we need to modify
> before updating the property.
> 
> This patch removes the BE => cpu conversion step in the clone routine and
> creates a drconf_property_to_cpu() routine to make this conversion for the
> one time we need to convert the entire property. This then allows us
> to remove dlpar_update_drconf_property() since we can now do everything
> in dlpar_update_device_tree_lmb().

Hi Nathan,

This sounds like a good cleanup on the face of it.

But even with it applied I still see a boat load of endian errors from sparse
in this file. That worries me, can you please try and fix them.

  arch/powerpc/platforms/pseries/hotplug-memory.c:121:20: warning: cast to restricted __be32
  arch/powerpc/platforms/pseries/hotplug-memory.c:126:38: warning: cast to restricted __be32
  arch/powerpc/platforms/pseries/hotplug-memory.c:129:39: warning: incorrect type in assignment (different base types)
  arch/powerpc/platforms/pseries/hotplug-memory.c:129:39:    expected unsigned int [unsigned] [usertype] flags
  arch/powerpc/platforms/pseries/hotplug-memory.c:129:39:    got restricted __be32 [usertype] <noident>
  arch/powerpc/platforms/pseries/hotplug-memory.c:130:42: warning: incorrect type in assignment (different base types)
  arch/powerpc/platforms/pseries/hotplug-memory.c:130:42:    expected unsigned int [unsigned] [usertype] aa_index
  arch/powerpc/platforms/pseries/hotplug-memory.c:130:42:    got restricted __be32 [usertype] <noident>
  arch/powerpc/platforms/pseries/hotplug-memory.c:188:21: warning: cast to restricted __be32
  arch/powerpc/platforms/pseries/hotplug-memory.c:189:28: warning: cast to restricted __be32
  arch/powerpc/platforms/pseries/hotplug-memory.c:296:16: warning: cast to restricted __be64
  arch/powerpc/platforms/pseries/hotplug-memory.c:615:33: warning: cast to restricted __be32
  arch/powerpc/platforms/pseries/hotplug-memory.c:675:14: warning: cast to restricted __be32
  arch/powerpc/platforms/pseries/hotplug-memory.c:681:37: warning: cast to restricted __be64
  arch/powerpc/platforms/pseries/hotplug-memory.c:682:37: warning: cast to restricted __be32
  arch/powerpc/platforms/pseries/hotplug-memory.c:683:33: warning: cast to restricted __be32
  arch/powerpc/platforms/pseries/hotplug-memory.c:694:15: warning: incorrect type in assignment (different base types)
  arch/powerpc/platforms/pseries/hotplug-memory.c:694:15:    expected unsigned int [unsigned] [usertype] count
  arch/powerpc/platforms/pseries/hotplug-memory.c:694:15:    got restricted __be32 [usertype] drc_count
  arch/powerpc/platforms/pseries/hotplug-memory.c:695:19: warning: incorrect type in assignment (different base types)
  arch/powerpc/platforms/pseries/hotplug-memory.c:695:19:    expected unsigned int [unsigned] [usertype] drc_index
  arch/powerpc/platforms/pseries/hotplug-memory.c:695:19:    got restricted __be32 [usertype] drc_index
  arch/powerpc/platforms/pseries/hotplug-memory.c:766:16: warning: cast to restricted __be64
  arch/powerpc/platforms/pseries/hotplug-memory.c:808:22: warning: cast to restricted __be32
  arch/powerpc/platforms/pseries/hotplug-memory.c:809:24: warning: cast to restricted __be32
  arch/powerpc/platforms/pseries/hotplug-memory.c:811:33: warning: cast to restricted __be64
  arch/powerpc/platforms/pseries/hotplug-memory.c:814:31: warning: cast to restricted __be32
  arch/powerpc/platforms/pseries/hotplug-memory.c:816:30: warning: cast to restricted __be32
  arch/powerpc/platforms/pseries/hotplug-memory.c:818:43: warning: cast to restricted __be64


I think the problem is you're using one type, struct of_drconf_cell, to hold
both BE and LE data, but at different times. That may be OK if you do the
conversions exactly right, but it is highly error prone, and also confuses the
checker, so is best avoided.

So we can either say that of_drconf_cell holds the device tree representation,
in which case it should use __be64/__be32, or we say that it holds the cpu
endian representation, in which case it stays as is.

My preference would be the latter, and we create an accessor which does the
conversion in exactly one place, ie. something like:

void of_property_read_drconf_cell(const struct property *p,
                                 struct of_drconf_cell *result)
{
       struct {
               __be64 base_addr;
               __be32 drc_index;
               __be32 reserved;
               __be32 aa_index;
               __be32 flags;
       } *s = (void *)p->value;

       result->base_addr = be64_to_cpu(s->base_addr);
       result->drc_index = be32_to_cpu(s->base_addr);
       result->reserved = be32_to_cpu(s->reserved);
       result->aa_index = be32_to_cpu(s->aa_index);
       result->flags = be32_to_cpu(s->flags);
}

cheers

Re: [3/3] powerpc/pseries: Cleanup property cloning in memory dlpar

[Michael Ellerman]

On Wed, 2016-03-02 at 10:02 +1100, Michael Ellerman wrote:
> On Wed, 2016-10-02 at 17:13:29 UTC, Nathan Fontenot wrote:
> > Now that the DLPAR add/remove flow updates the ibm,dynamic-memory device
> > tree property each time we add or remove a LMB the work needed to clone
> > this property can be reduced.
> > 
> > Prior to performing any memory DLPAR operation we now clone the device
> > tree property once and convert it to cpu format. This copy is then used
> > to walk through LMBs as we process them and is thrown away when we
> > are finished. There is no longer a need to convert the entire property to
> > cpu format and then back to BE every time we update it, we can just parse
> > it in its native BE format and update the one LMB we need to modify
> > before updating the property.
> > 
> > This patch removes the BE => cpu conversion step in the clone routine and
> > creates a drconf_property_to_cpu() routine to make this conversion for the
> > one time we need to convert the entire property. This then allows us
> > to remove dlpar_update_drconf_property() since we can now do everything
> > in dlpar_update_device_tree_lmb().
> 
> Hi Nathan,
> 
> This sounds like a good cleanup on the face of it.
> 
> But even with it applied I still see a boat load of endian errors from sparse
> in this file. That worries me, can you please try and fix them.

I can merge patches 1 and 2 if you like, and leave this one for you to fixup?
Or I can just wait for a v2 of the whole series. Let me know which you'd
prefer.

cheers

Re: [3/3] powerpc/pseries: Cleanup property cloning in memory dlpar

[Nathan Fontenot]

On 03/01/2016 07:47 PM, Michael Ellerman wrote:
> On Wed, 2016-03-02 at 10:02 +1100, Michael Ellerman wrote:
>> On Wed, 2016-10-02 at 17:13:29 UTC, Nathan Fontenot wrote:
>>> Now that the DLPAR add/remove flow updates the ibm,dynamic-memory device
>>> tree property each time we add or remove a LMB the work needed to clone
>>> this property can be reduced.
>>>
>>> Prior to performing any memory DLPAR operation we now clone the device
>>> tree property once and convert it to cpu format. This copy is then used
>>> to walk through LMBs as we process them and is thrown away when we
>>> are finished. There is no longer a need to convert the entire property to
>>> cpu format and then back to BE every time we update it, we can just parse
>>> it in its native BE format and update the one LMB we need to modify
>>> before updating the property.
>>>
>>> This patch removes the BE => cpu conversion step in the clone routine and
>>> creates a drconf_property_to_cpu() routine to make this conversion for the
>>> one time we need to convert the entire property. This then allows us
>>> to remove dlpar_update_drconf_property() since we can now do everything
>>> in dlpar_update_device_tree_lmb().
>>
>> Hi Nathan,
>>
>> This sounds like a good cleanup on the face of it.
>>
>> But even with it applied I still see a boat load of endian errors from sparse
>> in this file. That worries me, can you please try and fix them.
> 
> I can merge patches 1 and 2 if you like, and leave this one for you to fixup?
> Or I can just wait for a v2 of the whole series. Let me know which you'd
> prefer.
> 

If you don't mind merging patches 1 and 2 I will send out a new cleanup patch
to fix the issues with patch 3 of the series.

Thanks,
-Nathan