| Message ID | 20240815202424.766778-11-sjg@chromium.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | efi: Add a test for EFI bootmeth | expand |
Hi Simon, On Thu, 15 Aug 2024 at 23:24, Simon Glass <sjg@chromium.org> wrote: > > We don't want ANSI characters written in tests since it is a pain to > check the output with ut_assert_nextline() et al. > > Provide a way to tests to request that ANSI characters not be sent. > > Add a proper function comment while we are here, to encourage others. > > Signed-off-by: Simon Glass <sjg@chromium.org> > --- > > (no changes since v1) > > include/efi_loader.h | 21 ++++++++++++++++++++- > lib/efi_loader/efi_console.c | 26 +++++++++++++++++--------- > 2 files changed, 37 insertions(+), 10 deletions(-) > > diff --git a/include/efi_loader.h b/include/efi_loader.h > index f84852e384f..82b90ee0f1d 100644 > --- a/include/efi_loader.h > +++ b/include/efi_loader.h > @@ -531,8 +531,27 @@ efi_status_t efi_bootmgr_delete_boot_option(u16 boot_index); > efi_status_t efi_bootmgr_run(void *fdt); > /* search the boot option index in BootOrder */ > bool efi_search_bootorder(u16 *bootorder, efi_uintn_t num, u32 target, u32 *index); > -/* Set up console modes */ > + > +/** > + * efi_setup_console_size() - update the mode table. > + * > + * By default the only mode available is 80x25. If the console has at least 50 > + * lines, enable mode 80x50. If we can query the console size and it is neither > + * 80x25 nor 80x50, set it as an additional mode. > + */ > void efi_setup_console_size(void); > + > +/** > + * efi_console_set_ansi() - Set whether ANSI characters should be emitted > + * > + * These characters mess up tests which use ut_assert_nextline(). Call this > + * function to tell efi_loader not to emit these characters when starting up the > + * terminal > + * > + * @allow_ansi: Allow emitting ANSI characters > + */ > +void efi_console_set_ansi(bool allow_ansi); > + > /* Set up load options from environment variable */ > efi_status_t efi_env_set_load_options(efi_handle_t handle, const char *env_var, > u16 **load_options); > diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c > index cea50c748aa..569fc9199bc 100644 > --- a/lib/efi_loader/efi_console.c > +++ b/lib/efi_loader/efi_console.c > @@ -30,6 +30,17 @@ struct cout_mode { > > __maybe_unused static struct efi_object uart_obj; > > +/* > + * suppress emission of ANSI codes for use by unit tests. Leave it as 0 for the > + * default behaviour > + */ > +static bool no_ansi; > + > +void efi_console_set_ansi(bool allow_ansi) > +{ > + no_ansi = !allow_ansi; > +} > + > static struct cout_mode efi_cout_modes[] = { > /* EFI Mode 0 is 80x25 and always present */ > { > @@ -348,13 +359,6 @@ static int __maybe_unused query_vidconsole(int *rows, int *cols) > return 0; > } > > -/** > - * efi_setup_console_size() - update the mode table. > - * > - * By default the only mode available is 80x25. If the console has at least 50 > - * lines, enable mode 80x50. If we can query the console size and it is neither > - * 80x25 nor 80x50, set it as an additional mode. > - */ > void efi_setup_console_size(void) > { > int rows = 25, cols = 80; > @@ -362,8 +366,12 @@ void efi_setup_console_size(void) > > if (IS_ENABLED(CONFIG_VIDEO)) > ret = query_vidconsole(&rows, &cols); > - if (ret) > - ret = query_console_serial(&rows, &cols); > + if (ret) { > + if (no_ansi) What are you trying to do here? The function you are skipping just queries the number of columns and rows. The defaults on the function are 80x25, so the setup will continue Thanks /Ilias > + ret = 0; > + else > + ret = query_console_serial(&rows, &cols); > + } > if (ret) > return; > > -- > 2.34.1 >
Hi Ilias, On Mon, 26 Aug 2024 at 05:47, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Hi Simon, > > On Thu, 15 Aug 2024 at 23:24, Simon Glass <sjg@chromium.org> wrote: > > > > We don't want ANSI characters written in tests since it is a pain to > > check the output with ut_assert_nextline() et al. > > > > Provide a way to tests to request that ANSI characters not be sent. > > > > Add a proper function comment while we are here, to encourage others. > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > --- > > > > (no changes since v1) > > > > include/efi_loader.h | 21 ++++++++++++++++++++- > > lib/efi_loader/efi_console.c | 26 +++++++++++++++++--------- > > 2 files changed, 37 insertions(+), 10 deletions(-) > > > > diff --git a/include/efi_loader.h b/include/efi_loader.h > > index f84852e384f..82b90ee0f1d 100644 > > --- a/include/efi_loader.h > > +++ b/include/efi_loader.h > > @@ -531,8 +531,27 @@ efi_status_t efi_bootmgr_delete_boot_option(u16 boot_index); > > efi_status_t efi_bootmgr_run(void *fdt); > > /* search the boot option index in BootOrder */ > > bool efi_search_bootorder(u16 *bootorder, efi_uintn_t num, u32 target, u32 *index); > > -/* Set up console modes */ > > + > > +/** > > + * efi_setup_console_size() - update the mode table. > > + * > > + * By default the only mode available is 80x25. If the console has at least 50 > > + * lines, enable mode 80x50. If we can query the console size and it is neither > > + * 80x25 nor 80x50, set it as an additional mode. > > + */ > > void efi_setup_console_size(void); > > + > > +/** > > + * efi_console_set_ansi() - Set whether ANSI characters should be emitted > > + * > > + * These characters mess up tests which use ut_assert_nextline(). Call this > > + * function to tell efi_loader not to emit these characters when starting up the > > + * terminal > > + * > > + * @allow_ansi: Allow emitting ANSI characters > > + */ > > +void efi_console_set_ansi(bool allow_ansi); > > + > > /* Set up load options from environment variable */ > > efi_status_t efi_env_set_load_options(efi_handle_t handle, const char *env_var, > > u16 **load_options); > > diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c > > index cea50c748aa..569fc9199bc 100644 > > --- a/lib/efi_loader/efi_console.c > > +++ b/lib/efi_loader/efi_console.c > > @@ -30,6 +30,17 @@ struct cout_mode { > > > > __maybe_unused static struct efi_object uart_obj; > > > > +/* > > + * suppress emission of ANSI codes for use by unit tests. Leave it as 0 for the > > + * default behaviour > > + */ > > +static bool no_ansi; > > + > > +void efi_console_set_ansi(bool allow_ansi) > > +{ > > + no_ansi = !allow_ansi; > > +} > > + > > static struct cout_mode efi_cout_modes[] = { > > /* EFI Mode 0 is 80x25 and always present */ > > { > > @@ -348,13 +359,6 @@ static int __maybe_unused query_vidconsole(int *rows, int *cols) > > return 0; > > } > > > > -/** > > - * efi_setup_console_size() - update the mode table. > > - * > > - * By default the only mode available is 80x25. If the console has at least 50 > > - * lines, enable mode 80x50. If we can query the console size and it is neither > > - * 80x25 nor 80x50, set it as an additional mode. > > - */ > > void efi_setup_console_size(void) > > { > > int rows = 25, cols = 80; > > @@ -362,8 +366,12 @@ void efi_setup_console_size(void) > > > > if (IS_ENABLED(CONFIG_VIDEO)) > > ret = query_vidconsole(&rows, &cols); > > - if (ret) > > - ret = query_console_serial(&rows, &cols); > > + if (ret) { > > + if (no_ansi) > > What are you trying to do here? The function you are skipping just > queries the number of columns and rows. The defaults on the function > are 80x25, so the setup will continue Well, the query works by sending out some ANSI characters to the terminal, then checking the result. In a unit test, these characters are captured and the unit test fails when ut_assert_nextline() gets unexpected output. > > Thanks > /Ilias > > + ret = 0; > > + else > > + ret = query_console_serial(&rows, &cols); > > + } > > if (ret) > > return; > > > > -- > > 2.34.1 > > Regards, Simon
On 8/26/24 19:57, Simon Glass wrote: > Hi Ilias, > > On Mon, 26 Aug 2024 at 05:47, Ilias Apalodimas > <ilias.apalodimas@linaro.org> wrote: >> >> Hi Simon, >> >> On Thu, 15 Aug 2024 at 23:24, Simon Glass <sjg@chromium.org> wrote: >>> >>> We don't want ANSI characters written in tests since it is a pain to >>> check the output with ut_assert_nextline() et al. >>> >>> Provide a way to tests to request that ANSI characters not be sent. >>> >>> Add a proper function comment while we are here, to encourage others. >>> >>> Signed-off-by: Simon Glass <sjg@chromium.org> >>> --- >>> >>> (no changes since v1) >>> >>> include/efi_loader.h | 21 ++++++++++++++++++++- >>> lib/efi_loader/efi_console.c | 26 +++++++++++++++++--------- >>> 2 files changed, 37 insertions(+), 10 deletions(-) >>> >>> diff --git a/include/efi_loader.h b/include/efi_loader.h >>> index f84852e384f..82b90ee0f1d 100644 >>> --- a/include/efi_loader.h >>> +++ b/include/efi_loader.h >>> @@ -531,8 +531,27 @@ efi_status_t efi_bootmgr_delete_boot_option(u16 boot_index); >>> efi_status_t efi_bootmgr_run(void *fdt); >>> /* search the boot option index in BootOrder */ >>> bool efi_search_bootorder(u16 *bootorder, efi_uintn_t num, u32 target, u32 *index); >>> -/* Set up console modes */ >>> + >>> +/** >>> + * efi_setup_console_size() - update the mode table. >>> + * >>> + * By default the only mode available is 80x25. If the console has at least 50 >>> + * lines, enable mode 80x50. If we can query the console size and it is neither >>> + * 80x25 nor 80x50, set it as an additional mode. >>> + */ >>> void efi_setup_console_size(void); >>> + >>> +/** >>> + * efi_console_set_ansi() - Set whether ANSI characters should be emitted >>> + * >>> + * These characters mess up tests which use ut_assert_nextline(). Call this >>> + * function to tell efi_loader not to emit these characters when starting up the >>> + * terminal >>> + * >>> + * @allow_ansi: Allow emitting ANSI characters >>> + */ >>> +void efi_console_set_ansi(bool allow_ansi); >>> + >>> /* Set up load options from environment variable */ >>> efi_status_t efi_env_set_load_options(efi_handle_t handle, const char *env_var, >>> u16 **load_options); >>> diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c >>> index cea50c748aa..569fc9199bc 100644 >>> --- a/lib/efi_loader/efi_console.c >>> +++ b/lib/efi_loader/efi_console.c >>> @@ -30,6 +30,17 @@ struct cout_mode { >>> >>> __maybe_unused static struct efi_object uart_obj; >>> >>> +/* >>> + * suppress emission of ANSI codes for use by unit tests. Leave it as 0 for the >>> + * default behaviour >>> + */ >>> +static bool no_ansi; >>> + >>> +void efi_console_set_ansi(bool allow_ansi) >>> +{ >>> + no_ansi = !allow_ansi; >>> +} >>> + >>> static struct cout_mode efi_cout_modes[] = { >>> /* EFI Mode 0 is 80x25 and always present */ >>> { >>> @@ -348,13 +359,6 @@ static int __maybe_unused query_vidconsole(int *rows, int *cols) >>> return 0; >>> } >>> >>> -/** >>> - * efi_setup_console_size() - update the mode table. >>> - * >>> - * By default the only mode available is 80x25. If the console has at least 50 >>> - * lines, enable mode 80x50. If we can query the console size and it is neither >>> - * 80x25 nor 80x50, set it as an additional mode. >>> - */ >>> void efi_setup_console_size(void) >>> { >>> int rows = 25, cols = 80; >>> @@ -362,8 +366,12 @@ void efi_setup_console_size(void) >>> >>> if (IS_ENABLED(CONFIG_VIDEO)) >>> ret = query_vidconsole(&rows, &cols); >>> - if (ret) >>> - ret = query_console_serial(&rows, &cols); >>> + if (ret) { >>> + if (no_ansi) >> >> What are you trying to do here? The function you are skipping just >> queries the number of columns and rows. The defaults on the function >> are 80x25, so the setup will continue > > Well, the query works by sending out some ANSI characters to the > terminal, then checking the result. In a unit test, these characters > are captured and the unit test fails when ut_assert_nextline() gets > unexpected output. We should be able to compile U-Boot with all tests enabled such that it is still usable on real hardware. Your patch is not compatible with this. Please, adjust readline_check() to strip out non-printing ANSI codes. Best regards Heinrich > >> >> Thanks >> /Ilias >>> + ret = 0; >>> + else >>> + ret = query_console_serial(&rows, &cols); >>> + } >>> if (ret) >>> return; >>> >>> -- >>> 2.34.1 >>> > > Regards, > Simon
On Mon, Aug 26, 2024 at 08:16:45PM +0200, Heinrich Schuchardt wrote: > On 8/26/24 19:57, Simon Glass wrote: > > Hi Ilias, > > > > On Mon, 26 Aug 2024 at 05:47, Ilias Apalodimas > > <ilias.apalodimas@linaro.org> wrote: > > > > > > Hi Simon, > > > > > > On Thu, 15 Aug 2024 at 23:24, Simon Glass <sjg@chromium.org> wrote: > > > > > > > > We don't want ANSI characters written in tests since it is a pain to > > > > check the output with ut_assert_nextline() et al. > > > > > > > > Provide a way to tests to request that ANSI characters not be sent. > > > > > > > > Add a proper function comment while we are here, to encourage others. > > > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > > > --- > > > > > > > > (no changes since v1) > > > > > > > > include/efi_loader.h | 21 ++++++++++++++++++++- > > > > lib/efi_loader/efi_console.c | 26 +++++++++++++++++--------- > > > > 2 files changed, 37 insertions(+), 10 deletions(-) > > > > > > > > diff --git a/include/efi_loader.h b/include/efi_loader.h > > > > index f84852e384f..82b90ee0f1d 100644 > > > > --- a/include/efi_loader.h > > > > +++ b/include/efi_loader.h > > > > @@ -531,8 +531,27 @@ efi_status_t efi_bootmgr_delete_boot_option(u16 boot_index); > > > > efi_status_t efi_bootmgr_run(void *fdt); > > > > /* search the boot option index in BootOrder */ > > > > bool efi_search_bootorder(u16 *bootorder, efi_uintn_t num, u32 target, u32 *index); > > > > -/* Set up console modes */ > > > > + > > > > +/** > > > > + * efi_setup_console_size() - update the mode table. > > > > + * > > > > + * By default the only mode available is 80x25. If the console has at least 50 > > > > + * lines, enable mode 80x50. If we can query the console size and it is neither > > > > + * 80x25 nor 80x50, set it as an additional mode. > > > > + */ > > > > void efi_setup_console_size(void); > > > > + > > > > +/** > > > > + * efi_console_set_ansi() - Set whether ANSI characters should be emitted > > > > + * > > > > + * These characters mess up tests which use ut_assert_nextline(). Call this > > > > + * function to tell efi_loader not to emit these characters when starting up the > > > > + * terminal > > > > + * > > > > + * @allow_ansi: Allow emitting ANSI characters > > > > + */ > > > > +void efi_console_set_ansi(bool allow_ansi); > > > > + > > > > /* Set up load options from environment variable */ > > > > efi_status_t efi_env_set_load_options(efi_handle_t handle, const char *env_var, > > > > u16 **load_options); > > > > diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c > > > > index cea50c748aa..569fc9199bc 100644 > > > > --- a/lib/efi_loader/efi_console.c > > > > +++ b/lib/efi_loader/efi_console.c > > > > @@ -30,6 +30,17 @@ struct cout_mode { > > > > > > > > __maybe_unused static struct efi_object uart_obj; > > > > > > > > +/* > > > > + * suppress emission of ANSI codes for use by unit tests. Leave it as 0 for the > > > > + * default behaviour > > > > + */ > > > > +static bool no_ansi; > > > > + > > > > +void efi_console_set_ansi(bool allow_ansi) > > > > +{ > > > > + no_ansi = !allow_ansi; > > > > +} > > > > + > > > > static struct cout_mode efi_cout_modes[] = { > > > > /* EFI Mode 0 is 80x25 and always present */ > > > > { > > > > @@ -348,13 +359,6 @@ static int __maybe_unused query_vidconsole(int *rows, int *cols) > > > > return 0; > > > > } > > > > > > > > -/** > > > > - * efi_setup_console_size() - update the mode table. > > > > - * > > > > - * By default the only mode available is 80x25. If the console has at least 50 > > > > - * lines, enable mode 80x50. If we can query the console size and it is neither > > > > - * 80x25 nor 80x50, set it as an additional mode. > > > > - */ > > > > void efi_setup_console_size(void) > > > > { > > > > int rows = 25, cols = 80; > > > > @@ -362,8 +366,12 @@ void efi_setup_console_size(void) > > > > > > > > if (IS_ENABLED(CONFIG_VIDEO)) > > > > ret = query_vidconsole(&rows, &cols); > > > > - if (ret) > > > > - ret = query_console_serial(&rows, &cols); > > > > + if (ret) { > > > > + if (no_ansi) > > > > > > What are you trying to do here? The function you are skipping just > > > queries the number of columns and rows. The defaults on the function > > > are 80x25, so the setup will continue > > > > Well, the query works by sending out some ANSI characters to the > > terminal, then checking the result. In a unit test, these characters > > are captured and the unit test fails when ut_assert_nextline() gets > > unexpected output. > > We should be able to compile U-Boot with all tests enabled such that it > is still usable on real hardware. Please keep in mind "real hardware" is where this is the biggest problem today. I don't run a bunch of the EFI tests on real hardware because the ANSI stuff leads to spurious failures, and so it's best to just turn that off rather than re-run the test and see if it passes the next time.
diff --git a/include/efi_loader.h b/include/efi_loader.h index f84852e384f..82b90ee0f1d 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -531,8 +531,27 @@ efi_status_t efi_bootmgr_delete_boot_option(u16 boot_index); efi_status_t efi_bootmgr_run(void *fdt); /* search the boot option index in BootOrder */ bool efi_search_bootorder(u16 *bootorder, efi_uintn_t num, u32 target, u32 *index); -/* Set up console modes */ + +/** + * efi_setup_console_size() - update the mode table. + * + * By default the only mode available is 80x25. If the console has at least 50 + * lines, enable mode 80x50. If we can query the console size and it is neither + * 80x25 nor 80x50, set it as an additional mode. + */ void efi_setup_console_size(void); + +/** + * efi_console_set_ansi() - Set whether ANSI characters should be emitted + * + * These characters mess up tests which use ut_assert_nextline(). Call this + * function to tell efi_loader not to emit these characters when starting up the + * terminal + * + * @allow_ansi: Allow emitting ANSI characters + */ +void efi_console_set_ansi(bool allow_ansi); + /* Set up load options from environment variable */ efi_status_t efi_env_set_load_options(efi_handle_t handle, const char *env_var, u16 **load_options); diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c index cea50c748aa..569fc9199bc 100644 --- a/lib/efi_loader/efi_console.c +++ b/lib/efi_loader/efi_console.c @@ -30,6 +30,17 @@ struct cout_mode { __maybe_unused static struct efi_object uart_obj; +/* + * suppress emission of ANSI codes for use by unit tests. Leave it as 0 for the + * default behaviour + */ +static bool no_ansi; + +void efi_console_set_ansi(bool allow_ansi) +{ + no_ansi = !allow_ansi; +} + static struct cout_mode efi_cout_modes[] = { /* EFI Mode 0 is 80x25 and always present */ { @@ -348,13 +359,6 @@ static int __maybe_unused query_vidconsole(int *rows, int *cols) return 0; } -/** - * efi_setup_console_size() - update the mode table. - * - * By default the only mode available is 80x25. If the console has at least 50 - * lines, enable mode 80x50. If we can query the console size and it is neither - * 80x25 nor 80x50, set it as an additional mode. - */ void efi_setup_console_size(void) { int rows = 25, cols = 80; @@ -362,8 +366,12 @@ void efi_setup_console_size(void) if (IS_ENABLED(CONFIG_VIDEO)) ret = query_vidconsole(&rows, &cols); - if (ret) - ret = query_console_serial(&rows, &cols); + if (ret) { + if (no_ansi) + ret = 0; + else + ret = query_console_serial(&rows, &cols); + } if (ret) return;
We don't want ANSI characters written in tests since it is a pain to check the output with ut_assert_nextline() et al. Provide a way to tests to request that ANSI characters not be sent. Add a proper function comment while we are here, to encourage others. Signed-off-by: Simon Glass <sjg@chromium.org> --- (no changes since v1) include/efi_loader.h | 21 ++++++++++++++++++++- lib/efi_loader/efi_console.c | 26 +++++++++++++++++--------- 2 files changed, 37 insertions(+), 10 deletions(-)