Presence of earlier node affects delete-property in later node

[cxw42]

This is a weird one --- please let me know if I am missing something obvious! I did check the existing issues and Google search results without success.

Given two device trees that differ only in the presence of an empty / node:

diff -y 1sec.dts 2sec.dts
/dts-v1/;                                                       /dts-v1/;

                                                              > /  {
                                                              > };
                                                              >
/ {                                                             / {
        nonexistent;                                                    nonexistent;
        /delete-property/ nonexistent;                                  /delete-property/ nonexistent;
        existent;                                                       existent;
};                                                              };

Observed: dtc obeys the delete-property only when that empty node is present:

diff -y 1sec.log 2sec.log
1sec.dts                                                      | 2sec.dts
/dts-v1/;                                                       /dts-v1/;

/ {                                                             / {
        nonexistent;                                          <
        existent;                                                       existent;
};                                                              };

(processed with dtc -I dts -O dts foo.dts)

Expected: the nonexistent property is removed by /delete-property/ in both cases (regardless of whether this node exists earlier in the file or not).

Repro: download the attached issue37.tar.gz, and run DTC=<path to dtc> make -B.

Tested with master (1.6.0-g81e0919a) on Ubuntu 18.04 x64.

Thanks for considering this report!

[dgibson]

Ah, right. This happens because of the way we assemble the final tree. For each top-level section in the dts (/ { ... }, or &whatever { ... }) we create a temporary internal tree, in which deletes are encoded as a specially marked node. Then we merge those temporary trees together, and it's at that point we process the delete markers into actually deleting the nodes.

When there's only a single section, we never do a merge, and so the delete is not processed.

The obvious way to fix that would be to always start with a dummy, empty tree, and merge even the first base or master section into that. That would cause some other subtle behaviour changes. e.g. at the moment this:

/ {
         node {
                 this-property;
        };
        node {
                 that-property;
        };
};

Will create a tree with two '/node' nodes, which will then trip an error in the "checks" section of the code. With the change suggested here, it would create a single /node with two properties because of the merge.

I don't particularly like the merge semantics within a single section - generally the order of stuff in the .dts isn't supposed to matter within a single tree fragment, but if processing merges, it does.

But then, that's already true for sections after the first, so consistency might be more important here.

We could restore the original semantics, at least somewhat, if we did a separate checks pass on each fragment before merging them together. That's something I've had in mind for other reasons as well, but haven't had time to investigate.

[cxw42]

Thanks for the analysis! That makes sense to me.

I am a relative newcomer to device trees, and my impression has been that dts files were processed top to bottom, and later entries won. If order matters between fragments, I personally don't mind if it matters within a fragment. I think there is merit to consistency in this case.

The specification, both v0.3 (latest release) and master, just says "Previously defined nodes may be deleted" --- it doesn't clarify what "previously-defined" means. Would it be worth opening an issue in the spec to get other users' thoughts about this? I am happy to do so if you think it would be worthwhile.

[dgibson]

Thanks for the analysis! That makes sense to me.

I am a relative newcomer to device trees, and my impression has been that dts files were processed top to bottom, and later entries won. If order matters between fragments, I personally don't mind if it matters within a fragment. I think there is merit to consistency in this case.

Yes, I tend to agree. I was a little hesitant, because it alters my mental model of how these things work. But really that model was formed before overlays were a common thing, so I think it needs to be altered.

So, I agree the right thing is to do the property deleting / overwriting even in the "base tree" fragment. The easiest way to implement that is to create a dummy empty base tree, then merge all the fragments into that. I don't know when I'll have time to look at this though, so if someone wants to send a patch, that would certainly expedite things.

The specification, both v0.3 (latest release) and master, just says "Previously defined nodes may be deleted" --- it doesn't clarify what "previously-defined" means. Would it be worth opening an issue in the spec to get other users' thoughts about this? I am happy to do so if you think it would be worthwhile.

Maybe, yes.