Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 5 Feb 2020 16:45:08 +1100
From:   David Gibson <david@gibson.dropbear.id.au>
To:     Marek Szyprowski <m.szyprowski@samsung.com>
Cc:     devicetree-compiler@vger.kernel.org, linux-kernel@vger.kernel.org,
        Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Subject: Re: [PATCH] libfdt: place new nodes & properties after the parent's
 ones
Message-ID: <20200205054508.GG60221@umbus.fritz.box>
References: <CGME20200204125854eucas1p19ace564a5f45b9231e0fba8af07009cd@eucas1p1.samsung.com>
 <20200204125844.19955-1-m.szyprowski@samsung.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="MiFvc8Vo6wRSORdP"
Content-Disposition: inline
In-Reply-To: <20200204125844.19955-1-m.szyprowski@samsung.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


--MiFvc8Vo6wRSORdP
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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.
>=20
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

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(-)
>=20
> 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 nodeoff=
set, 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;
> =20
>  	if ((nextoffset =3D fdt_check_node_offset_(fdt, nodeoffset)) < 0)
>  		return nextoffset;
> =20
> +	/* Try to place the new property after the parent's properties */
> +	fdt_next_tag(fdt, nodeoffset, &nextoffset); /* skip the BEGIN_NODE */
> +	do {
> +		offset =3D nextoffset;
> +		tag =3D fdt_next_tag(fdt, offset, &nextoffset);
> +	} while ((tag =3D=3D FDT_PROP) || (tag =3D=3D FDT_NOP));
> +
>  	namestroff =3D fdt_find_add_string_(fdt, name, &allocated);
>  	if (namestroff < 0)
>  		return namestroff;
> =20
> -	*prop =3D fdt_offset_ptr_w_(fdt, nextoffset);
> +	*prop =3D fdt_offset_ptr_w_(fdt, offset);
>  	proplen =3D sizeof(**prop) + FDT_TAGALIGN(len);
> =20
>  	err =3D fdt_splice_struct_(fdt, *prop, 0, proplen);
> @@ -321,6 +329,7 @@ int fdt_add_subnode_namelen(void *fdt, int parentoffs=
et,
>  	struct fdt_node_header *nh;
>  	int offset, nextoffset;
>  	int nodelen;
> +	int depth =3D 0;
>  	int err;
>  	uint32_t tag;
>  	fdt32_t *endtag;
> @@ -333,12 +342,21 @@ int fdt_add_subnode_namelen(void *fdt, int parentof=
fset,
>  	else if (offset !=3D -FDT_ERR_NOTFOUND)
>  		return offset;
> =20
> -	/* 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 =3D nextoffset;
>  		tag =3D fdt_next_tag(fdt, offset, &nextoffset);
> -	} while ((tag =3D=3D FDT_PROP) || (tag =3D=3D FDT_NOP));
> +		if (depth && tag =3D=3D FDT_END_NODE) {
> +			depth--;
> +			goto again;
> +		}
> +		if (tag =3D=3D FDT_BEGIN_NODE) {
> +			depth++;
> +			goto again;
> +		}
> +	} while (depth || (tag =3D=3D FDT_PROP) || (tag =3D=3D FDT_NOP));
> =20
>  	nh =3D fdt_offset_ptr_w_(fdt, offset);
>  	nodelen =3D sizeof(*nh) + FDT_TAGALIGN(namelen+1) + FDT_TAGSIZE;

--=20
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

--MiFvc8Vo6wRSORdP
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEEdfRlhq5hpmzETofcbDjKyiDZs5IFAl46VmMACgkQbDjKyiDZ
s5KD3A//U4sv8jVoLxP+/L5hprNKU/EkWN2F3K31m0Tw7YXZeL3SyPV5rbzVqsHW
lYZ/VxuBU0huHffaPnSsXe7JOwmr8VZB+aueXNbFtgxggAqa2b+o8XZ2ILYEBufU
DjiZdN90fPalirsO9duLSTSVPCdZ9qEzgf9l5ts/xnrDNMngDfs2ZXjbJ+IcFEAs
G72pB+PGFz1s5FRLxMQBwlS32om4zQ5sGmYxLkt0C1b7i4hkRhU+aWJyAfLnak7i
8DZyJMXeC9wue4u4/4Z+rx/cRESXHvaRya8vtRQN/dVeOliI2PTw4rv9K3Hvtojg
v/ibSIK/9fIIz/4s1i585QInjsSoOHuQXRdy7y+MOfXKdpbWZaQVzS8Ya7IDLOo1
zxyv1InDzp72gWos2ZPd0KlPict2sFzZx6ua2DPWuguvO5b3kk44y+tmfKhqIX5j
gtchr6iRQK5HTfGPmyCDDhmzY1rpeA23keKHyALlrk08BgCYdnCN3ZsJ0jLKGrU7
Pk/nBi/UUc+seVy34TDLtCp+sL5E8UP14hFymSXeDFY10b+JzRXEvzjjE8U5hjeA
rUnOKvpTTSGaY3b/ZQKWx28OL3G1riWHCbh7y4xGmft9KH5jM7RuGbOyb1j/ERKD
YyOpNvvwTA9eCMgOsygwcoZzW/66ag5304TELK+ejdN90Pk9ttg=
=3XED
-----END PGP SIGNATURE-----

--MiFvc8Vo6wRSORdP--