[U-Boot] [PATCH 3/4] malloc_simple: Return NULL on malloc failure rather then calling panic()

[Siarhei Siamashka]

On Wed,  4 Feb 2015 13:05:50 +0100
Hans de Goede <hdegoede at redhat.com> wrote:

> All callers of malloc should already do error checking, and may even be able
> to continue without the alloc succeeding.
> 
> Moreover, common/malloc_simple.c is the only user of .rodata.str1.1 in
> common/built-in.o when building the SPL, triggering this gcc bug:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=54303
> 
> Causing .rodata to grow with e.g. 0xc21 bytes, nullifying all benefits of
> using malloc_simple in the first place.
> 
> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> ---
>  common/malloc_simple.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/common/malloc_simple.c b/common/malloc_simple.c
> index afdacff..64ae036 100644
> --- a/common/malloc_simple.c
> +++ b/common/malloc_simple.c
> @@ -19,7 +19,7 @@ void *malloc_simple(size_t bytes)
>  
>  	new_ptr = gd->malloc_ptr + bytes;
>  	if (new_ptr > gd->malloc_limit)
> -		panic("Out of pre-reloc memory");
> +		return NULL;
>  	ptr = map_sysmem(gd->malloc_base + gd->malloc_ptr, bytes);
>  	gd->malloc_ptr = ALIGN(new_ptr, sizeof(new_ptr));
>  	return ptr;

The other patches look great, but I'm not convinced that requiring the
malloc callers to do error checking is such a great idea. This means a
lot of checks in a lot of places with extra code paths instead of just
a single check in one place for the "impossible to happen" critical
failure. I think that we should normally assume that malloc always
succeeds in the production code and the panic may only happen while
debugging.

If the malloc pool is in the DRAM, then we usually have orders of
magnitude more space than necessary. While the code might be still
in the SRAM at the same time (the extra branching code logic for
errors checking may be wasting the scarce SRAM space).

If the malloc pool is in the SRAM and potentially may fail allocations,
then this is a major reliability problem by itself. The malloc pool is
always inefficient, has fragmentation problems, etc. If this is the
case, then IMHO the only right solution is to replace such problematic
dynamic allocations with static reservations in the ".data" section.
Otherwise the reliability critical things (something like Mars rovers
for example) will be sometimes failing. The Murphy law exists for
a reason :-)

The workaround for the GCC compiler bug is orthogonal to this.
Maybe there is some other solution?

-- 
Best regards,
Siarhei Siamashka