[PATCH 1/2] lmb: Avoid to add identical region in lmb_add_region_flags()

[PATCH 1/2] lmb: Avoid to add identical region in lmb_add_region_flags()

[Patrice Chotard]

In case lmb_add_region_flags() is called with the same parameter than
an already existing lmb and this lmb is adjacent to its previous lmb with
different flag, this lmb is added again.

Instead breaking the loop, continue, at the next iteration, we are able
to detect that this region already exist.

Issue reproduced on STM32MP157-DK2 with SCMI DT, bdinfo command's output
shows:

Before this patch, the last LMB [0xde000000-0xdfffffff] is duplicated:
...
lmb_dump_all:nnn
 memory.cnt = 0x1 / max = 0x2
 memory[0]      [0xc0000000-0xdfffffff], 0x20000000 bytes flags: 0
 reserved.cnt = 0x6 / 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]    [0xdaadf000-0xdfffffff], 0x05521000 bytes flags: 0
 reserved[4]    [0xde000000-0xdfffffff], 0x02000000 bytes flags: 4
 reserved[5]    [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]    [0xdaadf000-0xdfffffff], 0x05521000 bytes flags: 0
 reserved[4]    [0xde000000-0xdfffffff], 0x02000000 bytes flags: 4
...

Fixes: 59c0ea5df33f ("lmb: Add support of flags for no-map properties")

Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com>
---

 lib/lmb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/lmb.c b/lib/lmb.c
index 44f98205310..b6afb731440 100644
--- a/lib/lmb.c
+++ b/lib/lmb.c
@@ -285,14 +285,14 @@ 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)
-				break;
+				continue;
 			rgn->region[i].base -= size;
 			rgn->region[i].size += size;
 			coalesced++;
 			break;
 		} else if (adjacent < 0) {
 			if (flags != rgnflags)
-				break;
+				continue;
 			rgn->region[i].size += size;
 			coalesced++;
 			break;
-- 
2.25.1

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

[Patrice Chotard]

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]    [0xdaadf000-0xdfffffff], 0x05521000 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 = 0x6 / 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]    [0xdaadf000-0xdaaf7fff], 0x00019000 bytes flags: 0
 reserved[4]    [0xdbaf4380-0xddffffff], 0x0250bc80 bytes flags: 0
 reserved[5]    [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@foss.st.com>
---

 lib/lmb.c | 55 ++++++++++++++++++++++++++++---------------------------
 1 file changed, 28 insertions(+), 27 deletions(-)

diff --git a/lib/lmb.c b/lib/lmb.c
index b6afb731440..f583b10f776 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,19 @@ 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) {
+				/* no overlap detected, extend region */
+				if  (adjacent > 0)
+					rgn->region[i].base -= size;
+				rgn->region[i].size += size;
+				coalesced++;
+				break;
+			}
+			continue;
 		} else if (lmb_addrs_overlap(base, size, rgnbase, rgnsize)) {
 			/* regions overlap */
 			return -1;
@@ -420,21 +436,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);
-- 
2.25.1

Re: [PATCH 1/2] lmb: Avoid to add identical region in lmb_add_region_flags()

[Kumar, Udit]

On 3/11/2024 8:09 PM, Patrice Chotard wrote:
> In case lmb_add_region_flags() is called with the same parameter than
> an already existing lmb and this lmb is adjacent to its previous lmb with
> different flag, this lmb is added again.


Same parameter means , addr and size and different flags correct ?


> Instead breaking the loop, continue, at the next iteration, we are able
> to detect that this region already exist.

Once region already exist detected, then you can think of returning 
error , no ?


> Issue reproduced on STM32MP157-DK2 with SCMI DT, bdinfo command's output
> shows:
>
> Before this patch, the last LMB [0xde000000-0xdfffffff] is duplicated:
> ...
> lmb_dump_all:nnn
>   memory.cnt = 0x1 / max = 0x2
>   memory[0]      [0xc0000000-0xdfffffff], 0x20000000 bytes flags: 0
>   reserved.cnt = 0x6 / 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]    [0xdaadf000-0xdfffffff], 0x05521000 bytes flags: 0
>   reserved[4]    [0xde000000-0xdfffffff], 0x02000000 bytes flags: 4
>   reserved[5]    [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]    [0xdaadf000-0xdfffffff], 0x05521000 bytes flags: 0
>   reserved[4]    [0xde000000-0xdfffffff], 0x02000000 bytes flags: 4
> ...
>
> Fixes: 59c0ea5df33f ("lmb: Add support of flags for no-map properties")
>
> Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com>
> ---
>
>   lib/lmb.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/lib/lmb.c b/lib/lmb.c
> index 44f98205310..b6afb731440 100644
> --- a/lib/lmb.c
> +++ b/lib/lmb.c
> @@ -285,14 +285,14 @@ 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)
> -				break;
> +				continue;
>   			rgn->region[i].base -= size;
>   			rgn->region[i].size += size;
>   			coalesced++;
>   			break;
>   		} else if (adjacent < 0) {
>   			if (flags != rgnflags)
> -				break;
> +				continue;
>   			rgn->region[i].size += size;
>   			coalesced++;
>   			break;

Re: [PATCH 1/2] lmb: Avoid to add identical region in lmb_add_region_flags()

[Francesco Dolcini]

Hello Patrice,

On Mon, Mar 11, 2024 at 03:39:17PM +0100, Patrice Chotard wrote:
> In case lmb_add_region_flags() is called with the same parameter than
> an already existing lmb and this lmb is adjacent to its previous lmb with
> different flag, this lmb is added again.

Is this

On Mon, Mar 11, 2024 at 03:39:18PM +0100, 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]).

or this, related to some

ERROR: reserving fdt memory region failed

message in your opinion?

More details in https://lore.kernel.org/all/fe9431c5-6806-1b7a-f9f4-dbe97ee13bba@toradex.com/

Francesco

Re: [PATCH 1/2] lmb: Avoid to add identical region in lmb_add_region_flags()

[Patrice CHOTARD]

On 3/11/24 16:36, Kumar, Udit wrote:
> 
> On 3/11/2024 8:09 PM, Patrice Chotard wrote:
>> In case lmb_add_region_flags() is called with the same parameter than
>> an already existing lmb and this lmb is adjacent to its previous lmb with
>> different flag, this lmb is added again.
> 
> 
> Same parameter means , addr and size and different flags correct ?

Hi Udit,

Same parameters means same addr, same size and same flags.

> 
> 
>> Instead breaking the loop, continue, at the next iteration, we are able
>> to detect that this region already exist.
> 
> Once region already exist detected, then you can think of returning error , no ?

Before detecting this region is already present, it's detected 
that this region we want to add is adjacent to a region present in rgn[] array.

That's why i replace the "break" by a "continue" to be able to detect this region is already
present in rgn[] in the next iteration.

Thanks
Patrice

> 
> 
>> Issue reproduced on STM32MP157-DK2 with SCMI DT, bdinfo command's output
>> shows:
>>
>> Before this patch, the last LMB [0xde000000-0xdfffffff] is duplicated:
>> ...
>> lmb_dump_all:nnn
>>   memory.cnt = 0x1 / max = 0x2
>>   memory[0]      [0xc0000000-0xdfffffff], 0x20000000 bytes flags: 0
>>   reserved.cnt = 0x6 / 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]    [0xdaadf000-0xdfffffff], 0x05521000 bytes flags: 0
>>   reserved[4]    [0xde000000-0xdfffffff], 0x02000000 bytes flags: 4
>>   reserved[5]    [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]    [0xdaadf000-0xdfffffff], 0x05521000 bytes flags: 0
>>   reserved[4]    [0xde000000-0xdfffffff], 0x02000000 bytes flags: 4
>> ...
>>
>> Fixes: 59c0ea5df33f ("lmb: Add support of flags for no-map properties")
>>
>> Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com>
>> ---
>>
>>   lib/lmb.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/lmb.c b/lib/lmb.c
>> index 44f98205310..b6afb731440 100644
>> --- a/lib/lmb.c
>> +++ b/lib/lmb.c
>> @@ -285,14 +285,14 @@ 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)
>> -                break;
>> +                continue;
>>               rgn->region[i].base -= size;
>>               rgn->region[i].size += size;
>>               coalesced++;
>>               break;
>>           } else if (adjacent < 0) {
>>               if (flags != rgnflags)
>> -                break;
>> +                continue;
>>               rgn->region[i].size += size;
>>               coalesced++;
>>               break;

Re: [PATCH 1/2] lmb: Avoid to add identical region in lmb_add_region_flags()

[Patrice CHOTARD]

On 3/11/24 20:32, Francesco Dolcini wrote:
> Hello Patrice,
> 
> On Mon, Mar 11, 2024 at 03:39:17PM +0100, Patrice Chotard wrote:
>> In case lmb_add_region_flags() is called with the same parameter than
>> an already existing lmb and this lmb is adjacent to its previous lmb with
>> different flag, this lmb is added again.
> 
> Is this
> 
> On Mon, Mar 11, 2024 at 03:39:18PM +0100, 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]).
> 
> or this, related to some
> 
> ERROR: reserving fdt memory region failed
> 
> message in your opinion?
> 
> More details in https://lore.kernel.org/all/fe9431c5-6806-1b7a-f9f4-dbe97ee13bba@toradex.com/

Hi Francesco

I observed also "ERROR: reserving fdt memory region failed" on STM32MP1 platform,
the issue was fixed using efi_add_memory_map() as done in this patch :

https://patchwork.ozlabs.org/project/uboot/patch/20240308101230.2595220-1-patrice.chotard@foss.st.com/

Thanks
Patrice

> 
> Francesco
> 

Re: [PATCH 1/2] lmb: Avoid to add identical region in lmb_add_region_flags()

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1940 bytes --]

On Mon, Mar 11, 2024 at 03:39:17PM +0100, Patrice Chotard wrote:

> In case lmb_add_region_flags() is called with the same parameter than
> an already existing lmb and this lmb is adjacent to its previous lmb with
> different flag, this lmb is added again.
> 
> Instead breaking the loop, continue, at the next iteration, we are able
> to detect that this region already exist.
> 
> Issue reproduced on STM32MP157-DK2 with SCMI DT, bdinfo command's output
> shows:
> 
> Before this patch, the last LMB [0xde000000-0xdfffffff] is duplicated:
> ...
> lmb_dump_all:nnn
>  memory.cnt = 0x1 / max = 0x2
>  memory[0]      [0xc0000000-0xdfffffff], 0x20000000 bytes flags: 0
>  reserved.cnt = 0x6 / 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]    [0xdaadf000-0xdfffffff], 0x05521000 bytes flags: 0
>  reserved[4]    [0xde000000-0xdfffffff], 0x02000000 bytes flags: 4
>  reserved[5]    [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]    [0xdaadf000-0xdfffffff], 0x05521000 bytes flags: 0
>  reserved[4]    [0xde000000-0xdfffffff], 0x02000000 bytes flags: 4
> ...
> 
> Fixes: 59c0ea5df33f ("lmb: Add support of flags for no-map properties")
> 
> Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com>

This series leads to CI failures:
https://source.denx.de/u-boot/u-boot/-/jobs/814084

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 1/2] lmb: Avoid to add identical region in lmb_add_region_flags()

[Patrice CHOTARD]

On 4/10/24 17:28, Tom Rini wrote:
> On Mon, Mar 11, 2024 at 03:39:17PM +0100, Patrice Chotard wrote:
> 
>> In case lmb_add_region_flags() is called with the same parameter than
>> an already existing lmb and this lmb is adjacent to its previous lmb with
>> different flag, this lmb is added again.
>>
>> Instead breaking the loop, continue, at the next iteration, we are able
>> to detect that this region already exist.
>>
>> Issue reproduced on STM32MP157-DK2 with SCMI DT, bdinfo command's output
>> shows:
>>
>> Before this patch, the last LMB [0xde000000-0xdfffffff] is duplicated:
>> ...
>> lmb_dump_all:nnn
>>  memory.cnt = 0x1 / max = 0x2
>>  memory[0]      [0xc0000000-0xdfffffff], 0x20000000 bytes flags: 0
>>  reserved.cnt = 0x6 / 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]    [0xdaadf000-0xdfffffff], 0x05521000 bytes flags: 0
>>  reserved[4]    [0xde000000-0xdfffffff], 0x02000000 bytes flags: 4
>>  reserved[5]    [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]    [0xdaadf000-0xdfffffff], 0x05521000 bytes flags: 0
>>  reserved[4]    [0xde000000-0xdfffffff], 0x02000000 bytes flags: 4
>> ...
>>
>> Fixes: 59c0ea5df33f ("lmb: Add support of flags for no-map properties")
>>
>> Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com>
> 
> This series leads to CI failures:
> https://source.denx.de/u-boot/u-boot/-/jobs/814084
> 

Hi Tom

i will have a look at it , thanks

Patrice