overlay: ensure that existing phandles are not overwritten

[ukleinek]

A phandle in an overlay is not supposed to overwrite a phandle that already exists in the base dtb as this breaks references to the respective node in the base.

Traditionally all phandle values in the overlay were increased by a fixed offset such that the base and overlay handle values don't overlap. With this change the overlay's phandles are determined dynamically. Already existing phandles are kept and an array "phandlemap" is used to track how the overlay's phandles change.

overlay_adjust_local_phandles() which creates the mapping between old and new phandles in the overlay needs to check the nodes in the base. To be able to find the base's node, overlay_fixup_phandles() must be run first, so the order of functions fdt_overlay_apply() had to change.

A test is added that checks that newly added phandles and existing phandles work as expected.

[ukleinek]

Bah, the compiler on arch and fedora is stupid. I don't have a nicer idea than to initialize phandle.

[pelwell]

Yes, a scheme that preserves existing phandles rather than inventing new ones is ideal, allowing for both functionalities (intentionally adding a new label to a base dtb, and applying an overlay which just happens to have a label that will clash) but it does mean that the application step becomes more involved.

Bah, the compiler on arch and fedora is stupid. I don't have a nicer idea than to initialize phandle.

I don't understand what has provoked this comment - the automatic checks are happy.

[ukleinek]

Yes, a scheme that preserves existing phandles rather than inventing new ones is ideal, allowing for both functionalities (intentionally adding a new label to a base dtb, and applying an overlay which just happens to have a label that will clash) but it does mean that the application step becomes more involved.

If code has a bug because it makes wrong assumptions, it's quite usual that there is an extra effort to make it right.

Bah, the compiler on arch and fedora is stupid. I don't have a nicer idea than to initialize phandle.

I don't understand what has provoked this comment - the automatic checks are happy.

They are, but only after https://github.com/dgibson/dtc/compare/e8db66a5580ba91f1552406ec1fc73ad25108006..42b2aab5b6f182541ee1524395d642a3c829c6d2 .

[pelwell]

it's quite usual that there is an extra effort to make it right.

Of course, sometimes added complexity is a necessary evil

They are, but only after...

Wouldn't initialising phandle to 0 be neater, if less complainy?

[dgibson]

Sorry. One of the design constraints for libfdt is that it needs to work in constrained environments that don't have an allocator, which this patch breaks. That's exactly why we're using the kind of clunky means of adding a fixed offset.

[ukleinek]

Sorry. One of the design constraints for libfdt is that it needs to work in constrained environments that don't have an allocator, which this patch breaks. That's exactly why we're using the kind of clunky means of adding a fixed offset.

The bug however is real. If allocating memory isn't an option, then the only way to fix it would be to merge the functionality of overlay_adjust_local_phandles() and overlay_update_local_references() into a single function and iterate over __local_fixups__ once for each phandle value. Would you be open to such a change even though it increases runtime complexity? Or do you have a better idea?

[dgibson]

Sorry. One of the design constraints for libfdt is that it needs to work in constrained environments that don't have an allocator, which this patch breaks. That's exactly why we're using the kind of clunky means of adding a fixed offset.

The bug however is real. If allocating memory isn't an option, then the only way to fix it would be to merge the functionality of overlay_adjust_local_phandles() and overlay_update_local_references() into a single function and iterate over __local_fixups__ once for each phandle value. Would you be open to such a change even though it increases runtime complexity? Or do you have a better idea?

Yes, I'm open to such a change, even though it's certainly more complex than I'd ideally like. I had some more thoughts about the bug in #97 (here probably would have made more sense in hindsight).

[ukleinek]

Sorry. One of the design constraints for libfdt is that it needs to work in constrained environments that don't have an allocator, which this patch breaks. That's exactly why we're using the kind of clunky means of adding a fixed offset.

The bug however is real. If allocating memory isn't an option, then the only way to fix it would be to merge the functionality of overlay_adjust_local_phandles() and overlay_update_local_references() into a single function and iterate over __local_fixups__ once for each phandle value. Would you be open to such a change even though it increases runtime complexity? Or do you have a better idea?

I thought a bit about this, and the idea is buggy. We first have to implement the fixed shift in the overlay and then in an additional pass check for conflicts and fixup. Otherwise it might happen that I update all refs with value 3 in the overlay to 5 to match the base dtb and then modify these again when handling the overlay refs with value 5.