[Skiboot] [PATCH v9 05/11] dt: Add phandle fixup helpers

[Stewart Smith]

Anju T Sudhakar <anju at linux.vnet.ibm.com> writes:
> Patch to add a phandle fixup function for a new

You don't need to say "patch to add". You can drop the whole first
sentence.

> incoming subtree. When attaching a "subtree" to
> main dt, subtree could have "property" referring
> different nodes or multiple nodes via phandle as
> "labels". For this reason, new subtree will have
> "phandle" already assigned to some of it's nodes.
> If this new subtree is attached to main dt without
> phandle fixup, could lead to duplicate phandle errors
>
> dt_fixup_populate_hash() does two main things. It first
> uses a hash primitive to create a hashtable using phandle
> as key. It then loops through the list of properties specified
> as an input to this function (which needs to be fixed up),
> and calls the fixup "handler".
>
> Signed-off-by: Anju T Sudhakar <anju at linux.vnet.ibm.com>
> Signed-off-by: Madhavan Srinivasan <maddy at linux.vnet.ibm.com>
> ---
>  core/device.c    | 47 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/device.h | 19 +++++++++++++++++++
>  2 files changed, 66 insertions(+)
>
> diff --git a/core/device.c b/core/device.c
> index 170e0ee..e933c27 100644
> --- a/core/device.c
> +++ b/core/device.c
> @@ -17,6 +17,7 @@
>  #include <device.h>
>  #include <stdlib.h>
>  #include <skiboot.h>
> +#include <hash.h>
>  #include <libfdt/libfdt.h>
>  #include <libfdt/libfdt_internal.h>
>  #include <ccan/str/str.h>
> @@ -849,6 +850,52 @@ void dt_expand(const void *fdt)
>  		abort();
>  }
>
> +/* dt_fixup_populate_hash: populate the hash table, and invoke the fixup-handler
> + *			   to fixup the phandle value of the nodes.
> + */
> +int dt_fixup_populate_hash(struct dt_node *dev, hash_table_p hash, phandle_fixup_p list_prop )
> +{
> +	struct dt_node *node;
> +	hash_entry_p entry;
> +	fixup_prop_list *list_pp;
> +	u32 phandle;
> +	phandle_fixup_n *val;
> +
> +	/* Initialize the hashtable */
> +	hash_init(hash);
> +
> +	/* For each node of 'dev', insert the node-phandle pair into hashtable */
> +	dt_for_each_node(dev, node) {
> +		if (node->phandle > 0) {
> +			val = (phandle_fixup_n *)malloc(sizeof(phandle_fixup_n));
> +			if(!val) {
> +				prerror("Failed to allocate memory");
> +				return -1;
> +			}
> +			val->node = node;
> +			val->fixed = false;
> +			entry = hash_insert_entry(hash, node->phandle, val);
> +			if(!entry) {
> +				prerror("Failed to make hash table entry");
> +				return -1;
> +			}
> +		}
> +	}
> +	list_for_each(&list_prop->list, list_pp, list) {
> +		phandle = dt_prop_get_u32(list_pp->node, list_pp->prop->name);
> +		/* look for the entry, with phandle as the key to hashtable */
> +		entry = hash_lookup_entry(hash, phandle);
> +		if (entry) {
> +			/* If find a valid entry in the hashtable, do the fixup */
> +			list_prop->fixup(entry, list_pp->prop);
> +		} else {
> +			prerror("%s: Missing hash entry phandle = %x\n", __FUNCTION__, phandle);
> +			return -1;
> +		}
> +	}
> +	return 0;
> +}
> +

This looks overly complex for what needs to be done.

If we're importing a subtree that uses phandles say 1-100, and our
existing last_phandle=42, then all we need to do is go through and add
42 to all of the phandles in the subtree, and at the end, set
last_phandle=42+100.

This way we avoid having to look anything up.

The only way this could break is if the subtree is particularly insane,
but since we control it, it should never be (and the code could check
for that and abort out).

Unless I'm missing something, this should be both correct and simple, right?

-- 
Stewart Smith
OPAL Architect, IBM.