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

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