[PATCH 0/2] stacked overlay support

[PATCH 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 the previously submitted
"fdtoverlay, an overlay application tool" patchset and
"libfdt.h: Define FDT_PATH_MAX" patch.

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

 .gitignore                     |   1 +
 libfdt/fdt_overlay.c           | 148 ++++++++++++++++++++++++++++++++++++++++-
 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, 195 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

[PATCH 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 | 148 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 148 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..59fd7f3 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,148 @@ 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 = NULL;
+
+	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;
+
+	buf = malloc(FDT_PATH_MAX);
+	if (!buf)
+		return -FDT_ERR_NOSPACE;
+
+	/* 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) {
+			ret = path_len;
+			goto out;
+		}
+
+		/* skip autogenerated properties */
+		if (!strcmp(name, "name"))
+			continue;
+
+		/* format: /<fragment-name>/__overlay__/<relative-subnode-path> */
+
+		if (*path != '/') {
+			ret = -FDT_ERR_BADVALUE;
+			goto out;
+		}
+
+		/* get frag name first */
+		s = strchr(path + 1, '/');
+		if (!s) {
+			ret = -FDT_ERR_BADVALUE;
+			goto out;
+		}
+		frag_name = path + 1;
+		frag_name_len = s - path - 1;
+
+		/* verify format */
+		len = strlen("/__overlay__/");
+		if (strncmp(s, "/__overlay__/", len)) {
+			ret = -FDT_ERR_NOTFOUND;
+			goto out;
+		}
+
+		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) {
+			ret = -FDT_ERR_NOTFOUND;
+			goto out;
+		}
+
+		/* an __overlay__ subnode must exist */
+		ret = fdt_subnode_offset(fdto, fragment, "__overlay__");
+		if (ret < 0)
+			goto out;
+
+		/* get the target of the fragment */
+		ret = overlay_get_target(fdt, fdto, fragment);
+		if (ret < 0)
+			goto out;
+		target = ret;
+
+		/* get the path of the target */
+		ret = fdt_get_path(fdt, target, buf, FDT_PATH_MAX);
+		if (ret < 0)
+			goto out;
+
+		len = strlen(buf);
+
+		/* if the target is root strip leading / */
+		if (len == 1 && buf[0] == '/')
+			len = 0;
+
+		/* make sure we have enough space */
+		if (len + 1 + rel_path_len + 1 > FDT_PATH_MAX) {
+			ret = -FDT_ERR_NOSPACE;
+			goto out;
+		}
+
+		/* create new symbol path in place */
+		buf[len] = '/';
+		memcpy(buf + len + 1, rel_path, rel_path_len);
+		buf[len + 1 + rel_path_len] = '\0';
+
+		ret = fdt_setprop_string(fdt, root_sym, name, buf);
+		if (ret < 0)
+			goto out;
+	}
+
+	ret = 0;
+
+out:
+	if (buf)
+		free(buf);
+
+	return ret;
+}
+
 int fdt_overlay_apply(void *fdt, void *fdto)
 {
 	uint32_t delta = fdt_get_max_phandle(fdt);
@@ -654,6 +796,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

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

[David Gibson]

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

On Wed, Jun 14, 2017 at 05:52:25PM +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.

This seems to be doing things the hard way.

You're essentially extending the semantics of overlay application to
add the symbol merging.  You've implemented these extended semantics
in libfdt, which is all very well, but that's not the only overlay
application implementation.


It seems to me a better approach would be to change dtc's -@
implementation, so that in /plugin/ mode instead of making a global
__symbols__ node, it puts it into the individual fragments.  That way
the existing overlay application semantics will update the __symbols__
node.

-- 
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: 819 bytes --]

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

[Pantelis Antoniou]

Hi David,

On Mon, 2017-07-03 at 19:06 +1000, David Gibson wrote:
> On Wed, Jun 14, 2017 at 05:52:25PM +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.
> 
> This seems to be doing things the hard way.
> 

It is the minimal implementation to get things to work, with the current
overlay implementation. I do have plans for a version 2 with fixes to
a number of areas.
 
> You're essentially extending the semantics of overlay application to
> add the symbol merging.  You've implemented these extended semantics
> in libfdt, which is all very well, but that's not the only overlay
> application implementation.
> 
> 

This is a port of the same patch that's against the linux kernel.
As far as I know there's no other implementations, or at least none
that are open source.

> It seems to me a better approach would be to change dtc's -@
> implementation, so that in /plugin/ mode instead of making a global
> __symbols__ node, it puts it into the individual fragments.  That way
> the existing overlay application semantics will update the __symbols__
> node.
> 

A lot of things can be made better, on the next version. These are
minimally intrusive patches to address user requests for the current
implementation.

Why don't we start by making a list, and work towards that goal?

Care to start about what you want addressed and how?

Regards

-- Pantelis

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

[David Gibson]

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

On Mon, Jul 03, 2017 at 03:41:14PM +0300, Pantelis Antoniou wrote:
> Hi David,
> 
> On Mon, 2017-07-03 at 19:06 +1000, David Gibson wrote:
> > On Wed, Jun 14, 2017 at 05:52:25PM +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.
> > 
> > This seems to be doing things the hard way.
> > 
> 
> It is the minimal implementation to get things to work, with the current
> overlay implementation.

Is it, though?  I'd expect reworking the symbol creation during
compile to be of similar complexity to the symbol merging here.  And
it only needs to be done in one place, not two.  And it doesn't
implicitly extend the overlay spec.

> I do have plans for a version 2 with fixes to
> a number of areas.

Saying you'll fix it in v2 is missing the point.  If v1 is out there,
we have to keep supporting it.  The number of half-arsed overlay
variants out in the wild just seems to keep growing.

> > You're essentially extending the semantics of overlay application to
> > add the symbol merging.  You've implemented these extended semantics
> > in libfdt, which is all very well, but that's not the only overlay
> > application implementation.
> 
> This is a port of the same patch that's against the linux kernel.
> As far as I know there's no other implementations, or at least none
> that are open source.

So, it's already in the wild and we have to deal with it.  Yay.

The whole history of DT overlays has been this - hacking something up
to grab some desired feature with a complete lack of forethought about
what the long term, or even medium term, consequences will be.  It's
kind of pissing me off.

That's exactly why it took so long to get the overlay patches merged
in the first place.  I was hoping to encourage a bit more thinking
*before* putting an approach in the wild that would predictably cause
us trouble later on.  Didn't work, alas.

> > It seems to me a better approach would be to change dtc's -@
> > implementation, so that in /plugin/ mode instead of making a global
> > __symbols__ node, it puts it into the individual fragments.  That way
> > the existing overlay application semantics will update the __symbols__
> > node.
> 
> A lot of things can be made better, on the next version. These are
> minimally intrusive patches to address user requests for the current
> implementation.

Except that a) I'm not really convinced of that and b) I don't see any
signs of really trying to approach this methodically, rather than just
moving from one hack to the next.

> Why don't we start by making a list, and work towards that goal?
> 
> Care to start about what you want addressed and how?

The biggest thing is a question of design culture, not any specific
properties.  Think in terms of specification, rather than just
implementation, and make at least a minimal effort to ensure that that
specification is both sufficient and minimal for the requirements at
hand.  Overlays as they stand are a long way from either.

-- 
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: 819 bytes --]

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

[Tom Rini]

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

On Fri, Jul 07, 2017 at 05:09:15PM +1000, David Gibson wrote:
> On Mon, Jul 03, 2017 at 03:41:14PM +0300, Pantelis Antoniou wrote:
> > Hi David,
> > 
> > On Mon, 2017-07-03 at 19:06 +1000, David Gibson wrote:
> > > On Wed, Jun 14, 2017 at 05:52:25PM +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.
> > > 
> > > This seems to be doing things the hard way.
> > > 
> > 
> > It is the minimal implementation to get things to work, with the current
> > overlay implementation.
> 
> Is it, though?  I'd expect reworking the symbol creation during
> compile to be of similar complexity to the symbol merging here.  And
> it only needs to be done in one place, not two.  And it doesn't
> implicitly extend the overlay spec.
> 
> > I do have plans for a version 2 with fixes to
> > a number of areas.
> 
> Saying you'll fix it in v2 is missing the point.  If v1 is out there,
> we have to keep supporting it.  The number of half-arsed overlay
> variants out in the wild just seems to keep growing.

Erm, v1 is out there, because the patches are posted here in public,
where reviews happen.  If some group picks them up and runs with them,
perhaps it's worth asking why some group would be willing to pick up and
run with v1 of a series?  Which, AFAIK, hasn't actually happened just
yet..

> > > You're essentially extending the semantics of overlay application to
> > > add the symbol merging.  You've implemented these extended semantics
> > > in libfdt, which is all very well, but that's not the only overlay
> > > application implementation.
> > 
> > This is a port of the same patch that's against the linux kernel.
> > As far as I know there's no other implementations, or at least none
> > that are open source.
> 
> So, it's already in the wild and we have to deal with it.  Yay.
> 
> The whole history of DT overlays has been this - hacking something up
> to grab some desired feature with a complete lack of forethought about
> what the long term, or even medium term, consequences will be.  It's
> kind of pissing me off.
> 
> That's exactly why it took so long to get the overlay patches merged
> in the first place.  I was hoping to encourage a bit more thinking
> *before* putting an approach in the wild that would predictably cause
> us trouble later on.  Didn't work, alas.

So there's literally no one that thinks the way the first part over
overlay support happened was a good example?  Good.  Shall we try and
fix that moving forward?  Isn't the way that open source works is that
people develop, in the open and code evolves based on feedback from
other developers and real world uses?

> > > It seems to me a better approach would be to change dtc's -@
> > > implementation, so that in /plugin/ mode instead of making a global
> > > __symbols__ node, it puts it into the individual fragments.  That way
> > > the existing overlay application semantics will update the __symbols__
> > > node.
> > 
> > A lot of things can be made better, on the next version. These are
> > minimally intrusive patches to address user requests for the current
> > implementation.
> 
> Except that a) I'm not really convinced of that and b) I don't see any
> signs of really trying to approach this methodically, rather than just
> moving from one hack to the next.

So to be clear for (a), you're saying the change to dtc's -@
implementation would be less invasive?

> > Why don't we start by making a list, and work towards that goal?
> > 
> > Care to start about what you want addressed and how?
> 
> The biggest thing is a question of design culture, not any specific
> properties.  Think in terms of specification, rather than just
> implementation, and make at least a minimal effort to ensure that that
> specification is both sufficient and minimal for the requirements at
> hand.  Overlays as they stand are a long way from either.

So you wish that someone had written some "overlay doc" before posting
patches?  Documentation should come with code, not before code, as we
already (right?) have general understanding that things like overlays
need to exist and need to be useful because there's real use cases out
there.

-- 
Tom

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Frank Rowand]

On 07/07/17 07:01, Tom Rini wrote:
> On Fri, Jul 07, 2017 at 05:09:15PM +1000, David Gibson wrote:
>> On Mon, Jul 03, 2017 at 03:41:14PM +0300, Pantelis Antoniou wrote:
>>> Hi David,
>>>
>>> On Mon, 2017-07-03 at 19:06 +1000, David Gibson wrote:
>>>> On Wed, Jun 14, 2017 at 05:52:25PM +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.
>>>>
>>>> This seems to be doing things the hard way.
>>>>
>>>
>>> It is the minimal implementation to get things to work, with the current
>>> overlay implementation.
>>
>> Is it, though?  I'd expect reworking the symbol creation during
>> compile to be of similar complexity to the symbol merging here.  And
>> it only needs to be done in one place, not two.  And it doesn't
>> implicitly extend the overlay spec.
>>
>>> I do have plans for a version 2 with fixes to
>>> a number of areas.
>>
>> Saying you'll fix it in v2 is missing the point.  If v1 is out there,
>> we have to keep supporting it.  The number of half-arsed overlay
>> variants out in the wild just seems to keep growing.
> 
> Erm, v1 is out there, because the patches are posted here in public,
> where reviews happen.  If some group picks them up and runs with them,
> perhaps it's worth asking why some group would be willing to pick up and
> run with v1 of a series?  Which, AFAIK, hasn't actually happened just
> yet..

I think I'm responding to a conversation where there is some talking
past each other, and my added observation is not directly to the point
of that conversation, but instead to some of what seems to me to be
implied by several previous posts.  So please forgive my off topic
drift.

As I have noted before, just because some Linux code is in the wild
or in widespread use does _not_ mean that it has placed any
constraints on what will be accepted into the Linux kernel.  That
is not the way the kernel development process works; for example,
just because a feature was implemented in a distro does not mean
that it will be accepted into the mainline kernel, or if it is
accepted into the mainline kernel that it will be the same
implementation when it gets into the mainline kernel.  All of the
major distros have the mantra of "Upstream First", which avoids that
issue.

< snip >

-Frank
--
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 1/2] fdt: Allow stacked overlays phandle references

[Frank Rowand]

On 07/07/17 00:09, David Gibson wrote:
> On Mon, Jul 03, 2017 at 03:41:14PM +0300, Pantelis Antoniou wrote:
>> Hi David,
>>
>> On Mon, 2017-07-03 at 19:06 +1000, David Gibson wrote:
>>> On Wed, Jun 14, 2017 at 05:52:25PM +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.
>>>
>>> This seems to be doing things the hard way.
>>>
>>
>> It is the minimal implementation to get things to work, with the current
>> overlay implementation.
> 
> Is it, though?  I'd expect reworking the symbol creation during
> compile to be of similar complexity to the symbol merging here.  And
> it only needs to be done in one place, not two.  And it doesn't
> implicitly extend the overlay spec.
> 
>> I do have plans for a version 2 with fixes to
>> a number of areas.
> 
> Saying you'll fix it in v2 is missing the point.  If v1 is out there,
> we have to keep supporting it.  The number of half-arsed overlay
> variants out in the wild just seems to keep growing.
> 
>>> You're essentially extending the semantics of overlay application to
>>> add the symbol merging.  You've implemented these extended semantics
>>> in libfdt, which is all very well, but that's not the only overlay
>>> application implementation.
>>
>> This is a port of the same patch that's against the linux kernel.
>> As far as I know there's no other implementations, or at least none
>> that are open source.
> 
> So, it's already in the wild and we have to deal with it.  Yay.

It was only a proposed patch.  It is not in the kernel.  We don't
have to deal with it.

-Frank


> The whole history of DT overlays has been this - hacking something up
> to grab some desired feature with a complete lack of forethought about
> what the long term, or even medium term, consequences will be.  It's
> kind of pissing me off.
> 
> That's exactly why it took so long to get the overlay patches merged
> in the first place.  I was hoping to encourage a bit more thinking
> *before* putting an approach in the wild that would predictably cause
> us trouble later on.  Didn't work, alas.
> 
>>> It seems to me a better approach would be to change dtc's -@
>>> implementation, so that in /plugin/ mode instead of making a global
>>> __symbols__ node, it puts it into the individual fragments.  That way
>>> the existing overlay application semantics will update the __symbols__
>>> node.
>>
>> A lot of things can be made better, on the next version. These are
>> minimally intrusive patches to address user requests for the current
>> implementation.
> 
> Except that a) I'm not really convinced of that and b) I don't see any
> signs of really trying to approach this methodically, rather than just
> moving from one hack to the next.
> 
>> Why don't we start by making a list, and work towards that goal?
>>
>> Care to start about what you want addressed and how?
> 
> The biggest thing is a question of design culture, not any specific
> properties.  Think in terms of specification, rather than just
> implementation, and make at least a minimal effort to ensure that that
> specification is both sufficient and minimal for the requirements at
> hand.  Overlays as they stand are a long way from either.
> 

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

[David Gibson]

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

On Thu, Jul 13, 2017 at 12:40:24PM -0700, Frank Rowand wrote:
> On 07/07/17 00:09, David Gibson wrote:
> > On Mon, Jul 03, 2017 at 03:41:14PM +0300, Pantelis Antoniou wrote:
> >> Hi David,
> >>
> >> On Mon, 2017-07-03 at 19:06 +1000, David Gibson wrote:
> >>> On Wed, Jun 14, 2017 at 05:52:25PM +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.
> >>>
> >>> This seems to be doing things the hard way.
> >>>
> >>
> >> It is the minimal implementation to get things to work, with the current
> >> overlay implementation.
> > 
> > Is it, though?  I'd expect reworking the symbol creation during
> > compile to be of similar complexity to the symbol merging here.  And
> > it only needs to be done in one place, not two.  And it doesn't
> > implicitly extend the overlay spec.
> > 
> >> I do have plans for a version 2 with fixes to
> >> a number of areas.
> > 
> > Saying you'll fix it in v2 is missing the point.  If v1 is out there,
> > we have to keep supporting it.  The number of half-arsed overlay
> > variants out in the wild just seems to keep growing.
> > 
> >>> You're essentially extending the semantics of overlay application to
> >>> add the symbol merging.  You've implemented these extended semantics
> >>> in libfdt, which is all very well, but that's not the only overlay
> >>> application implementation.
> >>
> >> This is a port of the same patch that's against the linux kernel.
> >> As far as I know there's no other implementations, or at least none
> >> that are open source.
> > 
> > So, it's already in the wild and we have to deal with it.  Yay.
> 
> It was only a proposed patch.  It is not in the kernel.  We don't
> have to deal with it.

Ah, I misread.  Well, that's something.

-- 
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 1/2] fdt: Allow stacked overlays phandle references

[Frank Rowand]

On 07/03/17 05:41, Pantelis Antoniou wrote:
> Hi David,
> 
> On Mon, 2017-07-03 at 19:06 +1000, David Gibson wrote:
>> On Wed, Jun 14, 2017 at 05:52:25PM +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.
>>
>> This seems to be doing things the hard way.
>>
> 
> It is the minimal implementation to get things to work, with the current
> overlay implementation. I do have plans for a version 2 with fixes to
> a number of areas.
>  
>> You're essentially extending the semantics of overlay application to
>> add the symbol merging.  You've implemented these extended semantics
>> in libfdt, which is all very well, but that's not the only overlay
>> application implementation.
>>
>>
> 
> This is a port of the same patch that's against the linux kernel.
> As far as I know there's no other implementations, or at least none
> that are open source.

That patch never made it into the kernel.  I submitted a different
patch last week (now at v2, probably soon to be v3, then probably
v4 when 4.13-rc1 comes out), so hopefully we will get the overlay
symbol loading support into the kernel soon.


>> It seems to me a better approach would be to change dtc's -@
>> implementation, so that in /plugin/ mode instead of making a global
>> __symbols__ node, it puts it into the individual fragments.  That way
>> the existing overlay application semantics will update the __symbols__
>> node.
>>
> 
> A lot of things can be made better, on the next version. These are
> minimally intrusive patches to address user requests for the current
> implementation.
> 
> Why don't we start by making a list, and work towards that goal?
> 
> Care to start about what you want addressed and how?
> 
> Regards
> 
> -- Pantelis
> 
> 

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

[Frank Rowand]

On 07/03/17 02:06, David Gibson wrote:
> On Wed, Jun 14, 2017 at 05:52:25PM +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.
> 
> This seems to be doing things the hard way.
> 
> You're essentially extending the semantics of overlay application to
> add the symbol merging.  You've implemented these extended semantics
> in libfdt, which is all very well, but that's not the only overlay
> application implementation.
> 
> 
> It seems to me a better approach would be to change dtc's -@
> implementation, so that in /plugin/ mode instead of making a global
> __symbols__ node, it puts it into the individual fragments.  That way
> the existing overlay application semantics will update the __symbols__
> node.

If the __symbols__ node was inside a fragment, then the existing
code would add (or update) a __symbols__ node located at the location
pointed to by the fragment's target path, instead of updating the
node /__symbols__.

It makes sense to me to have only one global __symbols__ node instead
of several.

If there is a global __symbols__ node then we have a single name
space for symbols.

If there are multiple __symbols__ nodes spread throughout the tree,
then to me that would imply different name spaces spread throughout
the tree, where namespaces are determined by fragments.  This sounds
confusing to me.  Or if the intent is to have a single name space
then the __symbols__ information would be scattered throughout the
tree instead of located in a single node.

My current patch (under review), targeted for Linux 4.13-rc1, puts
an overlay's __symbols__ node properties into the overlay's
changeset, so they get added when the overlay is loaded and
removed when the overlay is unloaded.

-Frank

--
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 1/2] fdt: Allow stacked overlays phandle references

[Phil Elwell]

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

Can we also consider a mechanism for overlay-local symbols, i.e. symbols
that are used purely to create links within an overlay - perhaps using a
particular naming convention? This would make it easier to instantiate an
overlay multiple times without having to uniquify all symbols, and it would
avoid polluting the global namespace without reason.

Phil

On 13 Jul 2017 8:32 pm, "Frank Rowand" <frowand.list@gmail.com> wrote:

> On 07/03/17 02:06, David Gibson wrote:
> > On Wed, Jun 14, 2017 at 05:52:25PM +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.
> >
> > This seems to be doing things the hard way.
> >
> > You're essentially extending the semantics of overlay application to
> > add the symbol merging.  You've implemented these extended semantics
> > in libfdt, which is all very well, but that's not the only overlay
> > application implementation.
> >
> >
> > It seems to me a better approach would be to change dtc's -@
> > implementation, so that in /plugin/ mode instead of making a global
> > __symbols__ node, it puts it into the individual fragments.  That way
> > the existing overlay application semantics will update the __symbols__
> > node.
>
> If the __symbols__ node was inside a fragment, then the existing
> code would add (or update) a __symbols__ node located at the location
> pointed to by the fragment's target path, instead of updating the
> node /__symbols__.
>
> It makes sense to me to have only one global __symbols__ node instead
> of several.
>
> If there is a global __symbols__ node then we have a single name
> space for symbols.
>
> If there are multiple __symbols__ nodes spread throughout the tree,
> then to me that would imply different name spaces spread throughout
> the tree, where namespaces are determined by fragments.  This sounds
> confusing to me.  Or if the intent is to have a single name space
> then the __symbols__ information would be scattered throughout the
> tree instead of located in a single node.
>
> My current patch (under review), targeted for Linux 4.13-rc1, puts
> an overlay's __symbols__ node properties into the overlay's
> changeset, so they get added when the overlay is loaded and
> removed when the overlay is unloaded.
>
> -Frank
>
> --
> To unsubscribe from this list: send the line "unsubscribe
> devicetree-compiler" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

[-- Attachment #2: Type: text/html, Size: 3296 bytes --]

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

[Frank Rowand]

On 07/13/17 12:38, Phil Elwell wrote:

(I moved Phil's reply to after the email he replied to.)

> On 13 Jul 2017 8:32 pm, "Frank Rowand" <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 
>> On 07/03/17 02:06, David Gibson wrote:
>>> On Wed, Jun 14, 2017 at 05:52:25PM +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.
>>>
>>> This seems to be doing things the hard way.
>>>
>>> You're essentially extending the semantics of overlay application to
>>> add the symbol merging.  You've implemented these extended semantics
>>> in libfdt, which is all very well, but that's not the only overlay
>>> application implementation.
>>>
>>>
>>> It seems to me a better approach would be to change dtc's -@
>>> implementation, so that in /plugin/ mode instead of making a global
>>> __symbols__ node, it puts it into the individual fragments.  That way
>>> the existing overlay application semantics will update the __symbols__
>>> node.
>>
>> If the __symbols__ node was inside a fragment, then the existing
>> code would add (or update) a __symbols__ node located at the location
>> pointed to by the fragment's target path, instead of updating the
>> node /__symbols__.
>>
>> It makes sense to me to have only one global __symbols__ node instead
>> of several.
>>
>> If there is a global __symbols__ node then we have a single name
>> space for symbols.
>>
>> If there are multiple __symbols__ nodes spread throughout the tree,
>> then to me that would imply different name spaces spread throughout
>> the tree, where namespaces are determined by fragments.  This sounds
>> confusing to me.  Or if the intent is to have a single name space
>> then the __symbols__ information would be scattered throughout the
>> tree instead of located in a single node.
>>
>> My current patch (under review), targeted for Linux 4.13-rc1, puts
>> an overlay's __symbols__ node properties into the overlay's
>> changeset, so they get added when the overlay is loaded and
>> removed when the overlay is unloaded.

> Can we also consider a mechanism for overlay-local symbols, i.e. symbols
> that are used purely to create links within an overlay - perhaps using a
> particular naming convention? This would make it easier to instantiate an
> overlay multiple times without having to uniquify all symbols, and it would
> avoid polluting the global namespace without reason.
> 
> Phil

That is essentially the result you get if you compile the overlay dts
without '-@'.  There will be no __sympls__ node created even if there
are symbols within the overlay.

This is important if the overlay is for an add-on board which might
have several instances plugged into different sockets on the base
system.

But Phil does bring up an interesting use case.  If the add-on board
("level one add-on") in turn has a socket that an additional board
("level two add-on") can be plugged into, then the level two add-on
overlay might have a need to reference a symbol from the overlay
for the level one add-on.  And since there may be multiple level one
add-on cards in the system, the overlay for each of the level one
add-ons would need to export its symbols in a name space only
visible to the level two add-on plugged into that specific level
on add-on.

-Frank
--
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 1/2] fdt: Allow stacked overlays phandle references

[Frank Rowand]

On 07/13/17 13:07, Frank Rowand wrote:
> On 07/13/17 12:38, Phil Elwell wrote:
> 
> (I moved Phil's reply to after the email he replied to.)
> 
>> On 13 Jul 2017 8:32 pm, "Frank Rowand" <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>
>>> On 07/03/17 02:06, David Gibson wrote:
>>>> On Wed, Jun 14, 2017 at 05:52:25PM +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.
>>>>
>>>> This seems to be doing things the hard way.
>>>>
>>>> You're essentially extending the semantics of overlay application to
>>>> add the symbol merging.  You've implemented these extended semantics
>>>> in libfdt, which is all very well, but that's not the only overlay
>>>> application implementation.
>>>>
>>>>
>>>> It seems to me a better approach would be to change dtc's -@
>>>> implementation, so that in /plugin/ mode instead of making a global
>>>> __symbols__ node, it puts it into the individual fragments.  That way
>>>> the existing overlay application semantics will update the __symbols__
>>>> node.
>>>
>>> If the __symbols__ node was inside a fragment, then the existing
>>> code would add (or update) a __symbols__ node located at the location
>>> pointed to by the fragment's target path, instead of updating the
>>> node /__symbols__.
>>>
>>> It makes sense to me to have only one global __symbols__ node instead
>>> of several.
>>>
>>> If there is a global __symbols__ node then we have a single name
>>> space for symbols.
>>>
>>> If there are multiple __symbols__ nodes spread throughout the tree,
>>> then to me that would imply different name spaces spread throughout
>>> the tree, where namespaces are determined by fragments.  This sounds
>>> confusing to me.  Or if the intent is to have a single name space
>>> then the __symbols__ information would be scattered throughout the
>>> tree instead of located in a single node.
>>>
>>> My current patch (under review), targeted for Linux 4.13-rc1, puts
>>> an overlay's __symbols__ node properties into the overlay's
>>> changeset, so they get added when the overlay is loaded and
>>> removed when the overlay is unloaded.
> 
>> Can we also consider a mechanism for overlay-local symbols, i.e. symbols
>> that are used purely to create links within an overlay - perhaps using a
>> particular naming convention? This would make it easier to instantiate an
>> overlay multiple times without having to uniquify all symbols, and it would
>> avoid polluting the global namespace without reason.
>>
>> Phil
> 
> That is essentially the result you get if you compile the overlay dts
> without '-@'.  There will be no __sympls__ node created even if there

                                  ^^^^^^^^^^^ __symbols__

> are symbols within the overlay.
> 
> This is important if the overlay is for an add-on board which might
> have several instances plugged into different sockets on the base
> system.
> 
> But Phil does bring up an interesting use case.  If the add-on board
> ("level one add-on") in turn has a socket that an additional board
> ("level two add-on") can be plugged into, then the level two add-on
> overlay might have a need to reference a symbol from the overlay
> for the level one add-on.  And since there may be multiple level one
> add-on cards in the system, the overlay for each of the level one
> add-ons would need to export its symbols in a name space only
> visible to the level two add-on plugged into that specific level
> on add-on.
> 
> -Frank
> 

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

[Phil Elwell]

On 13/07/2017 21:07, Frank Rowand wrote:
> On 07/13/17 12:38, Phil Elwell wrote:
>
> (I moved Phil's reply to after the email he replied to.)

Thanks.

>> On 13 Jul 2017 8:32 pm, "Frank Rowand" <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>
>>> On 07/03/17 02:06, David Gibson wrote:
>>>> On Wed, Jun 14, 2017 at 05:52:25PM +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.
>>>>
>>>> This seems to be doing things the hard way.
>>>>
>>>> You're essentially extending the semantics of overlay application to
>>>> add the symbol merging.  You've implemented these extended semantics
>>>> in libfdt, which is all very well, but that's not the only overlay
>>>> application implementation.
>>>>
>>>>
>>>> It seems to me a better approach would be to change dtc's -@
>>>> implementation, so that in /plugin/ mode instead of making a global
>>>> __symbols__ node, it puts it into the individual fragments.  That way
>>>> the existing overlay application semantics will update the __symbols__
>>>> node.
>>>
>>> If the __symbols__ node was inside a fragment, then the existing
>>> code would add (or update) a __symbols__ node located at the location
>>> pointed to by the fragment's target path, instead of updating the
>>> node /__symbols__.
>>>
>>> It makes sense to me to have only one global __symbols__ node instead
>>> of several.
>>>
>>> If there is a global __symbols__ node then we have a single name
>>> space for symbols.
>>>
>>> If there are multiple __symbols__ nodes spread throughout the tree,
>>> then to me that would imply different name spaces spread throughout
>>> the tree, where namespaces are determined by fragments.  This sounds
>>> confusing to me.  Or if the intent is to have a single name space
>>> then the __symbols__ information would be scattered throughout the
>>> tree instead of located in a single node.
>>>
>>> My current patch (under review), targeted for Linux 4.13-rc1, puts
>>> an overlay's __symbols__ node properties into the overlay's
>>> changeset, so they get added when the overlay is loaded and
>>> removed when the overlay is unloaded.
>
>> Can we also consider a mechanism for overlay-local symbols, i.e. symbols
>> that are used purely to create links within an overlay - perhaps using a
>> particular naming convention? This would make it easier to instantiate an
>> overlay multiple times without having to uniquify all symbols, and it would
>> avoid polluting the global namespace without reason.
>>
>> Phil
>
> That is essentially the result you get if you compile the overlay dts
> without '-@'.  There will be no __symbols__ node created even if there
> are symbols within the overlay.

But (unless something has changed recently) the '-@' switch controls both
symbol and fixup generation, i.e. export and import of symbols. Unless one
religiously uses 'target-path' to place fragments (thus removing the
level of abstraction provided by symbols) overlays are useless without
the ability to reference external symbols, but in my experience very few
overlays need to add symbols to the global symbol table.

> This is important if the overlay is for an add-on board which might
> have several instances plugged into different sockets on the base
> system.
>
> But Phil does bring up an interesting use case.  If the add-on board
> ("level one add-on") in turn has a socket that an additional board
> ("level two add-on") can be plugged into, then the level two add-on
> overlay might have a need to reference a symbol from the overlay
> for the level one add-on.  And since there may be multiple level one
> add-on cards in the system, the overlay for each of the level one
> add-ons would need to export its symbols in a name space only
> visible to the level two add-on plugged into that specific level
> on add-on.

That use case adds a further level of complexity. Should it come to it, I
hope an inability to solve the problem posed by this advanced usage won't
prevent a solution to a simpler problem from being accepted.

Phil

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

[Frank Rowand]

On 07/13/17 14:22, Phil Elwell wrote:
> On 13/07/2017 21:07, Frank Rowand wrote:
>> On 07/13/17 12:38, Phil Elwell wrote:
>>
>> (I moved Phil's reply to after the email he replied to.)
> 
> Thanks.
> 
>>> On 13 Jul 2017 8:32 pm, "Frank Rowand" <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>
>>>> On 07/03/17 02:06, David Gibson wrote:
>>>>> On Wed, Jun 14, 2017 at 05:52:25PM +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.
>>>>>
>>>>> This seems to be doing things the hard way.
>>>>>
>>>>> You're essentially extending the semantics of overlay application to
>>>>> add the symbol merging.  You've implemented these extended semantics
>>>>> in libfdt, which is all very well, but that's not the only overlay
>>>>> application implementation.
>>>>>
>>>>>
>>>>> It seems to me a better approach would be to change dtc's -@
>>>>> implementation, so that in /plugin/ mode instead of making a global
>>>>> __symbols__ node, it puts it into the individual fragments.  That way
>>>>> the existing overlay application semantics will update the __symbols__
>>>>> node.
>>>>
>>>> If the __symbols__ node was inside a fragment, then the existing
>>>> code would add (or update) a __symbols__ node located at the location
>>>> pointed to by the fragment's target path, instead of updating the
>>>> node /__symbols__.
>>>>
>>>> It makes sense to me to have only one global __symbols__ node instead
>>>> of several.
>>>>
>>>> If there is a global __symbols__ node then we have a single name
>>>> space for symbols.
>>>>
>>>> If there are multiple __symbols__ nodes spread throughout the tree,
>>>> then to me that would imply different name spaces spread throughout
>>>> the tree, where namespaces are determined by fragments.  This sounds
>>>> confusing to me.  Or if the intent is to have a single name space
>>>> then the __symbols__ information would be scattered throughout the
>>>> tree instead of located in a single node.
>>>>
>>>> My current patch (under review), targeted for Linux 4.13-rc1, puts
>>>> an overlay's __symbols__ node properties into the overlay's
>>>> changeset, so they get added when the overlay is loaded and
>>>> removed when the overlay is unloaded.
>>
>>> Can we also consider a mechanism for overlay-local symbols, i.e. symbols
>>> that are used purely to create links within an overlay - perhaps using a
>>> particular naming convention? This would make it easier to instantiate an
>>> overlay multiple times without having to uniquify all symbols, and it would
>>> avoid polluting the global namespace without reason.
>>>
>>> Phil
>>
>> That is essentially the result you get if you compile the overlay dts
>> without '-@'.  There will be no __symbols__ node created even if there
>> are symbols within the overlay.
> 
> But (unless something has changed recently) the '-@' switch controls both
> symbol and fixup generation, i.e. export and import of symbols. Unless one
> religiously uses 'target-path' to place fragments (thus removing the
> level of abstraction provided by symbols) overlays are useless without
> the ability to reference external symbols, but in my experience very few
> overlays need to add symbols to the global symbol table.

For the dtc compiler in Linux 4.11, the '-@' switch is only needed
to generate the __symbols__ node.  The __fixups__ and __local-fixups__
nodes are generated whether '-@' is specified or not.

The __fixups__ and __local_fixups__ are generated when '/plugin/;'
is specified in the source file.


>> This is important if the overlay is for an add-on board which might
>> have several instances plugged into different sockets on the base
>> system.
>>
>> But Phil does bring up an interesting use case.  If the add-on board
>> ("level one add-on") in turn has a socket that an additional board
>> ("level two add-on") can be plugged into, then the level two add-on
>> overlay might have a need to reference a symbol from the overlay
>> for the level one add-on.  And since there may be multiple level one
>> add-on cards in the system, the overlay for each of the level one
>> add-ons would need to export its symbols in a name space only
>> visible to the level two add-on plugged into that specific level
>> on add-on.
> 
> That use case adds a further level of complexity. Should it come to it, I
> hope an inability to solve the problem posed by this advanced usage won't
> prevent a solution to a simpler problem from being accepted.
--
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 1/2] fdt: Allow stacked overlays phandle references

[Pantelis Antoniou]

Hi Frank,

On Thu, 2017-07-13 at 14:40 -0700, Frank Rowand wrote:
> On 07/13/17 14:22, Phil Elwell wrote:
> > On 13/07/2017 21:07, Frank Rowand wrote:
> >> On 07/13/17 12:38, Phil Elwell wrote:
> >>

[snip]

> > hope an inability to solve the problem posed by this advanced usage won't
> > prevent a solution to a simpler problem from being accepted.

I have waited until people started commenting on this patchset before
replying.

I think we agree on a few things to keep the discussion moving forward.

1. Stacked overlays are useful and make overlays easier to use.

2. Changing the overlay symbols format now would be unwise.

3. A number of extensions have been put forward/requested.

3.1. There should be a method to place a symbol on a node that didn't
have one originally (due to vendor supplying broken DTB or being
generated by firmware at runtime).

3.2. Scoping symbol visibility in case of clashes. This can the ability
to put multiple path references to a single label/symbol. i.e.
foo = "/path/bar", "/path/bar/baz";
Resolving the ambiguity would require the caller to provide it's
'location' on the tree. I.e. a device under /path/bar/baz would resolve
to the latter. It is a big change semantically.

Do you have anything else?

Regards

-- Pantelis

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

[Frank Rowand]

On 07/14/17 00:21, Pantelis Antoniou wrote:

Keeping in mind that this thread was originally about libfdt,
not the Linux kernel, I am mostly talking about the Linux
kernel implementation in this email.


> Hi Frank,
> 
> On Thu, 2017-07-13 at 14:40 -0700, Frank Rowand wrote:
>> On 07/13/17 14:22, Phil Elwell wrote:
>>> On 13/07/2017 21:07, Frank Rowand wrote:
>>>> On 07/13/17 12:38, Phil Elwell wrote:
>>>>
> 
> [snip]
> 
>>> hope an inability to solve the problem posed by this advanced usage won't
>>> prevent a solution to a simpler problem from being accepted.
> 
> I have waited until people started commenting on this patchset before
> replying.
> 
> I think we agree on a few things to keep the discussion moving forward.
> 
> 1. Stacked overlays are useful and make overlays easier to use.

Stacked overlays are required to handle an add-on board that
contains physical connectors to plug in yet more things.

I'm not sure what you mean when you say they "make overlays
easier to use".  Can you elaborate on that a little bit?


> 2. Changing the overlay symbols format now would be unwise.

I strongly disagree.  I would say that it is desirable to maintain
the current overlay format (not just __symbols__), and that there
will be pain (for bootloaders???) if the format changes.  But
the Linux implementation is not locked in if there is a good
reason to change the format.


> 3. A number of extensions have been put forward/requested.
> 
> 3.1. There should be a method to place a symbol on a node that didn't
> have one originally (due to vendor supplying broken DTB or being
> generated by firmware at runtime).

You saw my reaction of what to do about a broken vendor DTB in that
thread.  I do not think this method is a good idea.

I don't know why a DTB generated by firmware would be missing a symbol.
Was that discussed in that thread, and I'm just forgetting it?


> 3.2. Scoping symbol visibility in case of clashes.

Yes.  This especially makes sense when a board has
multiple identical connectors, and the add-on board
should not have to specify different symbols based
on which connector it is plugged in to.


> This can the ability

This can add the ability?


> to put multiple path references to a single label/symbol. i.e.
> foo = "/path/bar", "/path/bar/baz";
> Resolving the ambiguity would require the caller to provide it's
> 'location' on the tree. I.e. a device under /path/bar/baz would resolve
> to the latter. It is a big change semantically.

That is one possible implementation.

It would require changes to dtc.

For a simple example:

/ {
	target: node@0 {
	};
	node@1 {
		target: subnode@0 {
		};
	};
};

The current dtc will detect an error:
  <stdout>: ERROR (duplicate_label): Duplicate label 'target' on /node@1/subnode@0 and /node@0

To allow the same label at different scopes would lose the
ability to detect this type of error.  I think this kind of
error detection is critical since we rely so heavily on
including a number of dtsi files into a dts file.

Another possible implementation would be for the kernel to
associate the contents of the __symbols__ node with the
nodes added by the overlay that contained it.  Those
symbols would then be visible to a subsequent overlay
fragment that had a target that is either (1) the same
target as the first overlay or (2) a child of the target
of the first overlay, or (3) a descendant of the target
of the first overlay.  I haven't thought through the
implications of allowing (1) vs (2) vs (3).  For
instance, if the connector format was to have a connector
node that contained a child node which is where the
add-on board was loaded, then the second overlay target
would be that child node, and policy (3) would make sense.

This would work with a single fragment in an overlay.  If
there are multiple fragments in an overlay, maybe the
symbols could be split apart by fragment (since the
value of the properties in __symbols__ start with
"/fragment@XXX/__overlay__/...").  I need to think about
the implications of that a bit more.

This method also seems like it would work well with the
connector / plug architecture.

There is another use case where maybe the concept of
overlay symbol scoping would cause problems.  And that
is the beaglebone architecture, where the add-on board
connector does not contain just a "bus" or other I/O
interface, but exposes much of the SOC pins.  In that
case it might make more sense if overlay symbols were
global.

I'm sure there are other ways to implement scoping.
Suggestions are welcome.


> Do you have anything else?

There is the issue of tracking what a loaded overlay
is dependent upon.  At the moment this is avoided by
unloading overlays in the reverse order that they
were added.  But it would be nice to be able to
unload independent overlays in a different order.
That is not something that has to be handled in
the first implementation, but it is something to
keep in mind.

Nothing else at the moment about exposing overlay
symbols.  I'll keep thinking.


> Regards
> 
> -- Pantelis

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

[Phil Elwell]

On 24/07/2017 19:06, Frank Rowand wrote:
> On 07/14/17 00:21, Pantelis Antoniou wrote:
>
> Keeping in mind that this thread was originally about libfdt,
> not the Linux kernel, I am mostly talking about the Linux
> kernel implementation in this email.
>
>
>> Hi Frank,
>>
>> On Thu, 2017-07-13 at 14:40 -0700, Frank Rowand wrote:
>>> On 07/13/17 14:22, Phil Elwell wrote:
>>>> On 13/07/2017 21:07, Frank Rowand wrote:
>>>>> On 07/13/17 12:38, Phil Elwell wrote:
>>>>>
>>
>> [snip]
>>
>>>> hope an inability to solve the problem posed by this advanced usage won't
>>>> prevent a solution to a simpler problem from being accepted.
>>
>> I have waited until people started commenting on this patchset before
>> replying.
>>
>> I think we agree on a few things to keep the discussion moving forward.
>>
>> 1. Stacked overlays are useful and make overlays easier to use.
>
> Stacked overlays are required to handle an add-on board that
> contains physical connectors to plug in yet more things.
>
> I'm not sure what you mean when you say they "make overlays
> easier to use".  Can you elaborate on that a little bit?
>
>
>> 2. Changing the overlay symbols format now would be unwise.
>
> I strongly disagree.  I would say that it is desirable to maintain
> the current overlay format (not just __symbols__), and that there
> will be pain (for bootloaders???) if the format changes.  But
> the Linux implementation is not locked in if there is a good
> reason to change the format.

And I gently disagree. Provided changes are made in a way that permits
overlays written in the old style to be distinguished unambiguously then
a new format may be the best way to tackle all of the new requirements.

>> 3. A number of extensions have been put forward/requested.
>>
>> 3.1. There should be a method to place a symbol on a node that didn't
>> have one originally (due to vendor supplying broken DTB or being
>> generated by firmware at runtime).
>
> You saw my reaction of what to do about a broken vendor DTB in that
> thread.  I do not think this method is a good idea.
>
> I don't know why a DTB generated by firmware would be missing a symbol.
> Was that discussed in that thread, and I'm just forgetting it?
>
>
>> 3.2. Scoping symbol visibility in case of clashes.
>
> Yes.  This especially makes sense when a board has
> multiple identical connectors, and the add-on board
> should not have to specify different symbols based
> on which connector it is plugged in to.
>
>
>> This can the ability
>
> This can add the ability?
>
>
>> to put multiple path references to a single label/symbol. i.e.
>> foo = "/path/bar", "/path/bar/baz";
>> Resolving the ambiguity would require the caller to provide it's
>> 'location' on the tree. I.e. a device under /path/bar/baz would resolve
>> to the latter. It is a big change semantically.
>
> That is one possible implementation.
>
> It would require changes to dtc.
>
> For a simple example:
>
> / {
> 	target: node@0 {
> 	};
> 	node@1 {
> 		target: subnode@0 {
> 		};
> 	};
> };
>
> The current dtc will detect an error:
>   <stdout>: ERROR (duplicate_label): Duplicate label 'target' on /node@1/subnode@0 and /node@0
>
> To allow the same label at different scopes would lose the
> ability to detect this type of error.  I think this kind of
> error detection is critical since we rely so heavily on
> including a number of dtsi files into a dts file.
>
> Another possible implementation would be for the kernel to
> associate the contents of the __symbols__ node with the
> nodes added by the overlay that contained it.  Those
> symbols would then be visible to a subsequent overlay
> fragment that had a target that is either (1) the same
> target as the first overlay or (2) a child of the target
> of the first overlay, or (3) a descendant of the target
> of the first overlay.  I haven't thought through the
> implications of allowing (1) vs (2) vs (3).  For
> instance, if the connector format was to have a connector
> node that contained a child node which is where the
> add-on board was loaded, then the second overlay target
> would be that child node, and policy (3) would make sense.
>
> This would work with a single fragment in an overlay.  If
> there are multiple fragments in an overlay, maybe the
> symbols could be split apart by fragment (since the
> value of the properties in __symbols__ start with
> "/fragment@XXX/__overlay__/...").  I need to think about
> the implications of that a bit more.
>
> This method also seems like it would work well with the
> connector / plug architecture.
>
> There is another use case where maybe the concept of
> overlay symbol scoping would cause problems.  And that
> is the beaglebone architecture, where the add-on board
> connector does not contain just a "bus" or other I/O
> interface, but exposes much of the SOC pins.  In that
> case it might make more sense if overlay symbols were
> global.
>
> I'm sure there are other ways to implement scoping.
> Suggestions are welcome.

If a label is considered to be local to the scope of the containing
node and its children, is it necessary to permit the same label to be
redefined while a previous definition is in scope?

To modify your example, consider:

/ {
	target0: node@0 {
		subtarget: subnode@0 {
		};
	};
	target1: node@1 {
		subtarget: subnode@0 {
		};
	};
};

If duplicate symbols were restricted in this way then it would still
be possible to detect many cases of accidental symbol re-use without
removing much (any?) useful functionality.

>> Do you have anything else?
>
> There is the issue of tracking what a loaded overlay
> is dependent upon.  At the moment this is avoided by
> unloading overlays in the reverse order that they
> were added.  But it would be nice to be able to
> unload independent overlays in a different order.
> That is not something that has to be handled in
> the first implementation, but it is something to
> keep in mind.
>
> Nothing else at the moment about exposing overlay
> symbols.  I'll keep thinking.

Although not a symbol issue, I'd like to repeat my request for
a way for an overlay to delete properties (necessary for boolean
properties) and perhaps nodes, so that it can be considered
during any redesign.

Regards,

Phil

--
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 1/2] fdt: Allow stacked overlays phandle references

[Frank Rowand]

On 07/24/17 13:51, Phil Elwell wrote:
> On 24/07/2017 19:06, Frank Rowand wrote:
>> On 07/14/17 00:21, Pantelis Antoniou wrote:
>>
>> Keeping in mind that this thread was originally about libfdt,
>> not the Linux kernel, I am mostly talking about the Linux
>> kernel implementation in this email.
>>
>>
>>> Hi Frank,
>>>
>>> On Thu, 2017-07-13 at 14:40 -0700, Frank Rowand wrote:
>>>> On 07/13/17 14:22, Phil Elwell wrote:
>>>>> On 13/07/2017 21:07, Frank Rowand wrote:
>>>>>> On 07/13/17 12:38, Phil Elwell wrote:
>>>>>>
>>>
>>> [snip]
>>>
>>>>> hope an inability to solve the problem posed by this advanced usage won't
>>>>> prevent a solution to a simpler problem from being accepted.
>>>
>>> I have waited until people started commenting on this patchset before
>>> replying.
>>>
>>> I think we agree on a few things to keep the discussion moving forward.
>>>
>>> 1. Stacked overlays are useful and make overlays easier to use.
>>
>> Stacked overlays are required to handle an add-on board that
>> contains physical connectors to plug in yet more things.
>>
>> I'm not sure what you mean when you say they "make overlays
>> easier to use".  Can you elaborate on that a little bit?
>>
>>
>>> 2. Changing the overlay symbols format now would be unwise.
>>
>> I strongly disagree.  I would say that it is desirable to maintain
>> the current overlay format (not just __symbols__), and that there
>> will be pain (for bootloaders???) if the format changes.  But
>> the Linux implementation is not locked in if there is a good
>> reason to change the format.
> 
> And I gently disagree. Provided changes are made in a way that permits
> overlays written in the old style to be distinguished unambiguously then
> a new format may be the best way to tackle all of the new requirements.
> 
>>> 3. A number of extensions have been put forward/requested.
>>>
>>> 3.1. There should be a method to place a symbol on a node that didn't
>>> have one originally (due to vendor supplying broken DTB or being
>>> generated by firmware at runtime).
>>
>> You saw my reaction of what to do about a broken vendor DTB in that
>> thread.  I do not think this method is a good idea.
>>
>> I don't know why a DTB generated by firmware would be missing a symbol.
>> Was that discussed in that thread, and I'm just forgetting it?
>>
>>
>>> 3.2. Scoping symbol visibility in case of clashes.
>>
>> Yes.  This especially makes sense when a board has
>> multiple identical connectors, and the add-on board
>> should not have to specify different symbols based
>> on which connector it is plugged in to.
>>
>>
>>> This can the ability
>>
>> This can add the ability?
>>
>>
>>> to put multiple path references to a single label/symbol. i.e.
>>> foo = "/path/bar", "/path/bar/baz";
>>> Resolving the ambiguity would require the caller to provide it's
>>> 'location' on the tree. I.e. a device under /path/bar/baz would resolve
>>> to the latter. It is a big change semantically.
>>
>> That is one possible implementation.
>>
>> It would require changes to dtc.
>>
>> For a simple example:
>>
>> / {
>>     target: node@0 {
>>     };
>>     node@1 {
>>         target: subnode@0 {
>>         };
>>     };
>> };
>>
>> The current dtc will detect an error:
>>   <stdout>: ERROR (duplicate_label): Duplicate label 'target' on /node@1/subnode@0 and /node@0
>>
>> To allow the same label at different scopes would lose the
>> ability to detect this type of error.  I think this kind of
>> error detection is critical since we rely so heavily on
>> including a number of dtsi files into a dts file.
>>


========================================================
I think I went off the rails a little bit starting here:


>> Another possible implementation would be for the kernel to
>> associate the contents of the __symbols__ node with the
>> nodes added by the overlay that contained it.  Those
>> symbols would then be visible to a subsequent overlay
>> fragment that had a target that is either (1) the same
>> target as the first overlay or (2) a child of the target
>> of the first overlay, or (3) a descendant of the target
>> of the first overlay.  I haven't thought through the
>> implications of allowing (1) vs (2) vs (3).  For
>> instance, if the connector format was to have a connector
>> node that contained a child node which is where the
>> add-on board was loaded, then the second overlay target
>> would be that child node, and policy (3) would make sense.
>>
>> This would work with a single fragment in an overlay.  If
>> there are multiple fragments in an overlay, maybe the
>> symbols could be split apart by fragment (since the
>> value of the properties in __symbols__ start with
>> "/fragment@XXX/__overlay__/...").  I need to think about
>> the implications of that a bit more.
>>
>> This method also seems like it would work well with the
>> connector / plug architecture.
>>
>> There is another use case where maybe the concept of
>> overlay symbol scoping would cause problems.  And that
>> is the beaglebone architecture, where the add-on board
>> connector does not contain just a "bus" or other I/O
>> interface, but exposes much of the SOC pins.  In that
>> case it might make more sense if overlay symbols were
>> global.
>>
>> I'm sure there are other ways to implement scoping.
>> Suggestions are welcome.

and this is the end of me going off the rails
========================================================

What I should have done at this point in my previous reply was
to go back and look at what we had proposed in the context of
connectors, which deals with this whole issue by providing an
explicit mapping of resources that would be used by an add-on
board that is plugged into a connector.  There can be multiple
physical connectors with the same functionality, which would
all look identical to the node that is plugged in.  So if
you had two identical connectors (eg grove connectors), and
two identical add-on boards that you wanted to plug in, you
could use the same exact overlay (.dtb or .dtbo) for the two
add-on boards (the "add an overlay" request to the overlay
loader would explicitly request a target instead of relying
on a target property located in the overlay), and the mappings
in the two connectors would provide the symbol fixup information
that the overlay loader needs.  So the symbol scope problem
is solved by an explicit map in the connector's dts source
instead of implicitly by dtc providing the scoping.

The connector discussion (which did not come to a final
resolution, but has a lot of good content) is at:

   https://lkml.org/lkml/2016/7/18/332


> If a label is considered to be local to the scope of the containing
> node and its children, is it necessary to permit the same label to be
> redefined while a previous definition is in scope?
> 
> To modify your example, consider:
> 
> / {
>     target0: node@0 {
>         subtarget: subnode@0 {
>         };
>     };
>     target1: node@1 {
>         subtarget: subnode@0 {
>         };
>     };
> };
> 
> If duplicate symbols were restricted in this way then it would still
> be possible to detect many cases of accidental symbol re-use without
> removing much (any?) useful functionality.

Many cases, but not all cases.  The connector approach avoids this
problem totally.

>>> Do you have anything else?
>>
>> There is the issue of tracking what a loaded overlay
>> is dependent upon.  At the moment this is avoided by
>> unloading overlays in the reverse order that they
>> were added.  But it would be nice to be able to
>> unload independent overlays in a different order.
>> That is not something that has to be handled in
>> the first implementation, but it is something to
>> keep in mind.
>>
>> Nothing else at the moment about exposing overlay
>> symbols.  I'll keep thinking.
> 
> Although not a symbol issue, I'd like to repeat my request for
> a way for an overlay to delete properties (necessary for boolean
> properties) and perhaps nodes, so that it can be considered
> during any redesign.

Yes, not a symbol issue.  :-)


> Regards,
> 
> Phil
> 
> 

--
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 1/2] fdt: Allow stacked overlays phandle references

[David Gibson]

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

On Mon, Jul 24, 2017 at 11:06:49AM -0700, Frank Rowand wrote:
> On 07/14/17 00:21, Pantelis Antoniou wrote:
> 
> Keeping in mind that this thread was originally about libfdt,
> not the Linux kernel, I am mostly talking about the Linux
> kernel implementation in this email.
> 
> 
> > Hi Frank,
> > 
> > On Thu, 2017-07-13 at 14:40 -0700, Frank Rowand wrote:
> >> On 07/13/17 14:22, Phil Elwell wrote:
> >>> On 13/07/2017 21:07, Frank Rowand wrote:
> >>>> On 07/13/17 12:38, Phil Elwell wrote:
> >>>>
> > 
> > [snip]
> > 
> >>> hope an inability to solve the problem posed by this advanced usage won't
> >>> prevent a solution to a simpler problem from being accepted.
> > 
> > I have waited until people started commenting on this patchset before
> > replying.
> > 
> > I think we agree on a few things to keep the discussion moving forward.
> > 
> > 1. Stacked overlays are useful and make overlays easier to use.
> 
> Stacked overlays are required to handle an add-on board that
> contains physical connectors to plug in yet more things.
> 
> I'm not sure what you mean when you say they "make overlays
> easier to use".  Can you elaborate on that a little bit?
> 
> 
> > 2. Changing the overlay symbols format now would be unwise.
> 
> I strongly disagree.  I would say that it is desirable to maintain
> the current overlay format (not just __symbols__), and that there
> will be pain (for bootloaders???) if the format changes.  But
> the Linux implementation is not locked in if there is a good
> reason to change the format.
> 
> 
> > 3. A number of extensions have been put forward/requested.
> > 
> > 3.1. There should be a method to place a symbol on a node that didn't
> > have one originally (due to vendor supplying broken DTB or being
> > generated by firmware at runtime).
> 
> You saw my reaction of what to do about a broken vendor DTB in that
> thread.  I do not think this method is a good idea.
> 
> I don't know why a DTB generated by firmware would be missing a symbol.
> Was that discussed in that thread, and I'm just forgetting it?

Because 9 times out of 10, firmware is crap?

-- 
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 1/2] fdt: Allow stacked overlays phandle references

[Frank Rowand]

On 07/25/17 21:55, David Gibson wrote:
> On Mon, Jul 24, 2017 at 11:06:49AM -0700, Frank Rowand wrote:
>> On 07/14/17 00:21, Pantelis Antoniou wrote:
>>
>> Keeping in mind that this thread was originally about libfdt,
>> not the Linux kernel, I am mostly talking about the Linux
>> kernel implementation in this email.
>>
>>
>>> Hi Frank,
>>>
>>> On Thu, 2017-07-13 at 14:40 -0700, Frank Rowand wrote:
>>>> On 07/13/17 14:22, Phil Elwell wrote:
>>>>> On 13/07/2017 21:07, Frank Rowand wrote:
>>>>>> On 07/13/17 12:38, Phil Elwell wrote:
>>>>>>
>>>
>>> [snip]
>>>
>>>>> hope an inability to solve the problem posed by this advanced usage won't
>>>>> prevent a solution to a simpler problem from being accepted.
>>>
>>> I have waited until people started commenting on this patchset before
>>> replying.
>>>
>>> I think we agree on a few things to keep the discussion moving forward.
>>>
>>> 1. Stacked overlays are useful and make overlays easier to use.
>>
>> Stacked overlays are required to handle an add-on board that
>> contains physical connectors to plug in yet more things.
>>
>> I'm not sure what you mean when you say they "make overlays
>> easier to use".  Can you elaborate on that a little bit?
>>
>>
>>> 2. Changing the overlay symbols format now would be unwise.
>>
>> I strongly disagree.  I would say that it is desirable to maintain
>> the current overlay format (not just __symbols__), and that there
>> will be pain (for bootloaders???) if the format changes.  But
>> the Linux implementation is not locked in if there is a good
>> reason to change the format.
>>
>>
>>> 3. A number of extensions have been put forward/requested.
>>>
>>> 3.1. There should be a method to place a symbol on a node that didn't
>>> have one originally (due to vendor supplying broken DTB or being
>>> generated by firmware at runtime).
>>
>> You saw my reaction of what to do about a broken vendor DTB in that
>> thread.  I do not think this method is a good idea.
>>
>> I don't know why a DTB generated by firmware would be missing a symbol.
>> Was that discussed in that thread, and I'm just forgetting it?
> 
> Because 9 times out of 10, firmware is crap?

Which is part of why I want access to, ability to modify, and ability
to update device trees in the hands of the user, not just the vendor.

-Frank

--
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 1/2] fdt: Allow stacked overlays phandle references

[David Gibson]

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

On Wed, Jul 26, 2017 at 07:03:11AM -0700, Frank Rowand wrote:
> On 07/25/17 21:55, David Gibson wrote:
> > On Mon, Jul 24, 2017 at 11:06:49AM -0700, Frank Rowand wrote:
> >> On 07/14/17 00:21, Pantelis Antoniou wrote:
> >>
> >> Keeping in mind that this thread was originally about libfdt,
> >> not the Linux kernel, I am mostly talking about the Linux
> >> kernel implementation in this email.
> >>
> >>
> >>> Hi Frank,
> >>>
> >>> On Thu, 2017-07-13 at 14:40 -0700, Frank Rowand wrote:
> >>>> On 07/13/17 14:22, Phil Elwell wrote:
> >>>>> On 13/07/2017 21:07, Frank Rowand wrote:
> >>>>>> On 07/13/17 12:38, Phil Elwell wrote:
> >>>>>>
> >>>
> >>> [snip]
> >>>
> >>>>> hope an inability to solve the problem posed by this advanced usage won't
> >>>>> prevent a solution to a simpler problem from being accepted.
> >>>
> >>> I have waited until people started commenting on this patchset before
> >>> replying.
> >>>
> >>> I think we agree on a few things to keep the discussion moving forward.
> >>>
> >>> 1. Stacked overlays are useful and make overlays easier to use.
> >>
> >> Stacked overlays are required to handle an add-on board that
> >> contains physical connectors to plug in yet more things.
> >>
> >> I'm not sure what you mean when you say they "make overlays
> >> easier to use".  Can you elaborate on that a little bit?
> >>
> >>
> >>> 2. Changing the overlay symbols format now would be unwise.
> >>
> >> I strongly disagree.  I would say that it is desirable to maintain
> >> the current overlay format (not just __symbols__), and that there
> >> will be pain (for bootloaders???) if the format changes.  But
> >> the Linux implementation is not locked in if there is a good
> >> reason to change the format.
> >>
> >>
> >>> 3. A number of extensions have been put forward/requested.
> >>>
> >>> 3.1. There should be a method to place a symbol on a node that didn't
> >>> have one originally (due to vendor supplying broken DTB or being
> >>> generated by firmware at runtime).
> >>
> >> You saw my reaction of what to do about a broken vendor DTB in that
> >> thread.  I do not think this method is a good idea.
> >>
> >> I don't know why a DTB generated by firmware would be missing a symbol.
> >> Was that discussed in that thread, and I'm just forgetting it?
> > 
> > Because 9 times out of 10, firmware is crap?
> 
> Which is part of why I want access to, ability to modify, and ability
> to update device trees in the hands of the user, not just the
> vendor.

Couldn't agree more.

-- 
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 1/2] fdt: Allow stacked overlays phandle references

[David Gibson]

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

On Fri, Jul 14, 2017 at 10:21:01AM +0300, Pantelis Antoniou wrote:
> Hi Frank,
> 
> On Thu, 2017-07-13 at 14:40 -0700, Frank Rowand wrote:
> > On 07/13/17 14:22, Phil Elwell wrote:
> > > On 13/07/2017 21:07, Frank Rowand wrote:
> > >> On 07/13/17 12:38, Phil Elwell wrote:
> > >>
> 
> [snip]
> 
> > > hope an inability to solve the problem posed by this advanced usage won't
> > > prevent a solution to a simpler problem from being accepted.
> 
> I have waited until people started commenting on this patchset before
> replying.
> 
> I think we agree on a few things to keep the discussion moving forward.
> 
> 1. Stacked overlays are useful and make overlays easier to use.

Agreed.

> 2. Changing the overlay symbols format now would be unwise.

Agreed.  At least, I don't think updating the symbols alone would be
silly without revisiting everything in the overlay format and making
something completely new.

> 3. A number of extensions have been put forward/requested.
> 
> 3.1. There should be a method to place a symbol on a node that didn't
> have one originally (due to vendor supplying broken DTB or being
> generated by firmware at runtime).

There already is.  An overlay can update *anything* in the base tree,
including the /__symbols__ node.  Of course you need the exact path of
the node to tag in the base tree, but you were always going to need
that.

> 3.2. Scoping symbol visibility in case of clashes. This can the ability
> to put multiple path references to a single label/symbol. i.e.
> foo = "/path/bar", "/path/bar/baz";
> Resolving the ambiguity would require the caller to provide it's
> 'location' on the tree. I.e. a device under /path/bar/baz would resolve
> to the latter. It is a big change semantically.

I think this would be a nice idea, but trying to do it as a update to
the existing overlay format will be really difficult verging on
impossible.

Better to keep this in mind as a design goal for a new format to
replace overlays.

-- 
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 1/2] fdt: Allow stacked overlays phandle references

[Frank Rowand]

On 07/25/17 21:32, David Gibson wrote:
> On Fri, Jul 14, 2017 at 10:21:01AM +0300, Pantelis Antoniou wrote:
>> Hi Frank,
>>
>> On Thu, 2017-07-13 at 14:40 -0700, Frank Rowand wrote:
>>> On 07/13/17 14:22, Phil Elwell wrote:
>>>> On 13/07/2017 21:07, Frank Rowand wrote:
>>>>> On 07/13/17 12:38, Phil Elwell wrote:
>>>>>
>>
>> [snip]
>>
>>>> hope an inability to solve the problem posed by this advanced usage won't
>>>> prevent a solution to a simpler problem from being accepted.
>>
>> I have waited until people started commenting on this patchset before
>> replying.
>>
>> I think we agree on a few things to keep the discussion moving forward.
>>
>> 1. Stacked overlays are useful and make overlays easier to use.
> 
> Agreed.
> 
>> 2. Changing the overlay symbols format now would be unwise.
> 
> Agreed.  At least, I don't think updating the symbols alone would be
> silly without revisiting everything in the overlay format and making
> something completely new.
> 
>> 3. A number of extensions have been put forward/requested.
>>
>> 3.1. There should be a method to place a symbol on a node that didn't
>> have one originally (due to vendor supplying broken DTB or being
>> generated by firmware at runtime).
> 
> There already is.  An overlay can update *anything* in the base tree,
> including the /__symbols__ node.  Of course you need the exact path of
> the node to tag in the base tree, but you were always going to need
> that.

I haven't tested that, but it should work with the existing dtc and
Linux kernel.

But it will stop working in the future _if_ some changes are made
that I would like:

  - dtc no longer accept node names beginning with underscore as
    valid.
  - dtc supports Pantelis' sugar syntax

My intent behind these changes is to hide the implementation details
of the overlay extensions (__symbols__, __fixups__, __local_fixups__,
fragements nodes, etc) from the normal dts developer.  This should
make it easier to write and understand overlays, and reduce errors
in the dtb.

With those changes, it would not be possible to write an overlay
that applied against node __symbols__ because it would not be
possible to create a label on __symbols__, which would be needed
to reference __symbols__ with the sugar syntax.

-Frank

>> 3.2. Scoping symbol visibility in case of clashes. This can the ability
>> to put multiple path references to a single label/symbol. i.e.
>> foo = "/path/bar", "/path/bar/baz";
>> Resolving the ambiguity would require the caller to provide it's
>> 'location' on the tree. I.e. a device under /path/bar/baz would resolve
>> to the latter. It is a big change semantically.
> 
> I think this would be a nice idea, but trying to do it as a update to
> the existing overlay format will be really difficult verging on
> impossible.
> 
> Better to keep this in mind as a design goal for a new format to
> replace overlays.
> 

--
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 1/2] fdt: Allow stacked overlays phandle references

[David Gibson]

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

On Wed, Jul 26, 2017 at 06:59:35AM -0700, Frank Rowand wrote:
> On 07/25/17 21:32, David Gibson wrote:
> > On Fri, Jul 14, 2017 at 10:21:01AM +0300, Pantelis Antoniou wrote:
> >> Hi Frank,
> >>
> >> On Thu, 2017-07-13 at 14:40 -0700, Frank Rowand wrote:
> >>> On 07/13/17 14:22, Phil Elwell wrote:
> >>>> On 13/07/2017 21:07, Frank Rowand wrote:
> >>>>> On 07/13/17 12:38, Phil Elwell wrote:
> >>>>>
> >>
> >> [snip]
> >>
> >>>> hope an inability to solve the problem posed by this advanced usage won't
> >>>> prevent a solution to a simpler problem from being accepted.
> >>
> >> I have waited until people started commenting on this patchset before
> >> replying.
> >>
> >> I think we agree on a few things to keep the discussion moving forward.
> >>
> >> 1. Stacked overlays are useful and make overlays easier to use.
> > 
> > Agreed.
> > 
> >> 2. Changing the overlay symbols format now would be unwise.
> > 
> > Agreed.  At least, I don't think updating the symbols alone would be
> > silly without revisiting everything in the overlay format and making
> > something completely new.
> > 
> >> 3. A number of extensions have been put forward/requested.
> >>
> >> 3.1. There should be a method to place a symbol on a node that didn't
> >> have one originally (due to vendor supplying broken DTB or being
> >> generated by firmware at runtime).
> > 
> > There already is.  An overlay can update *anything* in the base tree,
> > including the /__symbols__ node.  Of course you need the exact path of
> > the node to tag in the base tree, but you were always going to need
> > that.
> 
> I haven't tested that, but it should work with the existing dtc and
> Linux kernel.
> 
> But it will stop working in the future _if_ some changes are made
> that I would like:
> 
>   - dtc no longer accept node names beginning with underscore as
>     valid.

I don't like that idea.  Warning, sure, but I don't wish to outright
ban constructs which are allowed by the syntax and IEEE1275.  Allowing
special effects like the above is one reason for that.

>   - dtc supports Pantelis' sugar syntax

Uh.. I don't see how that's relevant.

> My intent behind these changes is to hide the implementation details
> of the overlay extensions (__symbols__, __fixups__, __local_fixups__,
> fragements nodes, etc) from the normal dts developer.

A good goal, but that doesn't preclude them being accessible to
the.. uh.. abnormal dts developer?

> This should
> make it easier to write and understand overlays, and reduce errors
> in the dtb.
> 
> With those changes, it would not be possible to write an overlay
> that applied against node __symbols__ because it would not be
> possible to create a label on __symbols__, which would be needed
> to reference __symbols__ with the sugar syntax.

I haven't looked at Pantelis' latest patches.  But AFAIK it's based on
the existing compile time overlay syntax, which allows addressing by
path as well label.  So you could do

&{/__symbols__} {
	gadget = "/path/to/forgotten/gadget";
};

> 
> -Frank
> 
> >> 3.2. Scoping symbol visibility in case of clashes. This can the ability
> >> to put multiple path references to a single label/symbol. i.e.
> >> foo = "/path/bar", "/path/bar/baz";
> >> Resolving the ambiguity would require the caller to provide it's
> >> 'location' on the tree. I.e. a device under /path/bar/baz would resolve
> >> to the latter. It is a big change semantically.
> > 
> > I think this would be a nice idea, but trying to do it as a update to
> > the existing overlay format will be really difficult verging on
> > impossible.
> > 
> > Better to keep this in mind as a design goal for a new format to
> > replace overlays.
> > 
> 

-- 
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 1/2] fdt: Allow stacked overlays phandle references

[Frank Rowand]

On 07/27/17 00:23, David Gibson wrote:
> On Wed, Jul 26, 2017 at 06:59:35AM -0700, Frank Rowand wrote:
>> On 07/25/17 21:32, David Gibson wrote:
>>> On Fri, Jul 14, 2017 at 10:21:01AM +0300, Pantelis Antoniou wrote:
>>>> Hi Frank,
>>>>
>>>> On Thu, 2017-07-13 at 14:40 -0700, Frank Rowand wrote:
>>>>> On 07/13/17 14:22, Phil Elwell wrote:
>>>>>> On 13/07/2017 21:07, Frank Rowand wrote:
>>>>>>> On 07/13/17 12:38, Phil Elwell wrote:
>>>>>>>
>>>>
>>>> [snip]
>>>>
>>>>>> hope an inability to solve the problem posed by this advanced usage won't
>>>>>> prevent a solution to a simpler problem from being accepted.
>>>>
>>>> I have waited until people started commenting on this patchset before
>>>> replying.
>>>>
>>>> I think we agree on a few things to keep the discussion moving forward.
>>>>
>>>> 1. Stacked overlays are useful and make overlays easier to use.
>>>
>>> Agreed.
>>>
>>>> 2. Changing the overlay symbols format now would be unwise.
>>>
>>> Agreed.  At least, I don't think updating the symbols alone would be
>>> silly without revisiting everything in the overlay format and making
>>> something completely new.
>>>
>>>> 3. A number of extensions have been put forward/requested.
>>>>
>>>> 3.1. There should be a method to place a symbol on a node that didn't
>>>> have one originally (due to vendor supplying broken DTB or being
>>>> generated by firmware at runtime).
>>>
>>> There already is.  An overlay can update *anything* in the base tree,
>>> including the /__symbols__ node.  Of course you need the exact path of
>>> the node to tag in the base tree, but you were always going to need
>>> that.
>>
>> I haven't tested that, but it should work with the existing dtc and
>> Linux kernel.
>>
>> But it will stop working in the future _if_ some changes are made
>> that I would like:
>>
>>   - dtc no longer accept node names beginning with underscore as
>>     valid.
> 
> I don't like that idea.  Warning, sure, but I don't wish to outright
> ban constructs which are allowed by the syntax and IEEE1275.  Allowing
> special effects like the above is one reason for that.

Oops, my bad, sorry. I left out half of what I have been advocating
in the past, so basically I agree with you. My past statements also
included some escape mechanism, such as a command line option that
would allow node names beginning with an underscore.

You accepted the patch to add -Wnode_name_chars_strict that warns of
use of underscore anywhere in the node name, so Linux does have the
option of enabling that warning, which would be another partial
solution to what I was suggesting.

I wrote the previous two paragraphs, and  then realized I missed part
of what I was responding to.  The ePAPR and the Device Tree Specification
both say that a node name "shall start with a lower or uppercase character",
so the construct is not allowed by the syntax.

Sigh.  You explicitly said "IEEE125".  So off I go to read that a bit,
and as far as a quick reading goes, there is no restriction of what
the first character may be.

I have previously ignored IEEE1275, because the ePAPR was what I
thought the Linux kernel and dtc used as a reference (and now
Linux has moved on to the "Device Tree Specification").  What
is the policy of the dtc project when the ePAPR (and "Device
Tree Reference") differ from IEEE1275?


> 
>>   - dtc supports Pantelis' sugar syntax
> 
> Uh.. I don't see how that's relevant.

The sugar syntax is the only way that I am aware of to code an
overlay dts such that dtc would create the __fixups__, __local_fixups__,
and fragment nodes, without explicitly coding node names beginning
with an underscore in the source dts.


>> My intent behind these changes is to hide the implementation details
>> of the overlay extensions (__symbols__, __fixups__, __local_fixups__,
>> fragements nodes, etc) from the normal dts developer.
> 
> A good goal, but that doesn't preclude them being accessible to
> the.. uh.. abnormal dts developer?


(I wrote the following three paragraphs before looking at IEEE125.)

The ePAPR and the Device Tree Specification both state that a node
name "shall start with a lower or uppercase character".  That should
probably be 'letter' instead of 'character', but my understanding
is that 'letter' is the intent.

It seems useful for dtc to disallow what would seem to me to be an
error in dts source, when a node name started with some other
character.

But again, there should probably be a command line option or some
other method to allow an underscore as the first character of a
node name, at least in a transition period.


>> This should
>> make it easier to write and understand overlays, and reduce errors
>> in the dtb.
>>
>> With those changes, it would not be possible to write an overlay
>> that applied against node __symbols__ because it would not be
>> possible to create a label on __symbols__, which would be needed
>> to reference __symbols__ with the sugar syntax.
> 
> I haven't looked at Pantelis' latest patches.  But AFAIK it's based on
> the existing compile time overlay syntax, which allows addressing by
> path as well label.  So you could do
> 
> &{/__symbols__} {
> 	gadget = "/path/to/forgotten/gadget";
> };

Hmmmm.

(_If_ a leading underscore is not allowed as the first character of
a node name:)

I would fall back on my suggestion that the leading underscore should
be detected as an illegal node name here also (override allowed...).
I don't know if that would be the case in this specific location if
it was detected as illegal when specifying a node in the tree.  In
other words, I don't know how the dtc code checks for a valid node
name, so I don't know if the same code would check for valid node
name in the two different locations of the syntax.


>> -Frank
>>
>>>> 3.2. Scoping symbol visibility in case of clashes. This can the ability
>>>> to put multiple path references to a single label/symbol. i.e.
>>>> foo = "/path/bar", "/path/bar/baz";
>>>> Resolving the ambiguity would require the caller to provide it's
>>>> 'location' on the tree. I.e. a device under /path/bar/baz would resolve
>>>> to the latter. It is a big change semantically.
>>>
>>> I think this would be a nice idea, but trying to do it as a update to
>>> the existing overlay format will be really difficult verging on
>>> impossible.
>>>
>>> Better to keep this in mind as a design goal for a new format to
>>> replace overlays.
>>>
>>
> 

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

[David Gibson]

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

On Thu, Jul 27, 2017 at 10:24:17AM -0700, Frank Rowand wrote:
> On 07/27/17 00:23, David Gibson wrote:
> > On Wed, Jul 26, 2017 at 06:59:35AM -0700, Frank Rowand wrote:
> >> On 07/25/17 21:32, David Gibson wrote:
> >>> On Fri, Jul 14, 2017 at 10:21:01AM +0300, Pantelis Antoniou wrote:
> >>>> Hi Frank,
> >>>>
> >>>> On Thu, 2017-07-13 at 14:40 -0700, Frank Rowand wrote:
> >>>>> On 07/13/17 14:22, Phil Elwell wrote:
> >>>>>> On 13/07/2017 21:07, Frank Rowand wrote:
> >>>>>>> On 07/13/17 12:38, Phil Elwell wrote:
> >>>>>>>
> >>>>
> >>>> [snip]
> >>>>
> >>>>>> hope an inability to solve the problem posed by this advanced usage won't
> >>>>>> prevent a solution to a simpler problem from being accepted.
> >>>>
> >>>> I have waited until people started commenting on this patchset before
> >>>> replying.
> >>>>
> >>>> I think we agree on a few things to keep the discussion moving forward.
> >>>>
> >>>> 1. Stacked overlays are useful and make overlays easier to use.
> >>>
> >>> Agreed.
> >>>
> >>>> 2. Changing the overlay symbols format now would be unwise.
> >>>
> >>> Agreed.  At least, I don't think updating the symbols alone would be
> >>> silly without revisiting everything in the overlay format and making
> >>> something completely new.
> >>>
> >>>> 3. A number of extensions have been put forward/requested.
> >>>>
> >>>> 3.1. There should be a method to place a symbol on a node that didn't
> >>>> have one originally (due to vendor supplying broken DTB or being
> >>>> generated by firmware at runtime).
> >>>
> >>> There already is.  An overlay can update *anything* in the base tree,
> >>> including the /__symbols__ node.  Of course you need the exact path of
> >>> the node to tag in the base tree, but you were always going to need
> >>> that.
> >>
> >> I haven't tested that, but it should work with the existing dtc and
> >> Linux kernel.
> >>
> >> But it will stop working in the future _if_ some changes are made
> >> that I would like:
> >>
> >>   - dtc no longer accept node names beginning with underscore as
> >>     valid.
> > 
> > I don't like that idea.  Warning, sure, but I don't wish to outright
> > ban constructs which are allowed by the syntax and IEEE1275.  Allowing
> > special effects like the above is one reason for that.
> 
> Oops, my bad, sorry. I left out half of what I have been advocating
> in the past, so basically I agree with you. My past statements also
> included some escape mechanism, such as a command line option that
> would allow node names beginning with an underscore.
> 
> You accepted the patch to add -Wnode_name_chars_strict that warns of
> use of underscore anywhere in the node name, so Linux does have the
> option of enabling that warning, which would be another partial
> solution to what I was suggesting.

Ok.

> I wrote the previous two paragraphs, and  then realized I missed part
> of what I was responding to.  The ePAPR and the Device Tree Specification
> both say that a node name "shall start with a lower or uppercase character",
> so the construct is not allowed by the syntax.

Ah, ok, I hadn't remembered that.

> Sigh.  You explicitly said "IEEE125".  So off I go to read that a bit,
> and as far as a quick reading goes, there is no restriction of what
> the first character may be.

Yeah, it doesn't put a lot of restrictions on the namespace.  It does
have a list of acceptable characters but.. some of those are rarely if
ever used in practice, and there are some others which are used in the
wild even though they weren't allowed by 1275 :/.

> I have previously ignored IEEE1275, because the ePAPR was what I
> thought the Linux kernel and dtc used as a reference (and now
> Linux has moved on to the "Device Tree Specification").  What
> is the policy of the dtc project when the ePAPR (and "Device
> Tree Reference") differ from IEEE1275?

Well, it hasn't come enough to have a policy as such, but I guess the
principles to decide a specific case would be:
    * dtc should be able to deal with any tree that's valid under
      either 1275 or "modern" (epapr or dt spec) rules.
    * "modern" rules should be the default when it's not possible to
      automatically deal with both options

Note that IEEE1275 still has direct relevance, POWER servers still
have real 1275 firmware.  For that reason I don't think it's a good
idea to diverge from that for more modern specs unless there's a
really compelling reason to do so.

That said, I think the main relevance of IEEE1275 isn't as a direct
specification, but as inspiration.  When we come across some new
problem to solve, it's worth looking at IEEE1275 and it's ecosystem of
bindings solved similar problems in the past.

> >>   - dtc supports Pantelis' sugar syntax
> > 
> > Uh.. I don't see how that's relevant.
> 
> The sugar syntax is the only way that I am aware of to code an
> overlay dts such that dtc would create the __fixups__, __local_fixups__,
> and fragment nodes, without explicitly coding node names beginning
> with an underscore in the source dts.

Right.. but you _can_ explicitly code underscore names, even if it
does require suppressing a warning.

> >> My intent behind these changes is to hide the implementation details
> >> of the overlay extensions (__symbols__, __fixups__, __local_fixups__,
> >> fragements nodes, etc) from the normal dts developer.
> > 
> > A good goal, but that doesn't preclude them being accessible to
> > the.. uh.. abnormal dts developer?
> 
> 
> (I wrote the following three paragraphs before looking at IEEE125.)
> 
> The ePAPR and the Device Tree Specification both state that a node
> name "shall start with a lower or uppercase character".  That should
> probably be 'letter' instead of 'character', but my understanding
> is that 'letter' is the intent.
> 
> It seems useful for dtc to disallow what would seem to me to be an
> error in dts source, when a node name started with some other
> character.
> 
> But again, there should probably be a command line option or some
> other method to allow an underscore as the first character of a
> node name, at least in a transition period.

Right, I believe we can currently turn off individual warnings, so
that's straightforward enough.  And/or we can use the -f option.

> 
> 
> >> This should
> >> make it easier to write and understand overlays, and reduce errors
> >> in the dtb.
> >>
> >> With those changes, it would not be possible to write an overlay
> >> that applied against node __symbols__ because it would not be
> >> possible to create a label on __symbols__, which would be needed
> >> to reference __symbols__ with the sugar syntax.
> > 
> > I haven't looked at Pantelis' latest patches.  But AFAIK it's based on
> > the existing compile time overlay syntax, which allows addressing by
> > path as well label.  So you could do
> > 
> > &{/__symbols__} {
> > 	gadget = "/path/to/forgotten/gadget";
> > };
> 
> Hmmmm.
> 
> (_If_ a leading underscore is not allowed as the first character of
> a node name:)
> 
> I would fall back on my suggestion that the leading underscore should
> be detected as an illegal node name here also (override allowed...).
> I don't know if that would be the case in this specific location if
> it was detected as illegal when specifying a node in the tree.  In
> other words, I don't know how the dtc code checks for a valid node
> name, so I don't know if the same code would check for valid node
> name in the two different locations of the syntax.

In this case.. probably not.  Checks (other than the most basic
fundamentals, such as no nuls) aren't part of the main compile path,
but rather in the "checks" subsystem.  Once assembled into overlay
form, that target node name wouldn't actually appear in a node name -
only indirectly in the target-path property.

That said, the checks mechanism needs some reworking to deal with
overlay more sensibly.

> >> -Frank
> >>
> >>>> 3.2. Scoping symbol visibility in case of clashes. This can the ability
> >>>> to put multiple path references to a single label/symbol. i.e.
> >>>> foo = "/path/bar", "/path/bar/baz";
> >>>> Resolving the ambiguity would require the caller to provide it's
> >>>> 'location' on the tree. I.e. a device under /path/bar/baz would resolve
> >>>> to the latter. It is a big change semantically.
> >>>
> >>> I think this would be a nice idea, but trying to do it as a update to
> >>> the existing overlay format will be really difficult verging on
> >>> impossible.
> >>>
> >>> Better to keep this in mind as a design goal for a new format to
> >>> replace overlays.
> >>>
> >>
> > 
> 

-- 
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 1/2] fdt: Allow stacked overlays phandle references

[David Gibson]

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

On Thu, Jul 13, 2017 at 02:40:12PM -0700, Frank Rowand wrote:
> On 07/13/17 14:22, Phil Elwell wrote:
> > On 13/07/2017 21:07, Frank Rowand wrote:
> >> On 07/13/17 12:38, Phil Elwell wrote:
> >>
> >> (I moved Phil's reply to after the email he replied to.)
> > 
> > Thanks.
> > 
> >>> On 13 Jul 2017 8:32 pm, "Frank Rowand" <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >>>
> >>>> On 07/03/17 02:06, David Gibson wrote:
> >>>>> On Wed, Jun 14, 2017 at 05:52:25PM +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.
> >>>>>
> >>>>> This seems to be doing things the hard way.
> >>>>>
> >>>>> You're essentially extending the semantics of overlay application to
> >>>>> add the symbol merging.  You've implemented these extended semantics
> >>>>> in libfdt, which is all very well, but that's not the only overlay
> >>>>> application implementation.
> >>>>>
> >>>>>
> >>>>> It seems to me a better approach would be to change dtc's -@
> >>>>> implementation, so that in /plugin/ mode instead of making a global
> >>>>> __symbols__ node, it puts it into the individual fragments.  That way
> >>>>> the existing overlay application semantics will update the __symbols__
> >>>>> node.
> >>>>
> >>>> If the __symbols__ node was inside a fragment, then the existing
> >>>> code would add (or update) a __symbols__ node located at the location
> >>>> pointed to by the fragment's target path, instead of updating the
> >>>> node /__symbols__.
> >>>>
> >>>> It makes sense to me to have only one global __symbols__ node instead
> >>>> of several.
> >>>>
> >>>> If there is a global __symbols__ node then we have a single name
> >>>> space for symbols.
> >>>>
> >>>> If there are multiple __symbols__ nodes spread throughout the tree,
> >>>> then to me that would imply different name spaces spread throughout
> >>>> the tree, where namespaces are determined by fragments.  This sounds
> >>>> confusing to me.  Or if the intent is to have a single name space
> >>>> then the __symbols__ information would be scattered throughout the
> >>>> tree instead of located in a single node.
> >>>>
> >>>> My current patch (under review), targeted for Linux 4.13-rc1, puts
> >>>> an overlay's __symbols__ node properties into the overlay's
> >>>> changeset, so they get added when the overlay is loaded and
> >>>> removed when the overlay is unloaded.
> >>
> >>> Can we also consider a mechanism for overlay-local symbols, i.e. symbols
> >>> that are used purely to create links within an overlay - perhaps using a
> >>> particular naming convention? This would make it easier to instantiate an
> >>> overlay multiple times without having to uniquify all symbols, and it would
> >>> avoid polluting the global namespace without reason.
> >>>
> >>> Phil
> >>
> >> That is essentially the result you get if you compile the overlay dts
> >> without '-@'.  There will be no __symbols__ node created even if there
> >> are symbols within the overlay.
> > 
> > But (unless something has changed recently) the '-@' switch controls both
> > symbol and fixup generation, i.e. export and import of symbols. Unless one
> > religiously uses 'target-path' to place fragments (thus removing the
> > level of abstraction provided by symbols) overlays are useless without
> > the ability to reference external symbols, but in my experience very few
> > overlays need to add symbols to the global symbol table.
> 
> For the dtc compiler in Linux 4.11, the '-@' switch is only needed
> to generate the __symbols__ node.  The __fixups__ and __local-fixups__
> nodes are generated whether '-@' is specified or not.
> 
> The __fixups__ and __local_fixups__ are generated when '/plugin/;'
> is specified in the source file.

Yup, i.e. something *has* changed recently.

Building fixups based on -@ never made any sense.

-- 
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 1/2] fdt: Allow stacked overlays phandle references

[David Gibson]

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

On Thu, Jul 13, 2017 at 08:38:10PM +0100, Phil Elwell wrote:
> Can we also consider a mechanism for overlay-local symbols, i.e. symbols
> that are used purely to create links within an overlay - perhaps using a
> particular naming convention? This would make it easier to instantiate an
> overlay multiple times without having to uniquify all symbols, and it would
> avoid polluting the global namespace without reason.

I'd really prefer not to add yet more features and complexity to the
existing shoddily designed overlay mechanism.  I'd prefer for people
to focus on a better replacement - which includes considering these
sorts of use cases and how to handle them.

> 
> Phil
> 
> On 13 Jul 2017 8:32 pm, "Frank Rowand" <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 
> > On 07/03/17 02:06, David Gibson wrote:
> > > On Wed, Jun 14, 2017 at 05:52:25PM +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.
> > >
> > > This seems to be doing things the hard way.
> > >
> > > You're essentially extending the semantics of overlay application to
> > > add the symbol merging.  You've implemented these extended semantics
> > > in libfdt, which is all very well, but that's not the only overlay
> > > application implementation.
> > >
> > >
> > > It seems to me a better approach would be to change dtc's -@
> > > implementation, so that in /plugin/ mode instead of making a global
> > > __symbols__ node, it puts it into the individual fragments.  That way
> > > the existing overlay application semantics will update the __symbols__
> > > node.
> >
> > If the __symbols__ node was inside a fragment, then the existing
> > code would add (or update) a __symbols__ node located at the location
> > pointed to by the fragment's target path, instead of updating the
> > node /__symbols__.
> >
> > It makes sense to me to have only one global __symbols__ node instead
> > of several.
> >
> > If there is a global __symbols__ node then we have a single name
> > space for symbols.
> >
> > If there are multiple __symbols__ nodes spread throughout the tree,
> > then to me that would imply different name spaces spread throughout
> > the tree, where namespaces are determined by fragments.  This sounds
> > confusing to me.  Or if the intent is to have a single name space
> > then the __symbols__ information would be scattered throughout the
> > tree instead of located in a single node.
> >
> > My current patch (under review), targeted for Linux 4.13-rc1, puts
> > an overlay's __symbols__ node properties into the overlay's
> > changeset, so they get added when the overlay is loaded and
> > removed when the overlay is unloaded.
> >
> > -Frank
> >
> >

-- 
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 1/2] fdt: Allow stacked overlays phandle references

[Tom Rini]

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

On Wed, Jul 26, 2017 at 02:23:15PM +1000, David Gibson wrote:
> On Thu, Jul 13, 2017 at 08:38:10PM +0100, Phil Elwell wrote:
> > Can we also consider a mechanism for overlay-local symbols, i.e. symbols
> > that are used purely to create links within an overlay - perhaps using a
> > particular naming convention? This would make it easier to instantiate an
> > overlay multiple times without having to uniquify all symbols, and it would
> > avoid polluting the global namespace without reason.
> 
> I'd really prefer not to add yet more features and complexity to the
> existing shoddily designed overlay mechanism.  I'd prefer for people
> to focus on a better replacement - which includes considering these
> sorts of use cases and how to handle them.

Can we go for incremental progress over time instead of replacing what
was finally accepted?  Maybe one or two concrete examples that need
improvement upon, and go from there?  Thanks!

-- 
Tom

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[David Gibson]

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

On Wed, Jul 26, 2017 at 11:01:48AM -0400, Tom Rini wrote:
> On Wed, Jul 26, 2017 at 02:23:15PM +1000, David Gibson wrote:
> > On Thu, Jul 13, 2017 at 08:38:10PM +0100, Phil Elwell wrote:
> > > Can we also consider a mechanism for overlay-local symbols, i.e. symbols
> > > that are used purely to create links within an overlay - perhaps using a
> > > particular naming convention? This would make it easier to instantiate an
> > > overlay multiple times without having to uniquify all symbols, and it would
> > > avoid polluting the global namespace without reason.
> > 
> > I'd really prefer not to add yet more features and complexity to the
> > existing shoddily designed overlay mechanism.  I'd prefer for people
> > to focus on a better replacement - which includes considering these
> > sorts of use cases and how to handle them.
> 
> Can we go for incremental progress over time instead of replacing what
> was finally accepted?  Maybe one or two concrete examples that need
> improvement upon, and go from there?  Thanks!

As a general rule, I'd agree with that approach, but not this time.
Incremental improvements are great if you have a solid base from which
to work, at the moment we don't.  What we have is a half-arsed hack
that become widely deployed enough that we have to live with it.

-- 
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 1/2] fdt: Allow stacked overlays phandle references

[Tom Rini]

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

On Thu, Jul 27, 2017 at 05:25:34PM +1000, David Gibson wrote:
> On Wed, Jul 26, 2017 at 11:01:48AM -0400, Tom Rini wrote:
> > On Wed, Jul 26, 2017 at 02:23:15PM +1000, David Gibson wrote:
> > > On Thu, Jul 13, 2017 at 08:38:10PM +0100, Phil Elwell wrote:
> > > > Can we also consider a mechanism for overlay-local symbols, i.e. symbols
> > > > that are used purely to create links within an overlay - perhaps using a
> > > > particular naming convention? This would make it easier to instantiate an
> > > > overlay multiple times without having to uniquify all symbols, and it would
> > > > avoid polluting the global namespace without reason.
> > > 
> > > I'd really prefer not to add yet more features and complexity to the
> > > existing shoddily designed overlay mechanism.  I'd prefer for people
> > > to focus on a better replacement - which includes considering these
> > > sorts of use cases and how to handle them.
> > 
> > Can we go for incremental progress over time instead of replacing what
> > was finally accepted?  Maybe one or two concrete examples that need
> > improvement upon, and go from there?  Thanks!
> 
> As a general rule, I'd agree with that approach, but not this time.
> Incremental improvements are great if you have a solid base from which
> to work, at the moment we don't.  What we have is a half-arsed hack
> that become widely deployed enough that we have to live with it.

Are you planning to introduce this new design?  I thought the whole
point of finally accepting the current state of overlay support was so
that we could improve it over time, rather than say that we'll have to
re-hash everything all over to add the rest of the functionality that
was expected.

-- 
Tom

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[David Gibson]

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

On Thu, Jul 13, 2017 at 12:31:50PM -0700, Frank Rowand wrote:
> On 07/03/17 02:06, David Gibson wrote:
> > On Wed, Jun 14, 2017 at 05:52:25PM +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.
> > 
> > This seems to be doing things the hard way.
> > 
> > You're essentially extending the semantics of overlay application to
> > add the symbol merging.  You've implemented these extended semantics
> > in libfdt, which is all very well, but that's not the only overlay
> > application implementation.
> > 
> > 
> > It seems to me a better approach would be to change dtc's -@
> > implementation, so that in /plugin/ mode instead of making a global
> > __symbols__ node, it puts it into the individual fragments.  That way
> > the existing overlay application semantics will update the __symbols__
> > node.
> 
> If the __symbols__ node was inside a fragment, then the existing
> code would add (or update) a __symbols__ node located at the location
> pointed to by the fragment's target path, instead of updating the
> node /__symbols__.

No, what I meant was that an overlay can update anything, including
__symbols__, so you could have a fragment which made the __symbols__
updates.

But I realised shortly afterwards that doesn't work, because we don't
know the correct resolved target paths.

> It makes sense to me to have only one global __symbols__ node instead
> of several.

Having realized the above, that does make sense.

> If there is a global __symbols__ node then we have a single name
> space for symbols.
> 
> If there are multiple __symbols__ nodes spread throughout the tree,
> then to me that would imply different name spaces spread throughout
> the tree, where namespaces are determined by fragments.  This sounds
> confusing to me.  Or if the intent is to have a single name space
> then the __symbols__ information would be scattered throughout the
> tree instead of located in a single node.
> 
> My current patch (under review), targeted for Linux 4.13-rc1, puts
> an overlay's __symbols__ node properties into the overlay's
> changeset, so they get added when the overlay is loaded and
> removed when the overlay is unloaded.

True, but not relevant to what I was proposing.

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

[PATCH 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