* [PATCH v2] libfdt: overlay: ensure that existing phandles are not overwritten @ 2024-02-16 18:19 Uwe Kleine-König 2024-02-16 21:06 ` Uwe Kleine-König 2024-02-22 6:32 ` David Gibson 0 siblings, 2 replies; 10+ messages in thread From: Uwe Kleine-König @ 2024-02-16 18:19 UTC (permalink / raw) To: David Gibson; +Cc: Yves-Alexis Perez, devicetree-compiler, entwicklung 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. Best regards Uwe 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; } +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); +} + +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 *tree_val; + 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]); + + /* + * 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)); + + 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; + } + } + } + + 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) +{ + 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; + + 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; +} + +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; + 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 + PASS + else + FAIL + fi } pylibfdt_tests () { -- 2.43.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2] libfdt: overlay: ensure that existing phandles are not overwritten 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 1 sibling, 1 reply; 10+ messages in thread From: Uwe Kleine-König @ 2024-02-16 21:06 UTC (permalink / raw) To: David Gibson; +Cc: Yves-Alexis Perez, devicetree-compiler, entwicklung [-- Attachment #1: Type: text/plain, Size: 3739 bytes --] Hello, 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> After sending the patch out I had a nice idea for an optimisation here. It doesn't make the process faster, but improves the result. If phandle with the (shifted) value 17 is changed to 5, there is no phandle with the value 17 in the end. So while iterating over the fdto to replace all phandles with value 17 by 5 all values > 17 can be reduced by one. This way the phandle allocation in the resulting dtb is continuous if both inputs have continuous numbers. This can be implemented by the patch below on top of the patch I'm replying to. Best regards Uwe diff --git a/libfdt/fdt_overlay.c b/libfdt/fdt_overlay.c index 914acc5b14a6..bfdba50dee50 100644 --- a/libfdt/fdt_overlay.c +++ b/libfdt/fdt_overlay.c @@ -529,15 +529,25 @@ static int overlay_adjust_node_conflicting_phandle(void *fdto, int node, 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 (php && len == sizeof(*php)) { + if (fdt32_to_cpu(*php) == fdto_phandle) + ret = fdt_setprop_inplace_u32(fdto, node, "phandle", fdt_phandle); + else if (fdt32_to_cpu(*php) > fdto_phandle) + ret = fdt_setprop_inplace_u32(fdto, node, "phandle", fdt32_to_cpu(*php) - 1); + else + ret = 0; 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 (php && len == sizeof(*php)) { + if (fdt32_to_cpu(*php) == fdto_phandle) + ret = fdt_setprop_inplace_u32(fdto, node, "linux,phandle", fdt_phandle); + else if (fdt32_to_cpu(*php) > fdto_phandle) + ret = fdt_setprop_inplace_u32(fdto, node, "phandle", fdt32_to_cpu(*php) - 1); + else + ret = 0; if (ret) return ret; } @@ -609,23 +619,27 @@ static int overlay_update_node_conflicting_references(void *fdto, int tree_node, */ memcpy(&adj_val, tree_val + poffset, sizeof(adj_val)); - if (fdt32_to_cpu(adj_val) == fdto_phandle) { + if (fdt32_to_cpu(adj_val) < fdto_phandle) + continue; + if (fdt32_to_cpu(adj_val) == fdto_phandle) adj_val = cpu_to_fdt32(fdt_phandle); + else + adj_val = cpu_to_fdt32(fdt32_to_cpu(adj_val) - 1); - 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; - } + 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; } } -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ | [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2] libfdt: overlay: ensure that existing phandles are not overwritten 2024-02-16 21:06 ` Uwe Kleine-König @ 2024-02-22 6:33 ` David Gibson 0 siblings, 0 replies; 10+ messages in thread From: David Gibson @ 2024-02-22 6:33 UTC (permalink / raw) To: Uwe Kleine-König; +Cc: Yves-Alexis Perez, devicetree-compiler, entwicklung [-- Attachment #1: Type: text/plain, Size: 4191 bytes --] On Fri, Feb 16, 2024 at 10:06:37PM +0100, Uwe Kleine-König wrote: > Hello, > > 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> > > After sending the patch out I had a nice idea for an optimisation here. > It doesn't make the process faster, but improves the result. > > If phandle with the (shifted) value 17 is changed to 5, there is no > phandle with the value 17 in the end. So while iterating over the fdto > to replace all phandles with value 17 by 5 all values > 17 can be > reduced by one. This way the phandle allocation in the resulting dtb is > continuous if both inputs have continuous numbers. Honestly, I don't think the marginal advantage of improving the chances of contiguous phandles is worth the extra complexity. > > This can be implemented by the patch below on top of the patch I'm > replying to. > > Best regards > Uwe > > diff --git a/libfdt/fdt_overlay.c b/libfdt/fdt_overlay.c > index 914acc5b14a6..bfdba50dee50 100644 > --- a/libfdt/fdt_overlay.c > +++ b/libfdt/fdt_overlay.c > @@ -529,15 +529,25 @@ static int overlay_adjust_node_conflicting_phandle(void *fdto, int node, > 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 (php && len == sizeof(*php)) { > + if (fdt32_to_cpu(*php) == fdto_phandle) > + ret = fdt_setprop_inplace_u32(fdto, node, "phandle", fdt_phandle); > + else if (fdt32_to_cpu(*php) > fdto_phandle) > + ret = fdt_setprop_inplace_u32(fdto, node, "phandle", fdt32_to_cpu(*php) - 1); > + else > + ret = 0; > 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 (php && len == sizeof(*php)) { > + if (fdt32_to_cpu(*php) == fdto_phandle) > + ret = fdt_setprop_inplace_u32(fdto, node, "linux,phandle", fdt_phandle); > + else if (fdt32_to_cpu(*php) > fdto_phandle) > + ret = fdt_setprop_inplace_u32(fdto, node, "phandle", fdt32_to_cpu(*php) - 1); > + else > + ret = 0; > if (ret) > return ret; > } > @@ -609,23 +619,27 @@ static int overlay_update_node_conflicting_references(void *fdto, int tree_node, > */ > memcpy(&adj_val, tree_val + poffset, sizeof(adj_val)); > > - if (fdt32_to_cpu(adj_val) == fdto_phandle) { > + if (fdt32_to_cpu(adj_val) < fdto_phandle) > + continue; > > + if (fdt32_to_cpu(adj_val) == fdto_phandle) > adj_val = cpu_to_fdt32(fdt_phandle); > + else > + adj_val = cpu_to_fdt32(fdt32_to_cpu(adj_val) - 1); > > - 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; > - } > + 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; > } > } > -- 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 --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] libfdt: overlay: ensure that existing phandles are not overwritten 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:32 ` David Gibson 2024-02-22 7:22 ` Uwe Kleine-König 2024-02-22 9:38 ` Uwe Kleine-König 1 sibling, 2 replies; 10+ messages in thread From: David Gibson @ 2024-02-22 6:32 UTC (permalink / raw) To: Uwe Kleine-König; +Cc: Yves-Alexis Perez, devicetree-compiler, entwicklung [-- 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 --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] libfdt: overlay: ensure that existing phandles are not overwritten 2024-02-22 6:32 ` David Gibson @ 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 1 sibling, 1 reply; 10+ messages in thread From: Uwe Kleine-König @ 2024-02-22 7:22 UTC (permalink / raw) To: David Gibson; +Cc: Yves-Alexis Perez, devicetree-compiler, entwicklung [-- Attachment #1: Type: text/plain, Size: 4605 bytes --] Hello David, On Thu, Feb 22, 2024 at 05:32:10PM +1100, David Gibson wrote: > On Fri, Feb 16, 2024 at 07:19:20PM +0100, Uwe Kleine-König wrote: > > [...] > > 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). Yeah, I was lazy. I didn't expect this patch to go in without comments and avoided investing time documenting functions that maybe will change in the course of the review-resend loop. :-) > > [...] > > +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. Huh, I'm surprised about myself that I didn't notice this alone. > > + > > + /* > > + * 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. I copied this from somewhere, so I guess there is some potential for improvement in existing code. Will take a look. > > [...] > > + > > +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. Sounds good, wasn't too lucky with overlay_prevent_phandle_overwrite_node() either. > > +{ > > + 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. Oh right! > > [...] > > /** > > * 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. This is only to please the reader, right? Or is there a corner case I failed to see? The feedback I removed in this mail I agree with. Expect a v3 soon. Thanks for your time to review my patch, Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ | [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] libfdt: overlay: ensure that existing phandles are not overwritten 2024-02-22 7:22 ` Uwe Kleine-König @ 2024-02-22 9:02 ` David Gibson 0 siblings, 0 replies; 10+ messages in thread From: David Gibson @ 2024-02-22 9:02 UTC (permalink / raw) To: Uwe Kleine-König; +Cc: Yves-Alexis Perez, devicetree-compiler, entwicklung [-- Attachment #1: Type: text/plain, Size: 5039 bytes --] On Thu, Feb 22, 2024 at 08:22:48AM +0100, Uwe Kleine-König wrote: > Hello David, > > On Thu, Feb 22, 2024 at 05:32:10PM +1100, David Gibson wrote: > > On Fri, Feb 16, 2024 at 07:19:20PM +0100, Uwe Kleine-König wrote: > > > [...] > > > 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). > > Yeah, I was lazy. I didn't expect this patch to go in without comments > and avoided investing time documenting functions that maybe will change > in the course of the review-resend loop. :-) > > > > [...] > > > +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. > > Huh, I'm surprised about myself that I didn't notice this alone. > > > > + > > > + /* > > > + * 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. > > I copied this from somewhere, so I guess there is some potential for > improvement in existing code. Will take a look. > > > > [...] > > > + > > > +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. > > Sounds good, wasn't too lucky with > overlay_prevent_phandle_overwrite_node() either. > > > > +{ > > > + 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. > > Oh right! > > > > [...] > > > /** > > > * 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. > > This is only to please the reader, right? Or is there a corner case I > failed to see? Correct. At least as far as I've seen, too. > The feedback I removed in this mail I agree with. Expect a v3 soon. > > Thanks for your time to review my patch, > Uwe > -- 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 --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] libfdt: overlay: ensure that existing phandles are not overwritten 2024-02-22 6:32 ` David Gibson 2024-02-22 7:22 ` Uwe Kleine-König @ 2024-02-22 9:38 ` Uwe Kleine-König 2024-02-23 4:41 ` David Gibson 1 sibling, 1 reply; 10+ messages in thread From: Uwe Kleine-König @ 2024-02-22 9:38 UTC (permalink / raw) To: David Gibson; +Cc: Yves-Alexis Perez, devicetree-compiler, entwicklung [-- Attachment #1: Type: text/plain, Size: 3996 bytes --] On Thu, Feb 22, 2024 at 05:32:10PM +1100, David Gibson wrote: > On Fri, Feb 16, 2024 at 07:19:20PM +0100, Uwe Kleine-König wrote: > > +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 *tree_val; > > + 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 I don't see what you suggest here. fixup_val is assigned to in fdt_getprop_by_offset() which I need to get fixup_len and then fixup_val is already around. If you still think this could be improved, please be more explicit. > > + > > + /* > > + * 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. There is some complexity because tree_val is a const char * (as this is what fdt_getprop() returns) and fdt32_st() obviously doens't take a const pointer. If you want to play around with that, the code is mostly a copy of overlay_update_local_node_references() which could benefit from your suggestions in the same way. Can I lure you in improving overlay_update_local_node_references()? Then I could copy from the improved function for my patch. (Or maybe refactor the function to take a function as parameter which is uint32_t delta; uint32_t increase_by_delta(uint32_t phandle) { return phandle + delta; } for the overlay_update_local_node_references() case and uint32_t fdt_phandle; uint32_t fdto_phandle; uint32_t resolve_fdt_conflict(uint32_t phandle) { if (phandle == fdto_phandle) return fdt_phandle; else return phandle; } for my function (maybe passing the data stored in global vars here in a context parameter). Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ | [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] libfdt: overlay: ensure that existing phandles are not overwritten 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 0 siblings, 1 reply; 10+ messages in thread From: David Gibson @ 2024-02-23 4:41 UTC (permalink / raw) To: Uwe Kleine-König; +Cc: Yves-Alexis Perez, devicetree-compiler, entwicklung [-- Attachment #1: Type: text/plain, Size: 5157 bytes --] On Thu, Feb 22, 2024 at 10:38:54AM +0100, Uwe Kleine-König wrote: > On Thu, Feb 22, 2024 at 05:32:10PM +1100, David Gibson wrote: > > On Fri, Feb 16, 2024 at 07:19:20PM +0100, Uwe Kleine-König wrote: > > > +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 *tree_val; > > > + 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 > > I don't see what you suggest here. fixup_val is assigned to in > fdt_getprop_by_offset() which I need to get fixup_len and then fixup_val > is already around. If you still think this could be improved, please be > more explicit. poffset = fdt32_ld_(fixup_val + i); Thus combining the memory read and the byteswap into one operation. I guess it doesn't really simplify the code, but correctly handling reads and writes to memory within the dtb is what the fdt*_ld() and fdt*_st() helpers are for. > > > + > > > + /* > > > + * 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. Specifically adj_val = fdt32_ld(tree_val + poffset) And then replacing fdt32_to_cpu(adj_val) with just adj_val below. > > > > > + > > > + 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. > > There is some complexity because tree_val is a const char * (as this is > what fdt_getprop() returns) and fdt32_st() obviously doens't take a > const pointer. Ah, right. You can use fdt_getprop_w() to get a writable pointer instead. If there was a way to have fdt_getprop() return a const pointer only if the fdt pointer it was given was const, I'd do that. > If you want to play around with that, the code is mostly a copy of > overlay_update_local_node_references() which could benefit from your > suggestions in the same way. That's quite plausible. > Can I lure you in improving overlay_update_local_node_references()? Then Fair point.. https://github.com/dgibson/dtc/commit/24f60011fd43683d8e3916435c4c726e9baac9c9 > I could copy from the improved function for my patch. (Or maybe refactor > the function to take a function as parameter which is Eh... I'd prefer to avoid higher order functions in something this low level. > uint32_t delta; > uint32_t increase_by_delta(uint32_t phandle) > { > return phandle + delta; > } > > for the overlay_update_local_node_references() case and > > uint32_t fdt_phandle; > uint32_t fdto_phandle; > uint32_t resolve_fdt_conflict(uint32_t phandle) > { > if (phandle == fdto_phandle) > return fdt_phandle; > else > return phandle; > } > > for my function (maybe passing the data stored in global vars here in a > context parameter). > > Best regards > Uwe > -- 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 --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] libfdt: overlay: ensure that existing phandles are not overwritten 2024-02-23 4:41 ` David Gibson @ 2024-02-23 7:53 ` Uwe Kleine-König 2024-02-23 8:15 ` David Gibson 0 siblings, 1 reply; 10+ messages in thread From: Uwe Kleine-König @ 2024-02-23 7:53 UTC (permalink / raw) To: David Gibson; +Cc: Yves-Alexis Perez, devicetree-compiler, entwicklung [-- Attachment #1: Type: text/plain, Size: 1832 bytes --] On Fri, Feb 23, 2024 at 03:41:40PM +1100, David Gibson wrote: > On Thu, Feb 22, 2024 at 10:38:54AM +0100, Uwe Kleine-König wrote: > > On Thu, Feb 22, 2024 at 05:32:10PM +1100, David Gibson wrote: > > > And here you can use fdt32_st() on (tree_val + poffset), avoiding > > > quite some complexity. > > > > There is some complexity because tree_val is a const char * (as this is > > what fdt_getprop() returns) and fdt32_st() obviously doens't take a > > const pointer. > > Ah, right. You can use fdt_getprop_w() to get a writable pointer > instead. If there was a way to have fdt_getprop() return a const > pointer only if the fdt pointer it was given was const, I'd do that. Look at how container_of_const() is defined in the kernel[1], this requires C11 though. See https://en.cppreference.com/w/c/language/generic for some docs. [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/container_of.h?h=v6.7#n32 > > If you want to play around with that, the code is mostly a copy of > > overlay_update_local_node_references() which could benefit from your > > suggestions in the same way. > > That's quite plausible. > > > Can I lure you in improving overlay_update_local_node_references()? Then > > Fair point.. > > https://github.com/dgibson/dtc/commit/24f60011fd43683d8e3916435c4c726e9baac9c9 I gave feedback on that one. Looks good. > > I could copy from the improved function for my patch. (Or maybe refactor > > the function to take a function as parameter which is > > Eh... I'd prefer to avoid higher order functions in something this low > level. OK. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ | [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] libfdt: overlay: ensure that existing phandles are not overwritten 2024-02-23 7:53 ` Uwe Kleine-König @ 2024-02-23 8:15 ` David Gibson 0 siblings, 0 replies; 10+ messages in thread From: David Gibson @ 2024-02-23 8:15 UTC (permalink / raw) To: Uwe Kleine-König; +Cc: Yves-Alexis Perez, devicetree-compiler, entwicklung [-- Attachment #1: Type: text/plain, Size: 2461 bytes --] On Fri, Feb 23, 2024 at 08:53:37AM +0100, Uwe Kleine-König wrote: > On Fri, Feb 23, 2024 at 03:41:40PM +1100, David Gibson wrote: > > On Thu, Feb 22, 2024 at 10:38:54AM +0100, Uwe Kleine-König wrote: > > > On Thu, Feb 22, 2024 at 05:32:10PM +1100, David Gibson wrote: > > > > And here you can use fdt32_st() on (tree_val + poffset), avoiding > > > > quite some complexity. > > > > > > There is some complexity because tree_val is a const char * (as this is > > > what fdt_getprop() returns) and fdt32_st() obviously doens't take a > > > const pointer. > > > > Ah, right. You can use fdt_getprop_w() to get a writable pointer > > instead. If there was a way to have fdt_getprop() return a const > > pointer only if the fdt pointer it was given was const, I'd do that. > > Look at how container_of_const() is defined in the kernel[1], this > requires C11 though. See Right. Just like we avoid allocations to allow libfdt to be used in constrained environments like bootloaders, we're also very conservative with which compiler features we want to rely on. > https://en.cppreference.com/w/c/language/generic for some docs. > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/container_of.h?h=v6.7#n32 > > > > If you want to play around with that, the code is mostly a copy of > > > overlay_update_local_node_references() which could benefit from your > > > suggestions in the same way. > > > > That's quite plausible. > > > > > Can I lure you in improving overlay_update_local_node_references()? Then > > > > Fair point.. > > > > https://github.com/dgibson/dtc/commit/24f60011fd43683d8e3916435c4c726e9baac9c9 > > I gave feedback on that one. Looks good. > > > > I could copy from the improved function for my patch. (Or maybe refactor > > > the function to take a function as parameter which is > > > > Eh... I'd prefer to avoid higher order functions in something this low > > level. > > OK. To elaborate here, I can also image certain platform + environment + compiler combinations where indirect calls don't work properly in an early boot environment. They should, of course, but you'd be astonished how many messed up things I've seen in firmwares. -- 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 --] ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2024-02-23 8:16 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 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 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
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 NNTP newsgroup(s).