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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2013-09-09 at 10:59 +0100, Julien Grall wrote:
> 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.

Understood.

I think we have plenty of RAM at this point, so the small constant could
actually be quite large.

> >> 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.

I think a bit of ugliness is worth it in this case, since the code we
have inhereited is pretty complex and it would be nice to be able to
resync fixed versions in the future.

> 
> >> 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@xxxxxxxxxx>
> >>>> ---
> >>>>    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 )
> >>>
> >>>
> >>
> >>
> >
> >
> 
> 



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel