[PATCH 0/4] bootstd: Test and boot_targets improvements
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(-)
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);
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;
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);
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);
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)
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)
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!
participants (3)
-
Ivan T. Ivanov -
Simon Glass -
Tom Rini