| Message ID | 20180429180246.18287-1-xypron.glpk@gmx.de |
|---|---|
| State | Accepted |
| Commit | a4aa7bef3c66e473ab4671b0edef1bb66488a032 |
| Delegated to: | Alexander Graf |
| Headers | show |
| Series | [U-Boot,1/1] efi_loader: always check parameters in efi_cout_query_mode() | expand |
On Sun, Apr 29, 2018 at 08:02:46PM +0200, Heinrich Schuchardt wrote: > If we cannot determine the size of the serial terminal we still have > to check the parameters of efi_cout_query_mode(). > > Querying the size of the serial terminal drains the keyboard buffer. > So make sure we do this during the initialization and not in the midst > of an EFI application. > > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > --- > lib/efi_loader/efi_console.c | 90 +++++++++++++++++++++++--------------------- > 1 file changed, 48 insertions(+), 42 deletions(-) > > diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c > index d687362a50..f1b8db55d6 100644 > --- a/lib/efi_loader/efi_console.c > +++ b/lib/efi_loader/efi_console.c > @@ -13,8 +13,6 @@ > #include <stdio_dev.h> > #include <video_console.h> > > -static bool console_size_queried; > - > #define EFI_COUT_MODE_2 2 > #define EFI_MAX_COUT_MODE 3 > > @@ -206,6 +204,51 @@ static int query_console_serial(int *rows, int *cols) > return 0; > } > > +/* > + * 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. > + */ > +static void query_console_size(void) > +{ > + const char *stdout_name = env_get("stdout"); > + int rows, cols; > + > + if (stdout_name && !strcmp(stdout_name, "vidconsole") && > + IS_ENABLED(CONFIG_DM_VIDEO)) { > + struct stdio_dev *stdout_dev = > + stdio_get_by_name("vidconsole"); > + struct udevice *dev = stdout_dev->priv; > + struct vidconsole_priv *priv = > + dev_get_uclass_priv(dev); > + rows = priv->rows; > + cols = priv->cols; > + } else if (query_console_serial(&rows, &cols)) { > + return; > + } > + > + /* Test if we can have Mode 1 */ > + if (cols >= 80 && rows >= 50) { > + efi_cout_modes[1].present = 1; > + efi_con_mode.max_mode = 2; > + } > + > + /* > + * Install our mode as mode 2 if it is different > + * than mode 0 or 1 and set it as the currently selected mode > + */ > + if (!cout_mode_matches(&efi_cout_modes[0], rows, cols) && > + !cout_mode_matches(&efi_cout_modes[1], rows, cols)) { > + efi_cout_modes[EFI_COUT_MODE_2].columns = cols; > + efi_cout_modes[EFI_COUT_MODE_2].rows = rows; > + efi_cout_modes[EFI_COUT_MODE_2].present = 1; > + efi_con_mode.max_mode = EFI_MAX_COUT_MODE; > + efi_con_mode.mode = EFI_COUT_MODE_2; > + } > +} > + > static efi_status_t EFIAPI efi_cout_query_mode( > struct efi_simple_text_output_protocol *this, > unsigned long mode_number, unsigned long *columns, > @@ -213,52 +256,12 @@ static efi_status_t EFIAPI efi_cout_query_mode( > { > EFI_ENTRY("%p, %ld, %p, %p", this, mode_number, columns, rows); > > - if (!console_size_queried) { > - const char *stdout_name = env_get("stdout"); > - int rows, cols; > - > - console_size_queried = true; > - > - if (stdout_name && !strcmp(stdout_name, "vidconsole") && > - IS_ENABLED(CONFIG_DM_VIDEO)) { > - struct stdio_dev *stdout_dev = > - stdio_get_by_name("vidconsole"); > - struct udevice *dev = stdout_dev->priv; > - struct vidconsole_priv *priv = > - dev_get_uclass_priv(dev); > - rows = priv->rows; > - cols = priv->cols; > - } else if (query_console_serial(&rows, &cols)) { > - goto out; > - } > - > - /* Test if we can have Mode 1 */ > - if (cols >= 80 && rows >= 50) { > - efi_cout_modes[1].present = 1; > - efi_con_mode.max_mode = 2; > - } > - > - /* > - * Install our mode as mode 2 if it is different > - * than mode 0 or 1 and set it as the currently selected mode > - */ > - if (!cout_mode_matches(&efi_cout_modes[0], rows, cols) && > - !cout_mode_matches(&efi_cout_modes[1], rows, cols)) { > - efi_cout_modes[EFI_COUT_MODE_2].columns = cols; > - efi_cout_modes[EFI_COUT_MODE_2].rows = rows; > - efi_cout_modes[EFI_COUT_MODE_2].present = 1; > - efi_con_mode.max_mode = EFI_MAX_COUT_MODE; > - efi_con_mode.mode = EFI_COUT_MODE_2; > - } > - } > - > if (mode_number >= efi_con_mode.max_mode) > return EFI_EXIT(EFI_UNSUPPORTED); > > if (efi_cout_modes[mode_number].present != 1) > return EFI_EXIT(EFI_UNSUPPORTED); > > -out: > if (columns) > *columns = efi_cout_modes[mode_number].columns; > if (rows) > @@ -566,6 +569,9 @@ int efi_console_register(void) > struct efi_object *efi_console_output_obj; > struct efi_object *efi_console_input_obj; > > + /* Set up mode information */ > + query_console_size(); > + > /* Create handles */ > r = efi_create_handle((efi_handle_t *)&efi_console_output_obj); > if (r != EFI_SUCCESS) Even without this patch with a newer gcc we see: https://travis-ci.org/trini/u-boot/jobs/376853382#L916 Can you address that while doing this fixup as well? Thanks!
On 05/09/2018 10:00 PM, Tom Rini wrote: > On Sun, Apr 29, 2018 at 08:02:46PM +0200, Heinrich Schuchardt wrote: > >> If we cannot determine the size of the serial terminal we still have >> to check the parameters of efi_cout_query_mode(). >> >> Querying the size of the serial terminal drains the keyboard buffer. >> So make sure we do this during the initialization and not in the midst >> of an EFI application. >> >> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> <snip /> > > Even without this patch with a newer gcc we see: > https://travis-ci.org/trini/u-boot/jobs/376853382#L916 > > Can you address that while doing this fixup as well? Thanks! > I don't doubt there is an issue. But why don't we see the error outside of Travis CI? Do we set different flags when building? Can't see it with gcc (Debian 6.3.0-18+deb9u1) 6.3.0 20170516 nor with gcc (Debian 7.3.0-17) 7.3.0 Best regards Heinrich
On Wed, May 09, 2018 at 11:13:37PM +0200, Heinrich Schuchardt wrote: > On 05/09/2018 10:00 PM, Tom Rini wrote: > > On Sun, Apr 29, 2018 at 08:02:46PM +0200, Heinrich Schuchardt wrote: > > > >> If we cannot determine the size of the serial terminal we still have > >> to check the parameters of efi_cout_query_mode(). > >> > >> Querying the size of the serial terminal drains the keyboard buffer. > >> So make sure we do this during the initialization and not in the midst > >> of an EFI application. > >> > >> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > > <snip /> > > > > > Even without this patch with a newer gcc we see: > > https://travis-ci.org/trini/u-boot/jobs/376853382#L916 > > > > Can you address that while doing this fixup as well? Thanks! > > > > I don't doubt there is an issue. But why don't we see the error outside > of Travis CI? Do we set different flags when building? > > Can't see it with > gcc (Debian 6.3.0-18+deb9u1) 6.3.0 20170516 > nor with > gcc (Debian 7.3.0-17) 7.3.0 As a warning or an error? The toolchain in question is https://mirrors.edge.kernel.org/pub/tools/crosstool/files/bin/x86_64/7.3.0/x86_64-gcc-7.3.0-nolibc_x86_64-linux.tar.xz and on that specific branch is where I'm testing moving buildman up. Thanks!
On 05/09/2018 11:18 PM, Tom Rini wrote: > On Wed, May 09, 2018 at 11:13:37PM +0200, Heinrich Schuchardt wrote: >> On 05/09/2018 10:00 PM, Tom Rini wrote: >>> On Sun, Apr 29, 2018 at 08:02:46PM +0200, Heinrich Schuchardt wrote: >>> >>>> If we cannot determine the size of the serial terminal we still have >>>> to check the parameters of efi_cout_query_mode(). >>>> >>>> Querying the size of the serial terminal drains the keyboard buffer. >>>> So make sure we do this during the initialization and not in the midst >>>> of an EFI application. >>>> >>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> >> >> <snip /> >> >>> >>> Even without this patch with a newer gcc we see: >>> https://travis-ci.org/trini/u-boot/jobs/376853382#L916 >>> >>> Can you address that while doing this fixup as well? Thanks! >>> >> >> I don't doubt there is an issue. But why don't we see the error outside >> of Travis CI? Do we set different flags when building? >> >> Can't see it with >> gcc (Debian 6.3.0-18+deb9u1) 6.3.0 20170516 >> nor with >> gcc (Debian 7.3.0-17) 7.3.0 > > As a warning or an error? The toolchain in question is > https://mirrors.edge.kernel.org/pub/tools/crosstool/files/bin/x86_64/7.3.0/x86_64-gcc-7.3.0-nolibc_x86_64-linux.tar.xz > and on that specific branch is where I'm testing moving buildman up. > Thanks! When I run git checkout master make qemu-x86_defconfig make -j6 I see no warning. I strongly dislike using buildman locally because it pulls in binaries from some dubious source which are not even signed. Best regards Heinrich > > > > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > https://lists.denx.de/listinfo/u-boot >
diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c index d687362a50..f1b8db55d6 100644 --- a/lib/efi_loader/efi_console.c +++ b/lib/efi_loader/efi_console.c @@ -13,8 +13,6 @@ #include <stdio_dev.h> #include <video_console.h> -static bool console_size_queried; - #define EFI_COUT_MODE_2 2 #define EFI_MAX_COUT_MODE 3 @@ -206,6 +204,51 @@ static int query_console_serial(int *rows, int *cols) return 0; } +/* + * 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. + */ +static void query_console_size(void) +{ + const char *stdout_name = env_get("stdout"); + int rows, cols; + + if (stdout_name && !strcmp(stdout_name, "vidconsole") && + IS_ENABLED(CONFIG_DM_VIDEO)) { + struct stdio_dev *stdout_dev = + stdio_get_by_name("vidconsole"); + struct udevice *dev = stdout_dev->priv; + struct vidconsole_priv *priv = + dev_get_uclass_priv(dev); + rows = priv->rows; + cols = priv->cols; + } else if (query_console_serial(&rows, &cols)) { + return; + } + + /* Test if we can have Mode 1 */ + if (cols >= 80 && rows >= 50) { + efi_cout_modes[1].present = 1; + efi_con_mode.max_mode = 2; + } + + /* + * Install our mode as mode 2 if it is different + * than mode 0 or 1 and set it as the currently selected mode + */ + if (!cout_mode_matches(&efi_cout_modes[0], rows, cols) && + !cout_mode_matches(&efi_cout_modes[1], rows, cols)) { + efi_cout_modes[EFI_COUT_MODE_2].columns = cols; + efi_cout_modes[EFI_COUT_MODE_2].rows = rows; + efi_cout_modes[EFI_COUT_MODE_2].present = 1; + efi_con_mode.max_mode = EFI_MAX_COUT_MODE; + efi_con_mode.mode = EFI_COUT_MODE_2; + } +} + static efi_status_t EFIAPI efi_cout_query_mode( struct efi_simple_text_output_protocol *this, unsigned long mode_number, unsigned long *columns, @@ -213,52 +256,12 @@ static efi_status_t EFIAPI efi_cout_query_mode( { EFI_ENTRY("%p, %ld, %p, %p", this, mode_number, columns, rows); - if (!console_size_queried) { - const char *stdout_name = env_get("stdout"); - int rows, cols; - - console_size_queried = true; - - if (stdout_name && !strcmp(stdout_name, "vidconsole") && - IS_ENABLED(CONFIG_DM_VIDEO)) { - struct stdio_dev *stdout_dev = - stdio_get_by_name("vidconsole"); - struct udevice *dev = stdout_dev->priv; - struct vidconsole_priv *priv = - dev_get_uclass_priv(dev); - rows = priv->rows; - cols = priv->cols; - } else if (query_console_serial(&rows, &cols)) { - goto out; - } - - /* Test if we can have Mode 1 */ - if (cols >= 80 && rows >= 50) { - efi_cout_modes[1].present = 1; - efi_con_mode.max_mode = 2; - } - - /* - * Install our mode as mode 2 if it is different - * than mode 0 or 1 and set it as the currently selected mode - */ - if (!cout_mode_matches(&efi_cout_modes[0], rows, cols) && - !cout_mode_matches(&efi_cout_modes[1], rows, cols)) { - efi_cout_modes[EFI_COUT_MODE_2].columns = cols; - efi_cout_modes[EFI_COUT_MODE_2].rows = rows; - efi_cout_modes[EFI_COUT_MODE_2].present = 1; - efi_con_mode.max_mode = EFI_MAX_COUT_MODE; - efi_con_mode.mode = EFI_COUT_MODE_2; - } - } - if (mode_number >= efi_con_mode.max_mode) return EFI_EXIT(EFI_UNSUPPORTED); if (efi_cout_modes[mode_number].present != 1) return EFI_EXIT(EFI_UNSUPPORTED); -out: if (columns) *columns = efi_cout_modes[mode_number].columns; if (rows) @@ -566,6 +569,9 @@ int efi_console_register(void) struct efi_object *efi_console_output_obj; struct efi_object *efi_console_input_obj; + /* Set up mode information */ + query_console_size(); + /* Create handles */ r = efi_create_handle((efi_handle_t *)&efi_console_output_obj); if (r != EFI_SUCCESS)
If we cannot determine the size of the serial terminal we still have to check the parameters of efi_cout_query_mode(). Querying the size of the serial terminal drains the keyboard buffer. So make sure we do this during the initialization and not in the midst of an EFI application. Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> --- lib/efi_loader/efi_console.c | 90 +++++++++++++++++++++++--------------------- 1 file changed, 48 insertions(+), 42 deletions(-)