Re: [XEN][PATCH v5 06/17] xen/device-tree: Add device_tree_find_node_by_path() to find nodes in device tree

[Vikram Garhwal <vikram.garhwal@xxxxxxx>]

Hi Michal,

On 4/13/23 6:40 AM, Michal Orzel wrote:
> Hi Vikram,
>
> On 11/04/2023 21:16, Vikram Garhwal wrote:
> > Add device_tree_find_node_by_path() to find a matching node with path for a
> > dt_device_node.
> >
> > Reason behind this function:
> >      Each time overlay nodes are added using .dtbo, a new fdt(memcpy of
> >      device_tree_flattened) is created and updated with overlay nodes. This
> >      updated fdt is further unflattened to a dt_host_new. Next, we need to find
> >      the overlay nodes in dt_host_new, find the overlay node's parent in dt_host
> >      and add the nodes as child under their parent in the dt_host. Thus we need
> >      this function to search for node in different unflattened device trees.
> You do not mention making dt_find_node_by_path() static inline.
Will add a comment about it in v6.
> > Signed-off-by: Vikram Garhwal <vikram.garhwal@xxxxxxx>
> > ---
> >   xen/common/device_tree.c      |  5 +++--
> >   xen/include/xen/device_tree.h | 17 +++++++++++++++--
> >   2 files changed, 18 insertions(+), 4 deletions(-)
> >
> > diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> > index bf847b2584..507b4ac5b6 100644
> > --- a/xen/common/device_tree.c
> > +++ b/xen/common/device_tree.c
> > @@ -358,11 +358,12 @@ struct dt_device_node *dt_find_node_by_type(struct 
> > dt_device_node *from,
> >       return np;
> >   }
> >
> > -struct dt_device_node *dt_find_node_by_path(const char *path)
> > +struct dt_device_node *device_tree_find_node_by_path(struct dt_device_node *dt,
> > +                                                     const char *path)
> >   {
> >       struct dt_device_node *np;
> >
> > -    dt_for_each_device_node(dt_host, np)
> > +    dt_for_each_device_node(dt, np)
> >           if ( np->full_name && (dt_node_cmp(np->full_name, path) == 0) )
> >               break;
> >
> > diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> > index 58ac12abe3..998f972ebc 100644
> > --- a/xen/include/xen/device_tree.h
> > +++ b/xen/include/xen/device_tree.h
> > @@ -534,13 +534,26 @@ struct dt_device_node *dt_find_node_by_type(struct 
> > dt_device_node *from,
> >   struct dt_device_node *dt_find_node_by_alias(const char *alias);
> >
> >   /**
> > - * dt_find_node_by_path - Find a node matching a full DT path
> > + * device_tree_find_node_by_path - Generic function to find a node matching the
> > + * full DT path for any given unflatten device tree
> > + * @dt_node: The device tree to search
> This should be @dt to match the parameter. Also, shouldn't the description say:
> "the node to start searching from"
> or
> "device tree root node"
>
> FWICS, you expect to pass a root node as dt node. However, in 
> device_tree_find_node_by_path()
> you do not check if a provided node is a root node or not (e.g. no parent). Is 
> this intended?
Yeah, intent was to write a generic function where we can search from 
middle of a device_tree as long as we have the start node from where to 
search.
But yeah so far for dynamic programming it's been called for a root 
nodes only.
> ~Michal