Hi Tom, On Fri, 6 Dec 2024 at 12:49, Tom Rini <tr...@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 <tr...@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
