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

[Simon Glass]

Hi Mattijs,

On Wed, Jan 3, 2024 at 5:41 AM Mattijs Korpershoek
<mkorpershoek at baylibre.com> wrote:
>
> Hi Simon,
>
> On Tue, Jan 02, 2024 at 07:06, Simon Glass <sjg at chromium.org> wrote:
>
> > Hi Mattijs,
> >
> > On Tue, Jan 2, 2024 at 2:52 AM Mattijs Korpershoek
> > <mkorpershoek at baylibre.com> wrote:
> >>
> >> Hi Simon, Svyatoslav,
> >>
> >> On Thu, Dec 28, 2023 at 21:52, Svyatoslav Ryhel <clamor95 at gmail.com> wrote:
> >>
> >> > чт, 28 груд. 2023 р. о 21:48 Simon Glass <sjg at chromium.org> пише:
> >> >>
> >> >> On Thu, Dec 28, 2023 at 6:02 PM Svyatoslav Ryhel <clamor95 at gmail.com> wrote:
> >> >> >
> >> >> > From: Ion Agorria <ion at 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 at agorria.com>
> >> >> > Signed-off-by: Svyatoslav Ryhel <clamor95 at gmail.com>
> >> >> > Reviewed-by: Mattijs Korpershoek <mkorpershoek at baylibre.com>
> >> >> > ---
> >> >> >  common/console.c | 3 +++
> >> >> >  1 file changed, 3 insertions(+)
> >> >>
> >> >> Reviewed-by: Simon Glass <sjg at 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?


>
> 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