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

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(-)

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]

On Fri, Sep 04, 2015 at 10:03:46PM -0600, Stephen Warren wrote:

> 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>
> Acked-by: Lukasz Majewski <l.majewski@samsung.com>
> Tested-by: Lukasz Majewski <l.majewski@samsung.com>

Applied to u-boot/master, thanks!