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

[Simon Glass]

Hi Rasmus,

On Tue, 18 Apr 2023 at 23:30, Rasmus Villemoes
<rasmus.villemoes at prevas.dk> wrote:
>
> 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.

OK. Part of the issue with pinctrl might be that drivers should be
caching the DT in their internal structures, rather than scanning it
each time pinctrl_select_state() is called.

>
> > 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.

Probably it just needs to go in Makefile:

u-boot.dtb: dts/dt.dtb
    python <your_script.py> $<
    $(call cmd,copy)


>
> 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.

I've thought for a long time that it would be great to have a table of
phandles (array of node offsets in a property in a suitable node) and
a second 'aliases' node (with a different name) that uses node offsets
instead of strings. Haven't done it though...

>
> 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).

OK

Regards,
Simon