[U-Boot] [PATCH 0/4] dfu: remove limitation on partition size
I discover some limitation on DFU stack when I play on my ARM board with DFU for my external SDCARD : 16GB
It is mainly caused by the 'long' type (32 bits on ARMv7) used for size in dfu.c
I solve the issue with the 2 first patches of this serie - manage size and result correctly in get_medium_size() - change size to u64
The 2 next patch of the serie are not directly related to the issue. It is a cleanup and code factorization of dfu.c code
Patrick Delaunay (4): dfu: allow dfu read on partition greater than 2GB dfu: remove limitation on partition size dfu: factorize transaction cleanup dfu: add common function to initiate transaction
drivers/dfu/dfu.c | 100 ++++++++++++++++++++++--------------------------- drivers/dfu/dfu_mmc.c | 20 +++++----- drivers/dfu/dfu_nand.c | 6 ++- drivers/dfu/dfu_ram.c | 6 ++- drivers/dfu/dfu_sf.c | 6 ++- include/dfu.h | 4 +- 6 files changed, 69 insertions(+), 73 deletions(-)
solve issue on get_medium_size() function the detection of error is a simple test < 0 but for ARM platform, long is 32bits and 2GB = 0x80000000 is seen as error.
I solve the issue by changing the prototype fo the function to separate size and result. This patch prepare the next patch with size change to u64.
Signed-off-by: Patrick Delaunay patrick.delaunay@st.com ---
drivers/dfu/dfu.c | 6 +++--- drivers/dfu/dfu_mmc.c | 12 ++++++------ drivers/dfu/dfu_nand.c | 6 ++++-- drivers/dfu/dfu_ram.c | 6 ++++-- drivers/dfu/dfu_sf.c | 6 ++++-- include/dfu.h | 2 +- 6 files changed, 22 insertions(+), 16 deletions(-)
diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c index ceb33e3..c777016 100644 --- a/drivers/dfu/dfu.c +++ b/drivers/dfu/dfu.c @@ -339,9 +339,9 @@ int dfu_read(struct dfu_entity *dfu, void *buf, int size, int blk_seq_num) if (dfu->i_buf_start == NULL) return -ENOMEM;
- dfu->r_left = dfu->get_medium_size(dfu); - if (dfu->r_left < 0) - return dfu->r_left; + ret = dfu->get_medium_size(dfu, &dfu->r_left); + if (ret < 0) + return ret;
debug("%s: %s %ld [B]\n", __func__, dfu->name, dfu->r_left);
diff --git a/drivers/dfu/dfu_mmc.c b/drivers/dfu/dfu_mmc.c index 926ccbd..d918f95 100644 --- a/drivers/dfu/dfu_mmc.c +++ b/drivers/dfu/dfu_mmc.c @@ -209,23 +209,23 @@ int dfu_flush_medium_mmc(struct dfu_entity *dfu) return ret; }
-long dfu_get_medium_size_mmc(struct dfu_entity *dfu) +int dfu_get_medium_size_mmc(struct dfu_entity *dfu, long *size) { int ret; - long len;
switch (dfu->layout) { case DFU_RAW_ADDR: - return dfu->data.mmc.lba_size * dfu->data.mmc.lba_blk_size; + *size = dfu->data.mmc.lba_size * dfu->data.mmc.lba_blk_size; + return 0; case DFU_FS_FAT: case DFU_FS_EXT4: dfu_file_buf_filled = -1; - ret = mmc_file_op(DFU_OP_SIZE, dfu, NULL, &len); + ret = mmc_file_op(DFU_OP_SIZE, dfu, NULL, size); if (ret < 0) return ret; - if (len > CONFIG_SYS_DFU_MAX_FILE_SIZE) + if (*size > CONFIG_SYS_DFU_MAX_FILE_SIZE) return -1; - return len; + return 0; default: printf("%s: Layout (%s) not (yet) supported!\n", __func__, dfu_get_layout(dfu->layout)); diff --git a/drivers/dfu/dfu_nand.c b/drivers/dfu/dfu_nand.c index 97cd608..ed4ceb7 100644 --- a/drivers/dfu/dfu_nand.c +++ b/drivers/dfu/dfu_nand.c @@ -114,9 +114,11 @@ static int dfu_write_medium_nand(struct dfu_entity *dfu, return ret; }
-long dfu_get_medium_size_nand(struct dfu_entity *dfu) +int dfu_get_medium_size_nand(struct dfu_entity *dfu, long *size) { - return dfu->data.nand.size; + *size = dfu->data.nand.size; + + return 0; }
static int dfu_read_medium_nand(struct dfu_entity *dfu, u64 offset, void *buf, diff --git a/drivers/dfu/dfu_ram.c b/drivers/dfu/dfu_ram.c index c1b0021..b95cb9f 100644 --- a/drivers/dfu/dfu_ram.c +++ b/drivers/dfu/dfu_ram.c @@ -41,9 +41,11 @@ 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) +int dfu_get_medium_size_ram(struct dfu_entity *dfu, long *size) { - return dfu->data.ram.size; + *size = dfu->data.ram.size; + + return 0; }
static int dfu_read_medium_ram(struct dfu_entity *dfu, u64 offset, diff --git a/drivers/dfu/dfu_sf.c b/drivers/dfu/dfu_sf.c index b6d5fe2..2b26f2c 100644 --- a/drivers/dfu/dfu_sf.c +++ b/drivers/dfu/dfu_sf.c @@ -12,9 +12,11 @@ #include <spi.h> #include <spi_flash.h>
-static long dfu_get_medium_size_sf(struct dfu_entity *dfu) +static int dfu_get_medium_size_sf(struct dfu_entity *dfu, long *size) { - return dfu->data.sf.size; + *size = dfu->data.sf.size; + + return 0; }
static int dfu_read_medium_sf(struct dfu_entity *dfu, u64 offset, void *buf, diff --git a/include/dfu.h b/include/dfu.h index f39d3f1..5b621b5 100644 --- a/include/dfu.h +++ b/include/dfu.h @@ -110,7 +110,7 @@ struct dfu_entity { struct sf_internal_data sf; } data;
- long (*get_medium_size)(struct dfu_entity *dfu); + int (*get_medium_size)(struct dfu_entity *dfu, long *size);
int (*read_medium)(struct dfu_entity *dfu, u64 offset, void *buf, long *len);
Change long (32 bits on arm) to u64 (same type than offset) for size and read offset r_left
So partition and device used for DFU can be greater than 4GB
Signed-off-by: Patrick Delaunay patrick.delaunay@st.com ---
drivers/dfu/dfu.c | 2 +- drivers/dfu/dfu_mmc.c | 10 +++++----- drivers/dfu/dfu_nand.c | 2 +- drivers/dfu/dfu_ram.c | 2 +- drivers/dfu/dfu_sf.c | 2 +- include/dfu.h | 4 ++-- 6 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c index c777016..0f1ab0d 100644 --- a/drivers/dfu/dfu.c +++ b/drivers/dfu/dfu.c @@ -343,7 +343,7 @@ int dfu_read(struct dfu_entity *dfu, void *buf, int size, int blk_seq_num) if (ret < 0) return ret;
- debug("%s: %s %ld [B]\n", __func__, dfu->name, dfu->r_left); + debug("%s: %s %lld [B]\n", __func__, dfu->name, dfu->r_left);
dfu->i_blk_seq_num = 0; dfu->crc = 0; diff --git a/drivers/dfu/dfu_mmc.c b/drivers/dfu/dfu_mmc.c index d918f95..bb23e7f 100644 --- a/drivers/dfu/dfu_mmc.c +++ b/drivers/dfu/dfu_mmc.c @@ -17,7 +17,7 @@ #include <mmc.h>
static unsigned char *dfu_file_buf; -static long dfu_file_buf_len; +static u64 dfu_file_buf_len; static long dfu_file_buf_filled;
static int mmc_block_op(enum dfu_op op, struct dfu_entity *dfu, @@ -107,7 +107,7 @@ 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) + void *buf, u64 *len) { const char *fsname, *opname; char cmd_buf[DFU_CMD_BUF_SIZE]; @@ -150,7 +150,7 @@ static int mmc_file_op(enum dfu_op op, struct dfu_entity *dfu, sprintf(cmd_buf + strlen(cmd_buf), " %s", dfu->name);
if (op == DFU_OP_WRITE) - sprintf(cmd_buf + strlen(cmd_buf), " %lx", *len); + sprintf(cmd_buf + strlen(cmd_buf), " %llx", *len);
debug("%s: %s 0x%p\n", __func__, cmd_buf, cmd_buf);
@@ -209,7 +209,7 @@ int dfu_flush_medium_mmc(struct dfu_entity *dfu) return ret; }
-int dfu_get_medium_size_mmc(struct dfu_entity *dfu, long *size) +int dfu_get_medium_size_mmc(struct dfu_entity *dfu, u64 *size) { int ret;
@@ -237,7 +237,7 @@ static int mmc_file_unbuffer(struct dfu_entity *dfu, u64 offset, void *buf, long *len) { int ret; - long file_len; + u64 file_len;
if (dfu_file_buf_filled == -1) { ret = mmc_file_op(DFU_OP_READ, dfu, dfu_file_buf, &file_len); diff --git a/drivers/dfu/dfu_nand.c b/drivers/dfu/dfu_nand.c index ed4ceb7..6dc9ff7 100644 --- a/drivers/dfu/dfu_nand.c +++ b/drivers/dfu/dfu_nand.c @@ -114,7 +114,7 @@ static int dfu_write_medium_nand(struct dfu_entity *dfu, return ret; }
-int dfu_get_medium_size_nand(struct dfu_entity *dfu, long *size) +int dfu_get_medium_size_nand(struct dfu_entity *dfu, u64 *size) { *size = dfu->data.nand.size;
diff --git a/drivers/dfu/dfu_ram.c b/drivers/dfu/dfu_ram.c index b95cb9f..6e3f531 100644 --- a/drivers/dfu/dfu_ram.c +++ b/drivers/dfu/dfu_ram.c @@ -41,7 +41,7 @@ static int dfu_write_medium_ram(struct dfu_entity *dfu, u64 offset, return dfu_transfer_medium_ram(DFU_OP_WRITE, dfu, offset, buf, len); }
-int dfu_get_medium_size_ram(struct dfu_entity *dfu, long *size) +int dfu_get_medium_size_ram(struct dfu_entity *dfu, u64 *size) { *size = dfu->data.ram.size;
diff --git a/drivers/dfu/dfu_sf.c b/drivers/dfu/dfu_sf.c index 2b26f2c..2d2586d 100644 --- a/drivers/dfu/dfu_sf.c +++ b/drivers/dfu/dfu_sf.c @@ -12,7 +12,7 @@ #include <spi.h> #include <spi_flash.h>
-static int dfu_get_medium_size_sf(struct dfu_entity *dfu, long *size) +static int dfu_get_medium_size_sf(struct dfu_entity *dfu, u64 *size) { *size = dfu->data.sf.size;
diff --git a/include/dfu.h b/include/dfu.h index 5b621b5..7e322d9 100644 --- a/include/dfu.h +++ b/include/dfu.h @@ -110,7 +110,7 @@ struct dfu_entity { struct sf_internal_data sf; } data;
- int (*get_medium_size)(struct dfu_entity *dfu, long *size); + int (*get_medium_size)(struct dfu_entity *dfu, u64 *size);
int (*read_medium)(struct dfu_entity *dfu, u64 offset, void *buf, long *len); @@ -132,7 +132,7 @@ struct dfu_entity { u8 *i_buf; u8 *i_buf_start; u8 *i_buf_end; - long r_left; + u64 r_left; long b_left;
u32 bad_skip; /* for nand use */
rename cleanup function, as now called by read
Signed-off-by: Patrick Delaunay patrick.delaunay@st.com ---
drivers/dfu/dfu.c | 28 +++++++++++----------------- 1 file changed, 11 insertions(+), 17 deletions(-)
diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c index 0f1ab0d..63a5a44 100644 --- a/drivers/dfu/dfu.c +++ b/drivers/dfu/dfu.c @@ -165,7 +165,7 @@ static int dfu_write_buffer_drain(struct dfu_entity *dfu) return ret; }
-void dfu_write_transaction_cleanup(struct dfu_entity *dfu) +void dfu_transaction_cleanup(struct dfu_entity *dfu) { /* clear everything */ dfu->crc = 0; @@ -174,6 +174,10 @@ void dfu_write_transaction_cleanup(struct dfu_entity *dfu) dfu->i_buf_start = dfu_buf; dfu->i_buf_end = dfu_buf; dfu->i_buf = dfu->i_buf_start; + dfu->r_left = 0; + dfu->b_left = 0; + dfu->bad_skip = 0; + dfu->inited = 0; }
@@ -192,7 +196,7 @@ int dfu_flush(struct dfu_entity *dfu, void *buf, int size, int blk_seq_num) printf("\nDFU complete %s: 0x%08x\n", dfu_hash_algo->name, dfu->crc);
- dfu_write_transaction_cleanup(dfu); + dfu_transaction_cleanup(dfu);
return ret; } @@ -223,7 +227,7 @@ int dfu_write(struct dfu_entity *dfu, void *buf, int size, int blk_seq_num) 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); - dfu_write_transaction_cleanup(dfu); + dfu_transaction_cleanup(dfu); return -1; }
@@ -247,7 +251,7 @@ int dfu_write(struct dfu_entity *dfu, void *buf, int size, int blk_seq_num) if ((dfu->i_buf + size) > dfu->i_buf_end) { ret = dfu_write_buffer_drain(dfu); if (ret) { - dfu_write_transaction_cleanup(dfu); + dfu_transaction_cleanup(dfu); return ret; } } @@ -256,7 +260,7 @@ int dfu_write(struct dfu_entity *dfu, void *buf, int size, int blk_seq_num) if ((dfu->i_buf + size) > dfu->i_buf_end) { error("Buffer overflow! (0x%p + 0x%x > 0x%p)\n", dfu->i_buf, size, dfu->i_buf_end); - dfu_write_transaction_cleanup(dfu); + dfu_transaction_cleanup(dfu); return -1; }
@@ -267,7 +271,7 @@ int dfu_write(struct dfu_entity *dfu, void *buf, int size, int blk_seq_num) if (size == 0 || (dfu->i_buf + size) > dfu->i_buf_end) { ret = dfu_write_buffer_drain(dfu); if (ret) { - dfu_write_transaction_cleanup(dfu); + dfu_transaction_cleanup(dfu); return ret; } } @@ -377,17 +381,7 @@ int dfu_read(struct dfu_entity *dfu, void *buf, int size, int blk_seq_num) dfu_hash_algo->name, dfu->crc); puts("\nUPLOAD ... done\nCtrl+C to exit ...\n");
- dfu->i_blk_seq_num = 0; - dfu->crc = 0; - dfu->offset = 0; - dfu->i_buf_start = dfu_buf; - dfu->i_buf_end = dfu_buf; - dfu->i_buf = dfu->i_buf_start; - dfu->b_left = 0; - - dfu->bad_skip = 0; - - dfu->inited = 0; + dfu_transaction_cleanup(dfu); }
return ret;
- factorize code between read and write transaction - always use dfu_transaction_cleanup() to initialize the internal variable: easy maintenance - replace direct access by dfu_get_buf() and dfu_get_buf_size()
Signed-off-by: Patrick Delaunay patrick.delaunay@st.com --- the boolean 'read' is only used to avoid r_left initialization with call get_medium_size()
So the patch don't change the current dynamic but for my point of view it should removed
drivers/dfu/dfu.c | 72 ++++++++++++++++++++++++++----------------------------- 1 file changed, 34 insertions(+), 38 deletions(-)
diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c index 63a5a44..f6281f4 100644 --- a/drivers/dfu/dfu.c +++ b/drivers/dfu/dfu.c @@ -171,8 +171,8 @@ void dfu_transaction_cleanup(struct dfu_entity *dfu) dfu->crc = 0; dfu->offset = 0; dfu->i_blk_seq_num = 0; - dfu->i_buf_start = dfu_buf; - dfu->i_buf_end = dfu_buf; + dfu->i_buf_start = dfu_get_buf(dfu); + dfu->i_buf_end = dfu->i_buf_start; dfu->i_buf = dfu->i_buf_start; dfu->r_left = 0; dfu->b_left = 0; @@ -181,6 +181,32 @@ void dfu_transaction_cleanup(struct dfu_entity *dfu) dfu->inited = 0; }
+int dfu_transaction_initiate(struct dfu_entity *dfu, bool read) +{ + int ret = 0; + + if (dfu->inited) + return 0; + + dfu_transaction_cleanup(dfu); + + if (dfu->i_buf_start == NULL) + return -ENOMEM; + + dfu->i_buf_end = dfu->i_buf_start + dfu_get_buf_size(); + + if (read) { + ret = dfu->get_medium_size(dfu, &dfu->r_left); + if (ret < 0) + return ret; + debug("%s: %s %lld [B]\n", __func__, dfu->name, dfu->r_left); + } + + dfu->inited = 1; + + return 0; +} + int dfu_flush(struct dfu_entity *dfu, void *buf, int size, int blk_seq_num) { int ret = 0; @@ -209,20 +235,9 @@ 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, (unsigned long)(dfu->i_buf - dfu->i_buf_start));
- if (!dfu->inited) { - /* initial state */ - dfu->crc = 0; - dfu->offset = 0; - dfu->bad_skip = 0; - dfu->i_blk_seq_num = 0; - dfu->i_buf_start = dfu_get_buf(dfu); - if (dfu->i_buf_start == NULL) - return -ENOMEM; - dfu->i_buf_end = dfu_get_buf(dfu) + dfu_buf_size; - dfu->i_buf = dfu->i_buf_start; - - dfu->inited = 1; - } + ret = dfu_transaction_initiate(dfu, false); + if (ret < 0) + return ret;
if (dfu->i_blk_seq_num != blk_seq_num) { printf("%s: Wrong sequence number! [%d] [%d]\n", @@ -338,28 +353,9 @@ 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 (!dfu->inited) { - dfu->i_buf_start = dfu_get_buf(dfu); - if (dfu->i_buf_start == NULL) - return -ENOMEM; - - ret = dfu->get_medium_size(dfu, &dfu->r_left); - if (ret < 0) - return ret; - - debug("%s: %s %lld [B]\n", __func__, dfu->name, dfu->r_left); - - dfu->i_blk_seq_num = 0; - dfu->crc = 0; - dfu->offset = 0; - dfu->i_buf_end = dfu_get_buf(dfu) + dfu_buf_size; - dfu->i_buf = dfu->i_buf_start; - dfu->b_left = 0; - - dfu->bad_skip = 0; - - dfu->inited = 1; - } + ret = dfu_transaction_initiate(dfu, true); + if (ret < 0) + return ret;
if (dfu->i_blk_seq_num != blk_seq_num) { printf("%s: Wrong sequence number! [%d] [%d]\n",
Hi Patrick,
I discover some limitation on DFU stack when I play on my ARM board with DFU for my external SDCARD : 16GB
It is mainly caused by the 'long' type (32 bits on ARMv7) used for size in dfu.c
I solve the issue with the 2 first patches of this serie
- manage size and result correctly in get_medium_size()
- change size to u64
The 2 next patch of the serie are not directly related to the issue. It is a cleanup and code factorization of dfu.c code
Patrick Delaunay (4): dfu: allow dfu read on partition greater than 2GB dfu: remove limitation on partition size dfu: factorize transaction cleanup dfu: add common function to initiate transaction
Thanks for your patches and fixing DFU.
I've build tested them: https://travis-ci.org/lmajewski/u-boot-dfu/builds
And it is OK.
Acked-by: Lukasz Majewski lukma@denx.de Tested-by: Lukasz Majewski lukma@denx.de
Test HW (DFU):
am335x BBB (Beagle Bone Black).
I've applied this code to u-boot-dfu tree.
drivers/dfu/dfu.c | 100 ++++++++++++++++++++++--------------------------- drivers/dfu/dfu_mmc.c | 20 +++++----- drivers/dfu/dfu_nand.c | 6 ++- drivers/dfu/dfu_ram.c | 6 ++- drivers/dfu/dfu_sf.c | 6 ++- include/dfu.h | 4 +- 6 files changed, 69 insertions(+), 73 deletions(-)
participants (2)
-
Patrick Delaunay -
Łukasz Majewski