[PATCH 00/17] bootstd: Useability improvements

[PATCH 00/17] 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.


Simon Glass (17):
  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 a test image for 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          | 112 +++++++++++++++++++++++----------
 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 ++++++
 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                        |  28 ++++-----
 include/bootdev.h              |  60 ++++++++++--------
 include/bootflow.h             |   2 +-
 include/bootmeth.h             |  12 ++--
 include/bootstd.h              |  10 +++
 include/fs.h                   |  18 +++---
 test/boot/bootdev.c            |  83 ++++++++++++++++++------
 test/boot/bootflow.c           |  10 +--
 test/dm/blk.c                  |   4 +-
 test/dm/usb.c                  |   8 +--
 test/py/tests/test_ut.py       | 111 +++++++++++++++++++++++++-------
 26 files changed, 487 insertions(+), 203 deletions(-)

-- 
2.43.0

base-commit: 569681e993486b830035064f23ec87bcd70795d1
branch: schc

[PATCH 01/17] 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>
---

 boot/bootmeth-uclass.c | 19 ++++++++++---------
 fs/fs.c                | 25 +++++++++++--------------
 include/fs.h           |  8 +++++---
 3 files changed, 26 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 77f7879276a..e904b93258a 100644
--- a/fs/fs.c
+++ b/fs/fs.c
@@ -1010,31 +1010,27 @@ 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)
+	abuf_init(buf);
+	if (!abuf_realloc(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;
 }
@@ -1043,8 +1039,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))
@@ -1063,8 +1060,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 2474880385d..7ff649da821 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
@@ -326,11 +327,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 02/17] 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>
---

 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 e904b93258a..3505e698ba2 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>
@@ -1036,11 +1037,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;
 
@@ -1057,11 +1056,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 7ff649da821..653e6e37738 100644
--- a/include/fs.h
+++ b/include/fs.h
@@ -344,15 +344,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 03/17] 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>
---

 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 04/17] 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>
---

 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 05/17] 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>
---

 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 06/17] 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>
---

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

diff --git a/boot/bootflow.c b/boot/bootflow.c
index 28c606b0258..83a1d3a7c8b 100644
--- a/boot/bootflow.c
+++ b/boot/bootflow.c
@@ -198,27 +198,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 07/17] 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.

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

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

diff --git a/boot/bootflow.c b/boot/bootflow.c
index 83a1d3a7c8b..473e4d5f02c 100644
--- a/boot/bootflow.c
+++ b/boot/bootflow.c
@@ -292,7 +292,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 08/17] 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>
---

 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 b8adb597e11..aad5f36dcb5 100644
--- a/test/py/tests/test_ut.py
+++ b/test/py/tests/test_ut.py
@@ -187,27 +187,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 = 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 = 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)
 
@@ -225,11 +220,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}')
@@ -239,6 +235,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')
@@ -582,7 +605,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 09/17] test/py: Add a test image for Ubuntu

[Simon Glass]

Add an extlinux image that contains a few Ubuntu entries.

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

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

 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 aad5f36dcb5..81f68c48574 100644
--- a/test/py/tests/test_ut.py
+++ b/test/py/tests/test_ut.py
@@ -262,6 +262,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')
@@ -611,6 +650,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 10/17] 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>
---

 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 11/17] 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>
---

 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 12/17] 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>
---

 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 13/17] 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>
---

 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 14/17] 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>
---

 boot/bootdev-uclass.c     | 37 +++++++++++++++++++++++++++++++++++++
 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, 188 insertions(+)

diff --git a/boot/bootdev-uclass.c b/boot/bootdev-uclass.c
index cfb298d5be5..282f1d7b5c5 100644
--- a/boot/bootdev-uclass.c
+++ b/boot/bootdev-uclass.c
@@ -936,6 +936,43 @@ 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;
+
+	LOGR("bsb", uclass_first_device_err(UCLASS_BOOTSTD, &bootstd));
+
+	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 15/17] 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>
---

 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 16/17] 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>
---

 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 282f1d7b5c5..1e52ccc44c3 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 17/17] 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>

---

 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 09/17] test/py: Add a test image for Ubuntu

[Tom Rini]

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

On Wed, Mar 19, 2025 at 03:38:03PM +0100, Simon Glass wrote:

> Add an extlinux image that contains a few Ubuntu entries.
> 
> Increase the number of sandbox-USB-hub ports to permit this.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>

I don't understand what this test adds. In neither the current Fedora
test nor in this new test are we actually booting something, we're just
taking a sample extlinux.conf and making sure it doesn't fail. Is it
that we're testing in a useful fashion now having two labels?

We should probably be clear about what we're doing in the tests and
instead of adding seemingly arbitrary distributions add an extlinux test
and testcases.

-- 
Tom

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

Re: [PATCH 09/17] test/py: Add a test image for Ubuntu

[Simon Glass]

Hi Tom,

On Wed, 19 Mar 2025 at 15:52, Tom Rini <trini@konsulko.com> wrote:
>
> On Wed, Mar 19, 2025 at 03:38:03PM +0100, Simon Glass wrote:
>
> > Add an extlinux image that contains a few Ubuntu entries.
> >
> > Increase the number of sandbox-USB-hub ports to permit this.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
>
> I don't understand what this test adds. In neither the current Fedora
> test nor in this new test are we actually booting something, we're just
> taking a sample extlinux.conf and making sure it doesn't fail. Is it
> that we're testing in a useful fashion now having two labels?

I didn't think so either, which is why I never did this before. But it
turns out that there were some bugs, too.

>
> We should probably be clear about what we're doing in the tests and
> instead of adding seemingly arbitrary distributions add an extlinux test
> and testcases.

This is not actually a test case. It is simply creating a new image.
The test cases are in the other patches, so please take a look there.

Regards,
Simon

Re: [PATCH 09/17] test/py: Add a test image for Ubuntu

[Tom Rini]

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

On Wed, Mar 19, 2025 at 03:04:28PM +0000, Simon Glass wrote:
> Hi Tom,
> 
> On Wed, 19 Mar 2025 at 15:52, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Wed, Mar 19, 2025 at 03:38:03PM +0100, Simon Glass wrote:
> >
> > > Add an extlinux image that contains a few Ubuntu entries.
> > >
> > > Increase the number of sandbox-USB-hub ports to permit this.
> > >
> > > Signed-off-by: Simon Glass <sjg@chromium.org>
> >
> > I don't understand what this test adds. In neither the current Fedora
> > test nor in this new test are we actually booting something, we're just
> > taking a sample extlinux.conf and making sure it doesn't fail. Is it
> > that we're testing in a useful fashion now having two labels?
> 
> I didn't think so either, which is why I never did this before. But it
> turns out that there were some bugs, too.

I don't understand you, sorry. You don't think so either to what?

> > We should probably be clear about what we're doing in the tests and
> > instead of adding seemingly arbitrary distributions add an extlinux test
> > and testcases.
> 
> This is not actually a test case. It is simply creating a new image.
> The test cases are in the other patches, so please take a look there.

Nothing in this series quickly reads as adding tests and fixing problems
with extlinux parsing, it's all bootmeth stuff?

-- 
Tom

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

Re: [PATCH 09/17] test/py: Add a test image for Ubuntu

[Simon Glass]

Hi Tom,

On Wed, 19 Mar 2025 at 16:35, Tom Rini <trini@konsulko.com> wrote:
>
> On Wed, Mar 19, 2025 at 03:04:28PM +0000, Simon Glass wrote:
> > Hi Tom,
> >
> > On Wed, 19 Mar 2025 at 15:52, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Wed, Mar 19, 2025 at 03:38:03PM +0100, Simon Glass wrote:
> > >
> > > > Add an extlinux image that contains a few Ubuntu entries.
> > > >
> > > > Increase the number of sandbox-USB-hub ports to permit this.
> > > >
> > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > >
> > > I don't understand what this test adds. In neither the current Fedora
> > > test nor in this new test are we actually booting something, we're just
> > > taking a sample extlinux.conf and making sure it doesn't fail. Is it
> > > that we're testing in a useful fashion now having two labels?
> >
> > I didn't think so either, which is why I never did this before. But it
> > turns out that there were some bugs, too.
>
> I don't understand you, sorry. You don't think so either to what?

I didn't think we needed two extlinux examples on different boot
devices, but we do.

>
> > > We should probably be clear about what we're doing in the tests and
> > > instead of adding seemingly arbitrary distributions add an extlinux test
> > > and testcases.
> >
> > This is not actually a test case. It is simply creating a new image.
> > The test cases are in the other patches, so please take a look there.
>
> Nothing in this series quickly reads as adding tests and fixing problems
> with extlinux parsing, it's all bootmeth stuff?

It isn't about the actual parsing of the .conf file, although I would
like to add tests there as we have none apart from what I have added
in my PXE series. It's more about having multiple devices with
bootable OSes on them. This series tidies up and fixes this. We need
to have an image available on more than one device to spot these
problems.

Currently we have two accessible to sandbox, one extlinux and one
EFI*. I decided to add a third, using extlinux.

Again, this is not a test case, but provides an image for the test
cases in this series.

Regards,
Simon

* The boot_efi() test in your tree is only a subset of the full test,
as you know

Re: [PATCH 09/17] test/py: Add a test image for Ubuntu

[Tom Rini]

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

On Thu, Mar 20, 2025 at 03:41:33AM +0000, Simon Glass wrote:
> Hi Tom,
> 
> On Wed, 19 Mar 2025 at 16:35, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Wed, Mar 19, 2025 at 03:04:28PM +0000, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Wed, 19 Mar 2025 at 15:52, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Wed, Mar 19, 2025 at 03:38:03PM +0100, Simon Glass wrote:
> > > >
> > > > > Add an extlinux image that contains a few Ubuntu entries.
> > > > >
> > > > > Increase the number of sandbox-USB-hub ports to permit this.
> > > > >
> > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > >
> > > > I don't understand what this test adds. In neither the current Fedora
> > > > test nor in this new test are we actually booting something, we're just
> > > > taking a sample extlinux.conf and making sure it doesn't fail. Is it
> > > > that we're testing in a useful fashion now having two labels?
> > >
> > > I didn't think so either, which is why I never did this before. But it
> > > turns out that there were some bugs, too.
> >
> > I don't understand you, sorry. You don't think so either to what?
> 
> I didn't think we needed two extlinux examples on different boot
> devices, but we do.

I'm not sure it's particularly clear what you're doing here then, or
why, but I'll find some time to read it all more deeply.

> > > > We should probably be clear about what we're doing in the tests and
> > > > instead of adding seemingly arbitrary distributions add an extlinux test
> > > > and testcases.
> > >
> > > This is not actually a test case. It is simply creating a new image.
> > > The test cases are in the other patches, so please take a look there.
> >
> > Nothing in this series quickly reads as adding tests and fixing problems
> > with extlinux parsing, it's all bootmeth stuff?
> 
> It isn't about the actual parsing of the .conf file, although I would
> like to add tests there as we have none apart from what I have added
> in my PXE series. It's more about having multiple devices with
> bootable OSes on them. This series tidies up and fixes this. We need
> to have an image available on more than one device to spot these
> problems.

And the existing tests for pxelinux that we have in mainline already,
don't forget those.

> Currently we have two accessible to sandbox, one extlinux and one
> EFI*. I decided to add a third, using extlinux.
> 
> Again, this is not a test case, but provides an image for the test
> cases in this series.

Adding mocked up things for use in sandbox is adding test cases.

-- 
Tom

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

Re: [PATCH 00/17] bootstd: Useability improvements

[Tom Rini]

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

On Wed, Mar 19, 2025 at 03:37:54PM +0100, Simon Glass wrote:

> 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.

This isn't based on mainline, either:
> base-commit: 569681e993486b830035064f23ec87bcd70795d1
> branch: schc

And I don't know why.

-- 
Tom

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

Re: [PATCH 07/17] bootstd: Fully complete iteration of a uclass

[Tom Rini]

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

On Wed, Mar 19, 2025 at 03:38:01PM +0100, Simon Glass wrote:

> 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.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>

This seems like a real bugfix and a fixes tag would help make it clear
what it applies to.

-- 
Tom

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

Re: [PATCH 06/17] bootstd: Try all bootmeths on the final partition

[Tom Rini]

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

On Wed, Mar 19, 2025 at 03:38:00PM +0100, Simon Glass wrote:

> 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>
> ---
> 
>  boot/bootflow.c | 34 ++++++++++++++++------------------
>  1 file changed, 16 insertions(+), 18 deletions(-)

Behavior change should include documentation change.

-- 
Tom

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

Re: [PATCH 01/17] fs: boot: Update fs_read_alloc() to use abuf

[Tom Rini]

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

On Wed, Mar 19, 2025 at 03:37:55PM +0100, Simon Glass wrote:

> 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>
> ---
> 
>  boot/bootmeth-uclass.c | 19 ++++++++++---------
>  fs/fs.c                | 25 +++++++++++--------------
>  include/fs.h           |  8 +++++---
>  3 files changed, 26 insertions(+), 26 deletions(-)

So we grow platforms by ~200 bytes:
            sama7g54_curiosity_nandflash: all +204 text +204
               u-boot: add: 6/0, grow: 2/0 bytes: 204/0 (204)
                 function                                   old     new   delta
                 abuf_realloc                                 -      76     +76
                 abuf_uninit_move                             -      42     +42
                 memdup                                       -      28     +28
                 abuf_uninit                                  -      24     +24
                 fs_read_alloc                               96     106     +10
                 fs_load_alloc                              114     124     +10
                 abuf_init                                    -      10     +10
                 abuf_addr                                    -       4      +4

To move away from standard buffer usage and unwinding to move to
something homegrown instead. I am not a fan of growing using abuf here.
When it was introduced in:

commit 67bc59df05331eaac56cd0a00219d1386130aee2
Author: Simon Glass <sjg@chromium.org>
Date:   Sat Sep 25 07:03:07 2021 -0600

    Add support for an owned buffer

It sounded like something for some special cases. Not something to be
used everywhere to be different.

-- 
Tom

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

Re: [PATCH 01/17] fs: boot: Update fs_read_alloc() to use abuf

[Simon Glass]

Hi Tom,

On Tue, 1 Apr 2025 at 06:42, Tom Rini <trini@konsulko.com> wrote:
>
> On Wed, Mar 19, 2025 at 03:37:55PM +0100, Simon Glass wrote:
>
> > 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>
> > ---
> >
> >  boot/bootmeth-uclass.c | 19 ++++++++++---------
> >  fs/fs.c                | 25 +++++++++++--------------
> >  include/fs.h           |  8 +++++---
> >  3 files changed, 26 insertions(+), 26 deletions(-)
>
> So we grow platforms by ~200 bytes:
>             sama7g54_curiosity_nandflash: all +204 text +204
>                u-boot: add: 6/0, grow: 2/0 bytes: 204/0 (204)
>                  function                                   old     new   delta
>                  abuf_realloc                                 -      76     +76
>                  abuf_uninit_move                             -      42     +42
>                  memdup                                       -      28     +28
>                  abuf_uninit                                  -      24     +24
>                  fs_read_alloc                               96     106     +10
>                  fs_load_alloc                              114     124     +10
>                  abuf_init                                    -      10     +10
>                  abuf_addr                                    -       4      +4
>
> To move away from standard buffer usage and unwinding to move to
> something homegrown instead. I am not a fan of growing using abuf here.
> When it was introduced in:
>
> commit 67bc59df05331eaac56cd0a00219d1386130aee2
> Author: Simon Glass <sjg@chromium.org>
> Date:   Sat Sep 25 07:03:07 2021 -0600
>
>     Add support for an owned buffer
>
> It sounded like something for some special cases. Not something to be
> used everywhere to be different.

From what I can tell this is a one-off hit that I believe is worth
taking at some point. However I haven't seen a code-size reduction
yet, so I can understand your reluctance.

Regards,
Simon

Re: [PATCH 09/17] test/py: Add a test image for Ubuntu

[Simon Glass]

Hi Tom,

On Mon, 31 Mar 2025 at 03:45, Tom Rini <trini@konsulko.com> wrote:
>
> On Thu, Mar 20, 2025 at 03:41:33AM +0000, Simon Glass wrote:
> > Hi Tom,
> >
> > On Wed, 19 Mar 2025 at 16:35, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Wed, Mar 19, 2025 at 03:04:28PM +0000, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Wed, 19 Mar 2025 at 15:52, Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > On Wed, Mar 19, 2025 at 03:38:03PM +0100, Simon Glass wrote:
> > > > >
> > > > > > Add an extlinux image that contains a few Ubuntu entries.
> > > > > >
> > > > > > Increase the number of sandbox-USB-hub ports to permit this.
> > > > > >
> > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > >
> > > > > I don't understand what this test adds. In neither the current Fedora
> > > > > test nor in this new test are we actually booting something, we're just
> > > > > taking a sample extlinux.conf and making sure it doesn't fail. Is it
> > > > > that we're testing in a useful fashion now having two labels?
> > > >
> > > > I didn't think so either, which is why I never did this before. But it
> > > > turns out that there were some bugs, too.
> > >
> > > I don't understand you, sorry. You don't think so either to what?
> >
> > I didn't think we needed two extlinux examples on different boot
> > devices, but we do.
>
> I'm not sure it's particularly clear what you're doing here then, or
> why, but I'll find some time to read it all more deeply.

OK. This test case is how I found the bugs/problems in bootstd that
are fixed in this series.

>
> > > > > We should probably be clear about what we're doing in the tests and
> > > > > instead of adding seemingly arbitrary distributions add an extlinux test
> > > > > and testcases.
> > > >
> > > > This is not actually a test case. It is simply creating a new image.
> > > > The test cases are in the other patches, so please take a look there.
> > >
> > > Nothing in this series quickly reads as adding tests and fixing problems
> > > with extlinux parsing, it's all bootmeth stuff?
> >
> > It isn't about the actual parsing of the .conf file, although I would
> > like to add tests there as we have none apart from what I have added
> > in my PXE series. It's more about having multiple devices with
> > bootable OSes on them. This series tidies up and fixes this. We need
> > to have an image available on more than one device to spot these
> > problems.
>
> And the existing tests for pxelinux that we have in mainline already,
> don't forget those.

Yes. But those tests actually don't use bootstd, do they?

>
> > Currently we have two accessible to sandbox, one extlinux and one
> > EFI*. I decided to add a third, using extlinux.
> >
> > Again, this is not a test case, but provides an image for the test
> > cases in this series.
>
> Adding mocked up things for use in sandbox is adding test cases.

One is a test image for use by tests; the other is a test. Perhaps you
are just saying that there is no point in having one without the
other? Otherwise, I'm not sure what to do with this feedback.

Regards,
Simon

Re: [PATCH 00/17] bootstd: Useability improvements

[Simon Glass]

Hi Tom,

On Tue, 1 Apr 2025 at 06:40, Tom Rini <trini@konsulko.com> wrote:
>
> On Wed, Mar 19, 2025 at 03:37:54PM +0100, Simon Glass wrote:
>
> > 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.
>
> This isn't based on mainline, either:
> > base-commit: 569681e993486b830035064f23ec87bcd70795d1
> > branch: schc
>
> And I don't know why.

I am sending patches in the hope that people can review them and that
some of them may make it into your tree. I'm doing everything I can to
minimise the differences and I would encourage you to do the same.

Regards,
Simon

Re: [PATCH 09/17] test/py: Add a test image for Ubuntu

[Tom Rini]

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

On Wed, Apr 02, 2025 at 04:49:18AM +1300, Simon Glass wrote:
> Hi Tom,
> 
> On Mon, 31 Mar 2025 at 03:45, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Thu, Mar 20, 2025 at 03:41:33AM +0000, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Wed, 19 Mar 2025 at 16:35, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Wed, Mar 19, 2025 at 03:04:28PM +0000, Simon Glass wrote:
> > > > > Hi Tom,
> > > > >
> > > > > On Wed, 19 Mar 2025 at 15:52, Tom Rini <trini@konsulko.com> wrote:
> > > > > >
> > > > > > On Wed, Mar 19, 2025 at 03:38:03PM +0100, Simon Glass wrote:
> > > > > >
> > > > > > > Add an extlinux image that contains a few Ubuntu entries.
> > > > > > >
> > > > > > > Increase the number of sandbox-USB-hub ports to permit this.
> > > > > > >
> > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > >
> > > > > > I don't understand what this test adds. In neither the current Fedora
> > > > > > test nor in this new test are we actually booting something, we're just
> > > > > > taking a sample extlinux.conf and making sure it doesn't fail. Is it
> > > > > > that we're testing in a useful fashion now having two labels?
> > > > >
> > > > > I didn't think so either, which is why I never did this before. But it
> > > > > turns out that there were some bugs, too.
> > > >
> > > > I don't understand you, sorry. You don't think so either to what?
> > >
> > > I didn't think we needed two extlinux examples on different boot
> > > devices, but we do.
> >
> > I'm not sure it's particularly clear what you're doing here then, or
> > why, but I'll find some time to read it all more deeply.
> 
> OK. This test case is how I found the bugs/problems in bootstd that
> are fixed in this series.

It should be (a) better explained and likely (b) instead of dropping in
a seemingly verbatim installed file a specially crafted bit of content.
We aren't testing "Ubuntu". We are testing multiple labels.

> > > > > > We should probably be clear about what we're doing in the tests and
> > > > > > instead of adding seemingly arbitrary distributions add an extlinux test
> > > > > > and testcases.
> > > > >
> > > > > This is not actually a test case. It is simply creating a new image.
> > > > > The test cases are in the other patches, so please take a look there.
> > > >
> > > > Nothing in this series quickly reads as adding tests and fixing problems
> > > > with extlinux parsing, it's all bootmeth stuff?
> > >
> > > It isn't about the actual parsing of the .conf file, although I would
> > > like to add tests there as we have none apart from what I have added
> > > in my PXE series. It's more about having multiple devices with
> > > bootable OSes on them. This series tidies up and fixes this. We need
> > > to have an image available on more than one device to spot these
> > > problems.
> >
> > And the existing tests for pxelinux that we have in mainline already,
> > don't forget those.
> 
> Yes. But those tests actually don't use bootstd, do they?

No, they're testing pxelinux, the thing you said we don't have any tests
for.

> > > Currently we have two accessible to sandbox, one extlinux and one
> > > EFI*. I decided to add a third, using extlinux.
> > >
> > > Again, this is not a test case, but provides an image for the test
> > > cases in this series.
> >
> > Adding mocked up things for use in sandbox is adding test cases.
> 
> One is a test image for use by tests; the other is a test. Perhaps you
> are just saying that there is no point in having one without the
> other? Otherwise, I'm not sure what to do with this feedback.

Maybe I clarified better above now. You're not testing Ubuntu (or Fedora
or Armbian or ...) you're seeing if various pxelinux files parse
correctly. Making the tests be clearer about what's being tested is
what's missing at least in part.

-- 
Tom

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

Re: [PATCH 01/17] fs: boot: Update fs_read_alloc() to use abuf

[Tom Rini]

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

On Wed, Apr 02, 2025 at 04:46:23AM +1300, Simon Glass wrote:
> Hi Tom,
> 
> On Tue, 1 Apr 2025 at 06:42, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Wed, Mar 19, 2025 at 03:37:55PM +0100, Simon Glass wrote:
> >
> > > 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>
> > > ---
> > >
> > >  boot/bootmeth-uclass.c | 19 ++++++++++---------
> > >  fs/fs.c                | 25 +++++++++++--------------
> > >  include/fs.h           |  8 +++++---
> > >  3 files changed, 26 insertions(+), 26 deletions(-)
> >
> > So we grow platforms by ~200 bytes:
> >             sama7g54_curiosity_nandflash: all +204 text +204
> >                u-boot: add: 6/0, grow: 2/0 bytes: 204/0 (204)
> >                  function                                   old     new   delta
> >                  abuf_realloc                                 -      76     +76
> >                  abuf_uninit_move                             -      42     +42
> >                  memdup                                       -      28     +28
> >                  abuf_uninit                                  -      24     +24
> >                  fs_read_alloc                               96     106     +10
> >                  fs_load_alloc                              114     124     +10
> >                  abuf_init                                    -      10     +10
> >                  abuf_addr                                    -       4      +4
> >
> > To move away from standard buffer usage and unwinding to move to
> > something homegrown instead. I am not a fan of growing using abuf here.
> > When it was introduced in:
> >
> > commit 67bc59df05331eaac56cd0a00219d1386130aee2
> > Author: Simon Glass <sjg@chromium.org>
> > Date:   Sat Sep 25 07:03:07 2021 -0600
> >
> >     Add support for an owned buffer
> >
> > It sounded like something for some special cases. Not something to be
> > used everywhere to be different.
> 
> From what I can tell this is a one-off hit that I believe is worth
> taking at some point. However I haven't seen a code-size reduction
> yet, so I can understand your reluctance.

I'm not sure why it's worth taking. I'm not sure that abuf is worth
pushing in to every place we allocate a buffer for strings. If someone
wants to put in effort to make our handling of strings better/safer/more
secure we should look to all of the work Kees Cook (et al) did in the
kernel and borrow that, not invent something new and U-Boot specific.

-- 
Tom

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

Re: [PATCH 00/17] bootstd: Useability improvements

[Tom Rini]

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

On Wed, Apr 02, 2025 at 04:53:32AM +1300, Simon Glass wrote:
> Hi Tom,
> 
> On Tue, 1 Apr 2025 at 06:40, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Wed, Mar 19, 2025 at 03:37:54PM +0100, Simon Glass wrote:
> >
> > > 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.
> >
> > This isn't based on mainline, either:
> > > base-commit: 569681e993486b830035064f23ec87bcd70795d1
> > > branch: schc
> >
> > And I don't know why.
> 
> I am sending patches in the hope that people can review them and that
> some of them may make it into your tree. I'm doing everything I can to
> minimise the differences and I would encourage you to do the same.

I am not merging any of your patches that are not based on master or
next. You need to post patches based on master or next and not your
personal tree. Lots of people have personal trees and you're the only
person who does not follow the normal expected behavior here.

-- 
Tom

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

Re: [PATCH 09/17] test/py: Add a test image for Ubuntu

[Simon Glass]

Hi Tom,

On Wed, 2 Apr 2025 at 05:49, Tom Rini <trini@konsulko.com> wrote:
>
> On Wed, Apr 02, 2025 at 04:49:18AM +1300, Simon Glass wrote:
> > Hi Tom,
> >
> > On Mon, 31 Mar 2025 at 03:45, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Thu, Mar 20, 2025 at 03:41:33AM +0000, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Wed, 19 Mar 2025 at 16:35, Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > On Wed, Mar 19, 2025 at 03:04:28PM +0000, Simon Glass wrote:
> > > > > > Hi Tom,
> > > > > >
> > > > > > On Wed, 19 Mar 2025 at 15:52, Tom Rini <trini@konsulko.com> wrote:
> > > > > > >
> > > > > > > On Wed, Mar 19, 2025 at 03:38:03PM +0100, Simon Glass wrote:
> > > > > > >
> > > > > > > > Add an extlinux image that contains a few Ubuntu entries.
> > > > > > > >
> > > > > > > > Increase the number of sandbox-USB-hub ports to permit this.
> > > > > > > >
> > > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > >
> > > > > > > I don't understand what this test adds. In neither the current Fedora
> > > > > > > test nor in this new test are we actually booting something, we're just
> > > > > > > taking a sample extlinux.conf and making sure it doesn't fail. Is it
> > > > > > > that we're testing in a useful fashion now having two labels?
> > > > > >
> > > > > > I didn't think so either, which is why I never did this before. But it
> > > > > > turns out that there were some bugs, too.
> > > > >
> > > > > I don't understand you, sorry. You don't think so either to what?
> > > >
> > > > I didn't think we needed two extlinux examples on different boot
> > > > devices, but we do.
> > >
> > > I'm not sure it's particularly clear what you're doing here then, or
> > > why, but I'll find some time to read it all more deeply.
> >
> > OK. This test case is how I found the bugs/problems in bootstd that
> > are fixed in this series.
>
> It should be (a) better explained and likely (b) instead of dropping in
> a seemingly verbatim installed file a specially crafted bit of content.
> We aren't testing "Ubuntu". We are testing multiple labels.
>
> > > > > > > We should probably be clear about what we're doing in the tests and
> > > > > > > instead of adding seemingly arbitrary distributions add an extlinux test
> > > > > > > and testcases.
> > > > > >
> > > > > > This is not actually a test case. It is simply creating a new image.
> > > > > > The test cases are in the other patches, so please take a look there.
> > > > >
> > > > > Nothing in this series quickly reads as adding tests and fixing problems
> > > > > with extlinux parsing, it's all bootmeth stuff?
> > > >
> > > > It isn't about the actual parsing of the .conf file, although I would
> > > > like to add tests there as we have none apart from what I have added
> > > > in my PXE series. It's more about having multiple devices with
> > > > bootable OSes on them. This series tidies up and fixes this. We need
> > > > to have an image available on more than one device to spot these
> > > > problems.
> > >
> > > And the existing tests for pxelinux that we have in mainline already,
> > > don't forget those.
> >
> > Yes. But those tests actually don't use bootstd, do they?
>
> No, they're testing pxelinux, the thing you said we don't have any tests
> for.
>
> > > > Currently we have two accessible to sandbox, one extlinux and one
> > > > EFI*. I decided to add a third, using extlinux.
> > > >
> > > > Again, this is not a test case, but provides an image for the test
> > > > cases in this series.
> > >
> > > Adding mocked up things for use in sandbox is adding test cases.
> >
> > One is a test image for use by tests; the other is a test. Perhaps you
> > are just saying that there is no point in having one without the
> > other? Otherwise, I'm not sure what to do with this feedback.
>
> Maybe I clarified better above now. You're not testing Ubuntu (or Fedora
> or Armbian or ...) you're seeing if various pxelinux files parse
> correctly. Making the tests be clearer about what's being tested is
> what's missing at least in part.

You want me to remove the word 'Ubuntu' from the test files? This is
what I get when I install u-boot-tools so I am trying to use that,
rather than invent my own thing. This is the same approach I've taken
with Fedora and Armbian and I think it makes the most sense.

Regards,
Simon

Re: [PATCH 09/17] test/py: Add a test image for Ubuntu

[Tom Rini]

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

On Wed, Apr 02, 2025 at 06:55:02AM +1300, Simon Glass wrote:
> Hi Tom,
> 
> On Wed, 2 Apr 2025 at 05:49, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Wed, Apr 02, 2025 at 04:49:18AM +1300, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Mon, 31 Mar 2025 at 03:45, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Thu, Mar 20, 2025 at 03:41:33AM +0000, Simon Glass wrote:
> > > > > Hi Tom,
> > > > >
> > > > > On Wed, 19 Mar 2025 at 16:35, Tom Rini <trini@konsulko.com> wrote:
> > > > > >
> > > > > > On Wed, Mar 19, 2025 at 03:04:28PM +0000, Simon Glass wrote:
> > > > > > > Hi Tom,
> > > > > > >
> > > > > > > On Wed, 19 Mar 2025 at 15:52, Tom Rini <trini@konsulko.com> wrote:
> > > > > > > >
> > > > > > > > On Wed, Mar 19, 2025 at 03:38:03PM +0100, Simon Glass wrote:
> > > > > > > >
> > > > > > > > > Add an extlinux image that contains a few Ubuntu entries.
> > > > > > > > >
> > > > > > > > > Increase the number of sandbox-USB-hub ports to permit this.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > > >
> > > > > > > > I don't understand what this test adds. In neither the current Fedora
> > > > > > > > test nor in this new test are we actually booting something, we're just
> > > > > > > > taking a sample extlinux.conf and making sure it doesn't fail. Is it
> > > > > > > > that we're testing in a useful fashion now having two labels?
> > > > > > >
> > > > > > > I didn't think so either, which is why I never did this before. But it
> > > > > > > turns out that there were some bugs, too.
> > > > > >
> > > > > > I don't understand you, sorry. You don't think so either to what?
> > > > >
> > > > > I didn't think we needed two extlinux examples on different boot
> > > > > devices, but we do.
> > > >
> > > > I'm not sure it's particularly clear what you're doing here then, or
> > > > why, but I'll find some time to read it all more deeply.
> > >
> > > OK. This test case is how I found the bugs/problems in bootstd that
> > > are fixed in this series.
> >
> > It should be (a) better explained and likely (b) instead of dropping in
> > a seemingly verbatim installed file a specially crafted bit of content.
> > We aren't testing "Ubuntu". We are testing multiple labels.
> >
> > > > > > > > We should probably be clear about what we're doing in the tests and
> > > > > > > > instead of adding seemingly arbitrary distributions add an extlinux test
> > > > > > > > and testcases.
> > > > > > >
> > > > > > > This is not actually a test case. It is simply creating a new image.
> > > > > > > The test cases are in the other patches, so please take a look there.
> > > > > >
> > > > > > Nothing in this series quickly reads as adding tests and fixing problems
> > > > > > with extlinux parsing, it's all bootmeth stuff?
> > > > >
> > > > > It isn't about the actual parsing of the .conf file, although I would
> > > > > like to add tests there as we have none apart from what I have added
> > > > > in my PXE series. It's more about having multiple devices with
> > > > > bootable OSes on them. This series tidies up and fixes this. We need
> > > > > to have an image available on more than one device to spot these
> > > > > problems.
> > > >
> > > > And the existing tests for pxelinux that we have in mainline already,
> > > > don't forget those.
> > >
> > > Yes. But those tests actually don't use bootstd, do they?
> >
> > No, they're testing pxelinux, the thing you said we don't have any tests
> > for.
> >
> > > > > Currently we have two accessible to sandbox, one extlinux and one
> > > > > EFI*. I decided to add a third, using extlinux.
> > > > >
> > > > > Again, this is not a test case, but provides an image for the test
> > > > > cases in this series.
> > > >
> > > > Adding mocked up things for use in sandbox is adding test cases.
> > >
> > > One is a test image for use by tests; the other is a test. Perhaps you
> > > are just saying that there is no point in having one without the
> > > other? Otherwise, I'm not sure what to do with this feedback.
> >
> > Maybe I clarified better above now. You're not testing Ubuntu (or Fedora
> > or Armbian or ...) you're seeing if various pxelinux files parse
> > correctly. Making the tests be clearer about what's being tested is
> > what's missing at least in part.
> 
> You want me to remove the word 'Ubuntu' from the test files? This is
> what I get when I install u-boot-tools so I am trying to use that,
> rather than invent my own thing. This is the same approach I've taken
> with Fedora and Armbian and I think it makes the most sense.

I seem to be unable to explain that if you're constructing tests, I'd
like to see it clear that you're constructing test files. You're not
testing "Ubuntu" or "Fedora" or "Armbian" here. I'm not nak'ing this
patch as at the end of the day, it's adding some test, even if it's also
I believe being done in confusing ways.

-- 
Tom

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

Re: [PATCH 09/17] test/py: Add a test image for Ubuntu

[Simon Glass]

Hi Tom,

On Fri, 11 Apr 2025 at 13:13, Tom Rini <trini@konsulko.com> wrote:
>
> On Wed, Apr 02, 2025 at 06:55:02AM +1300, Simon Glass wrote:
> > Hi Tom,
> >
> > On Wed, 2 Apr 2025 at 05:49, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Wed, Apr 02, 2025 at 04:49:18AM +1300, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Mon, 31 Mar 2025 at 03:45, Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > On Thu, Mar 20, 2025 at 03:41:33AM +0000, Simon Glass wrote:
> > > > > > Hi Tom,
> > > > > >
> > > > > > On Wed, 19 Mar 2025 at 16:35, Tom Rini <trini@konsulko.com> wrote:
> > > > > > >
> > > > > > > On Wed, Mar 19, 2025 at 03:04:28PM +0000, Simon Glass wrote:
> > > > > > > > Hi Tom,
> > > > > > > >
> > > > > > > > On Wed, 19 Mar 2025 at 15:52, Tom Rini <trini@konsulko.com> wrote:
> > > > > > > > >
> > > > > > > > > On Wed, Mar 19, 2025 at 03:38:03PM +0100, Simon Glass wrote:
> > > > > > > > >
> > > > > > > > > > Add an extlinux image that contains a few Ubuntu entries.
> > > > > > > > > >
> > > > > > > > > > Increase the number of sandbox-USB-hub ports to permit this.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > > > >
> > > > > > > > > I don't understand what this test adds. In neither the current Fedora
> > > > > > > > > test nor in this new test are we actually booting something, we're just
> > > > > > > > > taking a sample extlinux.conf and making sure it doesn't fail. Is it
> > > > > > > > > that we're testing in a useful fashion now having two labels?
> > > > > > > >
> > > > > > > > I didn't think so either, which is why I never did this before. But it
> > > > > > > > turns out that there were some bugs, too.
> > > > > > >
> > > > > > > I don't understand you, sorry. You don't think so either to what?
> > > > > >
> > > > > > I didn't think we needed two extlinux examples on different boot
> > > > > > devices, but we do.
> > > > >
> > > > > I'm not sure it's particularly clear what you're doing here then, or
> > > > > why, but I'll find some time to read it all more deeply.
> > > >
> > > > OK. This test case is how I found the bugs/problems in bootstd that
> > > > are fixed in this series.
> > >
> > > It should be (a) better explained and likely (b) instead of dropping in
> > > a seemingly verbatim installed file a specially crafted bit of content.
> > > We aren't testing "Ubuntu". We are testing multiple labels.
> > >
> > > > > > > > > We should probably be clear about what we're doing in the tests and
> > > > > > > > > instead of adding seemingly arbitrary distributions add an extlinux test
> > > > > > > > > and testcases.
> > > > > > > >
> > > > > > > > This is not actually a test case. It is simply creating a new image.
> > > > > > > > The test cases are in the other patches, so please take a look there.
> > > > > > >
> > > > > > > Nothing in this series quickly reads as adding tests and fixing problems
> > > > > > > with extlinux parsing, it's all bootmeth stuff?
> > > > > >
> > > > > > It isn't about the actual parsing of the .conf file, although I would
> > > > > > like to add tests there as we have none apart from what I have added
> > > > > > in my PXE series. It's more about having multiple devices with
> > > > > > bootable OSes on them. This series tidies up and fixes this. We need
> > > > > > to have an image available on more than one device to spot these
> > > > > > problems.
> > > > >
> > > > > And the existing tests for pxelinux that we have in mainline already,
> > > > > don't forget those.
> > > >
> > > > Yes. But those tests actually don't use bootstd, do they?
> > >
> > > No, they're testing pxelinux, the thing you said we don't have any tests
> > > for.
> > >
> > > > > > Currently we have two accessible to sandbox, one extlinux and one
> > > > > > EFI*. I decided to add a third, using extlinux.
> > > > > >
> > > > > > Again, this is not a test case, but provides an image for the test
> > > > > > cases in this series.
> > > > >
> > > > > Adding mocked up things for use in sandbox is adding test cases.
> > > >
> > > > One is a test image for use by tests; the other is a test. Perhaps you
> > > > are just saying that there is no point in having one without the
> > > > other? Otherwise, I'm not sure what to do with this feedback.
> > >
> > > Maybe I clarified better above now. You're not testing Ubuntu (or Fedora
> > > or Armbian or ...) you're seeing if various pxelinux files parse
> > > correctly. Making the tests be clearer about what's being tested is
> > > what's missing at least in part.
> >
> > You want me to remove the word 'Ubuntu' from the test files? This is
> > what I get when I install u-boot-tools so I am trying to use that,
> > rather than invent my own thing. This is the same approach I've taken
> > with Fedora and Armbian and I think it makes the most sense.
>
> I seem to be unable to explain that if you're constructing tests, I'd
> like to see it clear that you're constructing test files. You're not
> testing "Ubuntu" or "Fedora" or "Armbian" here. I'm not nak'ing this
> patch as at the end of the day, it's adding some test, even if it's also
> I believe being done in confusing ways.

I actually do understand what you are saying. Indeed I am not testing
the OS booting, just that we can parse the files that they use to
describe the OS. I think it is better to use real files rather than
invent things that are not used in the wild.

Perhaps the commit subject is bad. 'Add an extlinux file as used by Ubuntu' ?

Regards,
Simon

Re: [PATCH 09/17] test/py: Add a test image for Ubuntu

[Tom Rini]

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

On Sat, Apr 12, 2025 at 06:45:02AM -0600, Simon Glass wrote:
> Hi Tom,
> 
> On Fri, 11 Apr 2025 at 13:13, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Wed, Apr 02, 2025 at 06:55:02AM +1300, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Wed, 2 Apr 2025 at 05:49, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Wed, Apr 02, 2025 at 04:49:18AM +1300, Simon Glass wrote:
> > > > > Hi Tom,
> > > > >
> > > > > On Mon, 31 Mar 2025 at 03:45, Tom Rini <trini@konsulko.com> wrote:
> > > > > >
> > > > > > On Thu, Mar 20, 2025 at 03:41:33AM +0000, Simon Glass wrote:
> > > > > > > Hi Tom,
> > > > > > >
> > > > > > > On Wed, 19 Mar 2025 at 16:35, Tom Rini <trini@konsulko.com> wrote:
> > > > > > > >
> > > > > > > > On Wed, Mar 19, 2025 at 03:04:28PM +0000, Simon Glass wrote:
> > > > > > > > > Hi Tom,
> > > > > > > > >
> > > > > > > > > On Wed, 19 Mar 2025 at 15:52, Tom Rini <trini@konsulko.com> wrote:
> > > > > > > > > >
> > > > > > > > > > On Wed, Mar 19, 2025 at 03:38:03PM +0100, Simon Glass wrote:
> > > > > > > > > >
> > > > > > > > > > > Add an extlinux image that contains a few Ubuntu entries.
> > > > > > > > > > >
> > > > > > > > > > > Increase the number of sandbox-USB-hub ports to permit this.
> > > > > > > > > > >
> > > > > > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > > > > >
> > > > > > > > > > I don't understand what this test adds. In neither the current Fedora
> > > > > > > > > > test nor in this new test are we actually booting something, we're just
> > > > > > > > > > taking a sample extlinux.conf and making sure it doesn't fail. Is it
> > > > > > > > > > that we're testing in a useful fashion now having two labels?
> > > > > > > > >
> > > > > > > > > I didn't think so either, which is why I never did this before. But it
> > > > > > > > > turns out that there were some bugs, too.
> > > > > > > >
> > > > > > > > I don't understand you, sorry. You don't think so either to what?
> > > > > > >
> > > > > > > I didn't think we needed two extlinux examples on different boot
> > > > > > > devices, but we do.
> > > > > >
> > > > > > I'm not sure it's particularly clear what you're doing here then, or
> > > > > > why, but I'll find some time to read it all more deeply.
> > > > >
> > > > > OK. This test case is how I found the bugs/problems in bootstd that
> > > > > are fixed in this series.
> > > >
> > > > It should be (a) better explained and likely (b) instead of dropping in
> > > > a seemingly verbatim installed file a specially crafted bit of content.
> > > > We aren't testing "Ubuntu". We are testing multiple labels.
> > > >
> > > > > > > > > > We should probably be clear about what we're doing in the tests and
> > > > > > > > > > instead of adding seemingly arbitrary distributions add an extlinux test
> > > > > > > > > > and testcases.
> > > > > > > > >
> > > > > > > > > This is not actually a test case. It is simply creating a new image.
> > > > > > > > > The test cases are in the other patches, so please take a look there.
> > > > > > > >
> > > > > > > > Nothing in this series quickly reads as adding tests and fixing problems
> > > > > > > > with extlinux parsing, it's all bootmeth stuff?
> > > > > > >
> > > > > > > It isn't about the actual parsing of the .conf file, although I would
> > > > > > > like to add tests there as we have none apart from what I have added
> > > > > > > in my PXE series. It's more about having multiple devices with
> > > > > > > bootable OSes on them. This series tidies up and fixes this. We need
> > > > > > > to have an image available on more than one device to spot these
> > > > > > > problems.
> > > > > >
> > > > > > And the existing tests for pxelinux that we have in mainline already,
> > > > > > don't forget those.
> > > > >
> > > > > Yes. But those tests actually don't use bootstd, do they?
> > > >
> > > > No, they're testing pxelinux, the thing you said we don't have any tests
> > > > for.
> > > >
> > > > > > > Currently we have two accessible to sandbox, one extlinux and one
> > > > > > > EFI*. I decided to add a third, using extlinux.
> > > > > > >
> > > > > > > Again, this is not a test case, but provides an image for the test
> > > > > > > cases in this series.
> > > > > >
> > > > > > Adding mocked up things for use in sandbox is adding test cases.
> > > > >
> > > > > One is a test image for use by tests; the other is a test. Perhaps you
> > > > > are just saying that there is no point in having one without the
> > > > > other? Otherwise, I'm not sure what to do with this feedback.
> > > >
> > > > Maybe I clarified better above now. You're not testing Ubuntu (or Fedora
> > > > or Armbian or ...) you're seeing if various pxelinux files parse
> > > > correctly. Making the tests be clearer about what's being tested is
> > > > what's missing at least in part.
> > >
> > > You want me to remove the word 'Ubuntu' from the test files? This is
> > > what I get when I install u-boot-tools so I am trying to use that,
> > > rather than invent my own thing. This is the same approach I've taken
> > > with Fedora and Armbian and I think it makes the most sense.
> >
> > I seem to be unable to explain that if you're constructing tests, I'd
> > like to see it clear that you're constructing test files. You're not
> > testing "Ubuntu" or "Fedora" or "Armbian" here. I'm not nak'ing this
> > patch as at the end of the day, it's adding some test, even if it's also
> > I believe being done in confusing ways.
> 
> I actually do understand what you are saying. Indeed I am not testing
> the OS booting, just that we can parse the files that they use to
> describe the OS. I think it is better to use real files rather than
> invent things that are not used in the wild.
> 
> Perhaps the commit subject is bad. 'Add an extlinux file as used by Ubuntu' ?

However you would like to expand the subject / body to clarify further
is fine, thanks.

-- 
Tom

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

Re: [PATCH 01/17] fs: boot: Update fs_read_alloc() to use abuf

[Simon Glass]

Hi Tom,

On Tue, 1 Apr 2025 at 10:50, Tom Rini <trini@konsulko.com> wrote:
>
> On Wed, Apr 02, 2025 at 04:46:23AM +1300, Simon Glass wrote:
> > Hi Tom,
> >
> > On Tue, 1 Apr 2025 at 06:42, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Wed, Mar 19, 2025 at 03:37:55PM +0100, Simon Glass wrote:
> > >
> > > > 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>
> > > > ---
> > > >
> > > >  boot/bootmeth-uclass.c | 19 ++++++++++---------
> > > >  fs/fs.c                | 25 +++++++++++--------------
> > > >  include/fs.h           |  8 +++++---
> > > >  3 files changed, 26 insertions(+), 26 deletions(-)
> > >
> > > So we grow platforms by ~200 bytes:
> > >             sama7g54_curiosity_nandflash: all +204 text +204
> > >                u-boot: add: 6/0, grow: 2/0 bytes: 204/0 (204)
> > >                  function                                   old     new   delta
> > >                  abuf_realloc                                 -      76     +76
> > >                  abuf_uninit_move                             -      42     +42
> > >                  memdup                                       -      28     +28
> > >                  abuf_uninit                                  -      24     +24
> > >                  fs_read_alloc                               96     106     +10
> > >                  fs_load_alloc                              114     124     +10
> > >                  abuf_init                                    -      10     +10
> > >                  abuf_addr                                    -       4      +4
> > >
> > > To move away from standard buffer usage and unwinding to move to
> > > something homegrown instead. I am not a fan of growing using abuf here.
> > > When it was introduced in:
> > >
> > > commit 67bc59df05331eaac56cd0a00219d1386130aee2
> > > Author: Simon Glass <sjg@chromium.org>
> > > Date:   Sat Sep 25 07:03:07 2021 -0600
> > >
> > >     Add support for an owned buffer
> > >
> > > It sounded like something for some special cases. Not something to be
> > > used everywhere to be different.
> >
> > From what I can tell this is a one-off hit that I believe is worth
> > taking at some point. However I haven't seen a code-size reduction
> > yet, so I can understand your reluctance.
>
> I'm not sure why it's worth taking. I'm not sure that abuf is worth
> pushing in to every place we allocate a buffer for strings. If someone
> wants to put in effort to make our handling of strings better/safer/more
> secure we should look to all of the work Kees Cook (et al) did in the
> kernel and borrow that, not invent something new and U-Boot specific.

This isn't about strings; it's about buffers. A buffer which can be
allocated or not, which can be resized and can be passed around
without worrying about pointers and sizes and so on. It also allows
one less parameter to be passed, although of course it is a pointer to
a struct, so it doesn't always make the code smaller than passing two
parameters.

I've had another look through this and I believe this is a step
forward in terms of code safety, if nothing else. I've also found a
way to reduce the impact on your sample board to 104 bytes. This is
largely a one-off cost, so move features converting to abuf will
benefit from this without further code-size impact.

Regards,
Simon