On Tue, Jul 25, 2017 at 08:24:35PM +0300, Pantelis Antoniou wrote:
> This patch enables an overlay to refer to a previous overlay's
> labels by performing a merge of symbol information at application
> time.
>
> In a nutshell it allows an overlay to refer to a symbol that a previous
> overlay has defined. It requires both the base and all the overlays
> to be compiled with the -@ command line switch so that symbol
> information is included.
Sorry I've taken so long to review this spin. There's a few nits
left, but it's very close to ready.
>
> base.dts
> --------
>
> /dts-v1/;
> / {
> foo: foonode {
> foo-property;
> };
> };
>
> $ dtc -@ -I dts -O dtb -o base.dtb base.dts
>
> bar.dts
> -------
>
> /dts-v1/;
> /plugin/;
> / {
> fragment@1 {
> target = <&foo>;
> __overlay__ {
> overlay-1-property;
> bar: barnode {
> bar-property;
> };
> };
> };
> };
>
> $ dtc -@ -I dts -O dtb -o bar.dtb bar.dts
>
> baz.dts
> -------
>
> /dts-v1/;
> /plugin/;
> / {
> fragment@1 {
> target = <&bar>;
> __overlay__ {
> overlay-2-property;
> baz: baznode {
> baz-property;
> };
> };
> };
> };
>
> $ dtc -@ -I dts -O dtb -o baz.dtb baz.dts
>
> Applying the overlays:
>
> $ fdtoverlay -i base.dtb -o target.dtb bar.dtb baz.dtb
>
> Dumping:
>
> $ fdtdump target.dtb
> / {
> foonode {
> overlay-1-property;
> foo-property;
> linux,phandle = <0x00000001>;
> phandle = <0x00000001>;
> barnode {
> overlay-2-property;
> phandle = <0x00000002>;
> linux,phandle = <0x00000002>;
> bar-property;
> baznode {
> phandle = <0x00000003>;
> linux,phandle = <0x00000003>;
> baz-property;
> };
> };
> };
> __symbols__ {
> baz = "/foonode/barnode/baznode";
> bar = "/foonode/barnode";
> foo = "/foonode";
> };
> };
>
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@xxxxxxxxxxxx>
> ---
> libfdt/fdt_overlay.c | 197 +++++++++++++++++++++++++++++++++++++++++++++------
> 1 file changed, 175 insertions(+), 22 deletions(-)
>
> diff --git a/libfdt/fdt_overlay.c b/libfdt/fdt_overlay.c
> index ceb9687..e6fe058 100644
> --- a/libfdt/fdt_overlay.c
> +++ b/libfdt/fdt_overlay.c
> @@ -39,6 +39,7 @@ static uint32_t overlay_get_target_phandle(const void *fdto, int fragment)
> * @fdt: Base device tree blob
> * @fdto: Device tree overlay blob
> * @fragment: node offset of the fragment in the overlay
> + * @pathp: pointer which receives the path of the target (or NULL)
> *
> * overlay_get_target() retrieves the target offset in the base
> * device tree of a fragment, no matter how the actual targetting is
> @@ -49,37 +50,47 @@ static uint32_t overlay_get_target_phandle(const void *fdto, int fragment)
> * Negative error code on error
> */
> static int overlay_get_target(const void *fdt, const void *fdto,
> - int fragment)
> + int fragment, char const **pathp)
> {
> uint32_t phandle;
> - const char *path;
> - int path_len;
> + const char *path = NULL;
> + int path_len = 0, ret;
>
> /* Try first to do a phandle based lookup */
> phandle = overlay_get_target_phandle(fdto, fragment);
> if (phandle == (uint32_t)-1)
> return -FDT_ERR_BADPHANDLE;
>
> - if (phandle)
> - return fdt_node_offset_by_phandle(fdt, phandle);
> + /* no phandle, try path */
> + if (!phandle) {
> + /* And then a path based lookup */
> + path = fdt_getprop(fdto, fragment, "target-path", &path_len);
> + if (path)
> + ret = fdt_path_offset(fdt, path);
> + else
> + ret = path_len;
> + } else
> + ret = fdt_node_offset_by_phandle(fdt, phandle);
>
> - /* And then a path based lookup */
> - path = fdt_getprop(fdto, fragment, "target-path", &path_len);
> - if (!path) {
> - /*
> - * If we haven't found either a target or a
> - * target-path property in a node that contains a
> - * __overlay__ subnode (we wouldn't be called
> - * otherwise), consider it a improperly written
> - * overlay
> - */
> - if (path_len == -FDT_ERR_NOTFOUND)
> - return -FDT_ERR_BADOVERLAY;
> + /*
> + * If we haven't found either a target or a
> + * target-path property in a node that contains a
> + * __overlay__ subnode (we wouldn't be called
> + * otherwise), consider it a improperly written
> + * overlay
> + */
> + if (ret < 0 && path_len == -FDT_ERR_NOTFOUND)
> + ret = -FDT_ERR_BADOVERLAY;
> +
> + /* return on error */
> + if (ret < 0)
> + return ret;
>
> - return path_len;
> - }
> + /* return pointer to path (if available) */
> + if (pathp)
> + *pathp = path ? path : NULL;
>
> - return fdt_path_offset(fdt, path);
> + return ret;
> }
>
> /**
> @@ -590,7 +601,7 @@ static int overlay_apply_node(void *fdt, int target,
> *
> * overlay_merge() merges an overlay into its base device tree.
> *
> - * This is the final step in the device tree overlay application
> + * This is the next to last step in the device tree overlay application
> * process, when all the phandles have been adjusted and resolved and
> * you just have to merge overlay into the base device tree.
> *
> @@ -618,7 +629,7 @@ static int overlay_merge(void *fdt, void *fdto)
> if (overlay < 0)
> return overlay;
>
> - target = overlay_get_target(fdt, fdto, fragment);
> + target = overlay_get_target(fdt, fdto, fragment, NULL);
> if (target < 0)
> return target;
>
> @@ -630,6 +641,144 @@ static int overlay_merge(void *fdt, void *fdto)
> return 0;
> }
>
> +/**
> + * overlay_symbol_update - Update the symbols of base tree after a merge
> + * @fdt: Base Device Tree blob
> + * @fdto: Device tree overlay blob
> + *
> + * overlay_symbol_update() updates the symbols of the base tree with the
> + * symbols of the applied overlay
> + *
> + * This is the last step in the device tree overlay application
> + * process, allowing the reference of overlay symbols by subsequent
> + * overlay operations.
> + *
> + * returns:
> + * 0 on success
> + * Negative error code on failure
> + */
> +static int overlay_symbol_update(void *fdt, void *fdto)
> +{
> + int root_sym, ov_sym, prop, path_len, fragment, target;
> + int len, frag_name_len, ret, rel_path_len;
> + const char *s;
> + const char *path;
> + const char *name;
> + const char *frag_name;
> + const char *rel_path;
> + const char *target_path;
> + char *buf;
> + void *p;
> +
> + ov_sym = fdt_subnode_offset(fdto, 0, "__symbols__");
> +
> + /* if no overlay symbols exist no problem */
> + if (ov_sym < 0)
> + return 0;
> +
> + root_sym = fdt_subnode_offset(fdt, 0, "__symbols__");
> +
> + /* it no root symbols exist we should create them */
> + if (root_sym < 0) {
> + root_sym = fdt_add_subnode(fdt, 0, "__symbols__");
> + if (root_sym < 0)
> + return root_sym;
> + }
> +
> + /* iterate over each overlay symbol */
> + fdt_for_each_property_offset(prop, fdto, ov_sym) {
> + path = fdt_getprop_by_offset(fdto, prop, &name, &path_len);
> + if (!path)
> + return path_len;
> +
> + /* verify it's a string property (terminated with \0) */
> + if (path_len < 1 || path[path_len - 1] != '\0')
> + return -FDT_ERR_BADVALUE;
That's probably enough to be safe, but checking that memchr(path,
'\0', prop_len) points at the last byte of the property would be more
complete.
> +
> + /* format: /<fragment-name>/__overlay__/<relative-subnode-path> */
> +
> + if (*path != '/')
> + return -FDT_ERR_BADVALUE;
> +
> + /* verify that no stray \0 exist in the property */
> + if (strlen(path) != path_len - 1)
> + return -FDT_ERR_BADVALUE;
> +
> + /* get fragment name first */
> + s = strchr(path + 1, '/');
> + if (!s)
> + return -FDT_ERR_BADVALUE;
BADOVERLAY for these two, I think. These values are part of the
overlay's structural information, rather than a value for direct
client consumption.
> +
> + frag_name = path + 1;
> + frag_name_len = s - path - 1;
> +
> + /* verify format; safe since "s" lies in \0 terminated prop */
> + len = strlen("/__overlay__/");
> + if (strncmp(s, "/__overlay__/", len))
memcmp is simpler here.
> + return -FDT_ERR_NOTFOUND;
> +
> + rel_path = s + len;
> + rel_path_len = strlen(rel_path);
> +
> + /* find the fragment index in which the symbol lies */
> + ret = fdt_subnode_offset_namelen(fdto, 0, frag_name,
> + frag_name_len);
> + /* not found? */
> + if (ret < 0)
> + return ret;
> + fragment = ret;
> +
> + /* an __overlay__ subnode must exist */
> + ret = fdt_subnode_offset(fdto, fragment, "__overlay__");
> + if (ret < 0)
> + return ret;
> +
> + /* get the target of the fragment */
> + ret = overlay_get_target(fdt, fdto, fragment, &target_path);
> + if (ret < 0)
> + return ret;
> + target = ret;
> +
> + /* if we have a target path use */
> + if (!target_path) {
> + ret = fdt_get_path_len(fdt, target);
Given the mess of trying to resolve the path for a phandle target,
using this is reasonable. However, I think it should be a local
function, not exported to discourage use of a really inefficient
approach.
> + if (ret < 0)
> + return ret;
> + len = ret;
> + } else
> + len = strlen(target_path);
> +
> + ret = fdt_setprop_placeholder(fdt, root_sym, name,
> + len + (len > 1) + rel_path_len + 1, &p);
> + if (ret < 0)
> + return ret;
> +
> + /* again in case setprop_placeholder changed it */
> + ret = overlay_get_target(fdt, fdto, fragment, &target_path);
> + if (ret < 0)
> + return ret;
> + target = ret;
> +
> + buf = p;
> + if (len > 1) { /* target is not root */
> + if (!target_path) {
> + ret = fdt_get_path(fdt, target, buf, len + 1);
> + if (ret < 0)
> + return ret;
> + } else
> + memcpy(buf, target_path, len + 1);
> +
> + } else
> + len--;
> +
> + buf[len] = '/';
> + memcpy(buf + len + 1, rel_path, rel_path_len);
> + buf[len + 1 + rel_path_len] = '\0';
> + }
> +
> + return 0;
> +}
> +
> int fdt_overlay_apply(void *fdt, void *fdto)
> {
> uint32_t delta = fdt_get_max_phandle(fdt);
> @@ -654,6 +803,10 @@ int fdt_overlay_apply(void *fdt, void *fdto)
> if (ret)
> goto err;
>
> + ret = overlay_symbol_update(fdt, fdto);
> + if (ret)
> + goto err;
> +
> /*
> * The overlay has been damaged, erase its magic.
> */
--
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
