Re: [PATCH 1/2] lmb: Fix lmb_add_region_flags() return codes and testing

Hi Ilias,

On 23/10/2024 17:22, Ilias Apalodimas wrote:

The function description says this should return 0 or -1 on failures. When regions coalesce though this returns the number of coalescedregions

* coalesced regions

which is confusing and requires special handling of the return code. On top of that no one is using the number of coalesced regions.

So let's just return 0 on success and adjust our selftests accordingly

Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org

Thanks for following up on this!

Reviewed-by: Caleb Connolly caleb.connolly@linaro.org

---

boot/image-fdt.c | 2 +- lib/lmb.c | 10 +++++----- test/lib/lmb.c | 10 +++++----- 3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/boot/image-fdt.c b/boot/image-fdt.c index 8eda521693dc..eac09059d955 100644 --- a/boot/image-fdt.c +++ b/boot/image-fdt.c @@ -73,7 +73,7 @@ static void boot_fdt_reserve_region(u64 addr, u64 size, enum lmb_flags flags) long ret;

ret = lmb_reserve_flags(addr, size, flags);

• if (ret >= 0) {

• if (!ret) { debug(" reserving fdt memory region: addr=%llx size=%llx flags=%x\n", (unsigned long long)addr, (unsigned long long)size, flags);

diff --git a/lib/lmb.c b/lib/lmb.c index 7e90f178763b..8ce58fcb9224 100644 --- a/lib/lmb.c +++ b/lib/lmb.c @@ -450,7 +450,7 @@ static long lmb_add_region_flags(struct alist *lmb_rgn_lst, phys_addr_t base, }

if (coalesced)

• return coalesced;
  

• return 0;
  
  

  if (alist_full(lmb_rgn_lst) && !alist_expand_by(lmb_rgn_lst, lmb_rgn_lst->alloc))

@@ -487,7 +487,7 @@ long lmb_add(phys_addr_t base, phys_size_t size) struct alist *lmb_rgn_lst = &lmb.free_mem;

ret = lmb_add_region(lmb_rgn_lst, base, size);

• if (ret < 0)

• if (ret) return ret;

  if (lmb_should_notify(LMB_NONE))

@@ -583,8 +583,8 @@ long lmb_reserve_flags(phys_addr_t base, phys_size_t size, enum lmb_flags flags) struct alist *lmb_rgn_lst = &lmb.used_mem;

ret = lmb_add_region_flags(lmb_rgn_lst, base, size, flags);

• if (ret < 0)

• return -1;
  

• if (ret)

• return ret;
  
  

  if (lmb_should_notify(flags)) return lmb_map_update_notify(base, size, MAP_OP_RESERVE);

@@ -651,7 +651,7 @@ static phys_addr_t _lmb_alloc_base(phys_size_t size, ulong align, if (rgn < 0) { /* This area isn't reserved, take it */ if (lmb_add_region_flags(&lmb.used_mem, base,

• 					 size, flags) < 0)
  

• 					 size, flags))
  		return 0;
  
  	if (lmb_should_notify(flags)) {
  

diff --git a/test/lib/lmb.c b/test/lib/lmb.c index b2c54fb4bcb8..c917115b7b66 100644 --- a/test/lib/lmb.c +++ b/test/lib/lmb.c @@ -473,7 +473,7 @@ static int lib_test_lmb_overlapping_reserve(struct unit_test_state *uts)

/* allocate overlapping region should return the coalesced count */ ret = lmb_reserve(0x40011000, 0x10000);

• ut_asserteq(ret, 1);

• ut_asserteq(ret, 0); ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 1, 0x40010000, 0x11000, 0, 0, 0, 0); /* allocate 3nd region */

@@ -748,13 +748,13 @@ static int lib_test_lmb_flags(struct unit_test_state *uts)

/* merge after */ ret = lmb_reserve_flags(0x40020000, 0x10000, LMB_NOMAP);

• ut_asserteq(ret, 1);

• ut_asserteq(ret, 0); ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 1, 0x40010000, 0x20000, 0, 0, 0, 0);

  /* merge before */ ret = lmb_reserve_flags(0x40000000, 0x10000, LMB_NOMAP);

• ut_asserteq(ret, 1);

• ut_asserteq(ret, 0); ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 1, 0x40000000, 0x30000, 0, 0, 0, 0);

@@ -770,7 +770,7 @@ static int lib_test_lmb_flags(struct unit_test_state *uts)

/* test that old API use LMB_NONE */ ret = lmb_reserve(0x40040000, 0x10000);

• ut_asserteq(ret, 1);

• ut_asserteq(ret, 0); ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 2, 0x40000000, 0x30000, 0x40030000, 0x20000, 0, 0);

@@ -789,7 +789,7 @@ static int lib_test_lmb_flags(struct unit_test_state *uts)

/* merge with 2 adjacent regions */ ret = lmb_reserve_flags(0x40060000, 0x10000, LMB_NOMAP);

• ut_asserteq(ret, 2);

• ut_asserteq(ret, 0); ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 3, 0x40000000, 0x30000, 0x40030000, 0x20000, 0x40050000, 0x30000);