| Message ID | 20241023152203.1218242-1-ilias.apalodimas@linaro.org |
|---|---|
| State | Accepted |
| Commit | 0f57b009e649e9d140b7f662599a5ace506e2418 |
| Headers | show |
| Series | [1/2] lmb: Fix lmb_add_region_flags() return codes and testing | expand |
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); >
On Wed, 23 Oct 2024 18:22:00 +0300, 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 > 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 > > [...] Applied to u-boot/master, thanks!
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);
The function description says this should return 0 or -1 on failures. When regions coalesce though this returns the number of coalescedregions 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> --- boot/image-fdt.c | 2 +- lib/lmb.c | 10 +++++----- test/lib/lmb.c | 10 +++++----- 3 files changed, 11 insertions(+), 11 deletions(-)