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

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

[Uwe Kleine-König]

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

Hello,

TL;DR: The node &{/somenode} in my overlay overwrites &{/somenode@27}.
Is this expected?

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):

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?

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

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

[Rob Herring]

On Tue, Apr 25, 2023 at 9:55 AM Uwe Kleine-König
<u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
>
> Hello,
>
> TL;DR: The node &{/somenode} in my overlay overwrites &{/somenode@27}.
> Is this expected?

Sounds like a bug to me. Probably due to our mixed of handling node
names with and without the unit-address included.

Rob

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

[David Gibson]

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

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

[Uwe Kleine-König]

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

Helle David,

On Fri, Apr 28, 2023 at 05:01:13PM +1000, David Gibson wrote:
> On Tue, Apr 25, 2023 at 04:55:04PM +0200, Uwe Kleine-König wrote:
> > 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

IMHO there is a need for a set of different lookup functions.

Something like fdt_subnode_offset_strict() that does a lookup without
unit address interpolation. And maybe rename fdt_path_offset to
fdt_path_offset_lax() (but keep the name without suffix for API/ABI
compatibility). And the same for all functions that have a similar
issue.

Then all internal usages could be reviewed and switched to the right
variant.

Changing the lax variants to error out if there is no unambiguous match
also sounds like a good idea.

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

Agreed. However it's unclear to me what information is missing. Of
course you don't know the base tree at the time the overlay is compiled,
but at application time the available information should be complete,
shouldn't it?

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

I agree that changing existing code paths might be fragile.

Still adding the strict flavour variants of the affected functions is a
good idea. Then future usages can consciously choose which semantic is
intended.

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

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

[Uwe Kleine-König]

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

Hello David,

On Fri, Apr 28, 2023 at 08:04:49PM +0200, Uwe Kleine-König wrote:
> Still adding the strict flavour variants of the affected functions is a
> good idea. Then future usages can consciously choose which semantic is
> intended.

Another surprising instance is fdt_get_alias() which might behave in
strange ways if there is a node at /aliases@12345.

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

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

[David Gibson]

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

On Fri, Apr 28, 2023 at 08:40:07PM +0200, Uwe Kleine-König wrote:
> Hello David,
> 
> On Fri, Apr 28, 2023 at 08:04:49PM +0200, Uwe Kleine-König wrote:
> > Still adding the strict flavour variants of the affected functions is a
> > good idea. Then future usages can consciously choose which semantic is
> > intended.
> 
> Another surprising instance is fdt_get_alias() which might behave in
> strange ways if there is a node at /aliases@12345.

Technically true, but having /aliases@anything is something that
should never happen anyway.  We should probably add a check for that,
if we don't have one yet.

-- 
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: Bug or feature? /somenode overwrites /somenode@17 in fdt-overlay

[David Gibson]

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

On Fri, Apr 28, 2023 at 08:04:41PM +0200, Uwe Kleine-König wrote:
> Helle David,
> 
> On Fri, Apr 28, 2023 at 05:01:13PM +1000, David Gibson wrote:
> > On Tue, Apr 25, 2023 at 04:55:04PM +0200, Uwe Kleine-König wrote:
> > > 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
> 
> IMHO there is a need for a set of different lookup functions.

Hmm.. I'm not yet convinced.

> Something like fdt_subnode_offset_strict() that does a lookup without
> unit address interpolation. And maybe rename fdt_path_offset to
> fdt_path_offset_lax() (but keep the name without suffix for API/ABI
> compatibility). And the same for all functions that have a similar
> issue.
> 
> Then all internal usages could be reviewed and switched to the right
> variant.

Hrm.  I think having two ways of looking up paths adds potential
confusion, so we need a very strong case if we're going to do that.

> Changing the lax variants to error out if there is no unambiguous match
> also sounds like a good idea.

This however, we should do.

> > 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.
> 
> Agreed. However it's unclear to me what information is missing. Of
> course you don't know the base tree at the time the overlay is compiled,
> but at application time the available information should be complete,
> shouldn't it?

Right, at overlay application time we have all the information.
However at overlay application time our options for reporting and
handling errors are typically rather more constrained than at compile
time.

> > 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.
> 
> I agree that changing existing code paths might be fragile.
> 
> Still adding the strict flavour variants of the affected functions is a
> good idea. Then future usages can consciously choose which semantic is
> intended.

Yeah... see that's a decision I'd prefer avoiding making people make
unless we really can't avoid it.

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

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