[Pdbg] [PATCH v2 08/22] libpdbg: Child traversal should handle virtual nodes correctly

Amitay Isaacs amitay at ozlabs.org
Mon Sep 23 16:37:58 AEST 2019


On Mon, 2019-09-23 at 12:26 +1000, Alistair Popple wrote:
> On Friday, 20 September 2019 3:16:37 PM AEST Amitay Isaacs wrote:
> > When traversing system device tree view (system == true), the
> > children
> > are calculated based on the system device tree view (using the
> > virtual
> > node attachments).
> > 
> > When traversing backend device tree (system == false), the virtual
> > nodes
> > are ignored except if they are part of the backend device tree
> > itself.
> > 
> > Signed-off-by: Amitay Isaacs <amitay at ozlabs.org>
> > ---
> >  libpdbg/libpdbg.c | 52 +++++++++++++++++++++++++++++++++++++++++
> > ------
> >  libpdbg/libpdbg.h |  6 +++---
> >  2 files changed, 49 insertions(+), 9 deletions(-)
> > 
> > diff --git a/libpdbg/libpdbg.c b/libpdbg/libpdbg.c
> > index aade5d3..7029647 100644
> > --- a/libpdbg/libpdbg.c
> > +++ b/libpdbg/libpdbg.c
> > @@ -43,18 +43,58 @@ retry:
> >  	}
> >  }
> >  
> > -struct pdbg_target *__pdbg_next_child_target(struct pdbg_target
> > *parent, 
> struct pdbg_target *last)
> > +static struct pdbg_target *pdbg_next_target_match(struct
> > pdbg_target *next, 
> bool system)
> >  {
> > -	if (!parent || list_empty(&parent->children))
> > +	if (system) {
> > +		if (!next->vnode)
> > +			return next;
> > +		else
> > +			if (!next->compatible)
> > +				return next->vnode;
> > +	} else {
> > +		if (next->compatible)
> > +			return next;
> > +	}
> > +
> > +	return NULL;
> > +}
> > +
> > +struct pdbg_target *__pdbg_next_child_target(struct pdbg_target
> > *parent, 
> struct pdbg_target *last, bool system)
> > +{
> > +	struct pdbg_target *next, *child = NULL;
> > +
> > +	if (!parent)
> >  		return NULL;
> >  
> > -	if (!last)
> > -		return list_top(&parent->children, struct pdbg_target,
> > list);
> > +	if (list_empty(&parent->children)) {
> 
> So this implies that a node cannot have any children if it has an
> associated 
> vnode. I assume that is always the case? If so it would make things
> clearer if 
> the logic was simplified by just adding a check for parent->vnode at
> the start 
> like so:
> 
> if (parent->vnode)
> 	parent = parent->vnode;
> 

Turns out that's not the case.  If the given parent has children, then
we don't want to map that parent to associated node.  Only if the
parent is the leaf node, then check the associated node.

> > +		if (parent->vnode)
> > +			parent = parent->vnode;
> > +		else
> > +			return NULL;
> > +	}
> > +
> > +	if (!last) {
> > +		list_for_each(&parent->children, child, list)
> > +			if ((next = pdbg_next_target_match(child,
> > system)))
> > +				return next;
> > +
> > +		return NULL;
> > +	}
> > +
> > +	if (system && last->vnode && last->compatible)
> > +		last = last->vnode;
> 
> I think I've missed something here. Why do we only follow the vnode
> path if 
> there is a compatible property? Especially given the filtering
> function 
> (pdbg_next_target_match) only returns the vnode if there is no
> compatible 
> property.
> 
> We need some comments here describing the rules and relationship
> between the 
> system/node/vnode/compatible tests.

I agree. It took me some time to figure out why the above line was
required.  This code was tricky to get right without the tests.

Lots of comments to follow.

> 
> > -	if (last->list.next == &parent->children.n)
> > +	if (last == list_tail(&parent->children, struct pdbg_target,
> > list))
> >  		return NULL;
> >  
> > -	return list_entry(last->list.next, struct pdbg_target, list);
> > +	child = last;
> > +	do {
> > +		child = list_entry(child->list.next, struct
> > pdbg_target, list);
> > +		if ((next = pdbg_next_target_match(child, system)))
> > +			return next;
> > +	} while (child != list_tail(&parent->children, struct
> > pdbg_target, list));
> > +
> > +	return NULL;
> >  }
> >  
> >  enum pdbg_target_status pdbg_target_status(struct pdbg_target
> > *target)
> > diff --git a/libpdbg/libpdbg.h b/libpdbg/libpdbg.h
> > index 2bd3aa0..2ae29cc 100644
> > --- a/libpdbg/libpdbg.h
> > +++ b/libpdbg/libpdbg.h
> > @@ -20,7 +20,7 @@ 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_child_target(struct pdbg_target
> > *parent, 
> struct pdbg_target *last);
> > +struct pdbg_target *__pdbg_next_child_target(struct pdbg_target
> > *parent, 
> struct pdbg_target *last, bool system);
> >  
> >  /*
> >   * Each target has a status associated with it. This is what each
> > status 
> means:
> > @@ -71,9 +71,9 @@ enum pdbg_backend { PDBG_DEFAULT_BACKEND = 0, 
> PDBG_BACKEND_FSI, PDBG_BACKEND_I2C
> >  	     target = __pdbg_next_target(class, NULL, target))
> >  
> >  #define pdbg_for_each_child_target(parent, target)	      \
> > -	for (target = __pdbg_next_child_target(parent, NULL); \
> > +	for (target = __pdbg_next_child_target(parent, NULL, true); \
> >  	     target;					      \
> > -	     target = __pdbg_next_child_target(parent, target))
> > +	     target = __pdbg_next_child_target(parent, target, true))
> >  
> >  /* Return the first parent target of the given class, or NULL if
> > the given
> >   * target does not have a parent of the given class. */
> > 
> 
> 
> 

Amitay.
-- 

A man is incomplete until he is married. After that, he is 
finished. - Zsa Zsa Gabor



More information about the Pdbg mailing list