[PATCH 2/3] powerpc/pseries: Little endian fixes for post mobility device tree update
Cyril Bur
cyrilbur at gmail.com
Wed Mar 4 12:20:07 AEDT 2015
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 <tyreld at 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 at lists.ozlabs.org
> > https://lists.ozlabs.org/listinfo/linuxppc-dev
> >
>
More information about the Linuxppc-dev
mailing list