From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from e7.ny.us.ibm.com (e7.ny.us.ibm.com [32.97.182.137]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 45F551A01DA for ; Tue, 3 Mar 2015 08:49:46 +1100 (AEDT) Received: from /spool/local by e7.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 2 Mar 2015 16:49:43 -0500 Received: from b01cxnp23034.gho.pok.ibm.com (b01cxnp23034.gho.pok.ibm.com [9.57.198.29]) by d01dlp02.pok.ibm.com (Postfix) with ESMTP id 8F9DC6E8048 for ; Mon, 2 Mar 2015 16:41:31 -0500 (EST) Received: from d01av02.pok.ibm.com (d01av02.pok.ibm.com [9.56.224.216]) by b01cxnp23034.gho.pok.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id t22Lne9Y28180510 for ; Mon, 2 Mar 2015 21:49:40 GMT Received: from d01av02.pok.ibm.com (localhost [127.0.0.1]) by d01av02.pok.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id t22Lncba011539 for ; Mon, 2 Mar 2015 16:49:39 -0500 Message-ID: <54F4DAF1.9050904@linux.vnet.ibm.com> Date: Mon, 02 Mar 2015 13:49:37 -0800 From: Tyrel Datwyler MIME-Version: 1.0 To: Cyril Bur Subject: Re: [PATCH 2/3] powerpc/pseries: Little endian fixes for post mobility device tree update References: <1425090283-27694-1-git-send-email-tyreld@linux.vnet.ibm.com> <1425090283-27694-3-git-send-email-tyreld@linux.vnet.ibm.com> <1425273624.2919.47.camel@cyril> In-Reply-To: <1425273624.2919.47.camel@cyril> Content-Type: text/plain; charset=utf-8 Cc: linuxppc-dev@lists.ozlabs.org, nfont@linux.vnet.ibm.com List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , 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 >> --- >> 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. 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. > >