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

[Simon Glass]

Hi Tom,

On Tue, 5 Jul 2022 at 10:42, Simon Glass <sjg at chromium.org> wrote:
>
> 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.

It looks like this didn't make it for the release. I'm not sure if
this is causing problems.

Shall I pick it up for the upcoming release?

Regards,
Simon