diff mbox series

[2/6] common: Tidy up how malloc() is inited

Message ID 20241009015020.25817-3-sjg@chromium.org
State Superseded
Delegated to: Tom Rini
Headers show
Series Allow showing the memory map | expand

Commit Message

Simon Glass Oct. 9, 2024, 1:50 a.m. UTC
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(-)

Comments

Ilias Apalodimas Oct. 9, 2024, 10:38 a.m. UTC | #1
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>
Simon Glass Oct. 9, 2024, 9:14 p.m. UTC | #2
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 mbox series

Patch

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