Re: [XEN][RFC PATCH v4 11/16] common/device_tree: Add rwlock for dt_host

[Julien Grall <julien@xxxxxxx>]

Hi,

On 01/02/2023 17:05, Vikram Garhwal wrote:
> On 12/7/22 8:31 AM, Julien Grall wrote:
> > > Signed-off-by: Vikram Garhwal <vikram.garhwal@xxxxxxx>
> > > ---
> > >   xen/common/device_tree.c      | 27 +++++++++++++++++++++++++++
> > >   xen/include/xen/device_tree.h |  6 ++++++
> > >   2 files changed, 33 insertions(+)
> > >
> > > diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> > > index acf26a411d..51ee2a5edf 100644
> > > --- a/xen/common/device_tree.c
> > > +++ b/xen/common/device_tree.c
> > > @@ -140,6 +140,8 @@ const struct dt_property *dt_find_property(const 
> > > struct dt_device_node *np,
> > >       if ( !np )
> > >           return NULL;
> > >   +    read_lock(&dt_host->lock);
> > > +
> > >       for ( pp = np->properties; pp; pp = pp->next )
> > >       {
> > >           if ( dt_prop_cmp(pp->name, name) == 0 )
> > > @@ -150,6 +152,7 @@ const struct dt_property *dt_find_property(const 
> > > struct dt_device_node *np,
> > >           }
> > >       }
> > >   +    read_unlock(&dt_host->lock);
> > >       return pp;
> > >   }
> > >   @@ -336,11 +339,14 @@ struct dt_device_node 
> > > *dt_find_node_by_name(struct dt_device_node *from,
> > >       struct dt_device_node *np;
> > >       struct dt_device_node *dt;
> > >   +    read_lock(&dt_host->lock);
> > > +
> > >       dt = from ? from->allnext : dt_host;
> > >       dt_for_each_device_node(dt, np)
> > >           if ( np->name && (dt_node_cmp(np->name, name) == 0) )
> > >               break;
> > >   +    read_unlock(&dt_host->lock);
> > >       return np;
> > ... I was expecting the read lock to also protect the value returned 
> > from being freed. But this is not the case.
> Okay. Shall I remove the lock from here and perhaps add it when 
> dt_find_node_by_name() and other related functions are called?
It is a possibility. I would need to see the end result to be sure a 
lock is actually suitable.
Maybe we will end up to need a refcounting instead if we keep the DT 
node for longer.
Cheers,

--
Julien Grall