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

[Amitay Isaacs]

On Thu, 2019-09-26 at 15:22 +1000, Alistair Popple wrote:
> Ok, I think with the comments I was able to convince myself the
> traversal was 
> correct. I assume a real target is one which has a compatible
> property right?

Correct.


> 
> Reviewed-by: Alistair Popple <alistair at popple.id.au>
> 
> On Monday, 23 September 2019 6:48:27 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 | 96
> > ++++++++++++++++++++++++++++++++++++++++++++---
> >  libpdbg/libpdbg.h |  6 +--
> >  2 files changed, 93 insertions(+), 9 deletions(-)
> > 
> > diff --git a/libpdbg/libpdbg.c b/libpdbg/libpdbg.c
> > index aade5d3..1f23208 100644
> > --- a/libpdbg/libpdbg.c
> > +++ b/libpdbg/libpdbg.c
> > @@ -43,18 +43,102 @@ retry:
> >  	}
> >  }
> >  
> > -struct pdbg_target *__pdbg_next_child_target(struct pdbg_target
> > *parent, 
> struct pdbg_target *last)
> > +static struct pdbg_target *target_map_child(struct pdbg_target
> > *next, bool 
> system)
> >  {
> > -	if (!parent || list_empty(&parent->children))
> > +	/*
> > +	 * Map a target in system tree:
> > +	 *
> > +	 * - If there is no virtual node assiociated, then return the
> > target
> > +	 *   (the target itself can be virtual or real)
> > +	 *
> > +	 * - If there is virtual node associated,
> > +	 *     - If the target is virtual, return the associated node
> > +	 *     - If the target is real, return NULL
> > +	 *       (this target is already covered by previous condition)
> > +	 *
> > +	 * Map a target in backend tree:
> > +	 *
> > +	 *  - If the target is real, return the target
> > +	 *
> > +	 *  - If the target is virtual, return NULL
> > +	 *    (no virtual nodes in backend tree)
> > +	 */
> > +	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);
> > +	/*
> > +	 * Parent node can be virtual or real.
> > +	 *
> > +	 * If the parent node doesn't have any children,
> > +	 *    - If there is associated virtual node,
> > +	 *        Use that node as parent
> > +	 *
> > +	 *    - If there is no associated virtual node,
> > +	 *        No children
> > +	 */
> > +	if (list_empty(&parent->children)) {
> > +		if (parent->vnode)
> > +			parent = parent->vnode;
> > +		else
> > +			return NULL;
> > +	}
> > +
> > +	 /*
> > +	 * If the parent node has children,
> > +	 *    - Traverse the children
> > +	 *       (map the children in system or backend tree)
> > +	 */
> > +	if (!last) {
> > +		list_for_each(&parent->children, child, list)
> > +			if ((next = target_map_child(child, system)))
> > +				return next;
> >  
> > -	if (last->list.next == &parent->children.n)
> >  		return NULL;
> > +	}
> >  
> > -	return list_entry(last->list.next, struct pdbg_target, list);
> > +	/*
> > +	 * In a system tree traversal, virtual targets with associated
> > +	 * nodes, get mapped to real targets.
> > +	 *
> > +	 * When the last child is specified:
> > +	 *   - If in a system tree traverse, and
> > +	 *     the last child has associated node, and
> > +	 *     the last child is real
> > +	 *        Then the last child is not the actual child of the
> > parent
> > +	 *          (the associated virtual node is the actual child)
> > +	 */
> > +	if (system && last->vnode && last->compatible)
> > +		last = last->vnode;
> > +
> > +	if (last == list_tail(&parent->children, struct pdbg_target,
> > list))
> > +		return NULL;
> > +
> > +	child = last;
> > +	do {
> > +		child = list_entry(child->list.next, struct
> > pdbg_target, list);
> > +		if ((next = target_map_child(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.
-- 

Think highly of yourself because the world takes you at your own estimate.