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
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
