Re: [PATCH 1/4] dtb: correct handling of #address-cells and #size-cells.

[Ian Campbell <Ian.Campbell@citrix.com>]

On Fri, 2013-02-15 at 12:43 +0000, Stefano Stabellini wrote:
> On Wed, 30 Jan 2013, Ian Campbell wrote:
> > If a node does not have #*-cells then the parent's value should be
> > used. Currently we were asssuming zero which is useless.
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> >  xen/arch/arm/domain_build.c   |    6 ++++--
> >  xen/common/device_tree.c      |   12 ++++++++----
> >  xen/include/xen/device_tree.h |    3 ++-
> >  3 files changed, 14 insertions(+), 7 deletions(-)
> > 
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 7403f1a..bfbe7c7 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -198,8 +198,10 @@ static int write_nodes(struct domain *d, struct kernel_info *kinfo,
> >          while ( last_depth-- >= depth )
> >              fdt_end_node(kinfo->fdt);
> >  
> > -        address_cells[depth] = device_tree_get_u32(fdt, node, "#address-cells");
> > -        size_cells[depth] = device_tree_get_u32(fdt, node, "#size-cells");
> > +        address_cells[depth] = device_tree_get_u32(fdt, node, "#address-cells",
> > +                                    depth > 0 ? address_cells[depth-1] : 0);
> > +        size_cells[depth] = device_tree_get_u32(fdt, node, "#size-cells",
> > +                                    depth > 0 ? size_cells[depth-1] : 0);
> >  
> >          fdt_begin_node(kinfo->fdt, name);
> 
> The depth is always increasing by steps of 1 in this loop, right?
> Because retrieving address-cells and size-cells should be recursive: if
> n-1 doesn't have them, let's look at n-2, etc. Of course if we start from
> depth = 0 and go from there without missing any levels the results will
> be the same.

That was what I thought too. Perhaps it is too subtle?

I bet my "xen: strip xen,multiboot-module nodes from dom0 device tree"
patch changes this invariant. Better to make it explicitly walk
backwards now I think. (or maybe set things for level in
last_depth..depth). I'll change things along these lines.

> I think I convinced myself that this is correct.
> 
> 
> > diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> > index 260c2d4..f10ba1b 100644
> > --- a/xen/common/device_tree.c
> > +++ b/xen/common/device_tree.c
> > @@ -120,13 +120,14 @@ void device_tree_set_reg(u32 **cell, u32 address_cells, u32 size_cells,
> >      set_val(cell, size_cells, size);
> >  }
> >  
> > -u32 device_tree_get_u32(const void *fdt, int node, const char *prop_name)
> > +u32 device_tree_get_u32(const void *fdt, int node, const char *prop_name,
> > +                        u32 dflt)
> >  {
> >      const struct fdt_property *prop;
> >  
> >      prop = fdt_get_property(fdt, node, prop_name, NULL);
> >      if ( !prop || prop->len < sizeof(u32) )
> > -        return 0; /* default to 0 */
> > +        return dflt;
> >  
> >      return fdt32_to_cpu(*(uint32_t*)prop->data);
> >  }
> 
> where did the vowels go? :)

Not sure. Unlike me ;-)

Ian.