[Pdbg] [PATCH v2 09/22] libpdbg: Parent retrieval should handle virtual nodes correctly

[Amitay Isaacs]

On Mon, 2019-09-23 at 13:23 +1000, Alistair Popple wrote:
> On Friday, 20 September 2019 3:16:38 PM AEST Amitay Isaacs wrote:
> > When traversing system device tree view (system == true), parent of
> > a
> > node can be a virtual node.
> > 
> > When traversing backend device tree, parent of a node will not be a
> > virtual node unless it's part of the backend device tree itself.
> > 
> > Signed-off-by: Amitay Isaacs <amitay at ozlabs.org>
> > ---
> >  libpdbg/libpdbg.c | 17 +++++++++++------
> >  libpdbg/libpdbg.h | 10 +++++-----
> >  libpdbg/target.c  | 27 ++++++++++++++++++++++++++-
> >  libpdbg/target.h  |  1 +
> >  src/thread.c      |  4 ++--
> >  5 files changed, 45 insertions(+), 14 deletions(-)
> > 
> > diff --git a/libpdbg/libpdbg.c b/libpdbg/libpdbg.c
> > index 7029647..db0a564 100644
> > --- a/libpdbg/libpdbg.c
> > +++ b/libpdbg/libpdbg.c
> > @@ -6,7 +6,7 @@
> >  
> >  static pdbg_progress_tick_t progress_tick;
> >  
> > -struct pdbg_target *__pdbg_next_target(const char *class, struct 
> pdbg_target *parent, struct pdbg_target *last)
> > +struct pdbg_target *__pdbg_next_target(const char *class, struct 
> pdbg_target *parent, struct pdbg_target *last, bool system)
> >  {
> >  	struct pdbg_target *next, *tmp;
> >  	struct pdbg_target_class *target_class;
> > @@ -33,7 +33,7 @@ retry:
> >  		return next;
> 
> Do we need to add any filtering with something like
> pdbg_next_target_match() 
> here? Or are we ok returning everything for a particular target
> class 
> regardless of which tree it falls under?

We do need a mapping.  But turns out the mapping for child and mapping
for parent is slightly different.  Figured that out after refactoring
it as common code. :-(


> 
> >  	else {
> >  		/* Check if this target is a child of the given parent
> > */
> > -		for (tmp = next; tmp && next->parent && tmp != parent;
> > tmp = tmp-
> > parent) {}
> > +		for (tmp = next; tmp && get_parent(tmp, system) && tmp
> > != parent; tmp 
> = get_parent(tmp, system)) {}
> >  		if (tmp == parent)
> >  			return next;
> >  		else {
> > @@ -116,7 +116,7 @@ uint32_t pdbg_target_index(struct pdbg_target
> > *target)
> >  {
> >  	struct pdbg_target *dn;
> >  
> > -	for (dn = target; dn && dn->index == -1; dn = dn->parent);
> > +	for (dn = target; dn && dn->index == -1; dn = get_parent(dn,
> > true));
> >  
> >  	if (!dn)
> >  		return -1;
> > @@ -130,10 +130,15 @@ struct pdbg_target *pdbg_target_parent(const
> > char 
> *class, struct pdbg_target *ta
> >  	struct pdbg_target *parent;
> >  
> >  	if (!class)
> > -		return target->parent;
> > +		return get_parent(target, true);
> >  
> > -	for (parent = target->parent; parent && parent->parent; parent
> > = parent-
> > parent) {
> > -		if (!strcmp(class, pdbg_target_class_name(parent)))
> > +	for (parent = get_parent(target, true); parent &&
> > get_parent(parent, 
> true); parent = get_parent(parent, true)) {
> > +		const char *tclass = pdbg_target_class_name(parent);
> > +
> > +		if (!tclass)
> > +			continue;
> > +
> > +		if (!strcmp(class, tclass))
> >  			return parent;
> >  	}
> >  
> > diff --git a/libpdbg/libpdbg.h b/libpdbg/libpdbg.h
> > index 2ae29cc..de3fc17 100644
> > --- a/libpdbg/libpdbg.h
> > +++ b/libpdbg/libpdbg.h
> > @@ -19,7 +19,7 @@ struct pdbg_target_class;
> >  struct pdbg_target *__pdbg_next_compatible_node(struct pdbg_target
> > *root,
> >                                                  struct pdbg_target
> > *prev,
> >                                                  const char
> > *compat);
> > -struct pdbg_target *__pdbg_next_target(const char *klass, struct 
> pdbg_target *parent, struct pdbg_target *last);
> > +struct pdbg_target *__pdbg_next_target(const char *klass, struct 
> pdbg_target *parent, struct pdbg_target *last, bool system);
> >  struct pdbg_target *__pdbg_next_child_target(struct pdbg_target
> > *parent, 
> struct pdbg_target *last, bool system);
> >  
> >  /*
> > @@ -61,14 +61,14 @@ enum pdbg_backend { PDBG_DEFAULT_BACKEND = 0, 
> PDBG_BACKEND_FSI, PDBG_BACKEND_I2C
> >               (target = __pdbg_next_compatible_node(parent, target,
> > compat)) 
> != NULL;)
> >  
> >  #define pdbg_for_each_target(class, parent, target)		
> > 	\
> > -	for (target = __pdbg_next_target(class, parent, NULL);		
> > \
> > +	for (target = __pdbg_next_target(class, parent, NULL, true);	
> > \
> >  	     target;							
> > \
> > -	     target = __pdbg_next_target(class, parent, target))
> > +	     target = __pdbg_next_target(class, parent, target, true))
> >  
> >  #define pdbg_for_each_class_target(class, target)		\
> > -	for (target = __pdbg_next_target(class, NULL, NULL);	\
> > +	for (target = __pdbg_next_target(class, NULL, NULL, true);	\
> >  	     target;						\
> > -	     target = __pdbg_next_target(class, NULL, target))
> > +	     target = __pdbg_next_target(class, NULL, target, true))
> >  
> >  #define pdbg_for_each_child_target(parent, target)	      \
> >  	for (target = __pdbg_next_child_target(parent, NULL, true); \
> > diff --git a/libpdbg/target.c b/libpdbg/target.c
> > index da24ba0..667e55d 100644
> > --- a/libpdbg/target.c
> > +++ b/libpdbg/target.c
> > @@ -27,7 +27,7 @@ static struct pdbg_target
> > *get_class_target_addr(struct 
> pdbg_target *target, con
> >  			*addr += pdbg_target_address(target, NULL);
> >  
> >  		/* Keep walking the tree translating addresses */
> > -		target = target->parent;
> > +		target = get_parent(target, false);
> >  
> >  		/* The root node doesn't have an address space so it's
> >  		 * an error in the device tree if we hit this. */
> > @@ -312,6 +312,31 @@ uint32_t sbe_ffdc_get(struct pdbg_target
> > *target, const 
> uint8_t **ffdc, uint32_t
> >  	return sbefifo->ffdc_get(sbefifo, ffdc, ffdc_len);
> >  }
> >  
> > +struct pdbg_target *get_parent(struct pdbg_target *target, bool
> > system)
> > +{
> > +	struct pdbg_target *parent;
> > +
> > +	if (!target)
> > +		return NULL;
> > +
> > +	if (system) {
> > +		if (target->compatible && target->vnode)
> > +			target = target->vnode;
> > +	} else {
> > +		if (!target->compatible && target->vnode)
> > +			target = target->vnode;
> > +	}
> 
> Still not sure I completely follow the reasoning behind this logic so
> it would 
> be good if we could get some comments describing the
> rules/assumptions for 
> each node type.

I will add comments to get_parent() to explain how the mapping is done.
 
> 
> - Alistair
> 
> > +	parent = target->parent;
> > +	if (!parent || !parent->vnode)
> > +		return parent;
> > +
> > +	if (parent->compatible)
> > +		return parent;
> > +
> > +	return parent->vnode;
> > +}
> > +
> >  struct pdbg_target *require_target_parent(struct pdbg_target
> > *target)
> >  {
> >  	assert(target->parent);
> > diff --git a/libpdbg/target.h b/libpdbg/target.h
> > index 7e08ac3..7419ce5 100644
> > --- a/libpdbg/target.h
> > +++ b/libpdbg/target.h
> > @@ -53,6 +53,7 @@ struct pdbg_target {
> >  	struct pdbg_target *vnode;
> >  };
> >  
> > +struct pdbg_target *get_parent(struct pdbg_target *target, bool
> > system);
> >  struct pdbg_target *require_target_parent(struct pdbg_target
> > *target);
> >  struct pdbg_target_class *find_target_class(const char *name);
> >  struct pdbg_target_class *require_target_class(const char *name);
> > diff --git a/src/thread.c b/src/thread.c
> > index 663f290..8ddf4ae 100644
> > --- a/src/thread.c
> > +++ b/src/thread.c
> > @@ -277,10 +277,10 @@ static int thread_status_print(void)
> >  		assert(path_target_add(pib));
> >  	}
> >  
> > -	pib = __pdbg_next_target("pib", pdbg_target_root(), NULL);
> > +	pib = __pdbg_next_target("pib", pdbg_target_root(), NULL,
> > true);
> >  	assert(pib);
> >  
> > -	core = __pdbg_next_target("core", pib, NULL);
> > +	core = __pdbg_next_target("core", pib, NULL, true);
> >  	assert(core);
> >  
> >  	pdbg_for_each_target("thread", core, thread)
> > 
> 
> 
> 

Amitay.
-- 

The man with imagination is never alone.