[PATCH v2 2/2] lmb: Fix adjacent region merge in lmb_add_region_flags()

[Patrice CHOTARD]

On 4/12/24 17:53, Patrice Chotard wrote:
> In case a new region is adjacent to a previous region with
> similar flag, this region is merged with its predecessor, but no
> check are done if this new added region is overlapping another region
> present in lmb (see reserved[3] which overlaps reserved[4]).
> 
> This occurs when the LMB [0xdaafd000-0xddb18000] is added and overlaps
> the LMB [0xdbaf4380-0xddffffff].
> 
> Call lmb_overlaps_region() before merging the new region with the
> adjacent region already present in lmb.
> 
> In case of adjacent region found, code is 90% similar in case
> adjacent region is located before/after the new region.
> Factorize adjacent region management in lmb_add_region_flags().
> 
> Issue reproduced on STM32MP157-DK2 with SCMI DT, bdinfo command's output
> shows:
> 
> before this patch:
> ...
> lmb_dump_all:
>  memory.cnt = 0x1 / max = 0x2
>  memory[0]      [0xc0000000-0xdfffffff], 0x20000000 bytes flags: 0
>  reserved.cnt = 0x5 / max = 0x10
>  reserved[0]    [0x10000000-0x10045fff], 0x00046000 bytes flags: 4
>  reserved[1]    [0x30000000-0x3003ffff], 0x00040000 bytes flags: 4
>  reserved[2]    [0x38000000-0x3800ffff], 0x00010000 bytes flags: 4
>  reserved[3]    [0xdaae1000-0xdfffffff], 0x0551f000 bytes flags: 0
>  reserved[4]    [0xde000000-0xdfffffff], 0x02000000 bytes flags: 4
> ...
> 
> after this patch:
> 
> ...
> lmb_dump_all:
>  memory.cnt = 0x1 / max = 0x2
>  memory[0]      [0xc0000000-0xdfffffff], 0x20000000 bytes flags: 0
>  reserved.cnt = 0x5 / max = 0x10
>  reserved[0]    [0x10000000-0x10045fff], 0x00046000 bytes flags: 4
>  reserved[1]    [0x30000000-0x3003ffff], 0x00040000 bytes flags: 4
>  reserved[2]    [0x38000000-0x3800ffff], 0x00010000 bytes flags: 4
>  reserved[3]    [0xdaae1000-0xddffffff], 0x0351f000 bytes flags: 0
>  reserved[4]    [0xde000000-0xdfffffff], 0x02000000 bytes flags: 4
> ...
> 
> Fixes: 4ed6552f7159 ("[new uImage] Introduce lmb from linux kernel for memory mgmt of boot images")
> 
> Signed-off-by: Patrice Chotard <patrice.chotard at foss.st.com>
> ---
> 
> Changes in v2:
>   _ Fix lmb_add_region_flags() by updating test which leads to
>     extend an existing region
> 
>  lib/lmb.c | 57 +++++++++++++++++++++++++++++--------------------------
>  1 file changed, 30 insertions(+), 27 deletions(-)
> 
> diff --git a/lib/lmb.c b/lib/lmb.c
> index b6afb731440..4ed60f4a843 100644
> --- a/lib/lmb.c
> +++ b/lib/lmb.c
> @@ -130,6 +130,22 @@ static void lmb_fix_over_lap_regions(struct lmb_region *rgn, unsigned long r1,
>  	lmb_remove_region(rgn, r2);
>  }
>  
> +static long lmb_overlaps_region(struct lmb_region *rgn, phys_addr_t base,
> +				phys_size_t size)
> +{
> +	unsigned long i;
> +
> +	for (i = 0; i < rgn->cnt; i++) {
> +		phys_addr_t rgnbase = rgn->region[i].base;
> +		phys_size_t rgnsize = rgn->region[i].size;
> +
> +		if (lmb_addrs_overlap(base, size, rgnbase, rgnsize))
> +			break;
> +	}
> +
> +	return (i < rgn->cnt) ? i : -1;
> +}
> +
>  void lmb_init(struct lmb *lmb)
>  {
>  #if IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS)
> @@ -257,7 +273,7 @@ static long lmb_add_region_flags(struct lmb_region *rgn, phys_addr_t base,
>  				 phys_size_t size, enum lmb_flags flags)
>  {
>  	unsigned long coalesced = 0;
> -	long adjacent, i;
> +	long adjacent, i, overlap;
>  
>  	if (rgn->cnt == 0) {
>  		rgn->region[0].base = base;
> @@ -283,19 +299,21 @@ static long lmb_add_region_flags(struct lmb_region *rgn, phys_addr_t base,
>  		}
>  
>  		adjacent = lmb_addrs_adjacent(base, size, rgnbase, rgnsize);
> -		if (adjacent > 0) {
> -			if (flags != rgnflags)
> -				continue;
> -			rgn->region[i].base -= size;
> -			rgn->region[i].size += size;
> -			coalesced++;
> -			break;
> -		} else if (adjacent < 0) {
> +		if (adjacent != 0) {
>  			if (flags != rgnflags)
>  				continue;
> -			rgn->region[i].size += size;
> -			coalesced++;
> -			break;
> +			overlap = lmb_overlaps_region(rgn, base, size);
> +			if (overlap < 0 || flags == rgn->region[overlap].flags) {
> +				/*
> +				 * no overlap detected or overlap with same flags detected,
> +				 * extend region
> +				 */
> +				if  (adjacent > 0)
> +					rgn->region[i].base -= size;
> +				rgn->region[i].size += size;
> +				coalesced++;
> +				break;
> +			}
>  		} else if (lmb_addrs_overlap(base, size, rgnbase, rgnsize)) {
>  			/* regions overlap */
>  			return -1;
> @@ -420,21 +438,6 @@ long lmb_reserve(struct lmb *lmb, phys_addr_t base, phys_size_t size)
>  	return lmb_reserve_flags(lmb, base, size, LMB_NONE);
>  }
>  
> -static long lmb_overlaps_region(struct lmb_region *rgn, phys_addr_t base,
> -				phys_size_t size)
> -{
> -	unsigned long i;
> -
> -	for (i = 0; i < rgn->cnt; i++) {
> -		phys_addr_t rgnbase = rgn->region[i].base;
> -		phys_size_t rgnsize = rgn->region[i].size;
> -		if (lmb_addrs_overlap(base, size, rgnbase, rgnsize))
> -			break;
> -	}
> -
> -	return (i < rgn->cnt) ? i : -1;
> -}
> -
>  phys_addr_t lmb_alloc(struct lmb *lmb, phys_size_t size, ulong align)
>  {
>  	return lmb_alloc_base(lmb, size, align, LMB_ALLOC_ANYWHERE);


I think this series (v2) is not correct even if now the CI tests are OK.
After re-reading carefully the lib_test_lmb_overlapping_reserve() test 
it appears to me there is a contradiction.

It's indicating that "check that calling lmb_reserve with overlapping regions fails" 

but the very last test of lib_test_lmb_overlapping_reserve() has this comment :
/* allocate 3rd region, coalesce with first and overlap with second */
and this test allows this overlap case.

It's not clear if LMB region can overlap each other or not ?

Udit, your patch edb5824be17f ("lmb: remove overlapping region with next range")
is authorizing LMB overlapping right ? 

Patrice