| Message ID | 1377701263-3319-9-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
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? > > 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 )
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. The others solutions was: 1) Add the logic during device tree creation 2) Add a boolean to know if the property is a fake 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 ) > >
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 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). > 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 ) > > > > > >
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 ) >>> >>> >> >> > >
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@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 ) > >>> > >>> > >> > >> > > > > > >
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 )
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. Signed-off-by: Julien Grall <julien.grall@linaro.org> --- xen/common/device_tree.c | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-)