[PATCH 2/3] fdt: Allow expanding the devicetree during relocation

[Simon Glass]

Hi Tom,

On Fri, 6 Dec 2024 at 12:49, Tom Rini <trini at konsulko.com> wrote:
>
> On Fri, Dec 06, 2024 at 12:18:07PM -0700, Simon Glass wrote:
> > Hi Tom,
> >
> > On Fri, 6 Dec 2024 at 07:13, Tom Rini <trini at konsulko.com> wrote:
> > >
> > > On Fri, Dec 06, 2024 at 06:11:12AM -0700, Simon Glass wrote:
> > >
> > > [snip]
> > > > A comment in that function says that we assume there is space after the
> > > > existing fdt to use for padding, with the padding size set to
> > > > CONFIG_SYS_FDT_PAD
> > > >
> > > > However, there is no guarantee that this space is available. If using
> > > > the control FDT, then global_data is immediately above it, so expanding
> > > > the FDT and adding FDT properties will cause U-Boot to fail.
> > >
> > > This sounds like a generic issue and should be fixed outside of a "Pi"
> > > series.
> >
> > Yes and I know you keep asking for multiple smaller series, but I've
> > explained that keeping track of all that for months at a time is too
> > challenging for me.
>
> Yes, and it's also too challenging to review for the rest of us.
>
> > > > diff --git a/dts/Kconfig b/dts/Kconfig
> > > > index 41a758e83a6..360611edd01 100644
> > > > --- a/dts/Kconfig
> > > > +++ b/dts/Kconfig
> > > > @@ -219,6 +219,17 @@ config OF_OMIT_DTB
> > > >         This is used for boards which normally provide a devicetree via a
> > > >         runtime mechanism (such as OF_BOARD), to avoid confusion.
> > > >
> > > > +config OF_EXPAND
> > > > +     hex # "Amount to allow the control FDT to expand"
> > >
> > > No "# ..." because the help explains what to do here, if needed.
> >
> > OK
> >
> > >
> > > > +     default SYS_FDT_PAD if OF_LIBFDT
> > > > +     default 0
> > >
> > > A "default 0" is almost always wrong.
> >
> > In this case it is correct, since it means not to add any extra space
> > for the FDT.
>
> Which is still wrong? Because we don't allow for this particular device
> tree in memory to be read-only and so the question is "Do you want to
> break things on some platforms or not?".
>
> > > > +     help
> > > > +       Some boards make use of the control FDT to boot an OS, thus when
> > > > +       image_setup_libfdt() adds extra things to the end of the FDT, there
> > > > +       needs to be enough space.
> > > > +
> > > > +       Set this to the number of bytes of extra space required for the FDT.
> > >
> > > We should just use SYS_FDT_PAD directly and not add a new option.
> >
> > I thought I read somewhere that fdt_high == -1 should not be used. So
> > for most boards, this padding is not needed?
>
> That's also true, and gets back to why putting multiple patches in a
> single series is a Bad Idea. The function in question isn't doing some
> check for if we've disabled device tree relocation for the OS, it's
> early on, it's reserving our control fdt, yes? And then the problem ends
> up being later on, we use the control fdt for the OS, and modify it, but
> haven't ensured there's enough space for said modifications to not stomp
> on something else? That is a much bigger generic problem, no?

It seems that people are already aware of the problem this patch
addresses, but since it is specific to fdt_high, which should not be
used, it seems unnecessary to worry about it.

I'm going to drop this patch.

Regards,
Simon