[PATCH 2/6] lmb: Return -EEXIST in lmb_add_region_flags() if region already added
Sughosh Ganu
sughosh.ganu at linaro.org
Mon Dec 9 08:07:48 CET 2024
On Sun, 8 Dec 2024 at 12:20, Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> 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;
> }
> }
> }
>
+1 for this approach. Otherwise we are effectively reverting 1d9aa4a283da.
Sam, can you please check if this change also fixes the issue that you
see. Thanks.
-sughosh
> 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