Re: [U-Boot] [PATCH v5 2/9] dfu: Support larger than memory transfers.

Hi Tom,

...

From: Pantelis Antoniou panto@antoniou-consulting.com

Previously we didn't support upload/download larger than available memory. This is pretty bad when you have to update your root filesystem for example.

This patch removes that limitation (and the crashes when you transfered any file larger than 4MB) by making raw image writes be done in chunks and making file maximum size be configurable.

The sequence number is a 16 bit counter; make sure we handle rollover correctly. This fixes the wrong transfers for large (> 256MB) images.

Also utilize a variable to handle initialization, so that we don't rely on just the counter sent by the host.

Signed-off-by: Pantelis Antoniou panto@antoniou-consulting.com Signed-off-by: Tom Rini trini@ti.com

Acked-by: Lukasz Majewski l.majewski@samsung.com

Test-hw: Exynos 4210 (Trats)

Tested-by: Lukasz Majewski l.majewski@samsung.com

Sorry but, I've found a regression for reading image from a file system. It happens with EXT4 mmc read (like uImage).

mmc_file_op: ext4load mmc 0:2 /uImage 0x0 0 0x7f95dc98 ** File not found 0x0 ** dfu: Read error!

ext4load params: ext4load - load binary file from a Ext4 filesystem

Usage: ext4load <interface> <dev[:part]> [addr] [filename] [bytes] - load binary file 'filename' from 'dev' on 'interface' to address 'addr' from ext4 filesystem. All numeric parameters are assumed to be hex.

Some parameters are wrong (buffer - 0x0) and some are switched (address and filename).

Please find more details below:

...

---

Changes in v5:

• Rework Pantelis' "dfu: Support larger than memory transfers" to

not break filesystem writing

Changes in v4: None Changes in v3: None Changes in v2: None

README | 7 ++ drivers/dfu/dfu.c | 245 ++++++++++++++++++++++++++++++++++++++----------- drivers/dfu/dfu_mmc.c | 116 +++++++++++++++++------ include/dfu.h | 26 +++++- 4 files changed, 310 insertions(+), 84 deletions(-)

...

+static int dfu_write_buffer_drain(struct dfu_entity *dfu) +{

• long w_size;
• int ret;
• /* flush size? */
• w_size = dfu->i_buf - dfu->i_buf_start;
• if (w_size == 0)

• return 0;
  

• /* update CRC32 */
• dfu->crc = crc32(dfu->crc, dfu->i_buf_start, w_size);
• ret = dfu->write_medium(dfu, dfu->offset, dfu->i_buf_start,

&w_size);

• if (ret)

• debug("%s: Write error!\n", __func__);
  

• /* point back */
• dfu->i_buf = dfu->i_buf_start;
• /* update offset */
• dfu->offset += w_size;
• puts("#");
• return ret;

+}

int dfu_write(struct dfu_entity *dfu, void *buf, int size, int blk_seq_num) {

• static unsigned char *i_buf;
• static int i_blk_seq_num;
• long w_size = 0; int ret = 0;

• int tret;
• debug("%s: name: %s buf: 0x%p size: 0x%x p_num: 0x%x "

• 	"offset: 0x%llx bufoffset: 0x%x\n",
  

•       __func__, dfu->name, buf, size, blk_seq_num,
  

dfu->offset,

•       dfu->i_buf - dfu->i_buf_start);
  

• if (!dfu->inited) {

• /* initial state */
  

• dfu->crc = 0;
  

• dfu->offset = 0;
  

• dfu->i_blk_seq_num = 0;
  

• dfu->i_buf_start = dfu_buf;
  

• dfu->i_buf_end = dfu_buf + sizeof(dfu_buf);
  

• dfu->i_buf = dfu->i_buf_start;
  

• dfu->inited = 1;
  

• }

• 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, i_buf);
  

• if (dfu->i_blk_seq_num != blk_seq_num) {

• printf("%s: Wrong sequence number! [%d] [%d]\n",
  

•        __func__, dfu->i_blk_seq_num, blk_seq_num);
  

• return -1;
  

• }

• if (blk_seq_num == 0) {

• i_buf = dfu_buf;
  

• i_blk_seq_num = 0;
  

• /* 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."

• *
  

• * This means that it's a 16 bit counter that roll-overs at
  

• * 0xffff -> 0x0000. By having a typical 4K transfer block
  

• * we roll-over at exactly 256MB. Not very fun to debug.
  

• *
  

• * Handling rollover, and having an inited variable,
  

• * makes things work.
  

• */
  

• /* 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) {

• tret = dfu_write_buffer_drain(dfu);
  

• if (ret == 0)
  

• 	ret = tret;
  

  }

• if (i_blk_seq_num++ != blk_seq_num) {

• printf("%s: Wrong sequence number! [%d] [%d]\n",
  

•        __func__, i_blk_seq_num, blk_seq_num);
  

• /* 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] - %d\n",
  

•        __func__, dfu->i_blk_seq_num, blk_seq_num,
  

size); return -1; }

• memcpy(i_buf, buf, size);
• i_buf += size;

• memcpy(dfu->i_buf, buf, size);

• dfu->i_buf += size;

• /* if end or if buffer full flush */

• if (size == 0 || (dfu->i_buf + size) > dfu->i_buf_end) {

• tret = dfu_write_buffer_drain(dfu);
  

• if (ret == 0)
  

• 	ret = tret;
  

• }

• /* end? */ if (size == 0) {

• /* Integrity check (if needed) */
  

• debug("%s: %s %d [B] CRC32: 0x%x\n", __func__,
  

dfu->name,

•        i_buf - dfu_buf, crc32(0, dfu_buf, i_buf -
  

dfu_buf));

• /* Now try and flush to the medium if needed. */
  

• if (dfu->flush_medium)
  

• 	ret = dfu->flush_medium(dfu);
  

• printf("\nDFU complete CRC32: 0x%08x\n", dfu->crc);
  
  

• w_size = i_buf - dfu_buf;
  

• ret = dfu->write_medium(dfu, dfu_buf, &w_size);
  

• if (ret)
  

• 	debug("%s: Write error!\n", __func__);
  

• /* clear everything */
  

• dfu->crc = 0;
  

• dfu->offset = 0;
  

• dfu->i_blk_seq_num = 0;
  

• dfu->i_buf_start = dfu_buf;
  

• dfu->i_buf_end = dfu_buf + sizeof(dfu_buf);
  

• dfu->i_buf = dfu->i_buf_start;
  

• dfu->inited = 0;
  
  

• i_blk_seq_num = 0;
  

• i_buf = NULL;
  

• return ret;
  

  }

• return ret;

• return ret = 0 ? size : ret;

+}

+static int dfu_read_buffer_fill(struct dfu_entity *dfu, void *buf, int size) +{

• long chunk;
• int ret, readn;
• readn = 0;
• while (size > 0) {

• /* get chunk that can be read */
  

• chunk = min(size, dfu->b_left);
  

• /* consume */
  

• if (chunk > 0) {
  

• 	memcpy(buf, dfu->i_buf, chunk);
  

• 	dfu->crc = crc32(dfu->crc, buf, chunk);
  

• 	dfu->i_buf += chunk;
  

• 	dfu->b_left -= chunk;
  

• 	size -= chunk;
  

• 	buf += chunk;
  

• 	readn += chunk;
  

• }
  

• /* all done */
  

• if (size > 0) {
  

• 	/* no more to read */
  

• 	if (dfu->r_left == 0)
  

• 		break;
  

• 	dfu->i_buf = dfu->i_buf_start;
  

• 	dfu->b_left = dfu->i_buf_end -
  

dfu->i_buf_start; +

• 	/* got to read, but buffer is empty */
  

• 	if (dfu->b_left > dfu->r_left)
  

• 		dfu->b_left = dfu->r_left;
  

• 	ret = dfu->read_medium(dfu, dfu->offset,
  

dfu->i_buf,

• 			&dfu->b_left);
  

• 	if (ret != 0) {
  

• 		debug("%s: Read error!\n",
  

__func__);

• 		return ret;
  

• 	}
  

• 	dfu->offset += dfu->b_left;
  

• 	dfu->r_left -= dfu->b_left;
  

• 	puts("#");
  

• }
  

• }
• return readn;

}

int dfu_read(struct dfu_entity *dfu, void *buf, int size, int blk_seq_num) {

• static unsigned char *i_buf;

• static int i_blk_seq_num;

• static long r_size;

• static u32 crc; int ret = 0;

  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, i_buf);
  

• if (blk_seq_num == 0) {

• i_buf = dfu_buf;
  

• ret = dfu->read_medium(dfu, i_buf, &r_size);
  

• debug("%s: %s %ld [B]\n", __func__, dfu->name,
  

r_size);

• i_blk_seq_num = 0;
  

• /* Integrity check (if needed) */
  

• crc = crc32(0, dfu_buf, r_size);
  

•       __func__, dfu->name, buf, size, blk_seq_num,
  

dfu->i_buf); +

• if (!dfu->inited) {

• ret = dfu->read_medium(dfu, 0, NULL, &dfu->r_left);
  

^^^^^^^^^^^^ this call causes read error. I suppose, that it is an initial "read". Does it read the whole file at once?

The problem is that the command is fromatted in a wrong way.

On the other hand write operations works on Trats.