Open pelwell opened 1 year ago
Hmm - am I right in thinking that an overlay:
You might want to check #98 which (I think) addresses the problem you describe in a more robust way.
Hmm - am I right in thinking that an overlay:
1. Is allowed to add a label to an existing node that doesn't already have one?
Yes, that's allowed.
2. Is not really allowed to add a second label to a node that does have one?
Hm, I think that should be allowed. In general nodes are allowed to have multiple labels.
3. Has no way to restrict the labels that are copied to the base dtb (ignoring those that reference something that never appears in the base dtb)?
I'm not really sure what you mean by this. If the label is not copied to the base dtb it has no effect.
Note that it's not the act of adding the label that causes the extra phandle to be assigned. The extra phandle is assigned when the node is referenced by a label that dtc thinks is local at compile time, but turns out not to be at overlay apply time.
4. As a result, my patch is not acceptable?
No, but I'm afraid it's not acceptable because it's very fragile. Although existing nodes being the top-level of a single overlay fragment is the most common case, it's entirely possible for overlay fragments to make tweaks to an entire tree of existing nodes, so checking the depth doesn't really help us.
So, there's a real issue, but how to deal with it is pretty tricky. This patch doesn't work because it relies on an inaccurate heuristic to work out whether we can apply the phandle change. #98 sent by @ukleinek doesn't work because it relies on dynamic allocation which is not allowed in libfdt.
Option 1
In fdt_apply_overlay()
simply prohibit rewriting phandles: throw an error if attempting to update an existing phandle
property in the base tree. This would effectively make any overlay which refers to a label defined in that overlay illegal if applied to a tree which already has a phandle on that node. Roughly speaking if you want to refer to a node in a overlay you must use it's original base tree label, not a new label. We can't tell that at overlay compile time, so it's pretty awful.
Option 1a
Ban overlays which rewrite phandles as in Option 1, also change the order of things so we merge the symbol tables before merging the trees themselves. On the dtc side, completely deprecate use of __local_fixups__
. Where we would have emitted a local fixup, instead use a regular fixup based on the label added in the overlay.
Option 2
Resolve the new and existing phandles at overlay apply time. I think it's possible, but it's pretty darn complex.
phandle
propertiesI think that will do it. It's at least O(n^2)-ish, but that might already be true of overlay application. I'm a bit worried I've missed something that will make it O(n^3) or worse.
Maybe there's a clever way to make that more elegant, but I haven't spotted it.
A Raspberry Pi user reported (https://github.com/raspberrypi/firmware/issues/1718) that some overlays that work with the Pi tools can't be applied by fdtoverlay (even ignoring the non-standard parameter stuff). After some digging I traced the problem to unwanted phandles being copied into the base dtb, overwriting an existing phandle and breaking references to that node.
This patch filters out phandles in the outer level of overlay nodes (i.e. not in subnodes).