From: David Gibson <david@gibson.dropbear.id.au>
To: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
Cc: Yves-Alexis Perez <corsac@debian.org>,
devicetree-compiler@vger.kernel.org, entwicklung@pengutronix.de
Subject: Re: [PATCH v2] libfdt: overlay: ensure that existing phandles are not overwritten
Date: Thu, 22 Feb 2024 17:32:10 +1100 [thread overview]
Message-ID: <ZdbqaoTUfBmSlC98@zatzit> (raw)
In-Reply-To: <20240216181919.1648843-2-u.kleine-koenig@pengutronix.de>
[-- Attachment #1: Type: text/plain, Size: 14014 bytes --]
On Fri, Feb 16, 2024 at 07:19:20PM +0100, Uwe Kleine-König wrote:
> A phandle in an overlay is not supposed to overwrite a phandle that
> already exists in the base dtb as this breaks references to the
> respective node in the base.
>
> So add another iteration over the fdto that checks for such overwrites
> and fixes the fdto phandle's value to match the fdt's.
>
> A test is added that checks that newly added phandles and existing
> phandles work as expected.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> Hello,
>
> last year I already tried to address the issue of phandle overwriting in
> overlays. See
> https://lore.kernel.org/all/20230426100231.484497-1-u.kleine-koenig@pengutronix.de
>
> The approach I took back then required allocating a mapping. This isn't
> suiteable for libfdt though because it's expected to be embedded in code
> that cannot allocate dynamic memory. So here I implement an algorithm
> that doesn't allocate memory at the cost of a higher run time because it
> iterates over the whole dtbo for each conflict found. But there isn't a
> better way to do it without allocating memory.
Thanks for following this up. I think I finally got my head around
what's going on here. I really wish there were a more elegant way of
dealing with it, but I don't think there is. That said, there are
some smaller improvements that I think can be made, detailed comments
below.
> libfdt/fdt_overlay.c | 230 ++++++++++++++++++++++++++++++
> tests/overlay_base_phandle.dts | 21 +++
> tests/overlay_overlay_phandle.dts | 30 ++++
> tests/run_tests.sh | 23 +++
> 4 files changed, 304 insertions(+)
> create mode 100644 tests/overlay_base_phandle.dts
> create mode 100644 tests/overlay_overlay_phandle.dts
>
> diff --git a/libfdt/fdt_overlay.c b/libfdt/fdt_overlay.c
> index 5c0c3981b89d..914acc5b14a6 100644
> --- a/libfdt/fdt_overlay.c
> +++ b/libfdt/fdt_overlay.c
> @@ -520,6 +520,228 @@ static int overlay_fixup_phandles(void *fdt, void *fdto)
> return 0;
> }
>
All of these new functions could do with descriptive comments saying
what they do (see the existing code for examples).
> +static int overlay_adjust_node_conflicting_phandle(void *fdto, int node,
> + uint32_t fdt_phandle,
> + uint32_t fdto_phandle)
> +{
> + const fdt32_t *php;
> + int len, ret;
> + int child;
> +
> + php = fdt_getprop(fdto, node, "phandle", &len);
> + if (php && len == sizeof(*php) && fdt32_to_cpu(*php) == fdto_phandle) {
> + ret = fdt_setprop_inplace_u32(fdto, node, "phandle", fdt_phandle);
> + if (ret)
> + return ret;
> + }
> +
> + php = fdt_getprop(fdto, node, "linux,phandle", &len);
> + if (php && len == sizeof(*php) && fdt32_to_cpu(*php) == fdto_phandle) {
> + ret = fdt_setprop_inplace_u32(fdto, node, "linux,phandle", fdt_phandle);
> + if (ret)
> + return ret;
> + }
> +
> + fdt_for_each_subnode(child, fdto, node) {
> + ret = overlay_adjust_node_conflicting_phandle(fdto, child,
> + fdt_phandle, fdto_phandle);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int overlay_adjust_local_conflicting_phandle(void *fdto,
> + uint32_t fdt_phandle,
> + uint32_t fdto_phandle)
> +{
> + return overlay_adjust_node_conflicting_phandle(fdto, 0, fdt_phandle,
> + fdto_phandle);
> +}
I don't believe you need the above two functions: see further comments
at the call site.
> +static int overlay_update_node_conflicting_references(void *fdto, int tree_node,
> + int fixup_node,
> + uint32_t fdt_phandle,
> + uint32_t fdto_phandle)
> +{
> + int fixup_prop;
> + int fixup_child;
> + int ret;
> +
> + fdt_for_each_property_offset(fixup_prop, fdto, fixup_node) {
> +
Extraneous blank line.
> + const fdt32_t *fixup_val;
> + const char *tree_val;
libfdt style is to use tab indents, looks like most of this block has
space indents instead.
> + const char *name;
> + int fixup_len;
> + int tree_len;
> + int i;
> +
> + fixup_val = fdt_getprop_by_offset(fdto, fixup_prop,
> + &name, &fixup_len);
> + if (!fixup_val)
> + return fixup_len;
> +
> + if (fixup_len % sizeof(uint32_t))
> + return -FDT_ERR_BADOVERLAY;
> + fixup_len /= sizeof(uint32_t);
> +
> + tree_val = fdt_getprop(fdto, tree_node, name, &tree_len);
> + if (!tree_val) {
> + if (tree_len == -FDT_ERR_NOTFOUND)
> + return -FDT_ERR_BADOVERLAY;
> +
> + return tree_len;
> + }
> +
> + for (i = 0; i < fixup_len; i++) {
> + fdt32_t adj_val;
> + uint32_t poffset;
> +
> + poffset = fdt32_to_cpu(fixup_val[i]);
You can use fdt32_ld_() here to slightly simplify this
> +
> + /*
> + * phandles to fixup can be unaligned.
> + *
> + * Use a memcpy for the architectures that do
> + * not support unaligned accesses.
> + */
> + memcpy(&adj_val, tree_val + poffset, sizeof(adj_val));
And here you can use fdt32_ld() which has unaligned access handling
built in.
> +
> + if (fdt32_to_cpu(adj_val) == fdto_phandle) {
> +
> + adj_val = cpu_to_fdt32(fdt_phandle);
> +
> + ret = fdt_setprop_inplace_namelen_partial(fdto,
> + tree_node,
> + name,
> + strlen(name),
> + poffset,
> + &adj_val,
> + sizeof(adj_val));
> + if (ret == -FDT_ERR_NOSPACE)
> + return -FDT_ERR_BADOVERLAY;
> +
> + if (ret)
> + return ret;
And here you can use fdt32_st() on (tree_val + poffset), avoiding
quite some complexity.
> + }
> + }
> + }
> +
> + fdt_for_each_subnode(fixup_child, fdto, fixup_node) {
> + const char *fixup_child_name = fdt_get_name(fdto, fixup_child, NULL);
> + int tree_child;
> +
> + tree_child = fdt_subnode_offset(fdto, tree_node, fixup_child_name);
> +
> + if (tree_child == -FDT_ERR_NOTFOUND)
> + return -FDT_ERR_BADOVERLAY;
> + if (tree_child < 0)
> + return tree_child;
> +
> + ret = overlay_update_node_conflicting_references(fdto, tree_child,
> + fixup_child,
> + fdt_phandle,
> + fdto_phandle);
> + }
> +
> + return 0;
> +}
> +
> +static int overlay_update_local_conflicting_references(void *fdto,
> + uint32_t fdt_phandle,
> + uint32_t fdto_phandle)
> +{
> + int fixups;
> +
> + fixups = fdt_path_offset(fdto, "/__local_fixups__");
> + if (fixups == -FDT_ERR_NOTFOUND)
> + return 0;
> + if (fixups < 0)
> + return fixups;
> +
> + return overlay_update_node_conflicting_references(fdto, 0, fixups,
> + fdt_phandle,
> + fdto_phandle);
> +}
> +
> +static int overlay_prevent_phandle_overwrite_node(void *fdt, int fdtnode,
> + void *fdto, int fdtonode)
Maybe "overlay_resolve_phandle_conflict()" for a slightly shorter
function name.
> +{
> + uint32_t fdt_phandle, fdto_phandle;
> + int fdtochild;
> +
> + fdt_phandle = fdt_get_phandle(fdt, fdtnode);
> + fdto_phandle = fdt_get_phandle(fdto, fdtonode);
> +
> + if (fdt_phandle && fdto_phandle) {
> + int ret;
> +
> + ret = overlay_adjust_local_conflicting_phandle(fdto,
> + fdt_phandle,
> + fdto_phandle);
> + if (ret)
> + return ret;
So while, as you've seen, there can be conflicting phandles between
the fdt and the fdto, within the fdto a particular phandle should only
appear on a single node. If a phandle is duplicated across nodes
witin a single fdto, you have bigger problems. Therefore, having
found a conflict, you're already found the only place you need to
change the phandle property itself - you don't need to scan the full
fdto again.
So I believe you can replace the above call with just setting the
phandle property on fdtonode.
> +
> + ret = overlay_update_local_conflicting_references(fdto,
> + fdt_phandle,
> + fdto_phandle);
> + if (ret)
> + return ret;
You do, of course, have to scan the whole fdto to update references.
> + }
> +
> + fdt_for_each_subnode(fdtochild, fdto, fdtonode) {
> + const char *name = fdt_get_name(fdto, fdtochild, NULL);
> + int fdtchild;
> + int ret;
> +
> + fdtchild = fdt_subnode_offset(fdt, fdtnode, name);
> + if (fdtchild == -FDT_ERR_NOTFOUND)
> + /*
> + * no further overwrites possible here as this node is
> + * new
> + */
> + continue;
> +
> + ret = overlay_prevent_phandle_overwrite_node(fdt, fdtchild,
> + fdto, fdtochild);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int overlay_prevent_phandle_overwrite(void *fdt, void *fdto)
> +{
> + int fragment;
> +
> + fdt_for_each_subnode(fragment, fdto, 0) {
> + int overlay;
> + int target;
> + int ret;
> +
> + overlay = fdt_subnode_offset(fdto, fragment, "__overlay__");
> + if (overlay == -FDT_ERR_NOTFOUND)
> + continue;
> +
> + if (overlay < 0)
> + return overlay;
> +
> + target = fdt_overlay_target_offset(fdt, fdto, fragment, NULL);
> + if (target < 0)
> + return target;
> +
> + ret = overlay_prevent_phandle_overwrite_node(fdt, target,
> + fdto, overlay);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> /**
> * overlay_apply_node - Merges a node into the base device tree
> * @fdt: Base Device Tree blob
> @@ -824,18 +1046,26 @@ int fdt_overlay_apply(void *fdt, void *fdto)
> if (ret)
> goto err;
>
> + /* Increase all phandles in the fdto by delta */
> ret = overlay_adjust_local_phandles(fdto, delta);
> if (ret)
> goto err;
>
> + /* Adapt the phandle values in fdto to the above increase */
> ret = overlay_update_local_references(fdto, delta);
> if (ret)
> goto err;
>
> + /* Update fdto's phandles using symbols from fdt */
> ret = overlay_fixup_phandles(fdt, fdto);
> if (ret)
> goto err;
>
> + /* Don't overwrite phandles in fdt */
> + ret = overlay_prevent_phandle_overwrite(fdt, fdto);
> + if (ret)
> + goto err;
I think this should go above overlay_fixup_phandles(), so that we're
completing all the __local_fixups__ based adjustments before going to
the __fixups__ based adjustments.
> +
> ret = overlay_merge(fdt, fdto);
> if (ret)
> goto err;
> diff --git a/tests/overlay_base_phandle.dts b/tests/overlay_base_phandle.dts
> new file mode 100644
> index 000000000000..36f013c772a2
> --- /dev/null
> +++ b/tests/overlay_base_phandle.dts
> @@ -0,0 +1,21 @@
> +/*
> + * Copyright (c) 2024 Uwe Kleine-König
> + *
> + * SPDX-License-Identifier: GPL-2.0+
> + */
> +
> +/dts-v1/;
> +
> +/ {
> + node_a: a {
> + value = <17>;
> + };
> +
> + node_b: b {
> + a = <&node_a>;
> + };
> +
> + node_c: c {
> + b = <&node_b>;
> + };
> +};
> diff --git a/tests/overlay_overlay_phandle.dts b/tests/overlay_overlay_phandle.dts
> new file mode 100644
> index 000000000000..218e5266b992
> --- /dev/null
> +++ b/tests/overlay_overlay_phandle.dts
> @@ -0,0 +1,30 @@
> +/*
> + * Copyright (c) 2024 Uwe Kleine-König
> + *
> + * SPDX-License-Identifier: GPL-2.0+
> + */
> +
> +/dts-v1/;
> +/plugin/;
> +
> +&{/} {
> + /* /a already has a label "node_a" in the base dts */
> + node_b2: b {
> + value = <42>;
> + c = <&node_c>;
> + };
> +
> + c {
> + a = <&node_a>;
> + d = <&node_d>;
> + };
> +
> + node_d: d {
> + value = <23>;
> + b = <&node_b2>;
> + };
> +};
> +
> +&node_a {
> + value = <32>;
> +};
> diff --git a/tests/run_tests.sh b/tests/run_tests.sh
> index 3225a12bbb06..256472b623a3 100755
> --- a/tests/run_tests.sh
> +++ b/tests/run_tests.sh
> @@ -1040,6 +1040,28 @@ fdtoverlay_tests() {
> run_dtc_test -@ -I dts -O dtb -o $stacked_addlabeldtb $stacked_addlabel
>
> run_fdtoverlay_test baz "/foonode/barnode/baznode" "baz-property" "-ts" ${stacked_base_nolabeldtb} ${stacked_addlabel_targetdtb} ${stacked_addlabeldtb} ${stacked_bardtb} ${stacked_bazdtb}
> +
> + # verify that labels are not overwritten
> + run_dtc_test -@ -I dts -O dtb -o overlay_base_phandle.test.dtb "$SRCDIR/overlay_base_phandle.dts"
> + run_dtc_test -@ -I dts -O dtb -o overlay_overlay_phandle.test.dtb "$SRCDIR/overlay_overlay_phandle.dts"
> + run_wrap_test $FDTOVERLAY -i overlay_base_phandle.test.dtb -o overlay_base_phandleO.test.dtb overlay_overlay_phandle.test.dtb
> +
> + pa=$($DTGET overlay_base_phandleO.test.dtb /a phandle)
> + pb=$($DTGET overlay_base_phandleO.test.dtb /b phandle)
> + pc=$($DTGET overlay_base_phandleO.test.dtb /c phandle)
> + pd=$($DTGET overlay_base_phandleO.test.dtb /d phandle)
> + ba=$($DTGET overlay_base_phandleO.test.dtb /b a)
> + bc=$($DTGET overlay_base_phandleO.test.dtb /b c)
> + ca=$($DTGET overlay_base_phandleO.test.dtb /c a)
> + cb=$($DTGET overlay_base_phandleO.test.dtb /c b)
> + cd=$($DTGET overlay_base_phandleO.test.dtb /c d)
> + db=$($DTGET overlay_base_phandleO.test.dtb /d b)
> + shorten_echo "check phandle wasn't overwritten: "
> + if test "$ba-$bc-$ca-$cb-$cd-$db" = "$pa-$pc-$pa-$pb-$pd-$pb"; then
I'd prefer to see each of these pairs as a separate test case.
> + PASS
> + else
> + FAIL
> + fi
> }
>
> pylibfdt_tests () {
--
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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
next prev parent reply other threads:[~2024-02-22 6:33 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-16 18:19 [PATCH v2] libfdt: overlay: ensure that existing phandles are not overwritten Uwe Kleine-König
2024-02-16 21:06 ` Uwe Kleine-König
2024-02-22 6:33 ` David Gibson
2024-02-22 6:32 ` David Gibson [this message]
2024-02-22 7:22 ` Uwe Kleine-König
2024-02-22 9:02 ` David Gibson
2024-02-22 9:38 ` Uwe Kleine-König
2024-02-23 4:41 ` David Gibson
2024-02-23 7:53 ` Uwe Kleine-König
2024-02-23 8:15 ` David Gibson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZdbqaoTUfBmSlC98@zatzit \
--to=david@gibson.dropbear.id.au \
--cc=corsac@debian.org \
--cc=devicetree-compiler@vger.kernel.org \
--cc=entwicklung@pengutronix.de \
--cc=u.kleine-koenig@pengutronix.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).