[PATCH v2 1/2] lmb: Return -EEXIST in lmb_add_region_flags() if region already added

[Ilias Apalodimas]

Thanks Sam!

On Wed, 11 Dec 2024 at 04:17, Sam Protsenko <semen.protsenko at linaro.org> wrote:
>
> An attempt to add the already added LMB region 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 the device tree twice, which produces an error message on
> second call.
>
> Return -EEXIST error code in case when the added 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 at linaro.org>
> ---
> Changes in v2:
>   - Removed the check for exactly the same region, and return -EEXIST in
>     the branch handling overlapping regions instead
>   - Reworded the commit message accordingly
>
>  lib/lmb.c      | 26 +++++++++++++-------------
>  test/lib/lmb.c |  2 +-
>  2 files changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/lib/lmb.c b/lib/lmb.c
> index b03237bc06cb..a695edf70dfa 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)
> @@ -217,17 +219,15 @@ static long lmb_add_region_flags(struct alist *lmb_rgn_lst, phys_addr_t base,
>                         coalesced++;
>                         break;
>                 } else if (lmb_addrs_overlap(base, size, rgnbase, rgnsize)) {
> -                       if (flags == LMB_NONE) {
> -                               ret = lmb_resize_regions(lmb_rgn_lst, i, base,
> -                                                        size);
> -                               if (ret < 0)
> -                                       return -1;
> +                       if (flags != LMB_NONE)
> +                               return -EEXIST;
>
> -                               coalesced++;
> -                               break;
> -                       } else {
> +                       ret = lmb_resize_regions(lmb_rgn_lst, i, base, size);
> +                       if (ret < 0)
>                                 return -1;
> -                       }
> +
> +                       coalesced++;
> +                       break;
>                 }
>         }
>
> @@ -667,7 +667,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 +818,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
>

Tom this needs to go in -master for the release
Reviewed-by: Ilias Apalodimas <ilias.apalodimas at linaro.org>