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:
signature.asc
Description: PGP signature
