Re: ftdoverlay overwrites phandle

["Uwe Kleine-König" <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>]

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

Hello Rob,

On Tue, Jul 12, 2022 at 09:53:57AM -0600, Rob Herring wrote:
> On Wed, Jun 29, 2022 at 3:20 PM Uwe Kleine-König <ukleinek-8fiUuRrzOP0dnm+yROfE0A@public.gmane.org> wrote:
> >
> > Hello,
> >
> > I want to apply an overlay to a device tree that is compiled without -@
> > because that's how dtbs are shipped by the Debian kernel package.
> 
> Does applying this overlay work as expected if -@ is used? I would
> imagine so as there is an assumption the base dtb was compiled with
> -@.

No it doesn't. I created a minimal (oh well, at least small) reproducer:

	uwe@taurus:~/gsrc/oftreemerge$ cat base.dts
	/dts-v1/;

	/ {
		#address-cells = <1>;
		#size-cells = <1>;

		node_a: a {
			prop = "blub";
		};

		node_b: b {
			a = <&node_a>;
		};
	};

	uwe@taurus:~/gsrc/oftreemerge$ cat overlay.dts
	/dts-v1/;
	/plugin/;

	/ {
		fragment@0 {
			target-path = "/";

			__overlay__ {
				node_a: a {
				};

				c {
					a = <&node_a>;
				};
			};
		};
	};

When compiling without -@ (as reported initially) I get:

	uwe@taurus:~/gsrc/oftreemerge$ dtc -I dts -O dtb -o base.dtb base.dts 
	uwe@taurus:~/gsrc/oftreemerge$ dtc -I dts -O dtb -o overlay.dtbo overlay.dts 
	uwe@taurus:~/gsrc/oftreemerge$ fdtoverlay -i base.dtb -o patched.dtb overlay.dtbo 
	uwe@taurus:~/gsrc/oftreemerge$ fdtdump patched.dtb 
	...
	/ {
	    #address-cells = <0x00000001>;
	    #size-cells = <0x00000001>;
	    c {
		a = <0x00000002>;
	    };
	    a {
		prop = "blub";
		phandle = <0x00000002>;
	    };
	    b {
		a = <0x00000001>;
	    };
	};

So b.a gets broken as the phandle for a got updated.

When doing the same with -@ the result is identical (apart from the
expected changes: more and different phandles, __symbols__ node etc):

	uwe@taurus:~/gsrc/oftreemerge$ dtc -@ -I dts -O dtb -o base.dtb base.dts
	uwe@taurus:~/gsrc/oftreemerge$ dtc -@ -I dts -O dtb -o overlay.dtbo overlay.dts
	uwe@taurus:~/gsrc/oftreemerge$ fdtoverlay -i base.dtb -o patched.dtb overlay.dtbo
	uwe@taurus:~/gsrc/oftreemerge$ fdtdump patched.dtb
	...
	/ {
	    #address-cells = <0x00000001>;
	    #size-cells = <0x00000001>;
	    c {
		a = <0x00000003>;
	    };
	    a {
		prop = "blub";
		phandle = <0x00000003>;
	    };
	    b {
		a = <0x00000001>;
		phandle = <0x00000002>;
	    };
	    __symbols__ {
		node_a = "/a";
		node_b = "/b";
	    };
	};

b.a still points to a non-existing node.

> The fix is to fix Debian dtbs

One of the blockers for that is that adding -@ to the kernel default
rules was rejected more than once.
(https://lore.kernel.org/linux-arm-kernel/CAK7LNAS5t1wew0MMFjdB5HGCAMerhU7pAGiFhcTtCRUAAjGLpw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org/)

> (or don't use them as dtbs should come from firmware rather than
> distros).

In this case the distro provides the firmware, too. So well, you could
argue the dtbs should (in this case) be provided by the raspi-firmware
package and not the kernel image package, but that only makes things
harder to handle, because the kernel is effectively the upstream for the
device tree files.

> There might be sufficient information to make your case work because
> IIRC all the (resolved) overlay phandle values get adjusted when
> applied as they could collide with the base tree phandle values.
> However, doing so would mean users may start working around base DTs
> compiled without -@. That would remove labels being an ABI which I
> don't love, but it would also remove the abstraction that labels
> provide.

My point of view is a bit different. It would allow to practically apply
overlays to device trees without -@. It doesn't limit in any way the
semantic of labels in the case where -@ is used. I can accept we're
differing in that point and I don't think working on reaching an
agreement here is time spend well.

> Certainly, the base phandle value shouldn't just be silently overwritten.

We agree here, which is good enough for me.

Fixing that is a bit complicated, because currently fdtoverlay just
applies a fixed offset to the phandle values defined in the overlay. I
think it would need another pass over the device tree to determine
already existing phandles and maintaining a mapping for these.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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