[PATCH 2/6] lmb: Return -EEXIST in lmb_add_region_flags() if region already added
Sam Protsenko
semen.protsenko at linaro.org
Wed Dec 11 02:36:41 CET 2024
On Mon, Dec 9, 2024 at 1:08 AM Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
>
> 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.
>
I'll send v2 soon, addressing all comments and including this suggestion.
> Sam, can you please check if this change also fixes the issue that you
> see. Thanks.
>
Indeed. Three birds with one stone :) Meaning the sandbox one is fixed
as well. That was a really good suggestion, to move -EEXIST. Thanks
for letting me know about fixed cases!
> -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