[PATCH] Revert "fdtdec: drop needlessly convoluted CONFIG_PHANDLE_CHECK_SEQ"

[Rasmus Villemoes]

On 31/07/2022 15.28, Tom Rini wrote:
> On Sat, Jul 30, 2022 at 07:27:26PM -0600, Simon Glass wrote:
>> Hi Tom,
>>

>> Shall I pick it up for the upcoming release?
> 
> I don't think we should pick up the revert as I don't think there's
> agreement that reverting this is the right step forward among all of the
> interested parties.
> 
> One thing I want to know at this point is, was the problematic device
> tree accepted upstream, or did they also say that 2 nodes with the same
> name but different paths is wrong, don't do that?

There's no problematic device tree, having two nodes with the same
(base)name is perfectly fine and in fact sometimes expected/required (in
the cases where a node name is standardized; e.g. having two identical
eeproms on different i2c buses both would/should be called eeprom at 50).

What Simon is concerned about is that the translation from full path to
blob offset is now done unconditionally, and that might be costly. I'm
not sure it is (and didn't know that it could be), but as I've said
repeatedly, I prefer code that works out-of-the-box, and for the boards
that do need this extra check (because just looking at the basename is
not enough), the fact that the previous code worked seemed to be pure
luck - because those dtbs were compiled with -@ due to some other
CONFIG_ knob being set. And again, involving phandles at all is what
make the code fragile, so a revert that reinstates a CONFIG_ knob with
PHANDLE in the name is not a good way forward, assuming there is even
anything to fix.

So if the performance thing is real, sure, we can introduce a
CONFIG_something, but only if at the same time we have a sanity check at
build-time that detects if one should enable/disable that option
(depending on whether we make it "positive" or "negative"). Something
like this seems to do the trick, but I haven't looked at hooking it up
in the build system:

=== scripts/check-alias-homonyms.py ===
#!/usr/bin/python3

import sys

sys.path.insert(0, 'scripts/dtc/pylibfdt')
sys.path.insert(1, 'tools')

from dtoc import fdt

ret = 0

for name in sys.argv[1:]:
    dtb = fdt.FdtScan(name)
    aliasnode = dtb.GetNode("/aliases")
    if not aliasnode:
        continue
    basenames = dict()
    for prop in aliasnode.props.values():
        alias = prop.name
        path = prop.value
        base = path[path.rfind('/')+1:]
        if base in basenames:
            basenames[base].append(alias)
        else:
            basenames[base] = [alias]
    for base, aliases in basenames.items():
        if len(aliases) == 1:
            continue
        print("Warning: In %s, the aliases %s all point at nodes with
the basename '%s' - consider (en/dis)abling CONFIG_..." % (name,
",".join(aliases), base))
        ret = 1

sys.exit(ret)
===

I imagine this being run over CONFIG_OF_LIST when CONFIG_... has the
value that disables the fdt_path_offset() call. For concreteness,
something like

config ASSUME_ALIAS_HOMONYMS # or whatever better name one can come up with
  bool "Assume there may be nodes pointed to by aliases in DT that have
identical basenames"
  help
    In most cases, the nodes pointed to by aliases in the device tree
all have different basenames. When this is satisfied, the
fdtdec_get_alias_seq() function can avoid a somewhat expensive full walk
of the dtb when looking for an alias for a given node. If the device
tree for your board does have aliases pointing at nodes with the same
basenames (but of course different full paths), that full walk is
necessary for correctness, and you can then enable this option.

plus

- if (offset != fdt_path_offset(blob, prop))
+ if (IS_ENABLED(CONFIG_ASSUME_ALIAS_HOMONYMS) && offset !=
fdt_path_offset(blob, prop))

I have no idea what running the above python script on the dtb(s) in the
common case of !CONFIG_ASSUME_ALIAS_HOMONYMS adds to buildtime, but I do
believe that before eliding some code that is necessary for correctness
in the general case, we need buildtime verification that it's ok.

Rasmus