[PATCH v2 00/18] bootstd: Useability improvements

[PATCH v2 00/18] bootstd: Useability improvements

[Simon Glass]

This series collects together some bootstd improvements:

- Improve iteration when there are a lot of devices
- Add a test image for Ubuntu (to compliment Fedora)
- Improve the naming of USB devices and bootdevs
- Add a new command to set the bootdev order
- Add a little more debugging
- Use an abuf when dealing with allocate files
- A few other minor things in bootstd

Most of these issues were found when adding the new test image and more
USB devices to sandbox.

Changes in v2:
- Add new patch with a helper for initing and allocating a buffer
- Update to use abuf_init_size()
- Update commit message to make it clear this is not actually an image
- Drop use of LOGR()
- Rebase on top of recent FS changes

Simon Glass (18):
  abuf: Add a helper for initing and allocating a buffer
  fs: boot: Update fs_read_alloc() to use abuf
  fs: boot: Update fs_load_alloc() to use abuf
  fs: boot: Update bootmeth_alloc_other() to use abuf
  bootstd: Add more debugging to bootmeth_efi
  bootstd: Add more debugging to bootmeth_extlinux
  bootstd: Try all bootmeths on the final partition
  bootstd: Fully complete iteration of a uclass
  test/py: Split out core of Fedora image into a new function
  test/py: Add an extlinux file similar to Ubuntu
  usb: Use more useful names for block devices
  sandbox: Use a unique name for each USB controller
  bootstd: Tweak scanning with labels
  bootstd: Tidy up a nested CONFIG_IS_ENABLED(BOOTSTD)
  bootstd: Provide a command to select the bootdev order
  bootstd: Correct the comment for bootmeth_set_order()
  bootstd: Expand debugging in bootdev_find_in_blk()
  bootstd: Mention FS state in bootmeth_read_bootflow()

 arch/sandbox/dts/test.dts      |  13 +++-
 boot/bootdev-uclass.c          | 114 +++++++++++++++++++++++----------
 boot/bootflow.c                |  37 ++++++-----
 boot/bootmeth-uclass.c         |  17 ++---
 boot/bootmeth_efi.c            |   5 +-
 boot/bootmeth_extlinux.c       |  14 +++-
 boot/bootmeth_script.c         |   6 +-
 boot/bootstd-uclass.c          |  18 ++++++
 boot/cedit.c                   |   3 +-
 boot/scene.c                   |   3 +-
 boot/scene_textline.c          |   3 +-
 cmd/bootdev.c                  |  37 +++++++++++
 cmd/bootflow.c                 |   2 +-
 cmd/cat.c                      |  13 ++--
 cmd/cedit.c                    |  18 +++---
 doc/usage/cmd/bootdev.rst      |  36 +++++++++++
 drivers/usb/emul/sandbox_hub.c |   2 +-
 drivers/usb/host/usb-uclass.c  |  14 +++-
 fs/fs.c                        |  27 +++-----
 include/abuf.h                 |  11 ++++
 include/bootdev.h              |  60 +++++++++--------
 include/bootflow.h             |   2 +-
 include/bootmeth.h             |  12 ++--
 include/bootstd.h              |  10 +++
 include/fs.h                   |  18 +++---
 lib/abuf.c                     |   9 +++
 lib/of_live.c                  |   3 +-
 test/boot/bootdev.c            |  83 ++++++++++++++++++------
 test/boot/bootflow.c           |  10 +--
 test/dm/blk.c                  |   4 +-
 test/dm/usb.c                  |   8 +--
 test/lib/abuf.c                |  22 +++++++
 test/py/tests/test_ut.py       | 111 +++++++++++++++++++++++++-------
 33 files changed, 534 insertions(+), 211 deletions(-)

-- 
2.43.0

base-commit: 37e1b5b09b6ad0a8dd15c580b4f915ea46de8935
branch: schc2

[PATCH v2 01/18] abuf: Add a helper for initing and allocating a buffer

[Simon Glass]

This construct appears in various places. Reduce code size by adding a
function for it.

It inits the abuf, then allocates it to the requested size.

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

Changes in v2:
- Add new patch with a helper for initing and allocating a buffer

 boot/cedit.c          |  3 +--
 boot/scene.c          |  3 +--
 boot/scene_textline.c |  3 +--
 include/abuf.h        | 11 +++++++++++
 lib/abuf.c            |  9 +++++++++
 lib/of_live.c         |  3 +--
 test/lib/abuf.c       | 22 ++++++++++++++++++++++
 7 files changed, 46 insertions(+), 8 deletions(-)

diff --git a/boot/cedit.c b/boot/cedit.c
index d69290c172e..4e80875828b 100644
--- a/boot/cedit.c
+++ b/boot/cedit.c
@@ -449,8 +449,7 @@ int cedit_write_settings(struct expo *exp, struct abuf *buf)
 	void *fdt;
 	int ret;
 
-	abuf_init(buf);
-	if (!abuf_realloc(buf, CEDIT_SIZE_INC))
+	if (!abuf_init_size(buf, CEDIT_SIZE_INC))
 		return log_msg_ret("buf", -ENOMEM);
 
 	fdt = abuf_data(buf);
diff --git a/boot/scene.c b/boot/scene.c
index 3290a40222a..9034aeeecf2 100644
--- a/boot/scene.c
+++ b/boot/scene.c
@@ -31,8 +31,7 @@ int scene_new(struct expo *exp, const char *name, uint id, struct scene **scnp)
 		return log_msg_ret("name", -ENOMEM);
 	}
 
-	abuf_init(&scn->buf);
-	if (!abuf_realloc(&scn->buf, EXPO_MAX_CHARS + 1)) {
+	if (!abuf_init_size(&scn->buf, EXPO_MAX_CHARS + 1)) {
 		free(scn->name);
 		free(scn);
 		return log_msg_ret("buf", -ENOMEM);
diff --git a/boot/scene_textline.c b/boot/scene_textline.c
index 6adef7cc173..90642a3f03d 100644
--- a/boot/scene_textline.c
+++ b/boot/scene_textline.c
@@ -31,8 +31,7 @@ int scene_textline(struct scene *scn, const char *name, uint id, uint max_chars,
 			    (struct scene_obj **)&tline);
 	if (ret < 0)
 		return log_msg_ret("obj", -ENOMEM);
-	abuf_init(&tline->buf);
-	if (!abuf_realloc(&tline->buf, max_chars + 1))
+	if (!abuf_init_size(&tline->buf, max_chars + 1))
 		return log_msg_ret("buf", -ENOMEM);
 	buf = abuf_data(&tline->buf);
 	*buf = '\0';
diff --git a/include/abuf.h b/include/abuf.h
index 62ff6499a0c..749bb188b0c 100644
--- a/include/abuf.h
+++ b/include/abuf.h
@@ -170,6 +170,17 @@ void abuf_init_set(struct abuf *abuf, void *data, size_t size);
  */
 void abuf_init_const(struct abuf *abuf, const void *data, size_t size);
 
+/**
+ * abuf_init_size() - Set up an allocated abuf
+ *
+ * Init a new abuf and allocate its size.
+ *
+ * @abuf: abuf to set up
+ * @data: New contents of abuf
+ * @size: New size of abuf
+ */
+bool abuf_init_size(struct abuf *buf, size_t size);
+
 /**
  * abuf_uninit() - Free any memory used by an abuf
  *
diff --git a/lib/abuf.c b/lib/abuf.c
index 61adf7fc6b1..3cbe320fb08 100644
--- a/lib/abuf.c
+++ b/lib/abuf.c
@@ -119,6 +119,15 @@ void abuf_init_set(struct abuf *abuf, void *data, size_t size)
 	abuf_set(abuf, data, size);
 }
 
+bool abuf_init_size(struct abuf *buf, size_t size)
+{
+	abuf_init(buf);
+	if (!abuf_realloc(buf, size))
+		return false;
+
+	return true;
+}
+
 void abuf_init_const(struct abuf *abuf, const void *data, size_t size)
 {
 	/* for now there is no flag indicating that the abuf data is constant */
diff --git a/lib/of_live.c b/lib/of_live.c
index bc8c73c6f36..470a687047b 100644
--- a/lib/of_live.c
+++ b/lib/of_live.c
@@ -448,8 +448,7 @@ int of_live_flatten(const struct device_node *root, struct abuf *buf)
 {
 	int ret;
 
-	abuf_init(buf);
-	if (!abuf_realloc(buf, BUF_STEP))
+	if (!abuf_init_size(buf, BUF_STEP))
 		return log_msg_ret("ini", -ENOMEM);
 
 	ret = fdt_create(abuf_data(buf), abuf_size(buf));
diff --git a/test/lib/abuf.c b/test/lib/abuf.c
index b38690fe1a9..916577ee3ea 100644
--- a/test/lib/abuf.c
+++ b/test/lib/abuf.c
@@ -419,3 +419,25 @@ static int lib_test_abuf_init(struct unit_test_state *uts)
 	return 0;
 }
 LIB_TEST(lib_test_abuf_init, 0);
+
+/* Test abuf_init_size() */
+static int lib_test_abuf_init_size(struct unit_test_state *uts)
+{
+	struct abuf buf;
+	ulong start;
+
+	start = ut_check_free();
+
+	ut_assert(abuf_init_size(&buf, TEST_DATA_LEN));
+	ut_assertnonnull(buf.data);
+	ut_asserteq(TEST_DATA_LEN, buf.size);
+	ut_asserteq(true, buf.alloced);
+	abuf_uninit(&buf);
+
+	/* Check for memory leaks */
+	ut_assertok(ut_check_delta(start));
+
+	return 0;
+	return 0;
+}
+LIB_TEST(lib_test_abuf_init_size, 0);
-- 
2.43.0

[PATCH v2 02/18] fs: boot: Update fs_read_alloc() to use abuf

[Simon Glass]

Using an abuf for this function simplifies returning the size and also
makes it easier to free memory afterwards. Update the API and callers.

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

Changes in v2:
- Update to use abuf_init_size()

 boot/bootmeth-uclass.c | 19 ++++++++++---------
 fs/fs.c                | 24 ++++++++++--------------
 include/fs.h           |  8 +++++---
 3 files changed, 25 insertions(+), 26 deletions(-)

diff --git a/boot/bootmeth-uclass.c b/boot/bootmeth-uclass.c
index 014b7588e8d..78a3671f96a 100644
--- a/boot/bootmeth-uclass.c
+++ b/boot/bootmeth-uclass.c
@@ -332,7 +332,8 @@ int bootmeth_alloc_file(struct bootflow *bflow, uint size_limit, uint align,
 			enum bootflow_img_t type)
 {
 	struct blk_desc *desc = NULL;
-	void *buf;
+	struct abuf buf;
+	ulong addr;
 	uint size;
 	int ret;
 
@@ -346,13 +347,13 @@ int bootmeth_alloc_file(struct bootflow *bflow, uint size_limit, uint align,
 		return log_msg_ret("all", ret);
 
 	bflow->state = BOOTFLOWST_READY;
-	bflow->buf = buf;
+	addr = abuf_addr(&buf);
+	bflow->buf = abuf_uninit_move(&buf, NULL);
 
 	if (bflow->blk)
 		desc = dev_get_uclass_plat(bflow->blk);
 
-	if (!bootflow_img_add(bflow, bflow->fname, type, map_to_sysmem(buf),
-			      size))
+	if (!bootflow_img_add(bflow, bflow->fname, type, addr, size))
 		return log_msg_ret("bai", -ENOMEM);
 
 	return 0;
@@ -362,9 +363,10 @@ int bootmeth_alloc_other(struct bootflow *bflow, const char *fname,
 			 enum bootflow_img_t type, void **bufp, uint *sizep)
 {
 	struct blk_desc *desc = NULL;
+	struct abuf buf;
 	char path[200];
 	loff_t size;
-	void *buf;
+	size_t bsize;
 	int ret;
 
 	snprintf(path, sizeof(path), "%s%s", bflow->subdir, fname);
@@ -388,12 +390,11 @@ int bootmeth_alloc_other(struct bootflow *bflow, const char *fname,
 	if (ret)
 		return log_msg_ret("all", ret);
 
-	if (!bootflow_img_add(bflow, bflow->fname, type, map_to_sysmem(buf),
-			      size))
+	if (!bootflow_img_add(bflow, bflow->fname, type, abuf_addr(&buf), size))
 		return log_msg_ret("boi", -ENOMEM);
 
-	*bufp = buf;
-	*sizep = size;
+	*bufp = abuf_uninit_move(&buf, &bsize);
+	*sizep = bsize;
 
 	return 0;
 }
diff --git a/fs/fs.c b/fs/fs.c
index 7def8a9d65d..61161fe5356 100644
--- a/fs/fs.c
+++ b/fs/fs.c
@@ -1128,31 +1128,26 @@ int do_fs_types(struct cmd_tbl *cmdtp, int flag, int argc, char * const argv[])
 	return CMD_RET_SUCCESS;
 }
 
-int fs_read_alloc(const char *fname, ulong size, uint align, void **bufp)
+int fs_read_alloc(const char *fname, ulong size, uint align, struct abuf *buf)
 {
 	loff_t bytes_read;
-	ulong addr;
-	char *buf;
 	int ret;
 
 	if (!align)
 		align = ARCH_DMA_MINALIGN;
 
-	buf = memalign(align, size + 1);
-	if (!buf)
+	if (!abuf_init_size(buf, size + 1))
 		return log_msg_ret("buf", -ENOMEM);
-	addr = map_to_sysmem(buf);
+	buf->size--;
 
-	ret = fs_read(fname, addr, 0, size, &bytes_read);
+	ret = fs_read(fname, abuf_addr(buf), 0, size, &bytes_read);
 	if (ret) {
-		free(buf);
+		abuf_uninit(buf);
 		return log_msg_ret("read", ret);
 	}
 	if (size != bytes_read)
 		return log_msg_ret("bread", -EIO);
-	buf[size] = '\0';
-
-	*bufp = buf;
+	((char *)buf->data)[size] = '\0';
 
 	return 0;
 }
@@ -1161,8 +1156,9 @@ int fs_load_alloc(const char *ifname, const char *dev_part_str,
 		  const char *fname, ulong max_size, ulong align, void **bufp,
 		  ulong *sizep)
 {
+	struct abuf buf;
+	size_t bsize;
 	loff_t size;
-	void *buf;
 	int ret;
 
 	if (fs_set_blk_dev(ifname, dev_part_str, FS_TYPE_ANY))
@@ -1181,8 +1177,8 @@ int fs_load_alloc(const char *ifname, const char *dev_part_str,
 	ret = fs_read_alloc(fname, size, align, &buf);
 	if (ret)
 		return log_msg_ret("al", ret);
-	*sizep = size;
-	*bufp = buf;
+	*bufp = abuf_uninit_move(&buf, &bsize);
+	*sizep = bsize;
 
 	return 0;
 }
diff --git a/include/fs.h b/include/fs.h
index 731aaa02637..86db5e04e80 100644
--- a/include/fs.h
+++ b/include/fs.h
@@ -7,6 +7,7 @@
 
 #include <rtc.h>
 
+struct abuf;
 struct cmd_tbl;
 
 #define FS_TYPE_ANY	0
@@ -341,11 +342,12 @@ int do_fs_types(struct cmd_tbl *cmdtp, int flag, int argc, char * const argv[]);
  * @fname: Filename to read
  * @size: Size of file to read (must be correct!)
  * @align: Alignment to use for memory allocation (0 for default: ARCH_DMA_MINALIGN)
- * @bufp: On success, returns the allocated buffer with the nul-terminated file
- *	in it
+ * @buf: On success, returns the allocated buffer with the nul-terminated file
+ *	in it. The buffer size is set to the size excluding the terminator. The
+ *	buffer is inited by this function and must be uninited by the caller
  * Return: 0 if OK, -ENOMEM if out of memory, -EIO if read failed
  */
-int fs_read_alloc(const char *fname, ulong size, uint align, void **bufp);
+int fs_read_alloc(const char *fname, ulong size, uint align, struct abuf *buf);
 
 /**
  * fs_load_alloc() - Load a file into allocated space
-- 
2.43.0

[PATCH v2 03/18] fs: boot: Update fs_load_alloc() to use abuf

[Simon Glass]

Using an abuf for this function simplifies returning the size and also
makes it easier to free memory afterwards. Update the API and callers.

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

(no changes since v1)

 cmd/cat.c    | 13 ++++++-------
 cmd/cedit.c  | 18 ++++++++----------
 fs/fs.c      | 11 ++++-------
 include/fs.h | 10 +++++-----
 4 files changed, 23 insertions(+), 29 deletions(-)

diff --git a/cmd/cat.c b/cmd/cat.c
index 6828b7b364e..24983cb9ca0 100644
--- a/cmd/cat.c
+++ b/cmd/cat.c
@@ -4,6 +4,7 @@
  * Roger Knecht <rknecht@pm.de>
  */
 
+#include <abuf.h>
 #include <command.h>
 #include <fs.h>
 #include <malloc.h>
@@ -12,11 +13,10 @@
 static int do_cat(struct cmd_tbl *cmdtp, int flag, int argc,
 		  char *const argv[])
 {
+	struct abuf buf;
 	char *ifname;
 	char *dev;
 	char *file;
-	char *buffer;
-	ulong file_size;
 	int ret;
 
 	if (argc < 4)
@@ -26,8 +26,7 @@ static int do_cat(struct cmd_tbl *cmdtp, int flag, int argc,
 	dev = argv[2];
 	file = argv[3];
 
-	ret = fs_load_alloc(ifname, dev, file, 0, 0, (void **)&buffer,
-			    &file_size);
+	ret = fs_load_alloc(ifname, dev, file, 0, 0, &buf);
 
 	// check file exists
 	switch (ret) {
@@ -51,10 +50,10 @@ static int do_cat(struct cmd_tbl *cmdtp, int flag, int argc,
 	}
 
 	// print file content
-	buffer[file_size] = '\0';
-	puts(buffer);
+	((char *)buf.data)[buf.size] = '\0';
+	puts(buf.data);
 
-	free(buffer);
+	abuf_uninit(&buf);
 
 	return 0;
 }
diff --git a/cmd/cedit.c b/cmd/cedit.c
index f696356419e..b0eca7b4daf 100644
--- a/cmd/cedit.c
+++ b/cmd/cedit.c
@@ -34,22 +34,21 @@ static int do_cedit_load(struct cmd_tbl *cmdtp, int flag, int argc,
 {
 	const char *fname;
 	struct expo *exp;
+	struct abuf buf;
 	oftree tree;
-	ulong size;
-	void *buf;
 	int ret;
 
 	if (argc < 4)
 		return CMD_RET_USAGE;
 	fname = argv[3];
 
-	ret = fs_load_alloc(argv[1], argv[2], argv[3], SZ_1M, 0, &buf, &size);
+	ret = fs_load_alloc(argv[1], argv[2], argv[3], SZ_1M, 0, &buf);
 	if (ret) {
 		printf("File not found\n");
 		return CMD_RET_FAILURE;
 	}
 
-	tree = oftree_from_fdt(buf);
+	tree = oftree_from_fdt(abuf_uninit_move(&buf, NULL));
 	if (!oftree_valid(tree)) {
 		printf("Cannot create oftree\n");
 		return CMD_RET_FAILURE;
@@ -125,31 +124,30 @@ static int do_cedit_read_fdt(struct cmd_tbl *cmdtp, int flag, int argc,
 			     char *const argv[])
 {
 	const char *fname;
-	void *buf;
+	struct abuf buf;
 	oftree tree;
-	ulong size;
 	int ret;
 
 	if (argc < 4)
 		return CMD_RET_USAGE;
 	fname = argv[3];
 
-	ret = fs_load_alloc(argv[1], argv[2], argv[3], SZ_1M, 0, &buf, &size);
+	ret = fs_load_alloc(argv[1], argv[2], argv[3], SZ_1M, 0, &buf);
 	if (ret) {
 		printf("File not found\n");
 		return CMD_RET_FAILURE;
 	}
 
-	tree = oftree_from_fdt(buf);
+	tree = oftree_from_fdt(buf.data);
 	if (!oftree_valid(tree)) {
-		free(buf);
+		abuf_uninit(&buf);
 		printf("Cannot create oftree\n");
 		return CMD_RET_FAILURE;
 	}
 
 	ret = cedit_read_settings(cur_exp, tree);
 	oftree_dispose(tree);
-	free(buf);
+	abuf_uninit(&buf);
 	if (ret) {
 		printf("Failed to read settings: %dE\n", ret);
 		return CMD_RET_FAILURE;
diff --git a/fs/fs.c b/fs/fs.c
index 61161fe5356..63ed6436493 100644
--- a/fs/fs.c
+++ b/fs/fs.c
@@ -5,6 +5,7 @@
 
 #define LOG_CATEGORY LOGC_CORE
 
+#include <abuf.h>
 #include <bootstd.h>
 #include <command.h>
 #include <config.h>
@@ -1153,11 +1154,9 @@ int fs_read_alloc(const char *fname, ulong size, uint align, struct abuf *buf)
 }
 
 int fs_load_alloc(const char *ifname, const char *dev_part_str,
-		  const char *fname, ulong max_size, ulong align, void **bufp,
-		  ulong *sizep)
+		  const char *fname, ulong max_size, ulong align,
+		  struct abuf *buf)
 {
-	struct abuf buf;
-	size_t bsize;
 	loff_t size;
 	int ret;
 
@@ -1174,11 +1173,9 @@ int fs_load_alloc(const char *ifname, const char *dev_part_str,
 	if (fs_set_blk_dev(ifname, dev_part_str, FS_TYPE_ANY))
 		return log_msg_ret("set", -ENOMEDIUM);
 
-	ret = fs_read_alloc(fname, size, align, &buf);
+	ret = fs_read_alloc(fname, size, align, buf);
 	if (ret)
 		return log_msg_ret("al", ret);
-	*bufp = abuf_uninit_move(&buf, &bsize);
-	*sizep = bsize;
 
 	return 0;
 }
diff --git a/include/fs.h b/include/fs.h
index 86db5e04e80..5cd9114f33b 100644
--- a/include/fs.h
+++ b/include/fs.h
@@ -359,15 +359,15 @@ int fs_read_alloc(const char *fname, ulong size, uint align, struct abuf *buf);
  * @fname: Filename to read
  * @max_size: Maximum allowed size for the file (use 0 for 1GB)
  * @align: Alignment to use for memory allocation (0 for default)
- * @bufp: On success, returns the allocated buffer with the nul-terminated file
- *	in it
- * @sizep: On success, returns the size of the file
+ * @buf: On success, returns the allocated buffer with the nul-terminated file
+ *	in it. The buffer size is set to the size excluding the terminator. The
+ *	buffer is inited by this function and must be uninited by the caller
  * Return: 0 if OK, -ENOMEM if out of memory, -ENOENT if the file does not
  * exist, -ENOMEDIUM if the device does not exist, -E2BIG if the file is too
  * large (greater than @max_size), -EIO if read failed
  */
 int fs_load_alloc(const char *ifname, const char *dev_part_str,
-		  const char *fname, ulong max_size, ulong align, void **bufp,
-		  ulong *sizep);
+		  const char *fname, ulong max_size, ulong align,
+		  struct abuf *buf);
 
 #endif /* _FS_H */
-- 
2.43.0

[PATCH v2 04/18] fs: boot: Update bootmeth_alloc_other() to use abuf

[Simon Glass]

Using an abuf for this function simplifies returning the size and also
makes it easier to free memory afterwards. Update the API and callers.

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

(no changes since v1)

 boot/bootmeth-uclass.c | 12 ++++--------
 boot/bootmeth_script.c |  6 ++++--
 cmd/bootflow.c         |  2 +-
 include/bootflow.h     |  2 +-
 include/bootmeth.h     |  5 ++---
 5 files changed, 12 insertions(+), 15 deletions(-)

diff --git a/boot/bootmeth-uclass.c b/boot/bootmeth-uclass.c
index 78a3671f96a..02bbe77b563 100644
--- a/boot/bootmeth-uclass.c
+++ b/boot/bootmeth-uclass.c
@@ -360,13 +360,11 @@ int bootmeth_alloc_file(struct bootflow *bflow, uint size_limit, uint align,
 }
 
 int bootmeth_alloc_other(struct bootflow *bflow, const char *fname,
-			 enum bootflow_img_t type, void **bufp, uint *sizep)
+			 enum bootflow_img_t type, struct abuf *buf)
 {
 	struct blk_desc *desc = NULL;
-	struct abuf buf;
 	char path[200];
 	loff_t size;
-	size_t bsize;
 	int ret;
 
 	snprintf(path, sizeof(path), "%s%s", bflow->subdir, fname);
@@ -386,16 +384,14 @@ int bootmeth_alloc_other(struct bootflow *bflow, const char *fname,
 	if (ret)
 		return log_msg_ret("fs", ret);
 
-	ret = fs_read_alloc(path, size, 0, &buf);
+	ret = fs_read_alloc(path, size, 0, buf);
 	if (ret)
 		return log_msg_ret("all", ret);
 
-	if (!bootflow_img_add(bflow, bflow->fname, type, abuf_addr(&buf), size))
+	if (!bootflow_img_add(bflow, bflow->fname, type, map_to_sysmem(buf),
+			      size))
 		return log_msg_ret("boi", -ENOMEM);
 
-	*bufp = abuf_uninit_move(&buf, &bsize);
-	*sizep = bsize;
-
 	return 0;
 }
 
diff --git a/boot/bootmeth_script.c b/boot/bootmeth_script.c
index 020cb8a7aec..b3a51a35e89 100644
--- a/boot/bootmeth_script.c
+++ b/boot/bootmeth_script.c
@@ -8,6 +8,7 @@
 
 #define LOG_CATEGORY UCLASS_BOOTSTD
 
+#include <abuf.h>
 #include <blk.h>
 #include <bootflow.h>
 #include <bootmeth.h>
@@ -68,6 +69,7 @@ static int script_read_bootflow_file(struct udevice *bootstd,
 	struct blk_desc *desc = NULL;
 	const char *const *prefixes;
 	const char *prefix;
+	struct abuf buf;
 	int ret, i;
 
 	ret = uclass_first_device_err(UCLASS_BOOTSTD, &bootstd);
@@ -107,8 +109,8 @@ static int script_read_bootflow_file(struct udevice *bootstd,
 	if (ret)
 		return log_msg_ret("inf", ret);
 
-	ret = bootmeth_alloc_other(bflow, "boot.bmp", BFI_LOGO,
-				   &bflow->logo, &bflow->logo_size);
+	ret = bootmeth_alloc_other(bflow, "boot.bmp", BFI_LOGO, &buf);
+	bflow->logo = abuf_uninit_move(&buf, &bflow->logo_size);
 	/* ignore error */
 
 	return 0;
diff --git a/cmd/bootflow.c b/cmd/bootflow.c
index e9ac9746104..0163129deba 100644
--- a/cmd/bootflow.c
+++ b/cmd/bootflow.c
@@ -419,7 +419,7 @@ static int do_bootflow_info(struct cmd_tbl *cmdtp, int flag, int argc,
 	printf("Logo:      %s\n", bflow->logo ?
 	       simple_xtoa((ulong)map_to_sysmem(bflow->logo)) : "(none)");
 	if (bflow->logo) {
-		printf("Logo size: %x (%d bytes)\n", bflow->logo_size,
+		printf("Logo size: %zx (%zd bytes)\n", bflow->logo_size,
 		       bflow->logo_size);
 	}
 	printf("FDT:       %s\n", bflow->fdt_fname);
diff --git a/include/bootflow.h b/include/bootflow.h
index 2caeb80b878..d988bc9355b 100644
--- a/include/bootflow.h
+++ b/include/bootflow.h
@@ -101,7 +101,7 @@ struct bootflow {
 	char *subdir;
 	char *fname;
 	void *logo;
-	uint logo_size;
+	size_t logo_size;
 	char *buf;
 	int size;
 	int err;
diff --git a/include/bootmeth.h b/include/bootmeth.h
index 03301c90580..c32bbbab7e9 100644
--- a/include/bootmeth.h
+++ b/include/bootmeth.h
@@ -385,12 +385,11 @@ int bootmeth_alloc_file(struct bootflow *bflow, uint size_limit, uint align,
  * @bflow: Information about file to read
  * @fname: Filename to read from (within bootflow->subdir)
  * @type: File type (IH_TYPE_...)
- * @bufp: Returns a pointer to the allocated buffer
- * @sizep: Returns the size of the buffer
+ * @buf: Returns the allocated buffer
  * Return: 0 if OK,  -ENOMEM if out of memory, other -ve on other error
  */
 int bootmeth_alloc_other(struct bootflow *bflow, const char *fname,
-			 enum bootflow_img_t type, void **bufp, uint *sizep);
+			 enum bootflow_img_t type, struct abuf *buf);
 
 /**
  * bootmeth_common_read_file() - Common handler for reading a file
-- 
2.43.0

[PATCH v2 05/18] bootstd: Add more debugging to bootmeth_efi

[Simon Glass]

Add a little more debugging to help figure out why bootflows are not
found.

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

(no changes since v1)

 boot/bootmeth_efi.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/boot/bootmeth_efi.c b/boot/bootmeth_efi.c
index 0c9b4c3d59d..01cc7ec3bec 100644
--- a/boot/bootmeth_efi.c
+++ b/boot/bootmeth_efi.c
@@ -103,6 +103,7 @@ static int distro_efi_try_bootflow_files(struct udevice *dev,
 	char fname[256];
 	int ret, seq;
 
+	log_debug("starting part %d\n", bflow->part);
 	/* We require a partition table */
 	if (!bflow->part) {
 		log_debug("no partitions\n");
@@ -116,7 +117,8 @@ static int distro_efi_try_bootflow_files(struct udevice *dev,
 		 desc = dev_get_uclass_plat(bflow->blk);
 	ret = bootmeth_try_file(bflow, desc, NULL, fname);
 	if (ret) {
-		log_debug("File '%s' not found\n", fname);
+		log_debug("File '%s' not found (desc devnum %d)\n", fname,
+			  desc->devnum);
 		return log_msg_ret("try", ret);
 	}
 
@@ -165,6 +167,7 @@ static int distro_efi_try_bootflow_files(struct udevice *dev,
 		log_debug("No device tree available\n");
 		bflow->flags |= BOOTFLOWF_USE_BUILTIN_FDT;
 	}
+	log_debug("found\n");
 
 	return 0;
 }
-- 
2.43.0

[PATCH v2 06/18] bootstd: Add more debugging to bootmeth_extlinux

[Simon Glass]

Add a little more debugging to help figure out why bootflows are not
found.

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

(no changes since v1)

 boot/bootmeth_extlinux.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/boot/bootmeth_extlinux.c b/boot/bootmeth_extlinux.c
index bb68391d551..71a9009479b 100644
--- a/boot/bootmeth_extlinux.c
+++ b/boot/bootmeth_extlinux.c
@@ -108,13 +108,18 @@ static int extlinux_read_bootflow(struct udevice *dev, struct bootflow *bflow)
 	loff_t size;
 	int ret, i;
 
+	log_debug("starting part %d\n", bflow->part);
 	ret = uclass_first_device_err(UCLASS_BOOTSTD, &bootstd);
-	if (ret)
+	if (ret) {
+		log_debug("no bootstd\n");
 		return log_msg_ret("std", ret);
+	}
 
 	/* If a block device, we require a partition table */
-	if (bflow->blk && !bflow->part)
+	if (bflow->blk && !bflow->part) {
+		log_debug("no partition table\n");
 		return -ENOENT;
+	}
 
 	prefixes = bootstd_get_prefixes(bootstd);
 	i = 0;
@@ -122,10 +127,13 @@ static int extlinux_read_bootflow(struct udevice *dev, struct bootflow *bflow)
 	do {
 		prefix = prefixes ? prefixes[i] : NULL;
 
+		log_debug("try prefix %s\n", prefix);
 		ret = bootmeth_try_file(bflow, desc, prefix, EXTLINUX_FNAME);
 	} while (ret && prefixes && prefixes[++i]);
-	if (ret)
+	if (ret) {
+		log_debug("no file found\n");
 		return log_msg_ret("try", ret);
+	}
 	size = bflow->size;
 
 	ret = bootmeth_alloc_file(bflow, 0x10000, ARCH_DMA_MINALIGN,
-- 
2.43.0

[PATCH v2 07/18] bootstd: Try all bootmeths on the final partition

[Simon Glass]

At present when one bootmeth fails on the final partition, the next
bootmeth is not tried. Adjust the logic to go to the next bootmeth,
which is the more natural behaviour.

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

(no changes since v1)

 boot/bootflow.c | 34 ++++++++++++++++------------------
 1 file changed, 16 insertions(+), 18 deletions(-)

diff --git a/boot/bootflow.c b/boot/bootflow.c
index 4054a966af8..345dd993c14 100644
--- a/boot/bootflow.c
+++ b/boot/bootflow.c
@@ -195,27 +195,25 @@ static int iter_incr(struct bootflow_iter *iter)
 	if (iter->err == BF_NO_MORE_DEVICES)
 		return BF_NO_MORE_DEVICES;
 
-	if (iter->err != BF_NO_MORE_PARTS) {
-		/* Get the next boothmethod */
-		if (++iter->cur_method < iter->num_methods) {
-			iter->method = iter->method_order[iter->cur_method];
-			return 0;
-		}
+	/* Get the next boothmethod */
+	if (++iter->cur_method < iter->num_methods) {
+		iter->method = iter->method_order[iter->cur_method];
+		return 0;
+	}
+
+	/*
+	 * If we have finished scanning the global bootmeths, start the
+	 * normal bootdev scan
+	 */
+	if (IS_ENABLED(CONFIG_BOOTMETH_GLOBAL) && global) {
+		iter->num_methods = iter->first_glob_method;
+		iter->doing_global = false;
 
 		/*
-		 * If we have finished scanning the global bootmeths, start the
-		 * normal bootdev scan
+		 * Don't move to the next dev as we haven't tried this
+		 * one yet!
 		 */
-		if (IS_ENABLED(CONFIG_BOOTMETH_GLOBAL) && global) {
-			iter->num_methods = iter->first_glob_method;
-			iter->doing_global = false;
-
-			/*
-			 * Don't move to the next dev as we haven't tried this
-			 * one yet!
-			 */
-			inc_dev = false;
-		}
+		inc_dev = false;
 	}
 
 	if (iter->flags & BOOTFLOWIF_SINGLE_PARTITION)
-- 
2.43.0

[PATCH v2 08/18] bootstd: Fully complete iteration of a uclass

[Simon Glass]

When trying all bootdevs in a uclass, the method flags are not preserved
in the iterator.

This has no impact on the first bootdev, since that is the one which
sets the flags. For the next one, iter_inc() is used and it finds the
next bootdev. However it sets the method_flags to 0

The result is that the third scan is conducted without the required
BOOTFLOW_METHF_SINGLE_UCLASS flag, so iteration procees to the next
label. This can miss bootdevs if there three or more USB-storage
devices, for example.

Fix this by keeping the method flags around in this case.

This patch is not quite a bug fix, but a further refinement on top of
this commit:

  7a790f018a8 ("bootstd: Scan all bootdevs in a boot_targets entry (take 2)")

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

(no changes since v1)

 boot/bootflow.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/boot/bootflow.c b/boot/bootflow.c
index 345dd993c14..76dadb93be1 100644
--- a/boot/bootflow.c
+++ b/boot/bootflow.c
@@ -288,7 +288,8 @@ static int iter_incr(struct bootflow_iter *iter)
 				 * bootdev_find_by_label() where this flag is
 				 * set up
 				 */
-				if (iter->method_flags &
+				method_flags = iter->method_flags;
+				if (method_flags &
 				    BOOTFLOW_METHF_SINGLE_UCLASS) {
 					scan_next_in_uclass(&dev);
 					log_debug("looking for next device %s: %s\n",
-- 
2.43.0

[PATCH v2 09/18] test/py: Split out core of Fedora image into a new function

[Simon Glass]

To permit easier adding of other images, move the Fedora-specific
portions of setup_bootflow_image() into a separate function.

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

(no changes since v1)

 test/py/tests/test_ut.py | 71 ++++++++++++++++++++++++++--------------
 1 file changed, 47 insertions(+), 24 deletions(-)

diff --git a/test/py/tests/test_ut.py b/test/py/tests/test_ut.py
index cdf54adc600..fc72496ba46 100644
--- a/test/py/tests/test_ut.py
+++ b/test/py/tests/test_ut.py
@@ -157,27 +157,22 @@ booti ${kernel_addr_r} ${ramdisk_addr_r} ${fdt_addr_r}
     utils.run_and_log(ubman, f'rm -rf {mnt}')
     utils.run_and_log(ubman, f'rm -f {fsfile}')
 
-def setup_bootflow_image(ubman):
-    """Create a 20MB disk image with a single FAT partition"""
-    mmc_dev = 1
-    fname, mnt = fs_helper.setup_image(ubman, mmc_dev, 0xc, second_part=True)
+def setup_bootflow_image(ubman, devnum, basename, vmlinux, initrd, dtbdir,
+                         script):
+    """Create a 20MB disk image with a single FAT partition
 
-    vmlinux = 'vmlinuz-5.3.7-301.fc31.armv7hl'
-    initrd = 'initramfs-5.3.7-301.fc31.armv7hl.img'
-    dtbdir = 'dtb-5.3.7-301.fc31.armv7hl'
-    script = '''# extlinux.conf generated by appliance-creator
-ui menu.c32
-menu autoboot Welcome to Fedora-Workstation-armhfp-31-1.9. Automatic boot in # second{,s}. Press a key for options.
-menu title Fedora-Workstation-armhfp-31-1.9 Boot Options.
-menu hidden
-timeout 20
-totaltimeout 600
+    Args:
+        ubman (ConsoleBase): Console to use
+        devnum (int): Device number to use, e.g. 1
+        basename (str): Base name to use in the filename, e.g. 'mmc'
+        vmlinux (str): Kernel filename
+        initrd (str): Ramdisk filename
+        dtbdir (str or None): Devicetree filename
+        script (str): Script to place in the extlinux.conf file
+    """
+    fname, mnt = fs_helper.setup_image(ubman, devnum, 0xc, second_part=True,
+                                       basename=basename)
 
-label Fedora-Workstation-armhfp-31-1.9 (5.3.7-301.fc31.armv7hl)
-        kernel /%s
-        append ro root=UUID=9732b35b-4cd5-458b-9b91-80f7047e0b8a rhgb quiet LANG=en_US.UTF-8 cma=192MB cma=256MB
-        fdtdir /%s/
-        initrd /%s''' % (vmlinux, dtbdir, initrd)
     ext = os.path.join(mnt, 'extlinux')
     mkdir_cond(ext)
 
@@ -195,11 +190,12 @@ label Fedora-Workstation-armhfp-31-1.9 (5.3.7-301.fc31.armv7hl)
     with open(os.path.join(mnt, initrd), 'w', encoding='ascii') as fd:
         print('initrd', file=fd)
 
-    mkdir_cond(os.path.join(mnt, dtbdir))
+    if dtbdir:
+        mkdir_cond(os.path.join(mnt, dtbdir))
 
-    dtb_file = os.path.join(mnt, f'{dtbdir}/sandbox.dtb')
-    utils.run_and_log(
-        ubman, f'dtc -o {dtb_file}', stdin=b'/dts-v1/; / {};')
+        dtb_file = os.path.join(mnt, f'{dtbdir}/sandbox.dtb')
+        utils.run_and_log(
+            ubman, f'dtc -o {dtb_file}', stdin=b'/dts-v1/; / {};')
 
     fsfile = 'vfat18M.img'
     utils.run_and_log(ubman, f'fallocate -l 18M {fsfile}')
@@ -209,6 +205,33 @@ label Fedora-Workstation-armhfp-31-1.9 (5.3.7-301.fc31.armv7hl)
     utils.run_and_log(ubman, f'rm -rf {mnt}')
     utils.run_and_log(ubman, f'rm -f {fsfile}')
 
+def setup_fedora_image(ubman, devnum, basename):
+    """Create a 20MB Fedora disk image with a single FAT partition
+
+    Args:
+        ubman (ConsoleBase): Console to use
+        devnum (int): Device number to use, e.g. 1
+        basename (str): Base name to use in the filename, e.g. 'mmc'
+    """
+    vmlinux = 'vmlinuz-5.3.7-301.fc31.armv7hl'
+    initrd = 'initramfs-5.3.7-301.fc31.armv7hl.img'
+    dtbdir = 'dtb-5.3.7-301.fc31.armv7hl'
+    script = '''# extlinux.conf generated by appliance-creator
+ui menu.c32
+menu autoboot Welcome to Fedora-Workstation-armhfp-31-1.9. Automatic boot in # second{,s}. Press a key for options.
+menu title Fedora-Workstation-armhfp-31-1.9 Boot Options.
+menu hidden
+timeout 20
+totaltimeout 600
+
+label Fedora-Workstation-armhfp-31-1.9 (5.3.7-301.fc31.armv7hl)
+        kernel /%s
+        append ro root=UUID=9732b35b-4cd5-458b-9b91-80f7047e0b8a rhgb quiet LANG=en_US.UTF-8 cma=192MB cma=256MB
+        fdtdir /%s/
+        initrd /%s''' % (vmlinux, dtbdir, initrd)
+    setup_bootflow_image(ubman, devnum, basename, vmlinux, initrd, dtbdir,
+                         script)
+
 def setup_cros_image(ubman):
     """Create a 20MB disk image with ChromiumOS partitions"""
     Partition = collections.namedtuple('part', 'start,size,name')
@@ -552,7 +575,7 @@ def setup_efi_image(ubman):
 def test_ut_dm_init_bootstd(ubman):
     """Initialise data for bootflow tests"""
 
-    setup_bootflow_image(ubman)
+    setup_fedora_image(ubman, 1, 'mmc')
     setup_bootmenu_image(ubman)
     setup_cedit_file(ubman)
     setup_cros_image(ubman)
-- 
2.43.0

[PATCH v2 10/18] test/py: Add an extlinux file similar to Ubuntu

[Simon Glass]

Add an extlinux configuration-file that contains a few entries as
created by the u-boot-menu package in Ubuntu 24.04

Increase the number of sandbox-USB-hub ports to permit this.

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

Changes in v2:
- Update commit message to make it clear this is not actually an image

 arch/sandbox/dts/test.dts      |  9 +++++++-
 drivers/usb/emul/sandbox_hub.c |  2 +-
 test/boot/bootdev.c            | 34 +++++++++++++++++++----------
 test/boot/bootflow.c           |  6 +++--
 test/dm/blk.c                  |  4 ++--
 test/dm/usb.c                  |  4 ++--
 test/py/tests/test_ut.py       | 40 ++++++++++++++++++++++++++++++++++
 7 files changed, 79 insertions(+), 20 deletions(-)

diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
index 6cd87b74a64..060c48eaefd 100644
--- a/arch/sandbox/dts/test.dts
+++ b/arch/sandbox/dts/test.dts
@@ -1515,6 +1515,7 @@
 			#address-cells = <1>;
 			#size-cells = <0>;
 			hub-emul {
+				/* See SANDBOX_NUM_PORTS if you need more ports */
 				compatible = "sandbox,usb-hub";
 				#address-cells = <1>;
 				#size-cells = <0>;
@@ -1536,8 +1537,14 @@
 					sandbox,filepath = "testflash2.bin";
 				};
 
-				keyb@3 {
+				flash-stick@3 {
 					reg = <3>;
+					compatible = "sandbox,usb-flash";
+					sandbox,filepath = "flash3.img";
+				};
+
+				keyb@4 {
+					reg = <4>;
 					compatible = "sandbox,usb-keyb";
 				};
 
diff --git a/drivers/usb/emul/sandbox_hub.c b/drivers/usb/emul/sandbox_hub.c
index 3b3e59f978f..dc348d152c2 100644
--- a/drivers/usb/emul/sandbox_hub.c
+++ b/drivers/usb/emul/sandbox_hub.c
@@ -10,7 +10,7 @@
 #include <dm/device-internal.h>
 
 /* We only support up to 8 */
-#define SANDBOX_NUM_PORTS	4
+#define SANDBOX_NUM_PORTS	8
 
 struct sandbox_hub_plat {
 	struct usb_dev_plat plat;
diff --git a/test/boot/bootdev.c b/test/boot/bootdev.c
index d5499918249..4333f3c977c 100644
--- a/test/boot/bootdev.c
+++ b/test/boot/bootdev.c
@@ -221,11 +221,14 @@ static int bootdev_test_order(struct unit_test_state *uts)
 	ut_assertok(env_set("boot_targets", "mmc1 mmc2 usb"));
 	ut_assertok(bootflow_scan_first(NULL, NULL, &iter, 0, &bflow));
 
-	/* get the usb device which has a backing file (flash1.img) */
+	/* get the first usb device which has a backing file (flash1.img) */
+	ut_asserteq(0, bootflow_scan_next(&iter, &bflow));
+
+	/* get the second usb device which has a backing file (flash3.img) */
 	ut_asserteq(0, bootflow_scan_next(&iter, &bflow));
 
 	ut_asserteq(-ENODEV, bootflow_scan_next(&iter, &bflow));
-	ut_asserteq(5, iter.num_devs);
+	ut_asserteq(6, iter.num_devs);
 	ut_asserteq_str("mmc1.bootdev", iter.dev_used[0]->name);
 	ut_asserteq_str("mmc2.bootdev", iter.dev_used[1]->name);
 	ut_asserteq_str("usb_mass_storage.lun0.bootdev",
@@ -264,12 +267,13 @@ static int bootdev_test_order(struct unit_test_state *uts)
 	ut_asserteq(2, iter.num_devs);
 
 	/*
-	 * Now scan past mmc1 and make sure that the 3 USB devices show up. The
-	 * first one has a backing file so returns success
+	 * Now scan past mmc1 and make sure that the 4 USB devices show up. The
+	 * first two have a backing file so returns success
 	 */
 	ut_asserteq(0, bootflow_scan_next(&iter, &bflow));
+	ut_asserteq(0, bootflow_scan_next(&iter, &bflow));
 	ut_asserteq(-ENODEV, bootflow_scan_next(&iter, &bflow));
-	ut_asserteq(6, iter.num_devs);
+	ut_asserteq(7, iter.num_devs);
 	ut_asserteq_str("mmc2.bootdev", iter.dev_used[0]->name);
 	ut_asserteq_str("mmc1.bootdev", iter.dev_used[1]->name);
 	ut_asserteq_str("mmc0.bootdev", iter.dev_used[2]->name);
@@ -330,11 +334,14 @@ static int bootdev_test_prio(struct unit_test_state *uts)
 	/* 3 MMC and 3 USB bootdevs: MMC should come before USB */
 	ut_assertok(bootflow_scan_first(NULL, NULL, &iter, 0, &bflow));
 
-	/* get the usb device which has a backing file (flash1.img) */
+	/* get the first usb device which has a backing file (flash1.img) */
+	ut_asserteq(0, bootflow_scan_next(&iter, &bflow));
+
+	/* get the second usb device which has a backing file (flash3.img) */
 	ut_asserteq(0, bootflow_scan_next(&iter, &bflow));
 
 	ut_asserteq(-ENODEV, bootflow_scan_next(&iter, &bflow));
-	ut_asserteq(6, iter.num_devs);
+	ut_asserteq(7, iter.num_devs);
 	ut_asserteq_str("mmc2.bootdev", iter.dev_used[0]->name);
 	ut_asserteq_str("usb_mass_storage.lun0.bootdev",
 			iter.dev_used[3]->name);
@@ -351,11 +358,14 @@ static int bootdev_test_prio(struct unit_test_state *uts)
 	ut_assertok(bootflow_scan_first(NULL, NULL, &iter, BOOTFLOWIF_HUNT,
 					&bflow));
 
-	/* get the usb device which has a backing file (flash1.img) */
+	/* get the first usb device which has a backing file (flash1.img) */
+	ut_asserteq(0, bootflow_scan_next(&iter, &bflow));
+
+	/* get the second usb device which has a backing file (flash3.img) */
 	ut_asserteq(0, bootflow_scan_next(&iter, &bflow));
 
 	ut_asserteq(-ENODEV, bootflow_scan_next(&iter, &bflow));
-	ut_asserteq(7, iter.num_devs);
+	ut_asserteq(8, iter.num_devs);
 	ut_asserteq_str("usb_mass_storage.lun0.bootdev",
 			iter.dev_used[0]->name);
 	ut_asserteq_str("mmc2.bootdev", iter.dev_used[1]->name);
@@ -393,7 +403,7 @@ static int bootdev_test_hunter(struct unit_test_state *uts)
 
 	ut_assertok(bootdev_hunt("usb1", false));
 	ut_assert_nextline(
-		"Bus usb@1: scanning bus usb@1 for devices... 5 USB Device(s) found");
+		"Bus usb@1: scanning bus usb@1 for devices... 6 USB Device(s) found");
 	ut_assert_console_end();
 
 	/* USB is 7th in the list, so bit 8 */
@@ -449,7 +459,7 @@ static int bootdev_test_cmd_hunt(struct unit_test_state *uts)
 	ut_assert_skip_to_line("Hunting with: spi_flash");
 	ut_assert_nextline("Hunting with: usb");
 	ut_assert_nextline(
-		"Bus usb@1: scanning bus usb@1 for devices... 5 USB Device(s) found");
+		"Bus usb@1: scanning bus usb@1 for devices... 6 USB Device(s) found");
 	ut_assert_nextline("Hunting with: virtio");
 	ut_assert_console_end();
 
@@ -552,7 +562,7 @@ static int bootdev_test_hunt_prio(struct unit_test_state *uts)
 	ut_assert_nextline("Hunting with: ide");
 	ut_assert_nextline("Hunting with: usb");
 	ut_assert_nextline(
-		"Bus usb@1: scanning bus usb@1 for devices... 5 USB Device(s) found");
+		"Bus usb@1: scanning bus usb@1 for devices... 6 USB Device(s) found");
 	ut_assert_console_end();
 
 	return 0;
diff --git a/test/boot/bootflow.c b/test/boot/bootflow.c
index 1447af2eb14..3ee87145dc8 100644
--- a/test/boot/bootflow.c
+++ b/test/boot/bootflow.c
@@ -1298,7 +1298,7 @@ static int bootflow_efi(struct unit_test_state *uts)
 
 	ut_assertok(run_command("bootflow scan", 0));
 	ut_assert_skip_to_line(
-		"Bus usb@1: scanning bus usb@1 for devices... 5 USB Device(s) found");
+		"Bus usb@1: scanning bus usb@1 for devices... 6 USB Device(s) found");
 
 	ut_assertok(run_command("bootflow list", 0));
 
@@ -1308,8 +1308,10 @@ static int bootflow_efi(struct unit_test_state *uts)
 	ut_assert_nextlinen("  0  extlinux");
 	ut_assert_nextlinen(
 		"  1  efi          ready   usb_mass_    1  usb_mass_storage.lun0.boo /EFI/BOOT/BOOTSBOX.EFI");
+	ut_assert_nextlinen(
+		"  2  extlinux     ready   usb_mass_    1  usb_mass_storage.lun0.boo /extlinux/extlinux.conf");
 	ut_assert_nextlinen("---");
-	ut_assert_skip_to_line("(2 bootflows, 2 valid)");
+	ut_assert_skip_to_line("(3 bootflows, 3 valid)");
 	ut_assert_console_end();
 
 	ut_assertok(run_command("bootflow select 1", 0));
diff --git a/test/dm/blk.c b/test/dm/blk.c
index aa5cbc63777..2e889d1a418 100644
--- a/test/dm/blk.c
+++ b/test/dm/blk.c
@@ -82,12 +82,12 @@ static int dm_test_blk_usb(struct unit_test_state *uts)
 	ut_asserteq_ptr(usb_dev, dev_get_parent(dev));
 
 	/* Check we have one block device for each mass storage device */
-	ut_asserteq(6, count_blk_devices());
+	ut_asserteq(7, count_blk_devices());
 
 	/* Now go around again, making sure the old devices were unbound */
 	ut_assertok(usb_stop());
 	ut_assertok(usb_init());
-	ut_asserteq(6, count_blk_devices());
+	ut_asserteq(7, count_blk_devices());
 	ut_assertok(usb_stop());
 
 	return 0;
diff --git a/test/dm/usb.c b/test/dm/usb.c
index fa894c1096e..d89d436d269 100644
--- a/test/dm/usb.c
+++ b/test/dm/usb.c
@@ -158,7 +158,7 @@ static int dm_test_usb_stop(struct unit_test_state *uts)
 	ut_assertok(uclass_get_device(UCLASS_MASS_STORAGE, 0, &dev));
 	ut_assertok(uclass_get_device(UCLASS_MASS_STORAGE, 1, &dev));
 	ut_assertok(uclass_get_device(UCLASS_MASS_STORAGE, 2, &dev));
-	ut_asserteq(6, count_usb_devices());
+	ut_asserteq(7, count_usb_devices());
 	ut_assertok(usb_stop());
 	ut_asserteq(0, count_usb_devices());
 
@@ -429,7 +429,7 @@ static int dm_test_usb_keyb(struct unit_test_state *uts)
 	/* Initially there should be no characters */
 	ut_asserteq(0, tstc());
 
-	ut_assertok(uclass_get_device_by_name(UCLASS_USB_EMUL, "keyb@3",
+	ut_assertok(uclass_get_device_by_name(UCLASS_USB_EMUL, "keyb@4",
 					      &dev));
 
 	/*
diff --git a/test/py/tests/test_ut.py b/test/py/tests/test_ut.py
index fc72496ba46..85ba6e3f5c3 100644
--- a/test/py/tests/test_ut.py
+++ b/test/py/tests/test_ut.py
@@ -232,6 +232,45 @@ label Fedora-Workstation-armhfp-31-1.9 (5.3.7-301.fc31.armv7hl)
     setup_bootflow_image(ubman, devnum, basename, vmlinux, initrd, dtbdir,
                          script)
 
+def setup_ubuntu_image(ubman, devnum, basename):
+    """Create a 20MB Ubuntu disk image with a single FAT partition
+
+    Args:
+        ubman (ConsoleBase): Console to use
+        devnum (int): Device number to use, e.g. 1
+        basename (str): Base name to use in the filename, e.g. 'mmc'
+    """
+    vmlinux = 'vmlinuz-6.8.0-53-generic'
+    initrd = 'initrd.img-6.8.0-53-generic'
+    dtbdir = None
+    script = '''## /boot/extlinux/extlinux.conf
+##
+## IMPORTANT WARNING
+##
+## The configuration of this file is generated automatically.
+## Do not edit this file manually, use: u-boot-update
+
+default l0
+menu title U-Boot menu
+prompt 1
+timeout 50
+
+
+label l0
+	menu label Ubuntu 24.04.1 LTS 6.8.0-53-generic
+	linux /boot/%s
+	initrd /boot/%s
+
+	append root=/dev/disk/by-uuid/bcfdda4a-8249-4f40-9f0f-7c1a76b6cbe8 ro earlycon
+
+label l0r
+	menu label Ubuntu 24.04.1 LTS 6.8.0-53-generic (rescue target)
+	linux /boot/%s
+	initrd /boot/%s
+''' % (vmlinux, initrd, vmlinux, initrd)
+    setup_bootflow_image(ubman, devnum, basename, vmlinux, initrd, dtbdir,
+                         script)
+
 def setup_cros_image(ubman):
     """Create a 20MB disk image with ChromiumOS partitions"""
     Partition = collections.namedtuple('part', 'start,size,name')
@@ -581,6 +620,7 @@ def test_ut_dm_init_bootstd(ubman):
     setup_cros_image(ubman)
     setup_android_image(ubman)
     setup_efi_image(ubman)
+    setup_ubuntu_image(ubman, 3, 'flash')
 
     # Restart so that the new mmc1.img is picked up
     ubman.restart_uboot()
-- 
2.43.0

[PATCH v2 11/18] usb: Use more useful names for block devices

[Simon Glass]

The driver name is typically not unique so using that as a basis for the
block and bootdev devices makes them hard to distinguish. This happens
when there are multiple USB controllers using the same driver.

Make use of the parent-device name and the hub port number. This gives a
reasonable chance that the name is unique.

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

(no changes since v1)

 drivers/usb/host/usb-uclass.c | 14 ++++++++++++--
 test/boot/bootdev.c           | 12 ++++++------
 test/boot/bootflow.c          |  6 +++---
 3 files changed, 21 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c
index bfec303e7af..43c8ec352f0 100644
--- a/drivers/usb/host/usb-uclass.c
+++ b/drivers/usb/host/usb-uclass.c
@@ -574,10 +574,20 @@ static int usb_find_and_bind_driver(struct udevice *parent,
 		struct usb_dev_plat *plat;
 
 		for (id = entry->match; id->match_flags; id++) {
+			char dev_name[30], *str;
+
 			if (!usb_match_one_id(desc, iface, id))
 				continue;
 
 			drv = entry->driver;
+			snprintf(dev_name, sizeof(dev_name), "%s.p%d.%s",
+				 parent->name, port, drv->name);
+			str = strdup(dev_name);
+			if (!str) {
+				ret = -ENOMEM;
+				goto error;
+			}
+
 			/*
 			 * We could pass the descriptor to the driver as
 			 * plat (instead of NULL) and allow its bind()
@@ -586,10 +596,10 @@ static int usb_find_and_bind_driver(struct udevice *parent,
 			 * find another driver. For now this doesn't seem
 			 * necesssary, so just bind the first match.
 			 */
-			ret = device_bind(parent, drv, drv->name, NULL, node,
-					  &dev);
+			ret = device_bind(parent, drv, str, NULL, node, &dev);
 			if (ret)
 				goto error;
+			device_set_name_alloced(dev);
 			debug("%s: Match found: %s\n", __func__, drv->name);
 			dev->driver_data = id->driver_info;
 			plat = dev_get_parent_plat(dev);
diff --git a/test/boot/bootdev.c b/test/boot/bootdev.c
index 4333f3c977c..7ef724f65bb 100644
--- a/test/boot/bootdev.c
+++ b/test/boot/bootdev.c
@@ -231,7 +231,7 @@ static int bootdev_test_order(struct unit_test_state *uts)
 	ut_asserteq(6, iter.num_devs);
 	ut_asserteq_str("mmc1.bootdev", iter.dev_used[0]->name);
 	ut_asserteq_str("mmc2.bootdev", iter.dev_used[1]->name);
-	ut_asserteq_str("usb_mass_storage.lun0.bootdev",
+	ut_asserteq_str("hub.p1.usb_mass_storage.lun0.bootdev",
 			iter.dev_used[2]->name);
 	bootflow_iter_uninit(&iter);
 
@@ -277,7 +277,7 @@ static int bootdev_test_order(struct unit_test_state *uts)
 	ut_asserteq_str("mmc2.bootdev", iter.dev_used[0]->name);
 	ut_asserteq_str("mmc1.bootdev", iter.dev_used[1]->name);
 	ut_asserteq_str("mmc0.bootdev", iter.dev_used[2]->name);
-	ut_asserteq_str("usb_mass_storage.lun0.bootdev",
+	ut_asserteq_str("hub.p1.usb_mass_storage.lun0.bootdev",
 			iter.dev_used[3]->name);
 	bootflow_iter_uninit(&iter);
 
@@ -343,11 +343,11 @@ static int bootdev_test_prio(struct unit_test_state *uts)
 	ut_asserteq(-ENODEV, bootflow_scan_next(&iter, &bflow));
 	ut_asserteq(7, iter.num_devs);
 	ut_asserteq_str("mmc2.bootdev", iter.dev_used[0]->name);
-	ut_asserteq_str("usb_mass_storage.lun0.bootdev",
+	ut_asserteq_str("hub.p1.usb_mass_storage.lun0.bootdev",
 			iter.dev_used[3]->name);
 
 	ut_assertok(bootdev_get_sibling_blk(iter.dev_used[3], &blk));
-	ut_asserteq_str("usb_mass_storage.lun0", blk->name);
+	ut_asserteq_str("hub.p1.usb_mass_storage.lun0", blk->name);
 
 	/* adjust the priority of the first USB bootdev to the highest */
 	ucp = dev_get_uclass_plat(iter.dev_used[3]);
@@ -366,7 +366,7 @@ static int bootdev_test_prio(struct unit_test_state *uts)
 
 	ut_asserteq(-ENODEV, bootflow_scan_next(&iter, &bflow));
 	ut_asserteq(8, iter.num_devs);
-	ut_asserteq_str("usb_mass_storage.lun0.bootdev",
+	ut_asserteq_str("hub.p1.usb_mass_storage.lun0.bootdev",
 			iter.dev_used[0]->name);
 	ut_asserteq_str("mmc2.bootdev", iter.dev_used[1]->name);
 
@@ -612,7 +612,7 @@ static int bootdev_test_hunt_label(struct unit_test_state *uts)
 	test_set_skip_delays(true);
 	ut_assertok(bootdev_hunt_and_find_by_label("usb", &dev, &mflags));
 	ut_assertnonnull(dev);
-	ut_asserteq_str("usb_mass_storage.lun0.bootdev", dev->name);
+	ut_asserteq_str("hub.p1.usb_mass_storage.lun0.bootdev", dev->name);
 	ut_asserteq(BOOTFLOW_METHF_SINGLE_UCLASS, mflags);
 	ut_assert_nextlinen("Bus usb@1: scanning bus usb@1");
 	ut_assert_console_end();
diff --git a/test/boot/bootflow.c b/test/boot/bootflow.c
index 3ee87145dc8..f1341312dac 100644
--- a/test/boot/bootflow.c
+++ b/test/boot/bootflow.c
@@ -1307,9 +1307,9 @@ static int bootflow_efi(struct unit_test_state *uts)
 	ut_assert_nextlinen("---");
 	ut_assert_nextlinen("  0  extlinux");
 	ut_assert_nextlinen(
-		"  1  efi          ready   usb_mass_    1  usb_mass_storage.lun0.boo /EFI/BOOT/BOOTSBOX.EFI");
+		"  1  efi          ready   usb_mass_    1  hub.p2.usb_mass_storage.l /EFI/BOOT/BOOTSBOX.EFI");
 	ut_assert_nextlinen(
-		"  2  extlinux     ready   usb_mass_    1  usb_mass_storage.lun0.boo /extlinux/extlinux.conf");
+		"  2  extlinux     ready   usb_mass_    1  hub.p4.usb_mass_storage.l /extlinux/extlinux.conf");
 	ut_assert_nextlinen("---");
 	ut_assert_skip_to_line("(3 bootflows, 3 valid)");
 	ut_assert_console_end();
@@ -1325,7 +1325,7 @@ static int bootflow_efi(struct unit_test_state *uts)
 
 	ut_asserteq(1, run_command("bootflow boot", 0));
 	ut_assert_nextline(
-		"** Booting bootflow 'usb_mass_storage.lun0.bootdev.part_1' with efi");
+		"** Booting bootflow 'hub.p2.usb_mass_storage.lun0.bootdev.part_1' with efi");
 	if (IS_ENABLED(CONFIG_LOGF_FUNC))
 		ut_assert_skip_to_line("       efi_run_image() Booting /\\EFI\\BOOT\\BOOTSBOX.EFI");
 	else
-- 
2.43.0

[PATCH v2 12/18] sandbox: Use a unique name for each USB controller

[Simon Glass]

Add a number after the node name so that it is clear which controller is
being used.

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

(no changes since v1)

 arch/sandbox/dts/test.dts |  4 ++--
 test/boot/bootdev.c       | 12 ++++++------
 test/boot/bootflow.c      |  6 +++---
 test/dm/usb.c             |  4 ++--
 4 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
index 060c48eaefd..caec8c8fd01 100644
--- a/arch/sandbox/dts/test.dts
+++ b/arch/sandbox/dts/test.dts
@@ -1495,7 +1495,7 @@
 	usb_0: usb@0 {
 		compatible = "sandbox,usb";
 		status = "disabled";
-		hub {
+		hub0 {
 			compatible = "sandbox,usb-hub";
 			#address-cells = <1>;
 			#size-cells = <0>;
@@ -1509,7 +1509,7 @@
 	usb_1: usb@1 {
 		compatible = "sandbox,usb";
 		iommus = <&iommu>;
-		hub {
+		hub1 {
 			compatible = "usb-hub";
 			usb,device-class = <9>;
 			#address-cells = <1>;
diff --git a/test/boot/bootdev.c b/test/boot/bootdev.c
index 7ef724f65bb..0ace3d0699c 100644
--- a/test/boot/bootdev.c
+++ b/test/boot/bootdev.c
@@ -231,7 +231,7 @@ static int bootdev_test_order(struct unit_test_state *uts)
 	ut_asserteq(6, iter.num_devs);
 	ut_asserteq_str("mmc1.bootdev", iter.dev_used[0]->name);
 	ut_asserteq_str("mmc2.bootdev", iter.dev_used[1]->name);
-	ut_asserteq_str("hub.p1.usb_mass_storage.lun0.bootdev",
+	ut_asserteq_str("hub1.p1.usb_mass_storage.lun0.bootdev",
 			iter.dev_used[2]->name);
 	bootflow_iter_uninit(&iter);
 
@@ -277,7 +277,7 @@ static int bootdev_test_order(struct unit_test_state *uts)
 	ut_asserteq_str("mmc2.bootdev", iter.dev_used[0]->name);
 	ut_asserteq_str("mmc1.bootdev", iter.dev_used[1]->name);
 	ut_asserteq_str("mmc0.bootdev", iter.dev_used[2]->name);
-	ut_asserteq_str("hub.p1.usb_mass_storage.lun0.bootdev",
+	ut_asserteq_str("hub1.p1.usb_mass_storage.lun0.bootdev",
 			iter.dev_used[3]->name);
 	bootflow_iter_uninit(&iter);
 
@@ -343,11 +343,11 @@ static int bootdev_test_prio(struct unit_test_state *uts)
 	ut_asserteq(-ENODEV, bootflow_scan_next(&iter, &bflow));
 	ut_asserteq(7, iter.num_devs);
 	ut_asserteq_str("mmc2.bootdev", iter.dev_used[0]->name);
-	ut_asserteq_str("hub.p1.usb_mass_storage.lun0.bootdev",
+	ut_asserteq_str("hub1.p1.usb_mass_storage.lun0.bootdev",
 			iter.dev_used[3]->name);
 
 	ut_assertok(bootdev_get_sibling_blk(iter.dev_used[3], &blk));
-	ut_asserteq_str("hub.p1.usb_mass_storage.lun0", blk->name);
+	ut_asserteq_str("hub1.p1.usb_mass_storage.lun0", blk->name);
 
 	/* adjust the priority of the first USB bootdev to the highest */
 	ucp = dev_get_uclass_plat(iter.dev_used[3]);
@@ -366,7 +366,7 @@ static int bootdev_test_prio(struct unit_test_state *uts)
 
 	ut_asserteq(-ENODEV, bootflow_scan_next(&iter, &bflow));
 	ut_asserteq(8, iter.num_devs);
-	ut_asserteq_str("hub.p1.usb_mass_storage.lun0.bootdev",
+	ut_asserteq_str("hub1.p1.usb_mass_storage.lun0.bootdev",
 			iter.dev_used[0]->name);
 	ut_asserteq_str("mmc2.bootdev", iter.dev_used[1]->name);
 
@@ -612,7 +612,7 @@ static int bootdev_test_hunt_label(struct unit_test_state *uts)
 	test_set_skip_delays(true);
 	ut_assertok(bootdev_hunt_and_find_by_label("usb", &dev, &mflags));
 	ut_assertnonnull(dev);
-	ut_asserteq_str("hub.p1.usb_mass_storage.lun0.bootdev", dev->name);
+	ut_asserteq_str("hub1.p1.usb_mass_storage.lun0.bootdev", dev->name);
 	ut_asserteq(BOOTFLOW_METHF_SINGLE_UCLASS, mflags);
 	ut_assert_nextlinen("Bus usb@1: scanning bus usb@1");
 	ut_assert_console_end();
diff --git a/test/boot/bootflow.c b/test/boot/bootflow.c
index f1341312dac..cbb5bcc0cf3 100644
--- a/test/boot/bootflow.c
+++ b/test/boot/bootflow.c
@@ -1307,9 +1307,9 @@ static int bootflow_efi(struct unit_test_state *uts)
 	ut_assert_nextlinen("---");
 	ut_assert_nextlinen("  0  extlinux");
 	ut_assert_nextlinen(
-		"  1  efi          ready   usb_mass_    1  hub.p2.usb_mass_storage.l /EFI/BOOT/BOOTSBOX.EFI");
+		"  1  efi          ready   usb_mass_    1  hub1.p2.usb_mass_storage. /EFI/BOOT/BOOTSBOX.EFI");
 	ut_assert_nextlinen(
-		"  2  extlinux     ready   usb_mass_    1  hub.p4.usb_mass_storage.l /extlinux/extlinux.conf");
+		"  2  extlinux     ready   usb_mass_    1  hub1.p4.usb_mass_storage. /extlinux/extlinux.conf");
 	ut_assert_nextlinen("---");
 	ut_assert_skip_to_line("(3 bootflows, 3 valid)");
 	ut_assert_console_end();
@@ -1325,7 +1325,7 @@ static int bootflow_efi(struct unit_test_state *uts)
 
 	ut_asserteq(1, run_command("bootflow boot", 0));
 	ut_assert_nextline(
-		"** Booting bootflow 'hub.p2.usb_mass_storage.lun0.bootdev.part_1' with efi");
+		"** Booting bootflow 'hub1.p2.usb_mass_storage.lun0.bootdev.part_1' with efi");
 	if (IS_ENABLED(CONFIG_LOGF_FUNC))
 		ut_assert_skip_to_line("       efi_run_image() Booting /\\EFI\\BOOT\\BOOTSBOX.EFI");
 	else
diff --git a/test/dm/usb.c b/test/dm/usb.c
index d89d436d269..66777b5ef00 100644
--- a/test/dm/usb.c
+++ b/test/dm/usb.c
@@ -109,12 +109,12 @@ static int dm_test_usb_fdt_node(struct unit_test_state *uts)
 	state_set_skip_delays(true);
 	ut_assertok(usb_init());
 	ut_assertok(uclass_get_device(UCLASS_MASS_STORAGE, 0, &dev));
-	node = ofnode_path("/usb@1/hub/usbstor@1");
+	node = ofnode_path("/usb@1/hub1/usbstor@1");
 	ut_asserteq(1, ofnode_equal(node, dev_ofnode(dev)));
 	ut_assertok(uclass_get_device(UCLASS_MASS_STORAGE, 1, &dev));
 	ut_asserteq(1, ofnode_equal(ofnode_null(), dev_ofnode(dev)));
 	ut_assertok(uclass_get_device(UCLASS_MASS_STORAGE, 2, &dev));
-	node = ofnode_path("/usb@1/hub/usbstor@3");
+	node = ofnode_path("/usb@1/hub1/usbstor@3");
 	ut_asserteq(1, ofnode_equal(node, dev_ofnode(dev)));
 	ut_assertok(usb_stop());
 
-- 
2.43.0

[PATCH v2 13/18] bootstd: Tweak scanning with labels

[Simon Glass]

The current implementation of labels uses the ordering of the media
devices to start its scan but then uses the ordering of the (child)
bootdev devices to iterate from then on. This is inconsistent and can
miss some bootdevs.

Update bootdev_next_label() so that it uses the ordering of the bootdev
devices to perform its scan. Check for running out of labels as well.

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

(no changes since v1)

 boot/bootdev-uclass.c | 70 +++++++++++++++++++++++--------------------
 1 file changed, 38 insertions(+), 32 deletions(-)

diff --git a/boot/bootdev-uclass.c b/boot/bootdev-uclass.c
index 3791ebfcb42..cfb298d5be5 100644
--- a/boot/bootdev-uclass.c
+++ b/boot/bootdev-uclass.c
@@ -421,7 +421,7 @@ int bootdev_find_by_label(const char *label, struct udevice **devp,
 			  int *method_flagsp)
 {
 	int seq, ret, method_flags = 0;
-	struct udevice *media;
+	struct udevice *bdev;
 	struct uclass *uc;
 	enum uclass_id id;
 
@@ -433,10 +433,27 @@ int bootdev_find_by_label(const char *label, struct udevice **devp,
 		return log_msg_ret("uc", ret);
 	id = ret;
 
-	/* Iterate through devices in the media uclass (e.g. UCLASS_MMC) */
-	uclass_id_foreach_dev(id, media, uc) {
-		struct udevice *bdev, *blk;
-		int ret;
+	/*
+	 * Iterate through all the bootdevs looking for the first one for the
+	 * correct media uclass (e.g. UCLASS_MMC). We do it this way so that
+	 * iter_incr() can continue iterating through others with the same media
+	 * uclass, thus making sure that every relevant bootdev is used.
+	 *
+	 * If instead we found the first media device of the correct uclass,
+	 * then started with its bootdev, that bootdev may be somewhere in the
+	 * middle of the UCLASS_BOOTDEV ordering, thus we would leave out quite
+	 * a few bootdevs as the iteration continues
+	 */
+	uclass_id_foreach_dev(UCLASS_BOOTDEV, bdev, uc) {
+		struct udevice *media;
+
+		media = dev_get_parent(bdev);
+		if (device_get_uclass_id(media) != id) {
+			log_debug("- skip, media '%s' want id '%s'\n",
+				  dev_get_uclass_name(media),
+				  uclass_get_name(id));
+			continue;
+		}
 
 		/* if there is no seq, match anything */
 		if (seq != -1 && dev_seq(media) != seq) {
@@ -444,33 +461,20 @@ int bootdev_find_by_label(const char *label, struct udevice **devp,
 			continue;
 		}
 
-		ret = device_find_first_child_by_uclass(media, UCLASS_BOOTDEV,
-							&bdev);
-		if (ret) {
-			log_debug("- looking via blk, seq=%d, id=%d\n", seq,
-				  id);
-			ret = blk_find_device(id, seq, &blk);
-			if (!ret) {
-				log_debug("- get from blk %s\n", blk->name);
-				ret = bootdev_get_from_blk(blk, &bdev);
-			}
-		}
-		if (!ret) {
-			log_debug("- found %s\n", bdev->name);
-			*devp = bdev;
-
-			/*
-			 * if no sequence number was provided, we must scan all
-			 * bootdevs for this media uclass
-			 */
-			if (seq == -1)
-				method_flags |= BOOTFLOW_METHF_SINGLE_UCLASS;
-			if (method_flagsp)
-				*method_flagsp = method_flags;
-			log_debug("method flags %x\n", method_flags);
-			return 0;
-		}
-		log_debug("- no device in %s\n", media->name);
+		log_debug("- found bdev, seq=%d id %s\n", dev_seq(bdev),
+			  uclass_get_name(id));
+		*devp = bdev;
+
+		/*
+		 * if no sequence number was provided, we must scan all
+		 * bootdevs for this media uclass
+		 */
+		if (seq == -1)
+			method_flags |= BOOTFLOW_METHF_SINGLE_UCLASS;
+		if (method_flagsp)
+			*method_flagsp = method_flags;
+		log_debug("method flags %x\n", method_flags);
+		return 0;
 	}
 
 	return -ENOENT;
@@ -586,6 +590,8 @@ int bootdev_next_label(struct bootflow_iter *iter, struct udevice **devp,
 		const char *label = iter->labels[iter->cur_label];
 		int ret;
 
+		if (!label)
+			break;
 		log_debug("Scanning: %s\n", label);
 		ret = bootdev_hunt_and_find_by_label(label, &dev,
 						     method_flagsp);
-- 
2.43.0

[PATCH v2 14/18] bootstd: Tidy up a nested CONFIG_IS_ENABLED(BOOTSTD)

[Simon Glass]

Fix a nested check for BOOTSTD in the bootdev.h header file.

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

(no changes since v1)

 include/bootdev.h | 47 ++++++++++++++++++++---------------------------
 1 file changed, 20 insertions(+), 27 deletions(-)

diff --git a/include/bootdev.h b/include/bootdev.h
index 12c90c4ec1b..9ab95cebc12 100644
--- a/include/bootdev.h
+++ b/include/bootdev.h
@@ -368,7 +368,6 @@ int bootdev_next_prio(struct bootflow_iter *iter, struct udevice **devp);
  */
 int bootdev_setup_for_dev(struct udevice *parent, const char *drv_name);
 
-#if CONFIG_IS_ENABLED(BOOTSTD)
 /**
  * bootdev_setup_for_sibling_blk() - Bind a new bootdev device for a blk device
  *
@@ -383,32 +382,6 @@ int bootdev_setup_for_dev(struct udevice *parent, const char *drv_name);
  * Return: 0 if OK, -ve on error
  */
 int bootdev_setup_for_sibling_blk(struct udevice *blk, const char *drv_name);
-#else
-static int bootdev_setup_for_sibling_blk(struct udevice *blk,
-					 const char *drv_name)
-{
-	return 0;
-}
-#endif
-
-/**
- * bootdev_get_sibling_blk() - Locate the block device for a bootdev
- *
- * @dev: bootdev to check
- * @blkp: returns associated block device
- * Return: 0 if OK, -EINVAL if @dev is not a bootdev device, other -ve on other
- *	error
- */
-int bootdev_get_sibling_blk(struct udevice *dev, struct udevice **blkp);
-
-/**
- * bootdev_get_from_blk() - Get the bootdev given a block device
- *
- * @blk: Block device to check
- * @bootdebp: Returns the bootdev found, if any
- * Return 0 if OK, -ve on error
- */
-int bootdev_get_from_blk(struct udevice *blk, struct udevice **bootdevp);
 
 /**
  * bootdev_unbind_dev() - Unbind a bootdev device
@@ -421,6 +394,7 @@ int bootdev_get_from_blk(struct udevice *blk, struct udevice **bootdevp);
  * Return: 0 if OK, -ve on error
  */
 int bootdev_unbind_dev(struct udevice *parent);
+
 #else
 static inline int bootdev_setup_for_dev(struct udevice *parent,
 					const char *drv_name)
@@ -440,4 +414,23 @@ static inline int bootdev_unbind_dev(struct udevice *parent)
 }
 #endif
 
+/**
+ * bootdev_get_sibling_blk() - Locate the block device for a bootdev
+ *
+ * @dev: bootdev to check
+ * @blkp: returns associated block device
+ * Return: 0 if OK, -EINVAL if @dev is not a bootdev device, other -ve on other
+ *	error
+ */
+int bootdev_get_sibling_blk(struct udevice *dev, struct udevice **blkp);
+
+/**
+ * bootdev_get_from_blk() - Get the bootdev given a block device
+ *
+ * @blk: Block device to check
+ * @bootdebp: Returns the bootdev found, if any
+ * Return 0 if OK, -ve on error
+ */
+int bootdev_get_from_blk(struct udevice *blk, struct udevice **bootdevp);
+
 #endif
-- 
2.43.0

[PATCH v2 15/18] bootstd: Provide a command to select the bootdev order

[Simon Glass]

It is sometimes useful to select or override the default bootdev order.
Add a command for this.

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

Changes in v2:
- Drop use of LOGR()

 boot/bootdev-uclass.c     | 39 +++++++++++++++++++++++++++++++++++++++
 boot/bootstd-uclass.c     | 18 ++++++++++++++++++
 cmd/bootdev.c             | 37 +++++++++++++++++++++++++++++++++++++
 doc/usage/cmd/bootdev.rst | 36 ++++++++++++++++++++++++++++++++++++
 include/bootdev.h         | 13 +++++++++++++
 include/bootstd.h         | 10 ++++++++++
 test/boot/bootdev.c       | 37 +++++++++++++++++++++++++++++++++++++
 7 files changed, 190 insertions(+)

diff --git a/boot/bootdev-uclass.c b/boot/bootdev-uclass.c
index cfb298d5be5..80371c755b2 100644
--- a/boot/bootdev-uclass.c
+++ b/boot/bootdev-uclass.c
@@ -936,6 +936,45 @@ void bootdev_list_hunters(struct bootstd_priv *std)
 	printf("(total hunters: %d)\n", n_ent);
 }
 
+int bootdev_set_order(const char *order_str)
+{
+	struct udevice *bootstd;
+	struct alist order;
+	const char *s, *p;
+	char *label;
+	int i, ret;
+
+	ret = uclass_first_device_err(UCLASS_BOOTSTD, &bootstd);
+	if (ret)
+		return log_msg_ret("bsb", ret);
+
+	alist_init_struct(&order, char *);
+	log_debug("order_str: %s\n", order_str);
+	if (order_str) {
+		for (i = 0, s = order_str; *s; s = p + (*p == ' '), i++) {
+			p = strchrnul(s, ' ');
+			label = strndup(s, p - s);
+			if (!label || !alist_add(&order, label))
+				goto err;
+		}
+	}
+	label = NULL;
+	if (!order.count)
+		bootstd_set_bootdev_order(bootstd, NULL);
+	else if (!alist_add(&order, label))
+		goto err;
+
+	bootstd_set_bootdev_order(bootstd,
+			  alist_uninit_move(&order, NULL, const char *));
+
+	return 0;
+
+err:
+	alist_uninit(&order);
+
+	return log_msg_ret("bso", -ENOMEM);
+}
+
 static int bootdev_pre_unbind(struct udevice *dev)
 {
 	int ret;
diff --git a/boot/bootstd-uclass.c b/boot/bootstd-uclass.c
index 9bee73ead58..294865feb64 100644
--- a/boot/bootstd-uclass.c
+++ b/boot/bootstd-uclass.c
@@ -6,6 +6,8 @@
  * Written by Simon Glass <sjg@chromium.org>
  */
 
+#define LOG_CATEGORY UCLASS_BOOTSTD
+
 #include <alist.h>
 #include <blk.h>
 #include <bootdev.h>
@@ -132,6 +134,22 @@ const char *const *const bootstd_get_bootdev_order(struct udevice *dev,
 	return std->bootdev_order;
 }
 
+void bootstd_set_bootdev_order(struct udevice *dev, const char **order_str)
+{
+	struct bootstd_priv *std = dev_get_priv(dev);
+	const char **name;
+
+	free(std->bootdev_order);  /* leak; convert to use alist */
+
+	std->bootdev_order = order_str;
+	log_debug("bootdev_order:");
+	if (order_str) {
+		for (name = order_str; *name; name++)
+			log_debug(" %s", *name);
+	}
+	log_debug("\n");
+}
+
 const char *const *const bootstd_get_prefixes(struct udevice *dev)
 {
 	struct bootstd_priv *std = dev_get_priv(dev);
diff --git a/cmd/bootdev.c b/cmd/bootdev.c
index 4bc229e809a..f05a865f609 100644
--- a/cmd/bootdev.c
+++ b/cmd/bootdev.c
@@ -138,14 +138,51 @@ static int do_bootdev_hunt(struct cmd_tbl *cmdtp, int flag, int argc,
 	return 0;
 }
 
+static int do_bootdev_order(struct cmd_tbl *cmdtp, int flag, int argc,
+			    char *const argv[])
+{
+	struct bootstd_priv *priv;
+	int ret = 0;
+
+	ret = bootstd_get_priv(&priv);
+	if (ret)
+		return ret;
+	if (argc == 2) {
+		if (!strncmp(argv[1], "clear", strlen(argv[1]))) {
+			bootdev_set_order(NULL);
+
+			return 0;
+		}
+
+		ret = bootdev_set_order(argv[1]);
+		if (ret) {
+			printf("Failed (err=%dE)\n", ret);
+			return CMD_RET_FAILURE;
+		}
+	} else {
+		const char **order = priv->bootdev_order;
+
+		if (!order) {
+			printf("No ordering\n");
+		} else {
+			while (*order)
+				printf("%s\n", *order++);
+		}
+	}
+
+	return 0;
+}
+
 U_BOOT_LONGHELP(bootdev,
 	"list [-p]         - list all available bootdevs (-p to probe)\n"
 	"bootdev hunt [-l|<spec>]  - use hunt drivers to find bootdevs\n"
+	"bootdev order [clear] | [<spec>,...]  - view or update bootdev order\n"
 	"bootdev select <bd>       - select a bootdev by name | label | seq\n"
 	"bootdev info [-p]         - show information about a bootdev (-p to probe)");
 
 U_BOOT_CMD_WITH_SUBCMDS(bootdev, "Boot devices", bootdev_help_text,
 	U_BOOT_SUBCMD_MKENT(list, 2, 1, do_bootdev_list),
 	U_BOOT_SUBCMD_MKENT(hunt, 2, 1, do_bootdev_hunt),
+	U_BOOT_SUBCMD_MKENT(order, 2, 1, do_bootdev_order),
 	U_BOOT_SUBCMD_MKENT(select, 2, 1, do_bootdev_select),
 	U_BOOT_SUBCMD_MKENT(info, 2, 1, do_bootdev_info));
diff --git a/doc/usage/cmd/bootdev.rst b/doc/usage/cmd/bootdev.rst
index 98a0f43c580..abede194cba 100644
--- a/doc/usage/cmd/bootdev.rst
+++ b/doc/usage/cmd/bootdev.rst
@@ -13,6 +13,7 @@ Synopsis
 
     bootdev list [-p]        - list all available bootdevs (-p to probe)
     bootdev hunt [-l|<spec>] - use hunt drivers to find bootdevs
+    bootdev order [clear] | [<spec> ...]  - view or update bootdev order
     bootdev select <bm>      - select a bootdev by name
     bootdev info [-p]        - show information about a bootdev
 
@@ -78,6 +79,27 @@ To run hunters, specify the name of the hunter to run, e.g. "mmc". If no
 name is provided, all hunters are run.
 
 
+bootdev order
+~~~~~~~~~~~~~
+
+This allows the bootdev order to be examined or set. With no argument the
+current ordering is shown, one item per line.
+
+The argument can either be 'clear' or a space-separated list of labels. Each
+label can be the name of a bootdev (e.g. "mmc1.bootdev"), a bootdev sequence
+number ("3") or a media uclass ("mmc") with an optional sequence number (mmc2).
+
+Use `bootdev order clear` to clear any ordering and use the default.
+
+By default, the ordering is defined by the `boot_targets` environment variable
+or, failing that, the bootstd node in the devicetree ("bootdev-order" property).
+If no ordering is provided, then a default one is used.
+
+Note that this command does not check that the ordering is valid. In fact the
+meaning of the ordering depends on what the bootflow iterator discovers when it
+is used. Invalid entries will result in no bootdevs being found for that entry,
+so they are effectively skipped.
+
 bootdev select
 ~~~~~~~~~~~~~~
 
@@ -171,6 +193,20 @@ This shows using one of the available hunters, then listing them::
                 Capacity: 0.0 MB = 0.0 GB (1 x 512)
     =>
 
+This shows viewing and changing the ordering::
+
+    => bootdev order
+    mmc2
+    mmc1
+    => bootdev order 'mmc usb'
+    => bootdev order
+    mmc
+    usb
+    => bootdev order clear
+    => bootdev order
+    No ordering
+    =>
+
 
 Return value
 ------------
diff --git a/include/bootdev.h b/include/bootdev.h
index 9ab95cebc12..038bae10e93 100644
--- a/include/bootdev.h
+++ b/include/bootdev.h
@@ -433,4 +433,17 @@ int bootdev_get_sibling_blk(struct udevice *dev, struct udevice **blkp);
  */
 int bootdev_get_from_blk(struct udevice *blk, struct udevice **bootdevp);
 
+/**
+ * bootdev_set_order() - Set the bootdev order
+ *
+ * This selects the ordering to use for bootdevs
+ *
+ * @order_str: NULL-terminated string list containing the ordering. This is a
+ * comma-separate list of bootdev labels, e.g. "mmc usb". If empty then a
+ * default ordering is used
+ * Return: 0 if OK, -ENODEV if an unknown bootmeth is mentioned, -ENOMEM if
+ * out of memory, -ENOENT if there are no bootmeth devices
+ */
+int bootdev_set_order(const char *order_str);
+
 #endif
diff --git a/include/bootstd.h b/include/bootstd.h
index ac3c1922fcc..d673050164c 100644
--- a/include/bootstd.h
+++ b/include/bootstd.h
@@ -77,6 +77,16 @@ struct bootstd_priv {
 const char *const *const bootstd_get_bootdev_order(struct udevice *dev,
 						   bool *okp);
 
+/**
+ * bootstd_set_bootdev_order() - Set the boot-order list
+ *
+ * @dev: bootstd device
+ * @order_str: list of string pointers, terminated by NULL, e.g.
+ * {"mmc0", "mmc2", NULL}; or NULL to remove boot order. The array and its
+ * members must be allocated by the caller
+ */
+void bootstd_set_bootdev_order(struct udevice *dev, const char **order_str);
+
 /**
  * bootstd_get_prefixes() - Get the filename-prefixes list
  *
diff --git a/test/boot/bootdev.c b/test/boot/bootdev.c
index 0ace3d0699c..84280caf7b5 100644
--- a/test/boot/bootdev.c
+++ b/test/boot/bootdev.c
@@ -781,3 +781,40 @@ static int bootdev_test_next_prio(struct unit_test_state *uts)
 }
 BOOTSTD_TEST(bootdev_test_next_prio, UTF_DM | UTF_SCAN_FDT | UTF_SF_BOOTDEV |
 	     UTF_CONSOLE);
+
+/* Check 'bootdev order' command */
+static int bootdev_test_cmd_order(struct unit_test_state *uts)
+{
+	test_set_skip_delays(true);
+	bootstd_reset_usb();
+
+	ut_assertok(run_command("bootdev order", 0));
+	ut_assert_nextline("mmc2");
+	ut_assert_nextline("mmc1");
+	ut_assert_console_end();
+
+	ut_assertok(run_command("bootdev order clear", 0));
+	ut_assertok(run_command("bootdev order", 0));
+	ut_assert_nextline("No ordering");
+	ut_assert_console_end();
+
+	ut_assertok(run_command("bootdev order 'invalid mmc'", 0));
+	ut_assertok(run_command("bootdev order", 0));
+	ut_assert_nextline("invalid");
+	ut_assert_nextline("mmc");
+	ut_assert_console_end();
+
+	/* check handling of invalid label */
+	ut_assertok(run_command("bootflow scan -l", 0));
+	if (IS_ENABLED(CONFIG_LOGF_FUNC)) {
+		ut_assert_skip_to_line(
+			"  bootdev_next_label() Unknown uclass 'invalid' in label");
+	} else {
+		ut_assert_skip_to_line("Unknown uclass 'invalid' in label");
+	}
+	ut_assert_skip_to_line("(1 bootflow, 1 valid)");
+	ut_assert_console_end();
+
+	return 0;
+}
+BOOTSTD_TEST(bootdev_test_cmd_order, UTF_DM | UTF_SCAN_FDT | UTF_CONSOLE);
-- 
2.43.0

[PATCH v2 16/18] bootstd: Correct the comment for bootmeth_set_order()

[Simon Glass]

The function is used to handle the boot_targets variable so must support
space-separated items. Update the comment to show this.

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

(no changes since v1)

 include/bootmeth.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/bootmeth.h b/include/bootmeth.h
index c32bbbab7e9..8f4706c555c 100644
--- a/include/bootmeth.h
+++ b/include/bootmeth.h
@@ -301,8 +301,8 @@ int bootmeth_setup_iter_order(struct bootflow_iter *iter, bool include_global);
  *
  * This selects the ordering to use for bootmeths
  *
- * @order_str: String containing the ordering. This is a comma-separate list of
- * bootmeth-device names, e.g. "extlinux,efi". If empty then a default ordering
+ * @order_str: String containing the ordering. This is a space-separate list of
+ * bootmeth-device names, e.g. "extlinux efi". If empty then a default ordering
  * is used, based on the sequence number of devices (i.e. using aliases)
  * Return: 0 if OK, -ENODEV if an unknown bootmeth is mentioned, -ENOMEM if
  * out of memory, -ENOENT if there are no bootmeth devices
-- 
2.43.0

[PATCH v2 17/18] bootstd: Expand debugging in bootdev_find_in_blk()

[Simon Glass]

Add more info in this function so that the partition number and the call
to bootmeth_read_bootflow() are logged.

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

(no changes since v1)

 boot/bootdev-uclass.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/boot/bootdev-uclass.c b/boot/bootdev-uclass.c
index 80371c755b2..76397a1a990 100644
--- a/boot/bootdev-uclass.c
+++ b/boot/bootdev-uclass.c
@@ -126,7 +126,7 @@ int bootdev_find_in_blk(struct udevice *dev, struct udevice *blk,
 	 * us whether there is valid media there
 	 */
 	ret = part_get_info(desc, iter->part, &info);
-	log_debug("part_get_info() returned %d\n", ret);
+	log_debug("part_get_info() part=%d returned %d\n", iter->part, ret);
 	if (!iter->part && ret == -ENOENT)
 		ret = 0;
 
@@ -188,8 +188,9 @@ int bootdev_find_in_blk(struct udevice *dev, struct udevice *blk,
 		bflow->state = BOOTFLOWST_FS;
 	}
 
-	log_debug("method %s\n", bflow->method->name);
+	log_debug("using method %s\n", bflow->method->name);
 	ret = bootmeth_read_bootflow(bflow->method, bflow);
+	log_debug("method %s returned ret=%d\n", bflow->method->name, ret);
 	if (ret)
 		return log_msg_ret("method", ret);
 
-- 
2.43.0

[PATCH v2 18/18] bootstd: Mention FS state in bootmeth_read_bootflow()

[Simon Glass]

Add a comment to help implementers deal with the need for calling
fs_set_blk_dev_with_part()

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

---

Changes in v2:
- Rebase on top of recent FS changes

 include/bootmeth.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/bootmeth.h b/include/bootmeth.h
index 8f4706c555c..d25718d9a14 100644
--- a/include/bootmeth.h
+++ b/include/bootmeth.h
@@ -213,6 +213,9 @@ int bootmeth_check(struct udevice *dev, struct bootflow_iter *iter);
 /**
  * bootmeth_read_bootflow() - set up a bootflow for a device
  *
+ * On entry fs_set_blk_dev_with_part() has been called so it should be possible
+ * to read the file without calling that again.
+ *
  * @dev:	Bootmethod device to check
  * @bflow:	On entry, provides dev, hwpart, part and method.
  *	Returns updated bootflow if found
-- 
2.43.0

Re: [PATCH v2 15/18] bootstd: Provide a command to select the bootdev order

[Quentin Schulz]

Hi Simon,

On 5/1/25 3:37 PM, Simon Glass wrote:
[...]
> diff --git a/boot/bootstd-uclass.c b/boot/bootstd-uclass.c
> index 9bee73ead58..294865feb64 100644
> --- a/boot/bootstd-uclass.c
> +++ b/boot/bootstd-uclass.c
> @@ -6,6 +6,8 @@
>    * Written by Simon Glass <sjg@chromium.org>
>    */
>   
> +#define LOG_CATEGORY UCLASS_BOOTSTD
> +
>   #include <alist.h>
>   #include <blk.h>
>   #include <bootdev.h>
> @@ -132,6 +134,22 @@ const char *const *const bootstd_get_bootdev_order(struct udevice *dev,
>   	return std->bootdev_order;
>   }
>   
> +void bootstd_set_bootdev_order(struct udevice *dev, const char **order_str)
> +{
> +	struct bootstd_priv *std = dev_get_priv(dev);
> +	const char **name;
> +
> +	free(std->bootdev_order);  /* leak; convert to use alist */
> +

leak? and aren't you using alist already?

[...]

> diff --git a/doc/usage/cmd/bootdev.rst b/doc/usage/cmd/bootdev.rst
> index 98a0f43c580..abede194cba 100644
> --- a/doc/usage/cmd/bootdev.rst
> +++ b/doc/usage/cmd/bootdev.rst
> @@ -13,6 +13,7 @@ Synopsis
>   
>       bootdev list [-p]        - list all available bootdevs (-p to probe)
>       bootdev hunt [-l|<spec>] - use hunt drivers to find bootdevs
> +    bootdev order [clear] | [<spec> ...]  - view or update bootdev order
>       bootdev select <bm>      - select a bootdev by name
>       bootdev info [-p]        - show information about a bootdev
>   
> @@ -78,6 +79,27 @@ To run hunters, specify the name of the hunter to run, e.g. "mmc". If no
>   name is provided, all hunters are run.
>   
>   
> +bootdev order
> +~~~~~~~~~~~~~
> +
> +This allows the bootdev order to be examined or set. With no argument the
> +current ordering is shown, one item per line.
> +
> +The argument can either be 'clear' or a space-separated list of labels. Each
> +label can be the name of a bootdev (e.g. "mmc1.bootdev"), a bootdev sequence
> +number ("3") or a media uclass ("mmc") with an optional sequence number (mmc2).
> +
> +Use `bootdev order clear` to clear any ordering and use the default.
> +
> +By default, the ordering is defined by the `boot_targets` environment variable
> +or, failing that, the bootstd node in the devicetree ("bootdev-order" property).
> +If no ordering is provided, then a default one is used.
> +

Not sure what's the benefit if we can simply set the environment variable?

FWIW, it's what we do in 
board/theobroma-systems/common/common.c:setup_boottargets()

One of the benefits I see is dumping the order if that variable isn't 
set (and also not in the DT), but shouldn't that be information we 
should be getting from `bootdev list` instead?

The point I'm trying to make is do we need yet another way to set the 
bootdev order? Printing the order would be welcomed I guess but that 
could be as simple as printing boot_targets environment variable, the DT 
property or whatever the default is if both are missing.

If we are to keep the clear command, I would use a flag instead of an 
argument to be consistent with the other bootdev commands, e.g. 
something like `bootdev order -c`.

> +Note that this command does not check that the ordering is valid. In fact the
> +meaning of the ordering depends on what the bootflow iterator discovers when it
> +is used. Invalid entries will result in no bootdevs being found for that entry,
> +so they are effectively skipped.
> +
>   bootdev select
>   ~~~~~~~~~~~~~~
>   
> @@ -171,6 +193,20 @@ This shows using one of the available hunters, then listing them::
>                   Capacity: 0.0 MB = 0.0 GB (1 x 512)
>       =>
>   
> +This shows viewing and changing the ordering::
> +
> +    => bootdev order
> +    mmc2
> +    mmc1
> +    => bootdev order 'mmc usb'
> +    => bootdev order
> +    mmc
> +    usb
> +    => bootdev order clear
> +    => bootdev order
> +    No ordering

Shouldn't we be provided with a default one?

Cheers,
Quentin

Re: [PATCH v2 15/18] bootstd: Provide a command to select the bootdev order

[Simon Glass]

Hi Quentin,

On Mon, 5 May 2025 at 10:34, Quentin Schulz <quentin.schulz@cherry.de> wrote:
>
> Hi Simon,
>
> On 5/1/25 3:37 PM, Simon Glass wrote:
> [...]
> > diff --git a/boot/bootstd-uclass.c b/boot/bootstd-uclass.c
> > index 9bee73ead58..294865feb64 100644
> > --- a/boot/bootstd-uclass.c
> > +++ b/boot/bootstd-uclass.c
> > @@ -6,6 +6,8 @@
> >    * Written by Simon Glass <sjg@chromium.org>
> >    */
> >
> > +#define LOG_CATEGORY UCLASS_BOOTSTD
> > +
> >   #include <alist.h>
> >   #include <blk.h>
> >   #include <bootdev.h>
> > @@ -132,6 +134,22 @@ const char *const *const bootstd_get_bootdev_order(struct udevice *dev,
> >       return std->bootdev_order;
> >   }
> >
> > +void bootstd_set_bootdev_order(struct udevice *dev, const char **order_str)
> > +{
> > +     struct bootstd_priv *std = dev_get_priv(dev);
> > +     const char **name;
> > +
> > +     free(std->bootdev_order);  /* leak; convert to use alist */
> > +
>
> leak? and aren't you using alist already?

Not for this, yet.

>
> [...]
>
> > diff --git a/doc/usage/cmd/bootdev.rst b/doc/usage/cmd/bootdev.rst
> > index 98a0f43c580..abede194cba 100644
> > --- a/doc/usage/cmd/bootdev.rst
> > +++ b/doc/usage/cmd/bootdev.rst
> > @@ -13,6 +13,7 @@ Synopsis
> >
> >       bootdev list [-p]        - list all available bootdevs (-p to probe)
> >       bootdev hunt [-l|<spec>] - use hunt drivers to find bootdevs
> > +    bootdev order [clear] | [<spec> ...]  - view or update bootdev order
> >       bootdev select <bm>      - select a bootdev by name
> >       bootdev info [-p]        - show information about a bootdev
> >
> > @@ -78,6 +79,27 @@ To run hunters, specify the name of the hunter to run, e.g. "mmc". If no
> >   name is provided, all hunters are run.
> >
> >
> > +bootdev order
> > +~~~~~~~~~~~~~
> > +
> > +This allows the bootdev order to be examined or set. With no argument the
> > +current ordering is shown, one item per line.
> > +
> > +The argument can either be 'clear' or a space-separated list of labels. Each
> > +label can be the name of a bootdev (e.g. "mmc1.bootdev"), a bootdev sequence
> > +number ("3") or a media uclass ("mmc") with an optional sequence number (mmc2).
> > +
> > +Use `bootdev order clear` to clear any ordering and use the default.
> > +
> > +By default, the ordering is defined by the `boot_targets` environment variable
> > +or, failing that, the bootstd node in the devicetree ("bootdev-order" property).
> > +If no ordering is provided, then a default one is used.
> > +
>
> Not sure what's the benefit if we can simply set the environment variable?

The environment variable is there to maintain backwards compatibility
with the distro scripts. It now seems that we are unlikely to ever
drop those, but you never know.

>
> FWIW, it's what we do in
> board/theobroma-systems/common/common.c:setup_boottargets()
>
> One of the benefits I see is dumping the order if that variable isn't
> set (and also not in the DT), but shouldn't that be information we
> should be getting from `bootdev list` instead?

In general, the ordering is not known until a scan is completed, since
scanning can invoke hunters to find other bootdevs not visible prior.

>
> The point I'm trying to make is do we need yet another way to set the
> bootdev order? Printing the order would be welcomed I guess but that
> could be as simple as printing boot_targets environment variable, the DT
> property or whatever the default is if both are missing.

As above, the boot_targets var is for backwards compatibility. The DT
property is fixed and cannot be changed at runtime. So I believe we do
need this command.

>
> If we are to keep the clear command, I would use a flag instead of an
> argument to be consistent with the other bootdev commands, e.g.
> something like `bootdev order -c`.

Yes that sounds better, thanks.

>
> > +Note that this command does not check that the ordering is valid. In fact the
> > +meaning of the ordering depends on what the bootflow iterator discovers when it
> > +is used. Invalid entries will result in no bootdevs being found for that entry,
> > +so they are effectively skipped.
> > +
> >   bootdev select
> >   ~~~~~~~~~~~~~~
> >
> > @@ -171,6 +193,20 @@ This shows using one of the available hunters, then listing them::
> >                   Capacity: 0.0 MB = 0.0 GB (1 x 512)
> >       =>
> >
> > +This shows viewing and changing the ordering::
> > +
> > +    => bootdev order
> > +    mmc2
> > +    mmc1
> > +    => bootdev order 'mmc usb'
> > +    => bootdev order
> > +    mmc
> > +    usb
> > +    => bootdev order clear
> > +    => bootdev order
> > +    No ordering
>
> Shouldn't we be provided with a default one?

See above. I suppose we could implement scanning for bootdevs, but
that would need to be a separate change.

Regards,
Simon

Re: [PATCH v2 15/18] bootstd: Provide a command to select the bootdev order

[Tom Rini]

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

On Mon, May 05, 2025 at 05:37:33PM +0200, Simon Glass wrote:
> Hi Quentin,
> 
> On Mon, 5 May 2025 at 10:34, Quentin Schulz <quentin.schulz@cherry.de> wrote:
> >
> > Hi Simon,
> >
> > On 5/1/25 3:37 PM, Simon Glass wrote:
> > [...]
> > > diff --git a/boot/bootstd-uclass.c b/boot/bootstd-uclass.c
> > > index 9bee73ead58..294865feb64 100644
> > > --- a/boot/bootstd-uclass.c
> > > +++ b/boot/bootstd-uclass.c
> > > @@ -6,6 +6,8 @@
> > >    * Written by Simon Glass <sjg@chromium.org>
> > >    */
> > >
> > > +#define LOG_CATEGORY UCLASS_BOOTSTD
> > > +
> > >   #include <alist.h>
> > >   #include <blk.h>
> > >   #include <bootdev.h>
> > > @@ -132,6 +134,22 @@ const char *const *const bootstd_get_bootdev_order(struct udevice *dev,
> > >       return std->bootdev_order;
> > >   }
> > >
> > > +void bootstd_set_bootdev_order(struct udevice *dev, const char **order_str)
> > > +{
> > > +     struct bootstd_priv *std = dev_get_priv(dev);
> > > +     const char **name;
> > > +
> > > +     free(std->bootdev_order);  /* leak; convert to use alist */
> > > +
> >
> > leak? and aren't you using alist already?
> 
> Not for this, yet.
> 
> >
> > [...]
> >
> > > diff --git a/doc/usage/cmd/bootdev.rst b/doc/usage/cmd/bootdev.rst
> > > index 98a0f43c580..abede194cba 100644
> > > --- a/doc/usage/cmd/bootdev.rst
> > > +++ b/doc/usage/cmd/bootdev.rst
> > > @@ -13,6 +13,7 @@ Synopsis
> > >
> > >       bootdev list [-p]        - list all available bootdevs (-p to probe)
> > >       bootdev hunt [-l|<spec>] - use hunt drivers to find bootdevs
> > > +    bootdev order [clear] | [<spec> ...]  - view or update bootdev order
> > >       bootdev select <bm>      - select a bootdev by name
> > >       bootdev info [-p]        - show information about a bootdev
> > >
> > > @@ -78,6 +79,27 @@ To run hunters, specify the name of the hunter to run, e.g. "mmc". If no
> > >   name is provided, all hunters are run.
> > >
> > >
> > > +bootdev order
> > > +~~~~~~~~~~~~~
> > > +
> > > +This allows the bootdev order to be examined or set. With no argument the
> > > +current ordering is shown, one item per line.
> > > +
> > > +The argument can either be 'clear' or a space-separated list of labels. Each
> > > +label can be the name of a bootdev (e.g. "mmc1.bootdev"), a bootdev sequence
> > > +number ("3") or a media uclass ("mmc") with an optional sequence number (mmc2).
> > > +
> > > +Use `bootdev order clear` to clear any ordering and use the default.
> > > +
> > > +By default, the ordering is defined by the `boot_targets` environment variable
> > > +or, failing that, the bootstd node in the devicetree ("bootdev-order" property).
> > > +If no ordering is provided, then a default one is used.
> > > +
> >
> > Not sure what's the benefit if we can simply set the environment variable?
> 
> The environment variable is there to maintain backwards compatibility
> with the distro scripts. It now seems that we are unlikely to ever
> drop those, but you never know.

Repeating myself from some other discussion before, it's not just "distro
scripts", managing things with the environment is an intentional
feature, not a backwards compatible legacy thing. Solutions to "boot
this device" that do not have some support for using the environment to
modify things as needed are unlikely to be flexible enough for all use
cases.

-- 
Tom

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

Re: [PATCH v2 01/18] abuf: Add a helper for initing and allocating a buffer

[Tom Rini]

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

On Thu, May 01, 2025 at 07:37:01AM -0600, Simon Glass wrote:

> This construct appears in various places. Reduce code size by adding a
> function for it.
> 
> It inits the abuf, then allocates it to the requested size.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
> Changes in v2:
> - Add new patch with a helper for initing and allocating a buffer
> 
>  boot/cedit.c          |  3 +--
>  boot/scene.c          |  3 +--
>  boot/scene_textline.c |  3 +--
>  include/abuf.h        | 11 +++++++++++
>  lib/abuf.c            |  9 +++++++++
>  lib/of_live.c         |  3 +--
>  test/lib/abuf.c       | 22 ++++++++++++++++++++++
>  7 files changed, 46 insertions(+), 8 deletions(-)

This just made me look again at the abuf implementation itself and
become filled with regret I didn't reject it back in 2021. We're
introducing wrappers around standard functions and calling conventions /
patterns with something homegrown (and so not intuitive to others) that
mainly hides the "sysmem" challenge we also have and I wish you were
interested in revisiting how that part of sandbox works instead. And
even if this is a better design, for the sake of argument, it's not
something everyone else is used to. And that's important.

If we *really* need something different / new here, I'd rather see us go
and wrap common/dlmalloc.c with kmalloc/kfree/etc and so give people
something even more familiar-looking.

-- 
Tom

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

Re: [PATCH v2 01/18] abuf: Add a helper for initing and allocating a buffer

[Simon Glass]

Hi Tom,

On Mon, 5 May 2025 at 22:38, Tom Rini <trini@konsulko.com> wrote:
>
> On Thu, May 01, 2025 at 07:37:01AM -0600, Simon Glass wrote:
>
> > This construct appears in various places. Reduce code size by adding a
> > function for it.
> >
> > It inits the abuf, then allocates it to the requested size.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> > Changes in v2:
> > - Add new patch with a helper for initing and allocating a buffer
> >
> >  boot/cedit.c          |  3 +--
> >  boot/scene.c          |  3 +--
> >  boot/scene_textline.c |  3 +--
> >  include/abuf.h        | 11 +++++++++++
> >  lib/abuf.c            |  9 +++++++++
> >  lib/of_live.c         |  3 +--
> >  test/lib/abuf.c       | 22 ++++++++++++++++++++++
> >  7 files changed, 46 insertions(+), 8 deletions(-)
>
> This just made me look again at the abuf implementation itself and
> become filled with regret I didn't reject it back in 2021. We're
> introducing wrappers around standard functions and calling conventions /
> patterns with something homegrown (and so not intuitive to others) that
> mainly hides the "sysmem" challenge we also have  and I wish you were
> interested in revisiting how that part of sandbox works instead. And
> even if this is a better design, for the sake of argument, it's not
> something everyone else is used to. And that's important.
>
> If we *really* need something different / new here, I'd rather see us go
> and wrap common/dlmalloc.c with kmalloc/kfree/etc and so give people
> something even more familiar-looking.

The purpose of abuf is actually not about hiding sysmem - you can see
that if you look at lib/abuf.c and the tests. It provides an easy way
to manage buffers, which we do a lot in U-Boot, particularly with
standard boot. It deals with whether the buffer is allocated or not,
so freeing is easy. With expo I want to be able to manage a buffer
containing an autoboot string in high-level code, say, without
worrying whether it has to be realloced.

For sysmem, I don't see a viable alternative, which keeps can reliably
keep addresses consistent across multiple phases (e.g. sandbox_vpl).
Also, I don't believe addresses like 0x7ffff7fbc000 are
human-friendly, yet that is what we would see in tests if we dropped
the mapping. The point of sandbox is to test U-Boot, not expose the
system internals. But we could discuss it if you like?

In general I am sensing that there are a lot of things which bug you
about the direction of U-Boot. and that you feel very differently
about some things than you did in 2021. I have quite a few concerns
too. So how about we make time to discuss these and see if we can get
on the same page, at least somewhat?

Coming back to this patch though, this is really about a code-side
win, rather than anything else.

Regards,
Simon

Re: [PATCH v2 15/18] bootstd: Provide a command to select the bootdev order

[Simon Glass]

Hi Tom,

On Mon, 5 May 2025 at 18:41, Tom Rini <trini@konsulko.com> wrote:
>
> On Mon, May 05, 2025 at 05:37:33PM +0200, Simon Glass wrote:
> > Hi Quentin,
> >
> > On Mon, 5 May 2025 at 10:34, Quentin Schulz <quentin.schulz@cherry.de> wrote:
> > >
> > > Hi Simon,
> > >
> > > On 5/1/25 3:37 PM, Simon Glass wrote:
> > > [...]
> > > > diff --git a/boot/bootstd-uclass.c b/boot/bootstd-uclass.c
> > > > index 9bee73ead58..294865feb64 100644
> > > > --- a/boot/bootstd-uclass.c
> > > > +++ b/boot/bootstd-uclass.c
> > > > @@ -6,6 +6,8 @@
> > > >    * Written by Simon Glass <sjg@chromium.org>
> > > >    */
> > > >
> > > > +#define LOG_CATEGORY UCLASS_BOOTSTD
> > > > +
> > > >   #include <alist.h>
> > > >   #include <blk.h>
> > > >   #include <bootdev.h>
> > > > @@ -132,6 +134,22 @@ const char *const *const bootstd_get_bootdev_order(struct udevice *dev,
> > > >       return std->bootdev_order;
> > > >   }
> > > >
> > > > +void bootstd_set_bootdev_order(struct udevice *dev, const char **order_str)
> > > > +{
> > > > +     struct bootstd_priv *std = dev_get_priv(dev);
> > > > +     const char **name;
> > > > +
> > > > +     free(std->bootdev_order);  /* leak; convert to use alist */
> > > > +
> > >
> > > leak? and aren't you using alist already?
> >
> > Not for this, yet.
> >
> > >
> > > [...]
> > >
> > > > diff --git a/doc/usage/cmd/bootdev.rst b/doc/usage/cmd/bootdev.rst
> > > > index 98a0f43c580..abede194cba 100644
> > > > --- a/doc/usage/cmd/bootdev.rst
> > > > +++ b/doc/usage/cmd/bootdev.rst
> > > > @@ -13,6 +13,7 @@ Synopsis
> > > >
> > > >       bootdev list [-p]        - list all available bootdevs (-p to probe)
> > > >       bootdev hunt [-l|<spec>] - use hunt drivers to find bootdevs
> > > > +    bootdev order [clear] | [<spec> ...]  - view or update bootdev order
> > > >       bootdev select <bm>      - select a bootdev by name
> > > >       bootdev info [-p]        - show information about a bootdev
> > > >
> > > > @@ -78,6 +79,27 @@ To run hunters, specify the name of the hunter to run, e.g. "mmc". If no
> > > >   name is provided, all hunters are run.
> > > >
> > > >
> > > > +bootdev order
> > > > +~~~~~~~~~~~~~
> > > > +
> > > > +This allows the bootdev order to be examined or set. With no argument the
> > > > +current ordering is shown, one item per line.
> > > > +
> > > > +The argument can either be 'clear' or a space-separated list of labels. Each
> > > > +label can be the name of a bootdev (e.g. "mmc1.bootdev"), a bootdev sequence
> > > > +number ("3") or a media uclass ("mmc") with an optional sequence number (mmc2).
> > > > +
> > > > +Use `bootdev order clear` to clear any ordering and use the default.
> > > > +
> > > > +By default, the ordering is defined by the `boot_targets` environment variable
> > > > +or, failing that, the bootstd node in the devicetree ("bootdev-order" property).
> > > > +If no ordering is provided, then a default one is used.
> > > > +
> > >
> > > Not sure what's the benefit if we can simply set the environment variable?
> >
> > The environment variable is there to maintain backwards compatibility
> > with the distro scripts. It now seems that we are unlikely to ever
> > drop those, but you never know.
>
> Repeating myself from some other discussion before, it's not just "distro
> scripts", managing things with the environment is an intentional
> feature, not a backwards compatible legacy thing. Solutions to "boot
> this device" that do not have some support for using the environment to
> modify things as needed are unlikely to be flexible enough for all use
> cases.

For bootstd I would rather define something designed for that purpose,
than use a boot_targets environment variable. If we are relying on an
environment variable to have a particular value, then erasing the
environment might stop the board from booting. The idea with bootstd
is that it should always be able to boot.

But in any case, we have this variable today. Removing it would
require some discussion and thought and is not likely in the
foreseeable future.

Regards,
Simon

Re: [PATCH v2 01/18] abuf: Add a helper for initing and allocating a buffer

[Tom Rini]

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

On Tue, May 06, 2025 at 03:23:39PM +0200, Simon Glass wrote:
> Hi Tom,
> 
> On Mon, 5 May 2025 at 22:38, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Thu, May 01, 2025 at 07:37:01AM -0600, Simon Glass wrote:
> >
> > > This construct appears in various places. Reduce code size by adding a
> > > function for it.
> > >
> > > It inits the abuf, then allocates it to the requested size.
> > >
> > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > ---
> > >
> > > Changes in v2:
> > > - Add new patch with a helper for initing and allocating a buffer
> > >
> > >  boot/cedit.c          |  3 +--
> > >  boot/scene.c          |  3 +--
> > >  boot/scene_textline.c |  3 +--
> > >  include/abuf.h        | 11 +++++++++++
> > >  lib/abuf.c            |  9 +++++++++
> > >  lib/of_live.c         |  3 +--
> > >  test/lib/abuf.c       | 22 ++++++++++++++++++++++
> > >  7 files changed, 46 insertions(+), 8 deletions(-)
> >
> > This just made me look again at the abuf implementation itself and
> > become filled with regret I didn't reject it back in 2021. We're
> > introducing wrappers around standard functions and calling conventions /
> > patterns with something homegrown (and so not intuitive to others) that
> > mainly hides the "sysmem" challenge we also have  and I wish you were
> > interested in revisiting how that part of sandbox works instead. And
> > even if this is a better design, for the sake of argument, it's not
> > something everyone else is used to. And that's important.
> >
> > If we *really* need something different / new here, I'd rather see us go
> > and wrap common/dlmalloc.c with kmalloc/kfree/etc and so give people
> > something even more familiar-looking.
> 
> The purpose of abuf is actually not about hiding sysmem - you can see
> that if you look at lib/abuf.c and the tests. It provides an easy way
> to manage buffers, which we do a lot in U-Boot, particularly with
> standard boot. It deals with whether the buffer is allocated or not,
> so freeing is easy. With expo I want to be able to manage a buffer
> containing an autoboot string in high-level code, say, without
> worrying whether it has to be realloced.

It's a wrapper around free/malloc/et al, which then also includes the
sandbox-specific requirement of *sysmap* stuff. This is best shown by
the next two patches which convert from standard malloc/free patterns
(which both humans and analysis checkers are fond of) to a home grown
invention.

> For sysmem, I don't see a viable alternative, which keeps can reliably
> keep addresses consistent across multiple phases (e.g. sandbox_vpl).
> Also, I don't believe addresses like 0x7ffff7fbc000 are
> human-friendly, yet that is what we would see in tests if we dropped
> the mapping. The point of sandbox is to test U-Boot, not expose the
> system internals. But we could discuss it if you like?

I'm not sure how this is relevant to the general high level point of
"sandbox should be able to, in 2025, be designed so that special macros
do not need to be everywhere for the sanity checking it can do to
happen". If my web browser is going to suck up 11GiB of memory all the
time I can live with sandbox using 1/2/4/whatever GiB of memory when it
runs. The "this is not a pointer" problem sandbox can solve could be
done differently these days.

> In general I am sensing that there are a lot of things which bug you
> about the direction of U-Boot. and that you feel very differently
> about some things than you did in 2021. I have quite a few concerns
> too. So how about we make time to discuss these and see if we can get
> on the same page, at least somewhat?

Yes, we've had numerous long threads, and a few calls. You are then
unhappy with what I say I want to see done, want me to just take your
changes instead and "Say Yes" more. I wonder if I had said No, or gather
more feedback from potential users, or just slow down, more back in 2021
if we would not be in a different position here.

> Coming back to this patch though, this is really about a code-side
> win, rather than anything else.

No, it's about how similar to the "log a message and return" convention
change you wanted to make and then dropped, how I do not see good value
in changing from standard convention of malloc/free to abuf being a win.
Maybe this is a case where borrowing the kernel's mempool logic would be
a win instead? Because another part of this is that we want to avoid
making up our own APIs for things if we can instead borrow something
from the kernel, where most of the rest of the community will already be
familiar.

-- 
Tom

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

Re: [PATCH v2 01/18] abuf: Add a helper for initing and allocating a buffer

[Simon Glass]

Hi Tom,

On Tue, 6 May 2025 at 21:48, Tom Rini <trini@konsulko.com> wrote:
>
> On Tue, May 06, 2025 at 03:23:39PM +0200, Simon Glass wrote:
> > Hi Tom,
> >
> > On Mon, 5 May 2025 at 22:38, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Thu, May 01, 2025 at 07:37:01AM -0600, Simon Glass wrote:
> > >
> > > > This construct appears in various places. Reduce code size by adding a
> > > > function for it.
> > > >
> > > > It inits the abuf, then allocates it to the requested size.
> > > >
> > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > ---
> > > >
> > > > Changes in v2:
> > > > - Add new patch with a helper for initing and allocating a buffer
> > > >
> > > >  boot/cedit.c          |  3 +--
> > > >  boot/scene.c          |  3 +--
> > > >  boot/scene_textline.c |  3 +--
> > > >  include/abuf.h        | 11 +++++++++++
> > > >  lib/abuf.c            |  9 +++++++++
> > > >  lib/of_live.c         |  3 +--
> > > >  test/lib/abuf.c       | 22 ++++++++++++++++++++++
> > > >  7 files changed, 46 insertions(+), 8 deletions(-)
> > >
> > > This just made me look again at the abuf implementation itself and
> > > become filled with regret I didn't reject it back in 2021. We're
> > > introducing wrappers around standard functions and calling conventions /
> > > patterns with something homegrown (and so not intuitive to others) that
> > > mainly hides the "sysmem" challenge we also have  and I wish you were
> > > interested in revisiting how that part of sandbox works instead. And
> > > even if this is a better design, for the sake of argument, it's not
> > > something everyone else is used to. And that's important.
> > >
> > > If we *really* need something different / new here, I'd rather see us go
> > > and wrap common/dlmalloc.c with kmalloc/kfree/etc and so give people
> > > something even more familiar-looking.
> >
> > The purpose of abuf is actually not about hiding sysmem - you can see
> > that if you look at lib/abuf.c and the tests. It provides an easy way
> > to manage buffers, which we do a lot in U-Boot, particularly with
> > standard boot. It deals with whether the buffer is allocated or not,
> > so freeing is easy. With expo I want to be able to manage a buffer
> > containing an autoboot string in high-level code, say, without
> > worrying whether it has to be realloced.
>
> It's a wrapper around free/malloc/et al, which then also includes the
> sandbox-specific requirement of *sysmap* stuff. This is best shown by
> the next two patches which convert from standard malloc/free patterns
> (which both humans and analysis checkers are fond of) to a home grown
> invention.

Yes, although I should point out that most of my code is a home-grown
invention, if by that you mean it wasn't invented by someone else and
then posted by me as if it were my own invention.

>
> > For sysmem, I don't see a viable alternative, which keeps can reliably
> > keep addresses consistent across multiple phases (e.g. sandbox_vpl).
> > Also, I don't believe addresses like 0x7ffff7fbc000 are
> > human-friendly, yet that is what we would see in tests if we dropped
> > the mapping. The point of sandbox is to test U-Boot, not expose the
> > system internals. But we could discuss it if you like?
>
> I'm not sure how this is relevant to the general high level point of
> "sandbox should be able to, in 2025, be designed so that special macros
> do not need to be everywhere for the sanity checking it can do to
> happen". If my web browser is going to suck up 11GiB of memory all the
> time I can live with sandbox using 1/2/4/whatever GiB of memory when it
> runs. The "this is not a pointer" problem sandbox can solve could be
> done differently these days.

The special macros have several purposes:
1. Allow sandbox to have a contiguous region of memory at address 0,
while the pointers are wherever the RAM buffer happens to end up
2. Provide an indication of whether code has been converted to sandbox
yet, or not
3. Provide a way to mark all casts to/from address/pointers so it is
clear that the cast is intentional
4. Ensure that sandbox seg-faults if a bad pointer is used

You seemed quite happy about it until my EFI test was rejected.

>
> > In general I am sensing that there are a lot of things which bug you
> > about the direction of U-Boot. and that you feel very differently
> > about some things than you did in 2021. I have quite a few concerns
> > too. So how about we make time to discuss these and see if we can get
> > on the same page, at least somewhat?
>
> Yes, we've had numerous long threads, and a few calls. You are then
> unhappy with what I say I want to see done, want me to just take your
> changes instead and "Say Yes" more. I wonder if I had said No, or gather
> more feedback from potential users, or just slow down, more back in 2021
> if we would not be in a different position here.

I wonder if you had said yes more, if we might have got standard boot
done a few years earlier and would not be stuck now on EFI bootmgr.

>
> > Coming back to this patch though, this is really about a code-side
> > win, rather than anything else.
>
> No, it's about how similar to the "log a message and return" convention
> change you wanted to make and then dropped, how I do not see good value
> in changing from standard convention of malloc/free to abuf being a win.
> Maybe this is a case where borrowing the kernel's mempool logic would be
> a win instead? Because another part of this is that we want to avoid
> making up our own APIs for things if we can instead borrow something
> from the kernel, where most of the rest of the community will already be
> familiar.

mempool is actually lmb, as I understand it. We haven't done that
rename. It allocates from a region of pre-allocated memory. It doesn't
use malloc(). So to me, that is quite different.

- Simon

Re: [PATCH v2 01/18] abuf: Add a helper for initing and allocating a buffer

[Tom Rini]

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

On Sat, May 10, 2025 at 01:27:03PM +0200, Simon Glass wrote:
> Hi Tom,
> 
> On Tue, 6 May 2025 at 21:48, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Tue, May 06, 2025 at 03:23:39PM +0200, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Mon, 5 May 2025 at 22:38, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Thu, May 01, 2025 at 07:37:01AM -0600, Simon Glass wrote:
> > > >
> > > > > This construct appears in various places. Reduce code size by adding a
> > > > > function for it.
> > > > >
> > > > > It inits the abuf, then allocates it to the requested size.
> > > > >
> > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > ---
> > > > >
> > > > > Changes in v2:
> > > > > - Add new patch with a helper for initing and allocating a buffer
> > > > >
> > > > >  boot/cedit.c          |  3 +--
> > > > >  boot/scene.c          |  3 +--
> > > > >  boot/scene_textline.c |  3 +--
> > > > >  include/abuf.h        | 11 +++++++++++
> > > > >  lib/abuf.c            |  9 +++++++++
> > > > >  lib/of_live.c         |  3 +--
> > > > >  test/lib/abuf.c       | 22 ++++++++++++++++++++++
> > > > >  7 files changed, 46 insertions(+), 8 deletions(-)
> > > >
> > > > This just made me look again at the abuf implementation itself and
> > > > become filled with regret I didn't reject it back in 2021. We're
> > > > introducing wrappers around standard functions and calling conventions /
> > > > patterns with something homegrown (and so not intuitive to others) that
> > > > mainly hides the "sysmem" challenge we also have  and I wish you were
> > > > interested in revisiting how that part of sandbox works instead. And
> > > > even if this is a better design, for the sake of argument, it's not
> > > > something everyone else is used to. And that's important.
> > > >
> > > > If we *really* need something different / new here, I'd rather see us go
> > > > and wrap common/dlmalloc.c with kmalloc/kfree/etc and so give people
> > > > something even more familiar-looking.
> > >
> > > The purpose of abuf is actually not about hiding sysmem - you can see
> > > that if you look at lib/abuf.c and the tests. It provides an easy way
> > > to manage buffers, which we do a lot in U-Boot, particularly with
> > > standard boot. It deals with whether the buffer is allocated or not,
> > > so freeing is easy. With expo I want to be able to manage a buffer
> > > containing an autoboot string in high-level code, say, without
> > > worrying whether it has to be realloced.
> >
> > It's a wrapper around free/malloc/et al, which then also includes the
> > sandbox-specific requirement of *sysmap* stuff. This is best shown by
> > the next two patches which convert from standard malloc/free patterns
> > (which both humans and analysis checkers are fond of) to a home grown
> > invention.
> 
> Yes, although I should point out that most of my code is a home-grown
> invention, if by that you mean it wasn't invented by someone else and
> then posted by me as if it were my own invention.

Well, I'm referring to the cases where solutions to the problems exist,
but you're inventing a new one instead.

> > > For sysmem, I don't see a viable alternative, which keeps can reliably
> > > keep addresses consistent across multiple phases (e.g. sandbox_vpl).
> > > Also, I don't believe addresses like 0x7ffff7fbc000 are
> > > human-friendly, yet that is what we would see in tests if we dropped
> > > the mapping. The point of sandbox is to test U-Boot, not expose the
> > > system internals. But we could discuss it if you like?
> >
> > I'm not sure how this is relevant to the general high level point of
> > "sandbox should be able to, in 2025, be designed so that special macros
> > do not need to be everywhere for the sanity checking it can do to
> > happen". If my web browser is going to suck up 11GiB of memory all the
> > time I can live with sandbox using 1/2/4/whatever GiB of memory when it
> > runs. The "this is not a pointer" problem sandbox can solve could be
> > done differently these days.
> 
> The special macros have several purposes:
> 1. Allow sandbox to have a contiguous region of memory at address 0,
> while the pointers are wherever the RAM buffer happens to end up
> 2. Provide an indication of whether code has been converted to sandbox
> yet, or not
> 3. Provide a way to mark all casts to/from address/pointers so it is
> clear that the cast is intentional
> 4. Ensure that sandbox seg-faults if a bad pointer is used
> 
> You seemed quite happy about it until my EFI test was rejected.

No, I've never really been happy with the macros and never understood
why we couldn't do the useful sanity checking wholly under arch/sandbox/

> > > In general I am sensing that there are a lot of things which bug you
> > > about the direction of U-Boot. and that you feel very differently
> > > about some things than you did in 2021. I have quite a few concerns
> > > too. So how about we make time to discuss these and see if we can get
> > > on the same page, at least somewhat?
> >
> > Yes, we've had numerous long threads, and a few calls. You are then
> > unhappy with what I say I want to see done, want me to just take your
> > changes instead and "Say Yes" more. I wonder if I had said No, or gather
> > more feedback from potential users, or just slow down, more back in 2021
> > if we would not be in a different position here.
> 
> I wonder if you had said yes more, if we might have got standard boot
> done a few years earlier and would not be stuck now on EFI bootmgr.

No, we probably would have just had more developers leave as I overrode
their objections.

> > > Coming back to this patch though, this is really about a code-side
> > > win, rather than anything else.
> >
> > No, it's about how similar to the "log a message and return" convention
> > change you wanted to make and then dropped, how I do not see good value
> > in changing from standard convention of malloc/free to abuf being a win.
> > Maybe this is a case where borrowing the kernel's mempool logic would be
> > a win instead? Because another part of this is that we want to avoid
> > making up our own APIs for things if we can instead borrow something
> > from the kernel, where most of the rest of the community will already be
> > familiar.
> 
> mempool is actually lmb, as I understand it. We haven't done that
> rename. It allocates from a region of pre-allocated memory. It doesn't
> use malloc(). So to me, that is quite different.

Then we should probably just drop the abuf thing and use standard
conventions that developers are used to.

-- 
Tom

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

Re: [PATCH v2 01/18] abuf: Add a helper for initing and allocating a buffer

[Simon Glass]

Hi Tom,

On Tue, 13 May 2025 at 00:35, Tom Rini <trini@konsulko.com> wrote:
>
> On Sat, May 10, 2025 at 01:27:03PM +0200, Simon Glass wrote:
> > Hi Tom,
> >
> > On Tue, 6 May 2025 at 21:48, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Tue, May 06, 2025 at 03:23:39PM +0200, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Mon, 5 May 2025 at 22:38, Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > On Thu, May 01, 2025 at 07:37:01AM -0600, Simon Glass wrote:
> > > > >
> > > > > > This construct appears in various places. Reduce code size by adding a
> > > > > > function for it.
> > > > > >
> > > > > > It inits the abuf, then allocates it to the requested size.
> > > > > >
> > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > ---
> > > > > >
> > > > > > Changes in v2:
> > > > > > - Add new patch with a helper for initing and allocating a buffer
> > > > > >
> > > > > >  boot/cedit.c          |  3 +--
> > > > > >  boot/scene.c          |  3 +--
> > > > > >  boot/scene_textline.c |  3 +--
> > > > > >  include/abuf.h        | 11 +++++++++++
> > > > > >  lib/abuf.c            |  9 +++++++++
> > > > > >  lib/of_live.c         |  3 +--
> > > > > >  test/lib/abuf.c       | 22 ++++++++++++++++++++++
> > > > > >  7 files changed, 46 insertions(+), 8 deletions(-)
> > > > >
> > > > > This just made me look again at the abuf implementation itself and
> > > > > become filled with regret I didn't reject it back in 2021. We're
> > > > > introducing wrappers around standard functions and calling conventions /
> > > > > patterns with something homegrown (and so not intuitive to others) that
> > > > > mainly hides the "sysmem" challenge we also have  and I wish you were
> > > > > interested in revisiting how that part of sandbox works instead. And
> > > > > even if this is a better design, for the sake of argument, it's not
> > > > > something everyone else is used to. And that's important.
> > > > >
> > > > > If we *really* need something different / new here, I'd rather see us go
> > > > > and wrap common/dlmalloc.c with kmalloc/kfree/etc and so give people
> > > > > something even more familiar-looking.
> > > >
> > > > The purpose of abuf is actually not about hiding sysmem - you can see
> > > > that if you look at lib/abuf.c and the tests. It provides an easy way
> > > > to manage buffers, which we do a lot in U-Boot, particularly with
> > > > standard boot. It deals with whether the buffer is allocated or not,
> > > > so freeing is easy. With expo I want to be able to manage a buffer
> > > > containing an autoboot string in high-level code, say, without
> > > > worrying whether it has to be realloced.
> > >
> > > It's a wrapper around free/malloc/et al, which then also includes the
> > > sandbox-specific requirement of *sysmap* stuff. This is best shown by
> > > the next two patches which convert from standard malloc/free patterns
> > > (which both humans and analysis checkers are fond of) to a home grown
> > > invention.
> >
> > Yes, although I should point out that most of my code is a home-grown
> > invention, if by that you mean it wasn't invented by someone else and
> > then posted by me as if it were my own invention.
>
> Well, I'm referring to the cases where solutions to the problems exist,
> but you're inventing a new one instead.
>
> > > > For sysmem, I don't see a viable alternative, which keeps can reliably
> > > > keep addresses consistent across multiple phases (e.g. sandbox_vpl).
> > > > Also, I don't believe addresses like 0x7ffff7fbc000 are
> > > > human-friendly, yet that is what we would see in tests if we dropped
> > > > the mapping. The point of sandbox is to test U-Boot, not expose the
> > > > system internals. But we could discuss it if you like?
> > >
> > > I'm not sure how this is relevant to the general high level point of
> > > "sandbox should be able to, in 2025, be designed so that special macros
> > > do not need to be everywhere for the sanity checking it can do to
> > > happen". If my web browser is going to suck up 11GiB of memory all the
> > > time I can live with sandbox using 1/2/4/whatever GiB of memory when it
> > > runs. The "this is not a pointer" problem sandbox can solve could be
> > > done differently these days.
> >
> > The special macros have several purposes:
> > 1. Allow sandbox to have a contiguous region of memory at address 0,
> > while the pointers are wherever the RAM buffer happens to end up
> > 2. Provide an indication of whether code has been converted to sandbox
> > yet, or not
> > 3. Provide a way to mark all casts to/from address/pointers so it is
> > clear that the cast is intentional
> > 4. Ensure that sandbox seg-faults if a bad pointer is used
> >
> > You seemed quite happy about it until my EFI test was rejected.
>
> No, I've never really been happy with the macros and never understood
> why we couldn't do the useful sanity checking wholly under arch/sandbox/
>
> > > > In general I am sensing that there are a lot of things which bug you
> > > > about the direction of U-Boot. and that you feel very differently
> > > > about some things than you did in 2021. I have quite a few concerns
> > > > too. So how about we make time to discuss these and see if we can get
> > > > on the same page, at least somewhat?
> > >
> > > Yes, we've had numerous long threads, and a few calls. You are then
> > > unhappy with what I say I want to see done, want me to just take your
> > > changes instead and "Say Yes" more. I wonder if I had said No, or gather
> > > more feedback from potential users, or just slow down, more back in 2021
> > > if we would not be in a different position here.
> >
> > I wonder if you had said yes more, if we might have got standard boot
> > done a few years earlier and would not be stuck now on EFI bootmgr.
>
> No, we probably would have just had more developers leave as I overrode
> their objections.
>
> > > > Coming back to this patch though, this is really about a code-side
> > > > win, rather than anything else.
> > >
> > > No, it's about how similar to the "log a message and return" convention
> > > change you wanted to make and then dropped, how I do not see good value
> > > in changing from standard convention of malloc/free to abuf being a win.
> > > Maybe this is a case where borrowing the kernel's mempool logic would be
> > > a win instead? Because another part of this is that we want to avoid
> > > making up our own APIs for things if we can instead borrow something
> > > from the kernel, where most of the rest of the community will already be
> > > familiar.
> >
> > mempool is actually lmb, as I understand it. We haven't done that
> > rename. It allocates from a region of pre-allocated memory. It doesn't
> > use malloc(). So to me, that is quite different.
>
> Then we should probably just drop the abuf thing and use standard
> conventions that developers are used to.

No, abuf is a valuable abstraction which makes it easier to avoid
alloc/free errors, size errors, etc. Python has bytes, C++ has
std::byte. Linux has sk_buff which is a bit like membuf. Perhaps Linux
would benefit from abuf but its memory model and usage is quite a bit
different, so I'm not sure.

Regards,
Simon

Re: [PATCH v2 01/18] abuf: Add a helper for initing and allocating a buffer

[Simon Glass]

Hi Tom,

On Wed, 14 May 2025 at 20:44, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Tom,
>
> On Tue, 13 May 2025 at 00:35, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Sat, May 10, 2025 at 01:27:03PM +0200, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Tue, 6 May 2025 at 21:48, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Tue, May 06, 2025 at 03:23:39PM +0200, Simon Glass wrote:
> > > > > Hi Tom,
> > > > >
> > > > > On Mon, 5 May 2025 at 22:38, Tom Rini <trini@konsulko.com> wrote:
> > > > > >
> > > > > > On Thu, May 01, 2025 at 07:37:01AM -0600, Simon Glass wrote:
> > > > > >
> > > > > > > This construct appears in various places. Reduce code size by adding a
> > > > > > > function for it.
> > > > > > >
> > > > > > > It inits the abuf, then allocates it to the requested size.
> > > > > > >
> > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > > ---
> > > > > > >
> > > > > > > Changes in v2:
> > > > > > > - Add new patch with a helper for initing and allocating a buffer
> > > > > > >
> > > > > > >  boot/cedit.c          |  3 +--
> > > > > > >  boot/scene.c          |  3 +--
> > > > > > >  boot/scene_textline.c |  3 +--
> > > > > > >  include/abuf.h        | 11 +++++++++++
> > > > > > >  lib/abuf.c            |  9 +++++++++
> > > > > > >  lib/of_live.c         |  3 +--
> > > > > > >  test/lib/abuf.c       | 22 ++++++++++++++++++++++
> > > > > > >  7 files changed, 46 insertions(+), 8 deletions(-)
> > > > > >
> > > > > > This just made me look again at the abuf implementation itself and
> > > > > > become filled with regret I didn't reject it back in 2021. We're
> > > > > > introducing wrappers around standard functions and calling conventions /
> > > > > > patterns with something homegrown (and so not intuitive to others) that
> > > > > > mainly hides the "sysmem" challenge we also have  and I wish you were
> > > > > > interested in revisiting how that part of sandbox works instead. And
> > > > > > even if this is a better design, for the sake of argument, it's not
> > > > > > something everyone else is used to. And that's important.
> > > > > >
> > > > > > If we *really* need something different / new here, I'd rather see us go
> > > > > > and wrap common/dlmalloc.c with kmalloc/kfree/etc and so give people
> > > > > > something even more familiar-looking.
> > > > >
> > > > > The purpose of abuf is actually not about hiding sysmem - you can see
> > > > > that if you look at lib/abuf.c and the tests. It provides an easy way
> > > > > to manage buffers, which we do a lot in U-Boot, particularly with
> > > > > standard boot. It deals with whether the buffer is allocated or not,
> > > > > so freeing is easy. With expo I want to be able to manage a buffer
> > > > > containing an autoboot string in high-level code, say, without
> > > > > worrying whether it has to be realloced.
> > > >
> > > > It's a wrapper around free/malloc/et al, which then also includes the
> > > > sandbox-specific requirement of *sysmap* stuff. This is best shown by
> > > > the next two patches which convert from standard malloc/free patterns
> > > > (which both humans and analysis checkers are fond of) to a home grown
> > > > invention.
> > >
> > > Yes, although I should point out that most of my code is a home-grown
> > > invention, if by that you mean it wasn't invented by someone else and
> > > then posted by me as if it were my own invention.
> >
> > Well, I'm referring to the cases where solutions to the problems exist,
> > but you're inventing a new one instead.
> >
> > > > > For sysmem, I don't see a viable alternative, which keeps can reliably
> > > > > keep addresses consistent across multiple phases (e.g. sandbox_vpl).
> > > > > Also, I don't believe addresses like 0x7ffff7fbc000 are
> > > > > human-friendly, yet that is what we would see in tests if we dropped
> > > > > the mapping. The point of sandbox is to test U-Boot, not expose the
> > > > > system internals. But we could discuss it if you like?
> > > >
> > > > I'm not sure how this is relevant to the general high level point of
> > > > "sandbox should be able to, in 2025, be designed so that special macros
> > > > do not need to be everywhere for the sanity checking it can do to
> > > > happen". If my web browser is going to suck up 11GiB of memory all the
> > > > time I can live with sandbox using 1/2/4/whatever GiB of memory when it
> > > > runs. The "this is not a pointer" problem sandbox can solve could be
> > > > done differently these days.
> > >
> > > The special macros have several purposes:
> > > 1. Allow sandbox to have a contiguous region of memory at address 0,
> > > while the pointers are wherever the RAM buffer happens to end up
> > > 2. Provide an indication of whether code has been converted to sandbox
> > > yet, or not
> > > 3. Provide a way to mark all casts to/from address/pointers so it is
> > > clear that the cast is intentional
> > > 4. Ensure that sandbox seg-faults if a bad pointer is used
> > >
> > > You seemed quite happy about it until my EFI test was rejected.
> >
> > No, I've never really been happy with the macros and never understood
> > why we couldn't do the useful sanity checking wholly under arch/sandbox/
> >
> > > > > In general I am sensing that there are a lot of things which bug you
> > > > > about the direction of U-Boot. and that you feel very differently
> > > > > about some things than you did in 2021. I have quite a few concerns
> > > > > too. So how about we make time to discuss these and see if we can get
> > > > > on the same page, at least somewhat?
> > > >
> > > > Yes, we've had numerous long threads, and a few calls. You are then
> > > > unhappy with what I say I want to see done, want me to just take your
> > > > changes instead and "Say Yes" more. I wonder if I had said No, or gather
> > > > more feedback from potential users, or just slow down, more back in 2021
> > > > if we would not be in a different position here.
> > >
> > > I wonder if you had said yes more, if we might have got standard boot
> > > done a few years earlier and would not be stuck now on EFI bootmgr.
> >
> > No, we probably would have just had more developers leave as I overrode
> > their objections.
> >
> > > > > Coming back to this patch though, this is really about a code-side
> > > > > win, rather than anything else.
> > > >
> > > > No, it's about how similar to the "log a message and return" convention
> > > > change you wanted to make and then dropped, how I do not see good value
> > > > in changing from standard convention of malloc/free to abuf being a win.
> > > > Maybe this is a case where borrowing the kernel's mempool logic would be
> > > > a win instead? Because another part of this is that we want to avoid
> > > > making up our own APIs for things if we can instead borrow something
> > > > from the kernel, where most of the rest of the community will already be
> > > > familiar.
> > >
> > > mempool is actually lmb, as I understand it. We haven't done that
> > > rename. It allocates from a region of pre-allocated memory. It doesn't
> > > use malloc(). So to me, that is quite different.
> >
> > Then we should probably just drop the abuf thing and use standard
> > conventions that developers are used to.
>
> No, abuf is a valuable abstraction which makes it easier to avoid
> alloc/free errors, size errors, etc. Python has bytes, C++ has
> std::byte. Linux has sk_buff which is a bit like membuf. Perhaps Linux
> would benefit from abuf but its memory model and usage is quite a bit
> different, so I'm not sure.

This series seems to be blocked on a desire to remove abuf from
U-Boot. Other than that I have a minor comment to resolve on the new
'bootdev order' command. So I sent a patch for that.

Regards,
Simon

Re: [PATCH v2 01/18] abuf: Add a helper for initing and allocating a buffer

[Tom Rini]

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

On Sat, May 24, 2025 at 08:51:43AM +0100, Simon Glass wrote:
> Hi Tom,
> 
> On Wed, 14 May 2025 at 20:44, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Tom,
> >
> > On Tue, 13 May 2025 at 00:35, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Sat, May 10, 2025 at 01:27:03PM +0200, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Tue, 6 May 2025 at 21:48, Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > On Tue, May 06, 2025 at 03:23:39PM +0200, Simon Glass wrote:
> > > > > > Hi Tom,
> > > > > >
> > > > > > On Mon, 5 May 2025 at 22:38, Tom Rini <trini@konsulko.com> wrote:
> > > > > > >
> > > > > > > On Thu, May 01, 2025 at 07:37:01AM -0600, Simon Glass wrote:
> > > > > > >
> > > > > > > > This construct appears in various places. Reduce code size by adding a
> > > > > > > > function for it.
> > > > > > > >
> > > > > > > > It inits the abuf, then allocates it to the requested size.
> > > > > > > >
> > > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > > > ---
> > > > > > > >
> > > > > > > > Changes in v2:
> > > > > > > > - Add new patch with a helper for initing and allocating a buffer
> > > > > > > >
> > > > > > > >  boot/cedit.c          |  3 +--
> > > > > > > >  boot/scene.c          |  3 +--
> > > > > > > >  boot/scene_textline.c |  3 +--
> > > > > > > >  include/abuf.h        | 11 +++++++++++
> > > > > > > >  lib/abuf.c            |  9 +++++++++
> > > > > > > >  lib/of_live.c         |  3 +--
> > > > > > > >  test/lib/abuf.c       | 22 ++++++++++++++++++++++
> > > > > > > >  7 files changed, 46 insertions(+), 8 deletions(-)
> > > > > > >
> > > > > > > This just made me look again at the abuf implementation itself and
> > > > > > > become filled with regret I didn't reject it back in 2021. We're
> > > > > > > introducing wrappers around standard functions and calling conventions /
> > > > > > > patterns with something homegrown (and so not intuitive to others) that
> > > > > > > mainly hides the "sysmem" challenge we also have  and I wish you were
> > > > > > > interested in revisiting how that part of sandbox works instead. And
> > > > > > > even if this is a better design, for the sake of argument, it's not
> > > > > > > something everyone else is used to. And that's important.
> > > > > > >
> > > > > > > If we *really* need something different / new here, I'd rather see us go
> > > > > > > and wrap common/dlmalloc.c with kmalloc/kfree/etc and so give people
> > > > > > > something even more familiar-looking.
> > > > > >
> > > > > > The purpose of abuf is actually not about hiding sysmem - you can see
> > > > > > that if you look at lib/abuf.c and the tests. It provides an easy way
> > > > > > to manage buffers, which we do a lot in U-Boot, particularly with
> > > > > > standard boot. It deals with whether the buffer is allocated or not,
> > > > > > so freeing is easy. With expo I want to be able to manage a buffer
> > > > > > containing an autoboot string in high-level code, say, without
> > > > > > worrying whether it has to be realloced.
> > > > >
> > > > > It's a wrapper around free/malloc/et al, which then also includes the
> > > > > sandbox-specific requirement of *sysmap* stuff. This is best shown by
> > > > > the next two patches which convert from standard malloc/free patterns
> > > > > (which both humans and analysis checkers are fond of) to a home grown
> > > > > invention.
> > > >
> > > > Yes, although I should point out that most of my code is a home-grown
> > > > invention, if by that you mean it wasn't invented by someone else and
> > > > then posted by me as if it were my own invention.
> > >
> > > Well, I'm referring to the cases where solutions to the problems exist,
> > > but you're inventing a new one instead.
> > >
> > > > > > For sysmem, I don't see a viable alternative, which keeps can reliably
> > > > > > keep addresses consistent across multiple phases (e.g. sandbox_vpl).
> > > > > > Also, I don't believe addresses like 0x7ffff7fbc000 are
> > > > > > human-friendly, yet that is what we would see in tests if we dropped
> > > > > > the mapping. The point of sandbox is to test U-Boot, not expose the
> > > > > > system internals. But we could discuss it if you like?
> > > > >
> > > > > I'm not sure how this is relevant to the general high level point of
> > > > > "sandbox should be able to, in 2025, be designed so that special macros
> > > > > do not need to be everywhere for the sanity checking it can do to
> > > > > happen". If my web browser is going to suck up 11GiB of memory all the
> > > > > time I can live with sandbox using 1/2/4/whatever GiB of memory when it
> > > > > runs. The "this is not a pointer" problem sandbox can solve could be
> > > > > done differently these days.
> > > >
> > > > The special macros have several purposes:
> > > > 1. Allow sandbox to have a contiguous region of memory at address 0,
> > > > while the pointers are wherever the RAM buffer happens to end up
> > > > 2. Provide an indication of whether code has been converted to sandbox
> > > > yet, or not
> > > > 3. Provide a way to mark all casts to/from address/pointers so it is
> > > > clear that the cast is intentional
> > > > 4. Ensure that sandbox seg-faults if a bad pointer is used
> > > >
> > > > You seemed quite happy about it until my EFI test was rejected.
> > >
> > > No, I've never really been happy with the macros and never understood
> > > why we couldn't do the useful sanity checking wholly under arch/sandbox/
> > >
> > > > > > In general I am sensing that there are a lot of things which bug you
> > > > > > about the direction of U-Boot. and that you feel very differently
> > > > > > about some things than you did in 2021. I have quite a few concerns
> > > > > > too. So how about we make time to discuss these and see if we can get
> > > > > > on the same page, at least somewhat?
> > > > >
> > > > > Yes, we've had numerous long threads, and a few calls. You are then
> > > > > unhappy with what I say I want to see done, want me to just take your
> > > > > changes instead and "Say Yes" more. I wonder if I had said No, or gather
> > > > > more feedback from potential users, or just slow down, more back in 2021
> > > > > if we would not be in a different position here.
> > > >
> > > > I wonder if you had said yes more, if we might have got standard boot
> > > > done a few years earlier and would not be stuck now on EFI bootmgr.
> > >
> > > No, we probably would have just had more developers leave as I overrode
> > > their objections.
> > >
> > > > > > Coming back to this patch though, this is really about a code-side
> > > > > > win, rather than anything else.
> > > > >
> > > > > No, it's about how similar to the "log a message and return" convention
> > > > > change you wanted to make and then dropped, how I do not see good value
> > > > > in changing from standard convention of malloc/free to abuf being a win.
> > > > > Maybe this is a case where borrowing the kernel's mempool logic would be
> > > > > a win instead? Because another part of this is that we want to avoid
> > > > > making up our own APIs for things if we can instead borrow something
> > > > > from the kernel, where most of the rest of the community will already be
> > > > > familiar.
> > > >
> > > > mempool is actually lmb, as I understand it. We haven't done that
> > > > rename. It allocates from a region of pre-allocated memory. It doesn't
> > > > use malloc(). So to me, that is quite different.
> > >
> > > Then we should probably just drop the abuf thing and use standard
> > > conventions that developers are used to.
> >
> > No, abuf is a valuable abstraction which makes it easier to avoid
> > alloc/free errors, size errors, etc. Python has bytes, C++ has
> > std::byte. Linux has sk_buff which is a bit like membuf. Perhaps Linux
> > would benefit from abuf but its memory model and usage is quite a bit
> > different, so I'm not sure.
> 
> This series seems to be blocked on a desire to remove abuf from
> U-Boot. Other than that I have a minor comment to resolve on the new
> 'bootdev order' command. So I sent a patch for that.

Yes, I disagree with spreading the use of abuf. Whatever benefits it may
or may not have are outweighed by not following any normal conventions
the rest of the developers use.

-- 
Tom

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

Re: [PATCH v2 01/18] abuf: Add a helper for initing and allocating a buffer

[Simon Glass]

Hi Tom,

On Sat, 24 May 2025 at 15:20, Tom Rini <trini@konsulko.com> wrote:
>
> On Sat, May 24, 2025 at 08:51:43AM +0100, Simon Glass wrote:
> > Hi Tom,
> >
> > On Wed, 14 May 2025 at 20:44, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Tom,
> > >
> > > On Tue, 13 May 2025 at 00:35, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Sat, May 10, 2025 at 01:27:03PM +0200, Simon Glass wrote:
> > > > > Hi Tom,
> > > > >
> > > > > On Tue, 6 May 2025 at 21:48, Tom Rini <trini@konsulko.com> wrote:
> > > > > >
> > > > > > On Tue, May 06, 2025 at 03:23:39PM +0200, Simon Glass wrote:
> > > > > > > Hi Tom,
> > > > > > >
> > > > > > > On Mon, 5 May 2025 at 22:38, Tom Rini <trini@konsulko.com> wrote:
> > > > > > > >
> > > > > > > > On Thu, May 01, 2025 at 07:37:01AM -0600, Simon Glass wrote:
> > > > > > > >
> > > > > > > > > This construct appears in various places. Reduce code size by adding a
> > > > > > > > > function for it.
> > > > > > > > >
> > > > > > > > > It inits the abuf, then allocates it to the requested size.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > > > > ---
> > > > > > > > >
> > > > > > > > > Changes in v2:
> > > > > > > > > - Add new patch with a helper for initing and allocating a buffer
> > > > > > > > >
> > > > > > > > >  boot/cedit.c          |  3 +--
> > > > > > > > >  boot/scene.c          |  3 +--
> > > > > > > > >  boot/scene_textline.c |  3 +--
> > > > > > > > >  include/abuf.h        | 11 +++++++++++
> > > > > > > > >  lib/abuf.c            |  9 +++++++++
> > > > > > > > >  lib/of_live.c         |  3 +--
> > > > > > > > >  test/lib/abuf.c       | 22 ++++++++++++++++++++++
> > > > > > > > >  7 files changed, 46 insertions(+), 8 deletions(-)
> > > > > > > >
> > > > > > > > This just made me look again at the abuf implementation itself and
> > > > > > > > become filled with regret I didn't reject it back in 2021. We're
> > > > > > > > introducing wrappers around standard functions and calling conventions /
> > > > > > > > patterns with something homegrown (and so not intuitive to others) that
> > > > > > > > mainly hides the "sysmem" challenge we also have  and I wish you were
> > > > > > > > interested in revisiting how that part of sandbox works instead. And
> > > > > > > > even if this is a better design, for the sake of argument, it's not
> > > > > > > > something everyone else is used to. And that's important.
> > > > > > > >
> > > > > > > > If we *really* need something different / new here, I'd rather see us go
> > > > > > > > and wrap common/dlmalloc.c with kmalloc/kfree/etc and so give people
> > > > > > > > something even more familiar-looking.
> > > > > > >
> > > > > > > The purpose of abuf is actually not about hiding sysmem - you can see
> > > > > > > that if you look at lib/abuf.c and the tests. It provides an easy way
> > > > > > > to manage buffers, which we do a lot in U-Boot, particularly with
> > > > > > > standard boot. It deals with whether the buffer is allocated or not,
> > > > > > > so freeing is easy. With expo I want to be able to manage a buffer
> > > > > > > containing an autoboot string in high-level code, say, without
> > > > > > > worrying whether it has to be realloced.
> > > > > >
> > > > > > It's a wrapper around free/malloc/et al, which then also includes the
> > > > > > sandbox-specific requirement of *sysmap* stuff. This is best shown by
> > > > > > the next two patches which convert from standard malloc/free patterns
> > > > > > (which both humans and analysis checkers are fond of) to a home grown
> > > > > > invention.
> > > > >
> > > > > Yes, although I should point out that most of my code is a home-grown
> > > > > invention, if by that you mean it wasn't invented by someone else and
> > > > > then posted by me as if it were my own invention.
> > > >
> > > > Well, I'm referring to the cases where solutions to the problems exist,
> > > > but you're inventing a new one instead.
> > > >
> > > > > > > For sysmem, I don't see a viable alternative, which keeps can reliably
> > > > > > > keep addresses consistent across multiple phases (e.g. sandbox_vpl).
> > > > > > > Also, I don't believe addresses like 0x7ffff7fbc000 are
> > > > > > > human-friendly, yet that is what we would see in tests if we dropped
> > > > > > > the mapping. The point of sandbox is to test U-Boot, not expose the
> > > > > > > system internals. But we could discuss it if you like?
> > > > > >
> > > > > > I'm not sure how this is relevant to the general high level point of
> > > > > > "sandbox should be able to, in 2025, be designed so that special macros
> > > > > > do not need to be everywhere for the sanity checking it can do to
> > > > > > happen". If my web browser is going to suck up 11GiB of memory all the
> > > > > > time I can live with sandbox using 1/2/4/whatever GiB of memory when it
> > > > > > runs. The "this is not a pointer" problem sandbox can solve could be
> > > > > > done differently these days.
> > > > >
> > > > > The special macros have several purposes:
> > > > > 1. Allow sandbox to have a contiguous region of memory at address 0,
> > > > > while the pointers are wherever the RAM buffer happens to end up
> > > > > 2. Provide an indication of whether code has been converted to sandbox
> > > > > yet, or not
> > > > > 3. Provide a way to mark all casts to/from address/pointers so it is
> > > > > clear that the cast is intentional
> > > > > 4. Ensure that sandbox seg-faults if a bad pointer is used
> > > > >
> > > > > You seemed quite happy about it until my EFI test was rejected.
> > > >
> > > > No, I've never really been happy with the macros and never understood
> > > > why we couldn't do the useful sanity checking wholly under arch/sandbox/
> > > >
> > > > > > > In general I am sensing that there are a lot of things which bug you
> > > > > > > about the direction of U-Boot. and that you feel very differently
> > > > > > > about some things than you did in 2021. I have quite a few concerns
> > > > > > > too. So how about we make time to discuss these and see if we can get
> > > > > > > on the same page, at least somewhat?
> > > > > >
> > > > > > Yes, we've had numerous long threads, and a few calls. You are then
> > > > > > unhappy with what I say I want to see done, want me to just take your
> > > > > > changes instead and "Say Yes" more. I wonder if I had said No, or gather
> > > > > > more feedback from potential users, or just slow down, more back in 2021
> > > > > > if we would not be in a different position here.
> > > > >
> > > > > I wonder if you had said yes more, if we might have got standard boot
> > > > > done a few years earlier and would not be stuck now on EFI bootmgr.
> > > >
> > > > No, we probably would have just had more developers leave as I overrode
> > > > their objections.
> > > >
> > > > > > > Coming back to this patch though, this is really about a code-side
> > > > > > > win, rather than anything else.
> > > > > >
> > > > > > No, it's about how similar to the "log a message and return" convention
> > > > > > change you wanted to make and then dropped, how I do not see good value
> > > > > > in changing from standard convention of malloc/free to abuf being a win.
> > > > > > Maybe this is a case where borrowing the kernel's mempool logic would be
> > > > > > a win instead? Because another part of this is that we want to avoid
> > > > > > making up our own APIs for things if we can instead borrow something
> > > > > > from the kernel, where most of the rest of the community will already be
> > > > > > familiar.
> > > > >
> > > > > mempool is actually lmb, as I understand it. We haven't done that
> > > > > rename. It allocates from a region of pre-allocated memory. It doesn't
> > > > > use malloc(). So to me, that is quite different.
> > > >
> > > > Then we should probably just drop the abuf thing and use standard
> > > > conventions that developers are used to.
> > >
> > > No, abuf is a valuable abstraction which makes it easier to avoid
> > > alloc/free errors, size errors, etc. Python has bytes, C++ has
> > > std::byte. Linux has sk_buff which is a bit like membuf. Perhaps Linux
> > > would benefit from abuf but its memory model and usage is quite a bit
> > > different, so I'm not sure.
> >
> > This series seems to be blocked on a desire to remove abuf from
> > U-Boot. Other than that I have a minor comment to resolve on the new
> > 'bootdev order' command. So I sent a patch for that.
>
> Yes, I disagree with spreading the use of abuf. Whatever benefits it may
> or may not have are outweighed by not following any normal conventions
> the rest of the developers use.

Are you completely set on this?

Regards,
SImon

Re: [PATCH v2 01/18] abuf: Add a helper for initing and allocating a buffer

[Tom Rini]

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

On Sat, May 24, 2025 at 03:31:42PM +0100, Simon Glass wrote:
> Hi Tom,
> 
> On Sat, 24 May 2025 at 15:20, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Sat, May 24, 2025 at 08:51:43AM +0100, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Wed, 14 May 2025 at 20:44, Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Tom,
> > > >
> > > > On Tue, 13 May 2025 at 00:35, Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > On Sat, May 10, 2025 at 01:27:03PM +0200, Simon Glass wrote:
> > > > > > Hi Tom,
> > > > > >
> > > > > > On Tue, 6 May 2025 at 21:48, Tom Rini <trini@konsulko.com> wrote:
> > > > > > >
> > > > > > > On Tue, May 06, 2025 at 03:23:39PM +0200, Simon Glass wrote:
> > > > > > > > Hi Tom,
> > > > > > > >
> > > > > > > > On Mon, 5 May 2025 at 22:38, Tom Rini <trini@konsulko.com> wrote:
> > > > > > > > >
> > > > > > > > > On Thu, May 01, 2025 at 07:37:01AM -0600, Simon Glass wrote:
> > > > > > > > >
> > > > > > > > > > This construct appears in various places. Reduce code size by adding a
> > > > > > > > > > function for it.
> > > > > > > > > >
> > > > > > > > > > It inits the abuf, then allocates it to the requested size.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > > > > > ---
> > > > > > > > > >
> > > > > > > > > > Changes in v2:
> > > > > > > > > > - Add new patch with a helper for initing and allocating a buffer
> > > > > > > > > >
> > > > > > > > > >  boot/cedit.c          |  3 +--
> > > > > > > > > >  boot/scene.c          |  3 +--
> > > > > > > > > >  boot/scene_textline.c |  3 +--
> > > > > > > > > >  include/abuf.h        | 11 +++++++++++
> > > > > > > > > >  lib/abuf.c            |  9 +++++++++
> > > > > > > > > >  lib/of_live.c         |  3 +--
> > > > > > > > > >  test/lib/abuf.c       | 22 ++++++++++++++++++++++
> > > > > > > > > >  7 files changed, 46 insertions(+), 8 deletions(-)
> > > > > > > > >
> > > > > > > > > This just made me look again at the abuf implementation itself and
> > > > > > > > > become filled with regret I didn't reject it back in 2021. We're
> > > > > > > > > introducing wrappers around standard functions and calling conventions /
> > > > > > > > > patterns with something homegrown (and so not intuitive to others) that
> > > > > > > > > mainly hides the "sysmem" challenge we also have  and I wish you were
> > > > > > > > > interested in revisiting how that part of sandbox works instead. And
> > > > > > > > > even if this is a better design, for the sake of argument, it's not
> > > > > > > > > something everyone else is used to. And that's important.
> > > > > > > > >
> > > > > > > > > If we *really* need something different / new here, I'd rather see us go
> > > > > > > > > and wrap common/dlmalloc.c with kmalloc/kfree/etc and so give people
> > > > > > > > > something even more familiar-looking.
> > > > > > > >
> > > > > > > > The purpose of abuf is actually not about hiding sysmem - you can see
> > > > > > > > that if you look at lib/abuf.c and the tests. It provides an easy way
> > > > > > > > to manage buffers, which we do a lot in U-Boot, particularly with
> > > > > > > > standard boot. It deals with whether the buffer is allocated or not,
> > > > > > > > so freeing is easy. With expo I want to be able to manage a buffer
> > > > > > > > containing an autoboot string in high-level code, say, without
> > > > > > > > worrying whether it has to be realloced.
> > > > > > >
> > > > > > > It's a wrapper around free/malloc/et al, which then also includes the
> > > > > > > sandbox-specific requirement of *sysmap* stuff. This is best shown by
> > > > > > > the next two patches which convert from standard malloc/free patterns
> > > > > > > (which both humans and analysis checkers are fond of) to a home grown
> > > > > > > invention.
> > > > > >
> > > > > > Yes, although I should point out that most of my code is a home-grown
> > > > > > invention, if by that you mean it wasn't invented by someone else and
> > > > > > then posted by me as if it were my own invention.
> > > > >
> > > > > Well, I'm referring to the cases where solutions to the problems exist,
> > > > > but you're inventing a new one instead.
> > > > >
> > > > > > > > For sysmem, I don't see a viable alternative, which keeps can reliably
> > > > > > > > keep addresses consistent across multiple phases (e.g. sandbox_vpl).
> > > > > > > > Also, I don't believe addresses like 0x7ffff7fbc000 are
> > > > > > > > human-friendly, yet that is what we would see in tests if we dropped
> > > > > > > > the mapping. The point of sandbox is to test U-Boot, not expose the
> > > > > > > > system internals. But we could discuss it if you like?
> > > > > > >
> > > > > > > I'm not sure how this is relevant to the general high level point of
> > > > > > > "sandbox should be able to, in 2025, be designed so that special macros
> > > > > > > do not need to be everywhere for the sanity checking it can do to
> > > > > > > happen". If my web browser is going to suck up 11GiB of memory all the
> > > > > > > time I can live with sandbox using 1/2/4/whatever GiB of memory when it
> > > > > > > runs. The "this is not a pointer" problem sandbox can solve could be
> > > > > > > done differently these days.
> > > > > >
> > > > > > The special macros have several purposes:
> > > > > > 1. Allow sandbox to have a contiguous region of memory at address 0,
> > > > > > while the pointers are wherever the RAM buffer happens to end up
> > > > > > 2. Provide an indication of whether code has been converted to sandbox
> > > > > > yet, or not
> > > > > > 3. Provide a way to mark all casts to/from address/pointers so it is
> > > > > > clear that the cast is intentional
> > > > > > 4. Ensure that sandbox seg-faults if a bad pointer is used
> > > > > >
> > > > > > You seemed quite happy about it until my EFI test was rejected.
> > > > >
> > > > > No, I've never really been happy with the macros and never understood
> > > > > why we couldn't do the useful sanity checking wholly under arch/sandbox/
> > > > >
> > > > > > > > In general I am sensing that there are a lot of things which bug you
> > > > > > > > about the direction of U-Boot. and that you feel very differently
> > > > > > > > about some things than you did in 2021. I have quite a few concerns
> > > > > > > > too. So how about we make time to discuss these and see if we can get
> > > > > > > > on the same page, at least somewhat?
> > > > > > >
> > > > > > > Yes, we've had numerous long threads, and a few calls. You are then
> > > > > > > unhappy with what I say I want to see done, want me to just take your
> > > > > > > changes instead and "Say Yes" more. I wonder if I had said No, or gather
> > > > > > > more feedback from potential users, or just slow down, more back in 2021
> > > > > > > if we would not be in a different position here.
> > > > > >
> > > > > > I wonder if you had said yes more, if we might have got standard boot
> > > > > > done a few years earlier and would not be stuck now on EFI bootmgr.
> > > > >
> > > > > No, we probably would have just had more developers leave as I overrode
> > > > > their objections.
> > > > >
> > > > > > > > Coming back to this patch though, this is really about a code-side
> > > > > > > > win, rather than anything else.
> > > > > > >
> > > > > > > No, it's about how similar to the "log a message and return" convention
> > > > > > > change you wanted to make and then dropped, how I do not see good value
> > > > > > > in changing from standard convention of malloc/free to abuf being a win.
> > > > > > > Maybe this is a case where borrowing the kernel's mempool logic would be
> > > > > > > a win instead? Because another part of this is that we want to avoid
> > > > > > > making up our own APIs for things if we can instead borrow something
> > > > > > > from the kernel, where most of the rest of the community will already be
> > > > > > > familiar.
> > > > > >
> > > > > > mempool is actually lmb, as I understand it. We haven't done that
> > > > > > rename. It allocates from a region of pre-allocated memory. It doesn't
> > > > > > use malloc(). So to me, that is quite different.
> > > > >
> > > > > Then we should probably just drop the abuf thing and use standard
> > > > > conventions that developers are used to.
> > > >
> > > > No, abuf is a valuable abstraction which makes it easier to avoid
> > > > alloc/free errors, size errors, etc. Python has bytes, C++ has
> > > > std::byte. Linux has sk_buff which is a bit like membuf. Perhaps Linux
> > > > would benefit from abuf but its memory model and usage is quite a bit
> > > > different, so I'm not sure.
> > >
> > > This series seems to be blocked on a desire to remove abuf from
> > > U-Boot. Other than that I have a minor comment to resolve on the new
> > > 'bootdev order' command. So I sent a patch for that.
> >
> > Yes, I disagree with spreading the use of abuf. Whatever benefits it may
> > or may not have are outweighed by not following any normal conventions
> > the rest of the developers use.
> 
> Are you completely set on this?

I sure would appreciate it if any other developer would chime in and say
that they do see value here and that I'm missing something. You've made
your case, I've explained what you're missing, and you seem set on
ignoring it.

-- 
Tom

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