Re: [PATCH v6 4/6] common: console: record console from the beginning
Hi Simon,
On mer., janv. 03, 2024 at 18:38, Simon Glass wrote:
> Hi Mattijs,
>
> On Wed, Jan 3, 2024 at 5:41 AM Mattijs Korpershoek
> wrote:
>>
>> Hi Simon,
>>
>> On Tue, Jan 02, 2024 at 07:06, Simon Glass wrote:
>>
>> > Hi Mattijs,
>> >
>> > On Tue, Jan 2, 2024 at 2:52 AM Mattijs Korpershoek
>> > wrote:
>> >>
>> >> Hi Simon, Svyatoslav,
>> >>
>> >> On Thu, Dec 28, 2023 at 21:52, Svyatoslav Ryhel
>> >> wrote:
>> >>
>> >> > чт, 28 груд. 2023 р. о 21:48 Simon Glass пише:
>> >> >>
>> >> >> On Thu, Dec 28, 2023 at 6:02 PM Svyatoslav Ryhel
>> >> >> wrote:
>> >> >> >
>> >> >> > From: Ion Agorria
>> >> >> >
>> >> >> > 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
>> >> >> > Signed-off-by: Svyatoslav Ryhel
>> >> >> > Reviewed-by: Mattijs Korpershoek
>> >> >> > ---
>> >> >> > common/console.c | 3 +++
>> >> >> > 1 file changed, 3 insertions(+)
>> >> >>
>> >> >> Reviewed-by: Simon Glass
>> >> >>
>> >> >> 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-clamo...@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
Re: [PATCH v6 4/6] common: console: record console from the beginning
Hi Mattijs, On Wed, Jan 3, 2024 at 5:41 AM Mattijs Korpershoek wrote: > > Hi Simon, > > On Tue, Jan 02, 2024 at 07:06, Simon Glass wrote: > > > Hi Mattijs, > > > > On Tue, Jan 2, 2024 at 2:52 AM Mattijs Korpershoek > > wrote: > >> > >> Hi Simon, Svyatoslav, > >> > >> On Thu, Dec 28, 2023 at 21:52, Svyatoslav Ryhel wrote: > >> > >> > чт, 28 груд. 2023 р. о 21:48 Simon Glass пише: > >> >> > >> >> On Thu, Dec 28, 2023 at 6:02 PM Svyatoslav Ryhel > >> >> wrote: > >> >> > > >> >> > From: Ion Agorria > >> >> > > >> >> > 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 > >> >> > Signed-off-by: Svyatoslav Ryhel > >> >> > Reviewed-by: Mattijs Korpershoek > >> >> > --- > >> >> > common/console.c | 3 +++ > >> >> > 1 file changed, 3 insertions(+) > >> >> > >> >> Reviewed-by: Simon Glass > >> >> > >> >> 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
Re: [PATCH v6 4/6] common: console: record console from the beginning
Hi Simon, On Tue, Jan 02, 2024 at 07:06, Simon Glass wrote: > Hi Mattijs, > > On Tue, Jan 2, 2024 at 2:52 AM Mattijs Korpershoek > wrote: >> >> Hi Simon, Svyatoslav, >> >> On Thu, Dec 28, 2023 at 21:52, Svyatoslav Ryhel wrote: >> >> > чт, 28 груд. 2023 р. о 21:48 Simon Glass пише: >> >> >> >> On Thu, Dec 28, 2023 at 6:02 PM Svyatoslav Ryhel >> >> wrote: >> >> > >> >> > From: Ion Agorria >> >> > >> >> > 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 >> >> > Signed-off-by: Svyatoslav Ryhel >> >> > Reviewed-by: Mattijs Korpershoek >> >> > --- >> >> > common/console.c | 3 +++ >> >> > 1 file changed, 3 insertions(+) >> >> >> >> Reviewed-by: Simon Glass >> >> >> >> 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
Re: [PATCH v6 4/6] common: console: record console from the beginning
Hi Mattijs, On Tue, Jan 2, 2024 at 2:52 AM Mattijs Korpershoek wrote: > > Hi Simon, Svyatoslav, > > On Thu, Dec 28, 2023 at 21:52, Svyatoslav Ryhel wrote: > > > чт, 28 груд. 2023 р. о 21:48 Simon Glass пише: > >> > >> On Thu, Dec 28, 2023 at 6:02 PM Svyatoslav Ryhel > >> wrote: > >> > > >> > From: Ion Agorria > >> > > >> > 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 > >> > Signed-off-by: Svyatoslav Ryhel > >> > Reviewed-by: Mattijs Korpershoek > >> > --- > >> > common/console.c | 3 +++ > >> > 1 file changed, 3 insertions(+) > >> > >> Reviewed-by: Simon Glass > >> > >> 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. 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. Regards, Simon
Re: [PATCH v6 4/6] common: console: record console from the beginning
Hi Simon, Svyatoslav, On Thu, Dec 28, 2023 at 21:52, Svyatoslav Ryhel wrote: > чт, 28 груд. 2023 р. о 21:48 Simon Glass пише: >> >> On Thu, Dec 28, 2023 at 6:02 PM Svyatoslav Ryhel wrote: >> > >> > From: Ion Agorria >> > >> > 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 >> > Signed-off-by: Svyatoslav Ryhel >> > Reviewed-by: Mattijs Korpershoek >> > --- >> > common/console.c | 3 +++ >> > 1 file changed, 3 insertions(+) >> >> Reviewed-by: Simon Glass >> >> 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(). > > Best Regards, > Svyatoslav R. > >> > >> > diff --git a/common/console.c b/common/console.c >> > index 6f2089caa0..e6d7ebe935 100644 >> > --- a/common/console.c >> > +++ b/common/console.c >> > @@ -821,6 +821,9 @@ int console_record_init(void) >> > ret = membuff_new((struct membuff *)&gd->console_in, >> > CONFIG_CONSOLE_RECORD_IN_SIZE); >> > >> > + /* Start recording from the beginning */ >> > + gd->flags |= GD_FLG_RECORD; >> > + >> > return ret; >> > } >> > >> > -- >> > 2.40.1 >> > >> >> Regards, >> Simon
Re: [PATCH v6 4/6] common: console: record console from the beginning
чт, 28 груд. 2023 р. о 21:48 Simon Glass пише: > > On Thu, Dec 28, 2023 at 6:02 PM Svyatoslav Ryhel wrote: > > > > From: Ion Agorria > > > > 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 > > Signed-off-by: Svyatoslav Ryhel > > Reviewed-by: Mattijs Korpershoek > > --- > > common/console.c | 3 +++ > > 1 file changed, 3 insertions(+) > > Reviewed-by: Simon Glass > > 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 :) Best Regards, Svyatoslav R. > > > > diff --git a/common/console.c b/common/console.c > > index 6f2089caa0..e6d7ebe935 100644 > > --- a/common/console.c > > +++ b/common/console.c > > @@ -821,6 +821,9 @@ int console_record_init(void) > > ret = membuff_new((struct membuff *)&gd->console_in, > > CONFIG_CONSOLE_RECORD_IN_SIZE); > > > > + /* Start recording from the beginning */ > > + gd->flags |= GD_FLG_RECORD; > > + > > return ret; > > } > > > > -- > > 2.40.1 > > > > Regards, > Simon
Re: [PATCH v6 4/6] common: console: record console from the beginning
On Thu, Dec 28, 2023 at 6:02 PM Svyatoslav Ryhel wrote: > > From: Ion Agorria > > 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 > Signed-off-by: Svyatoslav Ryhel > Reviewed-by: Mattijs Korpershoek > --- > common/console.c | 3 +++ > 1 file changed, 3 insertions(+) Reviewed-by: Simon Glass 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() ? > > diff --git a/common/console.c b/common/console.c > index 6f2089caa0..e6d7ebe935 100644 > --- a/common/console.c > +++ b/common/console.c > @@ -821,6 +821,9 @@ int console_record_init(void) > ret = membuff_new((struct membuff *)&gd->console_in, > CONFIG_CONSOLE_RECORD_IN_SIZE); > > + /* Start recording from the beginning */ > + gd->flags |= GD_FLG_RECORD; > + > return ret; > } > > -- > 2.40.1 > Regards, Simon
[PATCH v6 4/6] common: console: record console from the beginning
From: Ion Agorria 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 Signed-off-by: Svyatoslav Ryhel Reviewed-by: Mattijs Korpershoek --- common/console.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/common/console.c b/common/console.c index 6f2089caa0..e6d7ebe935 100644 --- a/common/console.c +++ b/common/console.c @@ -821,6 +821,9 @@ int console_record_init(void) ret = membuff_new((struct membuff *)&gd->console_in, CONFIG_CONSOLE_RECORD_IN_SIZE); + /* Start recording from the beginning */ + gd->flags |= GD_FLG_RECORD; + return ret; } -- 2.40.1
