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

[Alistair Popple]

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?

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. */
>