[U-Boot] [PATCH 05/10] dfu: don't keep freeing/reallocating

[Lukasz Majewski <l.majewski@samsung.com>]

Hi Stephen,

> From: Stephen Warren <swarren@nvidia.com>
> 
> DFU currently allocates buffer memory at the start of each data
> transfer operation and frees it at the end. Especially since
> memalign() is used to allocate the buffer, and various other
> allocations happen during the transfer, this can expose the code to
> heap fragmentation, which prevents the allocation from succeeding on
> subsequent transfers.
> 
> Fix the code to allocate the buffer once when DFU mode is initialized,
> and free the buffer once when DFU mode is exited, to reduce the
> exposure to heap fragmentation.
> 
> The failure mode is:
> 
> // Internally to memalign(), this allocates a lot more than s to
> guarantee // that alignment can occur, then returns chunks of memory
> at the start/ // end of the allocated buffer to the heap.
> p = memalign(a, s);
> // Various other malloc()s occur here, some of which allocate the RAM
> // immediately before/after "p".
> //
> // DFU transfer is complete, so buffer is released.
> free(p);
> // By chance, no other malloc()/free() here, in DFU at least.
> //
> // A new DFU transfer starts, so the buffer is allocated again.
> // In theory this should succeed since we just free()d a buffer of the
> // same size. However, this fails because memalign() internally
> attempts // to allocate much more than "s", yet free(p) above only
> free()d a // little more than "s".
> p = memalign(a, s);
> 
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
>  drivers/dfu/dfu.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c
> index 675162d927d8..d85d3f507a7b 100644
> --- a/drivers/dfu/dfu.c
> +++ b/drivers/dfu/dfu.c
> @@ -164,7 +164,6 @@ static int dfu_write_buffer_drain(struct
> dfu_entity *dfu) void dfu_write_transaction_cleanup(struct dfu_entity
> *dfu) {
>  	/* clear everything */
> -	dfu_free_buf();
>  	dfu->crc = 0;
>  	dfu->offset = 0;
>  	dfu->i_blk_seq_num = 0;
> @@ -385,7 +384,6 @@ int dfu_read(struct dfu_entity *dfu, void *buf,
> int size, int blk_seq_num) dfu_hash_algo->name, dfu->crc);
>  		puts("\nUPLOAD ... done\nCtrl+C to exit ...\n");
>  
> -		dfu_free_buf();
>  		dfu->i_blk_seq_num = 0;
>  		dfu->crc = 0;
>  		dfu->offset = 0;
> @@ -433,6 +431,7 @@ static int dfu_fill_entity(struct dfu_entity
> *dfu, char *s, int alt, __func__,  interface);
>  		return -1;
>  	}
> +	dfu_get_buf(dfu);
>  
>  	return 0;
>  }
> @@ -441,6 +440,7 @@ void dfu_free_entities(void)
>  {
>  	struct dfu_entity *dfu, *p, *t = NULL;
>  
> +	dfu_free_buf();
>  	list_for_each_entry_safe_reverse(dfu, p, &dfu_list, list) {
>  		list_del(&dfu->list);
>  		if (dfu->free_entity)

Acked-by: Lukasz Majewski <l.majewski@samsung.com>
Tested-by: Lukasz Majewski <l.majewski@samsung.com>

Test HW: Odroid XU3 - Exynos5433
[DFU tests]

-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group