[PATCH v2 0/8] cmd/mem.c fixups
v2: Add Simon's R-b to patch 1. Patches 2-8 are new.
This started as a simple one-liner (still patch 1), but when Simon asked me to update the documentation and add some test cases I stumbled on some things that might as well also get fixed.
Patch 2 is the documentation update, might be folded into patch 1. 3 and 4 are preparations for the test cases added in patch 5.
6 is an actual fix; 32 bit targets wrongly list .q in their help texts.
7 and 8 fix stale documentation.
Rasmus Villemoes (8): cmd/mem.c: use memmove in do_mem_cp() doc/usage/cmd/cp.rst: document that overlapping regions are supported cmd/command.c: constify "arg" argument of cmd_get_data_size() cmd/command.c: relax length check in cmd_get_data_size() test: add test of "cp" shell command cmd/mem.c: fix wrong use of ifdef, drop pointless SUPPORT_64BIT_DATA macro README: drop mention of MEM_SUPPORT_64BIT_DATA doc: drop references to non-existing CONFIG_MEM_SUPPORT_64BIT_DATA
README | 3 - cmd/mem.c | 40 +++++----- common/command.c | 4 +- doc/usage/cmd/cmp.rst | 2 +- doc/usage/cmd/cp.rst | 5 +- include/command.h | 2 +- test/cmd/Makefile | 1 + test/cmd/mem_copy.c | 169 ++++++++++++++++++++++++++++++++++++++++++ 8 files changed, 196 insertions(+), 30 deletions(-) create mode 100644 test/cmd/mem_copy.c
There's no 'mv' shell command for handling overlapping src and dst regions, and there's no point introducing one, when we can just make the existing 'cp' command DTRT in all cases. memmove() should at most be a few instructions more then memcpy() (to detect the appropriate direction to do the copy), which is of course completely in the noise with all the string processing that a shell command does.
Reviewed-by: Simon Glass sjg@chromium.org Signed-off-by: Rasmus Villemoes rasmus.villemoes@prevas.dk --- cmd/mem.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/cmd/mem.c b/cmd/mem.c index 66c2d36a148..c696b92a274 100644 --- a/cmd/mem.c +++ b/cmd/mem.c @@ -361,7 +361,7 @@ static int do_mem_cp(struct cmd_tbl *cmdtp, int flag, int argc, } #endif
- memcpy(dst, src, count * size); + memmove(dst, src, count * size);
unmap_sysmem(src); unmap_sysmem(dst);
On Wed, Jan 03, 2024 at 11:47:03AM +0100, Rasmus Villemoes wrote:
There's no 'mv' shell command for handling overlapping src and dst regions, and there's no point introducing one, when we can just make the existing 'cp' command DTRT in all cases. memmove() should at most be a few instructions more then memcpy() (to detect the appropriate direction to do the copy), which is of course completely in the noise with all the string processing that a shell command does.
Reviewed-by: Simon Glass sjg@chromium.org Signed-off-by: Rasmus Villemoes rasmus.villemoes@prevas.dk
For the series, applied to u-boot/master, thanks!
Now that the cp command is changed to use memmove() internally, update the documentation to explicitly state that overlapping regions are allowed.
Signed-off-by: Rasmus Villemoes rasmus.villemoes@prevas.dk --- doc/usage/cmd/cp.rst | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/doc/usage/cmd/cp.rst b/doc/usage/cmd/cp.rst index 12a24e19fee..67360e30e41 100644 --- a/doc/usage/cmd/cp.rst +++ b/doc/usage/cmd/cp.rst @@ -19,7 +19,8 @@ Description
The cp command is used to copy *count* chunks of memory from the *source* address to the *target* address. If the *target* address points to NOR flash, -the flash is programmed. +the flash is programmed. When the *target* address points at ordinary memory, +memmove() is used, so the two regions may overlap.
The number bytes in one chunk is defined by the suffix defaulting to 4 bytes:
This function obviously does not and must not modify "arg". Change the prototype to allow passing an argument of type "const char*" without requiring a cast.
Signed-off-by: Rasmus Villemoes rasmus.villemoes@prevas.dk --- common/command.c | 2 +- include/command.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/common/command.c b/common/command.c index 846e16e2ada..474ac98bc38 100644 --- a/common/command.c +++ b/common/command.c @@ -466,7 +466,7 @@ int cmd_auto_complete(const char *const prompt, char *buf, int *np, int *colp) #endif
#ifdef CMD_DATA_SIZE -int cmd_get_data_size(char* arg, int default_size) +int cmd_get_data_size(const char *arg, int default_size) { /* Check for a size specification .b, .w or .l. */ diff --git a/include/command.h b/include/command.h index 6262365e128..6ea678fbbe6 100644 --- a/include/command.h +++ b/include/command.h @@ -153,7 +153,7 @@ int cmd_process_error(struct cmd_tbl *cmdtp, int err); * Return: data size in bytes (1, 2, 4, 8) or CMD_DATA_SIZE_ERR for an invalid * character, or CMD_DATA_SIZE_STR for a string */ -int cmd_get_data_size(char *arg, int default_size); +int cmd_get_data_size(const char *arg, int default_size); #endif
#ifdef CONFIG_CMD_BOOTD
Just check that the length is at least 2. This allows passing strings like ".b", which can be convenient when constructing tests (i.e. parametrizing the suffix used).
Signed-off-by: Rasmus Villemoes rasmus.villemoes@prevas.dk --- common/command.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/common/command.c b/common/command.c index 474ac98bc38..f0354f41644 100644 --- a/common/command.c +++ b/common/command.c @@ -471,7 +471,7 @@ int cmd_get_data_size(const char *arg, int default_size) /* Check for a size specification .b, .w or .l. */ int len = strlen(arg); - if (len > 2 && arg[len-2] == '.') { + if (len >= 2 && arg[len-2] == '.') { switch (arg[len-1]) { case 'b': return 1;
Signed-off-by: Rasmus Villemoes rasmus.villemoes@prevas.dk --- test/cmd/Makefile | 1 + test/cmd/mem_copy.c | 169 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 170 insertions(+) create mode 100644 test/cmd/mem_copy.c
diff --git a/test/cmd/Makefile b/test/cmd/Makefile index e296ba1192b..b26b5b1ea85 100644 --- a/test/cmd/Makefile +++ b/test/cmd/Makefile @@ -17,6 +17,7 @@ obj-$(CONFIG_CONSOLE_TRUETYPE) += font.o obj-$(CONFIG_CMD_HISTORY) += history.o obj-$(CONFIG_CMD_LOADM) += loadm.o obj-$(CONFIG_CMD_MEM_SEARCH) += mem_search.o +obj-$(CONFIG_CMD_MEMORY) += mem_copy.o ifdef CONFIG_CMD_PCI obj-$(CONFIG_CMD_PCI_MPS) += pci_mps.o endif diff --git a/test/cmd/mem_copy.c b/test/cmd/mem_copy.c new file mode 100644 index 00000000000..072159b2906 --- /dev/null +++ b/test/cmd/mem_copy.c @@ -0,0 +1,169 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Tests for memory 'cp' command + */ + +#include <command.h> +#include <common.h> +#include <console.h> +#include <mapmem.h> +#include <dm/test.h> +#include <test/ut.h> + +#define BUF_SIZE 256 + +/* Declare a new mem test */ +#define MEM_TEST(_name) UNIT_TEST(_name, 0, mem_test) + +struct param { + int d, s, count; +}; + +static int do_test(struct unit_test_state *uts, + const char *suffix, int d, int s, int count) +{ + const long addr = 0x1000; + u8 shadow[BUF_SIZE]; + u8 *buf; + int i, w, bytes; + + buf = map_sysmem(addr, BUF_SIZE); + + /* Fill with distinct bytes. */ + for (i = 0; i < BUF_SIZE; ++i) + buf[i] = shadow[i] = i; + + /* Parameter sanity checking. */ + w = cmd_get_data_size(suffix, 4); + ut_assert(w == 1 || w == 2 || w == 4 || (MEM_SUPPORT_64BIT_DATA && w == 8)); + + bytes = count * w; + ut_assert(d < BUF_SIZE); + ut_assert(d + bytes <= BUF_SIZE); + ut_assert(s < BUF_SIZE); + ut_assert(s + bytes <= BUF_SIZE); + + /* This is exactly what we expect to happen to "buf" */ + memmove(shadow + d, shadow + s, bytes); + + run_commandf("cp%s 0x%lx 0x%lx 0x%x", suffix, addr + s, addr + d, count); + + ut_asserteq(0, memcmp(buf, shadow, BUF_SIZE)); + + unmap_sysmem(buf); + + return 0; +} + +static int mem_test_cp_b(struct unit_test_state *uts) +{ + static const struct param tests[] = { + { 0, 128, 128 }, + { 128, 0, 128 }, + { 0, 16, 32 }, + { 16, 0, 32 }, + { 60, 100, 100 }, + { 100, 60, 100 }, + { 123, 54, 96 }, + { 54, 123, 96 }, + }; + const struct param *p; + int ret, i; + + for (i = 0; i < ARRAY_SIZE(tests); ++i) { + p = &tests[i]; + ret = do_test(uts, ".b", p->d, p->s, p->count); + if (ret) + return ret; + } + + return 0; +} +MEM_TEST(mem_test_cp_b); + +static int mem_test_cp_w(struct unit_test_state *uts) +{ + static const struct param tests[] = { + { 0, 128, 64 }, + { 128, 0, 64 }, + { 0, 16, 16 }, + { 16, 0, 16 }, + { 60, 100, 50 }, + { 100, 60, 50 }, + { 123, 54, 48 }, + { 54, 123, 48 }, + }; + const struct param *p; + int ret, i; + + for (i = 0; i < ARRAY_SIZE(tests); ++i) { + p = &tests[i]; + ret = do_test(uts, ".w", p->d, p->s, p->count); + if (ret) + return ret; + } + + return 0; +} +MEM_TEST(mem_test_cp_w); + +static int mem_test_cp_l(struct unit_test_state *uts) +{ + static const struct param tests[] = { + { 0, 128, 32 }, + { 128, 0, 32 }, + { 0, 16, 8 }, + { 16, 0, 8 }, + { 60, 100, 25 }, + { 100, 60, 25 }, + { 123, 54, 24 }, + { 54, 123, 24 }, + }; + const struct param *p; + int ret, i; + + for (i = 0; i < ARRAY_SIZE(tests); ++i) { + p = &tests[i]; + ret = do_test(uts, ".l", p->d, p->s, p->count); + if (ret) + return ret; + } + + for (i = 0; i < ARRAY_SIZE(tests); ++i) { + p = &tests[i]; + ret = do_test(uts, "", p->d, p->s, p->count); + if (ret) + return ret; + } + + return 0; +} +MEM_TEST(mem_test_cp_l); + +#if MEM_SUPPORT_64BIT_DATA +static int mem_test_cp_q(struct unit_test_state *uts) +{ + static const struct param tests[] = { + { 0, 128, 16 }, + { 128, 0, 16 }, + { 0, 16, 8 }, + { 16, 0, 8 }, + { 60, 100, 15 }, + { 100, 60, 15 }, + { 123, 54, 12 }, + { 54, 123, 12 }, + }; + const struct param *p; + int ret, i; + + for (i = 0; i < ARRAY_SIZE(tests); ++i) { + p = &tests[i]; + ret = do_test(uts, ".q", p->d, p->s, p->count); + if (ret) + return ret; + } + + return 0; +} +MEM_TEST(mem_test_cp_q); +#endif
The macro MEM_SUPPORT_64BIT_DATA is always defined, as either 1 or 0, so using "#ifdef MEM_SUPPORT_64BIT_DATA" doesn't do what one expects.
This means that currently all 32 bit targets get compiled with the .q suffix mentioned in the help text, while it doesn't actually work.
Use the proper "#if" instead.
There's really no point defining another similarly-named macro with exactly the same value, so just use MEM_SUPPORT_64BIT_DATA throughout.
Signed-off-by: Rasmus Villemoes rasmus.villemoes@prevas.dk --- cmd/mem.c | 38 ++++++++++++++++++-------------------- 1 file changed, 18 insertions(+), 20 deletions(-)
diff --git a/cmd/mem.c b/cmd/mem.c index c696b92a274..768057e4d3f 100644 --- a/cmd/mem.c +++ b/cmd/mem.c @@ -35,11 +35,9 @@ DECLARE_GLOBAL_DATA_PTR;
/* Create a compile-time value */ -#ifdef MEM_SUPPORT_64BIT_DATA -#define SUPPORT_64BIT_DATA 1 +#if MEM_SUPPORT_64BIT_DATA #define HELP_Q ", .q" #else -#define SUPPORT_64BIT_DATA 0 #define HELP_Q "" #endif
@@ -131,7 +129,7 @@ static int do_mem_nm(struct cmd_tbl *cmdtp, int flag, int argc, static int do_mem_mw(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) { - ulong writeval; /* 64-bit if SUPPORT_64BIT_DATA */ + ulong writeval; /* 64-bit if MEM_SUPPORT_64BIT_DATA */ ulong addr, count; int size; void *buf, *start; @@ -152,7 +150,7 @@ static int do_mem_mw(struct cmd_tbl *cmdtp, int flag, int argc,
/* Get the value to write. */ - if (SUPPORT_64BIT_DATA) + if (MEM_SUPPORT_64BIT_DATA) writeval = simple_strtoull(argv[2], NULL, 16); else writeval = hextoul(argv[2], NULL); @@ -170,7 +168,7 @@ static int do_mem_mw(struct cmd_tbl *cmdtp, int flag, int argc, while (count-- > 0) { if (size == 4) *((u32 *)buf) = (u32)writeval; - else if (SUPPORT_64BIT_DATA && size == 8) + else if (MEM_SUPPORT_64BIT_DATA && size == 8) *((ulong *)buf) = writeval; else if (size == 2) *((u16 *)buf) = (u16)writeval; @@ -248,7 +246,7 @@ static int do_mem_cmp(struct cmd_tbl *cmdtp, int flag, int argc, int rcode = 0; const char *type; const void *buf1, *buf2, *base; - ulong word1, word2; /* 64-bit if SUPPORT_64BIT_DATA */ + ulong word1, word2; /* 64-bit if MEM_SUPPORT_64BIT_DATA */
if (argc != 4) return CMD_RET_USAGE; @@ -276,7 +274,7 @@ static int do_mem_cmp(struct cmd_tbl *cmdtp, int flag, int argc, if (size == 4) { word1 = *(u32 *)buf1; word2 = *(u32 *)buf2; - } else if (SUPPORT_64BIT_DATA && size == 8) { + } else if (MEM_SUPPORT_64BIT_DATA && size == 8) { word1 = *(ulong *)buf1; word2 = *(ulong *)buf2; } else if (size == 2) { @@ -528,7 +526,7 @@ static int do_mem_loop(struct cmd_tbl *cmdtp, int flag, int argc, { ulong addr, length, i, bytes; int size; - volatile ulong *llp; /* 64-bit if SUPPORT_64BIT_DATA */ + volatile ulong *llp; /* 64-bit if MEM_SUPPORT_64BIT_DATA */ volatile u32 *longp; volatile u16 *shortp; volatile u8 *cp; @@ -559,7 +557,7 @@ static int do_mem_loop(struct cmd_tbl *cmdtp, int flag, int argc, * If we have only one object, just run infinite loops. */ if (length == 1) { - if (SUPPORT_64BIT_DATA && size == 8) { + if (MEM_SUPPORT_64BIT_DATA && size == 8) { llp = (ulong *)buf; for (;;) i = *llp; @@ -579,7 +577,7 @@ static int do_mem_loop(struct cmd_tbl *cmdtp, int flag, int argc, i = *cp; }
- if (SUPPORT_64BIT_DATA && size == 8) { + if (MEM_SUPPORT_64BIT_DATA && size == 8) { for (;;) { llp = (ulong *)buf; i = length; @@ -620,8 +618,8 @@ static int do_mem_loopw(struct cmd_tbl *cmdtp, int flag, int argc, { ulong addr, length, i, bytes; int size; - volatile ulong *llp; /* 64-bit if SUPPORT_64BIT_DATA */ - ulong data; /* 64-bit if SUPPORT_64BIT_DATA */ + volatile ulong *llp; /* 64-bit if MEM_SUPPORT_64BIT_DATA */ + ulong data; /* 64-bit if MEM_SUPPORT_64BIT_DATA */ volatile u32 *longp; volatile u16 *shortp; volatile u8 *cp; @@ -646,7 +644,7 @@ static int do_mem_loopw(struct cmd_tbl *cmdtp, int flag, int argc, length = hextoul(argv[2], NULL);
/* data to write */ - if (SUPPORT_64BIT_DATA) + if (MEM_SUPPORT_64BIT_DATA) data = simple_strtoull(argv[3], NULL, 16); else data = hextoul(argv[3], NULL); @@ -658,7 +656,7 @@ static int do_mem_loopw(struct cmd_tbl *cmdtp, int flag, int argc, * If we have only one object, just run infinite loops. */ if (length == 1) { - if (SUPPORT_64BIT_DATA && size == 8) { + if (MEM_SUPPORT_64BIT_DATA && size == 8) { llp = (ulong *)buf; for (;;) *llp = data; @@ -678,7 +676,7 @@ static int do_mem_loopw(struct cmd_tbl *cmdtp, int flag, int argc, *cp = data; }
- if (SUPPORT_64BIT_DATA && size == 8) { + if (MEM_SUPPORT_64BIT_DATA && size == 8) { for (;;) { llp = (ulong *)buf; i = length; @@ -1151,7 +1149,7 @@ mod_mem(struct cmd_tbl *cmdtp, int incrflag, int flag, int argc, char *const argv[]) { ulong addr; - ulong i; /* 64-bit if SUPPORT_64BIT_DATA */ + ulong i; /* 64-bit if MEM_SUPPORT_64BIT_DATA */ int nbytes, size; void *ptr = NULL;
@@ -1186,7 +1184,7 @@ mod_mem(struct cmd_tbl *cmdtp, int incrflag, int flag, int argc, printf("%08lx:", addr); if (size == 4) printf(" %08x", *((u32 *)ptr)); - else if (SUPPORT_64BIT_DATA && size == 8) + else if (MEM_SUPPORT_64BIT_DATA && size == 8) printf(" %0lx", *((ulong *)ptr)); else if (size == 2) printf(" %04x", *((u16 *)ptr)); @@ -1211,7 +1209,7 @@ mod_mem(struct cmd_tbl *cmdtp, int incrflag, int flag, int argc, #endif else { char *endp; - if (SUPPORT_64BIT_DATA) + if (MEM_SUPPORT_64BIT_DATA) i = simple_strtoull(console_buffer, &endp, 16); else i = hextoul(console_buffer, &endp); @@ -1222,7 +1220,7 @@ mod_mem(struct cmd_tbl *cmdtp, int incrflag, int flag, int argc, bootretry_reset_cmd_timeout(); if (size == 4) *((u32 *)ptr) = i; - else if (SUPPORT_64BIT_DATA && size == 8) + else if (MEM_SUPPORT_64BIT_DATA && size == 8) *((ulong *)ptr) = i; else if (size == 2) *((u16 *)ptr) = i;
The first sentence is half-way true; the macro is always defined, but has the value 0 or 1.
The second is outright false. A lot of code guarded by MEM_SUPPORT_64BIT_DATA uses a "ulong" to store values, so if sizeof(long) is not 8, that code would probably compile, but not work at all as expected.
It would probably be possible to make all such code explicitly use u64 and thus make it work on 32 bit targets, but until that is done, do not pretend that it's ok to override the automatic value of MEM_SUPPORT_64BIT_DATA.
Signed-off-by: Rasmus Villemoes rasmus.villemoes@prevas.dk --- README | 3 --- 1 file changed, 3 deletions(-)
diff --git a/README b/README index 60c6b8a19db..bc7fb474348 100644 --- a/README +++ b/README @@ -1248,9 +1248,6 @@ typically in board_init_f() and board_init_r(). Configuration Settings: -----------------------
-- MEM_SUPPORT_64BIT_DATA: Defined automatically if compiled as 64-bit. - Optionally it can be defined to support 64-bit memory commands. - - CONFIG_SYS_LONGHELP: Defined when you want long help messages included; undefine this when you're short of memory.
Such a config option does not exist. Rephrase, and avoid mentioning MEM_SUPPORT_64BIT_DATA, which is an implementation detail.
Signed-off-by: Rasmus Villemoes rasmus.villemoes@prevas.dk --- doc/usage/cmd/cmp.rst | 2 +- doc/usage/cmd/cp.rst | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/doc/usage/cmd/cmp.rst b/doc/usage/cmd/cmp.rst index 8d196ee5786..66865ebd7ee 100644 --- a/doc/usage/cmd/cmp.rst +++ b/doc/usage/cmd/cmp.rst @@ -96,7 +96,7 @@ Configuration -------------
The cmp command is only available if CONFIG_CMD_MEMORY=y. The cmp.q command is -only available if additionally CONFIG_MEM_SUPPORT_64BIT_DATA=y. +only available on 64-bit targets.
Return value ------------ diff --git a/doc/usage/cmd/cp.rst b/doc/usage/cmd/cp.rst index 67360e30e41..bea379ff360 100644 --- a/doc/usage/cmd/cp.rst +++ b/doc/usage/cmd/cp.rst @@ -74,7 +74,7 @@ Configuration -------------
The cp command is available if CONFIG_CMD_MEMORY=y. Support for 64 bit words -(cp.q) depends on CONFIG_MEM_SUPPORT_64BIT_DATA=y. Copying to flash depends on +(cp.q) is only available on 64-bit targets. Copying to flash depends on CONFIG_MTD_NOR_FLASH=y.
Return value
participants (2)
-
Rasmus Villemoes -
Tom Rini