Re: [PATCH] test: boot: Bind bootmeth_{cros, android} only once per run

[Mattijs Korpershoek <mkorpershoek@baylibre.com>]

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