| Message ID | 20240311143918.2927406-1-patrice.chotard@foss.st.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [1/2] lmb: Avoid to add identical region in lmb_add_region_flags() | expand |
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;
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;
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
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
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;
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(-)