Thanks Sam! On Wed, 11 Dec 2024 at 04:17, Sam Protsenko <[email protected]> wrote: > > An attempt to add the already added LMB region 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 the device tree twice, which produces an error message on > second call. > > Return -EEXIST error code in case when the added 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 <[email protected]> > --- > Changes in v2: > - Removed the check for exactly the same region, and return -EEXIST in > the branch handling overlapping regions instead > - Reworded the commit message accordingly > > lib/lmb.c | 26 +++++++++++++------------- > test/lib/lmb.c | 2 +- > 2 files changed, 14 insertions(+), 14 deletions(-) > > diff --git a/lib/lmb.c b/lib/lmb.c > index b03237bc06cb..a695edf70dfa 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) > @@ -217,17 +219,15 @@ static long lmb_add_region_flags(struct alist > *lmb_rgn_lst, phys_addr_t base, > coalesced++; > break; > } else if (lmb_addrs_overlap(base, size, rgnbase, rgnsize)) { > - if (flags == LMB_NONE) { > - ret = lmb_resize_regions(lmb_rgn_lst, i, base, > - size); > - if (ret < 0) > - return -1; > + if (flags != LMB_NONE) > + return -EEXIST; > > - coalesced++; > - break; > - } else { > + ret = lmb_resize_regions(lmb_rgn_lst, i, base, size); > + if (ret < 0) > return -1; > - } > + > + coalesced++; > + break; > } > } > > @@ -667,7 +667,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 +818,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 >
Tom this needs to go in -master for the release Reviewed-by: Ilias Apalodimas <[email protected]>
