Re: [PATCH v6 4/6] common: console: record console from the beginning

[Mattijs Korpershoek <mkorpershoek@baylibre.com>]

Hi Simon,

On mer., janv. 03, 2024 at 18:38, Simon Glass <sjg@chromium.org> wrote:

> Hi Mattijs,
>
> On Wed, Jan 3, 2024 at 5:41 AM Mattijs Korpershoek
> <mkorpershoek@baylibre.com> wrote:
>>
>> Hi Simon,
>>
>> On Tue, Jan 02, 2024 at 07:06, Simon Glass <sjg@chromium.org> wrote:
>>
>> > Hi Mattijs,
>> >
>> > On Tue, Jan 2, 2024 at 2:52 AM Mattijs Korpershoek
>> > <mkorpershoek@baylibre.com> wrote:
>> >>
>> >> Hi Simon, Svyatoslav,
>> >>
>> >> On Thu, Dec 28, 2023 at 21:52, Svyatoslav Ryhel <clamor95@gmail.com> wrote:
>> >>
>> >> > чт, 28 груд. 2023 р. о 21:48 Simon Glass <sjg@chromium.org> пише:
>> >> >>
>> >> >> On Thu, Dec 28, 2023 at 6:02 PM Svyatoslav Ryhel <clamor95@gmail.com> wrote:
>> >> >> >
>> >> >> > From: Ion Agorria <ion@agorria.com>
>> >> >> >
>> >> >> > Set flag to enable console record on console_record_init
>> >> >> > and not only on console_record_reset_enable. This fixes
>> >> >> > missing start of U-Boot log for fastboot oem console
>> >> >> > command.
>> >> >> >
>> >> >> > Signed-off-by: Ion Agorria <ion@agorria.com>
>> >> >> > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
>> >> >> > Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
>> >> >> > ---
>> >> >> >  common/console.c | 3 +++
>> >> >> >  1 file changed, 3 insertions(+)
>> >> >>
>> >> >> Reviewed-by: Simon Glass <sjg@chromium.org>
>> >> >>
>> >> >> OK, I can see the use of this...but I wonder if we can now get rid of
>> >> >> the same line of code from console_record_reset_enable() ?
>> >> >>
>> >> >
>> >> > Interesting question but let's leave it to a dedicated patch :)
>> >>
>> >> I've looked a little more into to this, and I'm not so sure we can get
>> >> rid of the gd->flags |= GD_FLG_RECORD; in console_record_reset_enable().
>> >>
>> >> Removing the flag seems to break quite some tests in
>> >> test/py/tests/test_ut.py.
>> >>
>> >> The breakage can be explained that various unit tests clear the
>> >> GD_FLG_RECORD with:
>> >>
>> >>         gd->flags &= ~GD_FLG_RECORD;
>> >>
>> >> Therefore, I would suggest we keep the flag in
>> >> console_record_reset_enable().
>> >
>> > From my look all of those are not needed in tests, i.e. are bugs. If
>> > you are able to do a patch to remove those lines, it would avoid the
>> > confusion.
>>
>> With gd->flags |= GD_FLG_RECORD removed from
>> console_record_reset_enable(),
>>
>> When I run:
>> $ ./test/py/test.py --bd sandbox --build -k ut
>>
>> It produces this list of the the tests that fail:
>> https://paste.debian.net/1302906/
>>
>> I can also reproduce individually with a bootflow test, for example:
>> $ ./test/py/test.py --bd sandbox --build -k ut_bootstd_bootflow_cmd_boot
>>
>> Produces:
>> https://paste.debian.net/1302907/
>>
>> I did not investigate more on detail but it seems not trivial to me.
>
> Did you add UT_TESTF_CONSOLE_REC to each test as well?

Here are the steps I did:
1. Take base commit dffa6d0210f5 ("Merge tag 'dm-next-1124' of https://source.denx.de/u-boot/custodians/u-boot-dm into next")
2. Apply series: b4 shazam 20231228180154.50473-1-clamor95@gmail.com
3. Run bootflow_cmd_boot test, which passes.
4. Apply the following diff:

diff --git a/common/console.c b/common/console.c
index cad65891fc9f..1bff80029266 100644
--- a/common/console.c
+++ b/common/console.c
@@ -837,7 +837,6 @@ void console_record_reset(void)
 int console_record_reset_enable(void)
 {
        console_record_reset();
-       gd->flags |= GD_FLG_RECORD;
 
        return 0;
 }
diff --git a/test/boot/bootflow.c b/test/boot/bootflow.c
index 104f49deef2a..6a98f42f2707 100644
--- a/test/boot/bootflow.c
+++ b/test/boot/bootflow.c
@@ -482,7 +482,6 @@ BOOTSTD_TEST(bootflow_scan_glob_bootmeth, UT_TESTF_DM | UT_TESTF_SCAN_FDT);
 /* Check 'bootflow boot' to boot a selected bootflow */
 static int bootflow_cmd_boot(struct unit_test_state *uts)
 {
-       console_record_reset_enable();
        ut_assertok(run_command("bootdev select 1", 0));
        ut_assert_console_end();
        ut_assertok(run_command("bootflow scan", 0));
@@ -506,7 +505,7 @@ static int bootflow_cmd_boot(struct unit_test_state *uts)
 
        return 0;
 }
-BOOTSTD_TEST(bootflow_cmd_boot, UT_TESTF_DM | UT_TESTF_SCAN_FDT);
+BOOTSTD_TEST(bootflow_cmd_boot, UT_TESTF_DM | UT_TESTF_SCAN_FDT | UT_TESTF_CONSOLE_REC);
 
 /**
  * prep_mmc_bootdev() - Set up an mmc bootdev so we can access other distros

5. Run bootflow_cmd_boot test, which now fails.

>
>
>>
>> I can continue the investigation in the coming weeks but I would like
>> to apply this series this week.
>>
>> >
>> > Also, by setting UT_TESTF_CONSOLE_REC for a test, console recording is
>> > set up automatically, so the console_record_reset_enable() is not
>> > needed at the start of the test.
>>
>> I was not aware of that, thank you.
>>
>> >
>> > Regards,
>> > Simon