| Message ID | 56BB6FB9.2020701@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
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
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
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
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)
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(-)