| Message ID | 20241208002121.31887-3-semen.protsenko@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | lmb: Fix reserving the same region multiple times | expand |
Hi Sam, On Sun, 8 Dec 2024 at 02:21, Sam Protsenko <semen.protsenko@linaro.org> wrote: > > An attempt to add the already added LMB region (with exactly the same > start address, size and flags) 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 device tree twice, which > produces an error message on second call. > > Implement the detection of cases when the already added region is trying > to be added again, and return -EEXIST error code in case the 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 <semen.protsenko@linaro.org> > --- > lib/lmb.c | 18 ++++++++++++++---- > test/lib/lmb.c | 2 +- > 2 files changed, 15 insertions(+), 5 deletions(-) > > diff --git a/lib/lmb.c b/lib/lmb.c > index 713f072f75ee..ce0dc49345fb 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) > @@ -202,6 +204,14 @@ static long lmb_add_region_flags(struct alist *lmb_rgn_lst, phys_addr_t base, > phys_size_t rgnsize = rgn[i].size; > enum lmb_flags rgnflags = rgn[i].flags; > > + /* Already have this region, so we're done */ > + if (base == rgnbase && size == rgnsize && flags == rgnflags) { > + if (flags == LMB_NONE) > + return 0; > + else > + return -EEXIST; > + } The change looks sane and I did plan to clean up LMB after the release with similar logic. But I wonder should we return -EEXIST here or a few lines below in 'else if (lmb_addrs_overlap(base, size, rgnbase, rgnsize)) {' There are two cases we return -1 there, we could return -EEXIST. Something like diff --git a/lib/lmb.c b/lib/lmb.c index 3a765c11bee6..f3d5b616c376 100644 --- a/lib/lmb.c +++ b/lib/lmb.c @@ -226,7 +226,7 @@ static long lmb_add_region_flags(struct alist *lmb_rgn_lst, phys_addr_t base, coalesced++; break; } else { - return -1; + return -EEXIST; } } } I am not sure about the lmb_resize_regions() failure, I think we should return a different error > + > ret = lmb_addrs_adjacent(base, size, rgnbase, rgnsize); > if (ret > 0) { > if (flags != rgnflags) > @@ -667,7 +677,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 +828,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 > Other than that it looks good thanks for fixing this! Regards /Ilias
On Sun, 8 Dec 2024 at 12:20, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Hi Sam, > > On Sun, 8 Dec 2024 at 02:21, Sam Protsenko <semen.protsenko@linaro.org> wrote: > > > > An attempt to add the already added LMB region (with exactly the same > > start address, size and flags) 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 device tree twice, which > > produces an error message on second call. > > > > Implement the detection of cases when the already added region is trying > > to be added again, and return -EEXIST error code in case the 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 <semen.protsenko@linaro.org> > > --- > > lib/lmb.c | 18 ++++++++++++++---- > > test/lib/lmb.c | 2 +- > > 2 files changed, 15 insertions(+), 5 deletions(-) > > > > diff --git a/lib/lmb.c b/lib/lmb.c > > index 713f072f75ee..ce0dc49345fb 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) > > @@ -202,6 +204,14 @@ static long lmb_add_region_flags(struct alist *lmb_rgn_lst, phys_addr_t base, > > phys_size_t rgnsize = rgn[i].size; > > enum lmb_flags rgnflags = rgn[i].flags; > > > > + /* Already have this region, so we're done */ > > + if (base == rgnbase && size == rgnsize && flags == rgnflags) { > > + if (flags == LMB_NONE) > > + return 0; > > + else > > + return -EEXIST; > > + } > > The change looks sane and I did plan to clean up LMB after the release > with similar logic. > But I wonder should we return -EEXIST here or a few lines below in > 'else if (lmb_addrs_overlap(base, size, rgnbase, rgnsize)) {' > > There are two cases we return -1 there, we could return -EEXIST. Something like > diff --git a/lib/lmb.c b/lib/lmb.c > index 3a765c11bee6..f3d5b616c376 100644 > --- a/lib/lmb.c > +++ b/lib/lmb.c > @@ -226,7 +226,7 @@ static long lmb_add_region_flags(struct alist > *lmb_rgn_lst, phys_addr_t base, > coalesced++; > break; > } else { > - return -1; > + return -EEXIST; > } > } > } > +1 for this approach. Otherwise we are effectively reverting 1d9aa4a283da. Sam, can you please check if this change also fixes the issue that you see. Thanks. -sughosh > I am not sure about the lmb_resize_regions() failure, I think we > should return a different error > > > + > > ret = lmb_addrs_adjacent(base, size, rgnbase, rgnsize); > > if (ret > 0) { > > if (flags != rgnflags) > > @@ -667,7 +677,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 +828,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 > > > > Other than that it looks good thanks for fixing this! > Regards > /Ilias
On Sun, Dec 8, 2024 at 12:50 AM Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Hi Sam, > > On Sun, 8 Dec 2024 at 02:21, Sam Protsenko <semen.protsenko@linaro.org> wrote: > > > > An attempt to add the already added LMB region (with exactly the same > > start address, size and flags) 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 device tree twice, which > > produces an error message on second call. > > > > Implement the detection of cases when the already added region is trying > > to be added again, and return -EEXIST error code in case the 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 <semen.protsenko@linaro.org> > > --- > > lib/lmb.c | 18 ++++++++++++++---- > > test/lib/lmb.c | 2 +- > > 2 files changed, 15 insertions(+), 5 deletions(-) > > > > diff --git a/lib/lmb.c b/lib/lmb.c > > index 713f072f75ee..ce0dc49345fb 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) > > @@ -202,6 +204,14 @@ static long lmb_add_region_flags(struct alist *lmb_rgn_lst, phys_addr_t base, > > phys_size_t rgnsize = rgn[i].size; > > enum lmb_flags rgnflags = rgn[i].flags; > > > > + /* Already have this region, so we're done */ > > + if (base == rgnbase && size == rgnsize && flags == rgnflags) { > > + if (flags == LMB_NONE) > > + return 0; > > + else > > + return -EEXIST; > > + } > > The change looks sane and I did plan to clean up LMB after the release > with similar logic. > But I wonder should we return -EEXIST here or a few lines below in > 'else if (lmb_addrs_overlap(base, size, rgnbase, rgnsize)) {' > Yes, that makes sense. That's actually the exact line of code I initially debugged the problem to. Adding the "exactly the same region" corner case was a bad idea, because it misses the case when a region was coalesced with some other region, and is now contained inside of a bigger block. I will rework that in v2. > There are two cases we return -1 there, we could return -EEXIST. Something like > diff --git a/lib/lmb.c b/lib/lmb.c > index 3a765c11bee6..f3d5b616c376 100644 > --- a/lib/lmb.c > +++ b/lib/lmb.c > @@ -226,7 +226,7 @@ static long lmb_add_region_flags(struct alist > *lmb_rgn_lst, phys_addr_t base, > coalesced++; > break; > } else { > - return -1; > + return -EEXIST; > } > } > } > > I am not sure about the lmb_resize_regions() failure, I think we > should return a different error > No, that seems like a different case. I think all 'region exists' cases for !LMB_NONE are handled in the code you referenced above, and all LMB_NONE cases are handled below, in "if (coalesced) return 0;" code. Right now it looks like only LMB_NONE regions can be merged. I wonder if it's intended, because it's not mentioned in corresponding LMB_* flags documentation, other than LMB_NOOVERWRITE. Also, commit "lmb: allow for resizing lmb regions" only mentions LMB_NOOVERWRITE. Anyways, that's out of scope here I guess. > > + > > ret = lmb_addrs_adjacent(base, size, rgnbase, rgnsize); > > if (ret > 0) { > > if (flags != rgnflags) > > @@ -667,7 +677,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 +828,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 > > > > Other than that it looks good thanks for fixing this! > Regards > /Ilias
On Mon, Dec 9, 2024 at 1:08 AM Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > On Sun, 8 Dec 2024 at 12:20, Ilias Apalodimas > <ilias.apalodimas@linaro.org> wrote: > > > > Hi Sam, > > > > On Sun, 8 Dec 2024 at 02:21, Sam Protsenko <semen.protsenko@linaro.org> wrote: > > > > > > An attempt to add the already added LMB region (with exactly the same > > > start address, size and flags) 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 device tree twice, which > > > produces an error message on second call. > > > > > > Implement the detection of cases when the already added region is trying > > > to be added again, and return -EEXIST error code in case the 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 <semen.protsenko@linaro.org> > > > --- > > > lib/lmb.c | 18 ++++++++++++++---- > > > test/lib/lmb.c | 2 +- > > > 2 files changed, 15 insertions(+), 5 deletions(-) > > > > > > diff --git a/lib/lmb.c b/lib/lmb.c > > > index 713f072f75ee..ce0dc49345fb 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) > > > @@ -202,6 +204,14 @@ static long lmb_add_region_flags(struct alist *lmb_rgn_lst, phys_addr_t base, > > > phys_size_t rgnsize = rgn[i].size; > > > enum lmb_flags rgnflags = rgn[i].flags; > > > > > > + /* Already have this region, so we're done */ > > > + if (base == rgnbase && size == rgnsize && flags == rgnflags) { > > > + if (flags == LMB_NONE) > > > + return 0; > > > + else > > > + return -EEXIST; > > > + } > > > > The change looks sane and I did plan to clean up LMB after the release > > with similar logic. > > But I wonder should we return -EEXIST here or a few lines below in > > 'else if (lmb_addrs_overlap(base, size, rgnbase, rgnsize)) {' > > > > There are two cases we return -1 there, we could return -EEXIST. Something like > > diff --git a/lib/lmb.c b/lib/lmb.c > > index 3a765c11bee6..f3d5b616c376 100644 > > --- a/lib/lmb.c > > +++ b/lib/lmb.c > > @@ -226,7 +226,7 @@ static long lmb_add_region_flags(struct alist > > *lmb_rgn_lst, phys_addr_t base, > > coalesced++; > > break; > > } else { > > - return -1; > > + return -EEXIST; > > } > > } > > } > > > > +1 for this approach. Otherwise we are effectively reverting 1d9aa4a283da. > I'll send v2 soon, addressing all comments and including this suggestion. > Sam, can you please check if this change also fixes the issue that you > see. Thanks. > Indeed. Three birds with one stone :) Meaning the sandbox one is fixed as well. That was a really good suggestion, to move -EEXIST. Thanks for letting me know about fixed cases! > -sughosh > > > I am not sure about the lmb_resize_regions() failure, I think we > > should return a different error > > > > > + > > > ret = lmb_addrs_adjacent(base, size, rgnbase, rgnsize); > > > if (ret > 0) { > > > if (flags != rgnflags) > > > @@ -667,7 +677,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 +828,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 > > > > > > > Other than that it looks good thanks for fixing this! > > Regards > > /Ilias
diff --git a/lib/lmb.c b/lib/lmb.c index 713f072f75ee..ce0dc49345fb 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) @@ -202,6 +204,14 @@ static long lmb_add_region_flags(struct alist *lmb_rgn_lst, phys_addr_t base, phys_size_t rgnsize = rgn[i].size; enum lmb_flags rgnflags = rgn[i].flags; + /* Already have this region, so we're done */ + if (base == rgnbase && size == rgnsize && flags == rgnflags) { + if (flags == LMB_NONE) + return 0; + else + return -EEXIST; + } + ret = lmb_addrs_adjacent(base, size, rgnbase, rgnsize); if (ret > 0) { if (flags != rgnflags) @@ -667,7 +677,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 +828,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);
An attempt to add the already added LMB region (with exactly the same start address, size and flags) 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 device tree twice, which produces an error message on second call. Implement the detection of cases when the already added region is trying to be added again, and return -EEXIST error code in case the 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 <semen.protsenko@linaro.org> --- lib/lmb.c | 18 ++++++++++++++---- test/lib/lmb.c | 2 +- 2 files changed, 15 insertions(+), 5 deletions(-)