Re: [Xen-devel] [PATCH v5 1/7] xen/arm: pass node to device_tree_for_each_node

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 13 Aug 2019, Volodymyr Babchuk wrote:
> Hi Stefano,
> 
> Stefano Stabellini writes:
> 
> > Add a new parameter to device_tree_for_each_node: node, the node to
> > start the search from. Passing 0 triggers the old behavior.
> >
> > Set min_depth to depth of the current node + 1 and replace the for
> > loop with a do/while loop to avoid scanning siblings of the initial node
> > passed as an argument.
> >
> > We need this change because in follow-up patches we want to be able to
> > use reuse device_tree_for_each_node to call a function for each children
> > nodes of a provided node and the node itself.
> >
> > Signed-off-by: Stefano Stabellini <stefanos@xxxxxxxxxx>
> 
> You can have my
> 
> Reviewed-by: Volodymyr Babchuk <volodymyr.babchuk@xxxxxxxx>
> 
> providing that you'll fix formatting issue below.

Thank you! I'll fix the formatting issue, but won't add reviewed-by for
now as I'll change the function a bit to skip calling func() on the
first node.


> > ---
> > Changes in v5:
> > - go back to v3
> > - code style improvement in acpi/boot.c
> > - improve comments and commit message
> > - increase min_depth to avoid parsing siblings
> > - replace for with do/while loop and increase min_depth to avoid
> >   scanning siblings of the initial node
> > - pass only node, calculate depth
> >
> > Changes in v3:
> > - improve commit message
> > - improve in-code comments
> > - improve code style
> >
> > Changes in v2:
> > - new
> > ---
> >  xen/arch/arm/acpi/boot.c      |  8 +++++---
> >  xen/arch/arm/bootfdt.c        | 19 ++++++++++---------
> >  xen/include/xen/device_tree.h |  6 +++---
> >  3 files changed, 18 insertions(+), 15 deletions(-)
> >
> > diff --git a/xen/arch/arm/acpi/boot.c b/xen/arch/arm/acpi/boot.c
> > index 9b29769a10..d4957cca06 100644
> > --- a/xen/arch/arm/acpi/boot.c
> > +++ b/xen/arch/arm/acpi/boot.c
> > @@ -246,9 +246,11 @@ int __init acpi_boot_table_init(void)
> >       * - the device tree is not empty (it has more than just a /chosen 
> > node)
> >       *   and ACPI has not been force enabled (acpi=force)
> >       */
> > -    if ( param_acpi_off || ( !param_acpi_force
> > -                             && 
> > device_tree_for_each_node(device_tree_flattened,
> > -                                                   dt_scan_depth1_nodes, 
> > NULL)))
> > +    if ( param_acpi_off)
> > +        goto disable;
> > +   if ( !param_acpi_force &&
> > +            device_tree_for_each_node(device_tree_flattened, 0,
> > +                                          dt_scan_depth1_nodes, NULL) )
> There is 3 tabs, followed by spaces.
> 
> This file missed emacs magic at the end. I think, this is cause for this
> formatting issue.
> 
> >          goto disable;
> >  
> >      /*
> > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> > index 891b4b66ff..a872ea57d6 100644
> > --- a/xen/arch/arm/bootfdt.c
> > +++ b/xen/arch/arm/bootfdt.c
> > @@ -77,6 +77,7 @@ static u32 __init device_tree_get_u32(const void *fdt, 
> > int node,
> >  /**
> >   * device_tree_for_each_node - iterate over all device tree nodes
> >   * @fdt: flat device tree.
> > + * @node: node to start the search from
> >   * @func: function to call for each node.
> >   * @data: data to pass to @func.
> >   *
> > @@ -85,20 +86,17 @@ static u32 __init device_tree_get_u32(const void *fdt, 
> > int node,
> >   * Returns 0 if all nodes were iterated over successfully.  If @func
> >   * returns a value different from 0, that value is returned immediately.
> >   */
> > -int __init device_tree_for_each_node(const void *fdt,
> > +int __init device_tree_for_each_node(const void *fdt, int node,
> >                                       device_tree_node_func func,
> >                                       void *data)
> >  {
> > -    int node;
> > -    int depth;
> > +    int depth = fdt_node_depth(fdt, node);
> > +    int min_depth = depth + 1;
> >      u32 address_cells[DEVICE_TREE_MAX_DEPTH];
> >      u32 size_cells[DEVICE_TREE_MAX_DEPTH];
> >      int ret;
> >  
> > -    for ( node = 0, depth = 0;
> > -          node >=0 && depth >= 0;
> > -          node = fdt_next_node(fdt, node, &depth) )
> > -    {
> > +    do {
> >          const char *name = fdt_get_name(fdt, node, NULL);
> >          u32 as, ss;
> >  
> > @@ -120,7 +118,10 @@ int __init device_tree_for_each_node(const void *fdt,
> >          ret = func(fdt, node, name, depth, as, ss, data);
> >          if ( ret != 0 )
> >              return ret;
> > -    }
> > +
> > +        node = fdt_next_node(fdt, node, &depth);
> > +    } while ( node >= 0 && depth >= min_depth );
> > +
> >      return 0;
> >  }
> >  
> > @@ -357,7 +358,7 @@ size_t __init boot_fdt_info(const void *fdt, paddr_t 
> > paddr)
> >  
> >      add_boot_module(BOOTMOD_FDT, paddr, fdt_totalsize(fdt), false);
> >  
> > -    device_tree_for_each_node((void *)fdt, early_scan_node, NULL);
> > +    device_tree_for_each_node((void *)fdt, 0, early_scan_node, NULL);
> >      early_print_info();
> >  
> >      return fdt_totalsize(fdt);
> > diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> > index 83156297e2..9a7a8f2dab 100644
> > --- a/xen/include/xen/device_tree.h
> > +++ b/xen/include/xen/device_tree.h
> > @@ -158,9 +158,9 @@ typedef int (*device_tree_node_func)(const void *fdt,
> >  
> >  extern const void *device_tree_flattened;
> >  
> > -int device_tree_for_each_node(const void *fdt,
> > -                                     device_tree_node_func func,
> > -                                     void *data);
> > +int device_tree_for_each_node(const void *fdt, int node,
> > +                              device_tree_node_func func,
> > +                              void *data);
> >  
> >  /**
> >   * dt_unflatten_host_device_tree - Unflatten the host device tree
> 
> 
> -- 
> Volodymyr Babchuk at EPAM

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel