Re: [PATCH v2 1/2] overlays: auto allocate phandles for nodes in base fdt

Re: [PATCH v2 1/2] overlays: auto allocate phandles for nodes in base fdt

[Kyle Evans]

On Thu, Jan 4, 2018 at 2:11 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On 12/31/17 22:59, kevans-HZy0K5TPuP5AfugRpC6u6w@public.gmane.org wrote:
>> Currently, references cannot be made to nodes in the base that do not already
>> have phandles, limiting us to nodes that have been referenced in the base fdt.
>> Lift that restriction by allocating them on an as-needed basis.
>
> I am very confused.  If I understand correctly, the problem is:
>
>   A base devicetree contains a __symbols__ node, where one of the properties
>   in that node has a value which is a path to a node, where that node does
>   not have a phandle property.
>
> Is that correct?

This is correct. Currently, the overlay support resolves the path to
the node just fine, but then dies when it doesn't have a phandle. This
changes it to allocate that phandle so that it and any further
overlays can properly reference that node.

Re: [PATCH v2 1/2] overlays: auto allocate phandles for nodes in base fdt

[Frank Rowand]

On 01/04/18 12:15, Kyle Evans wrote:
> On Thu, Jan 4, 2018 at 2:11 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> On 12/31/17 22:59, kevans-HZy0K5TPuP5AfugRpC6u6w@public.gmane.org wrote:
>>> Currently, references cannot be made to nodes in the base that do not already
>>> have phandles, limiting us to nodes that have been referenced in the base fdt.
>>> Lift that restriction by allocating them on an as-needed basis.
>>
>> I am very confused.  If I understand correctly, the problem is:
>>
>>   A base devicetree contains a __symbols__ node, where one of the properties
>>   in that node has a value which is a path to a node, where that node does
>>   not have a phandle property.
>>
>> Is that correct?
> 
> This is correct. Currently, the overlay support resolves the path to
> the node just fine, but then dies when it doesn't have a phandle. This
> changes it to allocate that phandle so that it and any further
> overlays can properly reference that node.
> 

Then it appears that the base devicetree was not compiled with the
proper options.  Using the .dts from patch 2/2, I can cause the problem
with:

$ 
dtc -O dts overlay_base_manual_symbols_auto_phandle.dts 
/dts-v1/;

/ {

	test: test-node {
		test-int-property = <0x2a>;
		test-str-property = "foo";

		subtest: sub-test-node {
			sub-test-property;
		};
	};

	__symbols__ {
		test = "/test-node";
	};
};


But if I add the "-@" option, the phandle exists:

$ dtc -@ -O dts overlay_base_manual_symbols_auto_phandle.dts 
WARNING: label test already exists in /__symbols__/dts-v1/;

/ {

	test: test-node {
		test-int-property = <0x2a>;
		test-str-property = "foo";
		phandle = <0x1>;

		subtest: sub-test-node {
			sub-test-property;
			phandle = <0x2>;
		};
	};

	__symbols__ {
		test = "/test-node";
		subtest = "/test-node/sub-test-node";
	};
};


To go even further, there is no need to hand code a __symbols__
node with the current dtc:

$ cat overlay_base_manual_symbols_auto_phandle--no__symbols__.dts 
/*
 * Copyright (c) 2016 NextThing Co
 * Copyright (c) 2016 Free Electrons
 *
 * SPDX-License-Identifier:	GPL-2.0+
 */

/dts-v1/;

/ {
	test: test-node {
		test-int-property = <42>;
		test-str-property = "foo";

		subtest: sub-test-node {
			sub-test-property;
		};
	};
};

$ dtc -@ -O dts overlay_base_manual_symbols_auto_phandle--no__symbols__.dts 
/dts-v1/;

/ {

	test: test-node {
		test-int-property = <0x2a>;
		test-str-property = "foo";
		phandle = <0x1>;

		subtest: sub-test-node {
			sub-test-property;
			phandle = <0x2>;
		};
	};

	__symbols__ {
		test = "/test-node";
		subtest = "/test-node/sub-test-node";
	};
};


Does this remove the need for the proposed patch, or am I still
missing something?

-Frank

Re: [PATCH v2 1/2] overlays: auto allocate phandles for nodes in base fdt

[Kyle Evans]

On Thu, Jan 4, 2018 at 2:33 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> [... snip ...]
>
> Does this remove the need for the proposed patch, or am I still
> missing something?

... nope. Apparently I never tested this with this particular dtc(1)
and instead just assumed it did the same as ours- allocate phandle
sparsely, even with -@. That certainly removes the need for this
patch, and I'm somewhat upset that I hadn't previously considered
this.

@David, Jon: Please disregard all of the patches along these lines...
I'll fix this in our dtc, where it should be fixed.

Thanks, Frank!

Re: [PATCH v2 1/2] overlays: auto allocate phandles for nodes in base fdt

[Kyle Evans]

On Thu, Jan 4, 2018 at 2:41 PM, Kyle Evans <kevans-h+KGxgPPiopAfugRpC6u6w@public.gmane.org> wrote:
> On Thu, Jan 4, 2018 at 2:33 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> [... snip ...]
>>
>> Does this remove the need for the proposed patch, or am I still
>> missing something?
>
> ... nope. Apparently I never tested this with this particular dtc(1)
> and instead just assumed it did the same as ours- allocate phandle
> sparsely, even with -@. That certainly removes the need for this
> patch, and I'm somewhat upset that I hadn't previously considered
> this.
>
> @David, Jon: Please disregard all of the patches along these lines...
> I'll fix this in our dtc, where it should be fixed.
>
> Thanks, Frank!

Actually, I'm kind of torn on whether this is useful or not. With
being able to have EFI-provided FDT, it's hard to guarantee whether
the FDT we're provided has been compiled with GPL dtc(1) and -@. The
above solves this problem for most of my personal use-cases , though,
since I can guarantee that our FDT and U-Boot provided FDT is compiled
properly.

Re: [PATCH v2 1/2] overlays: auto allocate phandles for nodes in base fdt

[Kyle Evans]

On Thu, Jan 4, 2018 at 3:34 PM, Kyle Evans <kevans-h+KGxgPPiopAfugRpC6u6w@public.gmane.org> wrote:
> On Thu, Jan 4, 2018 at 2:41 PM, Kyle Evans <kevans-h+KGxgPPiopAfugRpC6u6w@public.gmane.org> wrote:
>> On Thu, Jan 4, 2018 at 2:33 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>> [... snip ...]
>>>
>>> Does this remove the need for the proposed patch, or am I still
>>> missing something?
>>
>> ... nope. Apparently I never tested this with this particular dtc(1)
>> and instead just assumed it did the same as ours- allocate phandle
>> sparsely, even with -@. That certainly removes the need for this
>> patch, and I'm somewhat upset that I hadn't previously considered
>> this.
>>
>> @David, Jon: Please disregard all of the patches along these lines...
>> I'll fix this in our dtc, where it should be fixed.
>>
>> Thanks, Frank!
>
> Actually, I'm kind of torn on whether this is useful or not. With
> being able to have EFI-provided FDT, it's hard to guarantee whether
> the FDT we're provided has been compiled with GPL dtc(1) and -@. The
> above solves this problem for most of my personal use-cases , though,
> since I can guarantee that our FDT and U-Boot provided FDT is compiled
> properly.

Apologies for the triple post; I realized that this argument is
inherently wrong, since we can't reference the node if there's no
symbol anyways.

The only way this might still be a good idea is to support more
minimal cases where an implementation might prefer to not create a
phandle for nodes that haven't been referenced.

In our case, we have a function [1] that walks the tree and generates
metadata on nodes that have phandles, under the assumption that these
have been referenced somewhere and provides a way to more quickly
reference these specifically through a separate linked link.
Allocating phandles for everything as GPL dtc does adds quite a bit
more overhead to this.

[1] http://src.illumos.org/source/xref/freebsd-head/sys/dev/ofw/openfirm.c#119

Re: [PATCH v2 1/2] overlays: auto allocate phandles for nodes in base fdt

[Ian Lepore]

On Thu, 2018-01-04 at 15:47 -0600, Kyle Evans wrote:
> On Thu, Jan 4, 2018 at 3:34 PM, Kyle Evans <kevans-h+KGxgPPiopAfugRpC6u6w@public.gmane.org> wrote:
> > 
> > On Thu, Jan 4, 2018 at 2:41 PM, Kyle Evans <kevans-h+KGxgPPiopAfugRpC6u6w@public.gmane.org> wrote:
> > > 
> > > On Thu, Jan 4, 2018 at 2:33 PM, Frank Rowand  wrote:
> > > > 
> > > > [... snip ...]
> > > > 
> > > > Does this remove the need for the proposed patch, or am I still
> > > > missing something?
> > > ... nope. Apparently I never tested this with this particular dtc(1)
> > > and instead just assumed it did the same as ours- allocate phandle
> > > sparsely, even with -@. That certainly removes the need for this
> > > patch, and I'm somewhat upset that I hadn't previously considered
> > > this.
> > > 
> > > @David, Jon: Please disregard all of the patches along these lines...
> > > I'll fix this in our dtc, where it should be fixed.
> > > 
> > > Thanks, Frank!
> > Actually, I'm kind of torn on whether this is useful or not. With
> > being able to have EFI-provided FDT, it's hard to guarantee whether
> > the FDT we're provided has been compiled with GPL dtc(1) and -@. The
> > above solves this problem for most of my personal use-cases , though,
> > since I can guarantee that our FDT and U-Boot provided FDT is compiled
> > properly.
> Apologies for the triple post; I realized that this argument is
> inherently wrong, since we can't reference the node if there's no
> symbol anyways.
> 
> The only way this might still be a good idea is to support more
> minimal cases where an implementation might prefer to not create a
> phandle for nodes that haven't been referenced.
> 
> In our case, we have a function [1] that walks the tree and generates
> metadata on nodes that have phandles, under the assumption that these
> have been referenced somewhere and provides a way to more quickly
> reference these specifically through a separate linked link.
> Allocating phandles for everything as GPL dtc does adds quite a bit
> more overhead to this.
> 
> [1] http://src.illumos.org/source/xref/freebsd-head/sys/dev/ofw/openfirm.c#119

In particular, it makes lookups more expensive as it now must traverse
a list that includes every node in the dtb, rather than just nodes that
are actually referenced.  (It also increases the amount of storage, but
at 20-ish bytes per node, that's not a big deal.)

-- Ian

Re: [PATCH v2 1/2] overlays: auto allocate phandles for nodes in base fdt

[Frank Rowand]

On 01/04/18 13:47, Kyle Evans wrote:
> On Thu, Jan 4, 2018 at 3:34 PM, Kyle Evans <kevans-h+KGxgPPiopAfugRpC6u6w@public.gmane.org> wrote:
>> On Thu, Jan 4, 2018 at 2:41 PM, Kyle Evans <kevans-h+KGxgPPiopAfugRpC6u6w@public.gmane.org> wrote:
>>> On Thu, Jan 4, 2018 at 2:33 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>> [... snip ...]
>>>>
>>>> Does this remove the need for the proposed patch, or am I still
>>>> missing something?
>>>
>>> ... nope. Apparently I never tested this with this particular dtc(1)
>>> and instead just assumed it did the same as ours- allocate phandle
>>> sparsely, even with -@. That certainly removes the need for this
>>> patch, and I'm somewhat upset that I hadn't previously considered
>>> this.
>>>
>>> @David, Jon: Please disregard all of the patches along these lines...
>>> I'll fix this in our dtc, where it should be fixed.
>>>
>>> Thanks, Frank!
>>
>> Actually, I'm kind of torn on whether this is useful or not. With
>> being able to have EFI-provided FDT, it's hard to guarantee whether
>> the FDT we're provided has been compiled with GPL dtc(1) and -@. The
>> above solves this problem for most of my personal use-cases , though,
>> since I can guarantee that our FDT and U-Boot provided FDT is compiled
>> properly.
> 
> Apologies for the triple post; I realized that this argument is
> inherently wrong, since we can't reference the node if there's no
> symbol anyways.
> 
> The only way this might still be a good idea is to support more
> minimal cases where an implementation might prefer to not create a
> phandle for nodes that haven't been referenced.
> 
> In our case, we have a function [1] that walks the tree and generates
> metadata on nodes that have phandles, under the assumption that these
> have been referenced somewhere and provides a way to more quickly
> reference these specifically through a separate linked link.
> Allocating phandles for everything as GPL dtc does adds quite a bit
> more overhead to this.
> 
> [1] http://src.illumos.org/source/xref/freebsd-head/sys/dev/ofw/openfirm.c#119

The "-@" option does not add a phandle to _every_ node, just to nodes
that have a label:

$ cat overlay_base_manual_symbols_auto_phandle--no__symbols__add_a_node.dts
/*
 * Copyright (c) 2016 NextThing Co
 * Copyright (c) 2016 Free Electrons
 *
 * SPDX-License-Identifier:	GPL-2.0+
 */

/dts-v1/;

/ {
	test: test-node {
		test-int-property = <42>;
		test-str-property = "foo";

		subtest: sub-test-node {
			sub-test-property;
		};

		sub-test-node-2 {
			sub-test-property;
		};

		sub-test-node-3 {
			sub-test-property;
		};
	};
};


$ dtc -@ -O dts overlay_base_manual_symbols_auto_phandle--no__symbols__add_a_node.dts
/dts-v1/;

/ {

	test: test-node {
		test-int-property = <0x2a>;
		test-str-property = "foo";
		phandle = <0x1>;

		subtest: sub-test-node {
			sub-test-property;
			phandle = <0x2>;
		};

		sub-test-node-2 {
			sub-test-property;
		};

		sub-test-node-3 {
			sub-test-property;
		};
	};

	__symbols__ {
		test = "/test-node";
		subtest = "/test-node/sub-test-node";
	};
};

Re: [PATCH v2 1/2] overlays: auto allocate phandles for nodes in base fdt

[Kyle Evans]

On Thu, Jan 4, 2018 at 7:55 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On 01/04/18 13:47, Kyle Evans wrote:
>> On Thu, Jan 4, 2018 at 3:34 PM, Kyle Evans <kevans-h+KGxgPPiopAfugRpC6u6w@public.gmane.org> wrote:
>>> On Thu, Jan 4, 2018 at 2:41 PM, Kyle Evans <kevans-h+KGxgPPiopAfugRpC6u6w@public.gmane.org> wrote:
>>>> On Thu, Jan 4, 2018 at 2:33 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>>> [... snip ...]
>>>>>
>>>>> Does this remove the need for the proposed patch, or am I still
>>>>> missing something?
>>>>
>>>> ... nope. Apparently I never tested this with this particular dtc(1)
>>>> and instead just assumed it did the same as ours- allocate phandle
>>>> sparsely, even with -@. That certainly removes the need for this
>>>> patch, and I'm somewhat upset that I hadn't previously considered
>>>> this.
>>>>
>>>> @David, Jon: Please disregard all of the patches along these lines...
>>>> I'll fix this in our dtc, where it should be fixed.
>>>>
>>>> Thanks, Frank!
>>>
>>> Actually, I'm kind of torn on whether this is useful or not. With
>>> being able to have EFI-provided FDT, it's hard to guarantee whether
>>> the FDT we're provided has been compiled with GPL dtc(1) and -@. The
>>> above solves this problem for most of my personal use-cases , though,
>>> since I can guarantee that our FDT and U-Boot provided FDT is compiled
>>> properly.
>>
>> Apologies for the triple post; I realized that this argument is
>> inherently wrong, since we can't reference the node if there's no
>> symbol anyways.
>>
>> The only way this might still be a good idea is to support more
>> minimal cases where an implementation might prefer to not create a
>> phandle for nodes that haven't been referenced.
>>
>> In our case, we have a function [1] that walks the tree and generates
>> metadata on nodes that have phandles, under the assumption that these
>> have been referenced somewhere and provides a way to more quickly
>> reference these specifically through a separate linked link.
>> Allocating phandles for everything as GPL dtc does adds quite a bit
>> more overhead to this.
>>
>> [1] http://src.illumos.org/source/xref/freebsd-head/sys/dev/ofw/openfirm.c#119
>
> The "-@" option does not add a phandle to _every_ node, just to nodes
> that have a label:
>

In practice, this is still a not necessarily close superset of those
that are actually going to actually get referenced in a given .dts. I
note that a number of nodes will still get labelled that are unlikely
to be referenced for any number of reasons.

For instance, as an example [1][2][3] of one of the boards I'm working
on currently, all of the ehci/ohci/mmc nodes, some set of the pio
nodes... almost every single node has a label, and most of them don't
need a phandle based on what is currently referenced and actually
used. I note that 27 of these nodes seem to be referenced, out of 39
nodes with labels (numbers are approximate), leaving ~30% (12) of
labelled nodes unreferenced.

For a slightly larger example, [4][5][6][7]; 74 referenced nodes out
of 113 labelled nodes, leaving ~35% (39) of labelled nodes
unreferenced.

This is only bothersome because it starts adding up as these things
get bigger, and I don't think it really needs to. The alternative to
keep phandles down to the minimum set required doesn't add much in
maintenance cost or overhead from the overlay application side;
especially for blobs generated by this dtc(1) that make the active
decision to allocate liberally rather than conservatively.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/sun8i-a83t.dtsi
[3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/sunxi-common-regulators.dtsi

[4] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/am335x-bonegreen.dts
[5] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/am33xx.dtsi
[6] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/am335x-bone-common.dtsi
[7] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/am335x-bonegreen-common.dtsi

Re: [PATCH v2 1/2] overlays: auto allocate phandles for nodes in base fdt

[Frank Rowand]

On 01/04/18 18:39, Kyle Evans wrote:
> On Thu, Jan 4, 2018 at 7:55 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> On 01/04/18 13:47, Kyle Evans wrote:
>>> On Thu, Jan 4, 2018 at 3:34 PM, Kyle Evans <kevans-h+KGxgPPiopAfugRpC6u6w@public.gmane.org> wrote:
>>>> On Thu, Jan 4, 2018 at 2:41 PM, Kyle Evans <kevans-h+KGxgPPiopAfugRpC6u6w@public.gmane.org> wrote:
>>>>> On Thu, Jan 4, 2018 at 2:33 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>>>> [... snip ...]
>>>>>>
>>>>>> Does this remove the need for the proposed patch, or am I still
>>>>>> missing something?
>>>>>
>>>>> ... nope. Apparently I never tested this with this particular dtc(1)
>>>>> and instead just assumed it did the same as ours- allocate phandle
>>>>> sparsely, even with -@. That certainly removes the need for this
>>>>> patch, and I'm somewhat upset that I hadn't previously considered
>>>>> this.
>>>>>
>>>>> @David, Jon: Please disregard all of the patches along these lines...
>>>>> I'll fix this in our dtc, where it should be fixed.
>>>>>
>>>>> Thanks, Frank!
>>>>
>>>> Actually, I'm kind of torn on whether this is useful or not. With
>>>> being able to have EFI-provided FDT, it's hard to guarantee whether
>>>> the FDT we're provided has been compiled with GPL dtc(1) and -@. The
>>>> above solves this problem for most of my personal use-cases , though,
>>>> since I can guarantee that our FDT and U-Boot provided FDT is compiled
>>>> properly.
>>>
>>> Apologies for the triple post; I realized that this argument is
>>> inherently wrong, since we can't reference the node if there's no
>>> symbol anyways.
>>>
>>> The only way this might still be a good idea is to support more
>>> minimal cases where an implementation might prefer to not create a
>>> phandle for nodes that haven't been referenced.
>>>
>>> In our case, we have a function [1] that walks the tree and generates
>>> metadata on nodes that have phandles, under the assumption that these
>>> have been referenced somewhere and provides a way to more quickly
>>> reference these specifically through a separate linked link.
>>> Allocating phandles for everything as GPL dtc does adds quite a bit
>>> more overhead to this.
>>>
>>> [1] http://src.illumos.org/source/xref/freebsd-head/sys/dev/ofw/openfirm.c#119
>>
>> The "-@" option does not add a phandle to _every_ node, just to nodes
>> that have a label:
>>
> 
> In practice, this is still a not necessarily close superset of those
> that are actually going to actually get referenced in a given .dts. I
> note that a number of nodes will still get labelled that are unlikely
> to be referenced for any number of reasons.
> 
> For instance, as an example [1][2][3] of one of the boards I'm working
> on currently, all of the ehci/ohci/mmc nodes, some set of the pio
> nodes... almost every single node has a label, and most of them don't
> need a phandle based on what is currently referenced and actually
> used. I note that 27 of these nodes seem to be referenced, out of 39
> nodes with labels (numbers are approximate), leaving ~30% (12) of
> labelled nodes unreferenced.
> 
> For a slightly larger example, [4][5][6][7]; 74 referenced nodes out
> of 113 labelled nodes, leaving ~35% (39) of labelled nodes
> unreferenced.
> 
> This is only bothersome because it starts adding up as these things
> get bigger, and I don't think it really needs to. The alternative to
> keep phandles down to the minimum set required doesn't add much in
> maintenance cost or overhead from the overlay application side;
> especially for blobs generated by this dtc(1) that make the active
> decision to allocate liberally rather than conservatively.

This concept relies on a hand coded __symbols__ node instead of
having dtc generate it.  This makes the overlay devicetree source
more fragile.  The example dts file in patch 2/2 is fairly safe,
but it is legal syntax to specify:

  test = "/test-node";

instead of:

  test = &test;

And the fragile syntax is exactly what results from a decompiled
overlay dtb.

On another note, in the case of Linux, I have to consider applying
overlays to a live devicetree, as well as possibly the case of
applying overlays to a flattened device tree.  I do not expect
that we will add the dynamic creation of missing phandles when
applying an overlay to a live devicetree.  This would result in
an overlay that can be applied to a flattened device tree, but
will fail to apply to a Linux live devicetree.  BSD and Linux
do not have to be fully compatible, but in my opinion, the more
compatible the better.

-Frank 

> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/sun8i-a83t.dtsi
> [3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/sunxi-common-regulators.dtsi
> 
> [4] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/am335x-bonegreen.dts
> [5] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/am33xx.dtsi
> [6] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/am335x-bone-common.dtsi
> [7] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/am335x-bonegreen-common.dtsi
> 

Re: [PATCH v2 1/2] overlays: auto allocate phandles for nodes in base fdt

[David Gibson]

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

On Thu, Jan 04, 2018 at 02:41:22PM -0600, Kyle Evans wrote:
> On Thu, Jan 4, 2018 at 2:33 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > [... snip ...]
> >
> > Does this remove the need for the proposed patch, or am I still
> > missing something?
> 
> ... nope. Apparently I never tested this with this particular dtc(1)
> and instead just assumed it did the same as ours- allocate phandle
> sparsely, even with -@. That certainly removes the need for this
> patch, and I'm somewhat upset that I hadn't previously considered
> this.

Ah! Sorry, I'd forgotten about that behaviour of -@.

> @David, Jon: Please disregard all of the patches along these lines...
> I'll fix this in our dtc, where it should be fixed.

Ok :).

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/2] overlays: auto allocate phandles for nodes in base fdt

[Kyle Evans]

On Thu, Jan 4, 2018 at 9:14 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On 01/04/18 18:39, Kyle Evans wrote:
>> On Thu, Jan 4, 2018 at 7:55 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>> On 01/04/18 13:47, Kyle Evans wrote:
>>>> On Thu, Jan 4, 2018 at 3:34 PM, Kyle Evans <kevans-h+KGxgPPiopAfugRpC6u6w@public.gmane.org> wrote:
>>>>> On Thu, Jan 4, 2018 at 2:41 PM, Kyle Evans <kevans-h+KGxgPPiopAfugRpC6u6w@public.gmane.org> wrote:
>>>>>> On Thu, Jan 4, 2018 at 2:33 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>>>>> [... snip ...]
>>>>>>>
>>>>>>> Does this remove the need for the proposed patch, or am I still
>>>>>>> missing something?
>>>>>>
>>>>>> ... nope. Apparently I never tested this with this particular dtc(1)
>>>>>> and instead just assumed it did the same as ours- allocate phandle
>>>>>> sparsely, even with -@. That certainly removes the need for this
>>>>>> patch, and I'm somewhat upset that I hadn't previously considered
>>>>>> this.
>>>>>>
>>>>>> @David, Jon: Please disregard all of the patches along these lines...
>>>>>> I'll fix this in our dtc, where it should be fixed.
>>>>>>
>>>>>> Thanks, Frank!
>>>>>
>>>>> Actually, I'm kind of torn on whether this is useful or not. With
>>>>> being able to have EFI-provided FDT, it's hard to guarantee whether
>>>>> the FDT we're provided has been compiled with GPL dtc(1) and -@. The
>>>>> above solves this problem for most of my personal use-cases , though,
>>>>> since I can guarantee that our FDT and U-Boot provided FDT is compiled
>>>>> properly.
>>>>
>>>> Apologies for the triple post; I realized that this argument is
>>>> inherently wrong, since we can't reference the node if there's no
>>>> symbol anyways.
>>>>
>>>> The only way this might still be a good idea is to support more
>>>> minimal cases where an implementation might prefer to not create a
>>>> phandle for nodes that haven't been referenced.
>>>>
>>>> In our case, we have a function [1] that walks the tree and generates
>>>> metadata on nodes that have phandles, under the assumption that these
>>>> have been referenced somewhere and provides a way to more quickly
>>>> reference these specifically through a separate linked link.
>>>> Allocating phandles for everything as GPL dtc does adds quite a bit
>>>> more overhead to this.
>>>>
>>>> [1] http://src.illumos.org/source/xref/freebsd-head/sys/dev/ofw/openfirm.c#119
>>>
>>> The "-@" option does not add a phandle to _every_ node, just to nodes
>>> that have a label:
>>>
>>
>> In practice, this is still a not necessarily close superset of those
>> that are actually going to actually get referenced in a given .dts. I
>> note that a number of nodes will still get labelled that are unlikely
>> to be referenced for any number of reasons.
>>
>> For instance, as an example [1][2][3] of one of the boards I'm working
>> on currently, all of the ehci/ohci/mmc nodes, some set of the pio
>> nodes... almost every single node has a label, and most of them don't
>> need a phandle based on what is currently referenced and actually
>> used. I note that 27 of these nodes seem to be referenced, out of 39
>> nodes with labels (numbers are approximate), leaving ~30% (12) of
>> labelled nodes unreferenced.
>>
>> For a slightly larger example, [4][5][6][7]; 74 referenced nodes out
>> of 113 labelled nodes, leaving ~35% (39) of labelled nodes
>> unreferenced.
>>
>> This is only bothersome because it starts adding up as these things
>> get bigger, and I don't think it really needs to. The alternative to
>> keep phandles down to the minimum set required doesn't add much in
>> maintenance cost or overhead from the overlay application side;
>> especially for blobs generated by this dtc(1) that make the active
>> decision to allocate liberally rather than conservatively.
>
> This concept relies on a hand coded __symbols__ node instead of
> having dtc generate it.  This makes the overlay devicetree source
> more fragile.  The example dts file in patch 2/2 is fairly safe,
> but it is legal syntax to specify:
>
>   test = "/test-node";
>
> instead of:
>
>   test = &test;
>
> And the fragile syntax is exactly what results from a decompiled
> overlay dtb.

I think I might be misunderstanding something here- our dtc (BSDL
dtc), with -@, still writes a full /__symbols__ node of all labeled
nodes, given that this is what -@ is expected to do. The difference is
that our dtc allocates phandles conservatively because they're a
concept for cross-referencing nodes. Conceptually, it doesn't seem
like either way is more or less wrong than the other.

> On another note, in the case of Linux, I have to consider applying
> overlays to a live devicetree, as well as possibly the case of
> applying overlays to a flattened device tree.  I do not expect
> that we will add the dynamic creation of missing phandles when
> applying an overlay to a live devicetree.  This would result in
> an overlay that can be applied to a flattened device tree, but
> will fail to apply to a Linux live devicetree.  BSD and Linux
> do not have to be fully compatible, but in my opinion, the more
> compatible the better.

I'm probably being dense here, so please excuse me, but I don't see
this as necessarily a problem- likely because I have no familiarity of
how Linux works with overlays or FDT bits in general. My stupid
question of the hour: Linux actually supports applying overlays to
live devicetree?

Our overlay support effectively ends at the bootloader. We load fdt
blob from the filesystem or pull it into bootloader memory from
EFI/U-Boot, apply any overlays, then pass it into the kernel. After
that, it's effectively immutable (IIRC; ian@ will have to clarify if
I'm wrong) and we don't support reloading FDT on a live system.

Re: [PATCH v2 1/2] overlays: auto allocate phandles for nodes in base fdt

[David Gibson]

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

On Thu, Jan 04, 2018 at 03:08:08PM -0700, Ian Lepore wrote:
> On Thu, 2018-01-04 at 15:47 -0600, Kyle Evans wrote:
> > On Thu, Jan 4, 2018 at 3:34 PM, Kyle Evans <kevans-h+KGxgPPiopAfugRpC6u6w@public.gmane.org> wrote:
> > > 
> > > On Thu, Jan 4, 2018 at 2:41 PM, Kyle Evans <kevans-h+KGxgPPiopAfugRpC6u6w@public.gmane.org> wrote:
> > > > 
> > > > On Thu, Jan 4, 2018 at 2:33 PM, Frank Rowand  wrote:
> > > > > 
> > > > > [... snip ...]
> > > > > 
> > > > > Does this remove the need for the proposed patch, or am I still
> > > > > missing something?
> > > > ... nope. Apparently I never tested this with this particular dtc(1)
> > > > and instead just assumed it did the same as ours- allocate phandle
> > > > sparsely, even with -@. That certainly removes the need for this
> > > > patch, and I'm somewhat upset that I hadn't previously considered
> > > > this.
> > > > 
> > > > @David, Jon: Please disregard all of the patches along these lines...
> > > > I'll fix this in our dtc, where it should be fixed.
> > > > 
> > > > Thanks, Frank!
> > > Actually, I'm kind of torn on whether this is useful or not. With
> > > being able to have EFI-provided FDT, it's hard to guarantee whether
> > > the FDT we're provided has been compiled with GPL dtc(1) and -@. The
> > > above solves this problem for most of my personal use-cases , though,
> > > since I can guarantee that our FDT and U-Boot provided FDT is compiled
> > > properly.
> > Apologies for the triple post; I realized that this argument is
> > inherently wrong, since we can't reference the node if there's no
> > symbol anyways.
> > 
> > The only way this might still be a good idea is to support more
> > minimal cases where an implementation might prefer to not create a
> > phandle for nodes that haven't been referenced.
> > 
> > In our case, we have a function [1] that walks the tree and generates
> > metadata on nodes that have phandles, under the assumption that these
> > have been referenced somewhere and provides a way to more quickly
> > reference these specifically through a separate linked link.
> > Allocating phandles for everything as GPL dtc does adds quite a bit
> > more overhead to this.
> > 
> > [1] http://src.illumos.org/source/xref/freebsd-head/sys/dev/ofw/openfirm.c#119
> 
> In particular, it makes lookups more expensive as it now must traverse
> a list that includes every node in the dtb, rather than just nodes that
> are actually referenced.  (It also increases the amount of storage, but
> at 20-ish bytes per node, that's not a big deal.)

Lookups of what exactly?  Aren't you unflattening the tree after you
read it in?

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/2] overlays: auto allocate phandles for nodes in base fdt

[Kyle Evans]

On Thu, Jan 4, 2018 at 9:40 PM, David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Thu, Jan 04, 2018 at 02:41:22PM -0600, Kyle Evans wrote:
>> On Thu, Jan 4, 2018 at 2:33 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> > [... snip ...]
>> >
>> > Does this remove the need for the proposed patch, or am I still
>> > missing something?
>>
>> ... nope. Apparently I never tested this with this particular dtc(1)
>> and instead just assumed it did the same as ours- allocate phandle
>> sparsely, even with -@. That certainly removes the need for this
>> patch, and I'm somewhat upset that I hadn't previously considered
>> this.
>
> Ah! Sorry, I'd forgotten about that behaviour of -@.
>
>> @David, Jon: Please disregard all of the patches along these lines...
>> I'll fix this in our dtc, where it should be fixed.
>
> Ok :).

Heh, sorry for the flip-flop, but this might have been premature. I'm
still trying to decide based on Frank's responses if it's really a bad
idea to support this in libfdt, given that I don't think we have any
intention of supporting such a thing on a running devicetree. It would
certainly help if there were some spec mandating that all nodes
eligible for cross-referencing have a phandle, but even [1] (albeit
recently published) doesn't have a strong position on this.

[1] https://github.com/devicetree-org/devicetree-specification/releases/download/v0.2/devicetree-specification-v0.2.pdf

Re: [PATCH v2 1/2] overlays: auto allocate phandles for nodes in base fdt

[Kyle Evans]

On Thu, Jan 4, 2018 at 9:53 PM, David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Thu, Jan 04, 2018 at 03:08:08PM -0700, Ian Lepore wrote:
>> On Thu, 2018-01-04 at 15:47 -0600, Kyle Evans wrote:
>> > On Thu, Jan 4, 2018 at 3:34 PM, Kyle Evans <kevans-h+KGxgPPiopAfugRpC6u6w@public.gmane.org> wrote:
>> > >
>> > > On Thu, Jan 4, 2018 at 2:41 PM, Kyle Evans <kevans-h+KGxgPPiopAfugRpC6u6w@public.gmane.org> wrote:
>> > > >
>> > > > On Thu, Jan 4, 2018 at 2:33 PM, Frank Rowand  wrote:
>> > > > >
>> > > > > [... snip ...]
>> > > > >
>> > > > > Does this remove the need for the proposed patch, or am I still
>> > > > > missing something?
>> > > > ... nope. Apparently I never tested this with this particular dtc(1)
>> > > > and instead just assumed it did the same as ours- allocate phandle
>> > > > sparsely, even with -@. That certainly removes the need for this
>> > > > patch, and I'm somewhat upset that I hadn't previously considered
>> > > > this.
>> > > >
>> > > > @David, Jon: Please disregard all of the patches along these lines...
>> > > > I'll fix this in our dtc, where it should be fixed.
>> > > >
>> > > > Thanks, Frank!
>> > > Actually, I'm kind of torn on whether this is useful or not. With
>> > > being able to have EFI-provided FDT, it's hard to guarantee whether
>> > > the FDT we're provided has been compiled with GPL dtc(1) and -@. The
>> > > above solves this problem for most of my personal use-cases , though,
>> > > since I can guarantee that our FDT and U-Boot provided FDT is compiled
>> > > properly.
>> > Apologies for the triple post; I realized that this argument is
>> > inherently wrong, since we can't reference the node if there's no
>> > symbol anyways.
>> >
>> > The only way this might still be a good idea is to support more
>> > minimal cases where an implementation might prefer to not create a
>> > phandle for nodes that haven't been referenced.
>> >
>> > In our case, we have a function [1] that walks the tree and generates
>> > metadata on nodes that have phandles, under the assumption that these
>> > have been referenced somewhere and provides a way to more quickly
>> > reference these specifically through a separate linked link.
>> > Allocating phandles for everything as GPL dtc does adds quite a bit
>> > more overhead to this.
>> >
>> > [1] http://src.illumos.org/source/xref/freebsd-head/sys/dev/ofw/openfirm.c#119
>>
>> In particular, it makes lookups more expensive as it now must traverse
>> a list that includes every node in the dtb, rather than just nodes that
>> are actually referenced.  (It also increases the amount of storage, but
>> at 20-ish bytes per node, that's not a big deal.)
>
> Lookups of what exactly?  Aren't you unflattening the tree after you
> read it in?

Lookup in this context would be a lookup of the device from the xref
phandle, see OF_device_from_xref [1] and its inverse
OF_xref_from_device right below it. Devices, as they attach, register
for the xref phandle, then consumers lookup the device associated with
it and generally hold a handle to the device afterwards.

[1] http://src.illumos.org/source/xref/freebsd-head/sys/dev/ofw/openfirm.c#628

Re: [PATCH v2 1/2] overlays: auto allocate phandles for nodes in base fdt

[Frank Rowand]

On 01/04/18 19:50, Kyle Evans wrote:
> On Thu, Jan 4, 2018 at 9:14 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> On 01/04/18 18:39, Kyle Evans wrote:
>>> On Thu, Jan 4, 2018 at 7:55 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>> On 01/04/18 13:47, Kyle Evans wrote:
>>>>> On Thu, Jan 4, 2018 at 3:34 PM, Kyle Evans <kevans-h+KGxgPPiopAfugRpC6u6w@public.gmane.org> wrote:
>>>>>> On Thu, Jan 4, 2018 at 2:41 PM, Kyle Evans <kevans-h+KGxgPPiopAfugRpC6u6w@public.gmane.org> wrote:
>>>>>>> On Thu, Jan 4, 2018 at 2:33 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>>>>>> [... snip ...]
>>>>>>>>
>>>>>>>> Does this remove the need for the proposed patch, or am I still
>>>>>>>> missing something?
>>>>>>>
>>>>>>> ... nope. Apparently I never tested this with this particular dtc(1)
>>>>>>> and instead just assumed it did the same as ours- allocate phandle
>>>>>>> sparsely, even with -@. That certainly removes the need for this
>>>>>>> patch, and I'm somewhat upset that I hadn't previously considered
>>>>>>> this.
>>>>>>>
>>>>>>> @David, Jon: Please disregard all of the patches along these lines...
>>>>>>> I'll fix this in our dtc, where it should be fixed.
>>>>>>>
>>>>>>> Thanks, Frank!
>>>>>>
>>>>>> Actually, I'm kind of torn on whether this is useful or not. With
>>>>>> being able to have EFI-provided FDT, it's hard to guarantee whether
>>>>>> the FDT we're provided has been compiled with GPL dtc(1) and -@. The
>>>>>> above solves this problem for most of my personal use-cases , though,
>>>>>> since I can guarantee that our FDT and U-Boot provided FDT is compiled
>>>>>> properly.
>>>>>
>>>>> Apologies for the triple post; I realized that this argument is
>>>>> inherently wrong, since we can't reference the node if there's no
>>>>> symbol anyways.
>>>>>
>>>>> The only way this might still be a good idea is to support more
>>>>> minimal cases where an implementation might prefer to not create a
>>>>> phandle for nodes that haven't been referenced.
>>>>>
>>>>> In our case, we have a function [1] that walks the tree and generates
>>>>> metadata on nodes that have phandles, under the assumption that these
>>>>> have been referenced somewhere and provides a way to more quickly
>>>>> reference these specifically through a separate linked link.
>>>>> Allocating phandles for everything as GPL dtc does adds quite a bit
>>>>> more overhead to this.
>>>>>
>>>>> [1] http://src.illumos.org/source/xref/freebsd-head/sys/dev/ofw/openfirm.c#119
>>>>
>>>> The "-@" option does not add a phandle to _every_ node, just to nodes
>>>> that have a label:
>>>>
>>>
>>> In practice, this is still a not necessarily close superset of those
>>> that are actually going to actually get referenced in a given .dts. I
>>> note that a number of nodes will still get labelled that are unlikely
>>> to be referenced for any number of reasons.
>>>
>>> For instance, as an example [1][2][3] of one of the boards I'm working
>>> on currently, all of the ehci/ohci/mmc nodes, some set of the pio
>>> nodes... almost every single node has a label, and most of them don't
>>> need a phandle based on what is currently referenced and actually
>>> used. I note that 27 of these nodes seem to be referenced, out of 39
>>> nodes with labels (numbers are approximate), leaving ~30% (12) of
>>> labelled nodes unreferenced.
>>>
>>> For a slightly larger example, [4][5][6][7]; 74 referenced nodes out
>>> of 113 labelled nodes, leaving ~35% (39) of labelled nodes
>>> unreferenced.
>>>
>>> This is only bothersome because it starts adding up as these things
>>> get bigger, and I don't think it really needs to. The alternative to
>>> keep phandles down to the minimum set required doesn't add much in
>>> maintenance cost or overhead from the overlay application side;
>>> especially for blobs generated by this dtc(1) that make the active
>>> decision to allocate liberally rather than conservatively.
>>
>> This concept relies on a hand coded __symbols__ node instead of
>> having dtc generate it.  This makes the overlay devicetree source
>> more fragile.  The example dts file in patch 2/2 is fairly safe,
>> but it is legal syntax to specify:
>>
>>   test = "/test-node";
>>
>> instead of:
>>
>>   test = &test;
>>
>> And the fragile syntax is exactly what results from a decompiled
>> overlay dtb.
> 
> I think I might be misunderstanding something here- our dtc (BSDL
> dtc), with -@, still writes a full /__symbols__ node of all labeled
> nodes, given that this is what -@ is expected to do. The difference is

I was talking about the .dts file in patch 2/2, where the __symbols__
node was hand coded, instead of being created by dtc.


> that our dtc allocates phandles conservatively because they're a
> concept for cross-referencing nodes. Conceptually, it doesn't seem
> like either way is more or less wrong than the other.

Yes, the phandles concept is for cross-referencing.  In your specific
context, creating phandle properties at the time that applying the
overlay determines that the cross-reference actually occurs does
allow dynamically creating the phandle properties.  If overlays
were only applied to flattened device trees, then that could be
a useful optimization.


>> On another note, in the case of Linux, I have to consider applying
>> overlays to a live devicetree, as well as possibly the case of
>> applying overlays to a flattened device tree.  I do not expect
>> that we will add the dynamic creation of missing phandles when
>> applying an overlay to a live devicetree.  This would result in
>> an overlay that can be applied to a flattened device tree, but
>> will fail to apply to a Linux live devicetree.  BSD and Linux
>> do not have to be fully compatible, but in my opinion, the more
>> compatible the better.
> 
> I'm probably being dense here, so please excuse me, but I don't see
> this as necessarily a problem- likely because I have no familiarity of
> how Linux works with overlays or FDT bits in general. My stupid
> question of the hour: Linux actually supports applying overlays to
> live devicetree?

Not fully implemented yet.  In fact there are some pretty important
issues to be resolved or implemented.  But there are people who
have implemented enough of it out of mainline to be able to
successfully apply specific overlays to the live devicetree for
specific use cases.  There is also the concept of removing an
overlay from the live devicetree at some point after having
applied it.


> Our overlay support effectively ends at the bootloader. We load fdt
> blob from the filesystem or pull it into bootloader memory from
> EFI/U-Boot, apply any overlays, then pass it into the kernel. After
> that, it's effectively immutable (IIRC; ian@ will have to clarify if
> I'm wrong) and we don't support reloading FDT on a live system.
> 

Re: [PATCH v2 1/2] overlays: auto allocate phandles for nodes in base fdt

[Kyle Evans]

On Thu, Jan 4, 2018 at 11:02 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On 01/04/18 19:50, Kyle Evans wrote:
>> On Thu, Jan 4, 2018 at 9:14 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>> On 01/04/18 18:39, Kyle Evans wrote:
>>>> On Thu, Jan 4, 2018 at 7:55 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>>> On 01/04/18 13:47, Kyle Evans wrote:
>>>>>> On Thu, Jan 4, 2018 at 3:34 PM, Kyle Evans <kevans-h+KGxgPPiopAfugRpC6u6w@public.gmane.org> wrote:
>>>>>>> On Thu, Jan 4, 2018 at 2:41 PM, Kyle Evans <kevans-h+KGxgPPiopAfugRpC6u6w@public.gmane.org> wrote:
>>>>>>>> On Thu, Jan 4, 2018 at 2:33 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>>>>>>> [... snip ...]
>>>>>>>>>
>>>>>>>>> Does this remove the need for the proposed patch, or am I still
>>>>>>>>> missing something?
>>>>>>>>
>>>>>>>> ... nope. Apparently I never tested this with this particular dtc(1)
>>>>>>>> and instead just assumed it did the same as ours- allocate phandle
>>>>>>>> sparsely, even with -@. That certainly removes the need for this
>>>>>>>> patch, and I'm somewhat upset that I hadn't previously considered
>>>>>>>> this.
>>>>>>>>
>>>>>>>> @David, Jon: Please disregard all of the patches along these lines...
>>>>>>>> I'll fix this in our dtc, where it should be fixed.
>>>>>>>>
>>>>>>>> Thanks, Frank!
>>>>>>>
>>>>>>> Actually, I'm kind of torn on whether this is useful or not. With
>>>>>>> being able to have EFI-provided FDT, it's hard to guarantee whether
>>>>>>> the FDT we're provided has been compiled with GPL dtc(1) and -@. The
>>>>>>> above solves this problem for most of my personal use-cases , though,
>>>>>>> since I can guarantee that our FDT and U-Boot provided FDT is compiled
>>>>>>> properly.
>>>>>>
>>>>>> Apologies for the triple post; I realized that this argument is
>>>>>> inherently wrong, since we can't reference the node if there's no
>>>>>> symbol anyways.
>>>>>>
>>>>>> The only way this might still be a good idea is to support more
>>>>>> minimal cases where an implementation might prefer to not create a
>>>>>> phandle for nodes that haven't been referenced.
>>>>>>
>>>>>> In our case, we have a function [1] that walks the tree and generates
>>>>>> metadata on nodes that have phandles, under the assumption that these
>>>>>> have been referenced somewhere and provides a way to more quickly
>>>>>> reference these specifically through a separate linked link.
>>>>>> Allocating phandles for everything as GPL dtc does adds quite a bit
>>>>>> more overhead to this.
>>>>>>
>>>>>> [1] http://src.illumos.org/source/xref/freebsd-head/sys/dev/ofw/openfirm.c#119
>>>>>
>>>>> The "-@" option does not add a phandle to _every_ node, just to nodes
>>>>> that have a label:
>>>>>
>>>>
>>>> In practice, this is still a not necessarily close superset of those
>>>> that are actually going to actually get referenced in a given .dts. I
>>>> note that a number of nodes will still get labelled that are unlikely
>>>> to be referenced for any number of reasons.
>>>>
>>>> For instance, as an example [1][2][3] of one of the boards I'm working
>>>> on currently, all of the ehci/ohci/mmc nodes, some set of the pio
>>>> nodes... almost every single node has a label, and most of them don't
>>>> need a phandle based on what is currently referenced and actually
>>>> used. I note that 27 of these nodes seem to be referenced, out of 39
>>>> nodes with labels (numbers are approximate), leaving ~30% (12) of
>>>> labelled nodes unreferenced.
>>>>
>>>> For a slightly larger example, [4][5][6][7]; 74 referenced nodes out
>>>> of 113 labelled nodes, leaving ~35% (39) of labelled nodes
>>>> unreferenced.
>>>>
>>>> This is only bothersome because it starts adding up as these things
>>>> get bigger, and I don't think it really needs to. The alternative to
>>>> keep phandles down to the minimum set required doesn't add much in
>>>> maintenance cost or overhead from the overlay application side;
>>>> especially for blobs generated by this dtc(1) that make the active
>>>> decision to allocate liberally rather than conservatively.
>>>
>>> This concept relies on a hand coded __symbols__ node instead of
>>> having dtc generate it.  This makes the overlay devicetree source
>>> more fragile.  The example dts file in patch 2/2 is fairly safe,
>>> but it is legal syntax to specify:
>>>
>>>   test = "/test-node";
>>>
>>> instead of:
>>>
>>>   test = &test;
>>>
>>> And the fragile syntax is exactly what results from a decompiled
>>> overlay dtb.
>>
>> I think I might be misunderstanding something here- our dtc (BSDL
>> dtc), with -@, still writes a full /__symbols__ node of all labeled
>> nodes, given that this is what -@ is expected to do. The difference is
>
> I was talking about the .dts file in patch 2/2, where the __symbols__
> node was hand coded, instead of being created by dtc.

Ah, ok. I was confused because I hadn't seen any mention of a
hand-rolled /__symbols__; this example base dts upon which overlays
are applied was copy+paste from the non-auto_phandle version of it,
and altered to reflect what our BSDL dtc would actually generate. I'm
also not sure what you mean by 'reliant' upon here; you use the same
overlays and .dts as you do now. I altered nothing but what libfdt
does when it takes a /__fixup__, looks up the corresponding symbol in
the base fdt, and fails to find a phandle at that path.

>> that our dtc allocates phandles conservatively because they're a
>> concept for cross-referencing nodes. Conceptually, it doesn't seem
>> like either way is more or less wrong than the other.
>
> Yes, the phandles concept is for cross-referencing.  In your specific
> context, creating phandle properties at the time that applying the
> overlay determines that the cross-reference actually occurs does
> allow dynamically creating the phandle properties.  If overlays
> were only applied to flattened device trees, then that could be
> a useful optimization.

Indeed, but...

>>> On another note, in the case of Linux, I have to consider applying
>>> overlays to a live devicetree, as well as possibly the case of
>>> applying overlays to a flattened device tree.  I do not expect
>>> that we will add the dynamic creation of missing phandles when
>>> applying an overlay to a live devicetree.  This would result in
>>> an overlay that can be applied to a flattened device tree, but
>>> will fail to apply to a Linux live devicetree.  BSD and Linux
>>> do not have to be fully compatible, but in my opinion, the more
>>> compatible the better.
>>
>> I'm probably being dense here, so please excuse me, but I don't see
>> this as necessarily a problem- likely because I have no familiarity of
>> how Linux works with overlays or FDT bits in general. My stupid
>> question of the hour: Linux actually supports applying overlays to
>> live devicetree?
>
> Not fully implemented yet.  In fact there are some pretty important
> issues to be resolved or implemented.  But there are people who
> have implemented enough of it out of mainline to be able to
> successfully apply specific overlays to the live devicetree for
> specific use cases.  There is also the concept of removing an
> overlay from the live devicetree at some point after having
> applied it.

I think that kind of changes my perspective on this. Given that it's
not a stable implementation, I honestly don't think it's reasonable to
expect the final implementation to be this strict and I do hope this
detail would be reconsidered. If an overlay can reasonably be applied,
why would you fail it based on a missing phandle?

Your implementation knows what my overlay means otherwise because I
reference a labelled node using &foo, dtc generated a /__fixup__ for
it, your implementation takes that /__fixup__ and does the lookup in
the symbol table. The symbol exists and points to a node, what's the
point of rejecting it there?

It seems unreasonable to me, because you cannot always control the
source of your live tree. In many cases, sure, it's generated in-tree
or in U-Boot and passed to you, and you can reasonably expect you
won't encounter this. What if you have vendor-provided tree?

I think the point I'm getting at is that it seems like this will have
to change at some point anyways simply because you can't control all
sources of your devicetree, and this isn't strictly wrong. Telling
someone "No, we can't apply that overlay because your vendor's
internal tool for generating dts and $other_format_used_internally
simultaneously didn't generate a phandle for that" is kind of hard,
especially when your reasoning ISN'T "their blob is malformed" or
"your overlay's reference is ambiguous" but rather, "we know what
that's pointing at, but we just don't generate handles."

Re: [PATCH v2 1/2] overlays: auto allocate phandles for nodes in base fdt

[Frank Rowand]

On 01/04/18 21:47, Kyle Evans wrote:
> On Thu, Jan 4, 2018 at 11:02 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> On 01/04/18 19:50, Kyle Evans wrote:
>>> On Thu, Jan 4, 2018 at 9:14 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>> On 01/04/18 18:39, Kyle Evans wrote:
>>>>> On Thu, Jan 4, 2018 at 7:55 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>>>> On 01/04/18 13:47, Kyle Evans wrote:
>>>>>>> On Thu, Jan 4, 2018 at 3:34 PM, Kyle Evans <kevans-h+KGxgPPiopAfugRpC6u6w@public.gmane.org> wrote:
>>>>>>>> On Thu, Jan 4, 2018 at 2:41 PM, Kyle Evans <kevans-h+KGxgPPiopAfugRpC6u6w@public.gmane.org> wrote:
>>>>>>>>> On Thu, Jan 4, 2018 at 2:33 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>>>>>>>> [... snip ...]
>>>>>>>>>>
>>>>>>>>>> Does this remove the need for the proposed patch, or am I still
>>>>>>>>>> missing something?
>>>>>>>>>
>>>>>>>>> ... nope. Apparently I never tested this with this particular dtc(1)
>>>>>>>>> and instead just assumed it did the same as ours- allocate phandle
>>>>>>>>> sparsely, even with -@. That certainly removes the need for this
>>>>>>>>> patch, and I'm somewhat upset that I hadn't previously considered
>>>>>>>>> this.
>>>>>>>>>
>>>>>>>>> @David, Jon: Please disregard all of the patches along these lines...
>>>>>>>>> I'll fix this in our dtc, where it should be fixed.
>>>>>>>>>
>>>>>>>>> Thanks, Frank!
>>>>>>>>
>>>>>>>> Actually, I'm kind of torn on whether this is useful or not. With
>>>>>>>> being able to have EFI-provided FDT, it's hard to guarantee whether
>>>>>>>> the FDT we're provided has been compiled with GPL dtc(1) and -@. The
>>>>>>>> above solves this problem for most of my personal use-cases , though,
>>>>>>>> since I can guarantee that our FDT and U-Boot provided FDT is compiled
>>>>>>>> properly.
>>>>>>>
>>>>>>> Apologies for the triple post; I realized that this argument is
>>>>>>> inherently wrong, since we can't reference the node if there's no
>>>>>>> symbol anyways.
>>>>>>>
>>>>>>> The only way this might still be a good idea is to support more
>>>>>>> minimal cases where an implementation might prefer to not create a
>>>>>>> phandle for nodes that haven't been referenced.
>>>>>>>
>>>>>>> In our case, we have a function [1] that walks the tree and generates
>>>>>>> metadata on nodes that have phandles, under the assumption that these
>>>>>>> have been referenced somewhere and provides a way to more quickly
>>>>>>> reference these specifically through a separate linked link.
>>>>>>> Allocating phandles for everything as GPL dtc does adds quite a bit
>>>>>>> more overhead to this.
>>>>>>>
>>>>>>> [1] http://src.illumos.org/source/xref/freebsd-head/sys/dev/ofw/openfirm.c#119
>>>>>>
>>>>>> The "-@" option does not add a phandle to _every_ node, just to nodes
>>>>>> that have a label:
>>>>>>
>>>>>
>>>>> In practice, this is still a not necessarily close superset of those
>>>>> that are actually going to actually get referenced in a given .dts. I
>>>>> note that a number of nodes will still get labelled that are unlikely
>>>>> to be referenced for any number of reasons.
>>>>>
>>>>> For instance, as an example [1][2][3] of one of the boards I'm working
>>>>> on currently, all of the ehci/ohci/mmc nodes, some set of the pio
>>>>> nodes... almost every single node has a label, and most of them don't
>>>>> need a phandle based on what is currently referenced and actually
>>>>> used. I note that 27 of these nodes seem to be referenced, out of 39
>>>>> nodes with labels (numbers are approximate), leaving ~30% (12) of
>>>>> labelled nodes unreferenced.
>>>>>
>>>>> For a slightly larger example, [4][5][6][7]; 74 referenced nodes out
>>>>> of 113 labelled nodes, leaving ~35% (39) of labelled nodes
>>>>> unreferenced.
>>>>>
>>>>> This is only bothersome because it starts adding up as these things
>>>>> get bigger, and I don't think it really needs to. The alternative to
>>>>> keep phandles down to the minimum set required doesn't add much in
>>>>> maintenance cost or overhead from the overlay application side;
>>>>> especially for blobs generated by this dtc(1) that make the active
>>>>> decision to allocate liberally rather than conservatively.
>>>>
>>>> This concept relies on a hand coded __symbols__ node instead of
>>>> having dtc generate it.  This makes the overlay devicetree source
>>>> more fragile.  The example dts file in patch 2/2 is fairly safe,
>>>> but it is legal syntax to specify:
>>>>
>>>>   test = "/test-node";
>>>>
>>>> instead of:
>>>>
>>>>   test = &test;
>>>>
>>>> And the fragile syntax is exactly what results from a decompiled
>>>> overlay dtb.
>>>
>>> I think I might be misunderstanding something here- our dtc (BSDL
>>> dtc), with -@, still writes a full /__symbols__ node of all labeled
>>> nodes, given that this is what -@ is expected to do. The difference is
>>
>> I was talking about the .dts file in patch 2/2, where the __symbols__
>> node was hand coded, instead of being created by dtc.
> 
> Ah, ok. I was confused because I hadn't seen any mention of a
> hand-rolled /__symbols__; this example base dts upon which overlays
> are applied was copy+paste from the non-auto_phandle version of it,
> and altered to reflect what our BSDL dtc would actually generate. I'm
> also not sure what you mean by 'reliant' upon here; you use the same

The problem case ("relies") occurs only because the __symbols__ node
in the base devicetree blob is generated from a hand coded node in the
base devicetree source file.  If the hand coded source did not exist
then the "-@" flag would have to be provided to dtc so that dtc
would auto generate the __symbols__ node in the base devicetree blob.

If compiled with "-@", there would be no missing phandles for nodes
reference from the __symbols__ node.


> overlays and .dts as you do now. I altered nothing but what libfdt
> does when it takes a /__fixup__, looks up the corresponding symbol in
> the base fdt, and fails to find a phandle at that path.
> 
>>> that our dtc allocates phandles conservatively because they're a
>>> concept for cross-referencing nodes. Conceptually, it doesn't seem
>>> like either way is more or less wrong than the other.
>>
>> Yes, the phandles concept is for cross-referencing.  In your specific
>> context, creating phandle properties at the time that applying the
>> overlay determines that the cross-reference actually occurs does
>> allow dynamically creating the phandle properties.  If overlays
>> were only applied to flattened device trees, then that could be
>> a useful optimization.
> 
> Indeed, but...
> 
>>>> On another note, in the case of Linux, I have to consider applying
>>>> overlays to a live devicetree, as well as possibly the case of
>>>> applying overlays to a flattened device tree.  I do not expect
>>>> that we will add the dynamic creation of missing phandles when
>>>> applying an overlay to a live devicetree.  This would result in
>>>> an overlay that can be applied to a flattened device tree, but
>>>> will fail to apply to a Linux live devicetree.  BSD and Linux
>>>> do not have to be fully compatible, but in my opinion, the more
>>>> compatible the better.
>>>
>>> I'm probably being dense here, so please excuse me, but I don't see
>>> this as necessarily a problem- likely because I have no familiarity of
>>> how Linux works with overlays or FDT bits in general. My stupid
>>> question of the hour: Linux actually supports applying overlays to
>>> live devicetree?
>>
>> Not fully implemented yet.  In fact there are some pretty important
>> issues to be resolved or implemented.  But there are people who
>> have implemented enough of it out of mainline to be able to
>> successfully apply specific overlays to the live devicetree for
>> specific use cases.  There is also the concept of removing an
>> overlay from the live devicetree at some point after having
>> applied it.
> 
> I think that kind of changes my perspective on this. Given that it's
> not a stable implementation, I honestly don't think it's reasonable to
> expect the final implementation to be this strict and I do hope this
> detail would be reconsidered. If an overlay can reasonably be applied,
> why would you fail it based on a missing phandle?

There is no a simple answer to the question.  To start answering it
leads to pulling on loose threads, and that leads to other loose
threads, and it just becomes a big tangled ball of yarn.  The full
answer to this question will be many individual implementation
details that will be discussed one by one as the code in the Linux
kernel evolves.


> Your implementation knows what my overlay means otherwise because I
> reference a labelled node using &foo, dtc generated a /__fixup__ for
> it, your implementation takes that /__fixup__ and does the lookup in
> the symbol table. The symbol exists and points to a node, what's the
> point of rejecting it there?> 
> It seems unreasonable to me, because you cannot always control the
> source of your live tree. In many cases, sure, it's generated in-tree
> or in U-Boot and passed to you, and you can reasonably expect you
> won't encounter this. What if you have vendor-provided tree?
> 
> I think the point I'm getting at is that it seems like this will have
> to change at some point anyways simply because you can't control all
> sources of your devicetree, and this isn't strictly wrong. Telling
> someone "No, we can't apply that overlay because your vendor's
> internal tool for generating dts and $other_format_used_internally
> simultaneously didn't generate a phandle for that" is kind of hard,> especially when your reasoning ISN'T "their blob is malformed" or
> "your overlay's reference is ambiguous" but rather, "we know what
> that's pointing at, but we just don't generate handles."

No, the blob _is_ malformed.  I know there is no official binding
document for overlays (we do need such things once we figure out
what we are doing), so this comment is purely my opinion.

The blob is malformed because it was not compiled with the "-@"
flag.  Mostly not because of anything in the source, although
again the __symbols__ node should not be hand coded.

Here is an analogy: the overlay metadata is conceptually similar
to the symbol table in an object file compiled from C source code.
The linker will use the symbol table to link a second object file
that contains references to the first object file, resulting in
a program object file.  It is not normal to hand code the symbol
table in the object file.  Similarly dtc creates the __symbols__
node, which is essentially a symbol table.  The Linux kernel
(or a bootloader, or whatever) performs the linking role as
part of applying on overlay devicetree to a base devicetree.

Also hand coded overlay meta data is fragile by nature.  My long
term goal is that overlay meta data is only generated by the
compiler.  Or maybe a compiler flag will be needed to allow hand
coded overlay meta data to not cause compile errors, so that hand
coded overlay meta data remains possible, but it is obvious that
it is discouraged.

But this conversation is now far afield from discussing the
patch series.

-Frank

Re: [PATCH v2 1/2] overlays: auto allocate phandles for nodes in base fdt

[Frank Rowand]

On 01/04/18 20:23, Kyle Evans wrote:
> On Thu, Jan 4, 2018 at 9:53 PM, David Gibson
> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
>> On Thu, Jan 04, 2018 at 03:08:08PM -0700, Ian Lepore wrote:
>>> On Thu, 2018-01-04 at 15:47 -0600, Kyle Evans wrote:
>>>> On Thu, Jan 4, 2018 at 3:34 PM, Kyle Evans <kevans-h+KGxgPPiopAfugRpC6u6w@public.gmane.org> wrote:
>>>>>
>>>>> On Thu, Jan 4, 2018 at 2:41 PM, Kyle Evans <kevans-h+KGxgPPiopAfugRpC6u6w@public.gmane.org> wrote:
>>>>>>
>>>>>> On Thu, Jan 4, 2018 at 2:33 PM, Frank Rowand  wrote:
>>>>>>>
>>>>>>> [... snip ...]
>>>>>>>
>>>>>>> Does this remove the need for the proposed patch, or am I still
>>>>>>> missing something?
>>>>>> ... nope. Apparently I never tested this with this particular dtc(1)
>>>>>> and instead just assumed it did the same as ours- allocate phandle
>>>>>> sparsely, even with -@. That certainly removes the need for this
>>>>>> patch, and I'm somewhat upset that I hadn't previously considered
>>>>>> this.
>>>>>>
>>>>>> @David, Jon: Please disregard all of the patches along these lines...
>>>>>> I'll fix this in our dtc, where it should be fixed.
>>>>>>
>>>>>> Thanks, Frank!
>>>>> Actually, I'm kind of torn on whether this is useful or not. With
>>>>> being able to have EFI-provided FDT, it's hard to guarantee whether
>>>>> the FDT we're provided has been compiled with GPL dtc(1) and -@. The
>>>>> above solves this problem for most of my personal use-cases , though,
>>>>> since I can guarantee that our FDT and U-Boot provided FDT is compiled
>>>>> properly.
>>>> Apologies for the triple post; I realized that this argument is
>>>> inherently wrong, since we can't reference the node if there's no
>>>> symbol anyways.
>>>>
>>>> The only way this might still be a good idea is to support more
>>>> minimal cases where an implementation might prefer to not create a
>>>> phandle for nodes that haven't been referenced.
>>>>
>>>> In our case, we have a function [1] that walks the tree and generates
>>>> metadata on nodes that have phandles, under the assumption that these
>>>> have been referenced somewhere and provides a way to more quickly
>>>> reference these specifically through a separate linked link.
>>>> Allocating phandles for everything as GPL dtc does adds quite a bit
>>>> more overhead to this.
>>>>
>>>> [1] http://src.illumos.org/source/xref/freebsd-head/sys/dev/ofw/openfirm.c#119
>>>
>>> In particular, it makes lookups more expensive as it now must traverse
>>> a list that includes every node in the dtb, rather than just nodes that
>>> are actually referenced.  (It also increases the amount of storage, but
>>> at 20-ish bytes per node, that's not a big deal.)
>>
>> Lookups of what exactly?  Aren't you unflattening the tree after you
>> read it in?

I was going to ask this question in a separate part of the thread, but
I'll keep it here in this context.  For my education:

    Aren't you unflattening the tree after you read it in?


> Lookup in this context would be a lookup of the device from the xref
> phandle, see OF_device_from_xref [1] and its inverse
> OF_xref_from_device right below it. Devices, as they attach, register
> for the xref phandle, then consumers lookup the device associated with
> it and generally hold a handle to the device afterwards.
> 
> [1] http://src.illumos.org/source/xref/freebsd-head/sys/dev/ofw/openfirm.c#628
> 

Re: [PATCH v2 1/2] overlays: auto allocate phandles for nodes in base fdt

[Kyle Evans]

On Fri, Jan 5, 2018 at 2:40 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On 01/04/18 21:47, Kyle Evans wrote:
>> On Thu, Jan 4, 2018 at 11:02 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> Your implementation knows what my overlay means otherwise because I
>> reference a labelled node using &foo, dtc generated a /__fixup__ for
>> it, your implementation takes that /__fixup__ and does the lookup in
>> the symbol table. The symbol exists and points to a node, what's the
>> point of rejecting it there?>
>> It seems unreasonable to me, because you cannot always control the
>> source of your live tree. In many cases, sure, it's generated in-tree
>> or in U-Boot and passed to you, and you can reasonably expect you
>> won't encounter this. What if you have vendor-provided tree?
>>
>> I think the point I'm getting at is that it seems like this will have
>> to change at some point anyways simply because you can't control all
>> sources of your devicetree, and this isn't strictly wrong. Telling
>> someone "No, we can't apply that overlay because your vendor's
>> internal tool for generating dts and $other_format_used_internally
>> simultaneously didn't generate a phandle for that" is kind of hard,> especially when your reasoning ISN'T "their blob is malformed" or
>> "your overlay's reference is ambiguous" but rather, "we know what
>> that's pointing at, but we just don't generate handles."
>
> No, the blob _is_ malformed.  I know there is no official binding
> document for overlays (we do need such things once we figure out
> what we are doing), so this comment is purely my opinion.
>
> The blob is malformed because it was not compiled with the "-@"
> flag.  Mostly not because of anything in the source, although
> again the __symbols__ node should not be hand coded.

I know this is only tangential to the point you're making above, but
the __symbols__ node in this specific example you're referencing was
hand coded by someone else, probably to avoid having to write
different invocations of DTC for the different test cases. I'm only
responsible for removing one line of this .dts, the phandle
assignment.

> Here is an analogy: the overlay metadata is conceptually similar
> to the symbol table in an object file compiled from C source code.
> The linker will use the symbol table to link a second object file
> that contains references to the first object file, resulting in
> a program object file.  It is not normal to hand code the symbol
> table in the object file.  Similarly dtc creates the __symbols__
> node, which is essentially a symbol table.  The Linux kernel
> (or a bootloader, or whatever) performs the linking role as
> part of applying on overlay devicetree to a base devicetree.

There is no hand coding of /__symbols__ nodes anywhere, except for
this test case that was written by someone else. I think your analogy
is inaccurate for the actual situation, though.

The linker has all of the metadata it needs to properly link, but
refuses to for dubious reasons. It can do the lookup in the symbol
table, and that symbol table points to a valid segment of code
("properties and subnodes"), but is refusing to complete the link
based on a missing property that's arbitrarily assigned by someone
else anyways and cannot be relied on to have any meaning or special
value.

To be perfectly clear here: we're talking about taking a blob compiled
from a sensible overlay, such as [1], and applying it to a fellow
sensible blob that has been generated with a proper /__symbols__ node
and phandles assigned only to nodes within its tree that have been
cross-referenced by other nodes within its tree.

> Also hand coded overlay meta data is fragile by nature.  My long
> term goal is that overlay meta data is only generated by the
> compiler.  Or maybe a compiler flag will be needed to allow hand
> coded overlay meta data to not cause compile errors, so that hand
> coded overlay meta data remains possible, but it is obvious that
> it is discouraged.
>
> But this conversation is now far afield from discussing the
> patch series.

Indeed.

[1] http://people.freebsd.org/~kevans/sun8i-a83t-emac.dts

Re: [PATCH v2 1/2] overlays: auto allocate phandles for nodes in base fdt

[Frank Rowand]

On 01/05/18 13:04, Kyle Evans wrote:
> On Fri, Jan 5, 2018 at 2:40 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> On 01/04/18 21:47, Kyle Evans wrote:
>>> On Thu, Jan 4, 2018 at 11:02 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>> Your implementation knows what my overlay means otherwise because I
>>> reference a labelled node using &foo, dtc generated a /__fixup__ for
>>> it, your implementation takes that /__fixup__ and does the lookup in
>>> the symbol table. The symbol exists and points to a node, what's the
>>> point of rejecting it there?>
>>> It seems unreasonable to me, because you cannot always control the
>>> source of your live tree. In many cases, sure, it's generated in-tree
>>> or in U-Boot and passed to you, and you can reasonably expect you
>>> won't encounter this. What if you have vendor-provided tree?
>>>
>>> I think the point I'm getting at is that it seems like this will have
>>> to change at some point anyways simply because you can't control all
>>> sources of your devicetree, and this isn't strictly wrong. Telling
>>> someone "No, we can't apply that overlay because your vendor's
>>> internal tool for generating dts and $other_format_used_internally
>>> simultaneously didn't generate a phandle for that" is kind of hard,> especially when your reasoning ISN'T "their blob is malformed" or
>>> "your overlay's reference is ambiguous" but rather, "we know what
>>> that's pointing at, but we just don't generate handles."
>>
>> No, the blob _is_ malformed.  I know there is no official binding
>> document for overlays (we do need such things once we figure out
>> what we are doing), so this comment is purely my opinion.
>>
>> The blob is malformed because it was not compiled with the "-@"
>> flag.  Mostly not because of anything in the source, although
>> again the __symbols__ node should not be hand coded.
> 
> I know this is only tangential to the point you're making above, but
> the __symbols__ node in this specific example you're referencing was
> hand coded by someone else, probably to avoid having to write
> different invocations of DTC for the different test cases. I'm only
> responsible for removing one line of this .dts, the phandle
> assignment.
> 
>> Here is an analogy: the overlay metadata is conceptually similar
>> to the symbol table in an object file compiled from C source code.
>> The linker will use the symbol table to link a second object file
>> that contains references to the first object file, resulting in
>> a program object file.  It is not normal to hand code the symbol
>> table in the object file.  Similarly dtc creates the __symbols__
>> node, which is essentially a symbol table.  The Linux kernel
>> (or a bootloader, or whatever) performs the linking role as
>> part of applying on overlay devicetree to a base devicetree.
> 
> There is no hand coding of /__symbols__ nodes anywhere, except for
> this test case that was written by someone else. I think your analogy
> is inaccurate for the actual situation, though.
> 
> The linker has all of the metadata it needs to properly link, but
> refuses to for dubious reasons. It can do the lookup in the symbol
> table, and that symbol table points to a valid segment of code
> ("properties and subnodes"), but is refusing to complete the link
> based on a missing property that's arbitrarily assigned by someone
> else anyways and cannot be relied on to have any meaning or special
> value.
> 
> To be perfectly clear here: we're talking about taking a blob compiled
> from a sensible overlay, such as [1], and applying it to a fellow
> sensible blob that has been generated with a proper /__symbols__ node

> and phandles assigned only to nodes within its tree that have been
> cross-referenced by other nodes within its tree.

Yes, so not a base overlay that has not been compiled by the dtc (in
this project) with the "-@" flag specified.  Either the __symbols__
node was hand coded, or a compiler other than dtc was used, which
chooses to not create a phandle property for a node whose label
is not de-referenced in the same source file.

This use case should have been clearly described in the patch
description.  And David can then choose to accept or reject the
patch as he sees fit.  I clearly think it is a bad idea, but
David will either agree or disagree with my opinion.


>> Also hand coded overlay meta data is fragile by nature.  My long
>> term goal is that overlay meta data is only generated by the
>> compiler.  Or maybe a compiler flag will be needed to allow hand
>> coded overlay meta data to not cause compile errors, so that hand
>> coded overlay meta data remains possible, but it is obvious that
>> it is discouraged.
>>
>> But this conversation is now far afield from discussing the
>> patch series.
> 
> Indeed.
> 
> [1] http://people.freebsd.org/~kevans/sun8i-a83t-emac.dts
> 

Re: [PATCH v2 1/2] overlays: auto allocate phandles for nodes in base fdt

[Kyle Evans]

On Fri, Jan 5, 2018 at 3:22 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On 01/05/18 13:04, Kyle Evans wrote:
>> On Fri, Jan 5, 2018 at 2:40 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>> On 01/04/18 21:47, Kyle Evans wrote:
>>>> On Thu, Jan 4, 2018 at 11:02 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>> Your implementation knows what my overlay means otherwise because I
>>>> reference a labelled node using &foo, dtc generated a /__fixup__ for
>>>> it, your implementation takes that /__fixup__ and does the lookup in
>>>> the symbol table. The symbol exists and points to a node, what's the
>>>> point of rejecting it there?>
>>>> It seems unreasonable to me, because you cannot always control the
>>>> source of your live tree. In many cases, sure, it's generated in-tree
>>>> or in U-Boot and passed to you, and you can reasonably expect you
>>>> won't encounter this. What if you have vendor-provided tree?
>>>>
>>>> I think the point I'm getting at is that it seems like this will have
>>>> to change at some point anyways simply because you can't control all
>>>> sources of your devicetree, and this isn't strictly wrong. Telling
>>>> someone "No, we can't apply that overlay because your vendor's
>>>> internal tool for generating dts and $other_format_used_internally
>>>> simultaneously didn't generate a phandle for that" is kind of hard,> especially when your reasoning ISN'T "their blob is malformed" or
>>>> "your overlay's reference is ambiguous" but rather, "we know what
>>>> that's pointing at, but we just don't generate handles."
>>>
>>> No, the blob _is_ malformed.  I know there is no official binding
>>> document for overlays (we do need such things once we figure out
>>> what we are doing), so this comment is purely my opinion.
>>>
>>> The blob is malformed because it was not compiled with the "-@"
>>> flag.  Mostly not because of anything in the source, although
>>> again the __symbols__ node should not be hand coded.
>>
>> I know this is only tangential to the point you're making above, but
>> the __symbols__ node in this specific example you're referencing was
>> hand coded by someone else, probably to avoid having to write
>> different invocations of DTC for the different test cases. I'm only
>> responsible for removing one line of this .dts, the phandle
>> assignment.
>>
>>> Here is an analogy: the overlay metadata is conceptually similar
>>> to the symbol table in an object file compiled from C source code.
>>> The linker will use the symbol table to link a second object file
>>> that contains references to the first object file, resulting in
>>> a program object file.  It is not normal to hand code the symbol
>>> table in the object file.  Similarly dtc creates the __symbols__
>>> node, which is essentially a symbol table.  The Linux kernel
>>> (or a bootloader, or whatever) performs the linking role as
>>> part of applying on overlay devicetree to a base devicetree.
>>
>> There is no hand coding of /__symbols__ nodes anywhere, except for
>> this test case that was written by someone else. I think your analogy
>> is inaccurate for the actual situation, though.
>>
>> The linker has all of the metadata it needs to properly link, but
>> refuses to for dubious reasons. It can do the lookup in the symbol
>> table, and that symbol table points to a valid segment of code
>> ("properties and subnodes"), but is refusing to complete the link
>> based on a missing property that's arbitrarily assigned by someone
>> else anyways and cannot be relied on to have any meaning or special
>> value.
>>
>> To be perfectly clear here: we're talking about taking a blob compiled
>> from a sensible overlay, such as [1], and applying it to a fellow
>> sensible blob that has been generated with a proper /__symbols__ node
>
>> and phandles assigned only to nodes within its tree that have been
>> cross-referenced by other nodes within its tree.
>
> Yes, so not a base overlay that has not been compiled by the dtc (in
> this project) with the "-@" flag specified.  Either the __symbols__
> node was hand coded, or a compiler other than dtc was used, which
> chooses to not create a phandle property for a node whose label
> is not de-referenced in the same source file.

My understanding was that libfdt was not necessarily supposed to be so
tightly coupled to this dtc that it would make such assumptions when
it's otherwise not a malformed blob. Ditto for the comment below about
the use case being described in the patch- I had goofed and not
checked that this particular dtc implementation does it differently,
but I also was of the belief that libfdt was only supposed to be
loosely coupled to the dtc implementation in this same repository.

Of course, I can't recall/find what I had looked at that gave me this
impression. =)

> This use case should have been clearly described in the patch
> description.  And David can then choose to accept or reject the
> patch as he sees fit.  I clearly think it is a bad idea, but
> David will either agree or disagree with my opinion.

David: If I may, I would appreciate if you would re-re-consider this
patch set on the following basis:

1.) For base blobs compiled by your dtc, this is a no-op. The latest
version does entail one extra tree walk at the beginning of applying
/__fixups__ even if all phandles are assigned, but I've since realized
this could be moved further down to a tree walk the first time it has
to assign a phandle so that it really is a no-op for blobs compiled by
your compiler.

2.) For other base blobs, you have compatibility. As of right now, how
phandles are assigned in a base blob is an implementation detail,
given that there is no clear spec on this. You gain nothing from
remaining strict on this, and you lose compatibility with blobs that
aren't blatantly invalid that libfdt can't control as well as
potential (other) users of your library that may value this. Of
course, you wouldn't lose us as users, we'd adapt.

I consider this one to be fairly important. We had to implement
reading of older formats (see nathanw@'s recent patch to this effect)
because we can't control what we're given in all cases. I fully expect
that even if I'm asked to drop this and we adapt our dtc, we'll
eventually run into a case where this is needed because of
vendor-provided DTS via EFI.

3.) There's nothing inherently wrong with what I've done in this
patch, I've just supplemented the arbitrary assignment of phandles by
dtc with arbitrary assignment of phandles by libfdt. There is no
change to how the referenced node is looked up or generated, and this
has no bearing on how anything is actually written.

As an aside, IMO, phandles assigned to nodes are pointless once you
have a symbol table to reference; just a nice thing to have so you
don't *have* to take the performance hit at runtime to assign them.
The labels you would otherwise be losing are now stored in the blob's
symbol table, so references are not ambiguous as they would be in a
normal blob compiled without -@.

I have other thoughts about the argument of blobs being able to be
applied to fdt but not a livetree, but I think they can all be summed
up as: this is a non-issue if you're expecting all of your base blobs
to be compiled by this dtc -@ anyways.

Re: [PATCH v2 1/2] overlays: auto allocate phandles for nodes in base fdt

[Kyle Evans]

On Fri, Jan 5, 2018 at 6:47 PM, Kyle Evans <kevans-h+KGxgPPiopAfugRpC6u6w@public.gmane.org> wrote:
> On Fri, Jan 5, 2018 at 3:22 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> On 01/05/18 13:04, Kyle Evans wrote:
>>> On Fri, Jan 5, 2018 at 2:40 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>> On 01/04/18 21:47, Kyle Evans wrote:
>>>>> On Thu, Jan 4, 2018 at 11:02 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>>> Your implementation knows what my overlay means otherwise because I
>>>>> reference a labelled node using &foo, dtc generated a /__fixup__ for
>>>>> it, your implementation takes that /__fixup__ and does the lookup in
>>>>> the symbol table. The symbol exists and points to a node, what's the
>>>>> point of rejecting it there?>
>>>>> It seems unreasonable to me, because you cannot always control the
>>>>> source of your live tree. In many cases, sure, it's generated in-tree
>>>>> or in U-Boot and passed to you, and you can reasonably expect you
>>>>> won't encounter this. What if you have vendor-provided tree?
>>>>>
>>>>> I think the point I'm getting at is that it seems like this will have
>>>>> to change at some point anyways simply because you can't control all
>>>>> sources of your devicetree, and this isn't strictly wrong. Telling
>>>>> someone "No, we can't apply that overlay because your vendor's
>>>>> internal tool for generating dts and $other_format_used_internally
>>>>> simultaneously didn't generate a phandle for that" is kind of hard,> especially when your reasoning ISN'T "their blob is malformed" or
>>>>> "your overlay's reference is ambiguous" but rather, "we know what
>>>>> that's pointing at, but we just don't generate handles."
>>>>
>>>> No, the blob _is_ malformed.  I know there is no official binding
>>>> document for overlays (we do need such things once we figure out
>>>> what we are doing), so this comment is purely my opinion.
>>>>
>>>> The blob is malformed because it was not compiled with the "-@"
>>>> flag.  Mostly not because of anything in the source, although
>>>> again the __symbols__ node should not be hand coded.
>>>
>>> I know this is only tangential to the point you're making above, but
>>> the __symbols__ node in this specific example you're referencing was
>>> hand coded by someone else, probably to avoid having to write
>>> different invocations of DTC for the different test cases. I'm only
>>> responsible for removing one line of this .dts, the phandle
>>> assignment.
>>>
>>>> Here is an analogy: the overlay metadata is conceptually similar
>>>> to the symbol table in an object file compiled from C source code.
>>>> The linker will use the symbol table to link a second object file
>>>> that contains references to the first object file, resulting in
>>>> a program object file.  It is not normal to hand code the symbol
>>>> table in the object file.  Similarly dtc creates the __symbols__
>>>> node, which is essentially a symbol table.  The Linux kernel
>>>> (or a bootloader, or whatever) performs the linking role as
>>>> part of applying on overlay devicetree to a base devicetree.
>>>
>>> There is no hand coding of /__symbols__ nodes anywhere, except for
>>> this test case that was written by someone else. I think your analogy
>>> is inaccurate for the actual situation, though.
>>>
>>> The linker has all of the metadata it needs to properly link, but
>>> refuses to for dubious reasons. It can do the lookup in the symbol
>>> table, and that symbol table points to a valid segment of code
>>> ("properties and subnodes"), but is refusing to complete the link
>>> based on a missing property that's arbitrarily assigned by someone
>>> else anyways and cannot be relied on to have any meaning or special
>>> value.
>>>
>>> To be perfectly clear here: we're talking about taking a blob compiled
>>> from a sensible overlay, such as [1], and applying it to a fellow
>>> sensible blob that has been generated with a proper /__symbols__ node
>>
>>> and phandles assigned only to nodes within its tree that have been
>>> cross-referenced by other nodes within its tree.
>>
>> Yes, so not a base overlay that has not been compiled by the dtc (in
>> this project) with the "-@" flag specified.  Either the __symbols__
>> node was hand coded, or a compiler other than dtc was used, which
>> chooses to not create a phandle property for a node whose label
>> is not de-referenced in the same source file.
>
> My understanding was that libfdt was not necessarily supposed to be so
> tightly coupled to this dtc that it would make such assumptions when
> it's otherwise not a malformed blob. Ditto for the comment below about
> the use case being described in the patch- I had goofed and not
> checked that this particular dtc implementation does it differently,
> but I also was of the belief that libfdt was only supposed to be
> loosely coupled to the dtc implementation in this same repository.
>
> Of course, I can't recall/find what I had looked at that gave me this
> impression. =)
>
>> This use case should have been clearly described in the patch
>> description.  And David can then choose to accept or reject the
>> patch as he sees fit.  I clearly think it is a bad idea, but
>> David will either agree or disagree with my opinion.
>
> David: If I may, I would appreciate if you would re-re-consider this
> patch set on the following basis:
>
> 1.) For base blobs compiled by your dtc, this is a no-op. The latest
> version does entail one extra tree walk at the beginning of applying
> /__fixups__ even if all phandles are assigned, but I've since realized
> this could be moved further down to a tree walk the first time it has
> to assign a phandle so that it really is a no-op for blobs compiled by
> your compiler.
>
> 2.) For other base blobs, you have compatibility. As of right now, how
> phandles are assigned in a base blob is an implementation detail,
> given that there is no clear spec on this. You gain nothing from
> remaining strict on this, and you lose compatibility with blobs that
> aren't blatantly invalid that libfdt can't control as well as
> potential (other) users of your library that may value this. Of
> course, you wouldn't lose us as users, we'd adapt.
>
> I consider this one to be fairly important. We had to implement
> reading of older formats (see nathanw@'s recent patch to this effect)
> because we can't control what we're given in all cases. I fully expect
> that even if I'm asked to drop this and we adapt our dtc, we'll
> eventually run into a case where this is needed because of
> vendor-provided DTS via EFI.
>
> 3.) There's nothing inherently wrong with what I've done in this
> patch, I've just supplemented the arbitrary assignment of phandles by
> dtc with arbitrary assignment of phandles by libfdt. There is no
> change to how the referenced node is looked up or generated, and this
> has no bearing on how anything is actually written.
>
> As an aside, IMO, phandles assigned to nodes are pointless once you
> have a symbol table to reference; just a nice thing to have so you
> don't *have* to take the performance hit at runtime to assign them.
> The labels you would otherwise be losing are now stored in the blob's
> symbol table, so references are not ambiguous as they would be in a
> normal blob compiled without -@.
>
> I have other thoughts about the argument of blobs being able to be
> applied to fdt but not a livetree, but I think they can all be summed
> up as: this is a non-issue if you're expecting all of your base blobs
> to be compiled by this dtc -@ anyways.

An afterthought to this: if you wouldn't consider it as a standard
behavior, I'd appreciate it if you would indicate possible acceptance
of another iteration that hides the allocation behind an #ifdef
FEATURE_ALLOCATE_PHANDLE or something of the sortr that neither you
nor the Linux folk will ever need to turn on. I don't foresee this
stuff changing enough in the future to actually require maintenance,
given how non-invasive the patch can be.

Re: [PATCH v2 1/2] overlays: auto allocate phandles for nodes in base fdt

[David Gibson]

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

On Thu, Jan 04, 2018 at 08:39:38PM -0600, Kyle Evans wrote:
> On Thu, Jan 4, 2018 at 7:55 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > On 01/04/18 13:47, Kyle Evans wrote:
> >> On Thu, Jan 4, 2018 at 3:34 PM, Kyle Evans <kevans-h+KGxgPPiopAfugRpC6u6w@public.gmane.org> wrote:
> >>> On Thu, Jan 4, 2018 at 2:41 PM, Kyle Evans <kevans-h+KGxgPPiopAfugRpC6u6w@public.gmane.org> wrote:
> >>>> On Thu, Jan 4, 2018 at 2:33 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >>>>> [... snip ...]
> >>>>>
> >>>>> Does this remove the need for the proposed patch, or am I still
> >>>>> missing something?
> >>>>
> >>>> ... nope. Apparently I never tested this with this particular dtc(1)
> >>>> and instead just assumed it did the same as ours- allocate phandle
> >>>> sparsely, even with -@. That certainly removes the need for this
> >>>> patch, and I'm somewhat upset that I hadn't previously considered
> >>>> this.
> >>>>
> >>>> @David, Jon: Please disregard all of the patches along these lines...
> >>>> I'll fix this in our dtc, where it should be fixed.
> >>>>
> >>>> Thanks, Frank!
> >>>
> >>> Actually, I'm kind of torn on whether this is useful or not. With
> >>> being able to have EFI-provided FDT, it's hard to guarantee whether
> >>> the FDT we're provided has been compiled with GPL dtc(1) and -@. The
> >>> above solves this problem for most of my personal use-cases , though,
> >>> since I can guarantee that our FDT and U-Boot provided FDT is compiled
> >>> properly.
> >>
> >> Apologies for the triple post; I realized that this argument is
> >> inherently wrong, since we can't reference the node if there's no
> >> symbol anyways.
> >>
> >> The only way this might still be a good idea is to support more
> >> minimal cases where an implementation might prefer to not create a
> >> phandle for nodes that haven't been referenced.
> >>
> >> In our case, we have a function [1] that walks the tree and generates
> >> metadata on nodes that have phandles, under the assumption that these
> >> have been referenced somewhere and provides a way to more quickly
> >> reference these specifically through a separate linked link.
> >> Allocating phandles for everything as GPL dtc does adds quite a bit
> >> more overhead to this.
> >>
> >> [1] http://src.illumos.org/source/xref/freebsd-head/sys/dev/ofw/openfirm.c#119
> >
> > The "-@" option does not add a phandle to _every_ node, just to nodes
> > that have a label:
> >
> 
> In practice, this is still a not necessarily close superset of those
> that are actually going to actually get referenced in a given .dts. I
> note that a number of nodes will still get labelled that are unlikely
> to be referenced for any number of reasons.
> 
> For instance, as an example [1][2][3] of one of the boards I'm working
> on currently, all of the ehci/ohci/mmc nodes, some set of the pio
> nodes... almost every single node has a label, and most of them don't
> need a phandle based on what is currently referenced and actually
> used. I note that 27 of these nodes seem to be referenced, out of 39
> nodes with labels (numbers are approximate), leaving ~30% (12) of
> labelled nodes unreferenced.
> 
> For a slightly larger example, [4][5][6][7]; 74 referenced nodes out
> of 113 labelled nodes, leaving ~35% (39) of labelled nodes
> unreferenced.

Right, but what I'm not getting is why having a lot of symbols is a
bad thing, other than because of a FreeBSD internal detail.

> 
> This is only bothersome because it starts adding up as these things
> get bigger, and I don't think it really needs to. The alternative to
> keep phandles down to the minimum set required doesn't add much in
> maintenance cost or overhead from the overlay application side;
> especially for blobs generated by this dtc(1) that make the active
> decision to allocate liberally rather than conservatively.
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/sun8i-a83t.dtsi
> [3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/sunxi-common-regulators.dtsi
> 
> [4] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/am335x-bonegreen.dts
> [5] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/am33xx.dtsi
> [6] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/am335x-bone-common.dtsi
> [7] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/am335x-bonegreen-common.dtsi

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/2] overlays: auto allocate phandles for nodes in base fdt

[David Gibson]

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

On Thu, Jan 04, 2018 at 09:50:54PM -0600, Kyle Evans wrote:
> On Thu, Jan 4, 2018 at 9:14 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > On 01/04/18 18:39, Kyle Evans wrote:
> >> On Thu, Jan 4, 2018 at 7:55 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >>> On 01/04/18 13:47, Kyle Evans wrote:
> >>>> On Thu, Jan 4, 2018 at 3:34 PM, Kyle Evans <kevans-h+KGxgPPiopAfugRpC6u6w@public.gmane.org> wrote:
> >>>>> On Thu, Jan 4, 2018 at 2:41 PM, Kyle Evans <kevans-h+KGxgPPiopAfugRpC6u6w@public.gmane.org> wrote:
> >>>>>> On Thu, Jan 4, 2018 at 2:33 PM, Frank Rowand <frowand.list@gmail.com> wrote:
> >>>>>>> [... snip ...]
> >>>>>>>
> >>>>>>> Does this remove the need for the proposed patch, or am I still
> >>>>>>> missing something?
> >>>>>>
> >>>>>> ... nope. Apparently I never tested this with this particular dtc(1)
> >>>>>> and instead just assumed it did the same as ours- allocate phandle
> >>>>>> sparsely, even with -@. That certainly removes the need for this
> >>>>>> patch, and I'm somewhat upset that I hadn't previously considered
> >>>>>> this.
> >>>>>>
> >>>>>> @David, Jon: Please disregard all of the patches along these lines...
> >>>>>> I'll fix this in our dtc, where it should be fixed.
> >>>>>>
> >>>>>> Thanks, Frank!
> >>>>>
> >>>>> Actually, I'm kind of torn on whether this is useful or not. With
> >>>>> being able to have EFI-provided FDT, it's hard to guarantee whether
> >>>>> the FDT we're provided has been compiled with GPL dtc(1) and -@. The
> >>>>> above solves this problem for most of my personal use-cases , though,
> >>>>> since I can guarantee that our FDT and U-Boot provided FDT is compiled
> >>>>> properly.
> >>>>
> >>>> Apologies for the triple post; I realized that this argument is
> >>>> inherently wrong, since we can't reference the node if there's no
> >>>> symbol anyways.
> >>>>
> >>>> The only way this might still be a good idea is to support more
> >>>> minimal cases where an implementation might prefer to not create a
> >>>> phandle for nodes that haven't been referenced.
> >>>>
> >>>> In our case, we have a function [1] that walks the tree and generates
> >>>> metadata on nodes that have phandles, under the assumption that these
> >>>> have been referenced somewhere and provides a way to more quickly
> >>>> reference these specifically through a separate linked link.
> >>>> Allocating phandles for everything as GPL dtc does adds quite a bit
> >>>> more overhead to this.
> >>>>
> >>>> [1] http://src.illumos.org/source/xref/freebsd-head/sys/dev/ofw/openfirm.c#119
> >>>
> >>> The "-@" option does not add a phandle to _every_ node, just to nodes
> >>> that have a label:
> >>>
> >>
> >> In practice, this is still a not necessarily close superset of those
> >> that are actually going to actually get referenced in a given .dts. I
> >> note that a number of nodes will still get labelled that are unlikely
> >> to be referenced for any number of reasons.
> >>
> >> For instance, as an example [1][2][3] of one of the boards I'm working
> >> on currently, all of the ehci/ohci/mmc nodes, some set of the pio
> >> nodes... almost every single node has a label, and most of them don't
> >> need a phandle based on what is currently referenced and actually
> >> used. I note that 27 of these nodes seem to be referenced, out of 39
> >> nodes with labels (numbers are approximate), leaving ~30% (12) of
> >> labelled nodes unreferenced.
> >>
> >> For a slightly larger example, [4][5][6][7]; 74 referenced nodes out
> >> of 113 labelled nodes, leaving ~35% (39) of labelled nodes
> >> unreferenced.
> >>
> >> This is only bothersome because it starts adding up as these things
> >> get bigger, and I don't think it really needs to. The alternative to
> >> keep phandles down to the minimum set required doesn't add much in
> >> maintenance cost or overhead from the overlay application side;
> >> especially for blobs generated by this dtc(1) that make the active
> >> decision to allocate liberally rather than conservatively.
> >
> > This concept relies on a hand coded __symbols__ node instead of
> > having dtc generate it.  This makes the overlay devicetree source
> > more fragile.  The example dts file in patch 2/2 is fairly safe,
> > but it is legal syntax to specify:
> >
> >   test = "/test-node";
> >
> > instead of:
> >
> >   test = &test;
> >
> > And the fragile syntax is exactly what results from a decompiled
> > overlay dtb.
> 
> I think I might be misunderstanding something here- our dtc (BSDL
> dtc), with -@, still writes a full /__symbols__ node of all labeled
> nodes, given that this is what -@ is expected to do. The difference is
> that our dtc allocates phandles conservatively because they're a
> concept for cross-referencing nodes.

Well, kind of.  But traditionally *every* node had a phandle (usually
it's the actual pointer to the node structure in a live OF).  Omitting
them is essentially an fdt size optimization.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/2] overlays: auto allocate phandles for nodes in base fdt

[David Gibson]

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

On Thu, Jan 04, 2018 at 09:02:38PM -0800, Frank Rowand wrote:
> On 01/04/18 19:50, Kyle Evans wrote:
> > On Thu, Jan 4, 2018 at 9:14 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >> On 01/04/18 18:39, Kyle Evans wrote:
> >>> On Thu, Jan 4, 2018 at 7:55 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >>>> On 01/04/18 13:47, Kyle Evans wrote:
> >>>>> On Thu, Jan 4, 2018 at 3:34 PM, Kyle Evans <kevans-h+KGxgPPiopAfugRpC6u6w@public.gmane.org> wrote:
> >>>>>> On Thu, Jan 4, 2018 at 2:41 PM, Kyle Evans <kevans-h+KGxgPPiopAfugRpC6u6w@public.gmane.org> wrote:
> >>>>>>> On Thu, Jan 4, 2018 at 2:33 PM, Frank Rowand <frowand.list@gmail.com> wrote:
> >>>>>>>> [... snip ...]
> >>>>>>>>
> >>>>>>>> Does this remove the need for the proposed patch, or am I still
> >>>>>>>> missing something?
> >>>>>>>
> >>>>>>> ... nope. Apparently I never tested this with this particular dtc(1)
> >>>>>>> and instead just assumed it did the same as ours- allocate phandle
> >>>>>>> sparsely, even with -@. That certainly removes the need for this
> >>>>>>> patch, and I'm somewhat upset that I hadn't previously considered
> >>>>>>> this.
> >>>>>>>
> >>>>>>> @David, Jon: Please disregard all of the patches along these lines...
> >>>>>>> I'll fix this in our dtc, where it should be fixed.
> >>>>>>>
> >>>>>>> Thanks, Frank!
> >>>>>>
> >>>>>> Actually, I'm kind of torn on whether this is useful or not. With
> >>>>>> being able to have EFI-provided FDT, it's hard to guarantee whether
> >>>>>> the FDT we're provided has been compiled with GPL dtc(1) and -@. The
> >>>>>> above solves this problem for most of my personal use-cases , though,
> >>>>>> since I can guarantee that our FDT and U-Boot provided FDT is compiled
> >>>>>> properly.
> >>>>>
> >>>>> Apologies for the triple post; I realized that this argument is
> >>>>> inherently wrong, since we can't reference the node if there's no
> >>>>> symbol anyways.
> >>>>>
> >>>>> The only way this might still be a good idea is to support more
> >>>>> minimal cases where an implementation might prefer to not create a
> >>>>> phandle for nodes that haven't been referenced.
> >>>>>
> >>>>> In our case, we have a function [1] that walks the tree and generates
> >>>>> metadata on nodes that have phandles, under the assumption that these
> >>>>> have been referenced somewhere and provides a way to more quickly
> >>>>> reference these specifically through a separate linked link.
> >>>>> Allocating phandles for everything as GPL dtc does adds quite a bit
> >>>>> more overhead to this.
> >>>>>
> >>>>> [1] http://src.illumos.org/source/xref/freebsd-head/sys/dev/ofw/openfirm.c#119
> >>>>
> >>>> The "-@" option does not add a phandle to _every_ node, just to nodes
> >>>> that have a label:
> >>>>
> >>>
> >>> In practice, this is still a not necessarily close superset of those
> >>> that are actually going to actually get referenced in a given .dts. I
> >>> note that a number of nodes will still get labelled that are unlikely
> >>> to be referenced for any number of reasons.
> >>>
> >>> For instance, as an example [1][2][3] of one of the boards I'm working
> >>> on currently, all of the ehci/ohci/mmc nodes, some set of the pio
> >>> nodes... almost every single node has a label, and most of them don't
> >>> need a phandle based on what is currently referenced and actually
> >>> used. I note that 27 of these nodes seem to be referenced, out of 39
> >>> nodes with labels (numbers are approximate), leaving ~30% (12) of
> >>> labelled nodes unreferenced.
> >>>
> >>> For a slightly larger example, [4][5][6][7]; 74 referenced nodes out
> >>> of 113 labelled nodes, leaving ~35% (39) of labelled nodes
> >>> unreferenced.
> >>>
> >>> This is only bothersome because it starts adding up as these things
> >>> get bigger, and I don't think it really needs to. The alternative to
> >>> keep phandles down to the minimum set required doesn't add much in
> >>> maintenance cost or overhead from the overlay application side;
> >>> especially for blobs generated by this dtc(1) that make the active
> >>> decision to allocate liberally rather than conservatively.
> >>
> >> This concept relies on a hand coded __symbols__ node instead of
> >> having dtc generate it.  This makes the overlay devicetree source
> >> more fragile.  The example dts file in patch 2/2 is fairly safe,
> >> but it is legal syntax to specify:
> >>
> >>   test = "/test-node";
> >>
> >> instead of:
> >>
> >>   test = &test;
> >>
> >> And the fragile syntax is exactly what results from a decompiled
> >> overlay dtb.
> > 
> > I think I might be misunderstanding something here- our dtc (BSDL
> > dtc), with -@, still writes a full /__symbols__ node of all labeled
> > nodes, given that this is what -@ is expected to do. The difference is
> 
> I was talking about the .dts file in patch 2/2, where the __symbols__
> node was hand coded, instead of being created by dtc.
> 
> 
> > that our dtc allocates phandles conservatively because they're a
> > concept for cross-referencing nodes. Conceptually, it doesn't seem
> > like either way is more or less wrong than the other.
> 
> Yes, the phandles concept is for cross-referencing.  In your specific
> context, creating phandle properties at the time that applying the
> overlay determines that the cross-reference actually occurs does
> allow dynamically creating the phandle properties.  If overlays
> were only applied to flattened device trees, then that could be
> a useful optimization.

I don't actually see why phandles couldn't be added dynamically for a
live tree.  In fact adding phandles for nodes that don't have them at
unflattening time seems like it might be a good idea on general
principle.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/2] overlays: auto allocate phandles for nodes in base fdt

[Kyle Evans]

On Wed, Jan 10, 2018 at 3:44 AM, David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Thu, Jan 04, 2018 at 09:02:38PM -0800, Frank Rowand wrote:
>> On 01/04/18 19:50, Kyle Evans wrote:
>> > On Thu, Jan 4, 2018 at 9:14 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> >> On 01/04/18 18:39, Kyle Evans wrote:
>> >>> On Thu, Jan 4, 2018 at 7:55 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> >>>> On 01/04/18 13:47, Kyle Evans wrote:
>> >>>>> On Thu, Jan 4, 2018 at 3:34 PM, Kyle Evans <kevans-h+KGxgPPiopAfugRpC6u6w@public.gmane.org> wrote:
>> >>>>>> On Thu, Jan 4, 2018 at 2:41 PM, Kyle Evans <kevans-h+KGxgPPiopAfugRpC6u6w@public.gmane.org> wrote:
>> >>>>>>> On Thu, Jan 4, 2018 at 2:33 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> >>>>>>>> [... snip ...]
>> >>>>>>>>
>> >>>>>>>> Does this remove the need for the proposed patch, or am I still
>> >>>>>>>> missing something?
>> >>>>>>>
>> >>>>>>> ... nope. Apparently I never tested this with this particular dtc(1)
>> >>>>>>> and instead just assumed it did the same as ours- allocate phandle
>> >>>>>>> sparsely, even with -@. That certainly removes the need for this
>> >>>>>>> patch, and I'm somewhat upset that I hadn't previously considered
>> >>>>>>> this.
>> >>>>>>>
>> >>>>>>> @David, Jon: Please disregard all of the patches along these lines...
>> >>>>>>> I'll fix this in our dtc, where it should be fixed.
>> >>>>>>>
>> >>>>>>> Thanks, Frank!
>> >>>>>>
>> >>>>>> Actually, I'm kind of torn on whether this is useful or not. With
>> >>>>>> being able to have EFI-provided FDT, it's hard to guarantee whether
>> >>>>>> the FDT we're provided has been compiled with GPL dtc(1) and -@. The
>> >>>>>> above solves this problem for most of my personal use-cases , though,
>> >>>>>> since I can guarantee that our FDT and U-Boot provided FDT is compiled
>> >>>>>> properly.
>> >>>>>
>> >>>>> Apologies for the triple post; I realized that this argument is
>> >>>>> inherently wrong, since we can't reference the node if there's no
>> >>>>> symbol anyways.
>> >>>>>
>> >>>>> The only way this might still be a good idea is to support more
>> >>>>> minimal cases where an implementation might prefer to not create a
>> >>>>> phandle for nodes that haven't been referenced.
>> >>>>>
>> >>>>> In our case, we have a function [1] that walks the tree and generates
>> >>>>> metadata on nodes that have phandles, under the assumption that these
>> >>>>> have been referenced somewhere and provides a way to more quickly
>> >>>>> reference these specifically through a separate linked link.
>> >>>>> Allocating phandles for everything as GPL dtc does adds quite a bit
>> >>>>> more overhead to this.
>> >>>>>
>> >>>>> [1] http://src.illumos.org/source/xref/freebsd-head/sys/dev/ofw/openfirm.c#119
>> >>>>
>> >>>> The "-@" option does not add a phandle to _every_ node, just to nodes
>> >>>> that have a label:
>> >>>>
>> >>>
>> >>> In practice, this is still a not necessarily close superset of those
>> >>> that are actually going to actually get referenced in a given .dts. I
>> >>> note that a number of nodes will still get labelled that are unlikely
>> >>> to be referenced for any number of reasons.
>> >>>
>> >>> For instance, as an example [1][2][3] of one of the boards I'm working
>> >>> on currently, all of the ehci/ohci/mmc nodes, some set of the pio
>> >>> nodes... almost every single node has a label, and most of them don't
>> >>> need a phandle based on what is currently referenced and actually
>> >>> used. I note that 27 of these nodes seem to be referenced, out of 39
>> >>> nodes with labels (numbers are approximate), leaving ~30% (12) of
>> >>> labelled nodes unreferenced.
>> >>>
>> >>> For a slightly larger example, [4][5][6][7]; 74 referenced nodes out
>> >>> of 113 labelled nodes, leaving ~35% (39) of labelled nodes
>> >>> unreferenced.
>> >>>
>> >>> This is only bothersome because it starts adding up as these things
>> >>> get bigger, and I don't think it really needs to. The alternative to
>> >>> keep phandles down to the minimum set required doesn't add much in
>> >>> maintenance cost or overhead from the overlay application side;
>> >>> especially for blobs generated by this dtc(1) that make the active
>> >>> decision to allocate liberally rather than conservatively.
>> >>
>> >> This concept relies on a hand coded __symbols__ node instead of
>> >> having dtc generate it.  This makes the overlay devicetree source
>> >> more fragile.  The example dts file in patch 2/2 is fairly safe,
>> >> but it is legal syntax to specify:
>> >>
>> >>   test = "/test-node";
>> >>
>> >> instead of:
>> >>
>> >>   test = &test;
>> >>
>> >> And the fragile syntax is exactly what results from a decompiled
>> >> overlay dtb.
>> >
>> > I think I might be misunderstanding something here- our dtc (BSDL
>> > dtc), with -@, still writes a full /__symbols__ node of all labeled
>> > nodes, given that this is what -@ is expected to do. The difference is
>>
>> I was talking about the .dts file in patch 2/2, where the __symbols__
>> node was hand coded, instead of being created by dtc.
>>
>>
>> > that our dtc allocates phandles conservatively because they're a
>> > concept for cross-referencing nodes. Conceptually, it doesn't seem
>> > like either way is more or less wrong than the other.
>>
>> Yes, the phandles concept is for cross-referencing.  In your specific
>> context, creating phandle properties at the time that applying the
>> overlay determines that the cross-reference actually occurs does
>> allow dynamically creating the phandle properties.  If overlays
>> were only applied to flattened device trees, then that could be
>> a useful optimization.
>
> I don't actually see why phandles couldn't be added dynamically for a
> live tree.  In fact adding phandles for nodes that don't have them at
> unflattening time seems like it might be a good idea on general
> principle.

After getting some more in-depth insight (in case we want to use it)
from Frank off-list about how they're planning on allowing live
overlays, I think I get it.

If I were personally the one implementing what he described to me with
the way this works in libfdt, the first thing I'd do after all drivers
have attached and I've effectively unflattened the FDT is strip almost
all of the phandles. As a libfdt user, I have no other way that
doesn't suck of making sure that an overlay can't access resources
they shouldn't be.

The alternative to relying on this is obviously walking the overlay's
tree and checking all of the /__fixups__ or node properties, depending
on your perspective on life. Why? That sucks performance wise,
especially when you're going to have to do it all over again if you've
seen nothing wrong when libfdt applies it.

It all boils down to: not all libfdt environments are the same. Some
environments may want to strictly control what an overlay can do (like
Frank's) without taking an absolutely stupid performance hit. Other
environments may effectively see the overlay as an extension of the
base blob and rather not inhibit what the overlay can do, like my
loader.

After what Frank's told me, I'd rather not see this as a default
behavior of libfdt, but I'd like it to be an option for those
environments that consider the overlay they're about to apply an
effective extension of the base.

I'll re-propose this, probably in a couple weeks, as a compile-time
feature that's off by default. I think that would address any concerns
I personally have of this- I fixed the specific case that started this
path in our dtc, but I'd still like this to be an option for our
loader-applied overlays.

In the meantime, nathanw@'s recent patch for reading older FDT is more
pressing in order for me to update our libfdt and punting our overlay
implementation in favor if libfdt's, so I'd rather not detract any
potential effort that could be put towards merging that in.

Re: [PATCH v2 1/2] overlays: auto allocate phandles for nodes in base fdt

[David Gibson]

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

On Fri, Jan 05, 2018 at 12:40:13PM -0800, Frank Rowand wrote:
> On 01/04/18 21:47, Kyle Evans wrote:
> > On Thu, Jan 4, 2018 at 11:02 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >> On 01/04/18 19:50, Kyle Evans wrote:
> >>> On Thu, Jan 4, 2018 at 9:14 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >>>> On 01/04/18 18:39, Kyle Evans wrote:
> >>>>> On Thu, Jan 4, 2018 at 7:55 PM, Frank Rowand <frowand.list-Re5JQEeQqe8@public.gmane.orgm> wrote:
> >>>>>> On 01/04/18 13:47, Kyle Evans wrote:
> >>>>>>> On Thu, Jan 4, 2018 at 3:34 PM, Kyle Evans <kevans-h+KGxgPPiopAfugRpC6u6w@public.gmane.org> wrote:
> >>>>>>>> On Thu, Jan 4, 2018 at 2:41 PM, Kyle Evans <kevans-h+KGxgPPiopAfugRpC6u6w@public.gmane.org> wrote:
> >>>>>>>>> On Thu, Jan 4, 2018 at 2:33 PM, Frank Rowand <frowand.list@gmail.com> wrote:
> >>>>>>>>>> [... snip ...]
[snip]
> > Your implementation knows what my overlay means otherwise because I
> > reference a labelled node using &foo, dtc generated a /__fixup__ for
> > it, your implementation takes that /__fixup__ and does the lookup in
> > the symbol table. The symbol exists and points to a node, what's the
> > point of rejecting it there?> 
> > It seems unreasonable to me, because you cannot always control the
> > source of your live tree. In many cases, sure, it's generated in-tree
> > or in U-Boot and passed to you, and you can reasonably expect you
> > won't encounter this. What if you have vendor-provided tree?
> > 
> > I think the point I'm getting at is that it seems like this will have
> > to change at some point anyways simply because you can't control all
> > sources of your devicetree, and this isn't strictly wrong. Telling
> > someone "No, we can't apply that overlay because your vendor's
> > internal tool for generating dts and $other_format_used_internally
> > simultaneously didn't generate a phandle for that" is kind of hard,> especially when your reasoning ISN'T "their blob is malformed" or
> > "your overlay's reference is ambiguous" but rather, "we know what
> > that's pointing at, but we just don't generate handles."
> 
> No, the blob _is_ malformed.  I know there is no official binding
> document for overlays (we do need such things once we figure out
> what we are doing), so this comment is purely my opinion.

In this case it's not a spec for overlays that's the issue, it's a
spec for what base trees require in order to accept overlays.

> The blob is malformed because it was not compiled with the "-@"
> flag.  Mostly not because of anything in the source, although
> again the __symbols__ node should not be hand coded.

I don't think it's reasonable to claim a blob is malformed based
purely on how it was generated, it needs to be something about it's
actualy contents.

So the question is: is "includes a phandle for every node with a
symbol" a requirement for an overlay accepting base tree.  It's never
been explicitly stated, since overlays were just kind of hacked
together rather than fully specced out.  dtc has been written assuming
that is a requirement, BSDdtc has not.

I'm inclined to say "yes, it should be a requirement" on the grounds
that:
    a) that's the interpretation that's more widely deployed already
and
    b) that simplifies the overlay application logic, which generally
       takes place in a more restricted environment than the initial
       compilation.

I'm entirely open to arguments against that position though.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/2] overlays: auto allocate phandles for nodes in base fdt

[Frank Rowand]

On 01/11/18 21:33, David Gibson wrote:
> On Fri, Jan 05, 2018 at 12:40:13PM -0800, Frank Rowand wrote:
>> On 01/04/18 21:47, Kyle Evans wrote:
>>> On Thu, Jan 4, 2018 at 11:02 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>> On 01/04/18 19:50, Kyle Evans wrote:
>>>>> On Thu, Jan 4, 2018 at 9:14 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>>>> On 01/04/18 18:39, Kyle Evans wrote:
>>>>>>> On Thu, Jan 4, 2018 at 7:55 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>>>>>> On 01/04/18 13:47, Kyle Evans wrote:
>>>>>>>>> On Thu, Jan 4, 2018 at 3:34 PM, Kyle Evans <kevans-h+KGxgPPiopAfugRpC6u6w@public.gmane.org> wrote:
>>>>>>>>>> On Thu, Jan 4, 2018 at 2:41 PM, Kyle Evans <kevans-h+KGxgPPiopAfugRpC6u6w@public.gmane.org> wrote:
>>>>>>>>>>> On Thu, Jan 4, 2018 at 2:33 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>>>>>>>>>> [... snip ...]
> [snip]
>>> Your implementation knows what my overlay means otherwise because I
>>> reference a labelled node using &foo, dtc generated a /__fixup__ for
>>> it, your implementation takes that /__fixup__ and does the lookup in
>>> the symbol table. The symbol exists and points to a node, what's the
>>> point of rejecting it there?> 
>>> It seems unreasonable to me, because you cannot always control the
>>> source of your live tree. In many cases, sure, it's generated in-tree
>>> or in U-Boot and passed to you, and you can reasonably expect you
>>> won't encounter this. What if you have vendor-provided tree?
>>>
>>> I think the point I'm getting at is that it seems like this will have
>>> to change at some point anyways simply because you can't control all
>>> sources of your devicetree, and this isn't strictly wrong. Telling
>>> someone "No, we can't apply that overlay because your vendor's
>>> internal tool for generating dts and $other_format_used_internally
>>> simultaneously didn't generate a phandle for that" is kind of hard,> especially when your reasoning ISN'T "their blob is malformed" or
>>> "your overlay's reference is ambiguous" but rather, "we know what
>>> that's pointing at, but we just don't generate handles."
>>
>> No, the blob _is_ malformed.  I know there is no official binding
>> document for overlays (we do need such things once we figure out
>> what we are doing), so this comment is purely my opinion.
> 
> In this case it's not a spec for overlays that's the issue, it's a
> spec for what base trees require in order to accept overlays.
> 
>> The blob is malformed because it was not compiled with the "-@"
>> flag.  Mostly not because of anything in the source, although
>> again the __symbols__ node should not be hand coded.
> 
> I don't think it's reasonable to claim a blob is malformed based
> purely on how it was generated, it needs to be something about it's
> actualy contents.

Yes, sorry, I was not clear about what I intended by that statement.

What I meant was that if the source had been compiled by dtc with
the "-@" flag then the blob would have contained what I was
claiming was missing data.  So I was trying to understand and
explain the mechanism or process as to how Kyle got the resulting
dtb, not making the claim that you quite rightly picked up on.


> So the question is: is "includes a phandle for every node with a
> symbol" a requirement for an overlay accepting base tree.  It's never
> been explicitly stated, since overlays were just kind of hacked
> together rather than fully specced out.  dtc has been written assuming
> that is a requirement, BSDdtc has not.
> 
> I'm inclined to say "yes, it should be a requirement" on the grounds
> that:
>     a) that's the interpretation that's more widely deployed already
> and
>     b) that simplifies the overlay application logic, which generally
>        takes place in a more restricted environment than the initial
>        compilation.
> 
> I'm entirely open to arguments against that position though.
> 

Re: [PATCH v2 1/2] overlays: auto allocate phandles for nodes in base fdt

[Kyle Evans]

On Thu, Jan 11, 2018 at 11:33 PM, David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Fri, Jan 05, 2018 at 12:40:13PM -0800, Frank Rowand wrote:
>> On 01/04/18 21:47, Kyle Evans wrote:
>> > On Thu, Jan 4, 2018 at 11:02 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> >> On 01/04/18 19:50, Kyle Evans wrote:
>> >>> On Thu, Jan 4, 2018 at 9:14 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> >>>> On 01/04/18 18:39, Kyle Evans wrote:
>> >>>>> On Thu, Jan 4, 2018 at 7:55 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> >>>>>> On 01/04/18 13:47, Kyle Evans wrote:
>> >>>>>>> On Thu, Jan 4, 2018 at 3:34 PM, Kyle Evans <kevans-h+KGxgPPiopAfugRpC6u6w@public.gmane.org> wrote:
>> >>>>>>>> On Thu, Jan 4, 2018 at 2:41 PM, Kyle Evans <kevans-h+KGxgPPiopAfugRpC6u6w@public.gmane.org> wrote:
>> >>>>>>>>> On Thu, Jan 4, 2018 at 2:33 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> >>>>>>>>>> [... snip ...]
> [snip]
>> > Your implementation knows what my overlay means otherwise because I
>> > reference a labelled node using &foo, dtc generated a /__fixup__ for
>> > it, your implementation takes that /__fixup__ and does the lookup in
>> > the symbol table. The symbol exists and points to a node, what's the
>> > point of rejecting it there?>
>> > It seems unreasonable to me, because you cannot always control the
>> > source of your live tree. In many cases, sure, it's generated in-tree
>> > or in U-Boot and passed to you, and you can reasonably expect you
>> > won't encounter this. What if you have vendor-provided tree?
>> >
>> > I think the point I'm getting at is that it seems like this will have
>> > to change at some point anyways simply because you can't control all
>> > sources of your devicetree, and this isn't strictly wrong. Telling
>> > someone "No, we can't apply that overlay because your vendor's
>> > internal tool for generating dts and $other_format_used_internally
>> > simultaneously didn't generate a phandle for that" is kind of hard,> especially when your reasoning ISN'T "their blob is malformed" or
>> > "your overlay's reference is ambiguous" but rather, "we know what
>> > that's pointing at, but we just don't generate handles."
>>
>> No, the blob _is_ malformed.  I know there is no official binding
>> document for overlays (we do need such things once we figure out
>> what we are doing), so this comment is purely my opinion.
>
> In this case it's not a spec for overlays that's the issue, it's a
> spec for what base trees require in order to accept overlays.
>
>> The blob is malformed because it was not compiled with the "-@"
>> flag.  Mostly not because of anything in the source, although
>> again the __symbols__ node should not be hand coded.
>
> I don't think it's reasonable to claim a blob is malformed based
> purely on how it was generated, it needs to be something about it's
> actualy contents.
>
> So the question is: is "includes a phandle for every node with a
> symbol" a requirement for an overlay accepting base tree.  It's never
> been explicitly stated, since overlays were just kind of hacked
> together rather than fully specced out.  dtc has been written assuming
> that is a requirement, BSDdtc has not.
>
> I'm inclined to say "yes, it should be a requirement" on the grounds
> that:
>     a) that's the interpretation that's more widely deployed already
> and
>     b) that simplifies the overlay application logic, which generally
>        takes place in a more restricted environment than the initial
>        compilation.
>
> I'm entirely open to arguments against that position though.

To be honest, I think both of these points are kind of wobbly. Just
because something has been largely interpreted a certain way does not
make it necessarily a good way to do so.

It's also kind of hard to make the simplification point, my latest
patch [1] that I run locally doesn't really touch existing stuff all
that much, and it certainly doesn't feel any more complex than what
was already there.

I would be inclined to say that a spec shouldn't hold a strong
position on this, for the following reasons:

1.) I've not yet seen a good technical reason for requiring explicit
assignment. Explicit assignment was useful when you had no other
method for resolving xrefs, but this isn't the case here.

2.) It's hard for a spec to say what the environmental restrictions
will be of an implementation. While you might be memory-constrained, I
might be disk-constrained. While you may not want to spend the extra
cycles to walk the tree the first time to figure out the next phandle
to be assigned, I might be OK with that trade-off.

Ultimately, it should be up to the implementation actually applying
these overlays to decide what it's willing to accept. I don't see that
as a big problem, but I'm also coming from a stand-point that the only
*blobs* I physically receive are those that I have no real control
over because they come from firmware. No one that I know is physically
passing blobs around to be used in u-boot or otherwise (save for
rpi-firmware)... the transparency is crap and that's just silly.

My suggested wording would likely be along the lines of "a base tree
for accepting overlays should have a phandle assigned to every node
that may be referenced in an overlay, but the mechanism for actually
applying overlays may or may not require this."

I like the 'should' wording, because it gives room to say "Look, if
you're going to force a blob on people or expect these blobs to work
on a wide range of systems, you should really do it this way for
optimal compatibility" while also not restricting what I do as a
consenting adult in the name of keeping things clean in an environment
that I otherwise control.

[1] https://people.freebsd.org/~kevans/libfdt-autoassign.diff

Re: [PATCH v2 1/2] overlays: auto allocate phandles for nodes in base fdt

[Kyle Evans]

On Fri, Jan 12, 2018 at 9:57 AM, Kyle Evans <kevans-h+KGxgPPiopAfugRpC6u6w@public.gmane.org> wrote:
> On Thu, Jan 11, 2018 at 11:33 PM, David Gibson
> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
>> On Fri, Jan 05, 2018 at 12:40:13PM -0800, Frank Rowand wrote:
>>> On 01/04/18 21:47, Kyle Evans wrote:
>>> > On Thu, Jan 4, 2018 at 11:02 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>> >> On 01/04/18 19:50, Kyle Evans wrote:
>>> >>> On Thu, Jan 4, 2018 at 9:14 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>> >>>> On 01/04/18 18:39, Kyle Evans wrote:
>>> >>>>> On Thu, Jan 4, 2018 at 7:55 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>> >>>>>> On 01/04/18 13:47, Kyle Evans wrote:
>>> >>>>>>> On Thu, Jan 4, 2018 at 3:34 PM, Kyle Evans <kevans-h+KGxgPPiopAfugRpC6u6w@public.gmane.org> wrote:
>>> >>>>>>>> On Thu, Jan 4, 2018 at 2:41 PM, Kyle Evans <kevans-h+KGxgPPiopAfugRpC6u6w@public.gmane.org> wrote:
>>> >>>>>>>>> On Thu, Jan 4, 2018 at 2:33 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>> >>>>>>>>>> [... snip ...]
>> [snip]
>>> > Your implementation knows what my overlay means otherwise because I
>>> > reference a labelled node using &foo, dtc generated a /__fixup__ for
>>> > it, your implementation takes that /__fixup__ and does the lookup in
>>> > the symbol table. The symbol exists and points to a node, what's the
>>> > point of rejecting it there?>
>>> > It seems unreasonable to me, because you cannot always control the
>>> > source of your live tree. In many cases, sure, it's generated in-tree
>>> > or in U-Boot and passed to you, and you can reasonably expect you
>>> > won't encounter this. What if you have vendor-provided tree?
>>> >
>>> > I think the point I'm getting at is that it seems like this will have
>>> > to change at some point anyways simply because you can't control all
>>> > sources of your devicetree, and this isn't strictly wrong. Telling
>>> > someone "No, we can't apply that overlay because your vendor's
>>> > internal tool for generating dts and $other_format_used_internally
>>> > simultaneously didn't generate a phandle for that" is kind of hard,> especially when your reasoning ISN'T "their blob is malformed" or
>>> > "your overlay's reference is ambiguous" but rather, "we know what
>>> > that's pointing at, but we just don't generate handles."
>>>
>>> No, the blob _is_ malformed.  I know there is no official binding
>>> document for overlays (we do need such things once we figure out
>>> what we are doing), so this comment is purely my opinion.
>>
>> In this case it's not a spec for overlays that's the issue, it's a
>> spec for what base trees require in order to accept overlays.
>>
>>> The blob is malformed because it was not compiled with the "-@"
>>> flag.  Mostly not because of anything in the source, although
>>> again the __symbols__ node should not be hand coded.
>>
>> I don't think it's reasonable to claim a blob is malformed based
>> purely on how it was generated, it needs to be something about it's
>> actualy contents.
>>
>> So the question is: is "includes a phandle for every node with a
>> symbol" a requirement for an overlay accepting base tree.  It's never
>> been explicitly stated, since overlays were just kind of hacked
>> together rather than fully specced out.  dtc has been written assuming
>> that is a requirement, BSDdtc has not.
>>
>> I'm inclined to say "yes, it should be a requirement" on the grounds
>> that:
>>     a) that's the interpretation that's more widely deployed already
>> and
>>     b) that simplifies the overlay application logic, which generally
>>        takes place in a more restricted environment than the initial
>>        compilation.
>>
>> I'm entirely open to arguments against that position though.
>
> To be honest, I think both of these points are kind of wobbly. Just
> because something has been largely interpreted a certain way does not
> make it necessarily a good way to do so.
>
> It's also kind of hard to make the simplification point, my latest
> patch [1] that I run locally doesn't really touch existing stuff all
> that much, and it certainly doesn't feel any more complex than what
> was already there.
>
> I would be inclined to say that a spec shouldn't hold a strong
> position on this, for the following reasons:
>
> 1.) I've not yet seen a good technical reason for requiring explicit
> assignment. Explicit assignment was useful when you had no other
> method for resolving xrefs, but this isn't the case here.
>
> 2.) It's hard for a spec to say what the environmental restrictions
> will be of an implementation. While you might be memory-constrained, I
> might be disk-constrained. While you may not want to spend the extra
> cycles to walk the tree the first time to figure out the next phandle
> to be assigned, I might be OK with that trade-off.
>
> Ultimately, it should be up to the implementation actually applying
> these overlays to decide what it's willing to accept. I don't see that
> as a big problem, but I'm also coming from a stand-point that the only
> *blobs* I physically receive are those that I have no real control
> over because they come from firmware. No one that I know is physically
> passing blobs around to be used in u-boot or otherwise (save for
> rpi-firmware)... the transparency is crap and that's just silly.
>
> My suggested wording would likely be along the lines of "a base tree
> for accepting overlays should have a phandle assigned to every node
> that may be referenced in an overlay, but the mechanism for actually
> applying overlays may or may not require this."
>
> I like the 'should' wording, because it gives room to say "Look, if
> you're going to force a blob on people or expect these blobs to work
> on a wide range of systems, you should really do it this way for
> optimal compatibility" while also not restricting what I do as a
> consenting adult in the name of keeping things clean in an environment
> that I otherwise control.
>

Sorry, I meant to add- this also gives you, Frank and co. room to say
"Sorry, this implementation doesn't accept this." This allows one to
strip explicit phandles from things that their implementation does not
want xref'd and for the resulting blob to still be considered 'overlay
accepting' and accepted by implementations, because the
implementations have some choice in what they accept for base trees.

Linux's loader implementation may accept willy-nilly for base trees,
while their live tree implementation will be a lot less accepting.
Sure, they also control the pipeline from loader to live tree, but for
demonstration purposes it's not that crazy to consider. Their live
tree implementation is still a valid implementation that applies
overlays to base trees, and I consider that fact significant to this
discussion.

It gives me room to add an optional flag to our BSDL dtc to go back to
our former method of assigning phandles at compile-time, so that the
default for '-@' is the compatible behavior but a minimal approach may
be taken instead for those that choose to do so.

Re: [PATCH v2 1/2] overlays: auto allocate phandles for nodes in base fdt

[David Gibson]

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

On Fri, Jan 12, 2018 at 09:57:26AM -0600, Kyle Evans wrote:
> On Thu, Jan 11, 2018 at 11:33 PM, David Gibson
> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > On Fri, Jan 05, 2018 at 12:40:13PM -0800, Frank Rowand wrote:
> >> On 01/04/18 21:47, Kyle Evans wrote:
> >> > On Thu, Jan 4, 2018 at 11:02 PM, Frank Rowand <frowand.list-Re5JQEeQqe8@public.gmane.orgm> wrote:
> >> >> On 01/04/18 19:50, Kyle Evans wrote:
> >> >>> On Thu, Jan 4, 2018 at 9:14 PM, Frank Rowand <frowand.list@gmail.com> wrote:
> >> >>>> On 01/04/18 18:39, Kyle Evans wrote:
> >> >>>>> On Thu, Jan 4, 2018 at 7:55 PM, Frank Rowand <frowand.list@gmail.com> wrote:
> >> >>>>>> On 01/04/18 13:47, Kyle Evans wrote:
> >> >>>>>>> On Thu, Jan 4, 2018 at 3:34 PM, Kyle Evans <kevans-h+KGxgPPiopAfugRpC6u6w@public.gmane.org> wrote:
> >> >>>>>>>> On Thu, Jan 4, 2018 at 2:41 PM, Kyle Evans <kevans-h+KGxgPPiorF2uMehF1BdA@public.gmane.orgg> wrote:
> >> >>>>>>>>> On Thu, Jan 4, 2018 at 2:33 PM, Frank Rowand <frowand.list@gmail.com> wrote:
> >> >>>>>>>>>> [... snip ...]
> > [snip]
> >> > Your implementation knows what my overlay means otherwise because I
> >> > reference a labelled node using &foo, dtc generated a /__fixup__ for
> >> > it, your implementation takes that /__fixup__ and does the lookup in
> >> > the symbol table. The symbol exists and points to a node, what's the
> >> > point of rejecting it there?>
> >> > It seems unreasonable to me, because you cannot always control the
> >> > source of your live tree. In many cases, sure, it's generated in-tree
> >> > or in U-Boot and passed to you, and you can reasonably expect you
> >> > won't encounter this. What if you have vendor-provided tree?
> >> >
> >> > I think the point I'm getting at is that it seems like this will have
> >> > to change at some point anyways simply because you can't control all
> >> > sources of your devicetree, and this isn't strictly wrong. Telling
> >> > someone "No, we can't apply that overlay because your vendor's
> >> > internal tool for generating dts and $other_format_used_internally
> >> > simultaneously didn't generate a phandle for that" is kind of hard,> especially when your reasoning ISN'T "their blob is malformed" or
> >> > "your overlay's reference is ambiguous" but rather, "we know what
> >> > that's pointing at, but we just don't generate handles."
> >>
> >> No, the blob _is_ malformed.  I know there is no official binding
> >> document for overlays (we do need such things once we figure out
> >> what we are doing), so this comment is purely my opinion.
> >
> > In this case it's not a spec for overlays that's the issue, it's a
> > spec for what base trees require in order to accept overlays.
> >
> >> The blob is malformed because it was not compiled with the "-@"
> >> flag.  Mostly not because of anything in the source, although
> >> again the __symbols__ node should not be hand coded.
> >
> > I don't think it's reasonable to claim a blob is malformed based
> > purely on how it was generated, it needs to be something about it's
> > actualy contents.
> >
> > So the question is: is "includes a phandle for every node with a
> > symbol" a requirement for an overlay accepting base tree.  It's never
> > been explicitly stated, since overlays were just kind of hacked
> > together rather than fully specced out.  dtc has been written assuming
> > that is a requirement, BSDdtc has not.
> >
> > I'm inclined to say "yes, it should be a requirement" on the grounds
> > that:
> >     a) that's the interpretation that's more widely deployed already
> > and
> >     b) that simplifies the overlay application logic, which generally
> >        takes place in a more restricted environment than the initial
> >        compilation.
> >
> > I'm entirely open to arguments against that position though.
> 
> To be honest, I think both of these points are kind of wobbly.

I'm not claiming they're conclusive, just the best arguments I've seen
so far.

> Just
> because something has been largely interpreted a certain way does not
> make it necessarily a good way to do so.

No, but the fact that widely deployed implementations rely on a
certain interpretation is a fairly strong argument for keeping it,
even if it's not the best by other measures.

> It's also kind of hard to make the simplification point, my latest
> patch [1] that I run locally doesn't really touch existing stuff all
> that much, and it certainly doesn't feel any more complex than what
> was already there.

It adds and doesn't remove code, so I think it's hard to argue it
doesn't add complexity.

> I would be inclined to say that a spec shouldn't hold a strong
> position on this, for the following reasons:
> 
> 1.) I've not yet seen a good technical reason for requiring explicit
> assignment. Explicit assignment was useful when you had no other
> method for resolving xrefs, but this isn't the case here.

I'm not really sure what you mean by explicit assignment.

> 2.) It's hard for a spec to say what the environmental restrictions
> will be of an implementation. While you might be memory-constrained, I
> might be disk-constrained. While you may not want to spend the extra
> cycles to walk the tree the first time to figure out the next phandle
> to be assigned, I might be OK with that trade-off.

True in principle, but stretched to breaking point in practice.  Given
that overlay application typically occurs in a bootloader, and
complication typically occurs in a full userspace environment, it's
pretty hard to see the overlay application environment as anything
other than (at least potentially) vastly more constrained in every
plausibly relevant dimension.

> Ultimately, it should be up to the implementation actually applying
> these overlays to decide what it's willing to accept.

Not if this is going to have relevant meaning as a
cross-implementation standard, even a de facto one.

> I don't see that
> as a big problem, but I'm also coming from a stand-point that the only
> *blobs* I physically receive are those that I have no real control
> over because they come from firmware. No one that I know is physically
> passing blobs around to be used in u-boot or otherwise (save for
> rpi-firmware)... the transparency is crap and that's just silly.
> 
> My suggested wording would likely be along the lines of "a base tree
> for accepting overlays should have a phandle assigned to every node
> that may be referenced in an overlay, but the mechanism for actually
> applying overlays may or may not require this."
> 
> I like the 'should' wording, because it gives room to say "Look, if
> you're going to force a blob on people or expect these blobs to work
> on a wide range of systems, you should really do it this way for
> optimal compatibility" while also not restricting what I do as a
> consenting adult in the name of keeping things clean in an environment
> that I otherwise control.

Well, that's fair enough, I guess.

Have you encountered the missing phandles with any blobs you've
encountered *other* than ones you've built with BSDdtc?

I mean, the basic position here is that we have a working[1]
implementation on both sides.  There is no formal spec - that's not a
good thing, but it's where we're at.  If your new implementation wants
to take a different interpretation, the onus is really on you to make
a concrete case that's stronger than staying with what we have.  So
far the best I've seen is "lots of phandles are inconvenient for our
kernel for poorly articulated reasons".  That doesn't have no weight,
but I don't think you've yet passed the threshhold required to
overcome inertia.

[1] To the extent that overlays can work, I have many, many issues
with the format, but that's a discussion that's been had elsewhere.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/2] overlays: auto allocate phandles for nodes in base fdt

[David Gibson]

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

On Fri, Jan 12, 2018 at 10:24:34AM -0600, Kyle Evans wrote:
> On Fri, Jan 12, 2018 at 9:57 AM, Kyle Evans <kevans-h+KGxgPPiopAfugRpC6u6w@public.gmane.org> wrote:
> > On Thu, Jan 11, 2018 at 11:33 PM, David Gibson
> > <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> >> On Fri, Jan 05, 2018 at 12:40:13PM -0800, Frank Rowand wrote:
> >>> On 01/04/18 21:47, Kyle Evans wrote:
> >>> > On Thu, Jan 4, 2018 at 11:02 PM, Frank Rowand <frowand.list@gmail.com> wrote:
> >>> >> On 01/04/18 19:50, Kyle Evans wrote:
> >>> >>> On Thu, Jan 4, 2018 at 9:14 PM, Frank Rowand <frowand.list@gmail.com> wrote:
> >>> >>>> On 01/04/18 18:39, Kyle Evans wrote:
> >>> >>>>> On Thu, Jan 4, 2018 at 7:55 PM, Frank Rowand <frowand.list@gmail.com> wrote:
> >>> >>>>>> On 01/04/18 13:47, Kyle Evans wrote:
> >>> >>>>>>> On Thu, Jan 4, 2018 at 3:34 PM, Kyle Evans <kevans-h+KGxgPPiorF2uMehF1BdA@public.gmane.orgg> wrote:
> >>> >>>>>>>> On Thu, Jan 4, 2018 at 2:41 PM, Kyle Evans <kevans@freebsd.org> wrote:
> >>> >>>>>>>>> On Thu, Jan 4, 2018 at 2:33 PM, Frank Rowand <frowand.list@gmail.com> wrote:
> >>> >>>>>>>>>> [... snip ...]
> >> [snip]
> >>> > Your implementation knows what my overlay means otherwise because I
> >>> > reference a labelled node using &foo, dtc generated a /__fixup__ for
> >>> > it, your implementation takes that /__fixup__ and does the lookup in
> >>> > the symbol table. The symbol exists and points to a node, what's the
> >>> > point of rejecting it there?>
> >>> > It seems unreasonable to me, because you cannot always control the
> >>> > source of your live tree. In many cases, sure, it's generated in-tree
> >>> > or in U-Boot and passed to you, and you can reasonably expect you
> >>> > won't encounter this. What if you have vendor-provided tree?
> >>> >
> >>> > I think the point I'm getting at is that it seems like this will have
> >>> > to change at some point anyways simply because you can't control all
> >>> > sources of your devicetree, and this isn't strictly wrong. Telling
> >>> > someone "No, we can't apply that overlay because your vendor's
> >>> > internal tool for generating dts and $other_format_used_internally
> >>> > simultaneously didn't generate a phandle for that" is kind of hard,> especially when your reasoning ISN'T "their blob is malformed" or
> >>> > "your overlay's reference is ambiguous" but rather, "we know what
> >>> > that's pointing at, but we just don't generate handles."
> >>>
> >>> No, the blob _is_ malformed.  I know there is no official binding
> >>> document for overlays (we do need such things once we figure out
> >>> what we are doing), so this comment is purely my opinion.
> >>
> >> In this case it's not a spec for overlays that's the issue, it's a
> >> spec for what base trees require in order to accept overlays.
> >>
> >>> The blob is malformed because it was not compiled with the "-@"
> >>> flag.  Mostly not because of anything in the source, although
> >>> again the __symbols__ node should not be hand coded.
> >>
> >> I don't think it's reasonable to claim a blob is malformed based
> >> purely on how it was generated, it needs to be something about it's
> >> actualy contents.
> >>
> >> So the question is: is "includes a phandle for every node with a
> >> symbol" a requirement for an overlay accepting base tree.  It's never
> >> been explicitly stated, since overlays were just kind of hacked
> >> together rather than fully specced out.  dtc has been written assuming
> >> that is a requirement, BSDdtc has not.
> >>
> >> I'm inclined to say "yes, it should be a requirement" on the grounds
> >> that:
> >>     a) that's the interpretation that's more widely deployed already
> >> and
> >>     b) that simplifies the overlay application logic, which generally
> >>        takes place in a more restricted environment than the initial
> >>        compilation.
> >>
> >> I'm entirely open to arguments against that position though.
> >
> > To be honest, I think both of these points are kind of wobbly. Just
> > because something has been largely interpreted a certain way does not
> > make it necessarily a good way to do so.
> >
> > It's also kind of hard to make the simplification point, my latest
> > patch [1] that I run locally doesn't really touch existing stuff all
> > that much, and it certainly doesn't feel any more complex than what
> > was already there.
> >
> > I would be inclined to say that a spec shouldn't hold a strong
> > position on this, for the following reasons:
> >
> > 1.) I've not yet seen a good technical reason for requiring explicit
> > assignment. Explicit assignment was useful when you had no other
> > method for resolving xrefs, but this isn't the case here.
> >
> > 2.) It's hard for a spec to say what the environmental restrictions
> > will be of an implementation. While you might be memory-constrained, I
> > might be disk-constrained. While you may not want to spend the extra
> > cycles to walk the tree the first time to figure out the next phandle
> > to be assigned, I might be OK with that trade-off.
> >
> > Ultimately, it should be up to the implementation actually applying
> > these overlays to decide what it's willing to accept. I don't see that
> > as a big problem, but I'm also coming from a stand-point that the only
> > *blobs* I physically receive are those that I have no real control
> > over because they come from firmware. No one that I know is physically
> > passing blobs around to be used in u-boot or otherwise (save for
> > rpi-firmware)... the transparency is crap and that's just silly.
> >
> > My suggested wording would likely be along the lines of "a base tree
> > for accepting overlays should have a phandle assigned to every node
> > that may be referenced in an overlay, but the mechanism for actually
> > applying overlays may or may not require this."
> >
> > I like the 'should' wording, because it gives room to say "Look, if
> > you're going to force a blob on people or expect these blobs to work
> > on a wide range of systems, you should really do it this way for
> > optimal compatibility" while also not restricting what I do as a
> > consenting adult in the name of keeping things clean in an environment
> > that I otherwise control.
> 
> Sorry, I meant to add- this also gives you, Frank and co. room to say
> "Sorry, this implementation doesn't accept this."

I really don't want to do that.  There's already rather a lot of fuzz
in DT and surrounding specs.  I don't want to add yet more room for
incompatible implementations if we can avoid it.

> This allows one to
> strip explicit phandles from things that their implementation does not
> want xref'd and for the resulting blob to still be considered 'overlay
> accepting' and accepted by implementations, because the
> implementations have some choice in what they accept for base trees.
> 
> Linux's loader implementation may accept willy-nilly for base trees,
> while their live tree implementation will be a lot less accepting.
> Sure, they also control the pipeline from loader to live tree, but for
> demonstration purposes it's not that crazy to consider. Their live
> tree implementation is still a valid implementation that applies
> overlays to base trees, and I consider that fact significant to this
> discussion.
> 
> It gives me room to add an optional flag to our BSDL dtc to go back to
> our former method of assigning phandles at compile-time, so that the
> default for '-@' is the compatible behavior but a minimal approach may
> be taken instead for those that choose to do so.

I'm still not seeing why you're so keen on avoiding adding phandles.
Doing so will already work with existing overlay implementations, and
it's pretty much trivial to add, so why not just make the change to
BSDdtc?

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/2] overlays: auto allocate phandles for nodes in base fdt

[David Gibson]

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

On Fri, Jan 05, 2018 at 06:47:46PM -0600, Kyle Evans wrote:
> On Fri, Jan 5, 2018 at 3:22 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > On 01/05/18 13:04, Kyle Evans wrote:
> >> On Fri, Jan 5, 2018 at 2:40 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >>> On 01/04/18 21:47, Kyle Evans wrote:
> >>>> On Thu, Jan 4, 2018 at 11:02 PM, Frank Rowand <frowand.list-Re5JQEeQqe8@public.gmane.orgm> wrote:
[snip]
> > Yes, so not a base overlay that has not been compiled by the dtc (in
> > this project) with the "-@" flag specified.  Either the __symbols__
> > node was hand coded, or a compiler other than dtc was used, which
> > chooses to not create a phandle property for a node whose label
> > is not de-referenced in the same source file.
> 
> My understanding was that libfdt was not necessarily supposed to be so
> tightly coupled to this dtc that it would make such assumptions when
> it's otherwise not a malformed blob. Ditto for the comment below about
> the use case being described in the patch- I had goofed and not
> checked that this particular dtc implementation does it differently,
> but I also was of the belief that libfdt was only supposed to be
> loosely coupled to the dtc implementation in this same repository.
> 
> Of course, I can't recall/find what I had looked at that gave me this
> impression. =)
> 
> > This use case should have been clearly described in the patch
> > description.  And David can then choose to accept or reject the
> > patch as he sees fit.  I clearly think it is a bad idea, but
> > David will either agree or disagree with my opinion.

Sorry I've taken so long to reply to all these, I've been super busy.

> David: If I may, I would appreciate if you would re-re-consider this
> patch set on the following basis:
> 
> 1.) For base blobs compiled by your dtc, this is a no-op. The latest
> version does entail one extra tree walk at the beginning of applying
> /__fixups__ even if all phandles are assigned, but I've since realized
> this could be moved further down to a tree walk the first time it has
> to assign a phandle so that it really is a no-op for blobs compiled by
> your compiler.
> 
> 2.) For other base blobs, you have compatibility. As of right now, how
> phandles are assigned in a base blob is an implementation detail,
> given that there is no clear spec on this. You gain nothing from
> remaining strict on this, and you lose compatibility with blobs that
> aren't blatantly invalid that libfdt can't control as well as
> potential (other) users of your library that may value this. Of
> course, you wouldn't lose us as users, we'd adapt.
> 
> I consider this one to be fairly important. We had to implement
> reading of older formats (see nathanw@'s recent patch to this effect)
> because we can't control what we're given in all cases. I fully expect
> that even if I'm asked to drop this and we adapt our dtc, we'll
> eventually run into a case where this is needed because of
> vendor-provided DTS via EFI.
> 
> 3.) There's nothing inherently wrong with what I've done in this
> patch, I've just supplemented the arbitrary assignment of phandles by
> dtc with arbitrary assignment of phandles by libfdt. There is no
> change to how the referenced node is looked up or generated, and this
> has no bearing on how anything is actually written.

Note that the comments I've made elsewhere in the thread about not
having seem strong arguments for your position were written before I
got to reading this mail.

You've got a reasonably compelling case above - let me mull on it a
while.

> As an aside, IMO, phandles assigned to nodes are pointless once you
> have a symbol table to reference; just a nice thing to have so you
> don't *have* to take the performance hit at runtime to assign them.

I'm really not sure what you're getting at here.  phandles are a
fundamental structural element of device trees going back to the year
dot, symbols really can't make them go away.  In particular you can't
easily determine if the phandle on a given node is necessary - doing
so requires full understanding of *every* binding used anywhere in the
tree so that you can interpret every other property to see if they
reference the phandle.

> The labels you would otherwise be losing are now stored in the blob's
> symbol table, so references are not ambiguous as they would be in a
> normal blob compiled without -@.

Not sure what you're getting at here.  References aren't _ambiguous_
without -@; they simply make no sense - what are you referencing.

> I have other thoughts about the argument of blobs being able to be
> applied to fdt but not a livetree, but I think they can all be summed
> up as: this is a non-issue if you're expecting all of your base blobs
> to be compiled by this dtc -@ anyways.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/2] overlays: auto allocate phandles for nodes in base fdt

[David Gibson]

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

On Fri, Jan 05, 2018 at 07:59:35PM -0600, Kyle Evans wrote:
> On Fri, Jan 5, 2018 at 6:47 PM, Kyle Evans <kevans-h+KGxgPPiopAfugRpC6u6w@public.gmane.org> wrote:
> > On Fri, Jan 5, 2018 at 3:22 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >> On 01/05/18 13:04, Kyle Evans wrote:
> >>> On Fri, Jan 5, 2018 at 2:40 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >>>> On 01/04/18 21:47, Kyle Evans wrote:
> >>>>> On Thu, Jan 4, 2018 at 11:02 PM, Frank Rowand <frowand.list@gmail.com> wrote:
> >>>>> Your implementation knows what my overlay means otherwise because I
> >>>>> reference a labelled node using &foo, dtc generated a /__fixup__ for
> >>>>> it, your implementation takes that /__fixup__ and does the lookup in
> >>>>> the symbol table. The symbol exists and points to a node, what's the
> >>>>> point of rejecting it there?>
> >>>>> It seems unreasonable to me, because you cannot always control the
> >>>>> source of your live tree. In many cases, sure, it's generated in-tree
> >>>>> or in U-Boot and passed to you, and you can reasonably expect you
> >>>>> won't encounter this. What if you have vendor-provided tree?
> >>>>>
> >>>>> I think the point I'm getting at is that it seems like this will have
> >>>>> to change at some point anyways simply because you can't control all
> >>>>> sources of your devicetree, and this isn't strictly wrong. Telling
> >>>>> someone "No, we can't apply that overlay because your vendor's
> >>>>> internal tool for generating dts and $other_format_used_internally
> >>>>> simultaneously didn't generate a phandle for that" is kind of hard,> especially when your reasoning ISN'T "their blob is malformed" or
> >>>>> "your overlay's reference is ambiguous" but rather, "we know what
> >>>>> that's pointing at, but we just don't generate handles."
> >>>>
> >>>> No, the blob _is_ malformed.  I know there is no official binding
> >>>> document for overlays (we do need such things once we figure out
> >>>> what we are doing), so this comment is purely my opinion.
> >>>>
> >>>> The blob is malformed because it was not compiled with the "-@"
> >>>> flag.  Mostly not because of anything in the source, although
> >>>> again the __symbols__ node should not be hand coded.
> >>>
> >>> I know this is only tangential to the point you're making above, but
> >>> the __symbols__ node in this specific example you're referencing was
> >>> hand coded by someone else, probably to avoid having to write
> >>> different invocations of DTC for the different test cases. I'm only
> >>> responsible for removing one line of this .dts, the phandle
> >>> assignment.
> >>>
> >>>> Here is an analogy: the overlay metadata is conceptually similar
> >>>> to the symbol table in an object file compiled from C source code.
> >>>> The linker will use the symbol table to link a second object file
> >>>> that contains references to the first object file, resulting in
> >>>> a program object file.  It is not normal to hand code the symbol
> >>>> table in the object file.  Similarly dtc creates the __symbols__
> >>>> node, which is essentially a symbol table.  The Linux kernel
> >>>> (or a bootloader, or whatever) performs the linking role as
> >>>> part of applying on overlay devicetree to a base devicetree.
> >>>
> >>> There is no hand coding of /__symbols__ nodes anywhere, except for
> >>> this test case that was written by someone else. I think your analogy
> >>> is inaccurate for the actual situation, though.
> >>>
> >>> The linker has all of the metadata it needs to properly link, but
> >>> refuses to for dubious reasons. It can do the lookup in the symbol
> >>> table, and that symbol table points to a valid segment of code
> >>> ("properties and subnodes"), but is refusing to complete the link
> >>> based on a missing property that's arbitrarily assigned by someone
> >>> else anyways and cannot be relied on to have any meaning or special
> >>> value.
> >>>
> >>> To be perfectly clear here: we're talking about taking a blob compiled
> >>> from a sensible overlay, such as [1], and applying it to a fellow
> >>> sensible blob that has been generated with a proper /__symbols__ node
> >>
> >>> and phandles assigned only to nodes within its tree that have been
> >>> cross-referenced by other nodes within its tree.
> >>
> >> Yes, so not a base overlay that has not been compiled by the dtc (in
> >> this project) with the "-@" flag specified.  Either the __symbols__
> >> node was hand coded, or a compiler other than dtc was used, which
> >> chooses to not create a phandle property for a node whose label
> >> is not de-referenced in the same source file.
> >
> > My understanding was that libfdt was not necessarily supposed to be so
> > tightly coupled to this dtc that it would make such assumptions when
> > it's otherwise not a malformed blob. Ditto for the comment below about
> > the use case being described in the patch- I had goofed and not
> > checked that this particular dtc implementation does it differently,
> > but I also was of the belief that libfdt was only supposed to be
> > loosely coupled to the dtc implementation in this same repository.
> >
> > Of course, I can't recall/find what I had looked at that gave me this
> > impression. =)
> >
> >> This use case should have been clearly described in the patch
> >> description.  And David can then choose to accept or reject the
> >> patch as he sees fit.  I clearly think it is a bad idea, but
> >> David will either agree or disagree with my opinion.
> >
> > David: If I may, I would appreciate if you would re-re-consider this
> > patch set on the following basis:
> >
> > 1.) For base blobs compiled by your dtc, this is a no-op. The latest
> > version does entail one extra tree walk at the beginning of applying
> > /__fixups__ even if all phandles are assigned, but I've since realized
> > this could be moved further down to a tree walk the first time it has
> > to assign a phandle so that it really is a no-op for blobs compiled by
> > your compiler.
> >
> > 2.) For other base blobs, you have compatibility. As of right now, how
> > phandles are assigned in a base blob is an implementation detail,
> > given that there is no clear spec on this. You gain nothing from
> > remaining strict on this, and you lose compatibility with blobs that
> > aren't blatantly invalid that libfdt can't control as well as
> > potential (other) users of your library that may value this. Of
> > course, you wouldn't lose us as users, we'd adapt.
> >
> > I consider this one to be fairly important. We had to implement
> > reading of older formats (see nathanw@'s recent patch to this effect)
> > because we can't control what we're given in all cases. I fully expect
> > that even if I'm asked to drop this and we adapt our dtc, we'll
> > eventually run into a case where this is needed because of
> > vendor-provided DTS via EFI.
> >
> > 3.) There's nothing inherently wrong with what I've done in this
> > patch, I've just supplemented the arbitrary assignment of phandles by
> > dtc with arbitrary assignment of phandles by libfdt. There is no
> > change to how the referenced node is looked up or generated, and this
> > has no bearing on how anything is actually written.
> >
> > As an aside, IMO, phandles assigned to nodes are pointless once you
> > have a symbol table to reference; just a nice thing to have so you
> > don't *have* to take the performance hit at runtime to assign them.
> > The labels you would otherwise be losing are now stored in the blob's
> > symbol table, so references are not ambiguous as they would be in a
> > normal blob compiled without -@.
> >
> > I have other thoughts about the argument of blobs being able to be
> > applied to fdt but not a livetree, but I think they can all be summed
> > up as: this is a non-issue if you're expecting all of your base blobs
> > to be compiled by this dtc -@ anyways.
> 
> An afterthought to this: if you wouldn't consider it as a standard
> behavior, I'd appreciate it if you would indicate possible acceptance
> of another iteration that hides the allocation behind an #ifdef
> FEATURE_ALLOCATE_PHANDLE or something of the sortr that neither you
> nor the Linux folk will ever need to turn on. I don't foresee this
> stuff changing enough in the future to actually require maintenance,
> given how non-invasive the patch can be.

No.  If I include it at all, it'll always be on: optional features can
easily make for a combinatorial explosion of possible variants which
might not all be compatible with each other.

My hesitation about the patches is nothing to do with implementation
cost.  It's all to do with the more subtle costs of having multiple
different implementations making it harder to know which pieces will
work with which others.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/2] overlays: auto allocate phandles for nodes in base fdt

[Kyle Evans]

On Tue, Jan 16, 2018 at 8:05 AM, David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Fri, Jan 05, 2018 at 06:47:46PM -0600, Kyle Evans wrote:
>> On Fri, Jan 5, 2018 at 3:22 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> > On 01/05/18 13:04, Kyle Evans wrote:
>> >> On Fri, Jan 5, 2018 at 2:40 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> >>> On 01/04/18 21:47, Kyle Evans wrote:
>> >>>> On Thu, Jan 4, 2018 at 11:02 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> [snip]
>> > Yes, so not a base overlay that has not been compiled by the dtc (in
>> > this project) with the "-@" flag specified.  Either the __symbols__
>> > node was hand coded, or a compiler other than dtc was used, which
>> > chooses to not create a phandle property for a node whose label
>> > is not de-referenced in the same source file.
>>
>> My understanding was that libfdt was not necessarily supposed to be so
>> tightly coupled to this dtc that it would make such assumptions when
>> it's otherwise not a malformed blob. Ditto for the comment below about
>> the use case being described in the patch- I had goofed and not
>> checked that this particular dtc implementation does it differently,
>> but I also was of the belief that libfdt was only supposed to be
>> loosely coupled to the dtc implementation in this same repository.
>>
>> Of course, I can't recall/find what I had looked at that gave me this
>> impression. =)
>>
>> > This use case should have been clearly described in the patch
>> > description.  And David can then choose to accept or reject the
>> > patch as he sees fit.  I clearly think it is a bad idea, but
>> > David will either agree or disagree with my opinion.
>
> Sorry I've taken so long to reply to all these, I've been super busy.
>
>> David: If I may, I would appreciate if you would re-re-consider this
>> patch set on the following basis:
>>
>> 1.) For base blobs compiled by your dtc, this is a no-op. The latest
>> version does entail one extra tree walk at the beginning of applying
>> /__fixups__ even if all phandles are assigned, but I've since realized
>> this could be moved further down to a tree walk the first time it has
>> to assign a phandle so that it really is a no-op for blobs compiled by
>> your compiler.
>>
>> 2.) For other base blobs, you have compatibility. As of right now, how
>> phandles are assigned in a base blob is an implementation detail,
>> given that there is no clear spec on this. You gain nothing from
>> remaining strict on this, and you lose compatibility with blobs that
>> aren't blatantly invalid that libfdt can't control as well as
>> potential (other) users of your library that may value this. Of
>> course, you wouldn't lose us as users, we'd adapt.
>>
>> I consider this one to be fairly important. We had to implement
>> reading of older formats (see nathanw@'s recent patch to this effect)
>> because we can't control what we're given in all cases. I fully expect
>> that even if I'm asked to drop this and we adapt our dtc, we'll
>> eventually run into a case where this is needed because of
>> vendor-provided DTS via EFI.
>>
>> 3.) There's nothing inherently wrong with what I've done in this
>> patch, I've just supplemented the arbitrary assignment of phandles by
>> dtc with arbitrary assignment of phandles by libfdt. There is no
>> change to how the referenced node is looked up or generated, and this
>> has no bearing on how anything is actually written.
>
> Note that the comments I've made elsewhere in the thread about not
> having seem strong arguments for your position were written before I
> got to reading this mail.
>
> You've got a reasonably compelling case above - let me mull on it a
> while.
>
>> As an aside, IMO, phandles assigned to nodes are pointless once you
>> have a symbol table to reference; just a nice thing to have so you
>> don't *have* to take the performance hit at runtime to assign them.
>
> I'm really not sure what you're getting at here.  phandles are a
> fundamental structural element of device trees going back to the year
> dot, symbols really can't make them go away.  In particular you can't
> easily determine if the phandle on a given node is necessary - doing
> so requires full understanding of *every* binding used anywhere in the
> tree so that you can interpret every other property to see if they
> reference the phandle.

Sorry, my wording is quite inconsistent here: the above should read
"explicit phandles" as I say in some other e-mails.

From my perspective, this isn't a particularly hard problem to solve-
dtc already has to read the entire DTS into its internal
representation before writing it to another format. It already has
some understanding of every binding used everywhere in the tree by the
time it's done. Deferring explicit phandle assignment until after it's
made a full pass over the DTS and knows what's been referenced
elsewhere isn't too bad.

>> The labels you would otherwise be losing are now stored in the blob's
>> symbol table, so references are not ambiguous as they would be in a
>> normal blob compiled without -@.
>
> Not sure what you're getting at here.  References aren't _ambiguous_
> without -@; they simply make no sense - what are you referencing.

Sorry, I've been dealing with a lot of "the labels match the unit
name" nodes lately. You're right. =) The point here is that you can
resolve these references in a DTS compiled with -@, with or without
phandles being explicitly assigned. They don't offer that much value
in this case, other than keeping the resolution of the references into
the compiler.

>> I have other thoughts about the argument of blobs being able to be
>> applied to fdt but not a livetree, but I think they can all be summed
>> up as: this is a non-issue if you're expecting all of your base blobs
>> to be compiled by this dtc -@ anyways.
>
> --
> 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

Re: [PATCH v2 1/2] overlays: auto allocate phandles for nodes in base fdt

[Kyle Evans]

On Tue, Jan 16, 2018 at 7:30 AM, David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Fri, Jan 12, 2018 at 09:57:26AM -0600, Kyle Evans wrote:
>> On Thu, Jan 11, 2018 at 11:33 PM, David Gibson
>> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
>> > On Fri, Jan 05, 2018 at 12:40:13PM -0800, Frank Rowand wrote:
>> >> On 01/04/18 21:47, Kyle Evans wrote:
>> >> > On Thu, Jan 4, 2018 at 11:02 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> >> >> On 01/04/18 19:50, Kyle Evans wrote:
>> >> >>> On Thu, Jan 4, 2018 at 9:14 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> >> >>>> On 01/04/18 18:39, Kyle Evans wrote:
>> >> >>>>> On Thu, Jan 4, 2018 at 7:55 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> >> >>>>>> On 01/04/18 13:47, Kyle Evans wrote:
>> >> >>>>>>> On Thu, Jan 4, 2018 at 3:34 PM, Kyle Evans <kevans-h+KGxgPPiopAfugRpC6u6w@public.gmane.org> wrote:
>> >> >>>>>>>> On Thu, Jan 4, 2018 at 2:41 PM, Kyle Evans <kevans-h+KGxgPPiopAfugRpC6u6w@public.gmane.org> wrote:
>> >> >>>>>>>>> On Thu, Jan 4, 2018 at 2:33 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> >> >>>>>>>>>> [... snip ...]
>> > [snip]
>> >> > Your implementation knows what my overlay means otherwise because I
>> >> > reference a labelled node using &foo, dtc generated a /__fixup__ for
>> >> > it, your implementation takes that /__fixup__ and does the lookup in
>> >> > the symbol table. The symbol exists and points to a node, what's the
>> >> > point of rejecting it there?>
>> >> > It seems unreasonable to me, because you cannot always control the
>> >> > source of your live tree. In many cases, sure, it's generated in-tree
>> >> > or in U-Boot and passed to you, and you can reasonably expect you
>> >> > won't encounter this. What if you have vendor-provided tree?
>> >> >
>> >> > I think the point I'm getting at is that it seems like this will have
>> >> > to change at some point anyways simply because you can't control all
>> >> > sources of your devicetree, and this isn't strictly wrong. Telling
>> >> > someone "No, we can't apply that overlay because your vendor's
>> >> > internal tool for generating dts and $other_format_used_internally
>> >> > simultaneously didn't generate a phandle for that" is kind of hard,> especially when your reasoning ISN'T "their blob is malformed" or
>> >> > "your overlay's reference is ambiguous" but rather, "we know what
>> >> > that's pointing at, but we just don't generate handles."
>> >>
>> >> No, the blob _is_ malformed.  I know there is no official binding
>> >> document for overlays (we do need such things once we figure out
>> >> what we are doing), so this comment is purely my opinion.
>> >
>> > In this case it's not a spec for overlays that's the issue, it's a
>> > spec for what base trees require in order to accept overlays.
>> >
>> >> The blob is malformed because it was not compiled with the "-@"
>> >> flag.  Mostly not because of anything in the source, although
>> >> again the __symbols__ node should not be hand coded.
>> >
>> > I don't think it's reasonable to claim a blob is malformed based
>> > purely on how it was generated, it needs to be something about it's
>> > actualy contents.
>> >
>> > So the question is: is "includes a phandle for every node with a
>> > symbol" a requirement for an overlay accepting base tree.  It's never
>> > been explicitly stated, since overlays were just kind of hacked
>> > together rather than fully specced out.  dtc has been written assuming
>> > that is a requirement, BSDdtc has not.
>> >
>> > I'm inclined to say "yes, it should be a requirement" on the grounds
>> > that:
>> >     a) that's the interpretation that's more widely deployed already
>> > and
>> >     b) that simplifies the overlay application logic, which generally
>> >        takes place in a more restricted environment than the initial
>> >        compilation.
>> >
>> > I'm entirely open to arguments against that position though.
>>
>> To be honest, I think both of these points are kind of wobbly.
>
> I'm not claiming they're conclusive, just the best arguments I've seen
> so far.
>
>> Just
>> because something has been largely interpreted a certain way does not
>> make it necessarily a good way to do so.
>
> No, but the fact that widely deployed implementations rely on a
> certain interpretation is a fairly strong argument for keeping it,
> even if it's not the best by other measures.
>
>> It's also kind of hard to make the simplification point, my latest
>> patch [1] that I run locally doesn't really touch existing stuff all
>> that much, and it certainly doesn't feel any more complex than what
>> was already there.
>
> It adds and doesn't remove code, so I think it's hard to argue it
> doesn't add complexity.

I think we have different definitions of complexity, then. =) To me,
this isn't added complexity unless it actually entails some kind of
real re-design and work put into it.

>> I would be inclined to say that a spec shouldn't hold a strong
>> position on this, for the following reasons:
>>
>> 1.) I've not yet seen a good technical reason for requiring explicit
>> assignment. Explicit assignment was useful when you had no other
>> method for resolving xrefs, but this isn't the case here.
>
> I'm not really sure what you mean by explicit assignment.

Explicit assignment of phandles to a node.

>> 2.) It's hard for a spec to say what the environmental restrictions
>> will be of an implementation. While you might be memory-constrained, I
>> might be disk-constrained. While you may not want to spend the extra
>> cycles to walk the tree the first time to figure out the next phandle
>> to be assigned, I might be OK with that trade-off.
>
> True in principle, but stretched to breaking point in practice.  Given
> that overlay application typically occurs in a bootloader, and
> complication typically occurs in a full userspace environment, it's
> pretty hard to see the overlay application environment as anything
> other than (at least potentially) vastly more constrained in every
> plausibly relevant dimension.
>
>> Ultimately, it should be up to the implementation actually applying
>> these overlays to decide what it's willing to accept.
>
> Not if this is going to have relevant meaning as a
> cross-implementation standard, even a de facto one.
>
>> I don't see that
>> as a big problem, but I'm also coming from a stand-point that the only
>> *blobs* I physically receive are those that I have no real control
>> over because they come from firmware. No one that I know is physically
>> passing blobs around to be used in u-boot or otherwise (save for
>> rpi-firmware)... the transparency is crap and that's just silly.
>>
>> My suggested wording would likely be along the lines of "a base tree
>> for accepting overlays should have a phandle assigned to every node
>> that may be referenced in an overlay, but the mechanism for actually
>> applying overlays may or may not require this."
>>
>> I like the 'should' wording, because it gives room to say "Look, if
>> you're going to force a blob on people or expect these blobs to work
>> on a wide range of systems, you should really do it this way for
>> optimal compatibility" while also not restricting what I do as a
>> consenting adult in the name of keeping things clean in an environment
>> that I otherwise control.
>
> Well, that's fair enough, I guess.
>
> Have you encountered the missing phandles with any blobs you've
> encountered *other* than ones you've built with BSDdtc?

Negative, yet. We also hadn't encountered older FDT blobs that libfdt
couldn't handle for a while either, so that's not necessarily saying
much.

> I mean, the basic position here is that we have a working[1]
> implementation on both sides.  There is no formal spec - that's not a
> good thing, but it's where we're at.  If your new implementation wants
> to take a different interpretation, the onus is really on you to make
> a concrete case that's stronger than staying with what we have.  So
> far the best I've seen is "lots of phandles are inconvenient for our
> kernel for poorly articulated reasons".  That doesn't have no weight,
> but I don't think you've yet passed the threshhold required to
> overcome inertia.
>
> [1] To the extent that overlays can work, I have many, many issues
> with the format, but that's a discussion that's been had elsewhere.
>
> --
> 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

Re: [PATCH v2 1/2] overlays: auto allocate phandles for nodes in base fdt

[Kyle Evans]

On Tue, Jan 16, 2018 at 7:34 AM, David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Fri, Jan 12, 2018 at 10:24:34AM -0600, Kyle Evans wrote:
>> On Fri, Jan 12, 2018 at 9:57 AM, Kyle Evans <kevans-h+KGxgPPiopAfugRpC6u6w@public.gmane.org> wrote:
>> > On Thu, Jan 11, 2018 at 11:33 PM, David Gibson
>> > <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
>> >> On Fri, Jan 05, 2018 at 12:40:13PM -0800, Frank Rowand wrote:
>> >>> On 01/04/18 21:47, Kyle Evans wrote:
>> >>> > On Thu, Jan 4, 2018 at 11:02 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> >>> >> On 01/04/18 19:50, Kyle Evans wrote:
>> >>> >>> On Thu, Jan 4, 2018 at 9:14 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> >>> >>>> On 01/04/18 18:39, Kyle Evans wrote:
>> >>> >>>>> On Thu, Jan 4, 2018 at 7:55 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> >>> >>>>>> On 01/04/18 13:47, Kyle Evans wrote:
>> >>> >>>>>>> On Thu, Jan 4, 2018 at 3:34 PM, Kyle Evans <kevans-h+KGxgPPiopAfugRpC6u6w@public.gmane.org> wrote:
>> >>> >>>>>>>> On Thu, Jan 4, 2018 at 2:41 PM, Kyle Evans <kevans-h+KGxgPPiopAfugRpC6u6w@public.gmane.org> wrote:
>> >>> >>>>>>>>> On Thu, Jan 4, 2018 at 2:33 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> >>> >>>>>>>>>> [... snip ...]
>> >> [snip]
>> >>> > Your implementation knows what my overlay means otherwise because I
>> >>> > reference a labelled node using &foo, dtc generated a /__fixup__ for
>> >>> > it, your implementation takes that /__fixup__ and does the lookup in
>> >>> > the symbol table. The symbol exists and points to a node, what's the
>> >>> > point of rejecting it there?>
>> >>> > It seems unreasonable to me, because you cannot always control the
>> >>> > source of your live tree. In many cases, sure, it's generated in-tree
>> >>> > or in U-Boot and passed to you, and you can reasonably expect you
>> >>> > won't encounter this. What if you have vendor-provided tree?
>> >>> >
>> >>> > I think the point I'm getting at is that it seems like this will have
>> >>> > to change at some point anyways simply because you can't control all
>> >>> > sources of your devicetree, and this isn't strictly wrong. Telling
>> >>> > someone "No, we can't apply that overlay because your vendor's
>> >>> > internal tool for generating dts and $other_format_used_internally
>> >>> > simultaneously didn't generate a phandle for that" is kind of hard,> especially when your reasoning ISN'T "their blob is malformed" or
>> >>> > "your overlay's reference is ambiguous" but rather, "we know what
>> >>> > that's pointing at, but we just don't generate handles."
>> >>>
>> >>> No, the blob _is_ malformed.  I know there is no official binding
>> >>> document for overlays (we do need such things once we figure out
>> >>> what we are doing), so this comment is purely my opinion.
>> >>
>> >> In this case it's not a spec for overlays that's the issue, it's a
>> >> spec for what base trees require in order to accept overlays.
>> >>
>> >>> The blob is malformed because it was not compiled with the "-@"
>> >>> flag.  Mostly not because of anything in the source, although
>> >>> again the __symbols__ node should not be hand coded.
>> >>
>> >> I don't think it's reasonable to claim a blob is malformed based
>> >> purely on how it was generated, it needs to be something about it's
>> >> actualy contents.
>> >>
>> >> So the question is: is "includes a phandle for every node with a
>> >> symbol" a requirement for an overlay accepting base tree.  It's never
>> >> been explicitly stated, since overlays were just kind of hacked
>> >> together rather than fully specced out.  dtc has been written assuming
>> >> that is a requirement, BSDdtc has not.
>> >>
>> >> I'm inclined to say "yes, it should be a requirement" on the grounds
>> >> that:
>> >>     a) that's the interpretation that's more widely deployed already
>> >> and
>> >>     b) that simplifies the overlay application logic, which generally
>> >>        takes place in a more restricted environment than the initial
>> >>        compilation.
>> >>
>> >> I'm entirely open to arguments against that position though.
>> >
>> > To be honest, I think both of these points are kind of wobbly. Just
>> > because something has been largely interpreted a certain way does not
>> > make it necessarily a good way to do so.
>> >
>> > It's also kind of hard to make the simplification point, my latest
>> > patch [1] that I run locally doesn't really touch existing stuff all
>> > that much, and it certainly doesn't feel any more complex than what
>> > was already there.
>> >
>> > I would be inclined to say that a spec shouldn't hold a strong
>> > position on this, for the following reasons:
>> >
>> > 1.) I've not yet seen a good technical reason for requiring explicit
>> > assignment. Explicit assignment was useful when you had no other
>> > method for resolving xrefs, but this isn't the case here.
>> >
>> > 2.) It's hard for a spec to say what the environmental restrictions
>> > will be of an implementation. While you might be memory-constrained, I
>> > might be disk-constrained. While you may not want to spend the extra
>> > cycles to walk the tree the first time to figure out the next phandle
>> > to be assigned, I might be OK with that trade-off.
>> >
>> > Ultimately, it should be up to the implementation actually applying
>> > these overlays to decide what it's willing to accept. I don't see that
>> > as a big problem, but I'm also coming from a stand-point that the only
>> > *blobs* I physically receive are those that I have no real control
>> > over because they come from firmware. No one that I know is physically
>> > passing blobs around to be used in u-boot or otherwise (save for
>> > rpi-firmware)... the transparency is crap and that's just silly.
>> >
>> > My suggested wording would likely be along the lines of "a base tree
>> > for accepting overlays should have a phandle assigned to every node
>> > that may be referenced in an overlay, but the mechanism for actually
>> > applying overlays may or may not require this."
>> >
>> > I like the 'should' wording, because it gives room to say "Look, if
>> > you're going to force a blob on people or expect these blobs to work
>> > on a wide range of systems, you should really do it this way for
>> > optimal compatibility" while also not restricting what I do as a
>> > consenting adult in the name of keeping things clean in an environment
>> > that I otherwise control.
>>
>> Sorry, I meant to add- this also gives you, Frank and co. room to say
>> "Sorry, this implementation doesn't accept this."
>
> I really don't want to do that.  There's already rather a lot of fuzz
> in DT and surrounding specs.  I don't want to add yet more room for
> incompatible implementations if we can avoid it.

Fair enough.

>> This allows one to
>> strip explicit phandles from things that their implementation does not
>> want xref'd and for the resulting blob to still be considered 'overlay
>> accepting' and accepted by implementations, because the
>> implementations have some choice in what they accept for base trees.
>>
>> Linux's loader implementation may accept willy-nilly for base trees,
>> while their live tree implementation will be a lot less accepting.
>> Sure, they also control the pipeline from loader to live tree, but for
>> demonstration purposes it's not that crazy to consider. Their live
>> tree implementation is still a valid implementation that applies
>> overlays to base trees, and I consider that fact significant to this
>> discussion.
>>
>> It gives me room to add an optional flag to our BSDL dtc to go back to
>> our former method of assigning phandles at compile-time, so that the
>> default for '-@' is the compatible behavior but a minimal approach may
>> be taken instead for those that choose to do so.
>
> I'm still not seeing why you're so keen on avoiding adding phandles.
> Doing so will already work with existing overlay implementations, and
> it's pretty much trivial to add, so why not just make the change to
> BSDdtc?

I've mentioned this a couple of times, I think, in e-mails you might
not have read yet- I have made the changes in BSDL dtc at least a
couple weeks ago to assign phandles to all labelled nodes. At this
point, it was about being flexible when possible rather than
unnecessarily rigid when it has no technical reason to be.

As I just saw your e-mail about not accepting it if it's behind an
#ifdef, this whole thing is moot.

Re: [PATCH v2 1/2] overlays: auto allocate phandles for nodes in base fdt

[David Gibson]

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

On Tue, Jan 16, 2018 at 09:24:44AM -0600, Kyle Evans wrote:
> On Tue, Jan 16, 2018 at 8:05 AM, David Gibson
> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > On Fri, Jan 05, 2018 at 06:47:46PM -0600, Kyle Evans wrote:
> >> On Fri, Jan 5, 2018 at 3:22 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >> > On 01/05/18 13:04, Kyle Evans wrote:
> >> >> On Fri, Jan 5, 2018 at 2:40 PM, Frank Rowand <frowand.list-Re5JQEeQqe8@public.gmane.orgm> wrote:
> >> >>> On 01/04/18 21:47, Kyle Evans wrote:
> >> >>>> On Thu, Jan 4, 2018 at 11:02 PM, Frank Rowand <frowand.list@gmail.com> wrote:
> > [snip]
> >> > Yes, so not a base overlay that has not been compiled by the dtc (in
> >> > this project) with the "-@" flag specified.  Either the __symbols__
> >> > node was hand coded, or a compiler other than dtc was used, which
> >> > chooses to not create a phandle property for a node whose label
> >> > is not de-referenced in the same source file.
> >>
> >> My understanding was that libfdt was not necessarily supposed to be so
> >> tightly coupled to this dtc that it would make such assumptions when
> >> it's otherwise not a malformed blob. Ditto for the comment below about
> >> the use case being described in the patch- I had goofed and not
> >> checked that this particular dtc implementation does it differently,
> >> but I also was of the belief that libfdt was only supposed to be
> >> loosely coupled to the dtc implementation in this same repository.
> >>
> >> Of course, I can't recall/find what I had looked at that gave me this
> >> impression. =)
> >>
> >> > This use case should have been clearly described in the patch
> >> > description.  And David can then choose to accept or reject the
> >> > patch as he sees fit.  I clearly think it is a bad idea, but
> >> > David will either agree or disagree with my opinion.
> >
> > Sorry I've taken so long to reply to all these, I've been super busy.
> >
> >> David: If I may, I would appreciate if you would re-re-consider this
> >> patch set on the following basis:
> >>
> >> 1.) For base blobs compiled by your dtc, this is a no-op. The latest
> >> version does entail one extra tree walk at the beginning of applying
> >> /__fixups__ even if all phandles are assigned, but I've since realized
> >> this could be moved further down to a tree walk the first time it has
> >> to assign a phandle so that it really is a no-op for blobs compiled by
> >> your compiler.
> >>
> >> 2.) For other base blobs, you have compatibility. As of right now, how
> >> phandles are assigned in a base blob is an implementation detail,
> >> given that there is no clear spec on this. You gain nothing from
> >> remaining strict on this, and you lose compatibility with blobs that
> >> aren't blatantly invalid that libfdt can't control as well as
> >> potential (other) users of your library that may value this. Of
> >> course, you wouldn't lose us as users, we'd adapt.
> >>
> >> I consider this one to be fairly important. We had to implement
> >> reading of older formats (see nathanw@'s recent patch to this effect)
> >> because we can't control what we're given in all cases. I fully expect
> >> that even if I'm asked to drop this and we adapt our dtc, we'll
> >> eventually run into a case where this is needed because of
> >> vendor-provided DTS via EFI.
> >>
> >> 3.) There's nothing inherently wrong with what I've done in this
> >> patch, I've just supplemented the arbitrary assignment of phandles by
> >> dtc with arbitrary assignment of phandles by libfdt. There is no
> >> change to how the referenced node is looked up or generated, and this
> >> has no bearing on how anything is actually written.
> >
> > Note that the comments I've made elsewhere in the thread about not
> > having seem strong arguments for your position were written before I
> > got to reading this mail.
> >
> > You've got a reasonably compelling case above - let me mull on it a
> > while.
> >
> >> As an aside, IMO, phandles assigned to nodes are pointless once you
> >> have a symbol table to reference; just a nice thing to have so you
> >> don't *have* to take the performance hit at runtime to assign them.
> >
> > I'm really not sure what you're getting at here.  phandles are a
> > fundamental structural element of device trees going back to the year
> > dot, symbols really can't make them go away.  In particular you can't
> > easily determine if the phandle on a given node is necessary - doing
> > so requires full understanding of *every* binding used anywhere in the
> > tree so that you can interpret every other property to see if they
> > reference the phandle.
> 
> Sorry, my wording is quite inconsistent here: the above should read
> "explicit phandles" as I say in some other e-mails.
> 
> From my perspective, this isn't a particularly hard problem to solve-
> dtc already has to read the entire DTS into its internal
> representation before writing it to another format. It already has
> some understanding of every binding used everywhere in the tree by the
> time it's done. Deferring explicit phandle assignment until after it's
> made a full pass over the DTS and knows what's been referenced
> elsewhere isn't too bad.

That doesn't follow.  dtc only operates more-or-less at the structural
level - it knows what the nodes are and what the properties are at
byte strings, but not how those byte strings are interpreted.  That
means it *doesn't* know about bindings, which tell you how to
interpret each property - whether it consists of addresses, strings,
phandles or whatever else.

dtc gets a *hint* about phandles, if you use the reference syntax, but
it's just a hint - and it's lost after the tree has passed through the
compiler once.

> 
> >> The labels you would otherwise be losing are now stored in the blob's
> >> symbol table, so references are not ambiguous as they would be in a
> >> normal blob compiled without -@.
> >
> > Not sure what you're getting at here.  References aren't _ambiguous_
> > without -@; they simply make no sense - what are you referencing.
> 
> Sorry, I've been dealing with a lot of "the labels match the unit
> name" nodes lately. You're right. =) The point here is that you can
> resolve these references in a DTS compiled with -@, with or without
> phandles being explicitly assigned. They don't offer that much value
> in this case, other than keeping the resolution of the references into
> the compiler.

I don't see what you mean by "keeping the resolution of the references
into the compiler".

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/2] overlays: auto allocate phandles for nodes in base fdt

[David Gibson]

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

On Tue, Jan 16, 2018 at 09:32:41AM -0600, Kyle Evans wrote:
> On Tue, Jan 16, 2018 at 7:30 AM, David Gibson
> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > On Fri, Jan 12, 2018 at 09:57:26AM -0600, Kyle Evans wrote:
> >> On Thu, Jan 11, 2018 at 11:33 PM, David Gibson
> >> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> >> > On Fri, Jan 05, 2018 at 12:40:13PM -0800, Frank Rowand wrote:
> >> >> On 01/04/18 21:47, Kyle Evans wrote:
> >> >> > On Thu, Jan 4, 2018 at 11:02 PM, Frank Rowand <frowand.list@gmail.com> wrote:
> >> >> >> On 01/04/18 19:50, Kyle Evans wrote:
> >> >> >>> On Thu, Jan 4, 2018 at 9:14 PM, Frank Rowand <frowand.list@gmail.com> wrote:
> >> >> >>>> On 01/04/18 18:39, Kyle Evans wrote:
> >> >> >>>>> On Thu, Jan 4, 2018 at 7:55 PM, Frank Rowand <frowand.list@gmail.com> wrote:
> >> >> >>>>>> On 01/04/18 13:47, Kyle Evans wrote:
> >> >> >>>>>>> On Thu, Jan 4, 2018 at 3:34 PM, Kyle Evans <kevans@freebsd.org> wrote:
> >> >> >>>>>>>> On Thu, Jan 4, 2018 at 2:41 PM, Kyle Evans <kevans@freebsd.org> wrote:
> >> >> >>>>>>>>> On Thu, Jan 4, 2018 at 2:33 PM, Frank Rowand <frowand.list@gmail.com> wrote:
> >> >> >>>>>>>>>> [... snip ...]
> >> > [snip]
> >> >> > Your implementation knows what my overlay means otherwise because I
> >> >> > reference a labelled node using &foo, dtc generated a /__fixup__ for
> >> >> > it, your implementation takes that /__fixup__ and does the lookup in
> >> >> > the symbol table. The symbol exists and points to a node, what's the
> >> >> > point of rejecting it there?>
> >> >> > It seems unreasonable to me, because you cannot always control the
> >> >> > source of your live tree. In many cases, sure, it's generated in-tree
> >> >> > or in U-Boot and passed to you, and you can reasonably expect you
> >> >> > won't encounter this. What if you have vendor-provided tree?
> >> >> >
> >> >> > I think the point I'm getting at is that it seems like this will have
> >> >> > to change at some point anyways simply because you can't control all
> >> >> > sources of your devicetree, and this isn't strictly wrong. Telling
> >> >> > someone "No, we can't apply that overlay because your vendor's
> >> >> > internal tool for generating dts and $other_format_used_internally
> >> >> > simultaneously didn't generate a phandle for that" is kind of hard,> especially when your reasoning ISN'T "their blob is malformed" or
> >> >> > "your overlay's reference is ambiguous" but rather, "we know what
> >> >> > that's pointing at, but we just don't generate handles."
> >> >>
> >> >> No, the blob _is_ malformed.  I know there is no official binding
> >> >> document for overlays (we do need such things once we figure out
> >> >> what we are doing), so this comment is purely my opinion.
> >> >
> >> > In this case it's not a spec for overlays that's the issue, it's a
> >> > spec for what base trees require in order to accept overlays.
> >> >
> >> >> The blob is malformed because it was not compiled with the "-@"
> >> >> flag.  Mostly not because of anything in the source, although
> >> >> again the __symbols__ node should not be hand coded.
> >> >
> >> > I don't think it's reasonable to claim a blob is malformed based
> >> > purely on how it was generated, it needs to be something about it's
> >> > actualy contents.
> >> >
> >> > So the question is: is "includes a phandle for every node with a
> >> > symbol" a requirement for an overlay accepting base tree.  It's never
> >> > been explicitly stated, since overlays were just kind of hacked
> >> > together rather than fully specced out.  dtc has been written assuming
> >> > that is a requirement, BSDdtc has not.
> >> >
> >> > I'm inclined to say "yes, it should be a requirement" on the grounds
> >> > that:
> >> >     a) that's the interpretation that's more widely deployed already
> >> > and
> >> >     b) that simplifies the overlay application logic, which generally
> >> >        takes place in a more restricted environment than the initial
> >> >        compilation.
> >> >
> >> > I'm entirely open to arguments against that position though.
> >>
> >> To be honest, I think both of these points are kind of wobbly.
> >
> > I'm not claiming they're conclusive, just the best arguments I've seen
> > so far.
> >
> >> Just
> >> because something has been largely interpreted a certain way does not
> >> make it necessarily a good way to do so.
> >
> > No, but the fact that widely deployed implementations rely on a
> > certain interpretation is a fairly strong argument for keeping it,
> > even if it's not the best by other measures.
> >
> >> It's also kind of hard to make the simplification point, my latest
> >> patch [1] that I run locally doesn't really touch existing stuff all
> >> that much, and it certainly doesn't feel any more complex than what
> >> was already there.
> >
> > It adds and doesn't remove code, so I think it's hard to argue it
> > doesn't add complexity.
> 
> I think we have different definitions of complexity, then. =) To me,
> this isn't added complexity unless it actually entails some kind of
> real re-design and work put into it.
> 
> >> I would be inclined to say that a spec shouldn't hold a strong
> >> position on this, for the following reasons:
> >>
> >> 1.) I've not yet seen a good technical reason for requiring explicit
> >> assignment. Explicit assignment was useful when you had no other
> >> method for resolving xrefs, but this isn't the case here.
> >
> > I'm not really sure what you mean by explicit assignment.
> 
> Explicit assignment of phandles to a node.

Um.. that didn't help.  As far as I can tell we're discussing implicit
assignment of phandles to nodes by the compiler, versus implicit
assignment of phandles to nodes by the overlay applier.  Nothing
explicit about either case.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/2] overlays: auto allocate phandles for nodes in base fdt

[David Gibson]

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

On Thu, Jan 04, 2018 at 08:21:34AM -0600, Kyle Evans wrote:
> On Wed, Jan 3, 2018 at 9:26 PM, David Gibson
> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > On Wed, Jan 03, 2018 at 08:04:52AM -0600, Kyle Evans wrote:
> >> [... snip ...]
> >> The problem is that these overlays aren't necessarily curated by a
> >> single authority, so conflicts with multiple overlays trying to
> >> reference a node is scary. Ideally, we'd like these things to be as
> >> usable as possible for average Joe Blow.
> >
> > Yeah, fair enough.  I'll re-evaluate your patches with that in mind.
> 
> I appreciate your consideration, thank you. =) I'm not necessarily too
> keen on requiring our users to likely have to disassemble every
> overlay they receive or consider which order they apply in if they're
> not functionally related.
> 
> It also helps me out as I work on hardware that doesn't have complete
> DTS in mainline Linux yet, so I'm frequently adding nodes via overlay
> that reference base nodes not yet with phandles. Adding phandles to
> everything as I go gets kind of messy in my overlays and makes it that
> much more difficult to assemble an upstreamable patch.
> 
> I've also had fleeting thoughts of writing a tool like dtdiff, but
> will actually generate an overlay of the difference between the two
> dts being compared. The main use case here being that we follow Linux
> releases (not -rc) for our dts, so automatically generating a DTBO
> from where we're at to the next stable release (perhaps in the later
> -rc stage) would be really really helpful to evaluate if we still work
> and what we need to fix.

That's a cool idea.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/2] overlays: auto allocate phandles for nodes in base fdt

[Frank Rowand]

On 12/31/17 22:59, kevans-HZy0K5TPuP5AfugRpC6u6w@public.gmane.org wrote:
> Currently, references cannot be made to nodes in the base that do not already
> have phandles, limiting us to nodes that have been referenced in the base fdt.
> Lift that restriction by allocating them on an as-needed basis.

I am very confused.  If I understand correctly, the problem is:

  A base devicetree contains a __symbols__ node, where one of the properties
  in that node has a value which is a path to a node, where that node does
  not have a phandle property.

Is that correct?

> 
> Signed-off-by: Kyle Evans <kevans-HZy0K5TPuP5AfugRpC6u6w@public.gmane.org>
> ---
> 
> Changes since v1:
>  - Added a function to grab the next phandle; once we've assigned one phandle
>  to a node automatically, we'll need to choose max phandle from the base
>  blob instead of the overlay since they've not necessarily been merged yet.
> 
>  libfdt/fdt_overlay.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 74 insertions(+), 9 deletions(-)
> 
> diff --git a/libfdt/fdt_overlay.c b/libfdt/fdt_overlay.c
> index bd81241..10a57ae 100644
> --- a/libfdt/fdt_overlay.c
> +++ b/libfdt/fdt_overlay.c
> @@ -335,6 +335,66 @@ static int overlay_update_local_references(void *fdto, uint32_t delta)
>  						    delta);
>  }
>  
> +/**
> + * overlay_next_phandle - Grab next phandle to be assigned
> + * @fdt: Base Device Tree blob
> + * @fdto: Device tree overlay blob
> + *
> + * overlay_next_phandle() determines the next phandle to be assigned to the
> + * fdt blob.
> + *
> + * This is part of the device tree overlay application process, when you need to
> + * assign a phandle to a node that doesn't currently have one.
> + *
> + * returns:
> + *      Next phandle to be assigned on success
> + *      Negative error code on failure
> + */
> +static int overlay_next_phandle(void *fdt, void *fdto)
> +{
> +	int base_max, overlay_max;
> +
> +	base_max = fdt_get_max_phandle(fdt);
> +	if (base_max < 0)
> +		return base_max;
> +	overlay_max = fdt_get_max_phandle(fdto);
> +	if (overlay_max < 0)
> +		return overlay_max;
> +	return (base_max > overlay_max ? base_max : overlay_max) + 1;
> +}
> +
> +/**
> + * overlay_assign_phandle - Assign a phandle to a symbol in the base fdt
> + * @fdt: Base Device Tree blob
> + * @fdto: Device tree overlay blob
> + * @symbol_off: Node offset of the symbol to be assigned a phandle
> + *
> + * overlay_assign_phandle() assigns the next phandle available to the requested
> + * node in the base device tree.
> + *
> + * This is part of the device tree overlay application process, when you want to
> + * reference a symbol in the base device tree that doesn't yet have a phandle.
> + *
> + * returns:
> + *      phandle assigned on success
> + *      0 on failure
> + */
> +static int overlay_assign_phandle(void *fdt, void *fdto, int symbol_off)
> +{
> +	int phandle, ret;
> +	fdt32_t phandle_val;
> +
> +	/* Overlay phandles have already been adjusted, grab highest phandle */
> +	phandle = overlay_next_phandle(fdt, fdto);
> +	if (phandle < 0)
> +		return 0;
> +	phandle_val = cpu_to_fdt32(phandle);
> +	ret = fdt_setprop(fdt, symbol_off, "phandle", &phandle_val, sizeof(phandle_val));
> +	if (!ret)
> +		return phandle;
> +	return 0;
> +}
> +
>  /**
>   * overlay_fixup_one_phandle - Set an overlay phandle to the base one
>   * @fdt: Base Device Tree blob
> @@ -359,7 +419,7 @@ static int overlay_update_local_references(void *fdto, uint32_t delta)
>   *      Negative error code on failure
>   */
>  static int overlay_fixup_one_phandle(void *fdt, void *fdto,
> -				     int symbols_off,
> +				     int *symbols_off,
>  				     const char *path, uint32_t path_len,
>  				     const char *name, uint32_t name_len,
>  				     int poffset, const char *label)
> @@ -370,10 +430,10 @@ static int overlay_fixup_one_phandle(void *fdt, void *fdto,
>  	int symbol_off, fixup_off;
>  	int prop_len;
>  
> -	if (symbols_off < 0)
> -		return symbols_off;
> +	if (*symbols_off < 0)
> +		return *symbols_off;
>  
> -	symbol_path = fdt_getprop(fdt, symbols_off, label,
> +	symbol_path = fdt_getprop(fdt, *symbols_off, label,
>  				  &prop_len);
>  	if (!symbol_path)
>  		return prop_len;
> @@ -383,8 +443,14 @@ static int overlay_fixup_one_phandle(void *fdt, void *fdto,
>  		return symbol_off;
>  
>  	phandle = fdt_get_phandle(fdt, symbol_off);
> -	if (!phandle)
> -		return -FDT_ERR_NOTFOUND;
> +	if (!phandle) {
> +		phandle = overlay_assign_phandle(fdt, fdto, symbol_off);
> +		if (phandle == 0)
> +			return -FDT_ERR_NOTFOUND;
> +
> +		/* Re-lookup symbols offset, it's been invalidated */
> +		*symbols_off = fdt_path_offset(fdt, "/__symbols__");
> +	}
>  
>  	fixup_off = fdt_path_offset_namelen(fdto, path, path_len);
>  	if (fixup_off == -FDT_ERR_NOTFOUND)
> @@ -418,7 +484,7 @@ static int overlay_fixup_one_phandle(void *fdt, void *fdto,
>   *      0 on success
>   *      Negative error code on failure
>   */
> -static int overlay_fixup_phandle(void *fdt, void *fdto, int symbols_off,
> +static int overlay_fixup_phandle(void *fdt, void *fdto, int *symbols_off,
>  				 int property)
>  {
>  	const char *value;
> @@ -519,8 +585,7 @@ static int overlay_fixup_phandles(void *fdt, void *fdto)
>  
>  	fdt_for_each_property_offset(property, fdto, fixups_off) {
>  		int ret;
> -
> -		ret = overlay_fixup_phandle(fdt, fdto, symbols_off, property);
> +		ret = overlay_fixup_phandle(fdt, fdto, &symbols_off, property);
>  		if (ret)
>  			return ret;
>  	}
> 

Re: [PATCH v2 1/2] overlays: auto allocate phandles for nodes in base fdt

[Kyle Evans]

On Wed, Jan 3, 2018 at 9:26 PM, David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Wed, Jan 03, 2018 at 08:04:52AM -0600, Kyle Evans wrote:
>> [... snip ...]
>> The problem is that these overlays aren't necessarily curated by a
>> single authority, so conflicts with multiple overlays trying to
>> reference a node is scary. Ideally, we'd like these things to be as
>> usable as possible for average Joe Blow.
>
> Yeah, fair enough.  I'll re-evaluate your patches with that in mind.

I appreciate your consideration, thank you. =) I'm not necessarily too
keen on requiring our users to likely have to disassemble every
overlay they receive or consider which order they apply in if they're
not functionally related.

It also helps me out as I work on hardware that doesn't have complete
DTS in mainline Linux yet, so I'm frequently adding nodes via overlay
that reference base nodes not yet with phandles. Adding phandles to
everything as I go gets kind of messy in my overlays and makes it that
much more difficult to assemble an upstreamable patch.

I've also had fleeting thoughts of writing a tool like dtdiff, but
will actually generate an overlay of the difference between the two
dts being compared. The main use case here being that we follow Linux
releases (not -rc) for our dts, so automatically generating a DTBO
from where we're at to the next stable release (perhaps in the later
-rc stage) would be really really helpful to evaluate if we still work
and what we need to fix.

Re: [PATCH v2 1/2] overlays: auto allocate phandles for nodes in base fdt

[David Gibson]

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

On Wed, Jan 03, 2018 at 08:04:52AM -0600, Kyle Evans wrote:
> [Resending with a proper mail client, because it didn't go to the lists]
> 
> On Tue, Jan 2, 2018 at 11:42 PM, David Gibson
> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > Regardless of anything else, these two patches need different one-line
> > summaries.
> >
> > On Mon, Jan 01, 2018 at 12:59:44AM -0600, kevans-HZy0K5TPuP5AfugRpC6u6w@public.gmane.org wrote:
> >> Currently, references cannot be made to nodes in the base that do not already
> >> have phandles, limiting us to nodes that have been referenced in the base fdt.
> >> Lift that restriction by allocating them on an as-needed basis.
> >
> > Hmm.  I'm a bit dubious about this.
> >
> > - My feeling is that one of the problems with the overlay format is
> >   that it's already too free, allowing the overlay to change
> >   essentially anything in the base tree.  So I'm not that keen on
> >   making it even more free.
> >
> > - An overlay can already add a 'phandle' property to a node in the
> >   base tree.  Can you use that directly instead of adding a new
> >   mechanism?
> >
> 
> That feels like it might be a bit difficult to work with, for a couple
> of reasons that might be logically wrong, so forgive me if I'm
> thinking wrong:
> 
>  - A phandle is just a number, so it won't get adjusted as overlays
> get applied. All of the overlays that one of my boards needs to apply
> would need to choose sufficiently high phandles that they hopefully
> won't conflict with other overlays, especially as new nodes get
> introduced with their own phandles.

So, I think that could be handled by applying a local fixup to it in
the overlay which would offset it correctly.  I'd have to work through
the details, but I won't bother because..

>  - If more than one overlay needs to reference a specific node, we
> have other conflicts. These overlays aren't necessarily related, but
> they would need to agree with each other on the phandle of the node OR
> make sure they apply in a specific order so one can add the phandle
> and subsequent overlays can reference them symbolically only as
> intended.

..that's a good point.  Adding the phandle in the overlay effectively
*requires* that the phandle be missing in the base, which gets really
messy if you have a bunch of overlays to juggle.

You could maybe handle it by handling "base tree fixes" as a separate
overlay which must be applied first before the "real" overlays, but at
that point your solution is probably a better one.

> The problem is that these overlays aren't necessarily curated by a
> single authority, so conflicts with multiple overlays trying to
> reference a node is scary. Ideally, we'd like these things to be as
> usable as possible for average Joe Blow.

Yeah, fair enough.  I'll re-evaluate your patches with that in mind.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/2] overlays: auto allocate phandles for nodes in base fdt

[Kyle Evans]

[Resending with a proper mail client, because it didn't go to the lists]

On Tue, Jan 2, 2018 at 11:42 PM, David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> Regardless of anything else, these two patches need different one-line
> summaries.
>
> On Mon, Jan 01, 2018 at 12:59:44AM -0600, kevans-HZy0K5TPuP5AfugRpC6u6w@public.gmane.org wrote:
>> Currently, references cannot be made to nodes in the base that do not already
>> have phandles, limiting us to nodes that have been referenced in the base fdt.
>> Lift that restriction by allocating them on an as-needed basis.
>
> Hmm.  I'm a bit dubious about this.
>
> - My feeling is that one of the problems with the overlay format is
>   that it's already too free, allowing the overlay to change
>   essentially anything in the base tree.  So I'm not that keen on
>   making it even more free.
>
> - An overlay can already add a 'phandle' property to a node in the
>   base tree.  Can you use that directly instead of adding a new
>   mechanism?
>

That feels like it might be a bit difficult to work with, for a couple
of reasons that might be logically wrong, so forgive me if I'm
thinking wrong:

 - A phandle is just a number, so it won't get adjusted as overlays
get applied. All of the overlays that one of my boards needs to apply
would need to choose sufficiently high phandles that they hopefully
won't conflict with other overlays, especially as new nodes get
introduced with their own phandles.

 - If more than one overlay needs to reference a specific node, we
have other conflicts. These overlays aren't necessarily related, but
they would need to agree with each other on the phandle of the node OR
make sure they apply in a specific order so one can add the phandle
and subsequent overlays can reference them symbolically only as
intended.

The problem is that these overlays aren't necessarily curated by a
single authority, so conflicts with multiple overlays trying to
reference a node is scary. Ideally, we'd like these things to be as
usable as possible for average Joe Blow.

Re: [PATCH v2 1/2] overlays: auto allocate phandles for nodes in base fdt

[David Gibson]

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

Regardless of anything else, these two patches need different one-line
summaries.

On Mon, Jan 01, 2018 at 12:59:44AM -0600, kevans-HZy0K5TPuP5AfugRpC6u6w@public.gmane.org wrote:
> Currently, references cannot be made to nodes in the base that do not already
> have phandles, limiting us to nodes that have been referenced in the base fdt.
> Lift that restriction by allocating them on an as-needed basis.

Hmm.  I'm a bit dubious about this.

- My feeling is that one of the problems with the overlay format is
  that it's already too free, allowing the overlay to change
  essentially anything in the base tree.  So I'm not that keen on
  making it even more free.

- An overlay can already add a 'phandle' property to a node in the
  base tree.  Can you use that directly instead of adding a new
  mechanism?


> Signed-off-by: Kyle Evans <kevans-HZy0K5TPuP5AfugRpC6u6w@public.gmane.org>
> ---
> 
> Changes since v1:
>  - Added a function to grab the next phandle; once we've assigned one phandle
>  to a node automatically, we'll need to choose max phandle from the base
>  blob instead of the overlay since they've not necessarily been merged yet.
> 
>  libfdt/fdt_overlay.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 74 insertions(+), 9 deletions(-)
> 
> diff --git a/libfdt/fdt_overlay.c b/libfdt/fdt_overlay.c
> index bd81241..10a57ae 100644
> --- a/libfdt/fdt_overlay.c
> +++ b/libfdt/fdt_overlay.c
> @@ -335,6 +335,66 @@ static int overlay_update_local_references(void *fdto, uint32_t delta)
>  						    delta);
>  }
>  
> +/**
> + * overlay_next_phandle - Grab next phandle to be assigned
> + * @fdt: Base Device Tree blob
> + * @fdto: Device tree overlay blob
> + *
> + * overlay_next_phandle() determines the next phandle to be assigned to the
> + * fdt blob.
> + *
> + * This is part of the device tree overlay application process, when you need to
> + * assign a phandle to a node that doesn't currently have one.
> + *
> + * returns:
> + *      Next phandle to be assigned on success
> + *      Negative error code on failure
> + */
> +static int overlay_next_phandle(void *fdt, void *fdto)
> +{
> +	int base_max, overlay_max;
> +
> +	base_max = fdt_get_max_phandle(fdt);
> +	if (base_max < 0)
> +		return base_max;
> +	overlay_max = fdt_get_max_phandle(fdto);
> +	if (overlay_max < 0)
> +		return overlay_max;
> +	return (base_max > overlay_max ? base_max : overlay_max) + 1;
> +}

This is deceptively expensive, doing a full scan of both base and
overlay blobs whenever you want to get a new phandle.  I also don't
think it's quite necessary: I'm pretty sure the way we adjust the
overlay phandles they always have to be greater than those in the base
tree.  You could find the maximum phandle of the (adjusted) overlay
while evaluating overlay_adjust_local_phandles().

That would let you generate new phandles without extra full scans
through the blobs.

> +
> +/**
> + * overlay_assign_phandle - Assign a phandle to a symbol in the base fdt
> + * @fdt: Base Device Tree blob
> + * @fdto: Device tree overlay blob
> + * @symbol_off: Node offset of the symbol to be assigned a phandle

I don't like that name - this is is the offset of the node that the
symbol references, not the symbol itself (which is a property).

> + *
> + * overlay_assign_phandle() assigns the next phandle available to the requested
> + * node in the base device tree.
> + *
> + * This is part of the device tree overlay application process, when you want to
> + * reference a symbol in the base device tree that doesn't yet have a phandle.
> + *
> + * returns:
> + *      phandle assigned on success
> + *      0 on failure
> + */
> +static int overlay_assign_phandle(void *fdt, void *fdto, int symbol_off)
> +{
> +	int phandle, ret;
> +	fdt32_t phandle_val;
> +
> +	/* Overlay phandles have already been adjusted, grab highest phandle */
> +	phandle = overlay_next_phandle(fdt, fdto);
> +	if (phandle < 0)
> +		return 0;
> +	phandle_val = cpu_to_fdt32(phandle);
> +	ret = fdt_setprop(fdt, symbol_off, "phandle", &phandle_val, sizeof(phandle_val));
> +	if (!ret)
> +		return phandle;

You can use fdt_setprop_u32() to avoid the manual byteswap.

> +	return 0;
> +}
> +
>  /**
>   * overlay_fixup_one_phandle - Set an overlay phandle to the base one
>   * @fdt: Base Device Tree blob
> @@ -359,7 +419,7 @@ static int overlay_update_local_references(void *fdto, uint32_t delta)
>   *      Negative error code on failure
>   */
>  static int overlay_fixup_one_phandle(void *fdt, void *fdto,
> -				     int symbols_off,
> +				     int *symbols_off,
>  				     const char *path, uint32_t path_len,
>  				     const char *name, uint32_t name_len,
>  				     int poffset, const char *label)
> @@ -370,10 +430,10 @@ static int overlay_fixup_one_phandle(void *fdt, void *fdto,
>  	int symbol_off, fixup_off;
>  	int prop_len;
>  
> -	if (symbols_off < 0)
> -		return symbols_off;
> +	if (*symbols_off < 0)
> +		return *symbols_off;
>  
> -	symbol_path = fdt_getprop(fdt, symbols_off, label,
> +	symbol_path = fdt_getprop(fdt, *symbols_off, label,
>  				  &prop_len);
>  	if (!symbol_path)
>  		return prop_len;
> @@ -383,8 +443,14 @@ static int overlay_fixup_one_phandle(void *fdt, void *fdto,
>  		return symbol_off;
>  
>  	phandle = fdt_get_phandle(fdt, symbol_off);
> -	if (!phandle)
> -		return -FDT_ERR_NOTFOUND;
> +	if (!phandle) {
> +		phandle = overlay_assign_phandle(fdt, fdto, symbol_off);
> +		if (phandle == 0)
> +			return -FDT_ERR_NOTFOUND;
> +
> +		/* Re-lookup symbols offset, it's been invalidated */
> +		*symbols_off = fdt_path_offset(fdt, "/__symbols__");
> +	}
>  
>  	fixup_off = fdt_path_offset_namelen(fdto, path, path_len);
>  	if (fixup_off == -FDT_ERR_NOTFOUND)
> @@ -418,7 +484,7 @@ static int overlay_fixup_one_phandle(void *fdt, void *fdto,
>   *      0 on success
>   *      Negative error code on failure
>   */
> -static int overlay_fixup_phandle(void *fdt, void *fdto, int symbols_off,
> +static int overlay_fixup_phandle(void *fdt, void *fdto, int *symbols_off,
>  				 int property)
>  {
>  	const char *value;
> @@ -519,8 +585,7 @@ static int overlay_fixup_phandles(void *fdt, void *fdto)
>  
>  	fdt_for_each_property_offset(property, fdto, fixups_off) {
>  		int ret;
> -
> -		ret = overlay_fixup_phandle(fdt, fdto, symbols_off, property);
> +		ret = overlay_fixup_phandle(fdt, fdto, &symbols_off, property);
>  		if (ret)
>  			return ret;
>  	}

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 1/2] overlays: auto allocate phandles for nodes in base fdt

[kevans-HZy0K5TPuP5AfugRpC6u6w]

Currently, references cannot be made to nodes in the base that do not already
have phandles, limiting us to nodes that have been referenced in the base fdt.
Lift that restriction by allocating them on an as-needed basis.

Signed-off-by: Kyle Evans <kevans-HZy0K5TPuP5AfugRpC6u6w@public.gmane.org>
---

Changes since v1:
 - Added a function to grab the next phandle; once we've assigned one phandle
 to a node automatically, we'll need to choose max phandle from the base
 blob instead of the overlay since they've not necessarily been merged yet.

 libfdt/fdt_overlay.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 74 insertions(+), 9 deletions(-)

diff --git a/libfdt/fdt_overlay.c b/libfdt/fdt_overlay.c
index bd81241..10a57ae 100644
--- a/libfdt/fdt_overlay.c
+++ b/libfdt/fdt_overlay.c
@@ -335,6 +335,66 @@ static int overlay_update_local_references(void *fdto, uint32_t delta)
 						    delta);
 }
 
+/**
+ * overlay_next_phandle - Grab next phandle to be assigned
+ * @fdt: Base Device Tree blob
+ * @fdto: Device tree overlay blob
+ *
+ * overlay_next_phandle() determines the next phandle to be assigned to the
+ * fdt blob.
+ *
+ * This is part of the device tree overlay application process, when you need to
+ * assign a phandle to a node that doesn't currently have one.
+ *
+ * returns:
+ *      Next phandle to be assigned on success
+ *      Negative error code on failure
+ */
+static int overlay_next_phandle(void *fdt, void *fdto)
+{
+	int base_max, overlay_max;
+
+	base_max = fdt_get_max_phandle(fdt);
+	if (base_max < 0)
+		return base_max;
+	overlay_max = fdt_get_max_phandle(fdto);
+	if (overlay_max < 0)
+		return overlay_max;
+	return (base_max > overlay_max ? base_max : overlay_max) + 1;
+}
+
+/**
+ * overlay_assign_phandle - Assign a phandle to a symbol in the base fdt
+ * @fdt: Base Device Tree blob
+ * @fdto: Device tree overlay blob
+ * @symbol_off: Node offset of the symbol to be assigned a phandle
+ *
+ * overlay_assign_phandle() assigns the next phandle available to the requested
+ * node in the base device tree.
+ *
+ * This is part of the device tree overlay application process, when you want to
+ * reference a symbol in the base device tree that doesn't yet have a phandle.
+ *
+ * returns:
+ *      phandle assigned on success
+ *      0 on failure
+ */
+static int overlay_assign_phandle(void *fdt, void *fdto, int symbol_off)
+{
+	int phandle, ret;
+	fdt32_t phandle_val;
+
+	/* Overlay phandles have already been adjusted, grab highest phandle */
+	phandle = overlay_next_phandle(fdt, fdto);
+	if (phandle < 0)
+		return 0;
+	phandle_val = cpu_to_fdt32(phandle);
+	ret = fdt_setprop(fdt, symbol_off, "phandle", &phandle_val, sizeof(phandle_val));
+	if (!ret)
+		return phandle;
+	return 0;
+}
+
 /**
  * overlay_fixup_one_phandle - Set an overlay phandle to the base one
  * @fdt: Base Device Tree blob
@@ -359,7 +419,7 @@ static int overlay_update_local_references(void *fdto, uint32_t delta)
  *      Negative error code on failure
  */
 static int overlay_fixup_one_phandle(void *fdt, void *fdto,
-				     int symbols_off,
+				     int *symbols_off,
 				     const char *path, uint32_t path_len,
 				     const char *name, uint32_t name_len,
 				     int poffset, const char *label)
@@ -370,10 +430,10 @@ static int overlay_fixup_one_phandle(void *fdt, void *fdto,
 	int symbol_off, fixup_off;
 	int prop_len;
 
-	if (symbols_off < 0)
-		return symbols_off;
+	if (*symbols_off < 0)
+		return *symbols_off;
 
-	symbol_path = fdt_getprop(fdt, symbols_off, label,
+	symbol_path = fdt_getprop(fdt, *symbols_off, label,
 				  &prop_len);
 	if (!symbol_path)
 		return prop_len;
@@ -383,8 +443,14 @@ static int overlay_fixup_one_phandle(void *fdt, void *fdto,
 		return symbol_off;
 
 	phandle = fdt_get_phandle(fdt, symbol_off);
-	if (!phandle)
-		return -FDT_ERR_NOTFOUND;
+	if (!phandle) {
+		phandle = overlay_assign_phandle(fdt, fdto, symbol_off);
+		if (phandle == 0)
+			return -FDT_ERR_NOTFOUND;
+
+		/* Re-lookup symbols offset, it's been invalidated */
+		*symbols_off = fdt_path_offset(fdt, "/__symbols__");
+	}
 
 	fixup_off = fdt_path_offset_namelen(fdto, path, path_len);
 	if (fixup_off == -FDT_ERR_NOTFOUND)
@@ -418,7 +484,7 @@ static int overlay_fixup_one_phandle(void *fdt, void *fdto,
  *      0 on success
  *      Negative error code on failure
  */
-static int overlay_fixup_phandle(void *fdt, void *fdto, int symbols_off,
+static int overlay_fixup_phandle(void *fdt, void *fdto, int *symbols_off,
 				 int property)
 {
 	const char *value;
@@ -519,8 +585,7 @@ static int overlay_fixup_phandles(void *fdt, void *fdto)
 
 	fdt_for_each_property_offset(property, fdto, fixups_off) {
 		int ret;
-
-		ret = overlay_fixup_phandle(fdt, fdto, symbols_off, property);
+		ret = overlay_fixup_phandle(fdt, fdto, &symbols_off, property);
 		if (ret)
 			return ret;
 	}
-- 
2.15.1