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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Mon, 19 Aug 2019, Julien Grall wrote:
> On 8/17/19 1:29 AM, Stefano Stabellini wrote:
> > On Fri, 16 Aug 2019, Julien Grall wrote:
> > > Hi,
> > > 
> > > On 16/08/2019 00:36, Stefano Stabellini wrote:
> > > > Add a new parameter to device_tree_for_each_node: node, the node to
> > > > start the search from. Passing 0 triggers the old behavior.
> > > 
> > > Here you say 0 triggers the old behavior but...
> > > 
> > > > 
> > > > Set min_depth to depth of the current node + 1 to avoid scanning
> > > > siblings of the initial node passed as an argument.
> > > > 
> > > > Don't call func() on the parent node passed as an argument. Clarify the
> > > > change in the comment on top of the function.
> > > 
> > > ... here you mention that the first node will be skipped. So the behavior
> > > is
> > > now different and should be explained in the commit message why this is
> > > fine
> > > to skip the root node.
> > 
> > Yes I'll update
> > 
> > 
> > > > 
> > > > Signed-off-by: Stefano Stabellini <stefanos@xxxxxxxxxx>
> > > > ---
> > > > Changes in v6:
> > > > - fix code style
> > > > - don't call 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        | 34 ++++++++++++++++++++--------------
> > > >    xen/include/xen/device_tree.h |  6 +++---
> > > >    3 files changed, 28 insertions(+), 20 deletions(-)
> > > > 
> > > > diff --git a/xen/arch/arm/acpi/boot.c b/xen/arch/arm/acpi/boot.c
> > > > index 9b29769a10..bf9c78b02c 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) )
> > > >            goto disable;
> > > >          /*
> > > > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> > > > index 891b4b66ff..f1614ef7fc 100644
> > > > --- a/xen/arch/arm/bootfdt.c
> > > > +++ b/xen/arch/arm/bootfdt.c
> > > > @@ -75,9 +75,10 @@ static u32 __init device_tree_get_u32(const void
> > > > *fdt,
> > > > int node,
> > > >    }
> > > >      /**
> > > > - * device_tree_for_each_node - iterate over all device tree nodes
> > > > + * device_tree_for_each_node - iterate over all device tree sub-nodes
> > > >     * @fdt: flat device tree.
> > > > - * @func: function to call for each node.
> > > > + * @node: parent node to start the search from
> > > > + * @func: function to call for each sub-node.
> > > >     * @data: data to pass to @func.
> > > >     *
> > > >     * Any nodes nested at DEVICE_TREE_MAX_DEPTH or deeper are ignored.
> > > > @@ -85,20 +86,18 @@ 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);
> > > 
> > > Sorry I didn't spot this in the previous version. As I pointed out on v4
> > > (and
> > > you actually agreed!), you don't need the absolute depth...
> > > 
> > > You only need the relative depth. So this is a waste of processing as you
> > > have
> > > to go through the FDT to calculate the depth.
> > > 
> > > This is not entirely critical so I would be willing to consider a patch on
> > > top
> > > of this series.
> > 
> > I tried when I sent this version of the series, but I couldn't quite do
> > it that way. I wanted to get rid of the depth parameter to
> > device_tree_for_each_node, and we need to know the depth of the first
> > node passed as an argument to compare it with the depth of the next node
> > and figure out if it is at the same level or one level deeper.
> 
> fdt_next_node() will increment/decrement whichever depth you pass in argument.
> 
> So if you pass 0, then any child of the node will be at depth 1. Any node at
> the same level as the parent node will also be depth 0...
> 
> Therefore initializing depth to 0 and checking it is strictly positive (i.e
> depth > 0) is enough for our use case.

That makes a lot of sense and will improve the code. Thanks for the
suggestion.

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