On Sat, Dec 19, 2020 at 05:28:08PM +0000, Paul Barker wrote:
> When applying an overlay fragment, we should take care not to overwrite
> an existing phandle property of the target node as this could break
> references to the target node elsewhere in the base dtb.
It could.. but my first inclination is to say "don't do that".
> In addition to potentially breaking references within the resulting fdt,
> if the overlay is built with symbols enabled (`-@` option to dtc) then
> fdtoverlay will be unable to merge the overlay with a base dtb file.
If we can manage, I really think the better fix is to *not* have the
overlay provide conflicting phandles, rather than having the merge
process ignore them.
I think we need to pin down the cirucmstances in which overlays with
phandles are being generated and address those, if possible.
> A new test case is added to check how fdtoverlay handles this case.
> Attempting to apply this test overlay without the fix in this patch
> results in the following output:
>
> input = tests/overlay_base_ref.test.dtb
> output = tests/overlay_overlay_ref.fdtoverlay.dtb
> overlay[0] = tests/overlay_overlay_ref.test.dtb
>
> Failed to apply 'tests/overlay_overlay_ref.test.dtb':
> FDT_ERR_NOTFOUND
[snip]
> diff --git a/tests/overlay_base_ref.dts b/tests/overlay_base_ref.dts
> new file mode 100644
> index 0000000..1fc02a2
> --- /dev/null
> +++ b/tests/overlay_base_ref.dts
> @@ -0,0 +1,19 @@
> +/*
> + * Copyright (c) 2016 NextThing Co
> + * Copyright (c) 2016 Free Electrons
> + * Copyright (c) 2016 Konsulko Inc.
> + *
> + * SPDX-License-Identifier: GPL-2.0+
> + */
> +
> +/dts-v1/;
> +
> +/ {
> + test: test-node {
> + test-int-property = <42>;
> + };
> +
> + test-refs {
> + refs = <&test>;
> + };
> +};
> diff --git a/tests/overlay_overlay_ref.dts b/tests/overlay_overlay_ref.dts
> new file mode 100644
> index 0000000..a45c95d
> --- /dev/null
> +++ b/tests/overlay_overlay_ref.dts
> @@ -0,0 +1,24 @@
> +/*
> + * Copyright (c) 2016 NextThing Co
> + * Copyright (c) 2016 Free Electrons
> + * Copyright (c) 2016 Konsulko Inc.
> + *
> + * SPDX-License-Identifier: GPL-2.0+
> + */
> +
> +/dts-v1/;
> +/plugin/;
Given that you're using the /plugin/ tag, it doesn't make sense to use
the "manual" way of constructing the overlay, rather than dtc's
built-in syntax:
&test {
...
}
> +
> +/ {
> + fragment@0 {
> + target = <&test>;
> +
> + frag0: __overlay__ {
> + test-int-property = <43>;
> + };
> + };
> +
> + test-ref {
> + ref = <&frag0>;
> + };
> +};
Things get weird in this example, because the point is we're equating
the __overlay__ node with the target node. Just ignoring the phandle
overwrite is *wrong* in this case, because it will break test-ref
(except that test-ref isn't in a fragment, and therefore discarded,
but that could be changed). Instead to handle this case correctly
we need to identify that we're asking the __overlay__ node to be the
same thing as &test and so &frag0 needs to be rewritten as &test.
> diff --git a/tests/run_tests.sh b/tests/run_tests.sh
> index 294585b..a65b166 100755
> --- a/tests/run_tests.sh
> +++ b/tests/run_tests.sh
> @@ -329,6 +329,11 @@ dtc_overlay_tests () {
> run_test check_path overlay_base_with_aliases.dtb not-exists "/__symbols__"
> run_test check_path overlay_base_with_aliases.dtb not-exists "/__fixups__"
> run_test check_path overlay_base_with_aliases.dtb not-exists "/__local_fixups__"
> +
> + # Test taking a reference to an overlay fragment
> + run_dtc_test -@ -I dts -O dtb -o overlay_base_ref.test.dtb "$SRCDIR/overlay_base_ref.dts"
> + run_dtc_test -@ -I dts -O dtb -o overlay_overlay_ref.test.dtb "$SRCDIR/overlay_overlay_ref.dts"
> + run_wrap_test $FDTOVERLAY -i overlay_base_ref.test.dtb overlay_overlay_ref.test.dtb -o overlay_overlay_ref.fdtoverlay.dtb
> }
>
> tree1_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:
signature.asc
Description: PGP signature
