| Message ID | 1354142330-31130-1-git-send-email-panto@antoniou-consulting.com |
|---|---|
| State | Superseded |
| Delegated to: | Marek Vasut |
| Headers | show |
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; > + } > + [...]
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);
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
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);
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);
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(-)