[PATCH v4 0/4] boot: Precursor series for global bootmeths

[PATCH v4 0/4] boot: Precursor series for global bootmeths

[Simon Glass]

This series contains a few patches pulled out from the global-bootmeth
series. No functional change is intended.

I left out this one:

   boot: Move showing of bootflows out of the command

since Tom has already said he doesn't want it.

Changes in v4:
- Series pulled out from previous glob-us3 series

Changes in v3:
- Drop the spacing changes

Simon Glass (4):
  boot: Improve comments related to global bootmeths
  boot: Move obtaining the label into a common file
  boot: Add more debugging to iter_incr()
  boot: Move preparing bootdev into a function

 boot/bootflow.c        | 83 ++++++++++++++++++++++++++++++++++--------
 boot/bootmeth-uclass.c |  1 +
 cmd/bootflow.c         |  8 ++--
 include/bootflow.h     | 11 +++++-
 test/boot/bootflow.c   |  4 +-
 5 files changed, 86 insertions(+), 21 deletions(-)

-- 
2.43.0

base-commit: 0ee639ff5af33342c6c2f4579e210d707abc9bc2
branch: glob-us4

[PATCH v4 1/4] boot: Improve comments related to global bootmeths

[Simon Glass]

Add a few comments about global bootmeths and first_glob_method

Signed-off-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---

(no changes since v3)

Changes in v3:
- Drop the spacing changes

 boot/bootflow.c        | 1 +
 boot/bootmeth-uclass.c | 1 +
 include/bootflow.h     | 3 ++-
 test/boot/bootflow.c   | 2 +-
 4 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/boot/bootflow.c b/boot/bootflow.c
index d79f303486d..15df7069209 100644
--- a/boot/bootflow.c
+++ b/boot/bootflow.c
@@ -344,6 +344,7 @@ static int bootflow_check(struct bootflow_iter *iter, struct bootflow *bflow)
 	struct udevice *dev;
 	int ret;
 
+	/* handle global bootmeths if needed */
 	if (IS_ENABLED(CONFIG_BOOTMETH_GLOBAL) && iter->doing_global) {
 		bootflow_iter_set_dev(iter, NULL, 0);
 		ret = bootmeth_get_bootflow(iter->method, bflow);
diff --git a/boot/bootmeth-uclass.c b/boot/bootmeth-uclass.c
index 188f6ea1895..bb2dd8447cf 100644
--- a/boot/bootmeth-uclass.c
+++ b/boot/bootmeth-uclass.c
@@ -204,6 +204,7 @@ int bootmeth_setup_iter_order(struct bootflow_iter *iter, bool include_global)
 		goto err_order;
 	}
 
+	/* start with the global bootmeths */
 	if (IS_ENABLED(CONFIG_BOOTMETH_GLOBAL) && include_global &&
 	    iter->first_glob_method != -1 && iter->first_glob_method != count) {
 		iter->cur_method = iter->first_glob_method;
diff --git a/include/bootflow.h b/include/bootflow.h
index 32422067723..2ef6eb25cf5 100644
--- a/include/bootflow.h
+++ b/include/bootflow.h
@@ -250,7 +250,8 @@ enum bootflow_meth_flags_t {
  * @cur_label: Current label being processed
  * @num_methods: Number of bootmeth devices in @method_order
  * @cur_method: Current method number, an index into @method_order
- * @first_glob_method: First global method, if any, else -1
+ * @first_glob_method: Index of first global method within @method_order[], if
+ * any, else -1
  * @cur_prio: Current priority being scanned
  * @method_order: List of bootmeth devices to use, in order. The normal methods
  *	appear first, then the global ones, if any
diff --git a/test/boot/bootflow.c b/test/boot/bootflow.c
index 8de5a310add..7cd83dc7443 100644
--- a/test/boot/bootflow.c
+++ b/test/boot/bootflow.c
@@ -401,7 +401,7 @@ static int bootflow_system(struct unit_test_state *uts)
 	ut_assertok(device_probe(dev));
 	sandbox_set_fake_efi_mgr_dev(dev, true);
 
-	/* We should get a single 'bootmgr' method right at the end */
+	/* We should get a single 'bootmgr' method at the start */
 	bootstd_clear_glob();
 	ut_assertok(run_command("bootflow scan -lH", 0));
 	ut_assert_skip_to_line(
-- 
2.43.0

Re: [PATCH v4 1/4] boot: Improve comments related to global bootmeths

[Mattijs Korpershoek]

Hi Simon,

Thank you for the patch.

On Thu, Oct 09, 2025 at 03:29, Simon Glass <sjg@chromium.org> wrote:

> Add a few comments about global bootmeths and first_glob_method
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

Reviewed-by: Mattijs Korpershoek <mkorpershoek@kernel.org>

> ---
>
> (no changes since v3)
>
> Changes in v3:
> - Drop the spacing changes
>
>  boot/bootflow.c        | 1 +
>  boot/bootmeth-uclass.c | 1 +
>  include/bootflow.h     | 3 ++-
>  test/boot/bootflow.c   | 2 +-
>  4 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/boot/bootflow.c b/boot/bootflow.c
> index d79f303486d..15df7069209 100644
> --- a/boot/bootflow.c
> +++ b/boot/bootflow.c
> @@ -344,6 +344,7 @@ static int bootflow_check(struct bootflow_iter *iter, struct bootflow *bflow)
>  	struct udevice *dev;
>  	int ret;
>  
> +	/* handle global bootmeths if needed */
>  	if (IS_ENABLED(CONFIG_BOOTMETH_GLOBAL) && iter->doing_global) {
>  		bootflow_iter_set_dev(iter, NULL, 0);
>  		ret = bootmeth_get_bootflow(iter->method, bflow);
> diff --git a/boot/bootmeth-uclass.c b/boot/bootmeth-uclass.c
> index 188f6ea1895..bb2dd8447cf 100644
> --- a/boot/bootmeth-uclass.c
> +++ b/boot/bootmeth-uclass.c
> @@ -204,6 +204,7 @@ int bootmeth_setup_iter_order(struct bootflow_iter *iter, bool include_global)
>  		goto err_order;
>  	}
>  
> +	/* start with the global bootmeths */
>  	if (IS_ENABLED(CONFIG_BOOTMETH_GLOBAL) && include_global &&
>  	    iter->first_glob_method != -1 && iter->first_glob_method != count) {
>  		iter->cur_method = iter->first_glob_method;
> diff --git a/include/bootflow.h b/include/bootflow.h
> index 32422067723..2ef6eb25cf5 100644
> --- a/include/bootflow.h
> +++ b/include/bootflow.h
> @@ -250,7 +250,8 @@ enum bootflow_meth_flags_t {
>   * @cur_label: Current label being processed
>   * @num_methods: Number of bootmeth devices in @method_order
>   * @cur_method: Current method number, an index into @method_order
> - * @first_glob_method: First global method, if any, else -1
> + * @first_glob_method: Index of first global method within @method_order[], if
> + * any, else -1
>   * @cur_prio: Current priority being scanned
>   * @method_order: List of bootmeth devices to use, in order. The normal methods
>   *	appear first, then the global ones, if any
> diff --git a/test/boot/bootflow.c b/test/boot/bootflow.c
> index 8de5a310add..7cd83dc7443 100644
> --- a/test/boot/bootflow.c
> +++ b/test/boot/bootflow.c
> @@ -401,7 +401,7 @@ static int bootflow_system(struct unit_test_state *uts)
>  	ut_assertok(device_probe(dev));
>  	sandbox_set_fake_efi_mgr_dev(dev, true);
>  
> -	/* We should get a single 'bootmgr' method right at the end */
> +	/* We should get a single 'bootmgr' method at the start */
>  	bootstd_clear_glob();
>  	ut_assertok(run_command("bootflow scan -lH", 0));
>  	ut_assert_skip_to_line(
> -- 
> 2.43.0

Re: [PATCH v4 1/4] boot: Improve comments related to global bootmeths

[Sam Protsenko]

On Thu, Oct 9, 2025 at 4:30 AM Simon Glass <sjg@chromium.org> wrote:
>
> Add a few comments about global bootmeths and first_glob_method
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---

Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>

>
> (no changes since v3)
>
> Changes in v3:
> - Drop the spacing changes
>
>  boot/bootflow.c        | 1 +
>  boot/bootmeth-uclass.c | 1 +
>  include/bootflow.h     | 3 ++-
>  test/boot/bootflow.c   | 2 +-
>  4 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/boot/bootflow.c b/boot/bootflow.c
> index d79f303486d..15df7069209 100644
> --- a/boot/bootflow.c
> +++ b/boot/bootflow.c
> @@ -344,6 +344,7 @@ static int bootflow_check(struct bootflow_iter *iter, struct bootflow *bflow)
>         struct udevice *dev;
>         int ret;
>
> +       /* handle global bootmeths if needed */
>         if (IS_ENABLED(CONFIG_BOOTMETH_GLOBAL) && iter->doing_global) {
>                 bootflow_iter_set_dev(iter, NULL, 0);
>                 ret = bootmeth_get_bootflow(iter->method, bflow);
> diff --git a/boot/bootmeth-uclass.c b/boot/bootmeth-uclass.c
> index 188f6ea1895..bb2dd8447cf 100644
> --- a/boot/bootmeth-uclass.c
> +++ b/boot/bootmeth-uclass.c
> @@ -204,6 +204,7 @@ int bootmeth_setup_iter_order(struct bootflow_iter *iter, bool include_global)
>                 goto err_order;
>         }
>
> +       /* start with the global bootmeths */
>         if (IS_ENABLED(CONFIG_BOOTMETH_GLOBAL) && include_global &&
>             iter->first_glob_method != -1 && iter->first_glob_method != count) {
>                 iter->cur_method = iter->first_glob_method;
> diff --git a/include/bootflow.h b/include/bootflow.h
> index 32422067723..2ef6eb25cf5 100644
> --- a/include/bootflow.h
> +++ b/include/bootflow.h
> @@ -250,7 +250,8 @@ enum bootflow_meth_flags_t {
>   * @cur_label: Current label being processed
>   * @num_methods: Number of bootmeth devices in @method_order
>   * @cur_method: Current method number, an index into @method_order
> - * @first_glob_method: First global method, if any, else -1
> + * @first_glob_method: Index of first global method within @method_order[], if
> + * any, else -1
>   * @cur_prio: Current priority being scanned
>   * @method_order: List of bootmeth devices to use, in order. The normal methods
>   *     appear first, then the global ones, if any
> diff --git a/test/boot/bootflow.c b/test/boot/bootflow.c
> index 8de5a310add..7cd83dc7443 100644
> --- a/test/boot/bootflow.c
> +++ b/test/boot/bootflow.c
> @@ -401,7 +401,7 @@ static int bootflow_system(struct unit_test_state *uts)
>         ut_assertok(device_probe(dev));
>         sandbox_set_fake_efi_mgr_dev(dev, true);
>
> -       /* We should get a single 'bootmgr' method right at the end */
> +       /* We should get a single 'bootmgr' method at the start */
>         bootstd_clear_glob();
>         ut_assertok(run_command("bootflow scan -lH", 0));
>         ut_assert_skip_to_line(
> --
> 2.43.0
>

[PATCH v4 2/4] boot: Move obtaining the label into a common file

[Simon Glass]

The 'bootflow list' command supports looking at the EFI device-path when
available. Move this piece into a common function so it can be used
elsewhere.

Use 'usb' instead of 'usb_mass_storage' for usb so that it fits in the
column space.

This updates the output from 'bootflow list'.

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

(no changes since v1)

 boot/bootflow.c      | 19 +++++++++++++++++++
 cmd/bootflow.c       |  8 +++++---
 include/bootflow.h   |  8 ++++++++
 test/boot/bootflow.c |  2 +-
 4 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/boot/bootflow.c b/boot/bootflow.c
index 15df7069209..78df09f369d 100644
--- a/boot/bootflow.c
+++ b/boot/bootflow.c
@@ -11,6 +11,7 @@
 #include <bootmeth.h>
 #include <bootstd.h>
 #include <dm.h>
+#include <efi_device_path.h>
 #include <env_internal.h>
 #include <malloc.h>
 #include <serial.h>
@@ -1010,3 +1011,21 @@ int bootflow_get_seq(const struct bootflow *bflow)
 
 	return alist_calc_index(&std->bootflows, bflow);
 }
+
+const char *bootflow_guess_label(const struct bootflow *bflow)
+{
+	const char *name = NULL;
+
+	if (bflow->dev) {
+		struct udevice *media = dev_get_parent(bflow->dev);
+
+		if (device_get_uclass_id(media) == UCLASS_MASS_STORAGE)
+			name = "usb";
+		else
+			name = dev_get_uclass_name(media);
+	}
+	if (!name)
+		name = "(none)";
+
+	return name;
+}
diff --git a/cmd/bootflow.c b/cmd/bootflow.c
index 551dffbb8b8..5b803d4ace5 100644
--- a/cmd/bootflow.c
+++ b/cmd/bootflow.c
@@ -70,10 +70,12 @@ static void report_bootflow_err(struct bootflow *bflow, int err)
  */
 static void show_bootflow(int index, struct bootflow *bflow, bool errors)
 {
+	const char *name = bootflow_guess_label(bflow);
+
 	printf("%3x  %-11s  %-6s  %-9.9s %4x  %-25.25s %s\n", index,
-	       bflow->method->name, bootflow_state_get_name(bflow->state),
-	       bflow->dev ? dev_get_uclass_name(dev_get_parent(bflow->dev)) :
-	       "(none)", bflow->part, bflow->name, bflow->fname ?: "");
+	       bflow->method ? bflow->method->name : "(none)",
+	       bootflow_state_get_name(bflow->state), name, bflow->part,
+	       bflow->name, bflow->fname ?: "");
 	if (errors)
 		report_bootflow_err(bflow, bflow->err);
 }
diff --git a/include/bootflow.h b/include/bootflow.h
index 2ef6eb25cf5..657e3731f11 100644
--- a/include/bootflow.h
+++ b/include/bootflow.h
@@ -692,4 +692,12 @@ int bootflow_menu_start(struct bootstd_priv *std, bool text_mode,
  */
 int bootflow_menu_poll(struct expo *exp, int *seqp);
 
+/**
+ * bootflow_guess_label() - Produce a plausible label for a bootflow
+ *
+ * This uses the uclass name or EFI device-path to come up with a useful label
+ * for display to the user. Ideally it will say "mmc", "usb", nvme", etc.
+ */
+const char *bootflow_guess_label(const struct bootflow *bflow);
+
 #endif
diff --git a/test/boot/bootflow.c b/test/boot/bootflow.c
index 7cd83dc7443..73fe3d34d0f 100644
--- a/test/boot/bootflow.c
+++ b/test/boot/bootflow.c
@@ -1301,7 +1301,7 @@ 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          1  usb_mass_storage.lun0.boo /EFI/BOOT/BOOTSBOX.EFI");
 	ut_assert_nextlinen("---");
 	ut_assert_skip_to_line("(2 bootflows, 2 valid)");
 	ut_assert_console_end();
-- 
2.43.0

Re: [PATCH v4 2/4] boot: Move obtaining the label into a common file

[Mattijs Korpershoek]

Hi Simon,

Thank you for the patch.

On Thu, Oct 09, 2025 at 03:29, Simon Glass <sjg@chromium.org> wrote:

> The 'bootflow list' command supports looking at the EFI device-path when
> available. Move this piece into a common function so it can be used
> elsewhere.
>
> Use 'usb' instead of 'usb_mass_storage' for usb so that it fits in the
> column space.
>
> This updates the output from 'bootflow list'.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>

Reviewed-by: Mattijs Korpershoek <mkorpershoek@kernel.org>

> ---
>
> (no changes since v1)
>
>  boot/bootflow.c      | 19 +++++++++++++++++++
>  cmd/bootflow.c       |  8 +++++---
>  include/bootflow.h   |  8 ++++++++
>  test/boot/bootflow.c |  2 +-
>  4 files changed, 33 insertions(+), 4 deletions(-)
>
> diff --git a/boot/bootflow.c b/boot/bootflow.c
> index 15df7069209..78df09f369d 100644
> --- a/boot/bootflow.c
> +++ b/boot/bootflow.c
> @@ -11,6 +11,7 @@
>  #include <bootmeth.h>
>  #include <bootstd.h>
>  #include <dm.h>
> +#include <efi_device_path.h>
>  #include <env_internal.h>
>  #include <malloc.h>
>  #include <serial.h>
> @@ -1010,3 +1011,21 @@ int bootflow_get_seq(const struct bootflow *bflow)
>  
>  	return alist_calc_index(&std->bootflows, bflow);
>  }
> +
> +const char *bootflow_guess_label(const struct bootflow *bflow)
> +{
> +	const char *name = NULL;
> +
> +	if (bflow->dev) {
> +		struct udevice *media = dev_get_parent(bflow->dev);
> +
> +		if (device_get_uclass_id(media) == UCLASS_MASS_STORAGE)
> +			name = "usb";
> +		else
> +			name = dev_get_uclass_name(media);
> +	}
> +	if (!name)
> +		name = "(none)";
> +
> +	return name;
> +}
> diff --git a/cmd/bootflow.c b/cmd/bootflow.c
> index 551dffbb8b8..5b803d4ace5 100644
> --- a/cmd/bootflow.c
> +++ b/cmd/bootflow.c
> @@ -70,10 +70,12 @@ static void report_bootflow_err(struct bootflow *bflow, int err)
>   */
>  static void show_bootflow(int index, struct bootflow *bflow, bool errors)
>  {
> +	const char *name = bootflow_guess_label(bflow);
> +
>  	printf("%3x  %-11s  %-6s  %-9.9s %4x  %-25.25s %s\n", index,
> -	       bflow->method->name, bootflow_state_get_name(bflow->state),
> -	       bflow->dev ? dev_get_uclass_name(dev_get_parent(bflow->dev)) :
> -	       "(none)", bflow->part, bflow->name, bflow->fname ?: "");
> +	       bflow->method ? bflow->method->name : "(none)",
> +	       bootflow_state_get_name(bflow->state), name, bflow->part,
> +	       bflow->name, bflow->fname ?: "");
>  	if (errors)
>  		report_bootflow_err(bflow, bflow->err);
>  }
> diff --git a/include/bootflow.h b/include/bootflow.h
> index 2ef6eb25cf5..657e3731f11 100644
> --- a/include/bootflow.h
> +++ b/include/bootflow.h
> @@ -692,4 +692,12 @@ int bootflow_menu_start(struct bootstd_priv *std, bool text_mode,
>   */
>  int bootflow_menu_poll(struct expo *exp, int *seqp);
>  
> +/**
> + * bootflow_guess_label() - Produce a plausible label for a bootflow
> + *
> + * This uses the uclass name or EFI device-path to come up with a useful label
> + * for display to the user. Ideally it will say "mmc", "usb", nvme", etc.
> + */
> +const char *bootflow_guess_label(const struct bootflow *bflow);
> +
>  #endif
> diff --git a/test/boot/bootflow.c b/test/boot/bootflow.c
> index 7cd83dc7443..73fe3d34d0f 100644
> --- a/test/boot/bootflow.c
> +++ b/test/boot/bootflow.c
> @@ -1301,7 +1301,7 @@ 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          1  usb_mass_storage.lun0.boo /EFI/BOOT/BOOTSBOX.EFI");
>  	ut_assert_nextlinen("---");
>  	ut_assert_skip_to_line("(2 bootflows, 2 valid)");
>  	ut_assert_console_end();
> -- 
> 2.43.0

Re: [PATCH v4 2/4] boot: Move obtaining the label into a common file

[Tom Rini]

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

On Thu, Oct 09, 2025 at 03:29:53AM -0600, Simon Glass wrote:

> The 'bootflow list' command supports looking at the EFI device-path when
> available. Move this piece into a common function so it can be used
> elsewhere.

The point of this was to enable the show_bootflow->bootflow_show chnage,
so should be dropped.

> Use 'usb' instead of 'usb_mass_storage' for usb so that it fits in the
> column space.
> 
> This updates the output from 'bootflow list'.

And this is unrelated and should be its own patch. Wikipedia says USB
MSC or UMS are the common short forms for USB Mass Storage devices, and
we should use one of them to have the output be more precise (since we
support USB networking devices for example).

-- 
Tom

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

Re: [PATCH v4 2/4] boot: Move obtaining the label into a common file

[Simon Glass]

Hi Tom,

On Thu, 9 Oct 2025 at 18:24, Tom Rini <trini@konsulko.com> wrote:
>
> On Thu, Oct 09, 2025 at 03:29:53AM -0600, Simon Glass wrote:
>
> > The 'bootflow list' command supports looking at the EFI device-path when
> > available. Move this piece into a common function so it can be used
> > elsewhere.
>
> The point of this was to enable the show_bootflow->bootflow_show chnage,
> so should be dropped.

OK.

>
> > Use 'usb' instead of 'usb_mass_storage' for usb so that it fits in the
> > column space.
> >
> > This updates the output from 'bootflow list'.
>
> And this is unrelated and should be its own patch. Wikipedia says USB
> MSC or UMS are the common short forms for USB Mass Storage devices, and
> we should use one of them to have the output be more precise (since we
> support USB networking devices for example).

Ok I can move it into its own patch.

Are you wanting this to say 'ums' instead of 'usb' ? Will anyone know
what that means?

Re the second point, at least in U-Boot a USB Ethernet device shows as
'ethernet', i.e. the interface doesn't matter. We don't have logic to
look a level deeper, at present.

Regards,
Simon

Re: [PATCH v4 2/4] boot: Move obtaining the label into a common file

[Mattijs Korpershoek]

On Fri, Oct 10, 2025 at 11:35, Simon Glass <sjg@chromium.org> wrote:

> Hi Tom,
>
> On Thu, 9 Oct 2025 at 18:24, Tom Rini <trini@konsulko.com> wrote:
>>
>> On Thu, Oct 09, 2025 at 03:29:53AM -0600, Simon Glass wrote:
>>
>> > The 'bootflow list' command supports looking at the EFI device-path when
>> > available. Move this piece into a common function so it can be used
>> > elsewhere.
>>
>> The point of this was to enable the show_bootflow->bootflow_show chnage,
>> so should be dropped.
>
> OK.
>
>>
>> > Use 'usb' instead of 'usb_mass_storage' for usb so that it fits in the
>> > column space.
>> >
>> > This updates the output from 'bootflow list'.
>>
>> And this is unrelated and should be its own patch. Wikipedia says USB
>> MSC or UMS are the common short forms for USB Mass Storage devices, and
>> we should use one of them to have the output be more precise (since we
>> support USB networking devices for example).
>
> Ok I can move it into its own patch.
>
> Are you wanting this to say 'ums' instead of 'usb' ? Will anyone know
> what that means?

Search for 'ums' gives the following U-Boot documentation:

https://docs.u-boot.org/en/latest/usage/cmd/ums.html#ums-command

Where the acronym is documented.
This is also the acronym used by the cmd (to configure usb gadget as USB
Mass Storage).

So I'd say it's a reasonable acronym

>
> Re the second point, at least in U-Boot a USB Ethernet device shows as
> 'ethernet', i.e. the interface doesn't matter. We don't have logic to
> look a level deeper, at present.
>
> Regards,
> Simon

Re: [PATCH v4 2/4] boot: Move obtaining the label into a common file

[Tom Rini]

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

On Fri, Oct 10, 2025 at 04:11:51PM +0200, Mattijs Korpershoek wrote:
> On Fri, Oct 10, 2025 at 11:35, Simon Glass <sjg@chromium.org> wrote:
> 
> > Hi Tom,
> >
> > On Thu, 9 Oct 2025 at 18:24, Tom Rini <trini@konsulko.com> wrote:
> >>
> >> On Thu, Oct 09, 2025 at 03:29:53AM -0600, Simon Glass wrote:
> >>
> >> > The 'bootflow list' command supports looking at the EFI device-path when
> >> > available. Move this piece into a common function so it can be used
> >> > elsewhere.
> >>
> >> The point of this was to enable the show_bootflow->bootflow_show chnage,
> >> so should be dropped.
> >
> > OK.
> >
> >>
> >> > Use 'usb' instead of 'usb_mass_storage' for usb so that it fits in the
> >> > column space.
> >> >
> >> > This updates the output from 'bootflow list'.
> >>
> >> And this is unrelated and should be its own patch. Wikipedia says USB
> >> MSC or UMS are the common short forms for USB Mass Storage devices, and
> >> we should use one of them to have the output be more precise (since we
> >> support USB networking devices for example).
> >
> > Ok I can move it into its own patch.
> >
> > Are you wanting this to say 'ums' instead of 'usb' ? Will anyone know
> > what that means?
> 
> Search for 'ums' gives the following U-Boot documentation:
> 
> https://docs.u-boot.org/en/latest/usage/cmd/ums.html#ums-command
> 
> Where the acronym is documented.
> This is also the acronym used by the cmd (to configure usb gadget as USB
> Mass Storage).
> 
> So I'd say it's a reasonable acronym

Thanks.

> > Re the second point, at least in U-Boot a USB Ethernet device shows as
> > 'ethernet', i.e. the interface doesn't matter. We don't have logic to
> > look a level deeper, at present.

And this is unfortunate and confusing then because it'd be really good
to know if I'm talking about the USB ethernet I plugged in or the
ethernet port on the board. Made more confusing in turn by the platforms
(more historical than modern I *think*) where that ethernet port on the
SBC itself is wired in via USB.

-- 
Tom

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

Re: [PATCH v4 2/4] boot: Move obtaining the label into a common file

[Simon Glass]

Hi Tom,

On Fri, 10 Oct 2025 at 15:39, Tom Rini <trini@konsulko.com> wrote:
>
> On Fri, Oct 10, 2025 at 04:11:51PM +0200, Mattijs Korpershoek wrote:
> > On Fri, Oct 10, 2025 at 11:35, Simon Glass <sjg@chromium.org> wrote:
> >
> > > Hi Tom,
> > >
> > > On Thu, 9 Oct 2025 at 18:24, Tom Rini <trini@konsulko.com> wrote:
> > >>
> > >> On Thu, Oct 09, 2025 at 03:29:53AM -0600, Simon Glass wrote:
> > >>
> > >> > The 'bootflow list' command supports looking at the EFI device-path when
> > >> > available. Move this piece into a common function so it can be used
> > >> > elsewhere.
> > >>
> > >> The point of this was to enable the show_bootflow->bootflow_show chnage,
> > >> so should be dropped.
> > >
> > > OK.
> > >
> > >>
> > >> > Use 'usb' instead of 'usb_mass_storage' for usb so that it fits in the
> > >> > column space.
> > >> >
> > >> > This updates the output from 'bootflow list'.
> > >>
> > >> And this is unrelated and should be its own patch. Wikipedia says USB
> > >> MSC or UMS are the common short forms for USB Mass Storage devices, and
> > >> we should use one of them to have the output be more precise (since we
> > >> support USB networking devices for example).
> > >
> > > Ok I can move it into its own patch.
> > >
> > > Are you wanting this to say 'ums' instead of 'usb' ? Will anyone know
> > > what that means?
> >
> > Search for 'ums' gives the following U-Boot documentation:
> >
> > https://docs.u-boot.org/en/latest/usage/cmd/ums.html#ums-command
> >
> > Where the acronym is documented.
> > This is also the acronym used by the cmd (to configure usb gadget as USB
> > Mass Storage).
> >
> > So I'd say it's a reasonable acronym
>
> Thanks.

OK so I'll update this to show 'ums'.

>
> > > Re the second point, at least in U-Boot a USB Ethernet device shows as
> > > 'ethernet', i.e. the interface doesn't matter. We don't have logic to
> > > look a level deeper, at present.
>
> And this is unfortunate and confusing then because it'd be really good
> to know if I'm talking about the USB ethernet I plugged in or the
> ethernet port on the board. Made more confusing in turn by the platforms
> (more historical than modern I *think*) where that ethernet port on the
> SBC itself is wired in via USB.

A user can only tell that with something like 'dm tree' today. Perhaps
this is outside of the scope of this patch, but what would you like it
to show if it is USB Ethernet? Are you asking for code which checks
for this and shows 'usb_ether' or similar?

I still know of boards which use on-board USB for Ethernet, but more
historical as you say.

Regards,
Simon

Re: [PATCH v4 2/4] boot: Move obtaining the label into a common file

[Tom Rini]

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

On Sat, Oct 11, 2025 at 08:21:15AM +0100, Simon Glass wrote:
> Hi Tom,
> 
> On Fri, 10 Oct 2025 at 15:39, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Fri, Oct 10, 2025 at 04:11:51PM +0200, Mattijs Korpershoek wrote:
> > > On Fri, Oct 10, 2025 at 11:35, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > > Hi Tom,
> > > >
> > > > On Thu, 9 Oct 2025 at 18:24, Tom Rini <trini@konsulko.com> wrote:
> > > >>
> > > >> On Thu, Oct 09, 2025 at 03:29:53AM -0600, Simon Glass wrote:
> > > >>
> > > >> > The 'bootflow list' command supports looking at the EFI device-path when
> > > >> > available. Move this piece into a common function so it can be used
> > > >> > elsewhere.
> > > >>
> > > >> The point of this was to enable the show_bootflow->bootflow_show chnage,
> > > >> so should be dropped.
> > > >
> > > > OK.
> > > >
> > > >>
> > > >> > Use 'usb' instead of 'usb_mass_storage' for usb so that it fits in the
> > > >> > column space.
> > > >> >
> > > >> > This updates the output from 'bootflow list'.
> > > >>
> > > >> And this is unrelated and should be its own patch. Wikipedia says USB
> > > >> MSC or UMS are the common short forms for USB Mass Storage devices, and
> > > >> we should use one of them to have the output be more precise (since we
> > > >> support USB networking devices for example).
> > > >
> > > > Ok I can move it into its own patch.
> > > >
> > > > Are you wanting this to say 'ums' instead of 'usb' ? Will anyone know
> > > > what that means?
> > >
> > > Search for 'ums' gives the following U-Boot documentation:
> > >
> > > https://docs.u-boot.org/en/latest/usage/cmd/ums.html#ums-command
> > >
> > > Where the acronym is documented.
> > > This is also the acronym used by the cmd (to configure usb gadget as USB
> > > Mass Storage).
> > >
> > > So I'd say it's a reasonable acronym
> >
> > Thanks.
> 
> OK so I'll update this to show 'ums'.

Thanks.

> > > > Re the second point, at least in U-Boot a USB Ethernet device shows as
> > > > 'ethernet', i.e. the interface doesn't matter. We don't have logic to
> > > > look a level deeper, at present.
> >
> > And this is unfortunate and confusing then because it'd be really good
> > to know if I'm talking about the USB ethernet I plugged in or the
> > ethernet port on the board. Made more confusing in turn by the platforms
> > (more historical than modern I *think*) where that ethernet port on the
> > SBC itself is wired in via USB.
> 
> A user can only tell that with something like 'dm tree' today. Perhaps
> this is outside of the scope of this patch, but what would you like it
> to show if it is USB Ethernet? Are you asking for code which checks
> for this and shows 'usb_ether' or similar?
> 
> I still know of boards which use on-board USB for Ethernet, but more
> historical as you say.

It's certainly outside the scope of the patch here, yes. And it may be
too much effort for too little gain as well. But it would be nice if we
could be more descriptive than "ethernet". But it's low priority and not
worth worrying about right this moment I believe.

-- 
Tom

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

[PATCH v4 3/4] boot: Add more debugging to iter_incr()

[Simon Glass]

This function is the core of the bootstd iteration. Add some debugging
for the decisions it makes along the way, to make it easier to track
what is going on.

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

(no changes since v1)

 boot/bootflow.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/boot/bootflow.c b/boot/bootflow.c
index 78df09f369d..de69e27bec7 100644
--- a/boot/bootflow.c
+++ b/boot/bootflow.c
@@ -193,8 +193,10 @@ static int iter_incr(struct bootflow_iter *iter)
 	log_debug("entry: err=%d\n", iter->err);
 	global = iter->doing_global;
 
-	if (iter->err == BF_NO_MORE_DEVICES)
+	if (iter->err == BF_NO_MORE_DEVICES) {
+		log_debug("-> err: no more devices1\n");
 		return BF_NO_MORE_DEVICES;
+	}
 
 	if (iter->err != BF_NO_MORE_PARTS) {
 		/* Get the next boothmethod */
@@ -218,9 +220,13 @@ static int iter_incr(struct bootflow_iter *iter)
 			inc_dev = false;
 		}
 	}
+	log_debug("! no more methods: cur_method %d num_methods %d\n",
+		  iter->cur_method, iter->num_methods);
 
-	if (iter->flags & BOOTFLOWIF_SINGLE_PARTITION)
+	if (iter->flags & BOOTFLOWIF_SINGLE_PARTITION) {
+		log_debug("-> single partition: no more devices\n");
 		return BF_NO_MORE_DEVICES;
+	}
 
 	/* No more bootmeths; start at the first one, and... */
 	iter->cur_method = 0;
@@ -228,11 +234,15 @@ static int iter_incr(struct bootflow_iter *iter)
 
 	if (iter->err != BF_NO_MORE_PARTS) {
 		/* ...select next partition  */
-		if (++iter->part <= iter->max_part)
+		if (++iter->part <= iter->max_part) {
+			log_debug("-> next partition %d max %d\n", iter->part,
+				  iter->max_part);
 			return 0;
+		}
 	}
 
 	/* No more partitions; start at the first one and... */
+	log_debug("! no more partitions\n");
 	iter->part = 0;
 
 	/*
@@ -326,8 +336,13 @@ static int iter_incr(struct bootflow_iter *iter)
 	}
 
 	/* if there are no more bootdevs, give up */
-	if (ret)
+	if (ret) {
+		log_debug("-> no more bootdevs\n");
 		return log_msg_ret("incr", BF_NO_MORE_DEVICES);
+	}
+
+	log_debug("-> bootdev '%s' method '%s'\n", dev->name,
+		  iter->method->name);
 
 	return 0;
 }
-- 
2.43.0

Re: [PATCH v4 3/4] boot: Add more debugging to iter_incr()

[Tom Rini]

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

On Thu, Oct 09, 2025 at 03:29:54AM -0600, Simon Glass wrote:

> This function is the core of the bootstd iteration. Add some debugging
> for the decisions it makes along the way, to make it easier to track
> what is going on.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
> (no changes since v1)
> 
>  boot/bootflow.c | 23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/boot/bootflow.c b/boot/bootflow.c
> index 78df09f369d..de69e27bec7 100644
> --- a/boot/bootflow.c
> +++ b/boot/bootflow.c
> @@ -193,8 +193,10 @@ static int iter_incr(struct bootflow_iter *iter)
>  	log_debug("entry: err=%d\n", iter->err);
>  	global = iter->doing_global;
>  
> -	if (iter->err == BF_NO_MORE_DEVICES)
> +	if (iter->err == BF_NO_MORE_DEVICES) {
> +		log_debug("-> err: no more devices1\n");
>  		return BF_NO_MORE_DEVICES;
> +	}

Thinking more about what I said in the previous iteration about git
blame history, ones like this should be log_msg_ret (the history on
"when did the test for == BF_NO_MORE_DEVICES come from is unchanged, but
now you can have debug statements when enabled).

[snip]
> @@ -228,11 +234,15 @@ static int iter_incr(struct bootflow_iter *iter)
>  
>  	if (iter->err != BF_NO_MORE_PARTS) {
>  		/* ...select next partition  */
> -		if (++iter->part <= iter->max_part)
> +		if (++iter->part <= iter->max_part) {
> +			log_debug("-> next partition %d max %d\n", iter->part,
> +				  iter->max_part);
>  			return 0;
> +		}

Shouldn't this be a debug message instead in the caller?

[snip]
> @@ -326,8 +336,13 @@ static int iter_incr(struct bootflow_iter *iter)
>  	}
>  
>  	/* if there are no more bootdevs, give up */
> -	if (ret)
> +	if (ret) {
> +		log_debug("-> no more bootdevs\n");
>  		return log_msg_ret("incr", BF_NO_MORE_DEVICES);
> +	}

Then do we actually need both a log_debug and a log_msg_ret?

-- 
Tom

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

Re: [PATCH v4 3/4] boot: Add more debugging to iter_incr()

[Simon Glass]

Hi Tom,

On Thu, 9 Oct 2025 at 18:30, Tom Rini <trini@konsulko.com> wrote:
>
> On Thu, Oct 09, 2025 at 03:29:54AM -0600, Simon Glass wrote:
>
> > This function is the core of the bootstd iteration. Add some debugging
> > for the decisions it makes along the way, to make it easier to track
> > what is going on.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> > (no changes since v1)
> >
> >  boot/bootflow.c | 23 +++++++++++++++++++----
> >  1 file changed, 19 insertions(+), 4 deletions(-)
> >
> > diff --git a/boot/bootflow.c b/boot/bootflow.c
> > index 78df09f369d..de69e27bec7 100644
> > --- a/boot/bootflow.c
> > +++ b/boot/bootflow.c
> > @@ -193,8 +193,10 @@ static int iter_incr(struct bootflow_iter *iter)
> >       log_debug("entry: err=%d\n", iter->err);
> >       global = iter->doing_global;
> >
> > -     if (iter->err == BF_NO_MORE_DEVICES)
> > +     if (iter->err == BF_NO_MORE_DEVICES) {
> > +             log_debug("-> err: no more devices1\n");
> >               return BF_NO_MORE_DEVICES;
> > +     }
>
> Thinking more about what I said in the previous iteration about git
> blame history, ones like this should be log_msg_ret (the history on
> "when did the test for == BF_NO_MORE_DEVICES come from is unchanged, but
> now you can have debug statements when enabled).

Yes we can add that as well, but I still want to have the log_debug()
as this doesn't require enabling CONFIG_LOG_ERROR_RETURN. That feature
produces a lot of output even in normal operation since it shows
errors dealt with by higher level code. It is really only designed to
find the source of a particular error when you are stuck.

>
> [snip]
> > @@ -228,11 +234,15 @@ static int iter_incr(struct bootflow_iter *iter)
> >
> >       if (iter->err != BF_NO_MORE_PARTS) {
> >               /* ...select next partition  */
> > -             if (++iter->part <= iter->max_part)
> > +             if (++iter->part <= iter->max_part) {
> > +                     log_debug("-> next partition %d max %d\n", iter->part,
> > +                               iter->max_part);
> >                       return 0;
> > +             }
>
> Shouldn't this be a debug message instead in the caller?

I am trying to log_debug() every exit from this function...so you can
see the entry and then which path it took.

>
> [snip]
> > @@ -326,8 +336,13 @@ static int iter_incr(struct bootflow_iter *iter)
> >       }
> >
> >       /* if there are no more bootdevs, give up */
> > -     if (ret)
> > +     if (ret) {
> > +             log_debug("-> no more bootdevs\n");
> >               return log_msg_ret("incr", BF_NO_MORE_DEVICES);
> > +     }
>
> Then do we actually need both a log_debug and a log_msg_ret?

Please see above.

Regards,

Re: [PATCH v4 3/4] boot: Add more debugging to iter_incr()

[Tom Rini]

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

On Fri, Oct 10, 2025 at 11:36:10AM +0100, Simon Glass wrote:
> Hi Tom,
> 
> On Thu, 9 Oct 2025 at 18:30, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Thu, Oct 09, 2025 at 03:29:54AM -0600, Simon Glass wrote:
> >
> > > This function is the core of the bootstd iteration. Add some debugging
> > > for the decisions it makes along the way, to make it easier to track
> > > what is going on.
> > >
> > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > ---
> > >
> > > (no changes since v1)
> > >
> > >  boot/bootflow.c | 23 +++++++++++++++++++----
> > >  1 file changed, 19 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/boot/bootflow.c b/boot/bootflow.c
> > > index 78df09f369d..de69e27bec7 100644
> > > --- a/boot/bootflow.c
> > > +++ b/boot/bootflow.c
> > > @@ -193,8 +193,10 @@ static int iter_incr(struct bootflow_iter *iter)
> > >       log_debug("entry: err=%d\n", iter->err);
> > >       global = iter->doing_global;
> > >
> > > -     if (iter->err == BF_NO_MORE_DEVICES)
> > > +     if (iter->err == BF_NO_MORE_DEVICES) {
> > > +             log_debug("-> err: no more devices1\n");
> > >               return BF_NO_MORE_DEVICES;
> > > +     }
> >
> > Thinking more about what I said in the previous iteration about git
> > blame history, ones like this should be log_msg_ret (the history on
> > "when did the test for == BF_NO_MORE_DEVICES come from is unchanged, but
> > now you can have debug statements when enabled).
> 
> Yes we can add that as well, but I still want to have the log_debug()
> as this doesn't require enabling CONFIG_LOG_ERROR_RETURN. That feature
> produces a lot of output even in normal operation since it shows
> errors dealt with by higher level code. It is really only designed to
> find the source of a particular error when you are stuck.
> 
> >
> > [snip]
> > > @@ -228,11 +234,15 @@ static int iter_incr(struct bootflow_iter *iter)
> > >
> > >       if (iter->err != BF_NO_MORE_PARTS) {
> > >               /* ...select next partition  */
> > > -             if (++iter->part <= iter->max_part)
> > > +             if (++iter->part <= iter->max_part) {
> > > +                     log_debug("-> next partition %d max %d\n", iter->part,
> > > +                               iter->max_part);
> > >                       return 0;
> > > +             }
> >
> > Shouldn't this be a debug message instead in the caller?
> 
> I am trying to log_debug() every exit from this function...so you can
> see the entry and then which path it took.
> 
> >
> > [snip]
> > > @@ -326,8 +336,13 @@ static int iter_incr(struct bootflow_iter *iter)
> > >       }
> > >
> > >       /* if there are no more bootdevs, give up */
> > > -     if (ret)
> > > +     if (ret) {
> > > +             log_debug("-> no more bootdevs\n");
> > >               return log_msg_ret("incr", BF_NO_MORE_DEVICES);
> > > +     }
> >
> > Then do we actually need both a log_debug and a log_msg_ret?
> 
> Please see above.

I guess my question is, but why? Is this something that's going to be
debugged frequently? Why doesn't every function have meaningful text
string log_debug messages, just in case? And then why bother with
log_msg_ret at all?

-- 
Tom

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

Re: [PATCH v4 3/4] boot: Add more debugging to iter_incr()

[Simon Glass]

Hi Tom,

On Fri, 10 Oct 2025 at 15:35, Tom Rini <trini@konsulko.com> wrote:
>
> On Fri, Oct 10, 2025 at 11:36:10AM +0100, Simon Glass wrote:
> > Hi Tom,
> >
> > On Thu, 9 Oct 2025 at 18:30, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Thu, Oct 09, 2025 at 03:29:54AM -0600, Simon Glass wrote:
> > >
> > > > This function is the core of the bootstd iteration. Add some debugging
> > > > for the decisions it makes along the way, to make it easier to track
> > > > what is going on.
> > > >
> > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > ---
> > > >
> > > > (no changes since v1)
> > > >
> > > >  boot/bootflow.c | 23 +++++++++++++++++++----
> > > >  1 file changed, 19 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/boot/bootflow.c b/boot/bootflow.c
> > > > index 78df09f369d..de69e27bec7 100644
> > > > --- a/boot/bootflow.c
> > > > +++ b/boot/bootflow.c
> > > > @@ -193,8 +193,10 @@ static int iter_incr(struct bootflow_iter *iter)
> > > >       log_debug("entry: err=%d\n", iter->err);
> > > >       global = iter->doing_global;
> > > >
> > > > -     if (iter->err == BF_NO_MORE_DEVICES)
> > > > +     if (iter->err == BF_NO_MORE_DEVICES) {
> > > > +             log_debug("-> err: no more devices1\n");
> > > >               return BF_NO_MORE_DEVICES;
> > > > +     }
> > >
> > > Thinking more about what I said in the previous iteration about git
> > > blame history, ones like this should be log_msg_ret (the history on
> > > "when did the test for == BF_NO_MORE_DEVICES come from is unchanged, but
> > > now you can have debug statements when enabled).
> >
> > Yes we can add that as well, but I still want to have the log_debug()
> > as this doesn't require enabling CONFIG_LOG_ERROR_RETURN. That feature
> > produces a lot of output even in normal operation since it shows
> > errors dealt with by higher level code. It is really only designed to
> > find the source of a particular error when you are stuck.
> >
> > >
> > > [snip]
> > > > @@ -228,11 +234,15 @@ static int iter_incr(struct bootflow_iter *iter)
> > > >
> > > >       if (iter->err != BF_NO_MORE_PARTS) {
> > > >               /* ...select next partition  */
> > > > -             if (++iter->part <= iter->max_part)
> > > > +             if (++iter->part <= iter->max_part) {
> > > > +                     log_debug("-> next partition %d max %d\n", iter->part,
> > > > +                               iter->max_part);
> > > >                       return 0;
> > > > +             }
> > >
> > > Shouldn't this be a debug message instead in the caller?
> >
> > I am trying to log_debug() every exit from this function...so you can
> > see the entry and then which path it took.
> >
> > >
> > > [snip]
> > > > @@ -326,8 +336,13 @@ static int iter_incr(struct bootflow_iter *iter)
> > > >       }
> > > >
> > > >       /* if there are no more bootdevs, give up */
> > > > -     if (ret)
> > > > +     if (ret) {
> > > > +             log_debug("-> no more bootdevs\n");
> > > >               return log_msg_ret("incr", BF_NO_MORE_DEVICES);
> > > > +     }
> > >
> > > Then do we actually need both a log_debug and a log_msg_ret?
> >
> > Please see above.
>
> I guess my question is, but why? Is this something that's going to be
> debugged frequently? Why doesn't every function have meaningful text
> string log_debug messages, just in case? And then why bother with
> log_msg_ret at all?

I have found myself in this function quite a few times, trying to work
out what it is up to, so I decided to add more debugging.

Anyway, would you like to just drop this patch, or something else?

Regards,
Simon

Re: [PATCH v4 3/4] boot: Add more debugging to iter_incr()

[Tom Rini]

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

On Sat, Oct 11, 2025 at 08:19:53AM +0100, Simon Glass wrote:
> Hi Tom,
> 
> On Fri, 10 Oct 2025 at 15:35, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Fri, Oct 10, 2025 at 11:36:10AM +0100, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Thu, 9 Oct 2025 at 18:30, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Thu, Oct 09, 2025 at 03:29:54AM -0600, Simon Glass wrote:
> > > >
> > > > > This function is the core of the bootstd iteration. Add some debugging
> > > > > for the decisions it makes along the way, to make it easier to track
> > > > > what is going on.
> > > > >
> > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > ---
> > > > >
> > > > > (no changes since v1)
> > > > >
> > > > >  boot/bootflow.c | 23 +++++++++++++++++++----
> > > > >  1 file changed, 19 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/boot/bootflow.c b/boot/bootflow.c
> > > > > index 78df09f369d..de69e27bec7 100644
> > > > > --- a/boot/bootflow.c
> > > > > +++ b/boot/bootflow.c
> > > > > @@ -193,8 +193,10 @@ static int iter_incr(struct bootflow_iter *iter)
> > > > >       log_debug("entry: err=%d\n", iter->err);
> > > > >       global = iter->doing_global;
> > > > >
> > > > > -     if (iter->err == BF_NO_MORE_DEVICES)
> > > > > +     if (iter->err == BF_NO_MORE_DEVICES) {
> > > > > +             log_debug("-> err: no more devices1\n");
> > > > >               return BF_NO_MORE_DEVICES;
> > > > > +     }
> > > >
> > > > Thinking more about what I said in the previous iteration about git
> > > > blame history, ones like this should be log_msg_ret (the history on
> > > > "when did the test for == BF_NO_MORE_DEVICES come from is unchanged, but
> > > > now you can have debug statements when enabled).
> > >
> > > Yes we can add that as well, but I still want to have the log_debug()
> > > as this doesn't require enabling CONFIG_LOG_ERROR_RETURN. That feature
> > > produces a lot of output even in normal operation since it shows
> > > errors dealt with by higher level code. It is really only designed to
> > > find the source of a particular error when you are stuck.
> > >
> > > >
> > > > [snip]
> > > > > @@ -228,11 +234,15 @@ static int iter_incr(struct bootflow_iter *iter)
> > > > >
> > > > >       if (iter->err != BF_NO_MORE_PARTS) {
> > > > >               /* ...select next partition  */
> > > > > -             if (++iter->part <= iter->max_part)
> > > > > +             if (++iter->part <= iter->max_part) {
> > > > > +                     log_debug("-> next partition %d max %d\n", iter->part,
> > > > > +                               iter->max_part);
> > > > >                       return 0;
> > > > > +             }
> > > >
> > > > Shouldn't this be a debug message instead in the caller?
> > >
> > > I am trying to log_debug() every exit from this function...so you can
> > > see the entry and then which path it took.
> > >
> > > >
> > > > [snip]
> > > > > @@ -326,8 +336,13 @@ static int iter_incr(struct bootflow_iter *iter)
> > > > >       }
> > > > >
> > > > >       /* if there are no more bootdevs, give up */
> > > > > -     if (ret)
> > > > > +     if (ret) {
> > > > > +             log_debug("-> no more bootdevs\n");
> > > > >               return log_msg_ret("incr", BF_NO_MORE_DEVICES);
> > > > > +     }
> > > >
> > > > Then do we actually need both a log_debug and a log_msg_ret?
> > >
> > > Please see above.
> >
> > I guess my question is, but why? Is this something that's going to be
> > debugged frequently? Why doesn't every function have meaningful text
> > string log_debug messages, just in case? And then why bother with
> > log_msg_ret at all?
> 
> I have found myself in this function quite a few times, trying to work
> out what it is up to, so I decided to add more debugging.
> 
> Anyway, would you like to just drop this patch, or something else?

I'd like to fix the underlying problem so that we don't have a similar
discussion on your next series. As yes, I think in general all of these
patches to add more detailed logging on top of log_msg_ret logging are
too much. And I gather you're debugging these problems on real hardware
where using some source level debugger with sandbox isn't an option?

-- 
Tom

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

Re: [PATCH v4 3/4] boot: Add more debugging to iter_incr()

[Simon Glass]

Hi Tom,

On Mon, 13 Oct 2025 at 15:15, Tom Rini <trini@konsulko.com> wrote:
>
> On Sat, Oct 11, 2025 at 08:19:53AM +0100, Simon Glass wrote:
> > Hi Tom,
> >
> > On Fri, 10 Oct 2025 at 15:35, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Fri, Oct 10, 2025 at 11:36:10AM +0100, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Thu, 9 Oct 2025 at 18:30, Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > On Thu, Oct 09, 2025 at 03:29:54AM -0600, Simon Glass wrote:
> > > > >
> > > > > > This function is the core of the bootstd iteration. Add some debugging
> > > > > > for the decisions it makes along the way, to make it easier to track
> > > > > > what is going on.
> > > > > >
> > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > ---
> > > > > >
> > > > > > (no changes since v1)
> > > > > >
> > > > > >  boot/bootflow.c | 23 +++++++++++++++++++----
> > > > > >  1 file changed, 19 insertions(+), 4 deletions(-)
> > > > > >
> > > > > > diff --git a/boot/bootflow.c b/boot/bootflow.c
> > > > > > index 78df09f369d..de69e27bec7 100644
> > > > > > --- a/boot/bootflow.c
> > > > > > +++ b/boot/bootflow.c
> > > > > > @@ -193,8 +193,10 @@ static int iter_incr(struct bootflow_iter *iter)
> > > > > >       log_debug("entry: err=%d\n", iter->err);
> > > > > >       global = iter->doing_global;
> > > > > >
> > > > > > -     if (iter->err == BF_NO_MORE_DEVICES)
> > > > > > +     if (iter->err == BF_NO_MORE_DEVICES) {
> > > > > > +             log_debug("-> err: no more devices1\n");
> > > > > >               return BF_NO_MORE_DEVICES;
> > > > > > +     }
> > > > >
> > > > > Thinking more about what I said in the previous iteration about git
> > > > > blame history, ones like this should be log_msg_ret (the history on
> > > > > "when did the test for == BF_NO_MORE_DEVICES come from is unchanged, but
> > > > > now you can have debug statements when enabled).
> > > >
> > > > Yes we can add that as well, but I still want to have the log_debug()
> > > > as this doesn't require enabling CONFIG_LOG_ERROR_RETURN. That feature
> > > > produces a lot of output even in normal operation since it shows
> > > > errors dealt with by higher level code. It is really only designed to
> > > > find the source of a particular error when you are stuck.
> > > >
> > > > >
> > > > > [snip]
> > > > > > @@ -228,11 +234,15 @@ static int iter_incr(struct bootflow_iter *iter)
> > > > > >
> > > > > >       if (iter->err != BF_NO_MORE_PARTS) {
> > > > > >               /* ...select next partition  */
> > > > > > -             if (++iter->part <= iter->max_part)
> > > > > > +             if (++iter->part <= iter->max_part) {
> > > > > > +                     log_debug("-> next partition %d max %d\n", iter->part,
> > > > > > +                               iter->max_part);
> > > > > >                       return 0;
> > > > > > +             }
> > > > >
> > > > > Shouldn't this be a debug message instead in the caller?
> > > >
> > > > I am trying to log_debug() every exit from this function...so you can
> > > > see the entry and then which path it took.
> > > >
> > > > >
> > > > > [snip]
> > > > > > @@ -326,8 +336,13 @@ static int iter_incr(struct bootflow_iter *iter)
> > > > > >       }
> > > > > >
> > > > > >       /* if there are no more bootdevs, give up */
> > > > > > -     if (ret)
> > > > > > +     if (ret) {
> > > > > > +             log_debug("-> no more bootdevs\n");
> > > > > >               return log_msg_ret("incr", BF_NO_MORE_DEVICES);
> > > > > > +     }
> > > > >
> > > > > Then do we actually need both a log_debug and a log_msg_ret?
> > > >
> > > > Please see above.
> > >
> > > I guess my question is, but why? Is this something that's going to be
> > > debugged frequently? Why doesn't every function have meaningful text
> > > string log_debug messages, just in case? And then why bother with
> > > log_msg_ret at all?
> >
> > I have found myself in this function quite a few times, trying to work
> > out what it is up to, so I decided to add more debugging.
> >
> > Anyway, would you like to just drop this patch, or something else?
>
> I'd like to fix the underlying problem so that we don't have a similar
> discussion on your next series. As yes, I think in general all of these
> patches to add more detailed logging on top of log_msg_ret logging are
> too much. And I gather you're debugging these problems on real hardware
> where using some source level debugger with sandbox isn't an option?

What is the underlying problem?

The debugging was useful with the EFI app where it is sort-of possible
to use a debugger, but is a bit painful. I ended up comparing logs to
figure it out. Debuggers are not easy when the function being debugged
is called dozens of times.

I can imagine it being useful when someone reports a problem with the
iteration...we could just turn on the option and the person could post
a dump.

Anyway, I'm happy to drop this patch.

Regards,
Simon

Re: [PATCH v4 3/4] boot: Add more debugging to iter_incr()

[Tom Rini]

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

On Mon, Oct 13, 2025 at 04:12:03PM +0100, Simon Glass wrote:
> Hi Tom,
> 
> On Mon, 13 Oct 2025 at 15:15, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Sat, Oct 11, 2025 at 08:19:53AM +0100, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Fri, 10 Oct 2025 at 15:35, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Fri, Oct 10, 2025 at 11:36:10AM +0100, Simon Glass wrote:
> > > > > Hi Tom,
> > > > >
> > > > > On Thu, 9 Oct 2025 at 18:30, Tom Rini <trini@konsulko.com> wrote:
> > > > > >
> > > > > > On Thu, Oct 09, 2025 at 03:29:54AM -0600, Simon Glass wrote:
> > > > > >
> > > > > > > This function is the core of the bootstd iteration. Add some debugging
> > > > > > > for the decisions it makes along the way, to make it easier to track
> > > > > > > what is going on.
> > > > > > >
> > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > > ---
> > > > > > >
> > > > > > > (no changes since v1)
> > > > > > >
> > > > > > >  boot/bootflow.c | 23 +++++++++++++++++++----
> > > > > > >  1 file changed, 19 insertions(+), 4 deletions(-)
> > > > > > >
> > > > > > > diff --git a/boot/bootflow.c b/boot/bootflow.c
> > > > > > > index 78df09f369d..de69e27bec7 100644
> > > > > > > --- a/boot/bootflow.c
> > > > > > > +++ b/boot/bootflow.c
> > > > > > > @@ -193,8 +193,10 @@ static int iter_incr(struct bootflow_iter *iter)
> > > > > > >       log_debug("entry: err=%d\n", iter->err);
> > > > > > >       global = iter->doing_global;
> > > > > > >
> > > > > > > -     if (iter->err == BF_NO_MORE_DEVICES)
> > > > > > > +     if (iter->err == BF_NO_MORE_DEVICES) {
> > > > > > > +             log_debug("-> err: no more devices1\n");
> > > > > > >               return BF_NO_MORE_DEVICES;
> > > > > > > +     }
> > > > > >
> > > > > > Thinking more about what I said in the previous iteration about git
> > > > > > blame history, ones like this should be log_msg_ret (the history on
> > > > > > "when did the test for == BF_NO_MORE_DEVICES come from is unchanged, but
> > > > > > now you can have debug statements when enabled).
> > > > >
> > > > > Yes we can add that as well, but I still want to have the log_debug()
> > > > > as this doesn't require enabling CONFIG_LOG_ERROR_RETURN. That feature
> > > > > produces a lot of output even in normal operation since it shows
> > > > > errors dealt with by higher level code. It is really only designed to
> > > > > find the source of a particular error when you are stuck.
> > > > >
> > > > > >
> > > > > > [snip]
> > > > > > > @@ -228,11 +234,15 @@ static int iter_incr(struct bootflow_iter *iter)
> > > > > > >
> > > > > > >       if (iter->err != BF_NO_MORE_PARTS) {
> > > > > > >               /* ...select next partition  */
> > > > > > > -             if (++iter->part <= iter->max_part)
> > > > > > > +             if (++iter->part <= iter->max_part) {
> > > > > > > +                     log_debug("-> next partition %d max %d\n", iter->part,
> > > > > > > +                               iter->max_part);
> > > > > > >                       return 0;
> > > > > > > +             }
> > > > > >
> > > > > > Shouldn't this be a debug message instead in the caller?
> > > > >
> > > > > I am trying to log_debug() every exit from this function...so you can
> > > > > see the entry and then which path it took.
> > > > >
> > > > > >
> > > > > > [snip]
> > > > > > > @@ -326,8 +336,13 @@ static int iter_incr(struct bootflow_iter *iter)
> > > > > > >       }
> > > > > > >
> > > > > > >       /* if there are no more bootdevs, give up */
> > > > > > > -     if (ret)
> > > > > > > +     if (ret) {
> > > > > > > +             log_debug("-> no more bootdevs\n");
> > > > > > >               return log_msg_ret("incr", BF_NO_MORE_DEVICES);
> > > > > > > +     }
> > > > > >
> > > > > > Then do we actually need both a log_debug and a log_msg_ret?
> > > > >
> > > > > Please see above.
> > > >
> > > > I guess my question is, but why? Is this something that's going to be
> > > > debugged frequently? Why doesn't every function have meaningful text
> > > > string log_debug messages, just in case? And then why bother with
> > > > log_msg_ret at all?
> > >
> > > I have found myself in this function quite a few times, trying to work
> > > out what it is up to, so I decided to add more debugging.
> > >
> > > Anyway, would you like to just drop this patch, or something else?
> >
> > I'd like to fix the underlying problem so that we don't have a similar
> > discussion on your next series. As yes, I think in general all of these
> > patches to add more detailed logging on top of log_msg_ret logging are
> > too much. And I gather you're debugging these problems on real hardware
> > where using some source level debugger with sandbox isn't an option?
> 
> What is the underlying problem?

Patch and code churn (and so history churn) for no functional changes
and increased reviewer load.

> The debugging was useful with the EFI app where it is sort-of possible
> to use a debugger, but is a bit painful. I ended up comparing logs to
> figure it out. Debuggers are not easy when the function being debugged
> is called dozens of times.
> 
> I can imagine it being useful when someone reports a problem with the
> iteration...we could just turn on the option and the person could post
> a dump.
> 
> Anyway, I'm happy to drop this patch.

Thanks.

-- 
Tom

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

[PATCH v4 4/4] boot: Move preparing bootdev into a function

[Simon Glass]

The code at the end of iter_inc() is already somewhat tortuous. Before
making it worse, move it into a function.

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

(no changes since v1)

 boot/bootflow.c | 40 +++++++++++++++++++++++++++++-----------
 1 file changed, 29 insertions(+), 11 deletions(-)

diff --git a/boot/bootflow.c b/boot/bootflow.c
index de69e27bec7..b1eae28b6fd 100644
--- a/boot/bootflow.c
+++ b/boot/bootflow.c
@@ -178,6 +178,32 @@ static void scan_next_in_uclass(struct udevice **devp)
 	*devp = dev;
 }
 
+/**
+ * prepare_bootdev() - Get ready to use a bootdev
+ *
+ * @iter: Bootflow iterator being used
+ * @dev: UCLASS_BOOTDEV device to use
+ * @method_flags: Method flag for the bootdev
+ * Return 0 if OK, -ve if the bootdev failed to probe
+ */
+static int prepare_bootdev(struct bootflow_iter *iter, struct udevice *dev,
+			   int method_flags)
+{
+	int ret;
+
+	/*
+	 * Probe the bootdev. This does not probe any attached block device,
+	 * since they are siblings
+	 */
+	ret = device_probe(dev);
+	log_debug("probe %s %d\n", dev->name, ret);
+	if (ret)
+		return log_msg_ret("probe", ret);
+	bootflow_iter_set_dev(iter, dev, method_flags);
+
+	return 0;
+}
+
 /**
  * iter_incr() - Move to the next item (method, part, bootdev)
  *
@@ -321,18 +347,10 @@ static int iter_incr(struct bootflow_iter *iter)
 		}
 		log_debug("ret=%d, dev=%p %s\n", ret, dev,
 			  dev ? dev->name : "none");
-		if (ret) {
+		if (ret)
 			bootflow_iter_set_dev(iter, NULL, 0);
-		} else {
-			/*
-			 * Probe the bootdev. This does not probe any attached
-			 * block device, since they are siblings
-			 */
-			ret = device_probe(dev);
-			log_debug("probe %s %d\n", dev->name, ret);
-			if (!log_msg_ret("probe", ret))
-				bootflow_iter_set_dev(iter, dev, method_flags);
-		}
+		else
+			ret = prepare_bootdev(iter, dev, method_flags);
 	}
 
 	/* if there are no more bootdevs, give up */
-- 
2.43.0

Re: [PATCH v4 4/4] boot: Move preparing bootdev into a function

[Tom Rini]

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

On Thu, Oct 09, 2025 at 03:29:55AM -0600, Simon Glass wrote:

> The code at the end of iter_inc() is already somewhat tortuous. Before
> making it worse, move it into a function.

This is not a great commit message. Taking a look at v2 as a whole, the
reason to extract this logic from the end of iter_inc is because you
will be checking it in multiple places. That is a good reason to move
the code. Adding another 5 lines of code (a single if test) alone would
not be.

-- 
Tom

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

Re: [PATCH v4 4/4] boot: Move preparing bootdev into a function

[Simon Glass]

Hi Tom,

On Thu, 9 Oct 2025 at 18:35, Tom Rini <trini@konsulko.com> wrote:
>
> On Thu, Oct 09, 2025 at 03:29:55AM -0600, Simon Glass wrote:
>
> > The code at the end of iter_inc() is already somewhat tortuous. Before
> > making it worse, move it into a function.
>
> This is not a great commit message. Taking a look at v2 as a whole, the
> reason to extract this logic from the end of iter_inc is because you
> will be checking it in multiple places. That is a good reason to move
> the code. Adding another 5 lines of code (a single if test) alone would
> not be.

OK, so how about:

The code at the end of iter_inc() is already somewhat tortuous. A future
series needs to call it in from two places in iter_inc(), so move it
into a function.

Regards,
Simon

Re: [PATCH v4 4/4] boot: Move preparing bootdev into a function

[Tom Rini]

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

On Fri, Oct 10, 2025 at 11:36:20AM +0100, Simon Glass wrote:
> Hi Tom,
> 
> On Thu, 9 Oct 2025 at 18:35, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Thu, Oct 09, 2025 at 03:29:55AM -0600, Simon Glass wrote:
> >
> > > The code at the end of iter_inc() is already somewhat tortuous. Before
> > > making it worse, move it into a function.
> >
> > This is not a great commit message. Taking a look at v2 as a whole, the
> > reason to extract this logic from the end of iter_inc is because you
> > will be checking it in multiple places. That is a good reason to move
> > the code. Adding another 5 lines of code (a single if test) alone would
> > not be.
> 
> OK, so how about:
> 
> The code at the end of iter_inc() is already somewhat tortuous. A future
> series needs to call it in from two places in iter_inc(), so move it
> into a function.

No, I dropped the "already somewhat tortuous" part on purpose, because
it's just normal regular easy to understand code. It's on that threshold
of indentation where you could break it out, or you could leave it, and
be fine. The entire reason to split it out is because it will be called
in multiple places.

-- 
Tom

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

Re: [PATCH v4 4/4] boot: Move preparing bootdev into a function

[Simon Glass]

Hi Tom,

On Fri, 10 Oct 2025 at 15:38, Tom Rini <trini@konsulko.com> wrote:
>
> On Fri, Oct 10, 2025 at 11:36:20AM +0100, Simon Glass wrote:
> > Hi Tom,
> >
> > On Thu, 9 Oct 2025 at 18:35, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Thu, Oct 09, 2025 at 03:29:55AM -0600, Simon Glass wrote:
> > >
> > > > The code at the end of iter_inc() is already somewhat tortuous. Before
> > > > making it worse, move it into a function.
> > >
> > > This is not a great commit message. Taking a look at v2 as a whole, the
> > > reason to extract this logic from the end of iter_inc is because you
> > > will be checking it in multiple places. That is a good reason to move
> > > the code. Adding another 5 lines of code (a single if test) alone would
> > > not be.
> >
> > OK, so how about:
> >
> > The code at the end of iter_inc() is already somewhat tortuous. A future
> > series needs to call it in from two places in iter_inc(), so move it
> > into a function.
>
> No, I dropped the "already somewhat tortuous" part on purpose, because
> it's just normal regular easy to understand code. It's on that threshold
> of indentation where you could break it out, or you could leave it, and
> be fine. The entire reason to split it out is because it will be called
> in multiple places.

OK, so how about:

We will want to use this same logic in another place within
iter_inc(), so split it out into its own function.

Otherwise, please suggest something.

Regards,
Simon

Re: [PATCH v4 4/4] boot: Move preparing bootdev into a function

[Tom Rini]

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

On Sat, Oct 11, 2025 at 08:20:02AM +0100, Simon Glass wrote:
> Hi Tom,
> 
> On Fri, 10 Oct 2025 at 15:38, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Fri, Oct 10, 2025 at 11:36:20AM +0100, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Thu, 9 Oct 2025 at 18:35, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Thu, Oct 09, 2025 at 03:29:55AM -0600, Simon Glass wrote:
> > > >
> > > > > The code at the end of iter_inc() is already somewhat tortuous. Before
> > > > > making it worse, move it into a function.
> > > >
> > > > This is not a great commit message. Taking a look at v2 as a whole, the
> > > > reason to extract this logic from the end of iter_inc is because you
> > > > will be checking it in multiple places. That is a good reason to move
> > > > the code. Adding another 5 lines of code (a single if test) alone would
> > > > not be.
> > >
> > > OK, so how about:
> > >
> > > The code at the end of iter_inc() is already somewhat tortuous. A future
> > > series needs to call it in from two places in iter_inc(), so move it
> > > into a function.
> >
> > No, I dropped the "already somewhat tortuous" part on purpose, because
> > it's just normal regular easy to understand code. It's on that threshold
> > of indentation where you could break it out, or you could leave it, and
> > be fine. The entire reason to split it out is because it will be called
> > in multiple places.
> 
> OK, so how about:
> 
> We will want to use this same logic in another place within
> iter_inc(), so split it out into its own function.

Yes, that's tone-neutral and so is fine, thanks.

-- 
Tom

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