Message ID | 20241009015020.25817-3-sjg@chromium.org |
---|---|
State | Superseded |
Delegated to: | Tom Rini |
Headers | show |
Series | Allow showing the memory map | expand |
Hi Simon, On Wed, 9 Oct 2024 at 04:50, Simon Glass <sjg@chromium.org> wrote: > > The call to malloc() is a bit strange. The naming of the arguments > suggests that an address is passed, but in fact it is a pointer, at > least in the board_init_r() function and SPL equivalent. > > Update it to work as described. Add a function comment as well. > > Note that this does adjustment does not extend into the malloc() > implementation itself, apart from changing mem_malloc_init(), since > there are lots of casts and pointers and integers are used > interchangeably. > > Signed-off-by: Simon Glass <sjg@chromium.org> > --- > > common/board_r.c | 3 +-- > common/dlmalloc.c | 8 +++++--- > common/spl/spl.c | 4 +--- > include/malloc.h | 8 ++++++++ > 4 files changed, 15 insertions(+), 8 deletions(-) > > diff --git a/common/board_r.c b/common/board_r.c > index 4faaa202421..fbb10fea494 100644 > --- a/common/board_r.c > +++ b/common/board_r.c > @@ -203,8 +203,7 @@ static int initr_malloc(void) > */ > start = gd->relocaddr - TOTAL_MALLOC_LEN; > gd_set_malloc_start(start); > - mem_malloc_init((ulong)map_sysmem(start, TOTAL_MALLOC_LEN), > - TOTAL_MALLOC_LEN); > + mem_malloc_init(start, TOTAL_MALLOC_LEN); > return 0; > } > > diff --git a/common/dlmalloc.c b/common/dlmalloc.c > index 1ac7ce3f43c..cc4d3a0a028 100644 > --- a/common/dlmalloc.c > +++ b/common/dlmalloc.c > @@ -16,6 +16,8 @@ > #include <asm/global_data.h> > > #include <malloc.h> > +#include <mapmem.h> > +#include <string.h> > #include <asm/io.h> > #include <valgrind/memcheck.h> > > @@ -598,9 +600,9 @@ void *sbrk(ptrdiff_t increment) > > void mem_malloc_init(ulong start, ulong size) > { > - mem_malloc_start = start; > - mem_malloc_end = start + size; > - mem_malloc_brk = start; > + mem_malloc_start = (ulong)map_sysmem(start, size); > + mem_malloc_end = mem_malloc_start + size; > + mem_malloc_brk = mem_malloc_start; > > #ifdef CONFIG_SYS_MALLOC_DEFAULT_TO_INIT > malloc_init(); > diff --git a/common/spl/spl.c b/common/spl/spl.c > index c13b2b8f714..fc8b38862e8 100644 > --- a/common/spl/spl.c > +++ b/common/spl/spl.c > @@ -679,9 +679,7 @@ void board_init_r(gd_t *dummy1, ulong dummy2) > spl_set_bd(); > > if (IS_ENABLED(CONFIG_SPL_SYS_MALLOC)) { > - mem_malloc_init((ulong)map_sysmem(SPL_SYS_MALLOC_START, > - SPL_SYS_MALLOC_SIZE), > - SPL_SYS_MALLOC_SIZE); > + mem_malloc_init(SPL_SYS_MALLOC_START, SPL_SYS_MALLOC_SIZE); > gd->flags |= GD_FLG_FULL_MALLOC_INIT; > } > if (!(gd->flags & GD_FLG_SPL_INIT)) { > diff --git a/include/malloc.h b/include/malloc.h > index 07d3e90a855..9e0be482416 100644 > --- a/include/malloc.h > +++ b/include/malloc.h > @@ -981,6 +981,14 @@ extern ulong mem_malloc_start; > extern ulong mem_malloc_end; > extern ulong mem_malloc_brk; > > +/** > + * mem_malloc_init() - Set up the malloc() pool > + * > + * Sets the region of memory to be used for all future calls to malloc(), etc. > + * > + * @start: Start address > + * @size: Size in bytes > + */ > void mem_malloc_init(ulong start, ulong size); > > #ifdef __cplusplus > -- > 2.43.0 > This looks ok as long as map_sysmem() is a 1:1 mapping for non-sandbox archs. NXP seems to be calling these with an address already. I guess since we only define an alternative map_sysmem() for sandbox this is ok Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Hi Ilias, On Wed, 9 Oct 2024 at 04:38, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Hi Simon, > > On Wed, 9 Oct 2024 at 04:50, Simon Glass <sjg@chromium.org> wrote: > > > > The call to malloc() is a bit strange. The naming of the arguments > > suggests that an address is passed, but in fact it is a pointer, at > > least in the board_init_r() function and SPL equivalent. > > > > Update it to work as described. Add a function comment as well. > > > > Note that this does adjustment does not extend into the malloc() > > implementation itself, apart from changing mem_malloc_init(), since > > there are lots of casts and pointers and integers are used > > interchangeably. > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > --- > > > > common/board_r.c | 3 +-- > > common/dlmalloc.c | 8 +++++--- > > common/spl/spl.c | 4 +--- > > include/malloc.h | 8 ++++++++ > > 4 files changed, 15 insertions(+), 8 deletions(-) > > > > diff --git a/common/board_r.c b/common/board_r.c > > index 4faaa202421..fbb10fea494 100644 > > --- a/common/board_r.c > > +++ b/common/board_r.c > > @@ -203,8 +203,7 @@ static int initr_malloc(void) > > */ > > start = gd->relocaddr - TOTAL_MALLOC_LEN; > > gd_set_malloc_start(start); > > - mem_malloc_init((ulong)map_sysmem(start, TOTAL_MALLOC_LEN), > > - TOTAL_MALLOC_LEN); > > + mem_malloc_init(start, TOTAL_MALLOC_LEN); > > return 0; > > } > > > > diff --git a/common/dlmalloc.c b/common/dlmalloc.c > > index 1ac7ce3f43c..cc4d3a0a028 100644 > > --- a/common/dlmalloc.c > > +++ b/common/dlmalloc.c > > @@ -16,6 +16,8 @@ > > #include <asm/global_data.h> > > > > #include <malloc.h> > > +#include <mapmem.h> > > +#include <string.h> > > #include <asm/io.h> > > #include <valgrind/memcheck.h> > > > > @@ -598,9 +600,9 @@ void *sbrk(ptrdiff_t increment) > > > > void mem_malloc_init(ulong start, ulong size) > > { > > - mem_malloc_start = start; > > - mem_malloc_end = start + size; > > - mem_malloc_brk = start; > > + mem_malloc_start = (ulong)map_sysmem(start, size); > > + mem_malloc_end = mem_malloc_start + size; > > + mem_malloc_brk = mem_malloc_start; > > > > #ifdef CONFIG_SYS_MALLOC_DEFAULT_TO_INIT > > malloc_init(); > > diff --git a/common/spl/spl.c b/common/spl/spl.c > > index c13b2b8f714..fc8b38862e8 100644 > > --- a/common/spl/spl.c > > +++ b/common/spl/spl.c > > @@ -679,9 +679,7 @@ void board_init_r(gd_t *dummy1, ulong dummy2) > > spl_set_bd(); > > > > if (IS_ENABLED(CONFIG_SPL_SYS_MALLOC)) { > > - mem_malloc_init((ulong)map_sysmem(SPL_SYS_MALLOC_START, > > - SPL_SYS_MALLOC_SIZE), > > - SPL_SYS_MALLOC_SIZE); > > + mem_malloc_init(SPL_SYS_MALLOC_START, SPL_SYS_MALLOC_SIZE); > > gd->flags |= GD_FLG_FULL_MALLOC_INIT; > > } > > if (!(gd->flags & GD_FLG_SPL_INIT)) { > > diff --git a/include/malloc.h b/include/malloc.h > > index 07d3e90a855..9e0be482416 100644 > > --- a/include/malloc.h > > +++ b/include/malloc.h > > @@ -981,6 +981,14 @@ extern ulong mem_malloc_start; > > extern ulong mem_malloc_end; > > extern ulong mem_malloc_brk; > > > > +/** > > + * mem_malloc_init() - Set up the malloc() pool > > + * > > + * Sets the region of memory to be used for all future calls to malloc(), etc. > > + * > > + * @start: Start address > > + * @size: Size in bytes > > + */ > > void mem_malloc_init(ulong start, ulong size); > > > > #ifdef __cplusplus > > -- > > 2.43.0 > > > > This looks ok as long as map_sysmem() is a 1:1 mapping for non-sandbox > archs. NXP seems to be calling these with an address already. Yes it is > I guess since we only define an alternative map_sysmem() for sandbox this is ok > > Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> Thanks. Regards, Simon
diff --git a/common/board_r.c b/common/board_r.c index 4faaa202421..fbb10fea494 100644 --- a/common/board_r.c +++ b/common/board_r.c @@ -203,8 +203,7 @@ static int initr_malloc(void) */ start = gd->relocaddr - TOTAL_MALLOC_LEN; gd_set_malloc_start(start); - mem_malloc_init((ulong)map_sysmem(start, TOTAL_MALLOC_LEN), - TOTAL_MALLOC_LEN); + mem_malloc_init(start, TOTAL_MALLOC_LEN); return 0; } diff --git a/common/dlmalloc.c b/common/dlmalloc.c index 1ac7ce3f43c..cc4d3a0a028 100644 --- a/common/dlmalloc.c +++ b/common/dlmalloc.c @@ -16,6 +16,8 @@ #include <asm/global_data.h> #include <malloc.h> +#include <mapmem.h> +#include <string.h> #include <asm/io.h> #include <valgrind/memcheck.h> @@ -598,9 +600,9 @@ void *sbrk(ptrdiff_t increment) void mem_malloc_init(ulong start, ulong size) { - mem_malloc_start = start; - mem_malloc_end = start + size; - mem_malloc_brk = start; + mem_malloc_start = (ulong)map_sysmem(start, size); + mem_malloc_end = mem_malloc_start + size; + mem_malloc_brk = mem_malloc_start; #ifdef CONFIG_SYS_MALLOC_DEFAULT_TO_INIT malloc_init(); diff --git a/common/spl/spl.c b/common/spl/spl.c index c13b2b8f714..fc8b38862e8 100644 --- a/common/spl/spl.c +++ b/common/spl/spl.c @@ -679,9 +679,7 @@ void board_init_r(gd_t *dummy1, ulong dummy2) spl_set_bd(); if (IS_ENABLED(CONFIG_SPL_SYS_MALLOC)) { - mem_malloc_init((ulong)map_sysmem(SPL_SYS_MALLOC_START, - SPL_SYS_MALLOC_SIZE), - SPL_SYS_MALLOC_SIZE); + mem_malloc_init(SPL_SYS_MALLOC_START, SPL_SYS_MALLOC_SIZE); gd->flags |= GD_FLG_FULL_MALLOC_INIT; } if (!(gd->flags & GD_FLG_SPL_INIT)) { diff --git a/include/malloc.h b/include/malloc.h index 07d3e90a855..9e0be482416 100644 --- a/include/malloc.h +++ b/include/malloc.h @@ -981,6 +981,14 @@ extern ulong mem_malloc_start; extern ulong mem_malloc_end; extern ulong mem_malloc_brk; +/** + * mem_malloc_init() - Set up the malloc() pool + * + * Sets the region of memory to be used for all future calls to malloc(), etc. + * + * @start: Start address + * @size: Size in bytes + */ void mem_malloc_init(ulong start, ulong size); #ifdef __cplusplus
The call to malloc() is a bit strange. The naming of the arguments suggests that an address is passed, but in fact it is a pointer, at least in the board_init_r() function and SPL equivalent. Update it to work as described. Add a function comment as well. Note that this does adjustment does not extend into the malloc() implementation itself, apart from changing mem_malloc_init(), since there are lots of casts and pointers and integers are used interchangeably. Signed-off-by: Simon Glass <sjg@chromium.org> --- common/board_r.c | 3 +-- common/dlmalloc.c | 8 +++++--- common/spl/spl.c | 4 +--- include/malloc.h | 8 ++++++++ 4 files changed, 15 insertions(+), 8 deletions(-)