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

[Rasmus Villemoes]

On 19/04/2023 03.49, Simon Glass wrote:
> Hi Rasmus,
> 
> Returning to this old thread...
> 
>>
>> 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.
> 
> The runtime performance impact is real. We actually have quite a
> problem in general, e.g. the pinctrl drivers can take ages to operate
> before relocation because they read the DT multiple times.

OK. For other reasons, I'm looking at the upstream dtc code, and have
some ideas for performance improvements to the "raw blob walking" code.
But I don't have any idea how large an impact it could have, if it would
help that pinctrl case, or when I'll actually find some spare cycles to
work on it.

> Your script seems like a great idea to me. Perhaps it should produce
> an error if the CONFIG is not enabled?

I was imagining that it would only run when the config is not enabled
(because when it is enabled, correctness is guaranteed at runtime), and
AFAICT it already does produce an error when it finds homonyms. But it
really was just something I wrote in five minutes, I have no idea how to
hook it up in the build system or how to propagate that error. Anybody
is welcome to take that code and polish it up and make a real patch out
of it.

I also think I suggested doing something else at build time, something
along the lines of 'for each alias, inject a property in the target node
with name "u-boot,dm-alias" and value the name of the alias'. Then the
run-time code could simply start by fetching the node's u-boot,dm-alias
property, and if present, check if its value starts with the required
stem. That would likely be even faster than what we do today, but the
current code would probably need to be kept as a fallback.

Of course that somewhat assumes that no node has two aliases, but the
semantics of that is already ill-defined, and I don't think it's a real
issue (and the code doing the injection could easily detect it and warn
or bail).

Rasmus