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

[Mattijs Korpershoek <mkorpershoek@baylibre.com>]

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.

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