On 03/03/2015 05:20 PM, Cyril Bur wrote:
> 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!
When the device tree is unflattened the phandle is byte swapped to cpu
endian. The following code is from unflatten_dt_node().
if (strcmp(pname, "ibm,phandle") == 0)
np->phandle = be32_to_cpup(p);
I added some debug to the of_find_node_by_phandle() and verified if the
phandle isn't swapped to cpu endian we fail to find a matching node
except in the case where the phandle is equivalent in both big and
little endian.
-Tyrel
>
>
> 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
