Improve dtdiff by avoiding or nulling the phandles

[jcormier]

The dtdiff tool is super useful when trying to compare two different device trees. However, it generally generates a ton of noise flagging all the phandles as changed between the two devices. This makes it hard to see the real changes.

How hard would it be to add a flag to the dtc compiler, maybe a phandle format that reports all phandles as 0x0?

[dragonsploder]

I agree, this would be very useful to have.

[dragonsploder]

Here is a temporary hack to print NULL for all phandles:

diff --git a/treesource.c b/treesource.c
index ae15839..8fb3bf4 100644
--- a/treesource.c
+++ b/treesource.c
@@ -204,6 +204,11 @@ static void write_propval(FILE *f, struct property *prop)
        enum markertype emit_type = TYPE_NONE;
        char *srcstr;
 
+       if (strcmp(prop->name, "phandle") == 0) {
+               fprintf(f, " = NULL;\n");
+               return;
+       }
+
        if (len == 0) {
                fprintf(f, ";");
                if (annotate) {

[dgibson]

If you just want to skip the actual phandle (and linux,phandle) properties, we could add an option for that and use it in dtdiff - we already have the sort (-s) option largely for the benefit of dtdiff. I won't have capacity to do this myself, but I'd by happy enough to review and merge patches for it.

If you wanted to skip phandles included in other properties, that would be much, much harder.

[jcormier]

If you just want to skip the actual phandle (and linux,phandle) properties

Would you recommend handling that in the input side of things or the output? The above hack is doing it on the output. Just looking for a starting place to look.

If you wanted to skip phandles included in other properties, that would be much, much harder.

Yeah I mean that would be good as well but agreed that it would be more involved. Is it reasonable to try and handle both scenarios by replacing the handle hex value with the node name its pointing too?

Also, do you have any suggestions for what you would like the new argument to be? I mentioned the phandle format option but looking at the code I'm unclear what that's doing, or if its even relevant. Would you prefer to add a generic --diff-format option that could be improved/expanded later. Or a more specific option (--skip-phandles) that describes what this change is doing.

[dgibson]

If you just want to skip the actual phandle (and linux,phandle) properties

Would you recommend handling that in the input side of things or the output? The above hack is doing it on the output. Just looking for a starting place to look.

Output definitely. That way all the internal processing of phandles remains unchanged.

If you wanted to skip phandles included in other properties, that would be much, much harder.

Yeah I mean that would be good as well but agreed that it would be more involved. Is it reasonable to try and handle both scenarios by replacing the handle hex value with the node name its pointing too?

Actually, that's a good point. If you're coming from dts input, the type markers we already have mean that references to phandles should already be shown as &label or whatever - at least if that's how they were given in the input. If you have dtb input, I don't think it's feasible to try to handle this case: you don't know where the phandle references are without detailed knowledge of the bindings which is out of scope for dtc.

Also, do you have any suggestions for what you would like the new argument to be? I mentioned the phandle format option but looking at the code I'm unclear what that's doing, or if its even relevant. Would you prefer to add a generic --diff-format option that could be improved/expanded later. Or a more specific option (--skip-phandles) that describes what this change is doing.

I'd suggest something specific, much like --sort. --omit-phandles would be the obvious choice to me.

[jcormier]

If you have dtb input, I don't think it's feasible to try to handle this case: you don't know where the phandle references are without detailed knowledge of the bindings which is out of scope for dtc.

Ahh I didn't know about that. I just read through https://elinux.org/Device_Tree_Mysteries#Phandle which explains it quite well.

Thus the value of the property "interrupt-parent" in node uart@200 is merely a u32 value. It is not a pointer to the location of node /soc/pic@100 in the device tree blob. When loaded by the kernel, it is also not a pointer in the expanded device tree. It is merely a value in a property. The only way that kernel code knows that the value in the "interrupt-parent" property is a phandle is because the binding documentation has defined that the value is a phandle. Thus the code that reads the "interrupt-parent" property has to be written to treat that value as a phandle. When the code needs to find what node the phandle points to, it calls of_find_node_by_phandle(), which searches for a node containing a "phandle" property containing the specified value.

[jcormier]

A simpler change would be something like the following, though it will add a dependency on sed or grep.

It brings up the question of do you want dtdiff to always drop phandles or is it okay to add argument handling to this script?

--- a/dtdiff
+++ b/dtdiff
@@ -28,7 +28,7 @@ source_and_sort () {
        exit 2
     fi
 
-    $DTC -I $IFORMAT -O dts -qq -f -s -o - "$DT"
+    $DTC -I $IFORMAT -O dts -qq -f -s -o - "$DT" | grep -v 'phandle ='
 }
 
 if [ $# != 2 ]; then

[dgibson]

A simpler change would be something like the following, though it will add a dependency on sed or grep.

Simpler, but not necessarily reliable. It will incorrectly omit lines that happen to have "phandle =" in them, even if it's a string in the property value.

[jcormier]

Sure, I don't think "phandle =" is likely to show up in a property value but if you'd prefer the more thorough implementation, I'll look into it.

[dgibson]

Sure, I don't think "phandle =" is likely to show up in a property value but if you'd prefer the more thorough implementation, I'll look into it.

It's certainly not likely, but I'd rather not a hard to debug edge case as a landmine for the future.

[jcormier]

Makes sense.

Separately I was thinking about trying to track phandles and thought up a possible method.

• After decompile, go through each file output and create a phandle -> node path map.
• Compare diff of the two files and only replace the value if the old and new value point to the same node path
• Run unified diff on filtered files

I will write something up in python to give this a try.

[jcormier]

Here is a first pass at implementing a dtdiff in python

https://gist.github.com/jcormier/4af3e1a33cfc4d20690350cfa9e134a1

[dgibson]

Here is a first pass at implementing a dtdiff in python

https://gist.github.com/jcormier/4af3e1a33cfc4d20690350cfa9e134a1

I don't think this is a wise approach. The basic idea of attempting to recognize phandle changes by the values is flawed, I think. It'll get it right most of the time, but it will be very confusing when it interprets some unrelated change as a phandle update.

More superficially this is using regexes to re-parse parts of the tree from source, which seems a very roundabout way of doing things when dtc can already parse and knows some of the mappings you're trying to determine internally. I'm not very convinced about the robustness of a bunch of those regexes either (phandles need not have exactly two hex digits, for one example).

In short, this seems like a lot of trouble and complexity to go to, to get a result that won't even be reliable.

[a3f]

FYI, #100 by @ukleinek was meant to be a first step towards improving dtdiff. By keeping around the information about labels, it's possible to decompile them and have dtdiff eventually be aware of them.

This doesn't help you however if the only thing you have is the compiled device trees that lack the fixup information.

[jcormier]

FYI, #100 by @ukleinek was meant to be a first step towards improving dtdiff. By keeping around the information about labels, it's possible to decompile them and have dtdiff eventually be aware of them.

Are the fixups the same thing that gets enabled when you want overlays to work?