[U-Boot] [PATCH V2 2/2] dfu: fix some issues with reads/uploads

[Lukasz Majewski <l.majewski@samsung.com>]

Hi Stephen,

> From: Stephen Warren <swarren@nvidia.com>
> 
> DFU read support appears to rely upon dfu->read_medium() updating the
> passed-by-reference len parameter to indicate the remaining size
> available for reading.
> 
> dfu_read_medium_mmc() never does this, and the implementation of
> dfu_read_medium_nand() will only work if called just once; it
> hard-codes the value to the total size of the NAND device
> irrespective of read offset.
> 
> I believe that overloading dfu->read_medium() is confusing. As such,
> this patch introduces a new function dfu->get_medium_size() which can
> be used to explicitly find out the medium size, and nothing else.
> dfu_read() is modified to use this function to set the initial value
> for dfu->r_left, rather than attempting to use the side-effects of
> dfu->read_medium() for this purpose.
> 
> Due to this change, dfu_read() must initially set dfu->b_left to 0,
> since no data has been read.
> 
> dfu_read_buffer_fill() must also be modified not to adjust dfu->r_left
> when simply copying data from dfu->i_buf_start to the upload request
> buffer. r_left represents the amount of data left to be read from HW.
> That value is not affected by the memcpy(), but only by calls to
> dfu->read_medium().
> 
> After this change, I can read from either a 4MB or 1.5MB chunk of a
> 4MB eMMC boot partion with CONFIG_SYS_DFU_DATA_BUF_SIZE==1MB. Without
> this change, attempting to do that would result in DFU read returning
> no data at all due to r_left never being set.
> 
> Signed-off-by: Stephen Warren <swarren@nvidia.com>

Stephen thanks for the patch.

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

Test HW: Trats2 (Exynos4412):
Tested-by: Lukasz Majewski <l.majewski@samsung.com>

I will pull this patch to -dfu tree when I receive Tom's and Marek's
responde to the first patch in this series.

> ---
> v2:
> * Fix dfu_get_medium_size_mmc() to handle DFU_FS_FAT/EXT5 layouts too,
>   by calling the recently introduced "size" command.
> ---
>  drivers/dfu/dfu.c      | 20 ++++++++++++-----
>  drivers/dfu/dfu_mmc.c  | 59
> ++++++++++++++++++++++++++++++++++++++++++--------
> drivers/dfu/dfu_nand.c |  7 +++++- drivers/dfu/dfu_ram.c  | 11
> +++++----- include/dfu.h          |  3 +++
>  5 files changed, 79 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c
> index dc09ff6466e6..dab5e7048ed5 100644
> --- a/drivers/dfu/dfu.c
> +++ b/drivers/dfu/dfu.c
> @@ -267,7 +267,6 @@ static int dfu_read_buffer_fill(struct dfu_entity
> *dfu, void *buf, int size) 
>  			dfu->i_buf += chunk;
>  			dfu->b_left -= chunk;
> -			dfu->r_left -= chunk;
>  			size -= chunk;
>  			buf += chunk;
>  			readn += chunk;
> @@ -313,10 +312,19 @@ int dfu_read(struct dfu_entity *dfu, void *buf,
> int size, int blk_seq_num) if (dfu->i_buf_start == NULL)
>  			return -ENOMEM;
>  
> -		ret = dfu->read_medium(dfu, 0, dfu->i_buf_start,
> &dfu->r_left);
> -		if (ret != 0) {
> -			debug("%s: failed to get r_left\n",
> __func__);
> -			return ret;
> +		dfu->r_left = dfu->get_medium_size(dfu);
> +		if (dfu->r_left < 0)
> +			return dfu->r_left;
> +		switch (dfu->layout) {
> +		case DFU_RAW_ADDR:
> +		case DFU_RAM_ADDR:
> +			break;
> +		default:
> +			if (dfu->r_left >= dfu_buf_size) {
> +				printf("%s: File too big for
> buffer\n",
> +				       __func__);
> +				return -EOVERFLOW;
> +			}
>  		}
>  
>  		debug("%s: %s %ld [B]\n", __func__, dfu->name,
> dfu->r_left); @@ -326,7 +334,7 @@ int dfu_read(struct dfu_entity
> *dfu, void *buf, int size, int blk_seq_num) dfu->offset = 0;
>  		dfu->i_buf_end = dfu_get_buf() + dfu_buf_size;
>  		dfu->i_buf = dfu->i_buf_start;
> -		dfu->b_left = min(dfu_buf_size, dfu->r_left);
> +		dfu->b_left = 0;
>  
>  		dfu->bad_skip = 0;
>  
> diff --git a/drivers/dfu/dfu_mmc.c b/drivers/dfu/dfu_mmc.c
> index 63cc876612c9..322bd8c5d2de 100644
> --- a/drivers/dfu/dfu_mmc.c
> +++ b/drivers/dfu/dfu_mmc.c
> @@ -12,6 +12,8 @@
>  #include <errno.h>
>  #include <div64.h>
>  #include <dfu.h>
> +#include <ext4fs.h>
> +#include <fat.h>
>  #include <mmc.h>
>  
>  static unsigned char __aligned(CONFIG_SYS_CACHELINE_SIZE)
> @@ -113,22 +115,17 @@ static int mmc_file_buffer(struct dfu_entity
> *dfu, void *buf, long *len) static int mmc_file_op(enum dfu_op op,
> struct dfu_entity *dfu, void *buf, long *len)
>  {
> +	const char *fsname, *opname;
>  	char cmd_buf[DFU_CMD_BUF_SIZE];
>  	char *str_env;
>  	int ret;
>  
>  	switch (dfu->layout) {
>  	case DFU_FS_FAT:
> -		sprintf(cmd_buf, "fat%s mmc %d:%d 0x%x %s",
> -			op == DFU_OP_READ ? "load" : "write",
> -			dfu->data.mmc.dev, dfu->data.mmc.part,
> -			(unsigned int) buf, dfu->name);
> +		fsname = "fat";
>  		break;
>  	case DFU_FS_EXT4:
> -		sprintf(cmd_buf, "ext4%s mmc %d:%d 0x%x /%s",
> -			op == DFU_OP_READ ? "load" : "write",
> -			dfu->data.mmc.dev, dfu->data.mmc.part,
> -			(unsigned int) buf, dfu->name);
> +		fsname = "ext4";
>  		break;
>  	default:
>  		printf("%s: Layout (%s) not (yet) supported!\n",
> __func__, @@ -136,6 +133,28 @@ static int mmc_file_op(enum dfu_op op,
> struct dfu_entity *dfu, return -1;
>  	}
>  
> +	switch (op) {
> +	case DFU_OP_READ:
> +		opname = "load";
> +		break;
> +	case DFU_OP_WRITE:
> +		opname = "write";
> +		break;
> +	case DFU_OP_SIZE:
> +		opname = "size";
> +		break;
> +	default:
> +		return -1;
> +	}
> +
> +	sprintf(cmd_buf, "%s%s mmc %d:%d", fsname, opname,
> +		dfu->data.mmc.dev, dfu->data.mmc.part);
> +
> +	if (op != DFU_OP_SIZE)
> +		sprintf(cmd_buf + strlen(cmd_buf), " 0x%x",
> (unsigned int)buf); +
> +	sprintf(cmd_buf + strlen(cmd_buf), " %s", dfu->name);
> +
>  	if (op == DFU_OP_WRITE)
>  		sprintf(cmd_buf + strlen(cmd_buf), " %lx", *len);
>  
> @@ -147,7 +166,7 @@ static int mmc_file_op(enum dfu_op op, struct
> dfu_entity *dfu, return ret;
>  	}
>  
> -	if (dfu->layout != DFU_RAW_ADDR && op == DFU_OP_READ) {
> +	if (op != DFU_OP_WRITE) {
>  		str_env = getenv("filesize");
>  		if (str_env == NULL) {
>  			puts("dfu: Wrong file size!\n");
> @@ -196,6 +215,27 @@ int dfu_flush_medium_mmc(struct dfu_entity *dfu)
>  	return ret;
>  }
>  
> +long dfu_get_medium_size_mmc(struct dfu_entity *dfu)
> +{
> +	int ret;
> +	long len;
> +
> +	switch (dfu->layout) {
> +	case DFU_RAW_ADDR:
> +		return dfu->data.mmc.lba_size *
> dfu->data.mmc.lba_blk_size;
> +	case DFU_FS_FAT:
> +	case DFU_FS_EXT4:
> +		ret = mmc_file_op(DFU_OP_SIZE, dfu, NULL, &len);
> +		if (ret < 0)
> +			return ret;
> +		return len;
> +	default:
> +		printf("%s: Layout (%s) not (yet) supported!\n",
> __func__,
> +		       dfu_get_layout(dfu->layout));
> +		return -1;
> +	}
> +}
> +
>  int dfu_read_medium_mmc(struct dfu_entity *dfu, u64 offset, void
> *buf, long *len)
>  {
> @@ -316,6 +356,7 @@ int dfu_fill_entity_mmc(struct dfu_entity *dfu,
> char *s) }
>  
>  	dfu->dev_type = DFU_DEV_MMC;
> +	dfu->get_medium_size = dfu_get_medium_size_mmc;
>  	dfu->read_medium = dfu_read_medium_mmc;
>  	dfu->write_medium = dfu_write_medium_mmc;
>  	dfu->flush_medium = dfu_flush_medium_mmc;
> diff --git a/drivers/dfu/dfu_nand.c b/drivers/dfu/dfu_nand.c
> index ccdbef6b75f2..e1c9a8849246 100644
> --- a/drivers/dfu/dfu_nand.c
> +++ b/drivers/dfu/dfu_nand.c
> @@ -114,6 +114,11 @@ static int dfu_write_medium_nand(struct
> dfu_entity *dfu, return ret;
>  }
>  
> +long dfu_get_medium_size_nand(struct dfu_entity *dfu)
> +{
> +	return dfu->data.nand.size;
> +}
> +
>  static int dfu_read_medium_nand(struct dfu_entity *dfu, u64 offset,
> void *buf, long *len)
>  {
> @@ -121,7 +126,6 @@ static int dfu_read_medium_nand(struct dfu_entity
> *dfu, u64 offset, void *buf, 
>  	switch (dfu->layout) {
>  	case DFU_RAW_ADDR:
> -		*len = dfu->data.nand.size;
>  		ret = nand_block_read(dfu, offset, buf, len);
>  		break;
>  	default:
> @@ -220,6 +224,7 @@ int dfu_fill_entity_nand(struct dfu_entity *dfu,
> char *s) return -1;
>  	}
>  
> +	dfu->get_medium_size = dfu_get_medium_size_nand;
>  	dfu->read_medium = dfu_read_medium_nand;
>  	dfu->write_medium = dfu_write_medium_nand;
>  	dfu->flush_medium = dfu_flush_medium_nand;
> diff --git a/drivers/dfu/dfu_ram.c b/drivers/dfu/dfu_ram.c
> index 335a8e1f2491..b6c6e60c443c 100644
> --- a/drivers/dfu/dfu_ram.c
> +++ b/drivers/dfu/dfu_ram.c
> @@ -41,14 +41,14 @@ static int dfu_write_medium_ram(struct dfu_entity
> *dfu, u64 offset, return dfu_transfer_medium_ram(DFU_OP_WRITE, dfu,
> offset, buf, len); }
>  
> +long dfu_get_medium_size_ram(struct dfu_entity *dfu)
> +{
> +	return dfu->data.ram.size;
> +}
> +
>  static int dfu_read_medium_ram(struct dfu_entity *dfu, u64 offset,
>  			       void *buf, long *len)
>  {
> -	if (!*len) {
> -		*len = dfu->data.ram.size;
> -		return 0;
> -	}
> -
>  	return dfu_transfer_medium_ram(DFU_OP_READ, dfu, offset,
> buf, len); }
>  
> @@ -69,6 +69,7 @@ int dfu_fill_entity_ram(struct dfu_entity *dfu,
> char *s) dfu->data.ram.size = simple_strtoul(s, &s, 16);
>  
>  	dfu->write_medium = dfu_write_medium_ram;
> +	dfu->get_medium_size = dfu_get_medium_size_ram;
>  	dfu->read_medium = dfu_read_medium_ram;
>  
>  	dfu->inited = 0;
> diff --git a/include/dfu.h b/include/dfu.h
> index 26ffbc8e81d2..df720310f2cc 100644
> --- a/include/dfu.h
> +++ b/include/dfu.h
> @@ -35,6 +35,7 @@ enum dfu_layout {
>  enum dfu_op {
>  	DFU_OP_READ = 1,
>  	DFU_OP_WRITE,
> +	DFU_OP_SIZE,
>  };
>  
>  struct mmc_internal_data {
> @@ -96,6 +97,8 @@ struct dfu_entity {
>  		struct ram_internal_data ram;
>  	} data;
>  
> +	long (*get_medium_size)(struct dfu_entity *dfu);
> +
>  	int (*read_medium)(struct dfu_entity *dfu,
>  			u64 offset, void *buf, long *len);
>  



-- 
Best regards,

Lukasz Majewski

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