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

[Alistair Popple]

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;

> +		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.

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