Re: [PATCH V1 08/29] xen/dts: Don't add a fake property "name" in the device tree

[Julien Grall <julien.grall@linaro.org>]

On 09/09/2013 10:40 AM, Ian Campbell wrote:
> On Mon, 2013-09-09 at 10:30 +0100, Julien Grall wrote:
>> On 09/06/2013 05:28 PM, Ian Campbell wrote:
>>> On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote:
>>>> On new Flat Device Tree version, the property "name" may not exist.
>>>> The property is never used in Xen code except to set the field "name" of
>>>> dt_device_node.
>>>>
>>>> For convenience, remove the fake property. It will save space during the
>>>> creation of the dom0 FDT.
>>>
>>> Is it worth diverging from the Linux code this is based on over this
>>> though?
>>
>> If we don't diverge, I need to add some logic in device tree creation to
>> know if the "name" property was generated by Xen.
>
> Because otherwise it gets included in the DTB given to dom0? Is that
> harmful though?

The current device tree creation code is not able to reallocate memory 
if the device tree is bigger than the real one plus a small constant. I 
don't think it's easy to handle it because we don't have realloc 
function in Xen.

>
>>
>> The others solutions was:
>>      1) Add the logic during device tree creation
>>      2) Add a boolean to know if the property is a fake
>
> Would it be possible to reduce the divergence, at the cost of a little
> bit of dead code by not actually removing the code which makes up the
> name, but rather short circuiting it with a "continue" or an "if (0
> &&  ...)" (plus an appropriate comment).

I found this solution a bit ugly, but if we can avoid a divergence and 
some logic in device creation... I will rework the patch with this solution.

>> Currently, the "name" property value is replicated in the name field of
>> dt_device_node. Except staying close to Linux code, I don't see a good
>> reason to create this fake property.
>>
>>>>
>>>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>>> ---
>>>>    xen/common/device_tree.c |   21 +++++++++------------
>>>>    1 file changed, 9 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
>>>> index be592d2..07a19ac 100644
>>>> --- a/xen/common/device_tree.c
>>>> +++ b/xen/common/device_tree.c
>>>> @@ -1543,6 +1543,7 @@ static unsigned long __init unflatten_dt_node(const void *fdt,
>>>>        if ( !has_name )
>>>>        {
>>>>            char *p1 = pathp, *ps = pathp, *pa = NULL;
>>>> +        char *tmp = NULL;
>>>>            int sz;
>>>>
>>>>            while ( *p1 )
>>>> @@ -1556,25 +1557,21 @@ static unsigned long __init unflatten_dt_node(const void *fdt,
>>>>            if ( pa < ps )
>>>>                pa = p1;
>>>>            sz = (pa - ps) + 1;
>>>> -        pp = unflatten_dt_alloc(&mem, sizeof(struct dt_property) + sz,
>>>> -                                __alignof__(struct dt_property));
>>>> +
>>>> +        tmp = unflatten_dt_alloc(&mem, sz, 1);
>>>>            if ( allnextpp )
>>>>            {
>>>> -            pp->name = "name";
>>>> -            pp->length = sz;
>>>> -            pp->value = pp + 1;
>>>> -            *prev_pp = pp;
>>>> -            prev_pp = &pp->next;
>>>> -            memcpy(pp->value, ps, sz - 1);
>>>> -            ((char *)pp->value)[sz - 1] = 0;
>>>> -            dt_dprintk("fixed up name for %s -> %s\n", pathp,
>>>> -                       (char *)pp->value);
>>>> +            memcpy(tmp, ps, sz - 1);
>>>> +            np->name = tmp;
>>>> +            tmp[sz - 1] = 0;
>>>> +            dt_dprintk("fixed up name for %s -> %s\n", pathp, np->name);
>>>>            }
>>>>        }
>>>> +
>>>>        if ( allnextpp )
>>>>        {
>>>>            *prev_pp = NULL;
>>>> -        np->name = dt_get_property(np, "name", NULL);
>>>> +        np->name = (np->name) ? : dt_get_property(np, "name", NULL);
>>>>            np->type = dt_get_property(np, "device_type", NULL);
>>>>
>>>>            if ( !np->name )
>>>
>>>
>>
>>
>
>


-- 
Julien Grall