Re: [PATCHv3 2/6] lmb: add LMB_FDT for fdt reserved regions

[Randolph Sapp <rs@ti.com>]

On Fri Apr 17, 2026 at 3:12 AM CDT, Ilias Apalodimas wrote:
> On Thu, 16 Apr 2026 at 23:32, Randolph Sapp <rs@ti.com> wrote:
>>
>> On Thu Apr 16, 2026 at 2:54 PM CDT, Ilias Apalodimas wrote:
>> > On Thu, 16 Apr 2026 at 22:23, Randolph Sapp <rs@ti.com> wrote:
>> >>
>> >> On Thu Apr 16, 2026 at 3:39 AM CDT, Ilias Apalodimas wrote:
>> >> > Hi Randolph.
>> >> >
>> >> > On Mon, 13 Apr 2026 at 23:36, <rs@ti.com> wrote:
>> >> >>
>> >> >> From: Randolph Sapp <rs@ti.com>
>> >> >>
>> >> >> Add an LMB_FDT bit for fdt reserved regions, so we can reclaim them when
>> >> >> parsing a new device tree and properly warn people when a reservation
>> >> >> overlaps with an existing allocation.
>> >> >
>> >> > The LMB has a set of regions that clearly describe memory in an
>> >> > abstact way, e.g reserved, no overwrite etc.
>> >> > I don't think adding we should open the door for treating reserved
>> >> > memory as special. Can't we apply the same fix without adding a new
>> >> > description?
>> >> >
>> >> > Thanks
>> >> > /Ilias
>> >>
>> >> I did have an initial implementation using a separate allocation list for fdt
>> >> regions in LMB. It works, but boy it's much more messy. All allocations and
>> >> frees had to check both the regular allocation list and the fdt allocation list.
>> >>
>> >> I thought about having the fdt helpers build their own local list separate from
>> >> LMB, but that's also a little messy in that it means we're either double
>> >> tracking regions or tying to reach into the guts of LMB to extract information
>> >> about that existing allocation region.
>> >>
>> >> Maybe that's fine considering LMB already reaches out to kick
>> >> boot_fdt_add_mem_rsv_regions. Just trying to keep things compartmentalized at
>> >> the moment.
>> >>
>> >> I'll revisit the localized reservation list for image-fdt. I'll just setup some
>> >> goofy linked list to track allocation start and size for the later free. I
>> >> assume we prefer a separate set of lmb_region structs for that list as it's a
>> >> little concerning to pass references to an active LMB region outside of the LMB
>> >> core.
>> >
>> > Can you explain a bit more when the problem happens? Is it because
>> > boot_fdt_add_mem_rsv_regions() get called twice in some boot flows? Or
>> > is there another reason?
>> >
>> > Thanks
>> > /Ilias
>>
>> The initial comment explicitly calls out that as the reason they disabled the
>> warning, but it should occur any time a device tree is loaded. Internal or
>> external.
>>
>
> Yes, but that's requesting to reserve specific address(es) which is
> always the same address ranges unless the DTB changes.  In which case
> you just have to find and free the reserved-ranges. Keeping in mind
> that's rare, you can re-parse the DT with minimal overhead and you
> dont have to keep any metadat stored in LMB or anywhere else. and the
> LMB subsystem is not the place to do that. The EEXIST basically means
> "I've already done that" here so I am trying to understand what can
> trigger this problem.
>
>> That also lead me to believe if one were to load multiple external device trees
>> that you could theoretically trip an OOM with a bunch of non-overlapping
>> reserved regions. I have yet to test that, but culling old allocations seemed
>> practical enough without that PoC.
>
> Yea but that's a user error. You can also load a bigger binary that
> your entire DRAM and trigger an OOM
>
> Thanks
> /Ilias

The explicit case were I really wish we had this warning was when the u-boot
stack was creeping into a FDT reserved region. The FDT reservation naturally
failed with EEXIST. That error is overloaded as is. We have to track the
reservations we successfully create in order to clean them up properly.

>>
>> >>
>> >> >>
>> >> >> If we don't at least warn the user of these reservation failures,
>> >> >> there's a chance that this region could be freed and reallocated for
>> >> >> something important later.
>> >> >>
>> >> >> This useful warning mechanism was broken in:
>> >> >> 5a6aa7d5913 ("boot: fdt: Handle already reserved memory in boot_fdt_reserve_region()")
>> >> >>
>> >> >> Signed-off-by: Randolph Sapp <rs@ti.com>
>> >> >> ---
>> >> >>  boot/image-fdt.c |  5 ++++-
>> >> >>  include/lmb.h    | 14 ++++++++++++++
>> >> >>  lib/lmb.c        | 33 +++++++++++++++++++++++++++++----
>> >> >>  3 files changed, 47 insertions(+), 5 deletions(-)
>> >> >>
>> >> >> diff --git a/boot/image-fdt.c b/boot/image-fdt.c
>> >> >> index a3a4fb8b558..0f5857f24d2 100644
>> >> >> --- a/boot/image-fdt.c
>> >> >> +++ b/boot/image-fdt.c
>> >> >> @@ -73,6 +73,7 @@ static void boot_fdt_reserve_region(u64 addr, u64 size, u32 flags)
>> >> >>  {
>> >> >>         long ret;
>> >> >>         phys_addr_t rsv_addr;
>> >> >> +       flags |= LMB_FDT;
>> >> >>
>> >> >>         rsv_addr = (phys_addr_t)addr;
>> >> >>         ret = lmb_alloc_mem(LMB_MEM_ALLOC_ADDR, 0, &rsv_addr, size, flags);
>> >> >> @@ -80,7 +81,7 @@ static void boot_fdt_reserve_region(u64 addr, u64 size, u32 flags)
>> >> >>                 debug("   reserving fdt memory region: addr=%llx size=%llx flags=%x\n",
>> >> >>                       (unsigned long long)addr,
>> >> >>                       (unsigned long long)size, flags);
>> >> >> -       } else if (ret != -EEXIST && ret != -EINVAL) {
>> >> >> +       } else if (ret != -EINVAL) {
>> >> >>                 puts("ERROR: reserving fdt memory region failed ");
>> >> >>                 printf("(addr=%llx size=%llx flags=%x)\n",
>> >> >>                        (unsigned long long)addr,
>> >> >> @@ -108,6 +109,8 @@ void boot_fdt_add_mem_rsv_regions(void *fdt_blob)
>> >> >>         if (fdt_check_header(fdt_blob) != 0)
>> >> >>                 return;
>> >> >>
>> >> >> +       lmb_free_fdt_regions();
>> >> >> +
>> >> >>         /* process memreserve sections */
>> >> >>         total = fdt_num_mem_rsv(fdt_blob);
>> >> >>         for (i = 0; i < total; i++) {
>> >> >> diff --git a/include/lmb.h b/include/lmb.h
>> >> >> index 427d701bc30..c6a1fc1ca47 100644
>> >> >> --- a/include/lmb.h
>> >> >> +++ b/include/lmb.h
>> >> >> @@ -51,6 +51,15 @@
>> >> >>   */
>> >> >>  #define LMB_NONOTIFY BIT(3)
>> >> >>
>> >> >> +/**
>> >> >> + * define LMB_FDT - reclaim this region with lmb_free_fdt_regions()
>> >> >> + *
>> >> >> + * LMB Memory region attribute flag to indicate that the region will be
>> >> >> + * reclaimed with lmb_free_fdt_regions(). This allows device tree reservations
>> >> >> + * to be cleaned up and tracked more granularly.
>> >> >> + */
>> >> >> +#define LMB_FDT BIT(4)
>> >> >> +
>> >> >>  /**
>> >> >>   * enum lmb_mem_type - type of memory allocation request
>> >> >>   * @LMB_MEM_ALLOC_ADDR:        request for a particular region of memory
>> >> >> @@ -235,6 +244,11 @@ phys_addr_t io_lmb_alloc(struct lmb *io_lmb, phys_size_t size, ulong align);
>> >> >>   */
>> >> >>  long io_lmb_free(struct lmb *io_lmb, phys_addr_t base, phys_size_t size);
>> >> >>
>> >> >> +/**
>> >> >> + * lmb_free_fdt_regions() - Reclaim all %LMB_FDT tagged reserved regions
>> >> >> + */
>> >> >> +void lmb_free_fdt_regions(void);
>> >> >> +
>> >> >>  #endif /* __KERNEL__ */
>> >> >>
>> >> >>  #endif /* _LINUX_LMB_H */
>> >> >> diff --git a/lib/lmb.c b/lib/lmb.c
>> >> >> index 8f12c6ad8e5..7ecc548d831 100644
>> >> >> --- a/lib/lmb.c
>> >> >> +++ b/lib/lmb.c
>> >> >> @@ -463,10 +463,10 @@ static int lmb_map_update_notify(phys_addr_t addr, phys_size_t size,
>> >> >>
>> >> >>  static void lmb_print_region_flags(u32 flags)
>> >> >>  {
>> >> >> -       const char * const flag_str[] = { "none", "no-map", "no-overwrite",
>> >> >> -                                         "no-notify" };
>> >> >> -       unsigned int pflags = flags &
>> >> >> -                             (LMB_NOMAP | LMB_NOOVERWRITE | LMB_NONOTIFY);
>> >> >> +       const char *const flag_str[] = { "none", "no-map", "no-overwrite",
>> >> >> +                                        "no-notify", "fdt" };
>> >> >> +       unsigned int pflags =
>> >> >> +               flags & (LMB_NOMAP | LMB_NOOVERWRITE | LMB_NONOTIFY | LMB_FDT);
>> >> >>
>> >> >>         if (flags != pflags) {
>> >> >>                 printf("invalid %#x\n", flags);
>> >> >> @@ -654,6 +654,31 @@ long lmb_free(phys_addr_t base, phys_size_t size, u32 flags)
>> >> >>         return lmb_map_update_notify(base, size, LMB_MAP_OP_FREE, flags);
>> >> >>  }
>> >> >>
>> >> >> +void lmb_free_fdt_regions(void)
>> >> >> +{
>> >> >> +       struct alist *lmb_rgn_lst = &lmb.used_mem;
>> >> >> +       struct lmb_region *rgn = lmb_rgn_lst->data;
>> >> >> +       long ret;
>> >> >> +       int i = 0;
>> >> >> +
>> >> >> +       while (i < lmb_rgn_lst->count) {
>> >> >> +               phys_addr_t base = rgn[i].base;
>> >> >> +               phys_size_t size = rgn[i].size;
>> >> >> +               u32 flags = rgn[i].flags;
>> >> >> +
>> >> >> +               if (flags & LMB_FDT) {
>> >> >> +                       ret = lmb_free(base, size, flags);
>> >> >> +                       if (ret < 0) {
>> >> >> +                               printf("Unable to free FDT memory at 0x%08lx\n",
>> >> >> +                                      (ulong)base);
>> >> >> +                               i++;
>> >> >> +                       }
>> >> >> +               } else {
>> >> >> +                       i++;
>> >> >> +               }
>> >> >> +       }
>> >> >> +}
>> >> >> +
>> >> >>  static int _lmb_alloc_base(phys_size_t size, ulong align,
>> >> >>                            phys_addr_t *addr, u32 flags)
>> >> >>  {
>> >> >> --
>> >> >> 2.53.0
>> >> >>
>> >>
>>