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

[Lukasz Majewski]

Hi Tom,


> 
> > From: Pantelis Antoniou <panto at 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 at antoniou-consulting.com>
> > Signed-off-by: Tom Rini <trini at ti.com>
> 
> Acked-by: Lukasz Majewski <l.majewski at samsung.com>
> 
> Test-hw: Exynos 4210 (Trats)
> 
> Tested-by: Lukasz Majewski <l.majewski at 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. 


-- 
Best regards,

Lukasz Majewski

Samsung R&D Poland (SRPOL) | Linux Platform Group