Re: [PATCH] libfdt: place new nodes & properties after the parent's ones

[David Gibson]

On Tue, Feb 04, 2020 at 01:58:44PM +0100, Marek Szyprowski wrote:
> While applying dt-overlays using libfdt code, the order of the applied
> properties and sub-nodes is reversed. This should not be a problem in
> ideal world (mainline), but this matters for some vendor specific/custom
> dtb files. This can be easily fixed by the little change to libfdt code:
> any new properties and sub-nodes should be added after the parent's node
> properties and subnodes.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>

I'm not convinced this is a good idea.

First, anything that relies on the order of properties or subnodes in
a dtb is deeply, fundamentally broken.  That can't even really be a
problem with a dtb file itself, only with the code processing it.

I'm also concerned this could have a negative performance impact,
since it has to skip over a bunch of existing things before adding the
new one.  On the other hand, that may be offset by the fact that it
will reduce the amount of stuff that needs to be memmove()ed later on.

> ---
>  libfdt/fdt_rw.c | 26 ++++++++++++++++++++++----
>  1 file changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/libfdt/fdt_rw.c b/libfdt/fdt_rw.c
> index 8795947..88c5930 100644
> --- a/libfdt/fdt_rw.c
> +++ b/libfdt/fdt_rw.c
> @@ -189,19 +189,27 @@ static int fdt_add_property_(void *fdt, int nodeoffset, const char *name,
>  			     int len, struct fdt_property **prop)
>  {
>  	int proplen;
> -	int nextoffset;
> +	int offset, nextoffset;
>  	int namestroff;
>  	int err;
>  	int allocated;
> +	uint32_t tag;
>  
>  	if ((nextoffset = fdt_check_node_offset_(fdt, nodeoffset)) < 0)
>  		return nextoffset;
>  
> +	/* Try to place the new property after the parent's properties */
> +	fdt_next_tag(fdt, nodeoffset, &nextoffset); /* skip the BEGIN_NODE */
> +	do {
> +		offset = nextoffset;
> +		tag = fdt_next_tag(fdt, offset, &nextoffset);
> +	} while ((tag == FDT_PROP) || (tag == FDT_NOP));
> +
>  	namestroff = fdt_find_add_string_(fdt, name, &allocated);
>  	if (namestroff < 0)
>  		return namestroff;
>  
> -	*prop = fdt_offset_ptr_w_(fdt, nextoffset);
> +	*prop = fdt_offset_ptr_w_(fdt, offset);
>  	proplen = sizeof(**prop) + FDT_TAGALIGN(len);
>  
>  	err = fdt_splice_struct_(fdt, *prop, 0, proplen);
> @@ -321,6 +329,7 @@ int fdt_add_subnode_namelen(void *fdt, int parentoffset,
>  	struct fdt_node_header *nh;
>  	int offset, nextoffset;
>  	int nodelen;
> +	int depth = 0;
>  	int err;
>  	uint32_t tag;
>  	fdt32_t *endtag;
> @@ -333,12 +342,21 @@ int fdt_add_subnode_namelen(void *fdt, int parentoffset,
>  	else if (offset != -FDT_ERR_NOTFOUND)
>  		return offset;
>  
> -	/* Try to place the new node after the parent's properties */
> +	/* Try to place the new node after the parent's subnodes */
>  	fdt_next_tag(fdt, parentoffset, &nextoffset); /* skip the BEGIN_NODE */
>  	do {
> +again:
>  		offset = nextoffset;
>  		tag = fdt_next_tag(fdt, offset, &nextoffset);
> -	} while ((tag == FDT_PROP) || (tag == FDT_NOP));
> +		if (depth && tag == FDT_END_NODE) {
> +			depth--;
> +			goto again;
> +		}
> +		if (tag == FDT_BEGIN_NODE) {
> +			depth++;
> +			goto again;
> +		}
> +	} while (depth || (tag == FDT_PROP) || (tag == FDT_NOP));
>  
>  	nh = fdt_offset_ptr_w_(fdt, offset);
>  	nodelen = sizeof(*nh) + FDT_TAGALIGN(namelen+1) + FDT_TAGSIZE;

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Attachment: signature.asc
Description: PGP signature