* [U-Boot] [PATCH] dfu: Handle large transfers correctly
@ 2012-11-28 22:38 Pantelis Antoniou
2012-11-29 6:26 ` Marek Vasut
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Pantelis Antoniou @ 2012-11-28 22:38 UTC (permalink / raw)
To: u-boot
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@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);
--
1.7.12
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH] dfu: Handle large transfers correctly
2012-11-28 22:38 [U-Boot] [PATCH] dfu: Handle large transfers correctly Pantelis Antoniou
@ 2012-11-29 6:26 ` Marek Vasut
2012-11-29 9:06 ` Lukasz Majewski
2012-11-29 10:25 ` Lukasz Majewski
2 siblings, 0 replies; 5+ messages in thread
From: Marek Vasut @ 2012-11-29 6:26 UTC (permalink / raw)
To: u-boot
Dear Pantelis Antoniou,
> 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@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) {
If you were to change this to dfu->inited, I think it'd be a bit more intuitive
;)
> /* 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) */
I can't make much sense from this comment :(
> + if ((dfu->i_buf + size) > dfu->i_buf_end) {
> + printf("%s: Wrong size! [%d] [%d] - \n",
And you should really make this output a bit more human readable please.
> + __func__, dfu->i_blk_seq_num, blk_seq_num, size);
> + return -1;
> + }
> +
[...]
^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH] dfu: Handle large transfers correctly
2012-11-28 22:38 [U-Boot] [PATCH] dfu: Handle large transfers correctly Pantelis Antoniou
2012-11-29 6:26 ` Marek Vasut
@ 2012-11-29 9:06 ` Lukasz Majewski
2012-11-29 9:35 ` Pantelis Antoniou
2012-11-29 10:25 ` Lukasz Majewski
2 siblings, 1 reply; 5+ messages in thread
From: Lukasz Majewski @ 2012-11-29 9:06 UTC (permalink / raw)
To: u-boot
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.
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?
>
> Signed-off-by: Pantelis Antoniou <panto@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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH] dfu: Handle large transfers correctly
2012-11-29 9:06 ` Lukasz Majewski
@ 2012-11-29 9:35 ` Pantelis Antoniou
0 siblings, 0 replies; 5+ messages in thread
From: Pantelis Antoniou @ 2012-11-29 9:35 UTC (permalink / raw)
To: u-boot
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@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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH] dfu: Handle large transfers correctly
2012-11-28 22:38 [U-Boot] [PATCH] dfu: Handle large transfers correctly Pantelis Antoniou
2012-11-29 6:26 ` Marek Vasut
2012-11-29 9:06 ` Lukasz Majewski
@ 2012-11-29 10:25 ` Lukasz Majewski
2 siblings, 0 replies; 5+ messages in thread
From: Lukasz Majewski @ 2012-11-29 10:25 UTC (permalink / raw)
To: u-boot
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@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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-11-29 10:25 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-11-28 22:38 [U-Boot] [PATCH] dfu: Handle large transfers correctly Pantelis Antoniou
2012-11-29 6:26 ` Marek Vasut
2012-11-29 9:06 ` Lukasz Majewski
2012-11-29 9:35 ` Pantelis Antoniou
2012-11-29 10:25 ` Lukasz Majewski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox