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

[Caleb Connolly]

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.apalodi...@linaro.org>

Thanks for following up on this!

Reviewed-by: Caleb Connolly <caleb.conno...@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);

--
// Caleb (they/them)