[U-Boot] [PATCH] dfu: Handle large transfers correctly

[Lukasz Majewski]

Hi Pantelis,

Please see below comments.

Moreover this patch doesn't solve the problem of bricking trats board
after flashing it with dfu after applying:

[U-Boot] [PATCH] dfu: Handle large transfers correctly

on top of:
[U-Boot] [PATCH 00/10] USB: Gadget & DFU related fixes

Without your patches it works, so thorough examination is needed.

I will have time to do that after weekend.

> The sequence number is a 16 bit counter; make sure we
> handle rollover correctly. This fixes the wrong transfers for
> large (> 256MB) images.
> 
> Signed-off-by: Pantelis Antoniou <panto at antoniou-consulting.com>
> ---
>  drivers/dfu/dfu.c     | 28 +++++++++++++++++++++++-----
>  drivers/dfu/dfu_mmc.c |  3 +++
>  include/dfu.h         |  2 ++
>  3 files changed, 28 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c
> index 1260c55..2483018 100644
> --- a/drivers/dfu/dfu.c
> +++ b/drivers/dfu/dfu.c
> @@ -82,7 +82,7 @@ int dfu_write(struct dfu_entity *dfu, void *buf,
> int size, int blk_seq_num) __func__, dfu->name, buf, size,
> blk_seq_num, dfu->offset, dfu->i_buf - dfu->i_buf_start);
>  
> -	if (blk_seq_num == 0) {
> +	if (dfu->do_init) {
>  		/* initial state */
>  		dfu->crc = 0;
>  		dfu->offset = 0;
> @@ -90,6 +90,8 @@ int dfu_write(struct dfu_entity *dfu, void *buf,
> int size, int blk_seq_num) dfu->i_buf_start = dfu_buf;
>  		dfu->i_buf_end = dfu_buf + sizeof(dfu_buf);
>  		dfu->i_buf = dfu->i_buf_start;
> +
> +		dfu->do_init = 0;
>  	}
>  
>  	if (dfu->i_blk_seq_num != blk_seq_num) {
> @@ -97,7 +99,8 @@ int dfu_write(struct dfu_entity *dfu, void *buf,
> int size, int blk_seq_num) __func__, dfu->i_blk_seq_num, blk_seq_num);
>  		return -1;
>  	}
> -	dfu->i_blk_seq_num++;
> +	/* handle rollover */
> +	dfu->i_blk_seq_num = (dfu->i_blk_seq_num + 1) & 0xffff;
>  
>  	/* flush buffer if overflow */
>  	if ((dfu->i_buf + size) > dfu->i_buf_end) {
> @@ -106,6 +109,13 @@ int dfu_write(struct dfu_entity *dfu, void *buf,
> int size, int blk_seq_num) ret = tret;
>  	}
>  
> +	/* we should be in buffer now (if not then size too large) */
> +	if ((dfu->i_buf + size) > dfu->i_buf_end) {
> +		printf("%s: Wrong size! [%d] [%d] - \n",
> +		       __func__, dfu->i_blk_seq_num, blk_seq_num,
> size);

Missing %d for size parameter.

> +		return -1;
> +	}
> +
>  	memcpy(dfu->i_buf, buf, size);
>  	dfu->i_buf += size;
>  
> @@ -120,7 +130,7 @@ int dfu_write(struct dfu_entity *dfu, void *buf,
> int size, int blk_seq_num) if (size == 0) {
>  		debug("%s: DFU complete CRC32: 0x%x\n", __func__,
> dfu->crc); 
> -		puts("\nDownload complete\n");
> +		printf("\nDownload complete (CRC32 0x%04x)\n",
> dfu->crc); 
>  		/* clear everything */
>  		dfu->crc = 0;
> @@ -129,6 +139,9 @@ int dfu_write(struct dfu_entity *dfu, void *buf,
> int size, int blk_seq_num) dfu->i_buf_start = dfu_buf;
>  		dfu->i_buf_end = dfu_buf + sizeof(dfu_buf);
>  		dfu->i_buf = dfu->i_buf_start;
> +
> +		dfu->do_init = 1;
> +
>  	}
>  
>  	return ret = 0 ? size : ret;
> @@ -190,7 +203,7 @@ int dfu_read(struct dfu_entity *dfu, void *buf,
> int size, int blk_seq_num) debug("%s: name: %s buf: 0x%p size: 0x%x
> p_num: 0x%x i_buf: 0x%p\n", __func__, dfu->name, buf, size,
> blk_seq_num, dfu->i_buf); 
> -	if (blk_seq_num == 0) {
> +	if (dfu->do_init) {
>  		ret = dfu->read_medium(dfu, 0, NULL, &dfu->r_left);
>  		if (ret != 0) {
>  			debug("%s: failed to get r_left\n",
> __func__); @@ -206,6 +219,8 @@ int dfu_read(struct dfu_entity *dfu,
> void *buf, int size, int blk_seq_num) dfu->i_buf_end = dfu_buf +
> sizeof(dfu_buf); dfu->i_buf = dfu->i_buf_start;
>  		dfu->b_left = 0;
> +
> +		dfu->do_init = 0;
>  	}
>  
>  	if (dfu->i_blk_seq_num != blk_seq_num) {
> @@ -213,7 +228,8 @@ int dfu_read(struct dfu_entity *dfu, void *buf,
> int size, int blk_seq_num) __func__, dfu->i_blk_seq_num, blk_seq_num);
>  		return -1;
>  	}
> -	dfu->i_blk_seq_num++;
> +	/* handle rollover */
> +	dfu->i_blk_seq_num = (dfu->i_blk_seq_num + 1) & 0xffff;
>  
>  	ret = dfu_read_buffer_fill(dfu, buf, size);
>  	if (ret < 0) {
> @@ -232,6 +248,8 @@ int dfu_read(struct dfu_entity *dfu, void *buf,
> int size, int blk_seq_num) dfu->i_buf_end = dfu_buf + sizeof(dfu_buf);
>  		dfu->i_buf = dfu->i_buf_start;
>  		dfu->b_left = 0;
> +
> +		dfu->do_init = 1;
>  	}
>  
>  	return ret;
> diff --git a/drivers/dfu/dfu_mmc.c b/drivers/dfu/dfu_mmc.c
> index 4c8994b..906df55 100644
> --- a/drivers/dfu/dfu_mmc.c
> +++ b/drivers/dfu/dfu_mmc.c
> @@ -234,5 +234,8 @@ int dfu_fill_entity_mmc(struct dfu_entity *dfu,
> char *s) dfu->read_medium = dfu_read_medium_mmc;
>  	dfu->write_medium = dfu_write_medium_mmc;
>  
> +	/* initial state */
> +	dfu->do_init = 1;
> +
>  	return 0;
>  }
> diff --git a/include/dfu.h b/include/dfu.h
> index b662488..0ce91b7 100644
> --- a/include/dfu.h
> +++ b/include/dfu.h
> @@ -90,6 +90,8 @@ struct dfu_entity {
>  	u8 *i_buf_end;
>  	long r_left;
>  	long b_left;
> +
> +	unsigned int do_init : 1
                                 ^^^Missing semicolon ....

>  };
>  
>  int dfu_config_entities(char *s, char *interface, int num);



-- 
Best regards,

Lukasz Majewski

Samsung Poland R&D Center | Linux Platform Group