On Sun, Feb 25, 2024 at 06:54:23PM +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@xxxxxxxxxxxxxx>
> ---
> Hello,
>
> here comes the next iteration of the patch that fixes overlay
> application to not overwrite existing phandles.
>
> It is rebased to current main branch. The changes since v2 are:
>
> - Add documentation
> - Apply the simplification from 24f60011fd43 ("libfdt: Simplify
> adjustment of values for local fixups") in the functions added here.
> - Rename functions using shorter and better names
> - Changed the test device trees to yield a hole in the phandle space
> - Checked each phandle value not being overwritten separately
>
> Note I didn't switch the order of overlay_prevent_phandle_overwrite() and
> overlay_fixup_phandles() because the overlay's phandles must be resolved
> before I can do the recursion needed in
> overlay_prevent_phandle_overwrite().
I'm not following what you mean here. IIUC, conflicts of the sort
you're handling can only arise when the overlay describes a phandle
for the target node of the reference - and therefore that target is in
the overlay. In that case all references to it in the overlay should
be encoded in __local_fixups__ rather than __fixups__. __fixups__, in
contrast describes references to nodes that aren't in the overlay, and
so can't be filled in - even with a tentative value - until the
overlay is applied.
So, I'm not seeing how fixing these conflicts depends on resolution of
those "external" fixups, rather than just the "local" fixups. Am I
missing something?
>
> Best regards
> Uwe
>
> libfdt/fdt_overlay.c | 251 ++++++++++++++++++++++++++++++
> tests/overlay_base_phandle.dts | 21 +++
> tests/overlay_overlay_phandle.dts | 34 ++++
> tests/run_tests.sh | 28 ++++
> 4 files changed, 334 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 7ec839ae7118..f500f2d07030 100644
> --- a/libfdt/fdt_overlay.c
> +++ b/libfdt/fdt_overlay.c
> @@ -498,6 +498,249 @@ static int overlay_fixup_phandles(void *fdt, void *fdto)
> return 0;
> }
>
> +/**
> + * overlay_adjust_local_conflicting_phandle: Changes a phandle value
Nothing about this function's behaviour is really specific to
conflicts or local fixups: it just changes the phandle value. So
maybe simply 'change_phandle()'. It's a static function, so it
doesn't need to have a prefix to namespace it.
> + * @fdto: Device tree overlay
No reason this has to be an overlay, any tree will do.
> + * @node: The node the phandle is set for
> + * @fdt_phandle: The new value for the phandle
No reason this value has to come from an fdt.
> + *
> + * returns:
> + * 0 on success
> + * Negative error code on failure
> + */
> +static int overlay_adjust_local_conflicting_phandle(void *fdto, int node,
> + uint32_t fdt_phandle)
> +{
> + const fdt32_t *php;
> + int len, ret;
> +
> + php = fdt_getprop(fdto, node, "phandle", &len);
> + if (php && len == sizeof(*php)) {
> + ret = fdt_setprop_inplace_u32(fdto, node, "phandle", fdt_phandle);
> + if (ret)
> + return ret;
> + }
This can also be simplified using fdt_getprop_w() and fdt_st_():
php = fdt_getprop_w(fdt, node, "phandle", &len);
if (php && len == sizeof(*php))
fdt32_st_(php, phandle);
> +
> + php = fdt_getprop(fdto, node, "linux,phandle", &len);
> + if (php && len == sizeof(*php)) {
> + ret = fdt_setprop_inplace_u32(fdto, node, "linux,phandle", fdt_phandle);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * overlay_update_node_conflicting_references - Recursively replace phandle values
This name is still pretty long, and it describes the use case rather
than what it does. How about: update_one_phandle_refs(), and describe
it in terms of making the necessary ref changes for a single phandle
value changing.
> + * @fdto: Device tree overlay blob
> + * @tree_node: Node to recurse into
> + * @fixup_node: Node offset of the matching local fixups node
> + * @fdt_phandle: Value to replace phandles with
> + * @fdto_phandle: Value to be replaced
> + *
> + * Replaces all phandles with value @fdto_phandle by @fdt_phandle.
> + *
> + * returns:
> + * 0 on success
> + * Negative error code on failure
> + */
> +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) {
> + const fdt32_t *fixup_val;
> + const char *name;
> + char *tree_val;
> + 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_w(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 *refp;
> + uint32_t valp;
The "p" in "refp" is referring to the fact it's a pointer (in the C
sense) to a reference (in the fdt sense). So "valp" isn't a good name
for the value when you dereference it - that suggests a pointer (in
the C sense) to a value (in ?? sense). Just 'ref' or 'refval' would
be better.
Or just inline the fdt32_ld() into the if, since you only use it once.
> +
> + refp = (fdt32_t *)(tree_val + fdt32_ld_(fixup_val + i));
> + valp = fdt32_ld(refp);
> +
> + if (valp == fdto_phandle)
> + fdt32_st(refp, fdt_phandle);
> + }
> + }
> +
> + 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);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * overlay_update_local_conflicting_references - Recursively replace phandle values
> + * @fdto: Device tree overlay blob
> + * @fdt_phandle: Value to replace phandles with
> + * @fdto_phandle: Value to be replaced
> + *
> + * Replaces all phandles with value @fdto_phandle by @fdt_phandle.
> + *
> + * returns:
> + * 0 on success
> + * Negative error code on failure
> + */
> +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);
> +}
> +
> +/**
> + * overlay_prevent_phandle_overwrite_node - Helper function for overlay_prevent_phandle_overwrite
> + * @fdt: Base Device tree blob
> + * @fdtnode: Node in fdt that is checked for an overwrite
> + * @fdto: Device tree overlay blob
> + * @fdtonode: Node in fdto matching @fdtnode
> + *
> + * returns:
> + * 0 on success
> + * Negative error code on failure
> + */
> +static int overlay_prevent_phandle_overwrite_node(void *fdt, int fdtnode,
> + void *fdto, int fdtonode)
> +{
> + 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, fdtonode,
> + fdt_phandle);
> + if (ret)
> + return ret;
> +
> + ret = overlay_update_local_conflicting_references(fdto,
> + fdt_phandle,
> + fdto_phandle);
> + if (ret)
> + return ret;
> + }
> +
> + 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;
> +}
> +
> +/**
> + * overlay_prevent_phandle_overwrite - Fixes overlay phandles to not overwrite base phandles
> + * @fdt: Base Device Tree blob
> + * @fdto: Device tree overlay blob
> + *
> + * Checks recursively if applying fdto overwrites phandle values in the base
> + * dtb. When such a phandle is found, the fdto is changed to use the fdt's
> + * phandle value to not break references in the base.
> + *
> + * returns:
> + * 0 on success
> + * Negative error code on failure
> + */
> +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
> @@ -802,18 +1045,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;
> +
> 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..20639a7f4026
> --- /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_d: d {
> + b = <&node_b>;
> + };
> +};
> diff --git a/tests/overlay_overlay_phandle.dts b/tests/overlay_overlay_phandle.dts
> new file mode 100644
> index 000000000000..a0fa668faa5c
> --- /dev/null
> +++ b/tests/overlay_overlay_phandle.dts
> @@ -0,0 +1,34 @@
> +/*
> + * Copyright (c) 2024 Uwe Kleine-König
> + *
> + * SPDX-License-Identifier: GPL-2.0+
> + */
> +
> +/dts-v1/;
> +/plugin/;
> +
> +&{/} {
> + /*
> + * /b already has a label "node_b" in the base dts, also b is already
> + * referenced in the base, so both the base's b and this b have a
> + * phandle value.
> + */
> + node_b2: b {
> + value = <42>;
> + d = <&node_d>;
> + };
> +
> + node_c: c {
> + value = <23>;
> + b = <&node_b2>;
> + };
> +
> + d {
> + a = <&node_a>;
> + c = <&node_c>;
> + };
> +};
> +
> +&node_a {
> + value = <32>;
> +};
> diff --git a/tests/run_tests.sh b/tests/run_tests.sh
> index 3225a12bbb06..f13cdb216d2b 100755
> --- a/tests/run_tests.sh
> +++ b/tests/run_tests.sh
> @@ -1040,6 +1040,34 @@ 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 phandles 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)
> + bd=$($DTGET overlay_base_phandleO.test.dtb /b d)
> + cb=$($DTGET overlay_base_phandleO.test.dtb /c b)
> + da=$($DTGET overlay_base_phandleO.test.dtb /d a)
> + db=$($DTGET overlay_base_phandleO.test.dtb /d b)
> + dc=$($DTGET overlay_base_phandleO.test.dtb /d c)
> +
> + shorten_echo "check phandle to noda a wasn't overwritten: "
> + run_wrap_test test "$ba-$da" = "$pa-$pa"
> +
> + shorten_echo "check phandle to noda b wasn't overwritten: "
> + run_wrap_test test "$cb-$db" = "$pb-$pb"
> +
> + shorten_echo "check phandle to noda c wasn't overwritten: "
> + run_wrap_test test "$dc" = "$pc"
> +
> + shorten_echo "check phandle to noda d wasn't overwritten: "
> + run_wrap_test test "$bd" = "$pd"
> }
>
> pylibfdt_tests () {
>
> base-commit: 24f60011fd43683d8e3916435c4c726e9baac9c9
--
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
