On Tue, 2015-03-03 at 15:15 -0800, Tyrel Datwyler wrote: > On 03/02/2015 01:49 PM, Tyrel Datwyler wrote: > > On 03/01/2015 09:20 PM, Cyril Bur wrote: > >> On Fri, 2015-02-27 at 18:24 -0800, Tyrel Datwyler wrote: > >>> We currently use the device tree update code in the kernel after resuming > >>> from a suspend operation to re-sync the kernels view of the device tree > >>> with > >>> that of the hypervisor. The code as it stands is not endian safe as it > >>> relies > >>> on parsing buffers returned by RTAS calls that thusly contains data in big > >>> endian format. > >>> > >>> This patch annotates variables and structure members with __be types as > >>> well > >>> as performing necessary byte swaps to cpu endian for data that needs to be > >>> parsed. > >>> > >>> Signed-off-by: Tyrel Datwyler <tyr...@linux.vnet.ibm.com> > >>> --- > >>> arch/powerpc/platforms/pseries/mobility.c | 36 > >>> ++++++++++++++++--------------- > >>> 1 file changed, 19 insertions(+), 17 deletions(-) > >>> > >>> diff --git a/arch/powerpc/platforms/pseries/mobility.c > >>> b/arch/powerpc/platforms/pseries/mobility.c > >>> index 29e4f04..0b1f70e 100644 > >>> --- a/arch/powerpc/platforms/pseries/mobility.c > >>> +++ b/arch/powerpc/platforms/pseries/mobility.c > >>> @@ -25,10 +25,10 @@ > >>> static struct kobject *mobility_kobj; > >>> > >>> struct update_props_workarea { > >>> - u32 phandle; > >>> - u32 state; > >>> - u64 reserved; > >>> - u32 nprops; > >>> + __be32 phandle; > >>> + __be32 state; > >>> + __be64 reserved; > >>> + __be32 nprops; > >>> } __packed; > >>> > >>> #define NODE_ACTION_MASK 0xff000000 > >>> @@ -127,7 +127,7 @@ static int update_dt_property(struct device_node *dn, > >>> struct property **prop, > >>> return 0; > >>> } > >>> > >>> -static int update_dt_node(u32 phandle, s32 scope) > >>> +static int update_dt_node(__be32 phandle, s32 scope) > >>> { > >> > >> On line 153 of this function: > >> dn = of_find_node_by_phandle(phandle); > >> > >> You're passing a __be32 to device tree code, if we can treat the phandle > >> as a opaque value returned to us from the rtas call and pass it around > >> like that then all good. > > After digging deeper the device_node->phandle is stored in cpu endian > under the covers. So, for the of_find_node_by_phandle() we do need to > convert the phandle to cpu endian first. It appears I got lucky with the > update fixing the observed RMC issue because the phandle for the root > node seems to always be 0xffffffff. > I think we've both switched opinions here, initially I thought an endian conversion was necessary but turns out that all of_find_node_by_phandle really does is: for_each_of_allnodes(np) if (np->phandle == handle) break; of_node_get(np);
The == is safe either way and I think the of code might be trying to imply that it doesn't matter by having a typedefed type 'phandle'. I'm still digging around, we want to get this right! Cyril > -Tyrel > > > > > Yes, of_find_node_by_phandle directly compares phandle passed in against > > the handle stored in each device_node when searching for a matching > > node. Since, the device tree is big endian it follows that the big > > endian phandle received in the rtas buffer needs no conversion. > > > > Further, we need to pass the phandle to ibm,update-properties in the > > work area which is also required to be big endian. So, again it seemed > > that converting to cpu endian was a waste of effort just to convert it > > back to big endian. > > > >> Its also hard to be sure if these need to be BE and have always been > >> that way because we've always run BE so they've never actually wanted > >> CPU endian its just that CPU endian has always been BE (I think I > >> started rambling...) > >> > >> Just want to check that *not* converting them is done on purpose. > > > > Yes, I explicitly did not convert them on purpose. As mentioned above we > > need phandle in BE for the ibm,update-properties rtas work area. > > Similarly, drc_index needs to be in BE for the ibm,configure-connector > > rtas work area. Outside, of that we do no other manipulation of those > > values. > > > >> > >> And having read on, I'm assuming the answer is yes since this > >> observation is true for your changes which affect: > >> delete_dt_node() > >> update_dt_node() > >> add_dt_node() > >> Worth noting that you didn't change the definition of delete_dt_node() > > > > You are correct. Oversight. I will fix that as it should generate a > > sparse complaint. > > > > -Tyrel > > > >> > >> I'll have a look once you address the non compiling in patch 1/3 (I'm > >> getting blocked the unused var because somehow Werror is on, odd it > >> didn't trip you up) but I also suspect this will have sparse go a bit > >> nuts. > >> I wonder if there is a nice way of shutting sparse up. > >> > >>> struct update_props_workarea *upwa; > >>> struct device_node *dn; > >>> @@ -136,6 +136,7 @@ static int update_dt_node(u32 phandle, s32 scope) > >>> char *prop_data; > >>> char *rtas_buf; > >>> int update_properties_token; > >>> + u32 nprops; > >>> u32 vd; > >>> > >>> update_properties_token = rtas_token("ibm,update-properties"); > >>> @@ -162,6 +163,7 @@ static int update_dt_node(u32 phandle, s32 scope) > >>> break; > >>> > >>> prop_data = rtas_buf + sizeof(*upwa); > >>> + nprops = be32_to_cpu(upwa->nprops); > >>> > >>> /* On the first call to ibm,update-properties for a node the > >>> * the first property value descriptor contains an empty > >>> @@ -170,17 +172,17 @@ static int update_dt_node(u32 phandle, s32 scope) > >>> */ > >>> if (*prop_data == 0) { > >>> prop_data++; > >>> - vd = *(u32 *)prop_data; > >>> + vd = be32_to_cpu(*(__be32 *)prop_data); > >>> prop_data += vd + sizeof(vd); > >>> - upwa->nprops--; > >>> + nprops--; > >>> } > >>> > >>> - for (i = 0; i < upwa->nprops; i++) { > >>> + for (i = 0; i < nprops; i++) { > >>> char *prop_name; > >>> > >>> prop_name = prop_data; > >>> prop_data += strlen(prop_name) + 1; > >>> - vd = *(u32 *)prop_data; > >>> + vd = be32_to_cpu(*(__be32 *)prop_data); > >>> prop_data += sizeof(vd); > >>> > >>> switch (vd) { > >>> @@ -212,7 +214,7 @@ static int update_dt_node(u32 phandle, s32 scope) > >>> return 0; > >>> } > >>> > >>> -static int add_dt_node(u32 parent_phandle, u32 drc_index) > >>> +static int add_dt_node(__be32 parent_phandle, __be32 drc_index) > >>> { > >>> struct device_node *dn; > >>> struct device_node *parent_dn; > >>> @@ -237,7 +239,7 @@ static int add_dt_node(u32 parent_phandle, u32 > >>> drc_index) > >>> int pseries_devicetree_update(s32 scope) > >>> { > >>> char *rtas_buf; > >>> - u32 *data; > >>> + __be32 *data; > >>> int update_nodes_token; > >>> int rc; > >>> > >>> @@ -254,17 +256,17 @@ int pseries_devicetree_update(s32 scope) > >>> if (rc && rc != 1) > >>> break; > >>> > >>> - data = (u32 *)rtas_buf + 4; > >>> - while (*data & NODE_ACTION_MASK) { > >>> + data = (__be32 *)rtas_buf + 4; > >>> + while (be32_to_cpu(*data) & NODE_ACTION_MASK) { > >>> int i; > >>> - u32 action = *data & NODE_ACTION_MASK; > >>> - int node_count = *data & NODE_COUNT_MASK; > >>> + u32 action = be32_to_cpu(*data) & NODE_ACTION_MASK; > >>> + u32 node_count = be32_to_cpu(*data) & NODE_COUNT_MASK; > >>> > >>> data++; > >>> > >>> for (i = 0; i < node_count; i++) { > >>> - u32 phandle = *data++; > >>> - u32 drc_index; > >>> + __be32 phandle = *data++; > >>> + __be32 drc_index; > >>> > >>> switch (action) { > >>> case DELETE_DT_NODE: > >> > >> The patch looks good, no nonsense endian fixing. > >> Worth noting that it leaves existing bugs in place, which is fine, I'll > >> rebase my patches which address endian and bugs on top of these so as to > >> address the bugs. > >> > >> > > > > _______________________________________________ > > Linuxppc-dev mailing list > > Linuxppc-dev@lists.ozlabs.org > > https://lists.ozlabs.org/listinfo/linuxppc-dev > > > _______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev