[PATCH 0/5] DFU: Update dfu_alt_info parser etc.
Hi,
This series is improving DFU subsystem. This improves dfu_alt_info parser and fixing documents etc.
When I was debuging my patch for updating dfu_alt_info on the DeveloperBox platform, I found that dfu_alt_info parser doesn't accept redundant spaces and tabs. Also the dfu.rst description seems wrong. Moreover, there is no way to check whether the parser parses the dfu_alt_info correctly.
These patches fixes such issues. [1/5] is just for avoiding buffer overrun, [2/5] and [3/5] improves dfu_alt_info parser to accept redundant spaces and tabs, and check the number of arguments strictly so that the parser (and user) can notice any unexpected parameters. [4/5] fixes the documents (there seems some wrong description maybe coming from copy&paste). [5/5] allows user to run 'dfu list' even if the platform doesn't support DFU_OVER_USB.
Thank you,
---
Masami Hiramatsu (5): DFU: Do not copy the entity name over the buffer size DFU: Accept redundant spaces and tabs in dfu_alt_info DFU: Check the number of arguments and argument string strictly doc: usage: DFU: Fix dfu_alt_info document cmd/dfu: Enable 'dfu list' command without DFU_OVER_USB
cmd/dfu.c | 6 +++-- doc/usage/dfu.rst | 57 ++++++++++++++++++++++++++++++++++-------------- drivers/dfu/dfu.c | 37 ++++++++++++++++++++++++------- drivers/dfu/dfu_mmc.c | 55 +++++++++++++++++++++++++++------------------- drivers/dfu/dfu_mtd.c | 34 +++++++++++++++++++---------- drivers/dfu/dfu_nand.c | 34 ++++++++++++++++++----------- drivers/dfu/dfu_ram.c | 24 ++++++++++---------- drivers/dfu/dfu_sf.c | 34 ++++++++++++++++++----------- drivers/dfu/dfu_virt.c | 5 +++- include/dfu.h | 33 ++++++++++++++++++---------- 10 files changed, 205 insertions(+), 114 deletions(-)
-- Masami Hiramatsu masami.hiramatsu@linaro.org
Use strlcpy() instead of strcpy() to prevent copying the entity name over the name buffer size.
Signed-off-by: Masami Hiramatsu masami.hiramatsu@linaro.org --- drivers/dfu/dfu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c index af3975925a..66c41b5e76 100644 --- a/drivers/dfu/dfu.c +++ b/drivers/dfu/dfu.c @@ -503,7 +503,7 @@ static int dfu_fill_entity(struct dfu_entity *dfu, char *s, int alt,
debug("%s: %s interface: %s dev: %s\n", __func__, s, interface, devstr); st = strsep(&s, " "); - strcpy(dfu->name, st); + strlcpy(dfu->name, st, DFU_NAME_SIZE);
dfu->alt = alt; dfu->max_buf_size = 0;
On Mon, 6 Dec 2021 at 07:44, Masami Hiramatsu masami.hiramatsu@linaro.org wrote:
Use strlcpy() instead of strcpy() to prevent copying the entity name over the name buffer size.
Signed-off-by: Masami Hiramatsu masami.hiramatsu@linaro.org
drivers/dfu/dfu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c index af3975925a..66c41b5e76 100644 --- a/drivers/dfu/dfu.c +++ b/drivers/dfu/dfu.c @@ -503,7 +503,7 @@ static int dfu_fill_entity(struct dfu_entity *dfu, char *s, int alt,
debug("%s: %s interface: %s dev: %s\n", __func__, s, interface, devstr); st = strsep(&s, " ");
strcpy(dfu->name, st);
strlcpy(dfu->name, st, DFU_NAME_SIZE); dfu->alt = alt; dfu->max_buf_size = 0;
Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org
If dfu_alt_info has repeated spaces or tab (for indentation or readability), the dfu fails to parse it. For example, if dfu_alt_info="mtd nor1=image raw 100000 200000" (double spaces after "raw"), the image entity start address is '0' and the size '0x100000'. This is because the repeated space is not skipped.
Use space and tab as a separater and apply skip_spaces() to skip redundant spaces and tabs.
Signed-off-by: Masami Hiramatsu masami.hiramatsu@linaro.org --- drivers/dfu/dfu.c | 6 ++++-- drivers/dfu/dfu_mmc.c | 11 ++++++++--- drivers/dfu/dfu_mtd.c | 8 ++++++-- drivers/dfu/dfu_nand.c | 11 ++++++++--- drivers/dfu/dfu_ram.c | 3 ++- drivers/dfu/dfu_sf.c | 12 +++++++++--- 6 files changed, 37 insertions(+), 14 deletions(-)
diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c index 66c41b5e76..18154774f9 100644 --- a/drivers/dfu/dfu.c +++ b/drivers/dfu/dfu.c @@ -123,9 +123,10 @@ int dfu_config_interfaces(char *env) s = env; while (s) { ret = -EINVAL; - i = strsep(&s, " "); + i = strsep(&s, " \t"); if (!i) break; + s = skip_spaces(s); d = strsep(&s, "="); if (!d) break; @@ -502,8 +503,9 @@ static int dfu_fill_entity(struct dfu_entity *dfu, char *s, int alt, char *st;
debug("%s: %s interface: %s dev: %s\n", __func__, s, interface, devstr); - st = strsep(&s, " "); + st = strsep(&s, " \t"); strlcpy(dfu->name, st, DFU_NAME_SIZE); + s = skip_spaces(s);
dfu->alt = alt; dfu->max_buf_size = 0; diff --git a/drivers/dfu/dfu_mmc.c b/drivers/dfu/dfu_mmc.c index 3dab5a5f63..d747ede66c 100644 --- a/drivers/dfu/dfu_mmc.c +++ b/drivers/dfu/dfu_mmc.c @@ -351,11 +351,12 @@ int dfu_fill_entity_mmc(struct dfu_entity *dfu, char *devstr, char *s) dfu->data.mmc.dev_num = dectoul(devstr, NULL);
for (; parg < argv + sizeof(argv) / sizeof(*argv); ++parg) { - *parg = strsep(&s, " "); + *parg = strsep(&s, " \t"); if (*parg == NULL) { pr_err("Invalid number of arguments.\n"); return -ENODEV; } + s = skip_spaces(s); }
entity_type = argv[0]; @@ -390,9 +391,11 @@ int dfu_fill_entity_mmc(struct dfu_entity *dfu, char *devstr, char *s) * specifying the mmc HW defined partition number */ if (s) - if (!strcmp(strsep(&s, " "), "mmcpart")) + if (!strcmp(strsep(&s, " \t"), "mmcpart")) { + s = skip_spaces(s); dfu->data.mmc.hw_partition = simple_strtoul(s, NULL, 0); + }
} else if (!strcmp(entity_type, "part")) { struct disk_partition partinfo; @@ -412,8 +415,10 @@ int dfu_fill_entity_mmc(struct dfu_entity *dfu, char *devstr, char *s) * specifying the mmc HW defined partition number */ if (s) - if (!strcmp(strsep(&s, " "), "offset")) + if (!strcmp(strsep(&s, " \t"), "offset")) { + s = skip_spaces(s); offset = simple_strtoul(s, NULL, 0); + }
dfu->layout = DFU_RAW_ADDR; dfu->data.mmc.lba_start = partinfo.start + offset; diff --git a/drivers/dfu/dfu_mtd.c b/drivers/dfu/dfu_mtd.c index 0b7f17761f..ee534a9d10 100644 --- a/drivers/dfu/dfu_mtd.c +++ b/drivers/dfu/dfu_mtd.c @@ -12,6 +12,7 @@ #include <mtd.h> #include <jffs2/load_kernel.h> #include <linux/err.h> +#include <linux/ctype.h>
static bool mtd_is_aligned_with_block_size(struct mtd_info *mtd, u64 size) { @@ -265,11 +266,14 @@ int dfu_fill_entity_mtd(struct dfu_entity *dfu, char *devstr, char *s) dfu->data.mtd.info = mtd; dfu->max_buf_size = mtd->erasesize;
- st = strsep(&s, " "); + st = strsep(&s, " \t"); + s = skip_spaces(s); if (!strcmp(st, "raw")) { dfu->layout = DFU_RAW_ADDR; dfu->data.mtd.start = hextoul(s, &s); - s++; + if (!isspace(*s)) + return -1; + s = skip_spaces(s); dfu->data.mtd.size = hextoul(s, &s); } else if ((!strcmp(st, "part")) || (!strcmp(st, "partubi"))) { char mtd_id[32]; diff --git a/drivers/dfu/dfu_nand.c b/drivers/dfu/dfu_nand.c index e53b35e42b..76761939ab 100644 --- a/drivers/dfu/dfu_nand.c +++ b/drivers/dfu/dfu_nand.c @@ -201,11 +201,14 @@ int dfu_fill_entity_nand(struct dfu_entity *dfu, char *devstr, char *s)
dfu->data.nand.ubi = 0; dfu->dev_type = DFU_DEV_NAND; - st = strsep(&s, " "); + st = strsep(&s, " \t"); + s = skip_spaces(s); if (!strcmp(st, "raw")) { dfu->layout = DFU_RAW_ADDR; dfu->data.nand.start = hextoul(s, &s); - s++; + if (!isspace(*s)) + return -1; + s = skip_spaces(s); dfu->data.nand.size = hextoul(s, &s); } else if ((!strcmp(st, "part")) || (!strcmp(st, "partubi"))) { char mtd_id[32]; @@ -216,7 +219,9 @@ int dfu_fill_entity_nand(struct dfu_entity *dfu, char *devstr, char *s) dfu->layout = DFU_RAW_ADDR;
dev = dectoul(s, &s); - s++; + if (!isspace(*s)) + return -1; + s = skip_spaces(s); part = dectoul(s, &s);
sprintf(mtd_id, "%s%d,%d", "nand", dev, part - 1); diff --git a/drivers/dfu/dfu_ram.c b/drivers/dfu/dfu_ram.c index cc7e45ba33..361a3ff8af 100644 --- a/drivers/dfu/dfu_ram.c +++ b/drivers/dfu/dfu_ram.c @@ -60,11 +60,12 @@ int dfu_fill_entity_ram(struct dfu_entity *dfu, char *devstr, char *s) const char **parg = argv;
for (; parg < argv + sizeof(argv) / sizeof(*argv); ++parg) { - *parg = strsep(&s, " "); + *parg = strsep(&s, " \t"); if (*parg == NULL) { pr_err("Invalid number of arguments.\n"); return -ENODEV; } + s = skip_spaces(s); }
dfu->dev_type = DFU_DEV_RAM; diff --git a/drivers/dfu/dfu_sf.c b/drivers/dfu/dfu_sf.c index b72493ced8..993e951bc3 100644 --- a/drivers/dfu/dfu_sf.c +++ b/drivers/dfu/dfu_sf.c @@ -13,6 +13,7 @@ #include <spi_flash.h> #include <jffs2/load_kernel.h> #include <linux/mtd/mtd.h> +#include <linux/ctype.h>
static int dfu_get_medium_size_sf(struct dfu_entity *dfu, u64 *size) { @@ -178,11 +179,14 @@ int dfu_fill_entity_sf(struct dfu_entity *dfu, char *devstr, char *s) dfu->dev_type = DFU_DEV_SF; dfu->max_buf_size = dfu->data.sf.dev->sector_size;
- st = strsep(&s, " "); + st = strsep(&s, " \t"); + s = skip_spaces(s); if (!strcmp(st, "raw")) { dfu->layout = DFU_RAW_ADDR; dfu->data.sf.start = hextoul(s, &s); - s++; + if (!isspace(*s)) + return -1; + s = skip_spaces(s); dfu->data.sf.size = hextoul(s, &s); } else if (CONFIG_IS_ENABLED(DFU_SF_PART) && (!strcmp(st, "part") || !strcmp(st, "partubi"))) { @@ -195,7 +199,9 @@ int dfu_fill_entity_sf(struct dfu_entity *dfu, char *devstr, char *s) dfu->layout = DFU_RAW_ADDR;
dev = dectoul(s, &s); - s++; + if (!isspace(*s)) + return -1; + s = skip_spaces(s); part = dectoul(s, &s);
sprintf(mtd_id, "%s%d,%d", "nor", dev, part - 1);
When parsing the dfu_alt_info, check the number of arguments and argument string strictly. If there is any garbage data (which is not able to be parsed correctly) in dfu_alt_info, that means something wrong and user may make a typo or mis- understanding about the syntax. Since the dfu_alt_info is used for updating the firmware, this mistake may lead to brick the hardware. Thus it should be checked strictly for making sure there is no mistake.
Signed-off-by: Masami Hiramatsu masami.hiramatsu@linaro.org --- drivers/dfu/dfu.c | 31 +++++++++++++++++++++------ drivers/dfu/dfu_mmc.c | 56 ++++++++++++++++++++++++++---------------------- drivers/dfu/dfu_mtd.c | 36 +++++++++++++++++++------------ drivers/dfu/dfu_nand.c | 39 ++++++++++++++++++--------------- drivers/dfu/dfu_ram.c | 25 ++++++++++----------- drivers/dfu/dfu_sf.c | 38 +++++++++++++++++---------------- drivers/dfu/dfu_virt.c | 5 +++- include/dfu.h | 33 ++++++++++++++++++---------- 8 files changed, 154 insertions(+), 109 deletions(-)
diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c index 18154774f9..516dda6179 100644 --- a/drivers/dfu/dfu.c +++ b/drivers/dfu/dfu.c @@ -500,12 +500,29 @@ int dfu_read(struct dfu_entity *dfu, void *buf, int size, int blk_seq_num) static int dfu_fill_entity(struct dfu_entity *dfu, char *s, int alt, char *interface, char *devstr) { + char *argv[DFU_MAX_ENTITY_ARGS]; + int argc; char *st;
debug("%s: %s interface: %s dev: %s\n", __func__, s, interface, devstr); st = strsep(&s, " \t"); strlcpy(dfu->name, st, DFU_NAME_SIZE); - s = skip_spaces(s); + + /* Parse arguments */ + for (argc = 0; s && argc < DFU_MAX_ENTITY_ARGS; argc++) { + s = skip_spaces(s); + if (!*s) + break; + argv[argc] = strsep(&s, " \t"); + } + + if (argc == DFU_MAX_ENTITY_ARGS && s) { + s = skip_spaces(s); + if (*s) { + log_err("Too many arguments for %s\n", dfu->name); + return -EINVAL; + } + }
dfu->alt = alt; dfu->max_buf_size = 0; @@ -513,22 +530,22 @@ static int dfu_fill_entity(struct dfu_entity *dfu, char *s, int alt,
/* Specific for mmc device */ if (strcmp(interface, "mmc") == 0) { - if (dfu_fill_entity_mmc(dfu, devstr, s)) + if (dfu_fill_entity_mmc(dfu, devstr, argv, argc)) return -1; } else if (strcmp(interface, "mtd") == 0) { - if (dfu_fill_entity_mtd(dfu, devstr, s)) + if (dfu_fill_entity_mtd(dfu, devstr, argv, argc)) return -1; } else if (strcmp(interface, "nand") == 0) { - if (dfu_fill_entity_nand(dfu, devstr, s)) + if (dfu_fill_entity_nand(dfu, devstr, argv, argc)) return -1; } else if (strcmp(interface, "ram") == 0) { - if (dfu_fill_entity_ram(dfu, devstr, s)) + if (dfu_fill_entity_ram(dfu, devstr, argv, argc)) return -1; } else if (strcmp(interface, "sf") == 0) { - if (dfu_fill_entity_sf(dfu, devstr, s)) + if (dfu_fill_entity_sf(dfu, devstr, argv, argc)) return -1; } else if (strcmp(interface, "virt") == 0) { - if (dfu_fill_entity_virt(dfu, devstr, s)) + if (dfu_fill_entity_virt(dfu, devstr, argv, argc)) return -1; } else { printf("%s: Device %s not (yet) supported!\n", diff --git a/drivers/dfu/dfu_mmc.c b/drivers/dfu/dfu_mmc.c index d747ede66c..a91da972d4 100644 --- a/drivers/dfu/dfu_mmc.c +++ b/drivers/dfu/dfu_mmc.c @@ -337,35 +337,34 @@ void dfu_free_entity_mmc(struct dfu_entity *dfu) * 4th (optional): * mmcpart <num> (access to HW eMMC partitions) */ -int dfu_fill_entity_mmc(struct dfu_entity *dfu, char *devstr, char *s) +int dfu_fill_entity_mmc(struct dfu_entity *dfu, char *devstr, char **argv, int argc) { const char *entity_type; ssize_t second_arg; size_t third_arg; - struct mmc *mmc; + char *s;
- const char *argv[3]; - const char **parg = argv; - - dfu->data.mmc.dev_num = dectoul(devstr, NULL); - - for (; parg < argv + sizeof(argv) / sizeof(*argv); ++parg) { - *parg = strsep(&s, " \t"); - if (*parg == NULL) { - pr_err("Invalid number of arguments.\n"); - return -ENODEV; - } - s = skip_spaces(s); + if (argc < 3) { + pr_err("The number of parameters are not enough.\n"); + return -EINVAL; }
+ dfu->data.mmc.dev_num = dectoul(devstr, &s); + if (*s) + return -EINVAL; + entity_type = argv[0]; /* * Base 0 means we'll accept (prefixed with 0x or 0) base 16, 8, * with default 10. */ - second_arg = simple_strtol(argv[1], NULL, 0); - third_arg = simple_strtoul(argv[2], NULL, 0); + second_arg = simple_strtol(argv[1], &s, 0); + if (*s) + return -EINVAL; + third_arg = simple_strtoul(argv[2], &s, 0); + if (*s) + return -EINVAL;
mmc = find_mmc_device(dfu->data.mmc.dev_num); if (mmc == NULL) { @@ -390,12 +389,14 @@ int dfu_fill_entity_mmc(struct dfu_entity *dfu, char *devstr, char *s) * Check for an extra entry at dfu_alt_info env variable * specifying the mmc HW defined partition number */ - if (s) - if (!strcmp(strsep(&s, " \t"), "mmcpart")) { - s = skip_spaces(s); - dfu->data.mmc.hw_partition = - simple_strtoul(s, NULL, 0); + if (argc > 3) { + if (argc != 5 || strcmp(argv[3], "mmcpart")) { + pr_err("DFU mmc raw accept 'mmcpart <partnum>' option.\n"); + return -EINVAL; } + dfu->data.mmc.hw_partition = + simple_strtoul(argv[4], NULL, 0); + }
} else if (!strcmp(entity_type, "part")) { struct disk_partition partinfo; @@ -414,15 +415,18 @@ int dfu_fill_entity_mmc(struct dfu_entity *dfu, char *devstr, char *s) * Check for an extra entry at dfu_alt_info env variable * specifying the mmc HW defined partition number */ - if (s) - if (!strcmp(strsep(&s, " \t"), "offset")) { - s = skip_spaces(s); - offset = simple_strtoul(s, NULL, 0); + if (argc > 3) { + if (argc != 5 || strcmp(argv[3], "offset")) { + pr_err("DFU mmc raw accept 'mmcpart <partnum>' option.\n"); + return -EINVAL; } + dfu->data.mmc.hw_partition = + simple_strtoul(argv[4], NULL, 0); + }
dfu->layout = DFU_RAW_ADDR; dfu->data.mmc.lba_start = partinfo.start + offset; - dfu->data.mmc.lba_size = partinfo.size-offset; + dfu->data.mmc.lba_size = partinfo.size - offset; dfu->data.mmc.lba_blk_size = partinfo.blksz; } else if (!strcmp(entity_type, "fat")) { dfu->layout = DFU_FS_FAT; diff --git a/drivers/dfu/dfu_mtd.c b/drivers/dfu/dfu_mtd.c index ee534a9d10..81357769ff 100644 --- a/drivers/dfu/dfu_mtd.c +++ b/drivers/dfu/dfu_mtd.c @@ -251,9 +251,9 @@ static unsigned int dfu_polltimeout_mtd(struct dfu_entity *dfu) return DFU_DEFAULT_POLL_TIMEOUT; }
-int dfu_fill_entity_mtd(struct dfu_entity *dfu, char *devstr, char *s) +int dfu_fill_entity_mtd(struct dfu_entity *dfu, char *devstr, char **argv, int argc) { - char *st; + char *s; struct mtd_info *mtd; int ret, part;
@@ -265,25 +265,33 @@ int dfu_fill_entity_mtd(struct dfu_entity *dfu, char *devstr, char *s) dfu->dev_type = DFU_DEV_MTD; dfu->data.mtd.info = mtd; dfu->max_buf_size = mtd->erasesize; + if (argc < 1) + return -EINVAL;
- st = strsep(&s, " \t"); - s = skip_spaces(s); - if (!strcmp(st, "raw")) { + if (!strcmp(argv[0], "raw")) { + if (argc != 3) + return -EINVAL; dfu->layout = DFU_RAW_ADDR; - dfu->data.mtd.start = hextoul(s, &s); - if (!isspace(*s)) - return -1; - s = skip_spaces(s); - dfu->data.mtd.size = hextoul(s, &s); - } else if ((!strcmp(st, "part")) || (!strcmp(st, "partubi"))) { + dfu->data.mtd.start = hextoul(argv[1], &s); + if (*s) + return -EINVAL; + dfu->data.mtd.size = hextoul(argv[2], &s); + if (*s) + return -EINVAL; + } else if ((!strcmp(argv[0], "part")) || (!strcmp(argv[0], "partubi"))) { char mtd_id[32]; struct mtd_device *mtd_dev; u8 part_num; struct part_info *pi;
+ if (argc != 2) + return -EINVAL; + dfu->layout = DFU_RAW_ADDR;
- part = dectoul(s, &s); + part = dectoul(argv[1], &s); + if (*s) + return -EINVAL;
sprintf(mtd_id, "%s,%d", devstr, part - 1); printf("using id '%s'\n", mtd_id); @@ -298,10 +306,10 @@ int dfu_fill_entity_mtd(struct dfu_entity *dfu, char *devstr, char *s)
dfu->data.mtd.start = pi->offset; dfu->data.mtd.size = pi->size; - if (!strcmp(st, "partubi")) + if (!strcmp(argv[0], "partubi")) dfu->data.mtd.ubi = 1; } else { - printf("%s: Memory layout (%s) not supported!\n", __func__, st); + printf("%s: Memory layout (%s) not supported!\n", __func__, argv[0]); return -1; }
diff --git a/drivers/dfu/dfu_nand.c b/drivers/dfu/dfu_nand.c index 76761939ab..08e8cf5cdb 100644 --- a/drivers/dfu/dfu_nand.c +++ b/drivers/dfu/dfu_nand.c @@ -194,23 +194,25 @@ unsigned int dfu_polltimeout_nand(struct dfu_entity *dfu) return DFU_DEFAULT_POLL_TIMEOUT; }
-int dfu_fill_entity_nand(struct dfu_entity *dfu, char *devstr, char *s) +int dfu_fill_entity_nand(struct dfu_entity *dfu, char *devstr, char **argv, int argc) { - char *st; + char *s; int ret, dev, part;
dfu->data.nand.ubi = 0; dfu->dev_type = DFU_DEV_NAND; - st = strsep(&s, " \t"); - s = skip_spaces(s); - if (!strcmp(st, "raw")) { + if (argc != 3) + return -EINVAL; + + if (!strcmp(argv[0], "raw")) { dfu->layout = DFU_RAW_ADDR; - dfu->data.nand.start = hextoul(s, &s); - if (!isspace(*s)) - return -1; - s = skip_spaces(s); - dfu->data.nand.size = hextoul(s, &s); - } else if ((!strcmp(st, "part")) || (!strcmp(st, "partubi"))) { + dfu->data.nand.start = hextoul(argv[1], &s); + if (*s) + return -EINVAL; + dfu->data.nand.size = hextoul(argv[2], &s); + if (*s) + return -EINVAL; + } else if ((!strcmp(argv[0], "part")) || (!strcmp(argv[0], "partubi"))) { char mtd_id[32]; struct mtd_device *mtd_dev; u8 part_num; @@ -218,11 +220,12 @@ int dfu_fill_entity_nand(struct dfu_entity *dfu, char *devstr, char *s)
dfu->layout = DFU_RAW_ADDR;
- dev = dectoul(s, &s); - if (!isspace(*s)) - return -1; - s = skip_spaces(s); - part = dectoul(s, &s); + dev = dectoul(argv[1], &s); + if (*s) + return -EINVAL; + part = dectoul(argv[2], &s); + if (*s) + return -EINVAL;
sprintf(mtd_id, "%s%d,%d", "nand", dev, part - 1); debug("using id '%s'\n", mtd_id); @@ -237,10 +240,10 @@ int dfu_fill_entity_nand(struct dfu_entity *dfu, char *devstr, char *s)
dfu->data.nand.start = pi->offset; dfu->data.nand.size = pi->size; - if (!strcmp(st, "partubi")) + if (!strcmp(argv[0], "partubi")) dfu->data.nand.ubi = 1; } else { - printf("%s: Memory layout (%s) not supported!\n", __func__, st); + printf("%s: Memory layout (%s) not supported!\n", __func__, argv[0]); return -1; }
diff --git a/drivers/dfu/dfu_ram.c b/drivers/dfu/dfu_ram.c index 361a3ff8af..9d10303164 100644 --- a/drivers/dfu/dfu_ram.c +++ b/drivers/dfu/dfu_ram.c @@ -54,18 +54,13 @@ static int dfu_read_medium_ram(struct dfu_entity *dfu, u64 offset, return dfu_transfer_medium_ram(DFU_OP_READ, dfu, offset, buf, len); }
-int dfu_fill_entity_ram(struct dfu_entity *dfu, char *devstr, char *s) +int dfu_fill_entity_ram(struct dfu_entity *dfu, char *devstr, char **argv, int argc) { - const char *argv[3]; - const char **parg = argv; - - for (; parg < argv + sizeof(argv) / sizeof(*argv); ++parg) { - *parg = strsep(&s, " \t"); - if (*parg == NULL) { - pr_err("Invalid number of arguments.\n"); - return -ENODEV; - } - s = skip_spaces(s); + char *s; + + if (argc != 3) { + pr_err("Invalid number of arguments.\n"); + return -EINVAL; }
dfu->dev_type = DFU_DEV_RAM; @@ -75,8 +70,12 @@ int dfu_fill_entity_ram(struct dfu_entity *dfu, char *devstr, char *s) }
dfu->layout = DFU_RAM_ADDR; - dfu->data.ram.start = hextoul(argv[1], NULL); - dfu->data.ram.size = hextoul(argv[2], NULL); + dfu->data.ram.start = hextoul(argv[1], &s); + if (*s) + return -EINVAL; + dfu->data.ram.size = hextoul(argv[2], &s); + if (*s) + return -EINVAL;
dfu->write_medium = dfu_write_medium_ram; dfu->get_medium_size = dfu_get_medium_size_ram; diff --git a/drivers/dfu/dfu_sf.c b/drivers/dfu/dfu_sf.c index 993e951bc3..25a9c81850 100644 --- a/drivers/dfu/dfu_sf.c +++ b/drivers/dfu/dfu_sf.c @@ -166,9 +166,9 @@ static struct spi_flash *parse_dev(char *devstr) return dev; }
-int dfu_fill_entity_sf(struct dfu_entity *dfu, char *devstr, char *s) +int dfu_fill_entity_sf(struct dfu_entity *dfu, char *devstr, char **argv, int argc) { - char *st; + char *s; char *devstr_bkup = strdup(devstr);
dfu->data.sf.dev = parse_dev(devstr_bkup); @@ -179,17 +179,18 @@ int dfu_fill_entity_sf(struct dfu_entity *dfu, char *devstr, char *s) dfu->dev_type = DFU_DEV_SF; dfu->max_buf_size = dfu->data.sf.dev->sector_size;
- st = strsep(&s, " \t"); - s = skip_spaces(s); - if (!strcmp(st, "raw")) { + if (argc != 3) + return -EINVAL; + if (!strcmp(argv[0], "raw")) { dfu->layout = DFU_RAW_ADDR; - dfu->data.sf.start = hextoul(s, &s); - if (!isspace(*s)) - return -1; - s = skip_spaces(s); - dfu->data.sf.size = hextoul(s, &s); + dfu->data.sf.start = hextoul(argv[1], &s); + if (*s) + return -EINVAL; + dfu->data.sf.size = hextoul(argv[2], &s); + if (*s) + return -EINVAL; } else if (CONFIG_IS_ENABLED(DFU_SF_PART) && - (!strcmp(st, "part") || !strcmp(st, "partubi"))) { + (!strcmp(argv[0], "part") || !strcmp(argv[0], "partubi"))) { char mtd_id[32]; struct mtd_device *mtd_dev; u8 part_num; @@ -198,11 +199,12 @@ int dfu_fill_entity_sf(struct dfu_entity *dfu, char *devstr, char *s)
dfu->layout = DFU_RAW_ADDR;
- dev = dectoul(s, &s); - if (!isspace(*s)) - return -1; - s = skip_spaces(s); - part = dectoul(s, &s); + dev = dectoul(argv[1], &s); + if (*s) + return -EINVAL; + part = dectoul(argv[2], &s); + if (*s) + return -EINVAL;
sprintf(mtd_id, "%s%d,%d", "nor", dev, part - 1); printf("using id '%s'\n", mtd_id); @@ -216,10 +218,10 @@ int dfu_fill_entity_sf(struct dfu_entity *dfu, char *devstr, char *s) } dfu->data.sf.start = pi->offset; dfu->data.sf.size = pi->size; - if (!strcmp(st, "partubi")) + if (!strcmp(argv[0], "partubi")) dfu->data.sf.ubi = 1; } else { - printf("%s: Memory layout (%s) not supported!\n", __func__, st); + printf("%s: Memory layout (%s) not supported!\n", __func__, argv[0]); spi_flash_free(dfu->data.sf.dev); return -1; } diff --git a/drivers/dfu/dfu_virt.c b/drivers/dfu/dfu_virt.c index 80c99cb06e..29f7a08f67 100644 --- a/drivers/dfu/dfu_virt.c +++ b/drivers/dfu/dfu_virt.c @@ -32,10 +32,13 @@ int __weak dfu_read_medium_virt(struct dfu_entity *dfu, u64 offset, return 0; }
-int dfu_fill_entity_virt(struct dfu_entity *dfu, char *devstr, char *s) +int dfu_fill_entity_virt(struct dfu_entity *dfu, char *devstr, char **argv, int argc) { debug("%s: devstr = %s\n", __func__, devstr);
+ if (argc != 0) + return -EINVAL; + dfu->dev_type = DFU_DEV_VIRT; dfu->layout = DFU_RAW_ADDR; dfu->data.virt.dev_num = dectoul(devstr, NULL); diff --git a/include/dfu.h b/include/dfu.h index f6868982df..dcb9cd9d79 100644 --- a/include/dfu.h +++ b/include/dfu.h @@ -432,11 +432,15 @@ static inline void dfu_set_defer_flush(struct dfu_entity *dfu) int dfu_write_from_mem_addr(struct dfu_entity *dfu, void *buf, int size);
/* Device specific */ +/* Each entity has 5 arguments in maximum. */ +#define DFU_MAX_ENTITY_ARGS 5 + #if CONFIG_IS_ENABLED(DFU_MMC) -extern int dfu_fill_entity_mmc(struct dfu_entity *dfu, char *devstr, char *s); +extern int dfu_fill_entity_mmc(struct dfu_entity *dfu, char *devstr, + char **argv, int argc); #else static inline int dfu_fill_entity_mmc(struct dfu_entity *dfu, char *devstr, - char *s) + char **argv, int argc) { puts("MMC support not available!\n"); return -1; @@ -444,10 +448,11 @@ static inline int dfu_fill_entity_mmc(struct dfu_entity *dfu, char *devstr, #endif
#if CONFIG_IS_ENABLED(DFU_NAND) -extern int dfu_fill_entity_nand(struct dfu_entity *dfu, char *devstr, char *s); +extern int dfu_fill_entity_nand(struct dfu_entity *dfu, char *devstr, + char **argv, int argc); #else static inline int dfu_fill_entity_nand(struct dfu_entity *dfu, char *devstr, - char *s) + char **argv, int argc) { puts("NAND support not available!\n"); return -1; @@ -455,10 +460,11 @@ static inline int dfu_fill_entity_nand(struct dfu_entity *dfu, char *devstr, #endif
#if CONFIG_IS_ENABLED(DFU_RAM) -extern int dfu_fill_entity_ram(struct dfu_entity *dfu, char *devstr, char *s); +extern int dfu_fill_entity_ram(struct dfu_entity *dfu, char *devstr, + char **argv, int argc); #else static inline int dfu_fill_entity_ram(struct dfu_entity *dfu, char *devstr, - char *s) + char **argv, int argc) { puts("RAM support not available!\n"); return -1; @@ -466,10 +472,11 @@ static inline int dfu_fill_entity_ram(struct dfu_entity *dfu, char *devstr, #endif
#if CONFIG_IS_ENABLED(DFU_SF) -extern int dfu_fill_entity_sf(struct dfu_entity *dfu, char *devstr, char *s); +extern int dfu_fill_entity_sf(struct dfu_entity *dfu, char *devstr, + char **argv, int argc); #else static inline int dfu_fill_entity_sf(struct dfu_entity *dfu, char *devstr, - char *s) + char **argv, int argc) { puts("SF support not available!\n"); return -1; @@ -477,10 +484,11 @@ static inline int dfu_fill_entity_sf(struct dfu_entity *dfu, char *devstr, #endif
#if CONFIG_IS_ENABLED(DFU_MTD) -int dfu_fill_entity_mtd(struct dfu_entity *dfu, char *devstr, char *s); +extern int dfu_fill_entity_mtd(struct dfu_entity *dfu, char *devstr, + char **argv, int argc); #else static inline int dfu_fill_entity_mtd(struct dfu_entity *dfu, char *devstr, - char *s) + char **argv, int argc) { puts("MTD support not available!\n"); return -1; @@ -488,7 +496,8 @@ static inline int dfu_fill_entity_mtd(struct dfu_entity *dfu, char *devstr, #endif
#ifdef CONFIG_DFU_VIRT -int dfu_fill_entity_virt(struct dfu_entity *dfu, char *devstr, char *s); +int dfu_fill_entity_virt(struct dfu_entity *dfu, char *devstr, + char **argv, int argc); int dfu_write_medium_virt(struct dfu_entity *dfu, u64 offset, void *buf, long *len); int dfu_get_medium_size_virt(struct dfu_entity *dfu, u64 *size); @@ -496,7 +505,7 @@ int dfu_read_medium_virt(struct dfu_entity *dfu, u64 offset, void *buf, long *len); #else static inline int dfu_fill_entity_virt(struct dfu_entity *dfu, char *devstr, - char *s) + char **argv, int argc) { puts("VIRT support not available!\n"); return -1;
Fix some typo and wrong information about dfu_alt_info. - Add the parameter format, decimal only or hexadecimal. - Use same parameter name for the same kind of parameters. (e.g. dev -> dev_id)
Signed-off-by: Masami Hiramatsu masami.hiramatsu@linaro.org --- doc/usage/dfu.rst | 57 +++++++++++++++++++++++++++++++++++++---------------- 1 file changed, 40 insertions(+), 17 deletions(-)
diff --git a/doc/usage/dfu.rst b/doc/usage/dfu.rst index 11c88072b8..ed47ff561e 100644 --- a/doc/usage/dfu.rst +++ b/doc/usage/dfu.rst @@ -113,9 +113,9 @@ mmc each element in *dfu_alt_info* being
* <name> raw <offset> <size> [mmcpart <num>] raw access to mmc device - * <name> part <dev> <part_id> [mmcpart <num>] raw access to partition - * <name> fat <dev> <part_id> [mmcpart <num>] file in FAT partition - * <name> ext4 <dev> <part_id> [mmcpart <num>] file in EXT4 partition + * <name> part <dev> <part_id> [offset <byte>] raw access to partition + * <name> fat <dev> <part_id> file in FAT partition + * <name> ext4 <dev> <part_id> file in EXT4 partition * <name> skip 0 0 ignore flashed data * <name> script 0 0 execute commands in shell
@@ -169,14 +169,20 @@ nand
each element in *dfu_alt_info* being either of
- * <name> raw <offset> <size> raw access to mmc device - * <name> part <dev> <part_id> raw acces to partition - * <name> partubi <dev> <part_id> raw acces to ubi partition + * <name> raw <offset> <size> raw access to nand device + * <name> part <dev_id> <part_id> raw access to partition + * <name> partubi <dev_id> <part_id> raw access to ubi partition
with
- partid - is the MTD partition index + offset + is the offset in the nand device (hexadecimal without "0x") + size + is the size of the access area (hexadecimal without "0x") + dev_id + is the NAND device index (decimal only) + part_id + is the NAND partition index (decimal only)
ram raw access to ram:: @@ -190,6 +196,13 @@ ram
<name> ram <offset> <size> raw access to ram
+ with + + offset + is the offset in the ram device (hexadecimal without "0x") + size + is the size of the access area (hexadecimal without "0x") + sf serial flash : NOR::
@@ -198,13 +211,19 @@ sf each element in *dfu_alt_info* being either of:
* <name> raw <offset> <size> raw access to sf device - * <name> part <dev> <part_id> raw acces to partition - * <name> partubi <dev> <part_id> raw acces to ubi partition + * <name> part <dev_id> <part_id> raw acces to partition + * <name> partubi <dev_id> <part_id> raw acces to ubi partition
with
- partid - is the MTD partition index + offset + is the offset in the sf device (hexadecimal without "0x") + size + is the size of the access area (hexadecimal without "0x") + dev_id + is the sf device index (the device is "nor<dev_id>") (deximal only) + part_id + is the MTD partition index (decimal only)
mtd all MTD device: NAND, SPI-NOR, SPI-NAND,...:: @@ -219,14 +238,18 @@ mtd
each element in *dfu_alt_info* being either of:
- * <name> raw <offset> <size> forraw access to mtd device - * <name> part <dev> <part_id> for raw acces to partition - * <name> partubi <dev> <part_id> for raw acces to ubi partition + * <name> raw <offset> <size> for raw access to mtd device + * <name> part <part_id> for raw access to partition + * <name> partubi <part_id> for raw access to ubi partition
with
- partid - is the MTD partition index + offset + is the offset in the mtd device (hexadecimal without "0x") + size + is the size of the access area (hexadecimal without "0x") + part_id + is the MTD partition index (decimal only)
virt virtual flash back end for DFU
Since dfu is not only used for USB, and some platform only supports DFU_OVER_TFTP or EFI capsule update, dfu_alt_info is defined on such platforms too.
For such platform, 'dfu list' command is useful to check how the current dfu_alt_info setting is parsed.
Signed-off-by: Masami Hiramatsu masami.hiramatsu@linaro.org --- cmd/dfu.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/cmd/dfu.c b/cmd/dfu.c index 4a288f74c2..208555c888 100644 --- a/cmd/dfu.c +++ b/cmd/dfu.c @@ -50,7 +50,6 @@ static int do_dfu(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) if (!strcmp(argv[1], "tftp")) return update_tftp(value, interface, devstring); #endif -#ifdef CONFIG_DFU_OVER_USB ret = dfu_init_env_entities(interface, devstring); if (ret) goto done; @@ -65,6 +64,7 @@ static int do_dfu(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) goto done; }
+#ifdef CONFIG_DFU_OVER_USB int controller_index = simple_strtoul(usb_controller, NULL, 0); bool retry = false; do { @@ -79,9 +79,9 @@ static int do_dfu(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) } } while (retry);
+#endif done: dfu_free_entities(); -#endif return ret; }
@@ -100,8 +100,8 @@ U_BOOT_CMD(dfu, CONFIG_SYS_MAXARGS, 1, do_dfu, #ifdef CONFIG_DFU_TIMEOUT " [<timeout>] - specify inactivity timeout in seconds\n" #endif - " [list] - list available alt settings\n" #endif + " [list] - list available alt settings\n" #ifdef CONFIG_DFU_OVER_TFTP #ifdef CONFIG_DFU_OVER_USB "dfu "
On Mon, Dec 06, 2021 at 02:45:11PM +0900, Masami Hiramatsu wrote:
Since dfu is not only used for USB, and some platform only supports DFU_OVER_TFTP or EFI capsule update, dfu_alt_info is defined on such platforms too.
For such platform, 'dfu list' command is useful to check how the current dfu_alt_info setting is parsed.
Signed-off-by: Masami Hiramatsu masami.hiramatsu@linaro.org
This patch breaks at least smegw01 and likely warp and iot2050 as well, please update the series and repost, thanks!
Hi Tom,
Thanks for testing, I confirmed the error.
CC cmd/dfu.o cmd/dfu.c: In function ‘do_dfu’: cmd/dfu.c:53:30: error: ‘interface’ undeclared (first use in this function); did you mean ‘usb_interface’? 53 | ret = dfu_init_env_entities(interface, devstring); | ^~~~~~~~~ | usb_interface cmd/dfu.c:53:30: note: each undeclared identifier is reported only once for each function it appears in cmd/dfu.c:53:41: error: ‘devstring’ undeclared (first use in this function); did you mean ‘fdt_string’? 53 | ret = dfu_init_env_entities(interface, devstring); | ^~~~~~~~~ | fdt_string
Let me fix it.
Thank you!
2022年1月29日(土) 1:30 Tom Rini trini@konsulko.com:
On Mon, Dec 06, 2021 at 02:45:11PM +0900, Masami Hiramatsu wrote:
Since dfu is not only used for USB, and some platform only supports DFU_OVER_TFTP or EFI capsule update, dfu_alt_info is defined on such platforms too.
For such platform, 'dfu list' command is useful to check how the current dfu_alt_info setting is parsed.
Signed-off-by: Masami Hiramatsu masami.hiramatsu@linaro.org
This patch breaks at least smegw01 and likely warp and iot2050 as well, please update the series and repost, thanks!
-- Tom
participants (3)
-
Ilias Apalodimas -
Masami Hiramatsu -
Tom Rini