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

[Simon Glass]

Hi Rasmus,

On Tue, 5 Jul 2022 at 07:47, Rasmus Villemoes
<rasmus.villemoes at prevas.dk> wrote:
>
> 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

I don't disagree that this is a bit odd, but it is efficient.

Another approach here would be to add better documentation since the
option doesn't quite work as advertised.

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

Opt-out means that everything pays the penalty, though. This is real
corner case. Arguably the device tree should be updated to avoid this
problem.

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

That's a nice idea.

The point of my revert was to get something in for the release. I
fully expect some sort of change to go in afterwards, but I don't
think people were aware of the impact of your patch (see context link
above).

So I still favour a revert, for now.

Regards,
Simon