[PATCH v2 0/2] stacked overlay support

[PATCH v2 0/2] stacked overlay support

[Pantelis Antoniou]

Overlay application has a shortcoming that it is not possible
to refer to labels that were defined by a previously applied
overlay.

This patchset fixes the problem by keeping around the labels
that each overlay contains.

It is dependent on two previously submitted patches
"Introduce fdt_get_path_len() method"
"Introduce fdt_setprop_alloc() method"

Changes since V1:
* Rework to remove reliance on malloc and FDT_PATH_MAX

Pantelis Antoniou (2):
  fdt: Allow stacked overlays phandle references
  tests: Add stacked overlay tests on fdtoverlay

 .gitignore                     |   1 +
 libfdt/fdt_overlay.c           | 131 ++++++++++++++++++++++++++++++++++++++++-
 tests/run_tests.sh             |  15 +++++
 tests/stacked_overlay_bar.dts  |  13 ++++
 tests/stacked_overlay_base.dts |   6 ++
 tests/stacked_overlay_baz.dts  |  13 ++++
 6 files changed, 178 insertions(+), 1 deletion(-)
 create mode 100644 tests/stacked_overlay_bar.dts
 create mode 100644 tests/stacked_overlay_base.dts
 create mode 100644 tests/stacked_overlay_baz.dts

-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 1/2] fdt: Allow stacked overlays phandle references

[Pantelis Antoniou]

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-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
---
 .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;
+
+
+	/* 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;
+
+		/* skip autogenerated properties */
+		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;
+
+		rel_path = s + len;
+		rel_path_len = strlen(rel_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;
+
+		/* 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);
+		if (ret < 0)
+			return ret;
+		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';
+	}
+
+	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.
 	 */
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/2] tests: Add stacked overlay tests on fdtoverlay

[Pantelis Antoniou]

Add a stacked overlay unit test, piggybacking on fdtoverlay.

Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
---
 tests/run_tests.sh             | 15 +++++++++++++++
 tests/stacked_overlay_bar.dts  | 13 +++++++++++++
 tests/stacked_overlay_base.dts |  6 ++++++
 tests/stacked_overlay_baz.dts  | 13 +++++++++++++
 4 files changed, 47 insertions(+)
 create mode 100644 tests/stacked_overlay_bar.dts
 create mode 100644 tests/stacked_overlay_base.dts
 create mode 100644 tests/stacked_overlay_baz.dts

diff --git a/tests/run_tests.sh b/tests/run_tests.sh
index d20729c..cecc6d2 100755
--- a/tests/run_tests.sh
+++ b/tests/run_tests.sh
@@ -791,6 +791,21 @@ fdtoverlay_tests() {
 
     # test that the new property is installed
     run_fdtoverlay_test foobar "/test-node" "test-str-property" "-ts" ${basedtb} ${targetdtb} ${overlaydtb}
+
+    stacked_base=stacked_overlay_base.dts
+    stacked_basedtb=stacked_overlay_base.fdtoverlay.test.dtb
+    stacked_bar=stacked_overlay_bar.dts
+    stacked_bardtb=stacked_overlay_bar.fdtoverlay.test.dtb
+    stacked_baz=stacked_overlay_baz.dts
+    stacked_bazdtb=stacked_overlay_baz.fdtoverlay.test.dtb
+    stacked_targetdtb=stacked_overlay_target.fdoverlay.test.dtb
+
+    run_dtc_test -@ -I dts -O dtb -o $stacked_basedtb $stacked_base
+    run_dtc_test -@ -I dts -O dtb -o $stacked_bardtb $stacked_bar
+    run_dtc_test -@ -I dts -O dtb -o $stacked_bazdtb $stacked_baz
+
+    # test that baz correctly inserted the property
+    run_fdtoverlay_test baz "/foonode/barnode/baznode" "baz-property" "-ts" ${stacked_basedtb} ${stacked_targetdtb} ${stacked_bardtb} ${stacked_bazdtb}
 }
 
 pylibfdt_tests () {
diff --git a/tests/stacked_overlay_bar.dts b/tests/stacked_overlay_bar.dts
new file mode 100644
index 0000000..c646399
--- /dev/null
+++ b/tests/stacked_overlay_bar.dts
@@ -0,0 +1,13 @@
+/dts-v1/;
+/plugin/;
+/ {
+	fragment@1 {
+		target = <&foo>;
+		__overlay__ {
+			overlay-1-property;
+			bar: barnode {
+				bar-property = "bar";
+			};
+		};
+	};
+};
diff --git a/tests/stacked_overlay_base.dts b/tests/stacked_overlay_base.dts
new file mode 100644
index 0000000..2916423
--- /dev/null
+++ b/tests/stacked_overlay_base.dts
@@ -0,0 +1,6 @@
+/dts-v1/;
+/ {
+	foo: foonode {
+		foo-property = "foo";
+	};
+};
diff --git a/tests/stacked_overlay_baz.dts b/tests/stacked_overlay_baz.dts
new file mode 100644
index 0000000..a52f0cc
--- /dev/null
+++ b/tests/stacked_overlay_baz.dts
@@ -0,0 +1,13 @@
+/dts-v1/;
+/plugin/;
+/ {
+	fragment@1 {
+		target = <&bar>;
+		__overlay__ {
+			overlay-2-property;
+			baz: baznode {
+				baz-property = "baz";
+			};
+		};
+	};
+};
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/2] fdt: Allow stacked overlays phandle references

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 8620 bytes --]

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-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> ---
>  .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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/2] fdt: Allow stacked overlays phandle references

[Pantelis Antoniou]

Hi David,

On Fri, 2017-07-14 at 20:22 +1000, David Gibson wrote:
> 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-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> > ---
> >  .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.
> 

OK, although in practice there is almost no chance that a valid tree
blob will contain no symbols.

> > +
> > +	/* 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.
> 

The comment is valid; there are auto generated properties and name is
one of them. Overlays should work even on 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.
> 

OK, but could only happen on really badly formatted (even maliciously
so) properties.
 
> > +
> > +		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.
> 

OK

> > +
> > +		/* 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().
> 

OK

> > +
> > +		/* 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.
> 

OK

> > +
> > +		/* 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.
> 

OK

> > +		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.
> 

OK

> > +	}
> > +
> > +	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.
> >  	 */
> 

Regards

-- Pantelis

Re: [PATCH v2 1/2] fdt: Allow stacked overlays phandle references

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 8242 bytes --]

On Fri, Jul 14, 2017 at 02:56:37PM +0300, Pantelis Antoniou wrote:
> Hi David,
> 
> On Fri, 2017-07-14 at 20:22 +1000, David Gibson wrote:
> > 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-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> > > ---
> > >  .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.
> > 
> 
> OK, although in practice there is almost no chance that a valid tree
> blob will contain no symbols.

Sure, but it's pretty easy to get right, even for that rare case.

> > > +
> > > +	/* 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.
> 
> The comment is valid; there are auto generated properties and name is
> one of them. Overlays should work even on really old dtb versions.

No, it's really not.  Usually *all* the properites in this node will
be autogenerated, and you're certainly not skipping them all.

And no, overlays really don't need to work with dtbs *that* old.  In
dtc, "name" properties are only generated for v3 and earlier dtbs.  v3
was obsolete for years before overlays were invented.  In fact v3 was
obsolete even before libfdt was created - libfdt won't work with them
at all (see FDT_FIRST_SUPPORTED_VERSION).

It's possible some other tool is adding "name" properties, but in that
case we should reference that tool as the reason, not make a comment
which is both vague and factually inaccurate.

> 
> > > +		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.
> > 
> 
> OK, but could only happen on really badly formatted (even maliciously
> so) properties.

Failing gracefully on corrupted or maliciously constructed blobs is a
design goal for libfdt.

-- 
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 --]

Re: [PATCH v2 2/2] tests: Add stacked overlay tests on fdtoverlay

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3280 bytes --]

On Thu, Jul 13, 2017 at 12:24:19AM +0300, Pantelis Antoniou wrote:
> Add a stacked overlay unit test, piggybacking on fdtoverlay.
> 
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>

Reviewed-by: David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>

> ---
>  tests/run_tests.sh             | 15 +++++++++++++++
>  tests/stacked_overlay_bar.dts  | 13 +++++++++++++
>  tests/stacked_overlay_base.dts |  6 ++++++
>  tests/stacked_overlay_baz.dts  | 13 +++++++++++++
>  4 files changed, 47 insertions(+)
>  create mode 100644 tests/stacked_overlay_bar.dts
>  create mode 100644 tests/stacked_overlay_base.dts
>  create mode 100644 tests/stacked_overlay_baz.dts
> 
> diff --git a/tests/run_tests.sh b/tests/run_tests.sh
> index d20729c..cecc6d2 100755
> --- a/tests/run_tests.sh
> +++ b/tests/run_tests.sh
> @@ -791,6 +791,21 @@ fdtoverlay_tests() {
>  
>      # test that the new property is installed
>      run_fdtoverlay_test foobar "/test-node" "test-str-property" "-ts" ${basedtb} ${targetdtb} ${overlaydtb}
> +
> +    stacked_base=stacked_overlay_base.dts
> +    stacked_basedtb=stacked_overlay_base.fdtoverlay.test.dtb
> +    stacked_bar=stacked_overlay_bar.dts
> +    stacked_bardtb=stacked_overlay_bar.fdtoverlay.test.dtb
> +    stacked_baz=stacked_overlay_baz.dts
> +    stacked_bazdtb=stacked_overlay_baz.fdtoverlay.test.dtb
> +    stacked_targetdtb=stacked_overlay_target.fdoverlay.test.dtb
> +
> +    run_dtc_test -@ -I dts -O dtb -o $stacked_basedtb $stacked_base
> +    run_dtc_test -@ -I dts -O dtb -o $stacked_bardtb $stacked_bar
> +    run_dtc_test -@ -I dts -O dtb -o $stacked_bazdtb $stacked_baz
> +
> +    # test that baz correctly inserted the property
> +    run_fdtoverlay_test baz "/foonode/barnode/baznode" "baz-property" "-ts" ${stacked_basedtb} ${stacked_targetdtb} ${stacked_bardtb} ${stacked_bazdtb}
>  }
>  
>  pylibfdt_tests () {
> diff --git a/tests/stacked_overlay_bar.dts b/tests/stacked_overlay_bar.dts
> new file mode 100644
> index 0000000..c646399
> --- /dev/null
> +++ b/tests/stacked_overlay_bar.dts
> @@ -0,0 +1,13 @@
> +/dts-v1/;
> +/plugin/;
> +/ {
> +	fragment@1 {
> +		target = <&foo>;
> +		__overlay__ {
> +			overlay-1-property;
> +			bar: barnode {
> +				bar-property = "bar";
> +			};
> +		};
> +	};
> +};
> diff --git a/tests/stacked_overlay_base.dts b/tests/stacked_overlay_base.dts
> new file mode 100644
> index 0000000..2916423
> --- /dev/null
> +++ b/tests/stacked_overlay_base.dts
> @@ -0,0 +1,6 @@
> +/dts-v1/;
> +/ {
> +	foo: foonode {
> +		foo-property = "foo";
> +	};
> +};
> diff --git a/tests/stacked_overlay_baz.dts b/tests/stacked_overlay_baz.dts
> new file mode 100644
> index 0000000..a52f0cc
> --- /dev/null
> +++ b/tests/stacked_overlay_baz.dts
> @@ -0,0 +1,13 @@
> +/dts-v1/;
> +/plugin/;
> +/ {
> +	fragment@1 {
> +		target = <&bar>;
> +		__overlay__ {
> +			overlay-2-property;
> +			baz: baznode {
> +				baz-property = "baz";
> +			};
> +		};
> +	};
> +};

-- 
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 --]

Re: [PATCH v2 1/2] fdt: Allow stacked overlays phandle references

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4961 bytes --]

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.

[snip]
> +/**
> + * 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;
> +
> +
> +	/* 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;
> +
> +		/* skip autogenerated properties */
> +		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;
> +
> +		rel_path = s + len;
> +		rel_path_len = strlen(rel_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;
> +
> +		/* 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);
> +		if (ret < 0)
> +			return ret;
> +		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';
> +	}
> +
> +	return 0;
> +}

So, as noted in the other thread, even by the standards of the slow
functions in libfdt, fdt_get_path_len() is really slow as proposed.
So the question is can we avoid using fdt_get_path() here.

When the target is designated with target-path, then yes we can:
rather than going via the actual target node, we can just take the
path directly from the property.

For targets designated by phandle it's.. fiddly.  AFAICT the path will
already be found in the existing blobs: specifically it will be in the
__symbols__ of the base tree for the label which the target is being
applied to.  Getting there is kind of awkward though: we'd have to
search __fixups__ in the overlay for the entry applying to the
relevant target property, then follow that to find the right
__symbols__ entry.

I'm not sure at this point whether that's preferable to the slow
fdt_get_path_len().

-- 
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 --]