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

[Tom Rini]

On Sat, Jul 30, 2022 at 07:27:26PM -0600, Simon Glass wrote:
> 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?

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?

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20220731/c50664ed/attachment.sig>