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