[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.apalodimas at linaro.org>

Thanks for following up on this!

Reviewed-by: Caleb Connolly <caleb.connolly at 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)