[PATCH 08/21] dm: core: Allow adding ofnode subnodes

[Rasmus Villemoes]

On 31/08/2022 05.08, Simon Glass wrote:
> Add this feature to the ofnode interface, supporting both livetree and
> flattree.
> 
> Signed-off-by: Simon Glass <sjg at chromium.org>
> ---
> 
>  drivers/core/of_access.c | 50 ++++++++++++++++++++++++++++++++++++++++
>  drivers/core/ofnode.c    | 30 ++++++++++++++++++++++++
>  include/dm/of_access.h   | 12 ++++++++++
>  include/dm/ofnode.h      | 12 ++++++++++
>  test/dm/ofnode.c         | 17 ++++++++++++++
>  5 files changed, 121 insertions(+)
> 
> diff --git a/drivers/core/of_access.c b/drivers/core/of_access.c
> index ee09bbb7550..765b21cecdb 100644
> --- a/drivers/core/of_access.c
> +++ b/drivers/core/of_access.c
> @@ -931,3 +931,53 @@ int of_write_prop(struct device_node *np, const char *propname, int len,
>  
>  	return 0;
>  }
> +
> +int of_add_subnode(struct device_node *parent, const char *name, int len,
> +		   struct device_node **childp)
> +{
> +	struct device_node *child, *new, *last_sibling = NULL;
> +	char *new_name, *full_name;
> +	int parent_fnl;
> +
> +	__for_each_child_of_node(parent, child) {
> +		if (!strncmp(child->name, name, len) && strlen(name) == len)

I'm confused. The documentation comment further down says that len is
the length of name. So why this check? And further, the only caller (at
least introduced here, haven't been through the whole series) indeed
passes strlen(name).

So why is that even a parameter? Can't of_add_subnode() just do
len=strlen(name) itself? And then of course the strlen(name)==len
condition goes away, and the strncmp() can just be spelled strcmp() -
which also looks much more natural when asking "does such a child node
already exist".

> +			return -EEXIST;

It seems natural to provide the existing node in *childp when -EEXIST.
Callers that want to error out on -EEXIST can just do that and ignore
that value, but callers that just wants to ensure there is a subnode
with the given name can avoid doing a find_subnode() upfront, which
should make much code much more compact.

> +		last_sibling = child;
> +	}
> +
> +	/* Property does not exist -> append new property */

s/property/[sub]node/, or just delete the comment, it's obvious.

> +	new = calloc(1, sizeof(struct device_node));
> +	if (!new)
> +		return -ENOMEM;
> +
> +	new_name = malloc(len + 1);
> +	if (!name) {

That's not the variable you want to test...

> +		free(new);
> +		return -ENOMEM;
> +	}
> +	strlcpy(new_name, name, len + 1);
> +	new->name = new_name;

new->name = memdup(name, len+1);
if (!new->name) {...}

is a bit shorter.

> +	parent_fnl = parent->name ? strlen(parent->full_name) : 0;
> +	full_name = calloc(1, parent_fnl + 1 + len + 1);
> +	if (!full_name) {
> +		free(new_name);
> +		free(new);
> +		return -ENOMEM;
> +	}
> +	strcpy(full_name, parent->full_name);

Confused. Above, we use parent->name as a condition for whether
parent->full_name exists (or at least whether we should compute its
strlen). Here we unconditionally access parent->full_name. I don't know
the rules for the root node, but I can kind of guess that it has ->name
NULL and ->full_name set to "". Perhaps that could be clarified in the
documentation for struct device_node?

And if I'm right, that means there's no point asking if parent->name is
NULL or not; strlen(parent->full_name) gives the right answer regardless.

Rasmus