[PATCH 0/2] improve 'read' command, add 'write' command

[PATCH 0/2] improve 'read' command, add 'write' command

[Rasmus Villemoes]

The first patch simplies do_read somewhat by making use of an existing
helper instead of parsing the dev_part string manually. As a bonus
(and my actual motivation), it now understands dev#partname syntax -
hard-coded partition numbers are so last decade.

I also need the symmetrical operation, being able to write to a named
raw partition, and fortunately it doesn't require that many lines of
code to implement that.

There's a very minor change in the error reporting due to using
cmdtp->name to generate the new messages, but I don't think "Error
reading blocks" offers much that "read error" doesn't.



Rasmus Villemoes (2):
  cmd: read: use part_get_info_by_dev_and_name_or_num() instead of
    open-coded dev_part parsing
  cmd: introduce 'write' command

 cmd/Kconfig  |  5 +++++
 cmd/Makefile |  1 +
 cmd/read.c   | 61 ++++++++++++++++++++++++++--------------------------
 3 files changed, 36 insertions(+), 31 deletions(-)

-- 
2.37.2

[PATCH 1/2] cmd: read: use part_get_info_by_dev_and_name_or_num() instead of open-coded dev_part parsing

[Rasmus Villemoes]

Use the helper part_get_info_by_dev_and_name_or_num() for parsing a
dev[:part] string and obtaining the partition info in one go, instead
of open-coding all that.

As a bonus, this will automatically allow using the dev#partname
syntax as well, for accessing raw partitions by name.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 cmd/read.c | 32 ++++++++------------------------
 1 file changed, 8 insertions(+), 24 deletions(-)

diff --git a/cmd/read.c b/cmd/read.c
index fecfadaa1f..8645db49bb 100644
--- a/cmd/read.c
+++ b/cmd/read.c
@@ -15,50 +15,34 @@
 
 int do_read(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 {
-	char *ep;
 	struct blk_desc *dev_desc = NULL;
-	int dev;
-	int part = 0;
 	struct disk_partition part_info;
-	ulong offset = 0u;
-	ulong limit = 0u;
+	ulong offset, limit;
 	void *addr;
 	uint blk;
 	uint cnt;
+	int part;
 
 	if (argc != 6) {
 		cmd_usage(cmdtp);
 		return 1;
 	}
 
-	dev = (int)hextoul(argv[2], &ep);
-	if (*ep) {
-		if (*ep != ':') {
-			printf("Invalid block device %s\n", argv[2]);
-			return 1;
-		}
-		part = (int)hextoul(++ep, NULL);
-	}
-
-	dev_desc = blk_get_dev(argv[1], dev);
-	if (dev_desc == NULL) {
-		printf("Block device %s %d not supported\n", argv[1], dev);
+	part = part_get_info_by_dev_and_name_or_num(argv[1], argv[2],
+						    &dev_desc, &part_info, 1);
+	if (part < 0)
 		return 1;
-	}
 
 	addr = map_sysmem(hextoul(argv[3], NULL), 0);
 	blk = hextoul(argv[4], NULL);
 	cnt = hextoul(argv[5], NULL);
 
-	if (part != 0) {
-		if (part_get_info(dev_desc, part, &part_info)) {
-			printf("Cannot find partition %d\n", part);
-			return 1;
-		}
+	if (part > 0) {
 		offset = part_info.start;
 		limit = part_info.size;
 	} else {
 		/* Largest address not available in struct blk_desc. */
+		offset = 0;
 		limit = ~0;
 	}
 
@@ -78,5 +62,5 @@ int do_read(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 U_BOOT_CMD(
 	read,	6,	0,	do_read,
 	"Load binary data from a partition",
-	"<interface> <dev[:part]> addr blk# cnt"
+	"<interface> <dev[:part|#partname]> addr blk# cnt"
 );
-- 
2.37.2

[PATCH 2/2] cmd: introduce 'write' command

[Rasmus Villemoes]

It's almost no extra code to hook up a buddy to the 'read' command. In
fact, since the command is passed its own 'struct cmd_tbl', we can use
the exact same callback, and let it figure out for itself whether it
was invoked as "read" or "write".

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 cmd/Kconfig  |  5 +++++
 cmd/Makefile |  1 +
 cmd/read.c   | 29 ++++++++++++++++++++++-------
 3 files changed, 28 insertions(+), 7 deletions(-)

diff --git a/cmd/Kconfig b/cmd/Kconfig
index 4fe2c75de2..e87796c538 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -1556,6 +1556,11 @@ config CMD_WDT
 	help
 	  This provides commands to control the watchdog timer devices.
 
+config CMD_WRITE
+	bool "write - Write binary data to a partition"
+	help
+	  Provides low-level write access to a partition.
+
 config CMD_AXI
 	bool "axi"
 	depends on AXI
diff --git a/cmd/Makefile b/cmd/Makefile
index 0b6a96c1d9..cfbabb9ef6 100644
--- a/cmd/Makefile
+++ b/cmd/Makefile
@@ -142,6 +142,7 @@ obj-$(CONFIG_CMD_PXE) += pxe.o
 obj-$(CONFIG_CMD_WOL) += wol.o
 obj-$(CONFIG_CMD_QFW) += qfw.o
 obj-$(CONFIG_CMD_READ) += read.o
+obj-$(CONFIG_CMD_WRITE) += read.o
 obj-$(CONFIG_CMD_REGINFO) += reginfo.o
 obj-$(CONFIG_CMD_REISER) += reiser.o
 obj-$(CONFIG_CMD_REMOTEPROC) += remoteproc.o
diff --git a/cmd/read.c b/cmd/read.c
index 8645db49bb..1218e7acfd 100644
--- a/cmd/read.c
+++ b/cmd/read.c
@@ -13,14 +13,14 @@
 #include <mapmem.h>
 #include <part.h>
 
-int do_read(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
+static int
+do_rw(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 {
 	struct blk_desc *dev_desc = NULL;
 	struct disk_partition part_info;
 	ulong offset, limit;
+	uint blk, cnt, res;
 	void *addr;
-	uint blk;
-	uint cnt;
 	int part;
 
 	if (argc != 6) {
@@ -47,20 +47,35 @@ int do_read(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 	}
 
 	if (cnt + blk > limit) {
-		printf("Read out of range\n");
+		printf("%s out of range\n", cmdtp->name);
 		return 1;
 	}
 
-	if (blk_dread(dev_desc, offset + blk, cnt, addr) != cnt) {
-		printf("Error reading blocks\n");
+	if (IS_ENABLED(CONFIG_CMD_WRITE) && !strcmp(cmdtp->name, "write"))
+		res = blk_dwrite(dev_desc, offset + blk, cnt, addr);
+	else
+		res = blk_dread(dev_desc, offset + blk, cnt, addr);
+
+	if (res != cnt) {
+		printf("%s error\n", cmdtp->name);
 		return 1;
 	}
 
 	return 0;
 }
 
+#ifdef CONFIG_CMD_READ
 U_BOOT_CMD(
-	read,	6,	0,	do_read,
+	read,	6,	0,	do_rw,
 	"Load binary data from a partition",
 	"<interface> <dev[:part|#partname]> addr blk# cnt"
 );
+#endif
+
+#ifdef CONFIG_CMD_WRITE
+U_BOOT_CMD(
+	write,	6,	0,	do_rw,
+	"Store binary data to a partition",
+	"<interface> <dev[:part|#partname]> addr blk# cnt"
+);
+#endif
-- 
2.37.2

Re: [PATCH 2/2] cmd: introduce 'write' command

[Simon Glass]

Hi Rasmus,

On Fri, 17 Feb 2023 at 17:59, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> It's almost no extra code to hook up a buddy to the 'read' command. In
> fact, since the command is passed its own 'struct cmd_tbl', we can use
> the exact same callback, and let it figure out for itself whether it
> was invoked as "read" or "write".
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  cmd/Kconfig  |  5 +++++
>  cmd/Makefile |  1 +
>  cmd/read.c   | 29 ++++++++++++++++++++++-------
>  3 files changed, 28 insertions(+), 7 deletions(-)

Great, but please do add a little more code :-)

- doc/usage/cmd
- test/cmd/write.c

For the test you can use sandbox's mmc0 - see dm_test_mmc_blk() for an
example of a test that does that. For checking the command response,
the acpi.c test is an example, although I suppose there is actually no
output.

Regards,
Simon

Re: [PATCH 1/2] cmd: read: use part_get_info_by_dev_and_name_or_num() instead of open-coded dev_part parsing

[Simon Glass]

On Fri, 17 Feb 2023 at 17:59, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> Use the helper part_get_info_by_dev_and_name_or_num() for parsing a
> dev[:part] string and obtaining the partition info in one go, instead
> of open-coding all that.
>
> As a bonus, this will automatically allow using the dev#partname
> syntax as well, for accessing raw partitions by name.
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  cmd/read.c | 32 ++++++++------------------------
>  1 file changed, 8 insertions(+), 24 deletions(-)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH v2 0/5] improve 'read' command, add 'write' command

[Rasmus Villemoes]

The first patch simplies do_read somewhat by making use of an existing
helper instead of parsing the dev_part string manually. As a bonus
(and my actual motivation), it now understands dev#partname syntax -
hard-coded partition numbers are so last decade.

I also need the symmetrical operation, being able to write to a named
raw partition, and fortunately it doesn't require that many lines of
code to implement that.

There's a very minor change in the error reporting due to using
cmdtp->name to generate the new messages, but I don't think "Error
reading blocks" offers much that "read error" doesn't.

New in v2: the last three patches add documentation, ensure CMD_WRITE
is set for sandbox and adds some basic test cases for the various ways
of accessing the partitions (by number, name, or as raw offset within
the whole disk).

Rasmus Villemoes (5):
  cmd: read: use part_get_info_by_dev_and_name_or_num() instead of
    open-coded dev_part parsing
  cmd: introduce 'write' command
  doc: document read/write commands
  sandbox: enable CMD_WRITE
  test: add tests of 'read' and 'write' shell commands

 cmd/Kconfig                 |   5 ++
 cmd/Makefile                |   1 +
 cmd/read.c                  |  61 +++++++++++----------
 configs/sandbox64_defconfig |   1 +
 configs/sandbox_defconfig   |   1 +
 doc/usage/cmd/read.rst      |  40 ++++++++++++++
 doc/usage/index.rst         |   1 +
 test/cmd/Makefile           |   1 +
 test/cmd/rw.c               | 104 ++++++++++++++++++++++++++++++++++++
 9 files changed, 184 insertions(+), 31 deletions(-)
 create mode 100644 doc/usage/cmd/read.rst
 create mode 100644 test/cmd/rw.c

-- 
2.37.2

[PATCH v2 1/5] cmd: read: use part_get_info_by_dev_and_name_or_num() instead of open-coded dev_part parsing

[Rasmus Villemoes]

Use the helper part_get_info_by_dev_and_name_or_num() for parsing a
dev[:part] string and obtaining the partition info in one go, instead
of open-coding all that.

As a bonus, this will automatically allow using the dev#partname
syntax as well, for accessing raw partitions by name.

Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 cmd/read.c | 32 ++++++++------------------------
 1 file changed, 8 insertions(+), 24 deletions(-)

diff --git a/cmd/read.c b/cmd/read.c
index fecfadaa1f..8645db49bb 100644
--- a/cmd/read.c
+++ b/cmd/read.c
@@ -15,50 +15,34 @@
 
 int do_read(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 {
-	char *ep;
 	struct blk_desc *dev_desc = NULL;
-	int dev;
-	int part = 0;
 	struct disk_partition part_info;
-	ulong offset = 0u;
-	ulong limit = 0u;
+	ulong offset, limit;
 	void *addr;
 	uint blk;
 	uint cnt;
+	int part;
 
 	if (argc != 6) {
 		cmd_usage(cmdtp);
 		return 1;
 	}
 
-	dev = (int)hextoul(argv[2], &ep);
-	if (*ep) {
-		if (*ep != ':') {
-			printf("Invalid block device %s\n", argv[2]);
-			return 1;
-		}
-		part = (int)hextoul(++ep, NULL);
-	}
-
-	dev_desc = blk_get_dev(argv[1], dev);
-	if (dev_desc == NULL) {
-		printf("Block device %s %d not supported\n", argv[1], dev);
+	part = part_get_info_by_dev_and_name_or_num(argv[1], argv[2],
+						    &dev_desc, &part_info, 1);
+	if (part < 0)
 		return 1;
-	}
 
 	addr = map_sysmem(hextoul(argv[3], NULL), 0);
 	blk = hextoul(argv[4], NULL);
 	cnt = hextoul(argv[5], NULL);
 
-	if (part != 0) {
-		if (part_get_info(dev_desc, part, &part_info)) {
-			printf("Cannot find partition %d\n", part);
-			return 1;
-		}
+	if (part > 0) {
 		offset = part_info.start;
 		limit = part_info.size;
 	} else {
 		/* Largest address not available in struct blk_desc. */
+		offset = 0;
 		limit = ~0;
 	}
 
@@ -78,5 +62,5 @@ int do_read(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 U_BOOT_CMD(
 	read,	6,	0,	do_read,
 	"Load binary data from a partition",
-	"<interface> <dev[:part]> addr blk# cnt"
+	"<interface> <dev[:part|#partname]> addr blk# cnt"
 );
-- 
2.37.2

[PATCH v2 2/5] cmd: introduce 'write' command

[Rasmus Villemoes]

It's almost no extra code to hook up a buddy to the 'read' command. In
fact, since the command is passed its own 'struct cmd_tbl', we can use
the exact same callback, and let it figure out for itself whether it
was invoked as "read" or "write".

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 cmd/Kconfig  |  5 +++++
 cmd/Makefile |  1 +
 cmd/read.c   | 29 ++++++++++++++++++++++-------
 3 files changed, 28 insertions(+), 7 deletions(-)

diff --git a/cmd/Kconfig b/cmd/Kconfig
index 2caa4af71c..008ae55e02 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -1562,6 +1562,11 @@ config CMD_WDT
 	help
 	  This provides commands to control the watchdog timer devices.
 
+config CMD_WRITE
+	bool "write - Write binary data to a partition"
+	help
+	  Provides low-level write access to a partition.
+
 config CMD_AXI
 	bool "axi"
 	depends on AXI
diff --git a/cmd/Makefile b/cmd/Makefile
index 36d2daf22a..f2429c18ab 100644
--- a/cmd/Makefile
+++ b/cmd/Makefile
@@ -140,6 +140,7 @@ obj-$(CONFIG_CMD_PXE) += pxe.o
 obj-$(CONFIG_CMD_WOL) += wol.o
 obj-$(CONFIG_CMD_QFW) += qfw.o
 obj-$(CONFIG_CMD_READ) += read.o
+obj-$(CONFIG_CMD_WRITE) += read.o
 obj-$(CONFIG_CMD_REGINFO) += reginfo.o
 obj-$(CONFIG_CMD_REISER) += reiser.o
 obj-$(CONFIG_CMD_REMOTEPROC) += remoteproc.o
diff --git a/cmd/read.c b/cmd/read.c
index 8645db49bb..1218e7acfd 100644
--- a/cmd/read.c
+++ b/cmd/read.c
@@ -13,14 +13,14 @@
 #include <mapmem.h>
 #include <part.h>
 
-int do_read(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
+static int
+do_rw(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 {
 	struct blk_desc *dev_desc = NULL;
 	struct disk_partition part_info;
 	ulong offset, limit;
+	uint blk, cnt, res;
 	void *addr;
-	uint blk;
-	uint cnt;
 	int part;
 
 	if (argc != 6) {
@@ -47,20 +47,35 @@ int do_read(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 	}
 
 	if (cnt + blk > limit) {
-		printf("Read out of range\n");
+		printf("%s out of range\n", cmdtp->name);
 		return 1;
 	}
 
-	if (blk_dread(dev_desc, offset + blk, cnt, addr) != cnt) {
-		printf("Error reading blocks\n");
+	if (IS_ENABLED(CONFIG_CMD_WRITE) && !strcmp(cmdtp->name, "write"))
+		res = blk_dwrite(dev_desc, offset + blk, cnt, addr);
+	else
+		res = blk_dread(dev_desc, offset + blk, cnt, addr);
+
+	if (res != cnt) {
+		printf("%s error\n", cmdtp->name);
 		return 1;
 	}
 
 	return 0;
 }
 
+#ifdef CONFIG_CMD_READ
 U_BOOT_CMD(
-	read,	6,	0,	do_read,
+	read,	6,	0,	do_rw,
 	"Load binary data from a partition",
 	"<interface> <dev[:part|#partname]> addr blk# cnt"
 );
+#endif
+
+#ifdef CONFIG_CMD_WRITE
+U_BOOT_CMD(
+	write,	6,	0,	do_rw,
+	"Store binary data to a partition",
+	"<interface> <dev[:part|#partname]> addr blk# cnt"
+);
+#endif
-- 
2.37.2

[PATCH v2 3/5] doc: document read/write commands

[Rasmus Villemoes]

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 doc/usage/cmd/read.rst | 40 ++++++++++++++++++++++++++++++++++++++++
 doc/usage/index.rst    |  1 +
 2 files changed, 41 insertions(+)
 create mode 100644 doc/usage/cmd/read.rst

diff --git a/doc/usage/cmd/read.rst b/doc/usage/cmd/read.rst
new file mode 100644
index 0000000000..705d5a3e0c
--- /dev/null
+++ b/doc/usage/cmd/read.rst
@@ -0,0 +1,40 @@
+.. SPDX-License-Identifier: GPL-2.0+:
+
+read/write commands
+===================
+
+Synopsis
+--------
+
+::
+
+    read <interface> <dev[:part|#partname]> <addr> <blk#> <cnt>
+    write <interface> <dev[:part|#partname]> <addr> <blk#> <cnt>
+
+The read and write commands can be used for raw access to data in
+block devices (or partitions therein), i.e. without going through a
+file system.
+
+read
+----
+
+The block device is specified using the <interface> (e.g. "mmc") and
+<dev> parameters. If the block device has a partition table, one can
+optionally specify a partition number (using the :part syntax) or
+partition name (using the #partname syntax). The command then reads
+the <cnt> blocks of data starting at block number <blk#> of the given
+device/partition to the memory address <addr>.
+
+Examples:
+
+    # Read 2 MiB from partition 3 of mmc device 2 to $loadaddr
+    read mmc 2.3 $loadaddr 0 0x1000
+
+    # Read 16 MiB from the partition named 'kernel' of mmc device 1 to $loadaddr
+    read mmc 1#kernel $loadaddr 0 0x8000
+
+write
+-----
+
+The write command is completely equivalent to the read command, except
+of course that the transer direction is reversed.
diff --git a/doc/usage/index.rst b/doc/usage/index.rst
index 840c20c934..d6365cf8a4 100644
--- a/doc/usage/index.rst
+++ b/doc/usage/index.rst
@@ -71,6 +71,7 @@ Shell commands
    cmd/printenv
    cmd/pstore
    cmd/qfw
+   cmd/read
    cmd/reset
    cmd/rng
    cmd/sbi
-- 
2.37.2

[PATCH v2 4/5] sandbox: enable CMD_WRITE

[Rasmus Villemoes]

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 configs/sandbox64_defconfig | 1 +
 configs/sandbox_defconfig   | 1 +
 2 files changed, 2 insertions(+)

diff --git a/configs/sandbox64_defconfig b/configs/sandbox64_defconfig
index ccbc18aad0..b7737814af 100644
--- a/configs/sandbox64_defconfig
+++ b/configs/sandbox64_defconfig
@@ -58,6 +58,7 @@ CONFIG_CMD_SPI=y
 CONFIG_CMD_TEMPERATURE=y
 CONFIG_CMD_USB=y
 CONFIG_CMD_WDT=y
+CONFIG_CMD_WRITE=y
 CONFIG_CMD_CAT=y
 CONFIG_BOOTP_DNS2=y
 CONFIG_CMD_TFTPPUT=y
diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig
index 77ade1f1d8..daae539102 100644
--- a/configs/sandbox_defconfig
+++ b/configs/sandbox_defconfig
@@ -84,6 +84,7 @@ CONFIG_CMD_SPI=y
 CONFIG_CMD_TEMPERATURE=y
 CONFIG_CMD_USB=y
 CONFIG_CMD_WDT=y
+CONFIG_CMD_WRITE=y
 CONFIG_CMD_AXI=y
 CONFIG_CMD_CAT=y
 CONFIG_CMD_SETEXPR_FMT=y
-- 
2.37.2

[PATCH v2 5/5] test: add tests of 'read' and 'write' shell commands

[Rasmus Villemoes]

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 test/cmd/Makefile |   1 +
 test/cmd/rw.c     | 104 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 105 insertions(+)
 create mode 100644 test/cmd/rw.c

diff --git a/test/cmd/Makefile b/test/cmd/Makefile
index 2ffde8703a..7848f348bc 100644
--- a/test/cmd/Makefile
+++ b/test/cmd/Makefile
@@ -18,6 +18,7 @@ obj-$(CONFIG_CMD_PINMUX) += pinmux.o
 obj-$(CONFIG_CMD_PWM) += pwm.o
 obj-$(CONFIG_CMD_SEAMA) += seama.o
 ifdef CONFIG_SANDBOX
+obj-$(CONFIG_CMD_READ) += rw.o
 obj-$(CONFIG_CMD_SETEXPR) += setexpr.o
 endif
 obj-$(CONFIG_CMD_TEMPERATURE) += temperature.o
diff --git a/test/cmd/rw.c b/test/cmd/rw.c
new file mode 100644
index 0000000000..74c2fe5f31
--- /dev/null
+++ b/test/cmd/rw.c
@@ -0,0 +1,104 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Tests for read and write commands
+ */
+
+#include <common.h>
+#include <dm/test.h>
+#include <mapmem.h>
+#include <part.h>
+#include <test/test.h>
+#include <test/ut.h>
+
+static int setup_partitions(struct unit_test_state *uts, struct blk_desc **mmc_dev_desc)
+{
+	char str_disk_guid[UUID_STR_LEN + 1];
+	struct disk_partition parts[2] = {
+		{
+			.start = 48, /* GPT data takes up the first 34 blocks or so */
+			.size = 4,
+			.name = "data",
+		},
+		{
+			.start = 52,
+			.size = 10,
+			.name = "log",
+		},
+	};
+
+	ut_asserteq(2, blk_get_device_by_str("mmc", "2", mmc_dev_desc));
+	if (CONFIG_IS_ENABLED(RANDOM_UUID)) {
+		gen_rand_uuid_str(parts[0].uuid, UUID_STR_FORMAT_STD);
+		gen_rand_uuid_str(parts[1].uuid, UUID_STR_FORMAT_STD);
+		gen_rand_uuid_str(str_disk_guid, UUID_STR_FORMAT_STD);
+	}
+	ut_assertok(gpt_restore(*mmc_dev_desc, str_disk_guid, parts,
+				ARRAY_SIZE(parts)));
+	return 0;
+}
+
+/* Fill the write buffer with pseudo-random data, clear the read buffer. */
+static void init_buffers(char *rb, char *wb, size_t size, unsigned seed)
+{
+	memset(rb, 0, size);
+	while (size--) {
+		*wb++ = seed;
+		seed *= 43;
+		seed += 17 + size/4;
+	}
+}
+
+static int dm_test_read_write(struct unit_test_state *uts)
+{
+	struct blk_desc *dev_desc;
+	char wbuf[1024], rbuf[1024];
+	ulong wa, ra;
+
+#define INIT_BUFFERS() init_buffers(rbuf, wbuf, sizeof(rbuf), __LINE__)
+
+	ut_assertok(setup_partitions(uts, &dev_desc));
+
+	wa = map_to_sysmem(wbuf);
+	ra = map_to_sysmem(rbuf);
+
+	/* Simple test, write to/read from same partition. */
+	INIT_BUFFERS();
+	ut_assertok(run_commandf("write mmc 2:1 0x%lx 0 2", wa));
+	ut_assertok(run_commandf("read  mmc 2:1 0x%lx 0 2", ra));
+	ut_assertok(memcmp(wbuf, rbuf, sizeof(wbuf)));
+	ut_assertok(run_commandf("read  mmc 2:1 0x%lx 1 1", ra));
+	ut_assertok(memcmp(&wbuf[512], rbuf, 512));
+
+	/* Use name for write, number for read. */
+	INIT_BUFFERS();
+	ut_assertok(run_commandf("write mmc 2#log 0x%lx 0 2", wa));
+	ut_assertok(run_commandf("read  mmc 2:2   0x%lx 0 2", ra));
+	ut_assertok(memcmp(wbuf, rbuf, sizeof(wbuf)));
+	
+	/* Use full device for write, name for read. */
+	INIT_BUFFERS();
+	ut_assertok(run_commandf("write mmc 2:0    0x%lx 0x30 2", wa));
+	ut_assertok(run_commandf("read  mmc 2#data 0x%lx    0 2", ra));
+	ut_assertok(memcmp(wbuf, rbuf, sizeof(wbuf)));
+
+	/* Use name for write, full device for read */
+	INIT_BUFFERS();
+	ut_assertok(run_commandf("write mmc 2#log 0x%lx    1 2", wa));
+	ut_assertok(run_commandf("read  mmc 2:0   0x%lx 0x35 2", ra));
+	ut_assertok(memcmp(wbuf, rbuf, sizeof(wbuf)));
+
+	/* Read/write outside partition bounds should be rejected upfront. */
+	console_record_reset_enable();
+        ut_asserteq(1, run_commandf("read mmc 2#data 0x%lx 3 2", ra));
+        ut_assert_nextlinen("read out of range");
+        ut_assert_console_end();
+
+	console_record_reset_enable();
+        ut_asserteq(1, run_commandf("write mmc 2#log 0x%lx 9 2", wa));
+        ut_assert_nextlinen("write out of range");
+        ut_assert_console_end();
+
+	return 0;
+}
+
+DM_TEST(dm_test_read_write, UT_TESTF_SCAN_FDT | UT_TESTF_CONSOLE_REC);
-- 
2.37.2

Re: [PATCH v2 4/5] sandbox: enable CMD_WRITE

[Simon Glass]

On Wed, 1 Mar 2023 at 14:13, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  configs/sandbox64_defconfig | 1 +
>  configs/sandbox_defconfig   | 1 +
>  2 files changed, 2 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH v2 3/5] doc: document read/write commands

[Simon Glass]

Hi Rasmus,

On Wed, 1 Mar 2023 at 14:13, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  doc/usage/cmd/read.rst | 40 ++++++++++++++++++++++++++++++++++++++++
>  doc/usage/index.rst    |  1 +
>  2 files changed, 41 insertions(+)
>  create mode 100644 doc/usage/cmd/read.rst
>

Reviewed-by: Simon Glass <sjg@chromium.org>

Please add a commit msg

> diff --git a/doc/usage/cmd/read.rst b/doc/usage/cmd/read.rst
> new file mode 100644
> index 0000000000..705d5a3e0c
> --- /dev/null
> +++ b/doc/usage/cmd/read.rst
> @@ -0,0 +1,40 @@
> +.. SPDX-License-Identifier: GPL-2.0+:
> +
> +read/write commands
> +===================
> +
> +Synopsis
> +--------
> +
> +::
> +
> +    read <interface> <dev[:part|#partname]> <addr> <blk#> <cnt>
> +    write <interface> <dev[:part|#partname]> <addr> <blk#> <cnt>
> +
> +The read and write commands can be used for raw access to data in
> +block devices (or partitions therein), i.e. without going through a
> +file system.
> +
> +read
> +----
> +
> +The block device is specified using the <interface> (e.g. "mmc") and
> +<dev> parameters. If the block device has a partition table, one can
> +optionally specify a partition number (using the :part syntax) or
> +partition name (using the #partname syntax). The command then reads
> +the <cnt> blocks of data starting at block number <blk#> of the given
> +device/partition to the memory address <addr>.
> +
> +Examples:

For other commands the examples go at the bottom.

> +
> +    # Read 2 MiB from partition 3 of mmc device 2 to $loadaddr
> +    read mmc 2.3 $loadaddr 0 0x1000
> +
> +    # Read 16 MiB from the partition named 'kernel' of mmc device 1 to $loadaddr
> +    read mmc 1#kernel $loadaddr 0 0x8000
> +
> +write
> +-----

I think this needs its own file, with the material basically repeated
(perhaps even use an example which includes a read and a write?)

> +
> +The write command is completely equivalent to the read command, except
> +of course that the transer direction is reversed.

transfer

> diff --git a/doc/usage/index.rst b/doc/usage/index.rst
> index 840c20c934..d6365cf8a4 100644
> --- a/doc/usage/index.rst
> +++ b/doc/usage/index.rst
> @@ -71,6 +71,7 @@ Shell commands
>     cmd/printenv
>     cmd/pstore
>     cmd/qfw
> +   cmd/read
>     cmd/reset
>     cmd/rng
>     cmd/sbi
> --
> 2.37.2
>

Regards,
Simon

Re: [PATCH v2 2/5] cmd: introduce 'write' command

[Simon Glass]

On Wed, 1 Mar 2023 at 14:13, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> It's almost no extra code to hook up a buddy to the 'read' command. In
> fact, since the command is passed its own 'struct cmd_tbl', we can use
> the exact same callback, and let it figure out for itself whether it
> was invoked as "read" or "write".
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  cmd/Kconfig  |  5 +++++
>  cmd/Makefile |  1 +
>  cmd/read.c   | 29 ++++++++++++++++++++++-------
>  3 files changed, 28 insertions(+), 7 deletions(-)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH v2 5/5] test: add tests of 'read' and 'write' shell commands

[Simon Glass]

On Wed, 1 Mar 2023 at 14:13, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  test/cmd/Makefile |   1 +
>  test/cmd/rw.c     | 104 ++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 105 insertions(+)
>  create mode 100644 test/cmd/rw.c

Reviewed-by: Simon Glass <sjg@chromium.org>

nit below

>
> diff --git a/test/cmd/Makefile b/test/cmd/Makefile
> index 2ffde8703a..7848f348bc 100644
> --- a/test/cmd/Makefile
> +++ b/test/cmd/Makefile
> @@ -18,6 +18,7 @@ obj-$(CONFIG_CMD_PINMUX) += pinmux.o
>  obj-$(CONFIG_CMD_PWM) += pwm.o
>  obj-$(CONFIG_CMD_SEAMA) += seama.o
>  ifdef CONFIG_SANDBOX
> +obj-$(CONFIG_CMD_READ) += rw.o
>  obj-$(CONFIG_CMD_SETEXPR) += setexpr.o
>  endif
>  obj-$(CONFIG_CMD_TEMPERATURE) += temperature.o
> diff --git a/test/cmd/rw.c b/test/cmd/rw.c
> new file mode 100644
> index 0000000000..74c2fe5f31
> --- /dev/null
> +++ b/test/cmd/rw.c
> @@ -0,0 +1,104 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Tests for read and write commands
> + */
> +
> +#include <common.h>
> +#include <dm/test.h>
> +#include <mapmem.h>
> +#include <part.h>
> +#include <test/test.h>
> +#include <test/ut.h>
> +
> +static int setup_partitions(struct unit_test_state *uts, struct blk_desc **mmc_dev_desc)
> +{
> +       char str_disk_guid[UUID_STR_LEN + 1];
> +       struct disk_partition parts[2] = {
> +               {
> +                       .start = 48, /* GPT data takes up the first 34 blocks or so */
> +                       .size = 4,
> +                       .name = "data",
> +               },
> +               {
> +                       .start = 52,
> +                       .size = 10,
> +                       .name = "log",
> +               },
> +       };
> +
> +       ut_asserteq(2, blk_get_device_by_str("mmc", "2", mmc_dev_desc));
> +       if (CONFIG_IS_ENABLED(RANDOM_UUID)) {
> +               gen_rand_uuid_str(parts[0].uuid, UUID_STR_FORMAT_STD);
> +               gen_rand_uuid_str(parts[1].uuid, UUID_STR_FORMAT_STD);
> +               gen_rand_uuid_str(str_disk_guid, UUID_STR_FORMAT_STD);
> +       }
> +       ut_assertok(gpt_restore(*mmc_dev_desc, str_disk_guid, parts,
> +                               ARRAY_SIZE(parts)));
> +       return 0;
> +}
> +
> +/* Fill the write buffer with pseudo-random data, clear the read buffer. */
> +static void init_buffers(char *rb, char *wb, size_t size, unsigned seed)
> +{
> +       memset(rb, 0, size);
> +       while (size--) {
> +               *wb++ = seed;
> +               seed *= 43;
> +               seed += 17 + size/4;
> +       }
> +}
> +
> +static int dm_test_read_write(struct unit_test_state *uts)
> +{
> +       struct blk_desc *dev_desc;
> +       char wbuf[1024], rbuf[1024];
> +       ulong wa, ra;
> +
> +#define INIT_BUFFERS() init_buffers(rbuf, wbuf, sizeof(rbuf), __LINE__)
> +
> +       ut_assertok(setup_partitions(uts, &dev_desc));
> +
> +       wa = map_to_sysmem(wbuf);
> +       ra = map_to_sysmem(rbuf);
> +
> +       /* Simple test, write to/read from same partition. */
> +       INIT_BUFFERS();
> +       ut_assertok(run_commandf("write mmc 2:1 0x%lx 0 2", wa));
> +       ut_assertok(run_commandf("read  mmc 2:1 0x%lx 0 2", ra));
> +       ut_assertok(memcmp(wbuf, rbuf, sizeof(wbuf)));
> +       ut_assertok(run_commandf("read  mmc 2:1 0x%lx 1 1", ra));
> +       ut_assertok(memcmp(&wbuf[512], rbuf, 512));
> +
> +       /* Use name for write, number for read. */
> +       INIT_BUFFERS();
> +       ut_assertok(run_commandf("write mmc 2#log 0x%lx 0 2", wa));
> +       ut_assertok(run_commandf("read  mmc 2:2   0x%lx 0 2", ra));
> +       ut_assertok(memcmp(wbuf, rbuf, sizeof(wbuf)));
> +
> +       /* Use full device for write, name for read. */
> +       INIT_BUFFERS();
> +       ut_assertok(run_commandf("write mmc 2:0    0x%lx 0x30 2", wa));
> +       ut_assertok(run_commandf("read  mmc 2#data 0x%lx    0 2", ra));
> +       ut_assertok(memcmp(wbuf, rbuf, sizeof(wbuf)));
> +
> +       /* Use name for write, full device for read */
> +       INIT_BUFFERS();
> +       ut_assertok(run_commandf("write mmc 2#log 0x%lx    1 2", wa));
> +       ut_assertok(run_commandf("read  mmc 2:0   0x%lx 0x35 2", ra));
> +       ut_assertok(memcmp(wbuf, rbuf, sizeof(wbuf)));
> +
> +       /* Read/write outside partition bounds should be rejected upfront. */
> +       console_record_reset_enable();
> +        ut_asserteq(1, run_commandf("read mmc 2#data 0x%lx 3 2", ra));

There are spaces in front of these lines

> +        ut_assert_nextlinen("read out of range");
> +        ut_assert_console_end();
> +
> +       console_record_reset_enable();
> +        ut_asserteq(1, run_commandf("write mmc 2#log 0x%lx 9 2", wa));
> +        ut_assert_nextlinen("write out of range");
> +        ut_assert_console_end();
> +
> +       return 0;
> +}
> +
> +DM_TEST(dm_test_read_write, UT_TESTF_SCAN_FDT | UT_TESTF_CONSOLE_REC);
> --
> 2.37.2
>

Regards,
Simon

[PATCH v3 0/5] improve 'read' command, add 'write' command

[Rasmus Villemoes]

The first patch simplies do_read somewhat by making use of an existing
helper instead of parsing the dev_part string manually. As a bonus
(and my actual motivation), it now understands dev#partname syntax -
hard-coded partition numbers are so last decade.

I also need the symmetrical operation, being able to write to a named
raw partition, and fortunately it doesn't require that many lines of
code to implement that.

There's a very minor change in the error reporting due to using
cmdtp->name to generate the new messages, but I don't think "Error
reading blocks" offers much that "read error" doesn't.

New in v2: the last three patches add documentation, ensure CMD_WRITE
is set for sandbox and adds some basic test cases for the various ways
of accessing the partitions (by number, name, or as raw offset within
the whole disk).

v3: Add Simon's R-b to patches 2, 4, 5, fixup whitespace in patch 5.

I don't want to duplicate the documentation, but I can see the value
in 'write' having its own entry in the TOC, so I added a stub
write.rst that just refers to the read.rst, which then explicitly
documents both.

[And now, a small rant]

Then I wanted to test that rst magic and see that the linking works,
so I ran

  make htmldocs

and was greeted with

  Running Sphinx v1.8.5
  ...
  Sphinx version error:
  This project needs at least Sphinx v2.4.4 and therefore cannot be built with this version.

Interestingly, doc/sphinx/requirements.txt says
Sphinx==3.4.3. Whatever, lemme go try and build in an official
container. So I fired up "docker run --rm -ti
trini/u-boot-gitlab-ci-runner:bionic-20200807-02Sep2020", and tried
doing the steps listed in both .azure-pipelines.yml and
.gitlab-ci.yml, namely

  virtualenv -p /usr/bin/python3 /tmp/venvhtml
  . /tmp/venvhtml/bin/activate
  pip install -r doc/sphinx/requirements.txt
  make htmldocs

but the third step fails with

  ERROR: Could not find a version that satisfies the requirement MarkupSafe==2.1.1 (from versions: 0.9, 0.9.1, 0.9.2, 0.9.3, 0.11, 0.12, 0.13, 0.14, 0.15, 0.16, 0.17, 0.18, 0.19, 0.20, 0.21, 0.22, 0.23, 1.0, 1.1.0, 1.1.1, 2.0.0a1, 2.0.0rc1, 2.0.0rc2, 2.0.0, 2.0.1)
  ERROR: No matching distribution found for MarkupSafe==2.1.1

And then I give up.

Rasmus Villemoes (5):
  cmd: read: use part_get_info_by_dev_and_name_or_num() instead of
    open-coded dev_part parsing
  cmd: introduce 'write' command
  doc: document read/write commands
  sandbox: enable CMD_WRITE
  test: add tests of 'read' and 'write' shell commands

 cmd/Kconfig                 |   5 ++
 cmd/Makefile                |   1 +
 cmd/read.c                  |  61 +++++++++++----------
 configs/sandbox64_defconfig |   1 +
 configs/sandbox_defconfig   |   1 +
 doc/usage/cmd/read.rst      |  44 +++++++++++++++
 doc/usage/cmd/write.rst     |   6 +++
 doc/usage/index.rst         |   2 +
 test/cmd/Makefile           |   1 +
 test/cmd/rw.c               | 104 ++++++++++++++++++++++++++++++++++++
 10 files changed, 195 insertions(+), 31 deletions(-)
 create mode 100644 doc/usage/cmd/read.rst
 create mode 100644 doc/usage/cmd/write.rst
 create mode 100644 test/cmd/rw.c

-- 
2.37.2

[PATCH v3 1/5] cmd: read: use part_get_info_by_dev_and_name_or_num() instead of open-coded dev_part parsing

[Rasmus Villemoes]

Use the helper part_get_info_by_dev_and_name_or_num() for parsing a
dev[:part] string and obtaining the partition info in one go, instead
of open-coding all that.

As a bonus, this will automatically allow using the dev#partname
syntax as well, for accessing raw partitions by name.

Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 cmd/read.c | 32 ++++++++------------------------
 1 file changed, 8 insertions(+), 24 deletions(-)

diff --git a/cmd/read.c b/cmd/read.c
index fecfadaa1f..8645db49bb 100644
--- a/cmd/read.c
+++ b/cmd/read.c
@@ -15,50 +15,34 @@
 
 int do_read(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 {
-	char *ep;
 	struct blk_desc *dev_desc = NULL;
-	int dev;
-	int part = 0;
 	struct disk_partition part_info;
-	ulong offset = 0u;
-	ulong limit = 0u;
+	ulong offset, limit;
 	void *addr;
 	uint blk;
 	uint cnt;
+	int part;
 
 	if (argc != 6) {
 		cmd_usage(cmdtp);
 		return 1;
 	}
 
-	dev = (int)hextoul(argv[2], &ep);
-	if (*ep) {
-		if (*ep != ':') {
-			printf("Invalid block device %s\n", argv[2]);
-			return 1;
-		}
-		part = (int)hextoul(++ep, NULL);
-	}
-
-	dev_desc = blk_get_dev(argv[1], dev);
-	if (dev_desc == NULL) {
-		printf("Block device %s %d not supported\n", argv[1], dev);
+	part = part_get_info_by_dev_and_name_or_num(argv[1], argv[2],
+						    &dev_desc, &part_info, 1);
+	if (part < 0)
 		return 1;
-	}
 
 	addr = map_sysmem(hextoul(argv[3], NULL), 0);
 	blk = hextoul(argv[4], NULL);
 	cnt = hextoul(argv[5], NULL);
 
-	if (part != 0) {
-		if (part_get_info(dev_desc, part, &part_info)) {
-			printf("Cannot find partition %d\n", part);
-			return 1;
-		}
+	if (part > 0) {
 		offset = part_info.start;
 		limit = part_info.size;
 	} else {
 		/* Largest address not available in struct blk_desc. */
+		offset = 0;
 		limit = ~0;
 	}
 
@@ -78,5 +62,5 @@ int do_read(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 U_BOOT_CMD(
 	read,	6,	0,	do_read,
 	"Load binary data from a partition",
-	"<interface> <dev[:part]> addr blk# cnt"
+	"<interface> <dev[:part|#partname]> addr blk# cnt"
 );
-- 
2.37.2

[PATCH v3 2/5] cmd: introduce 'write' command

[Rasmus Villemoes]

It's almost no extra code to hook up a buddy to the 'read' command. In
fact, since the command is passed its own 'struct cmd_tbl', we can use
the exact same callback, and let it figure out for itself whether it
was invoked as "read" or "write".

Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 cmd/Kconfig  |  5 +++++
 cmd/Makefile |  1 +
 cmd/read.c   | 29 ++++++++++++++++++++++-------
 3 files changed, 28 insertions(+), 7 deletions(-)

diff --git a/cmd/Kconfig b/cmd/Kconfig
index 2caa4af71c..008ae55e02 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -1562,6 +1562,11 @@ config CMD_WDT
 	help
 	  This provides commands to control the watchdog timer devices.
 
+config CMD_WRITE
+	bool "write - Write binary data to a partition"
+	help
+	  Provides low-level write access to a partition.
+
 config CMD_AXI
 	bool "axi"
 	depends on AXI
diff --git a/cmd/Makefile b/cmd/Makefile
index 36d2daf22a..f2429c18ab 100644
--- a/cmd/Makefile
+++ b/cmd/Makefile
@@ -140,6 +140,7 @@ obj-$(CONFIG_CMD_PXE) += pxe.o
 obj-$(CONFIG_CMD_WOL) += wol.o
 obj-$(CONFIG_CMD_QFW) += qfw.o
 obj-$(CONFIG_CMD_READ) += read.o
+obj-$(CONFIG_CMD_WRITE) += read.o
 obj-$(CONFIG_CMD_REGINFO) += reginfo.o
 obj-$(CONFIG_CMD_REISER) += reiser.o
 obj-$(CONFIG_CMD_REMOTEPROC) += remoteproc.o
diff --git a/cmd/read.c b/cmd/read.c
index 8645db49bb..1218e7acfd 100644
--- a/cmd/read.c
+++ b/cmd/read.c
@@ -13,14 +13,14 @@
 #include <mapmem.h>
 #include <part.h>
 
-int do_read(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
+static int
+do_rw(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 {
 	struct blk_desc *dev_desc = NULL;
 	struct disk_partition part_info;
 	ulong offset, limit;
+	uint blk, cnt, res;
 	void *addr;
-	uint blk;
-	uint cnt;
 	int part;
 
 	if (argc != 6) {
@@ -47,20 +47,35 @@ int do_read(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 	}
 
 	if (cnt + blk > limit) {
-		printf("Read out of range\n");
+		printf("%s out of range\n", cmdtp->name);
 		return 1;
 	}
 
-	if (blk_dread(dev_desc, offset + blk, cnt, addr) != cnt) {
-		printf("Error reading blocks\n");
+	if (IS_ENABLED(CONFIG_CMD_WRITE) && !strcmp(cmdtp->name, "write"))
+		res = blk_dwrite(dev_desc, offset + blk, cnt, addr);
+	else
+		res = blk_dread(dev_desc, offset + blk, cnt, addr);
+
+	if (res != cnt) {
+		printf("%s error\n", cmdtp->name);
 		return 1;
 	}
 
 	return 0;
 }
 
+#ifdef CONFIG_CMD_READ
 U_BOOT_CMD(
-	read,	6,	0,	do_read,
+	read,	6,	0,	do_rw,
 	"Load binary data from a partition",
 	"<interface> <dev[:part|#partname]> addr blk# cnt"
 );
+#endif
+
+#ifdef CONFIG_CMD_WRITE
+U_BOOT_CMD(
+	write,	6,	0,	do_rw,
+	"Store binary data to a partition",
+	"<interface> <dev[:part|#partname]> addr blk# cnt"
+);
+#endif
-- 
2.37.2

[PATCH v3 3/5] doc: document read/write commands

[Rasmus Villemoes]

The read and write commands are, deliberately, implemented in the same
file, so that they stay feature-compatible (e.g. if someone implements
support for "read the full partition, however large that is", that
same syntax should also work for write). In order to ensure the
documentation for both are similarly kept in sync, and to avoid
duplication, document them both in read.rst, and add a stub write.rst
referring to read.rst.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 doc/usage/cmd/read.rst  | 44 +++++++++++++++++++++++++++++++++++++++++
 doc/usage/cmd/write.rst |  6 ++++++
 doc/usage/index.rst     |  2 ++
 3 files changed, 52 insertions(+)
 create mode 100644 doc/usage/cmd/read.rst
 create mode 100644 doc/usage/cmd/write.rst

diff --git a/doc/usage/cmd/read.rst b/doc/usage/cmd/read.rst
new file mode 100644
index 0000000000..840846728f
--- /dev/null
+++ b/doc/usage/cmd/read.rst
@@ -0,0 +1,44 @@
+.. SPDX-License-Identifier: GPL-2.0-or-later:
+
+read and write commands
+=======================
+
+Synopsis
+--------
+
+::
+
+    read <interface> <dev[:part|#partname]> <addr> <blk#> <cnt>
+    write <interface> <dev[:part|#partname]> <addr> <blk#> <cnt>
+
+The read and write commands can be used for raw access to data in
+block devices (or partitions therein), i.e. without going through a
+file system.
+
+read
+----
+
+The block device is specified using the <interface> (e.g. "mmc") and
+<dev> parameters. If the block device has a partition table, one can
+optionally specify a partition number (using the :part syntax) or
+partition name (using the #partname syntax). The command then reads
+the <cnt> blocks of data starting at block number <blk#> of the given
+device/partition to the memory address <addr>.
+
+write
+-----
+
+The write command is completely equivalent to the read command, except
+of course that the transfer direction is reversed.
+
+Examples
+--------
+
+    # Read 2 MiB from partition 3 of mmc device 2 to $loadaddr
+    read mmc 2.3 $loadaddr 0 0x1000
+
+    # Read 16 MiB from the partition named 'kernel' of mmc device 1 to $loadaddr
+    read mmc 1#kernel $loadaddr 0 0x8000
+
+    # Write to the third sector of the partition named 'bootdata' of mmc device 0
+    write mmc 0#bootdata $loadaddr 2 1
diff --git a/doc/usage/cmd/write.rst b/doc/usage/cmd/write.rst
new file mode 100644
index 0000000000..06ad77e359
--- /dev/null
+++ b/doc/usage/cmd/write.rst
@@ -0,0 +1,6 @@
+.. SPDX-License-Identifier: GPL-2.0-or-later:
+
+write command
+=============
+
+See :doc:`cmd/read`.
diff --git a/doc/usage/index.rst b/doc/usage/index.rst
index 840c20c934..7d55beace4 100644
--- a/doc/usage/index.rst
+++ b/doc/usage/index.rst
@@ -71,6 +71,7 @@ Shell commands
    cmd/printenv
    cmd/pstore
    cmd/qfw
+   cmd/read
    cmd/reset
    cmd/rng
    cmd/sbi
@@ -91,6 +92,7 @@ Shell commands
    cmd/ut
    cmd/wdt
    cmd/wget
+   cmd/write
    cmd/xxd
 
 Booting OS
-- 
2.37.2

[PATCH v3 4/5] sandbox: enable CMD_WRITE

[Rasmus Villemoes]

Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 configs/sandbox64_defconfig | 1 +
 configs/sandbox_defconfig   | 1 +
 2 files changed, 2 insertions(+)

diff --git a/configs/sandbox64_defconfig b/configs/sandbox64_defconfig
index ccbc18aad0..b7737814af 100644
--- a/configs/sandbox64_defconfig
+++ b/configs/sandbox64_defconfig
@@ -58,6 +58,7 @@ CONFIG_CMD_SPI=y
 CONFIG_CMD_TEMPERATURE=y
 CONFIG_CMD_USB=y
 CONFIG_CMD_WDT=y
+CONFIG_CMD_WRITE=y
 CONFIG_CMD_CAT=y
 CONFIG_BOOTP_DNS2=y
 CONFIG_CMD_TFTPPUT=y
diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig
index 77ade1f1d8..daae539102 100644
--- a/configs/sandbox_defconfig
+++ b/configs/sandbox_defconfig
@@ -84,6 +84,7 @@ CONFIG_CMD_SPI=y
 CONFIG_CMD_TEMPERATURE=y
 CONFIG_CMD_USB=y
 CONFIG_CMD_WDT=y
+CONFIG_CMD_WRITE=y
 CONFIG_CMD_AXI=y
 CONFIG_CMD_CAT=y
 CONFIG_CMD_SETEXPR_FMT=y
-- 
2.37.2

[PATCH v3 5/5] test: add tests of 'read' and 'write' shell commands

[Rasmus Villemoes]

Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 test/cmd/Makefile |   1 +
 test/cmd/rw.c     | 104 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 105 insertions(+)
 create mode 100644 test/cmd/rw.c

diff --git a/test/cmd/Makefile b/test/cmd/Makefile
index 2ffde8703a..7848f348bc 100644
--- a/test/cmd/Makefile
+++ b/test/cmd/Makefile
@@ -18,6 +18,7 @@ obj-$(CONFIG_CMD_PINMUX) += pinmux.o
 obj-$(CONFIG_CMD_PWM) += pwm.o
 obj-$(CONFIG_CMD_SEAMA) += seama.o
 ifdef CONFIG_SANDBOX
+obj-$(CONFIG_CMD_READ) += rw.o
 obj-$(CONFIG_CMD_SETEXPR) += setexpr.o
 endif
 obj-$(CONFIG_CMD_TEMPERATURE) += temperature.o
diff --git a/test/cmd/rw.c b/test/cmd/rw.c
new file mode 100644
index 0000000000..b087810400
--- /dev/null
+++ b/test/cmd/rw.c
@@ -0,0 +1,104 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Tests for read and write commands
+ */
+
+#include <common.h>
+#include <dm/test.h>
+#include <mapmem.h>
+#include <part.h>
+#include <test/test.h>
+#include <test/ut.h>
+
+static int setup_partitions(struct unit_test_state *uts, struct blk_desc **mmc_dev_desc)
+{
+	char str_disk_guid[UUID_STR_LEN + 1];
+	struct disk_partition parts[2] = {
+		{
+			.start = 48, /* GPT data takes up the first 34 blocks or so */
+			.size = 4,
+			.name = "data",
+		},
+		{
+			.start = 52,
+			.size = 10,
+			.name = "log",
+		},
+	};
+
+	ut_asserteq(2, blk_get_device_by_str("mmc", "2", mmc_dev_desc));
+	if (CONFIG_IS_ENABLED(RANDOM_UUID)) {
+		gen_rand_uuid_str(parts[0].uuid, UUID_STR_FORMAT_STD);
+		gen_rand_uuid_str(parts[1].uuid, UUID_STR_FORMAT_STD);
+		gen_rand_uuid_str(str_disk_guid, UUID_STR_FORMAT_STD);
+	}
+	ut_assertok(gpt_restore(*mmc_dev_desc, str_disk_guid, parts,
+				ARRAY_SIZE(parts)));
+	return 0;
+}
+
+/* Fill the write buffer with pseudo-random data, clear the read buffer. */
+static void init_buffers(char *rb, char *wb, size_t size, unsigned seed)
+{
+	memset(rb, 0, size);
+	while (size--) {
+		*wb++ = seed;
+		seed *= 43;
+		seed += 17 + size/4;
+	}
+}
+
+static int dm_test_read_write(struct unit_test_state *uts)
+{
+	struct blk_desc *dev_desc;
+	char wbuf[1024], rbuf[1024];
+	ulong wa, ra;
+
+#define INIT_BUFFERS() init_buffers(rbuf, wbuf, sizeof(rbuf), __LINE__)
+
+	ut_assertok(setup_partitions(uts, &dev_desc));
+
+	wa = map_to_sysmem(wbuf);
+	ra = map_to_sysmem(rbuf);
+
+	/* Simple test, write to/read from same partition. */
+	INIT_BUFFERS();
+	ut_assertok(run_commandf("write mmc 2:1 0x%lx 0 2", wa));
+	ut_assertok(run_commandf("read  mmc 2:1 0x%lx 0 2", ra));
+	ut_assertok(memcmp(wbuf, rbuf, sizeof(wbuf)));
+	ut_assertok(run_commandf("read  mmc 2:1 0x%lx 1 1", ra));
+	ut_assertok(memcmp(&wbuf[512], rbuf, 512));
+
+	/* Use name for write, number for read. */
+	INIT_BUFFERS();
+	ut_assertok(run_commandf("write mmc 2#log 0x%lx 0 2", wa));
+	ut_assertok(run_commandf("read  mmc 2:2   0x%lx 0 2", ra));
+	ut_assertok(memcmp(wbuf, rbuf, sizeof(wbuf)));
+	
+	/* Use full device for write, name for read. */
+	INIT_BUFFERS();
+	ut_assertok(run_commandf("write mmc 2:0    0x%lx 0x30 2", wa));
+	ut_assertok(run_commandf("read  mmc 2#data 0x%lx    0 2", ra));
+	ut_assertok(memcmp(wbuf, rbuf, sizeof(wbuf)));
+
+	/* Use name for write, full device for read */
+	INIT_BUFFERS();
+	ut_assertok(run_commandf("write mmc 2#log 0x%lx    1 2", wa));
+	ut_assertok(run_commandf("read  mmc 2:0   0x%lx 0x35 2", ra));
+	ut_assertok(memcmp(wbuf, rbuf, sizeof(wbuf)));
+
+	/* Read/write outside partition bounds should be rejected upfront. */
+	console_record_reset_enable();
+	ut_asserteq(1, run_commandf("read mmc 2#data 0x%lx 3 2", ra));
+	ut_assert_nextlinen("read out of range");
+	ut_assert_console_end();
+
+	console_record_reset_enable();
+	ut_asserteq(1, run_commandf("write mmc 2#log 0x%lx 9 2", wa));
+	ut_assert_nextlinen("write out of range");
+	ut_assert_console_end();
+
+	return 0;
+}
+
+DM_TEST(dm_test_read_write, UT_TESTF_SCAN_FDT | UT_TESTF_CONSOLE_REC);
-- 
2.37.2

Re: [PATCH v3 3/5] doc: document read/write commands

[Simon Glass]

On Thu, 2 Mar 2023 at 01:12, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> The read and write commands are, deliberately, implemented in the same
> file, so that they stay feature-compatible (e.g. if someone implements
> support for "read the full partition, however large that is", that
> same syntax should also work for write). In order to ensure the
> documentation for both are similarly kept in sync, and to avoid
> duplication, document them both in read.rst, and add a stub write.rst
> referring to read.rst.
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  doc/usage/cmd/read.rst  | 44 +++++++++++++++++++++++++++++++++++++++++
>  doc/usage/cmd/write.rst |  6 ++++++
>  doc/usage/index.rst     |  2 ++
>  3 files changed, 52 insertions(+)
>  create mode 100644 doc/usage/cmd/read.rst
>  create mode 100644 doc/usage/cmd/write.rst
>

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH v3 0/5] improve 'read' command, add 'write' command

[Rasmus Villemoes]

On 02/03/2023 09.12, Rasmus Villemoes wrote:
> The first patch simplies do_read somewhat by making use of an existing
> helper instead of parsing the dev_part string manually. As a bonus
> (and my actual motivation), it now understands dev#partname syntax -
> hard-coded partition numbers are so last decade.
> 
> I also need the symmetrical operation, being able to write to a named
> raw partition, and fortunately it doesn't require that many lines of
> code to implement that.
> 
> There's a very minor change in the error reporting due to using
> cmdtp->name to generate the new messages, but I don't think "Error
> reading blocks" offers much that "read error" doesn't.
> 
> New in v2: the last three patches add documentation, ensure CMD_WRITE
> is set for sandbox and adds some basic test cases for the various ways
> of accessing the partitions (by number, name, or as raw offset within
> the whole disk).
> 
> v3: Add Simon's R-b to patches 2, 4, 5, fixup whitespace in patch 5.

Can this series be picked up for -next, please?

Rasmus

Re: [PATCH v3 0/5] improve 'read' command, add 'write' command

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1244 bytes --]

On Thu, Mar 16, 2023 at 02:24:05PM +0100, Rasmus Villemoes wrote:
> On 02/03/2023 09.12, Rasmus Villemoes wrote:
> > The first patch simplies do_read somewhat by making use of an existing
> > helper instead of parsing the dev_part string manually. As a bonus
> > (and my actual motivation), it now understands dev#partname syntax -
> > hard-coded partition numbers are so last decade.
> > 
> > I also need the symmetrical operation, being able to write to a named
> > raw partition, and fortunately it doesn't require that many lines of
> > code to implement that.
> > 
> > There's a very minor change in the error reporting due to using
> > cmdtp->name to generate the new messages, but I don't think "Error
> > reading blocks" offers much that "read error" doesn't.
> > 
> > New in v2: the last three patches add documentation, ensure CMD_WRITE
> > is set for sandbox and adds some basic test cases for the various ways
> > of accessing the partitions (by number, name, or as raw offset within
> > the whole disk).
> > 
> > v3: Add Simon's R-b to patches 2, 4, 5, fixup whitespace in patch 5.
> 
> Can this series be picked up for -next, please?

Yes. Getting out of a backlog now due to some lab issues.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v3 1/5] cmd: read: use part_get_info_by_dev_and_name_or_num() instead of open-coded dev_part parsing

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 548 bytes --]

On Thu, Mar 02, 2023 at 09:12:21AM +0100, Rasmus Villemoes wrote:

> Use the helper part_get_info_by_dev_and_name_or_num() for parsing a
> dev[:part] string and obtaining the partition info in one go, instead
> of open-coding all that.
> 
> As a bonus, this will automatically allow using the dev#partname
> syntax as well, for accessing raw partitions by name.
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>

For the series, applied to u-boot/next, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]