* [PATCH] test: boot: Bind bootmeth_{cros,android} only once per run
@ 2024-11-21 15:02 Mattijs Korpershoek
2024-11-22 13:37 ` [PATCH] test: boot: Bind bootmeth_{cros, android} " Simon Glass
0 siblings, 1 reply; 3+ messages in thread
From: Mattijs Korpershoek @ 2024-11-21 15:02 UTC (permalink / raw)
To: Tom Rini, Simon Glass; +Cc: Guillaume La Roque, u-boot, Mattijs Korpershoek
We make fewer calls to dm_test_restore() since
commit fbdac8155c89 ("test: Expand implementation of ut_list_has_dm_tests()")
Because of this some valid test combinations are now broken:
$ ./test/py/test.py --bd sandbox --build -k test_ut
$ ./test/py/test.py --bd sandbox --build -k "bootflow_android or bootflow_cros"
Shows:
Expected ' 2 cros ready mmc 4 mmc5.bootdev.part_4 ',
got ' 2 cros ready mmc 2 mmc5.bootdev.part_2 '
Here prep_mmc_bootdev() is called twice and it will bind bootmeth_cros twice.
Since bootmeth_cros is bound twice, 'bootflow scan' will find 2x the
expected bootflows.
Before
commit fbdac8155c89 ("test: Expand implementation of ut_list_has_dm_tests()")
this did not happen because a cleanup was called each time.
Check if the bootstd already has a "cros" or "android" driver and only bind it
if it does not exist.
Fixes: fbdac8155c89 ("test: Expand implementation of ut_list_has_dm_tests()")
Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
---
Initially, this was found when merging some Android bootflow patches
from [1].
However, this can be reproduced on 'next' as well.
Here is the test output that's broken (we have 4 cros bootmethods instead of 2)
=> ut bootstd bootflow_cros
Test: bootflow_cros: bootflow.c
Showing all bootflows
Seq Method State Uclass Part Name Filename
--- ----------- ------ -------- ---- ------------------------ ----------------
0 extlinux ready mmc 1 mmc1.bootdev.part_1 /extlinux/extlinux.conf
1 cros ready mmc 2 mmc5.bootdev.part_2
2 cros ready mmc 2 mmc5.bootdev.part_2
3 cros ready mmc 4 mmc5.bootdev.part_4
4 cros ready mmc 4 mmc5.bootdev.part_4
--- ----------- ------ -------- ---- ------------------------ ----------------
(5 bootflows, 5 valid)
test/boot/bootflow.c:1192, bootflow_cros(): console:
Expected ' 2 cros ready mmc 4 mmc5.bootdev.part_4 ',
got ' 2 cros ready mmc 2 mmc5.bootdev.part_2 '
Test bootflow_cros failed 1 times
[1] https://lore.kernel.org/r/all/87mshvxhq3.fsf@baylibre.com/
---
test/boot/bootflow.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/test/boot/bootflow.c b/test/boot/bootflow.c
index 9397328609d0..835d2e6e35a6 100644
--- a/test/boot/bootflow.c
+++ b/test/boot/bootflow.c
@@ -552,15 +552,21 @@ static int prep_mmc_bootdev(struct unit_test_state *uts, const char *mmc_dev,
/* Enable the cros bootmeth if needed */
if (IS_ENABLED(CONFIG_BOOTMETH_CROS) && bind_cros_android) {
ut_assertok(uclass_first_device_err(UCLASS_BOOTSTD, &bootstd));
- ut_assertok(device_bind(bootstd, DM_DRIVER_REF(bootmeth_cros),
- "cros", 0, ofnode_null(), &dev));
+ /* Only bind the bootmeth once to avoid duplicate scan results */
+ if (device_find_child_by_name(bootstd, "cros", &dev) == -ENODEV) {
+ ut_assertok(device_bind(bootstd, DM_DRIVER_REF(bootmeth_cros),
+ "cros", 0, ofnode_null(), &dev));
+ }
}
/* Enable the android bootmeths if needed */
if (IS_ENABLED(CONFIG_BOOTMETH_ANDROID) && bind_cros_android) {
ut_assertok(uclass_first_device_err(UCLASS_BOOTSTD, &bootstd));
- ut_assertok(device_bind(bootstd, DM_DRIVER_REF(bootmeth_android),
- "android", 0, ofnode_null(), &dev));
+ /* Only bind the bootmeth once to avoid duplicate scan results */
+ if (device_find_child_by_name(bootstd, "android", &dev) == -ENODEV) {
+ ut_assertok(device_bind(bootstd, DM_DRIVER_REF(bootmeth_android),
+ "android", 0, ofnode_null(), &dev));
+ }
}
/* Change the order to include the device */
---
base-commit: dc1859f8d2ac3faaa5e2e1d465ec4bd8980520a5
change-id: 20241121-bootstd-test-fix-multiple-bind-159bd2523414
Best regards,
--
Mattijs Korpershoek <mkorpershoek@baylibre.com>
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] test: boot: Bind bootmeth_{cros, android} only once per run
2024-11-21 15:02 [PATCH] test: boot: Bind bootmeth_{cros,android} only once per run Mattijs Korpershoek
@ 2024-11-22 13:37 ` Simon Glass
2024-11-22 14:05 ` Mattijs Korpershoek
0 siblings, 1 reply; 3+ messages in thread
From: Simon Glass @ 2024-11-22 13:37 UTC (permalink / raw)
To: Mattijs Korpershoek; +Cc: Tom Rini, Guillaume La Roque, u-boot
Hi Mattijs,
On Thu, 21 Nov 2024 at 08:03, Mattijs Korpershoek
<mkorpershoek@baylibre.com> wrote:
>
> We make fewer calls to dm_test_restore() since
> commit fbdac8155c89 ("test: Expand implementation of ut_list_has_dm_tests()")
>
> Because of this some valid test combinations are now broken:
>
> $ ./test/py/test.py --bd sandbox --build -k test_ut
> $ ./test/py/test.py --bd sandbox --build -k "bootflow_android or bootflow_cros"
>
> Shows:
>
> Expected ' 2 cros ready mmc 4 mmc5.bootdev.part_4 ',
> got ' 2 cros ready mmc 2 mmc5.bootdev.part_2 '
>
> Here prep_mmc_bootdev() is called twice and it will bind bootmeth_cros twice.
>
> Since bootmeth_cros is bound twice, 'bootflow scan' will find 2x the
> expected bootflows.
>
> Before
> commit fbdac8155c89 ("test: Expand implementation of ut_list_has_dm_tests()")
> this did not happen because a cleanup was called each time.
>
> Check if the bootstd already has a "cros" or "android" driver and only bind it
> if it does not exist.
>
> Fixes: fbdac8155c89 ("test: Expand implementation of ut_list_has_dm_tests()")
> Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
> ---
> Initially, this was found when merging some Android bootflow patches
> from [1].
>
> However, this can be reproduced on 'next' as well.
>
> Here is the test output that's broken (we have 4 cros bootmethods instead of 2)
>
> => ut bootstd bootflow_cros
> Test: bootflow_cros: bootflow.c
> Showing all bootflows
> Seq Method State Uclass Part Name Filename
> --- ----------- ------ -------- ---- ------------------------ ----------------
> 0 extlinux ready mmc 1 mmc1.bootdev.part_1 /extlinux/extlinux.conf
> 1 cros ready mmc 2 mmc5.bootdev.part_2
> 2 cros ready mmc 2 mmc5.bootdev.part_2
> 3 cros ready mmc 4 mmc5.bootdev.part_4
> 4 cros ready mmc 4 mmc5.bootdev.part_4
> --- ----------- ------ -------- ---- ------------------------ ----------------
> (5 bootflows, 5 valid)
> test/boot/bootflow.c:1192, bootflow_cros(): console:
> Expected ' 2 cros ready mmc 4 mmc5.bootdev.part_4 ',
> got ' 2 cros ready mmc 2 mmc5.bootdev.part_2 '
> Test bootflow_cros failed 1 times
>
> [1] https://lore.kernel.org/r/all/87mshvxhq3.fsf@baylibre.com/
> ---
> test/boot/bootflow.c | 14 ++++++++++----
> 1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/test/boot/bootflow.c b/test/boot/bootflow.c
> index 9397328609d0..835d2e6e35a6 100644
> --- a/test/boot/bootflow.c
> +++ b/test/boot/bootflow.c
> @@ -552,15 +552,21 @@ static int prep_mmc_bootdev(struct unit_test_state *uts, const char *mmc_dev,
> /* Enable the cros bootmeth if needed */
> if (IS_ENABLED(CONFIG_BOOTMETH_CROS) && bind_cros_android) {
> ut_assertok(uclass_first_device_err(UCLASS_BOOTSTD, &bootstd));
> - ut_assertok(device_bind(bootstd, DM_DRIVER_REF(bootmeth_cros),
> - "cros", 0, ofnode_null(), &dev));
> + /* Only bind the bootmeth once to avoid duplicate scan results */
> + if (device_find_child_by_name(bootstd, "cros", &dev) == -ENODEV) {
> + ut_assertok(device_bind(bootstd, DM_DRIVER_REF(bootmeth_cros),
> + "cros", 0, ofnode_null(), &dev));
> + }
> }
>
> /* Enable the android bootmeths if needed */
> if (IS_ENABLED(CONFIG_BOOTMETH_ANDROID) && bind_cros_android) {
> ut_assertok(uclass_first_device_err(UCLASS_BOOTSTD, &bootstd));
> - ut_assertok(device_bind(bootstd, DM_DRIVER_REF(bootmeth_android),
> - "android", 0, ofnode_null(), &dev));
> + /* Only bind the bootmeth once to avoid duplicate scan results */
> + if (device_find_child_by_name(bootstd, "android", &dev) == -ENODEV) {
> + ut_assertok(device_bind(bootstd, DM_DRIVER_REF(bootmeth_android),
> + "android", 0, ofnode_null(), &dev));
> + }
> }
>
> /* Change the order to include the device */
>
> ---
> base-commit: dc1859f8d2ac3faaa5e2e1d465ec4bd8980520a5
> change-id: 20241121-bootstd-test-fix-multiple-bind-159bd2523414
>
> Best regards,
> --
> Mattijs Korpershoek <mkorpershoek@baylibre.com>
>
Instead of this, could you add flags to the test to tell it to reset
driver mode?
UTF_DM | UTF_SCAN_FDT
Regards,
Simon
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] test: boot: Bind bootmeth_{cros, android} only once per run
2024-11-22 13:37 ` [PATCH] test: boot: Bind bootmeth_{cros, android} " Simon Glass
@ 2024-11-22 14:05 ` Mattijs Korpershoek
0 siblings, 0 replies; 3+ messages in thread
From: Mattijs Korpershoek @ 2024-11-22 14:05 UTC (permalink / raw)
To: Simon Glass; +Cc: Tom Rini, Guillaume La Roque, u-boot
Hi Simon,
On ven., nov. 22, 2024 at 06:37, Simon Glass <sjg@chromium.org> wrote:
> Hi Mattijs,
>
> On Thu, 21 Nov 2024 at 08:03, Mattijs Korpershoek
> <mkorpershoek@baylibre.com> wrote:
>>
>> We make fewer calls to dm_test_restore() since
>> commit fbdac8155c89 ("test: Expand implementation of ut_list_has_dm_tests()")
>>
>> Because of this some valid test combinations are now broken:
>>
>> $ ./test/py/test.py --bd sandbox --build -k test_ut
>> $ ./test/py/test.py --bd sandbox --build -k "bootflow_android or bootflow_cros"
>>
>> Shows:
>>
>> Expected ' 2 cros ready mmc 4 mmc5.bootdev.part_4 ',
>> got ' 2 cros ready mmc 2 mmc5.bootdev.part_2 '
>>
>> Here prep_mmc_bootdev() is called twice and it will bind bootmeth_cros twice.
>>
>> Since bootmeth_cros is bound twice, 'bootflow scan' will find 2x the
>> expected bootflows.
>>
>> Before
>> commit fbdac8155c89 ("test: Expand implementation of ut_list_has_dm_tests()")
>> this did not happen because a cleanup was called each time.
>>
>> Check if the bootstd already has a "cros" or "android" driver and only bind it
>> if it does not exist.
>>
>> Fixes: fbdac8155c89 ("test: Expand implementation of ut_list_has_dm_tests()")
>> Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
>> ---
>> Initially, this was found when merging some Android bootflow patches
>> from [1].
>>
>> However, this can be reproduced on 'next' as well.
>>
>> Here is the test output that's broken (we have 4 cros bootmethods instead of 2)
>>
>> => ut bootstd bootflow_cros
>> Test: bootflow_cros: bootflow.c
>> Showing all bootflows
>> Seq Method State Uclass Part Name Filename
>> --- ----------- ------ -------- ---- ------------------------ ----------------
>> 0 extlinux ready mmc 1 mmc1.bootdev.part_1 /extlinux/extlinux.conf
>> 1 cros ready mmc 2 mmc5.bootdev.part_2
>> 2 cros ready mmc 2 mmc5.bootdev.part_2
>> 3 cros ready mmc 4 mmc5.bootdev.part_4
>> 4 cros ready mmc 4 mmc5.bootdev.part_4
>> --- ----------- ------ -------- ---- ------------------------ ----------------
>> (5 bootflows, 5 valid)
>> test/boot/bootflow.c:1192, bootflow_cros(): console:
>> Expected ' 2 cros ready mmc 4 mmc5.bootdev.part_4 ',
>> got ' 2 cros ready mmc 2 mmc5.bootdev.part_2 '
>> Test bootflow_cros failed 1 times
>>
>> [1] https://lore.kernel.org/r/all/87mshvxhq3.fsf@baylibre.com/
>> ---
>> test/boot/bootflow.c | 14 ++++++++++----
>> 1 file changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/test/boot/bootflow.c b/test/boot/bootflow.c
>> index 9397328609d0..835d2e6e35a6 100644
>> --- a/test/boot/bootflow.c
>> +++ b/test/boot/bootflow.c
>> @@ -552,15 +552,21 @@ static int prep_mmc_bootdev(struct unit_test_state *uts, const char *mmc_dev,
>> /* Enable the cros bootmeth if needed */
>> if (IS_ENABLED(CONFIG_BOOTMETH_CROS) && bind_cros_android) {
>> ut_assertok(uclass_first_device_err(UCLASS_BOOTSTD, &bootstd));
>> - ut_assertok(device_bind(bootstd, DM_DRIVER_REF(bootmeth_cros),
>> - "cros", 0, ofnode_null(), &dev));
>> + /* Only bind the bootmeth once to avoid duplicate scan results */
>> + if (device_find_child_by_name(bootstd, "cros", &dev) == -ENODEV) {
>> + ut_assertok(device_bind(bootstd, DM_DRIVER_REF(bootmeth_cros),
>> + "cros", 0, ofnode_null(), &dev));
>> + }
>> }
>>
>> /* Enable the android bootmeths if needed */
>> if (IS_ENABLED(CONFIG_BOOTMETH_ANDROID) && bind_cros_android) {
>> ut_assertok(uclass_first_device_err(UCLASS_BOOTSTD, &bootstd));
>> - ut_assertok(device_bind(bootstd, DM_DRIVER_REF(bootmeth_android),
>> - "android", 0, ofnode_null(), &dev));
>> + /* Only bind the bootmeth once to avoid duplicate scan results */
>> + if (device_find_child_by_name(bootstd, "android", &dev) == -ENODEV) {
>> + ut_assertok(device_bind(bootstd, DM_DRIVER_REF(bootmeth_android),
>> + "android", 0, ofnode_null(), &dev));
>> + }
>> }
>>
>> /* Change the order to include the device */
>>
>> ---
>> base-commit: dc1859f8d2ac3faaa5e2e1d465ec4bd8980520a5
>> change-id: 20241121-bootstd-test-fix-multiple-bind-159bd2523414
>>
>> Best regards,
>> --
>> Mattijs Korpershoek <mkorpershoek@baylibre.com>
>>
>
> Instead of this, could you add flags to the test to tell it to reset
> driver mode?
>
> UTF_DM | UTF_SCAN_FDT
That works as well, and seems like a cleaner approach. Thanks a lot for
the suggestion.
Will send a v2.
>
> Regards,
> Simon
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-11-22 14:06 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-11-21 15:02 [PATCH] test: boot: Bind bootmeth_{cros,android} only once per run Mattijs Korpershoek
2024-11-22 13:37 ` [PATCH] test: boot: Bind bootmeth_{cros, android} " Simon Glass
2024-11-22 14:05 ` Mattijs Korpershoek
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.