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

[Simon Glass]

Hi Rasmus,

On Mon, 1 Aug 2022 at 07:13, Rasmus Villemoes
<rasmus.villemoes at prevas.dk> wrote:
>
> 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?

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.

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

Regards,
Simon