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

[Rasmus Villemoes]

On 05/07/2022 11.47, Simon Glass wrote:
> Hi Tom,
> 
> On Sun, 3 Jul 2022 at 06:43, Tom Rini <trini at konsulko.com> wrote:
>>
>> On Sun, Jul 03, 2022 at 02:32:42AM -0600, Simon Glass wrote:
>>> Hi,
>>>
>>> On Sun, 3 Jul 2022 at 02:25, Simon Glass <sjg at chromium.org> wrote:
>>>>
>>>> The fdt_path_offset() function is slow since it must scan the tree.
>>>> This substantial overhead now applies to all boards.
>>>>
>>>> The original code may not be ideal but it is fit for purpose and is only
>>>> needed on a few boards.
>>>>
>>>> We should revert this in time for the release.
>>>>
>>>> This reverts commit 26f981f295d00351b6f0c69b5317b254b2361cc0.
>>>>
>>>> Signed-off-by: Simon Glass <sjg at chromium.org>
>>>> ---
>>>>
>>>>  configs/am65x_evm_a53_defconfig  | 1 +
>>>>  configs/evb-ast2600_defconfig    | 1 +
>>>>  configs/sama7g5ek_mmc1_defconfig | 1 +
>>>>  configs/sama7g5ek_mmc_defconfig  | 1 +
>>>>  lib/Kconfig                      | 7 +++++++
>>>>  lib/fdtdec.c                     | 7 +++++--
>>>>  6 files changed, 16 insertions(+), 2 deletions(-)
>>>
>>> Please also see the context here:
>>>
>>> https://patchwork.ozlabs.org/project/uboot/patch/20201111142605.17034-1-a-govindraju@ti.com/
>>
>> Previously when we've had issues of making the fast path slow, people
>> have posted measurements of before/after in order to demonstrate the
>> problem.  Can we please get some logs of before/after and various
>> possible solutions?  Thanks.
> 
> Well this code is not needed at all for all but four boards.

Three. One board seems to enable that config for no reason at all. And
it wouldn't work on that board, because the code was fragile and error
prone. See my detailed explanations in the original patch thread.

It does a
> very expensive check of the DT and this can happen before relocation,
> or is SPL. I don't have a board to test with at present, but I expect
> it would cost 10s of milliseconds on AT91, for example. 

As I've said before, I prefer code which is correct out-of-the-box. I
also prefer simpler code with less ifdefs (yeah yeah, IS_ENABLED...) and
fewer configuration options to worry about. The three boards which have
aliases where the leaf nodes have duplicate names also happen to enable
some other unrelated magic config knob which happens to make the phandle
comparison work. So at the very least the code should stop comparing
phandles and just compare the node offsets; whether that check should be
under a config knob can be discussed (certainly that config knob should
not have PHANDLE in it), but, again, as I've said, it should be opt-out,
and preferably with a build-time check that verifies that no two aliases
point at same basenames.

_If_ that 10s of milliseconds figure is true, there are other things one
could do at build time. Say have some pass over the dtb which simply
adds "u-boot,seq" properties to the target nodes of aliases, using that
if present, or add a "back pointer" "u-boot,alias" property one could
compare to name.

Rasmus