Re: Bug or feature? /somenode overwrites /somenode@17 in fdt-overlay

[David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>]

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

On Tue, Apr 25, 2023 at 04:55:04PM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> TL;DR: The node &{/somenode} in my overlay overwrites &{/somenode@27}.
> Is this expected?

Huh, this is a really interesting case.  So I think the overall answer
is "no" that's not expected.  However, what exactly we should do
instead is not entirely obvious

This happens as a consequence of the fact that that using a path
reference here is interpreted just like a path reference anywhere -
and I don't think we want to change that.

DT paths - for convenience, and to match long standing OF conventions
- allow you to omit the unit addresses, and will resolve to something
matching the base name.

The case of overlays is particularly surprising, but it occurs to me
that this could also be confusing for other uses of path references:

/ {
    somenode@abcd1234fedc9876 {
        . . .
    };
    othernode@100 {
        something = <&{/somenode}>;
    };
}

Could be desirable for brevity.  However it becomes ambiguous and
potentially confusing if another somenode@<different address> is added
later.

So, I think when resolving path references, we should stop just
accepting the first match we find, but instead search the whole tree
and trip an error if it's ambiguous.  That should avoid the confusing
situation you encountered

> I have the following base and overlay dts files:
> 
> 	uwe@taurus:~/tmp/fdtoverlay$ cat base.dts
> 	/dts-v1/;
> 
> 	/ {
> 		somenode@12 {
> 			property = <0x2a>;
> 		};
> 
> 		somenode@27 {
> 			property = <0x20>;
> 		};
> 	};
> 	uwe@taurus:~/tmp/fdtoverlay$ cat overlay.dts
> 	/dts-v1/;
> 	/plugin/;
> 
> 	&{/somenode} {
> 		property = <0x17>;
> 	};
> 
> When compiling these and using them with fdtoverlay I see that
> /somenode@12 is overwritten(. And if I swap the order of /somenode@12
> and /somenode@27 in base.dts, the latter is overwritten instead):

Of course in the case of runtime overlays we can't perform this check
at compile time.  We can, and probably should, check for ambiguity in
path reference during overlay application.  However, that doesn't
entirely address the problem - because we don't know everything about
the base tree, doing this is kind of bad practice in an overlay.

Unfortunately, we can't outright ban (or deprecate) fragment targets
with no unit address, because we might need to update a node which has
no unit address.

We could change the semantics of runtime overlays so that paths are
interpreted strictly - no missing unit addresses allowed.  I'm pretty
disinclined to do that though: it means paths there would be
interpreted differently from how they are everywhere else.  It could,
at least in theory, break existing overlays that work in situations
where the target is not ambiguous.  This might even make some sensible
seeming overlays impossible: imagine an overlay designed to patch a
property on the ethernet node for a closely related family of SoCs.
We only expect there to be one ethernet on the SoC, but maybe
different minor revisions changed the NIC's address.  With the current
behaviour a single overlay can handle both revisions, with the changed
behaviour it couldn't.

Another thing to bear in mind is that if you're using path-target
overlays you're already kind of in fragile hack territory.  A base
tree that's designed for overlay patching should be exporting symbols
with a clear convention on what labels a compatible overlay can
expect.

> uwe@taurus:~/tmp/fdtoverlay$ dtc -I dts -O dtb -o base.dtb base.dts && dtc -I dts -O dtb -o overlay.dtb overlay.dts && fdtoverlay -i base.dtb -o base+overlay.dtb overlay.dtb && fdtdump base+overlay.dtb
> base.dts:4.14-6.4: Warning (unit_address_vs_reg): /somenode@12: node has a unit name, but no reg or ranges property
> base.dts:8.14-10.4: Warning (unit_address_vs_reg): /somenode@27: node has a unit name, but no reg or ranges property
> 
> **** fdtdump is a low-level debugging tool, not meant for general use.
> **** If you want to decompile a dtb, you probably want
> ****     dtc -I dtb -O dts <filename>
> 
> /dts-v1/;
> // magic:		0xd00dfeed
> // totalsize:		0x99 (153)
> // off_dt_struct:	0x38
> // off_dt_strings:	0x90
> // off_mem_rsvmap:	0x28
> // version:		17
> // last_comp_version:	16
> // boot_cpuid_phys:	0x0
> // size_dt_strings:	0x9
> // size_dt_struct:	0x58
> 
> / {
>     somenode@12 {
>         property = <0x00000017>;
>     };
>     somenode@27 {
>         property = <0x00000020>;
>     };
> };
> 
> I was surprised and wonder if this is as designed or if this is a bug
> that should be fixed. Any insights?

As above, it's not really either.  This is a consequence of applying
established rules in a new situation where the results are a bit
counterintuitive.

I think the best we can do is to report error on ambiguous path
resolution, both within dtc itself and in the overlay application code
of libfdt.

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

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