[PATCH 2/6] lmb: Return -EEXIST in lmb_add_region_flags() if region already added
Ilias Apalodimas
ilias.apalodimas at linaro.org
Sun Dec 8 07:49:49 CET 2024
Hi Sam,
On Sun, 8 Dec 2024 at 02:21, Sam Protsenko <semen.protsenko at linaro.org> wrote:
>
> An attempt to add the already added LMB region (with exactly the same
> start address, size and flags) using lmb_add_region_flags() ends up in
> lmb_addrs_overlap() check, which eventually leads to either returning 0
> if 'flags' is LMB_NONE, or -1 otherwise. It makes it impossible for the
> user of this function to catch the case when the region is already added
> and differentiate it from regular errors. That in turn may lead to
> incorrect error handling in the caller code, like reporting misleading
> errors or interrupting the normal code path where it could be treated as
> the normal case. An example is boot_fdt_reserve_region() function, which
> might be called twice (e.g. during board startup in initr_lmb(), and
> then during 'booti' command booting the OS), thus trying to reserve
> exactly the same memory regions described in device tree twice, which
> produces an error message on second call.
>
> Implement the detection of cases when the already added region is trying
> to be added again, and return -EEXIST error code in case the region
> exists and it's not LMB_NONE; for LMB_NONE return 0, to conform to unit
> tests (specifically test_alloc_addr() in test/lib/lmb.c) and the
> preferred behavior described in commit 1d9aa4a283da ("lmb: Fix the
> allocation of overlapping memory areas with !LMB_NONE"). The change of
> lmb_add_region_flags() return values is described in the table below:
>
> Return case Pre-1d9 1d9 New
> -----------------------------------------------------------
> Added successfully 0 0 0
> Failed to add -1 -1 -1
> Already added, flags == LMB_NONE 0 0 0
> Already added, flags != LMB_NONE 0 -1 -EEXIST
>
> Rework all affected functions and their documentation. Also fix the
> corresponding unit test which checks reserving the same region with the
> same flags to account for the changed return value.
>
> No functional change is intended (by this patch itself).
>
> Fixes: 1d9aa4a283da ("lmb: Fix the allocation of overlapping memory areas with !LMB_NONE")
> Signed-off-by: Sam Protsenko <semen.protsenko at linaro.org>
> ---
> lib/lmb.c | 18 ++++++++++++++----
> test/lib/lmb.c | 2 +-
> 2 files changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/lib/lmb.c b/lib/lmb.c
> index 713f072f75ee..ce0dc49345fb 100644
> --- a/lib/lmb.c
> +++ b/lib/lmb.c
> @@ -183,8 +183,10 @@ static long lmb_resize_regions(struct alist *lmb_rgn_lst,
> * the function might resize an already existing region or coalesce two
> * adjacent regions.
> *
> - *
> - * Returns: 0 if the region addition successful, -1 on failure
> + * Return:
> + * * %0 - Added successfully, or it's already added (only if LMB_NONE)
> + * * %-EEXIST - The region is already added, and flags != LMB_NONE
> + * * %-1 - Failure
> */
> static long lmb_add_region_flags(struct alist *lmb_rgn_lst, phys_addr_t base,
> phys_size_t size, enum lmb_flags flags)
> @@ -202,6 +204,14 @@ static long lmb_add_region_flags(struct alist *lmb_rgn_lst, phys_addr_t base,
> phys_size_t rgnsize = rgn[i].size;
> enum lmb_flags rgnflags = rgn[i].flags;
>
> + /* Already have this region, so we're done */
> + if (base == rgnbase && size == rgnsize && flags == rgnflags) {
> + if (flags == LMB_NONE)
> + return 0;
> + else
> + return -EEXIST;
> + }
The change looks sane and I did plan to clean up LMB after the release
with similar logic.
But I wonder should we return -EEXIST here or a few lines below in
'else if (lmb_addrs_overlap(base, size, rgnbase, rgnsize)) {'
There are two cases we return -1 there, we could return -EEXIST. Something like
diff --git a/lib/lmb.c b/lib/lmb.c
index 3a765c11bee6..f3d5b616c376 100644
--- a/lib/lmb.c
+++ b/lib/lmb.c
@@ -226,7 +226,7 @@ static long lmb_add_region_flags(struct alist
*lmb_rgn_lst, phys_addr_t base,
coalesced++;
break;
} else {
- return -1;
+ return -EEXIST;
}
}
}
I am not sure about the lmb_resize_regions() failure, I think we
should return a different error
> +
> ret = lmb_addrs_adjacent(base, size, rgnbase, rgnsize);
> if (ret > 0) {
> if (flags != rgnflags)
> @@ -667,7 +677,7 @@ long lmb_add(phys_addr_t base, phys_size_t size)
> *
> * Free up a region of memory.
> *
> - * Return: 0 if successful, -1 on failure
> + * Return: 0 if successful, negative error code on failure
> */
> long lmb_free_flags(phys_addr_t base, phys_size_t size,
> uint flags)
> @@ -818,7 +828,7 @@ static phys_addr_t _lmb_alloc_addr(phys_addr_t base, phys_size_t size,
> lmb_memory[rgn].size,
> base + size - 1, 1)) {
> /* ok, reserve the memory */
> - if (lmb_reserve_flags(base, size, flags) >= 0)
> + if (!lmb_reserve_flags(base, size, flags))
> return base;
> }
> }
> diff --git a/test/lib/lmb.c b/test/lib/lmb.c
> index 0bd29e2a4fe7..48c3c966f8f2 100644
> --- a/test/lib/lmb.c
> +++ b/test/lib/lmb.c
> @@ -754,7 +754,7 @@ static int lib_test_lmb_flags(struct unit_test_state *uts)
>
> /* reserve again, same flag */
> ret = lmb_reserve_flags(0x40010000, 0x10000, LMB_NOMAP);
> - ut_asserteq(ret, -1L);
> + ut_asserteq(ret, -EEXIST);
> ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 1, 0x40010000, 0x10000,
> 0, 0, 0, 0);
>
> --
> 2.39.5
>
Other than that it looks good thanks for fixing this!
Regards
/Ilias
More information about the U-Boot
mailing list