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

[Pantelis Antoniou]

Hi Lukasz,

On Nov 29, 2012, at 11:06 AM, Lukasz Majewski wrote:

> Hi Pantelis,
> 
>> The sequence number is a 16 bit counter; make sure we
>> handle rollover correctly. This fixes the wrong transfers for
>> large (> 256MB) images.
> 
> This is a bit misleading comment. 
> DFU 1.1 standard says:
> "The wBlockNum field is a block sequence number. It increments each time
> a block is transferred, wrapping to zero from 65,535. It is used to
> provide useful context to the DFU loader in the device."
> 
> Maybe above statement shall be put to the comment.
> 

No problem there.

> One more thing:
> 
> wTransferSize = 4096 and the DFU 1.1 standard says:
> 
> "The host sends between bMaxPacketSize0 and wTransferSize" and
> increments wBlockNum (which is 16 bits).
> 
> Maybe we shall increase the wTransferSize (defined at 
> static const struct dfu_function_descriptor dfu_func) to e.g. 32 KiB
> and leave the code unchanged?
> 
> Can you test this option on your setup?
> 

No. The code as it stands, it's just broken. You will only delay the inevitable,
i.e. the rollover that is not handled correctly.

This is just bandaid, and is not fixing the problem.

Regards

-- Pantelis

>> 
>> 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);
>> +		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
>> };
>> 
>> int dfu_config_entities(char *s, char *interface, int num);
> 
> 
> 
> -- 
> Best regards,
> 
> Lukasz Majewski
> 
> Samsung Poland R&D Center | Linux Platform Group