On Thu, Jul 13, 2017 at 12:24:18AM +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.
>
> 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>
> ---
> .gitignore | 1 +
> libfdt/fdt_overlay.c | 131 ++++++++++++++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 131 insertions(+), 1 deletion(-)
>
> diff --git a/.gitignore b/.gitignore
> index 545b899..f333c28 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -15,3 +15,4 @@ lex.yy.c
> /fdtput
> /patches
> /.pc
> +/fdtoverlay
> diff --git a/libfdt/fdt_overlay.c b/libfdt/fdt_overlay.c
> index ceb9687..fb17ef2 100644
> --- a/libfdt/fdt_overlay.c
> +++ b/libfdt/fdt_overlay.c
> @@ -590,7 +590,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.
> *
> @@ -630,6 +630,131 @@ 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;
> + char *buf;
> + void *p;
> +
> + root_sym = fdt_subnode_offset(fdt, 0, "__symbols__");
> + ov_sym = fdt_subnode_offset(fdto, 0, "__symbols__");
> +
> + /* if neither exist we can't update symbols, but that's OK */
> + if (root_sym < 0 || ov_sym < 0)
> + return 0;
This isn't correct. If ov_sym isn't there, then indeed there is
nothing to do, but if root_sym is not there, you should create it.
> +
> + /* iterate over each overlay symbol */
> + fdt_for_each_property_offset(prop, fdto, ov_sym) {
> +
Stray blank line.
> + path = fdt_getprop_by_offset(fdto, prop, &name, &path_len);
> + if (!path)
> + return path_len;
> +
> + /* skip autogenerated properties */
Comment isn't correct. *Everything* in __symbols__ will typically be
autogenerated. 'name' probably is a special case, but I'm a bit
surprised it's there anyway, I though we only generated that for
really old dtb versions.
> + if (!strcmp(name, "name"))
> + continue;
> +
> + /* format: /<fragment-name>/__overlay__/<relative-subnode-path> */
> +
> + if (*path != '/')
> + return -FDT_ERR_BADVALUE;
> +
> + /* get fragment name first */
> + s = strchr(path + 1, '/');
> + if (!s)
> + return -FDT_ERR_BADVALUE;
> +
> + frag_name = path + 1;
> + frag_name_len = s - path - 1;
> +
> + /* verify format */
> + len = strlen("/__overlay__/");
> + if (strncmp(s, "/__overlay__/", len))
> + return -FDT_ERR_NOTFOUND;
This isn't correct, you're assuming the property is nul terminated
(which it should be, but you can't count on). Instead you should
compare path_len versus the length of /__overlay__/, then you can just
memcmp() to see if the right thing is there.
> +
> + rel_path = s + len;
> + rel_path_len = strlen(rel_path);
Again, this should be determined from path_len, not using strlen.
That said, it might be worth doing a memchr() to make sure there
aren't stray \0s in the path.
> +
> + /* find the fragment index in which the symbol lies */
> + fdt_for_each_subnode(fragment, fdto, 0) {
> +
> + s = fdt_get_name(fdto, fragment, &len);
> + if (!s)
> + continue;
> +
> + /* name must match */
> + if (len == frag_name_len && !memcmp(s, frag_name, len))
> + break;
> + }
> +
> + /* not found? */
> + if (fragment < 0)
> + return -FDT_ERR_NOTFOUND;
The entire loop above can be replaced with an
fdt_subnode_offset_namelen().
> +
> + /* an __overlay__ subnode must exist */
> + ret = fdt_subnode_offset(fdto, fragment, "__overlay__");
> + if (ret < 0)
> + return ret;
NOTFOUND is probably not the error code you want in this context.
> +
> + /* get the target of the fragment */
> + ret = overlay_get_target(fdt, fdto, fragment);
> + if (ret < 0)
> + return ret;
An error here should probably be translated to FDT_ERR_INTERNAL, since
we've already verified the targets of each fragment can be resolved
earlier in the application process.
> + target = ret;
> +
> + len = fdt_get_path_len(fdt, target);
> +
> + ret = fdt_setprop_alloc(fdt, root_sym, name,
> + len + (len > 1) + rel_path_len + 1, &p);
> + if (ret < 0)
> + return ret;
> +
> + /* again in case setprop_alloc changed it */
> + ret = overlay_get_target(fdt, fdto, fragment);
> + if (ret < 0)
> + return ret;
> + target = ret;
> +
> + buf = p;
> + if (len > 1) { /* target is not root */
> + ret = fdt_get_path(fdt, target, buf, len + 1);
> + if (ret < 0)
> + return ret;
> +
> + } else
> + len--;
> +
> + buf[len] = '/';
> + memcpy(buf + len + 1, rel_path, rel_path_len);
> + buf[len + 1 + rel_path_len] = '\0';
You know (or rather, you can and should verify) that the rel_path is
\0 terminated, so you can copy the \0 in the same memcpy() rather than
adding it extra.
> + }
> +
> + return 0;
> +}
> +
> int fdt_overlay_apply(void *fdt, void *fdto)
> {
> uint32_t delta = fdt_get_max_phandle(fdt);
> @@ -654,6 +779,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
