[PATCH 0/4] bootstd: Test and boot_targets improvements

[PATCH 0/4] bootstd: Test and boot_targets improvements

[Simon Glass]

This series makes another attempt to support things like "mmc" in the
boot_targets variable. The previous attempt introduced a bug which made
iteration fail.

New test coverage is provided for some previously untested behaviour, to
prevent regressions.


Simon Glass (4):
  Revert "bootstd: Scan all bootdevs in a boot_targets entry"
  bootstd: Expand boot-ordering test to include USB
  bootstd: Correct logic for single uclass
  bootstd: Scan all bootdevs in a boot_targets entry (take 2)

 boot/bootflow.c     | 29 ++++++++++++++++++++----
 test/boot/bootdev.c | 54 ++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 73 insertions(+), 10 deletions(-)

-- 
2.42.0.655.g421f12c284-goog

[PATCH 1/4] Revert "bootstd: Scan all bootdevs in a boot_targets entry"

[Simon Glass]

This commit was intended to allow all bootdevs in each boot_targets
entry to be scanned. However it causes bad ordering with bootdevs, e.g.
scanning Ethernet bootdevs when it should be keeping to mmc.

Revert it so we can try another approach.

This reverts commit e824d0d0c219bc6da767f13f90c5b00eefe929f0.

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

 boot/bootdev-uclass.c |  3 +--
 boot/bootflow.c       | 21 ++-------------------
 test/boot/bootdev.c   | 10 ----------
 3 files changed, 3 insertions(+), 31 deletions(-)

diff --git a/boot/bootdev-uclass.c b/boot/bootdev-uclass.c
index 44ae98a9269c..974ddee5d2fa 100644
--- a/boot/bootdev-uclass.c
+++ b/boot/bootdev-uclass.c
@@ -469,11 +469,10 @@ int bootdev_find_by_label(const char *label, struct udevice **devp,
 			 * if no sequence number was provided, we must scan all
 			 * bootdevs for this media uclass
 			 */
-			if (seq == -1)
+			if (IS_ENABLED(CONFIG_BOOTSTD_FULL) && 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);
diff --git a/boot/bootflow.c b/boot/bootflow.c
index e03932e65a74..6ef62e1d1896 100644
--- a/boot/bootflow.c
+++ b/boot/bootflow.c
@@ -260,25 +260,8 @@ static int iter_incr(struct bootflow_iter *iter)
 		} else {
 			log_debug("labels %p\n", iter->labels);
 			if (iter->labels) {
-				/*
-				 * when the label is "mmc" we want to scan all
-				 * mmc bootdevs, not just the first. See
-				 * bootdev_find_by_label() where this flag is
-				 * set up
-				 */
-				if (iter->method_flags & BOOTFLOW_METHF_SINGLE_UCLASS) {
-					uclass_next_device(&dev);
-					log_debug("looking for next device %s: %s\n",
-						  iter->dev->name,
-						  dev ? dev->name : "<none>");
-				} else {
-					dev = NULL;
-				}
-				if (!dev) {
-					log_debug("looking at next label\n");
-					ret = bootdev_next_label(iter, &dev,
-								 &method_flags);
-				}
+				ret = bootdev_next_label(iter, &dev,
+							 &method_flags);
 			} else {
 				ret = bootdev_next_prio(iter, &dev);
 				method_flags = 0;
diff --git a/test/boot/bootdev.c b/test/boot/bootdev.c
index c5f14a7a1323..6b29213416db 100644
--- a/test/boot/bootdev.c
+++ b/test/boot/bootdev.c
@@ -221,16 +221,6 @@ static int bootdev_test_order(struct unit_test_state *uts)
 	ut_asserteq_str("mmc2.bootdev", iter.dev_used[1]->name);
 	bootflow_iter_uninit(&iter);
 
-	/* Make sure it scans a bootdevs in each target */
-	ut_assertok(env_set("boot_targets", "mmc spi"));
-	ut_asserteq(0, bootflow_scan_first(NULL, NULL, &iter, 0, &bflow));
-	ut_asserteq(-ENODEV, bootflow_scan_next(&iter, &bflow));
-	ut_asserteq(3, 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);
-	bootflow_iter_uninit(&iter);
-
 	return 0;
 }
 BOOTSTD_TEST(bootdev_test_order, UT_TESTF_DM | UT_TESTF_SCAN_FDT);
-- 
2.42.0.655.g421f12c284-goog

[PATCH 2/4] bootstd: Expand boot-ordering test to include USB

[Simon Glass]

Scan the USB bus as well, so we can check that different uclasses work
correctly in boot_targets

update the function comment with more detail.

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

 test/boot/bootdev.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/test/boot/bootdev.c b/test/boot/bootdev.c
index 6b29213416db..7228f545e9e6 100644
--- a/test/boot/bootdev.c
+++ b/test/boot/bootdev.c
@@ -190,12 +190,21 @@ static int bootdev_test_any(struct unit_test_state *uts)
 BOOTSTD_TEST(bootdev_test_any, UT_TESTF_DM | UT_TESTF_SCAN_FDT |
 	     UT_TESTF_ETH_BOOTDEV);
 
-/* Check bootdev ordering with the bootdev-order property */
+/*
+ * Check bootdev ordering with the bootdev-order property and boot_targets
+ * environment variable
+ */
 static int bootdev_test_order(struct unit_test_state *uts)
 {
 	struct bootflow_iter iter;
 	struct bootflow bflow;
 
+	test_set_skip_delays(true);
+
+	/* Start up USB which gives us three additional bootdevs */
+	usb_started = false;
+	ut_assertok(run_command("usb start", 0));
+
 	/*
 	 * First try the order set by the bootdev-order property
 	 * Like all sandbox unit tests this relies on the devicetree setting up
@@ -213,12 +222,14 @@ static int bootdev_test_order(struct unit_test_state *uts)
 	bootflow_iter_uninit(&iter);
 
 	/* Use the environment variable to override it */
-	ut_assertok(env_set("boot_targets", "mmc1 mmc2"));
+	ut_assertok(env_set("boot_targets", "mmc1 mmc2 usb"));
 	ut_assertok(bootflow_scan_first(NULL, NULL, &iter, 0, &bflow));
 	ut_asserteq(-ENODEV, bootflow_scan_next(&iter, &bflow));
-	ut_asserteq(2, iter.num_devs);
+	ut_asserteq(3, 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",
+			iter.dev_used[2]->name);
 	bootflow_iter_uninit(&iter);
 
 	return 0;
-- 
2.42.0.655.g421f12c284-goog

[PATCH 3/4] bootstd: Correct logic for single uclass

[Simon Glass]

The current logic for "bootflow mmc" is flawed since it checks the
uclass of the bootdev instead of its parent, the media device. Correct
this and add a test that covers this scenario.

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

 boot/bootflow.c     | 24 ++++++++++++++++++++++--
 test/boot/bootdev.c | 13 +++++++++++++
 2 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/boot/bootflow.c b/boot/bootflow.c
index 6ef62e1d1896..7f5b0e942078 100644
--- a/boot/bootflow.c
+++ b/boot/bootflow.c
@@ -155,6 +155,27 @@ static void bootflow_iter_set_dev(struct bootflow_iter *iter,
 	}
 }
 
+/**
+ * scan_next_in_uclass() - Scan for the next bootdev in the same media uclass
+ *
+ * Move through the following bootdevs until we find another in this media
+ * uclass, or run out
+ *
+ * @devp: On entry, the device to check, on exit the new device, or NULL if
+ * there is none
+ */
+static void scan_next_in_uclass(struct udevice **devp)
+{
+	struct udevice *dev = *devp;
+	enum uclass_id cur_id = device_get_uclass_id(dev->parent);
+
+	do {
+		uclass_find_next_device(&dev);
+	} while (dev && cur_id != device_get_uclass_id(dev->parent));
+
+	*devp = dev;
+}
+
 /**
  * iter_incr() - Move to the next item (method, part, bootdev)
  *
@@ -230,8 +251,7 @@ static int iter_incr(struct bootflow_iter *iter)
 						 &method_flags);
 		} else if (IS_ENABLED(CONFIG_BOOTSTD_FULL) &&
 			   (iter->flags & BOOTFLOWIF_SINGLE_UCLASS)) {
-			/* Move to the next bootdev in this uclass */
-			uclass_find_next_device(&dev);
+			scan_next_in_uclass(&dev);
 			if (!dev) {
 				log_debug("finished uclass %s\n",
 					  dev_get_uclass_name(dev));
diff --git a/test/boot/bootdev.c b/test/boot/bootdev.c
index 7228f545e9e6..637861748057 100644
--- a/test/boot/bootdev.c
+++ b/test/boot/bootdev.c
@@ -232,6 +232,19 @@ static int bootdev_test_order(struct unit_test_state *uts)
 			iter.dev_used[2]->name);
 	bootflow_iter_uninit(&iter);
 
+	/* Try a single uclass */
+	ut_assertok(env_set("boot_targets", NULL));
+	ut_assertok(bootflow_scan_first(NULL, "mmc", &iter, 0, &bflow));
+	ut_asserteq(2, iter.num_devs);
+
+	/* Now scan pass mmc1 and make sure that only mmc0 shows up */
+	ut_asserteq(-ENODEV, bootflow_scan_next(&iter, &bflow));
+	ut_asserteq(3, 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);
+	bootflow_iter_uninit(&iter);
+
 	return 0;
 }
 BOOTSTD_TEST(bootdev_test_order, UT_TESTF_DM | UT_TESTF_SCAN_FDT);
-- 
2.42.0.655.g421f12c284-goog

[PATCH 4/4] bootstd: Scan all bootdevs in a boot_targets entry (take 2)

[Simon Glass]

When the boot_targets environment variable is used with the distro-boot
scripts, each device is included individually. For example, if there
are three mmc devices, then we will have something like:

   boot_targets="mmc0 mmc1 mmc2"

In contrast, standard boot supports specifying just the uclass, i.e.:

   boot_targets="mmc"

The intention is that this should scan all MMC devices, but in fact it
currently only scans the first.

Update the logic to handle this case, without required BOOTSTD_FULL to
be enabled.

Signed-off-by: Simon Glass <sjg@chromium.org>
Reported-by: Date Huang <tjjh89017@hotmail.com>
Reported-by: Vincent Stehlé <vincent.stehle@arm.com>
Reported-by: Ivan Ivanov <ivan.ivanov@suse.com>
---

 boot/bootdev-uclass.c |  3 ++-
 boot/bootflow.c       | 22 ++++++++++++++++++++--
 test/boot/bootdev.c   | 32 ++++++++++++++++++++++++++++++--
 3 files changed, 52 insertions(+), 5 deletions(-)

diff --git a/boot/bootdev-uclass.c b/boot/bootdev-uclass.c
index 974ddee5d2fa..44ae98a9269c 100644
--- a/boot/bootdev-uclass.c
+++ b/boot/bootdev-uclass.c
@@ -469,10 +469,11 @@ int bootdev_find_by_label(const char *label, struct udevice **devp,
 			 * if no sequence number was provided, we must scan all
 			 * bootdevs for this media uclass
 			 */
-			if (IS_ENABLED(CONFIG_BOOTSTD_FULL) && seq == -1)
+			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);
diff --git a/boot/bootflow.c b/boot/bootflow.c
index 7f5b0e942078..be543c8588cc 100644
--- a/boot/bootflow.c
+++ b/boot/bootflow.c
@@ -280,8 +280,26 @@ static int iter_incr(struct bootflow_iter *iter)
 		} else {
 			log_debug("labels %p\n", iter->labels);
 			if (iter->labels) {
-				ret = bootdev_next_label(iter, &dev,
-							 &method_flags);
+				/*
+				 * when the label is "mmc" we want to scan all
+				 * mmc bootdevs, not just the first. See
+				 * bootdev_find_by_label() where this flag is
+				 * set up
+				 */
+				if (iter->method_flags &
+				    BOOTFLOW_METHF_SINGLE_UCLASS) {
+					scan_next_in_uclass(&dev);
+					log_debug("looking for next device %s: %s\n",
+						  iter->dev->name,
+						  dev ? dev->name : "<none>");
+				} else {
+					dev = NULL;
+				}
+				if (!dev) {
+					log_debug("looking at next label\n");
+					ret = bootdev_next_label(iter, &dev,
+								 &method_flags);
+				}
 			} else {
 				ret = bootdev_next_prio(iter, &dev);
 				method_flags = 0;
diff --git a/test/boot/bootdev.c b/test/boot/bootdev.c
index 637861748057..0702fccdae60 100644
--- a/test/boot/bootdev.c
+++ b/test/boot/bootdev.c
@@ -225,7 +225,7 @@ 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));
 	ut_asserteq(-ENODEV, bootflow_scan_next(&iter, &bflow));
-	ut_asserteq(3, iter.num_devs);
+	ut_asserteq(5, 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",
@@ -237,7 +237,20 @@ static int bootdev_test_order(struct unit_test_state *uts)
 	ut_assertok(bootflow_scan_first(NULL, "mmc", &iter, 0, &bflow));
 	ut_asserteq(2, iter.num_devs);
 
-	/* Now scan pass mmc1 and make sure that only mmc0 shows up */
+	/* Now scan past mmc1 and make sure that only mmc0 shows up */
+	ut_asserteq(-ENODEV, bootflow_scan_next(&iter, &bflow));
+	ut_asserteq(3, 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);
+	bootflow_iter_uninit(&iter);
+
+	/* Try a single uclass with boot_targets */
+	ut_assertok(env_set("boot_targets", "mmc"));
+	ut_assertok(bootflow_scan_first(NULL, NULL, &iter, 0, &bflow));
+	ut_asserteq(2, iter.num_devs);
+
+	/* Now scan past mmc1 and make sure that only mmc0 shows up */
 	ut_asserteq(-ENODEV, bootflow_scan_next(&iter, &bflow));
 	ut_asserteq(3, iter.num_devs);
 	ut_asserteq_str("mmc2.bootdev", iter.dev_used[0]->name);
@@ -245,6 +258,21 @@ static int bootdev_test_order(struct unit_test_state *uts)
 	ut_asserteq_str("mmc0.bootdev", iter.dev_used[2]->name);
 	bootflow_iter_uninit(&iter);
 
+	/* Try a single uclass with boot_targets */
+	ut_assertok(env_set("boot_targets", "mmc usb"));
+	ut_assertok(bootflow_scan_first(NULL, NULL, &iter, 0, &bflow));
+	ut_asserteq(2, iter.num_devs);
+
+	/* Now scan past mmc1 and make sure that the 3 USB devices show up */
+	ut_asserteq(-ENODEV, bootflow_scan_next(&iter, &bflow));
+	ut_asserteq(6, 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);
+	ut_asserteq_str("usb_mass_storage.lun0.bootdev",
+			iter.dev_used[3]->name);
+	bootflow_iter_uninit(&iter);
+
 	return 0;
 }
 BOOTSTD_TEST(bootdev_test_order, UT_TESTF_DM | UT_TESTF_SCAN_FDT);
-- 
2.42.0.655.g421f12c284-goog

Re: [PATCH 0/4] bootstd: Test and boot_targets improvements

[Ivan T. Ivanov]

On 10-23 00:02, Simon Glass wrote:
> 
> This series makes another attempt to support things like "mmc" in the
> boot_targets variable. The previous attempt introduced a bug which made
> iteration fail.
> 
> New test coverage is provided for some previously untested behaviour, to
> prevent regressions.
> 
> 

Thank you! Booting RPi4 from USB and uSB looks fine now.
I have updated also our Bugzilla entry with the boot log.

Tested-by: Ivan T.Ivanov <iivanov@suse.de>

Regards,
Ivan

[1] https://bugzilla.suse.com/show_bug.cgi?id=1216036

> Simon Glass (4):
>   Revert "bootstd: Scan all bootdevs in a boot_targets entry"
>   bootstd: Expand boot-ordering test to include USB
>   bootstd: Correct logic for single uclass
>   bootstd: Scan all bootdevs in a boot_targets entry (take 2)
> 

Re: [PATCH 0/4] bootstd: Test and boot_targets improvements

[Simon Glass]

Hi Ivan,

On Mon, 23 Oct 2023 at 02:44, Ivan T. Ivanov <iivanov@suse.de> wrote:
>
> On 10-23 00:02, Simon Glass wrote:
> >
> > This series makes another attempt to support things like "mmc" in the
> > boot_targets variable. The previous attempt introduced a bug which made
> > iteration fail.
> >
> > New test coverage is provided for some previously untested behaviour, to
> > prevent regressions.
> >
> >
>
> Thank you! Booting RPi4 from USB and uSB looks fine now.
> I have updated also our Bugzilla entry with the boot log.
>
> Tested-by: Ivan T.Ivanov <iivanov@suse.de>

Thank you for checking it.

Regards,
Simon


>
> Regards,
> Ivan
>
> [1] https://bugzilla.suse.com/show_bug.cgi?id=1216036
>
> > Simon Glass (4):
> >   Revert "bootstd: Scan all bootdevs in a boot_targets entry"
> >   bootstd: Expand boot-ordering test to include USB
> >   bootstd: Correct logic for single uclass
> >   bootstd: Scan all bootdevs in a boot_targets entry (take 2)
> >

Re: [PATCH 0/4] bootstd: Test and boot_targets improvements

[Tom Rini]

On Mon, 23 Oct 2023 00:02:09 -0700, Simon Glass wrote:

> This series makes another attempt to support things like "mmc" in the
> boot_targets variable. The previous attempt introduced a bug which made
> iteration fail.
> 
> New test coverage is provided for some previously untested behaviour, to
> prevent regressions.
> 
> [...]

Applied to u-boot/master, thanks!

-- 
Tom