Re: [PATCH 1/1] efi_loader: initialize console size late

[Heinrich Schuchardt <heinrich.schuchardt@canonical.com>]

On 6/15/22 08:34, AKASHI Takahiro wrote:
> On Wed, Jun 15, 2022 at 08:27:26AM +0200, Heinrich Schuchardt wrote:
>>
>>
>> On 6/15/22 08:16, AKASHI Takahiro wrote:
>>> On Tue, Jun 14, 2022 at 08:02:03AM +0200, Heinrich Schuchardt wrote:
>>>> From: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>>
>>>> If CONFIG_VIDEO_DM=n we query the display size from the serial console.
>>>> Especially when using a remote console the response can be so late that
>>>> it interferes with autoboot.
>>>>
>>>> Only query the console size when running an EFI binary.
>>>>
>>>> Add debug output showing the determined console size.
>>>>
>>>> Reported-by: Fabio Estevam <festevam@gmail.com>
>>>> Fixes: a9bf024b2933 ("efi_loader: disk: a helper function to create efi_disk objects from udevice")
>>
>> Said patch made CONFIG_EFI_SETUP_EARLY=y the default.
> 
> I don't think so.
> Any config with this option enabled could cause the issue.

We could additionally blame your patches that created this config option.

> 
>>>
>>> If the key part of this patch is to move query_console_size() from
>>> efi_init_early() to efi_init_obj_list(), the to-be-fixed patch is not
>>> the one above but
>>>           commit a57ad20d07e8 ("efi_loader: split efi_init_obj_list() into two stages")
>>>
>>> Moreover, this is just a warning but once Sughosh's patch,
>>>           https://lists.denx.de/pipermail/u-boot/2022-June/485977.html
>>> is merged and FWU_MULTI_BANK_UPDATE is enabled, the said phenomenon can
>>> be triggered again because efi_init_obj_list(), hence query_console_size(),
>>> will be called in board_init_r() before showing the U-Boot prompt.
>>
>> Then we should not merge it as is.
> 
> I think that your patch is a tentative workaround.

What makes you think so?
Any better proposal that we can get in in time for v2022.07?

Best regards

Heinrich

> 
> -Takahiro Akashi
> 
> 
>> Best regards
>>
>> Heinrich
>>
>>>
>>> -Takahiro Akashi
>>>
>>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>>>> ---
>>>>    include/efi_loader.h         |  2 ++
>>>>    lib/efi_loader/efi_console.c | 20 +++++++++++++-------
>>>>    lib/efi_loader/efi_setup.c   |  4 ++++
>>>>    3 files changed, 19 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>>>> index f6651e2c60..c1e00ebac3 100644
>>>> --- a/include/efi_loader.h
>>>> +++ b/include/efi_loader.h
>>>> @@ -499,6 +499,8 @@ extern struct list_head efi_register_notify_events;
>>>>    int efi_init_early(void);
>>>>    /* Initialize efi execution environment */
>>>>    efi_status_t efi_init_obj_list(void);
>>>> +/* Set up console modes */
>>>> +void efi_setup_console_size(void);
>>>>    /* Install device tree */
>>>>    efi_status_t efi_install_fdt(void *fdt);
>>>>    /* Run loaded UEFI image */
>>>> diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c
>>>> index 60a3fc85ac..3164fd484e 100644
>>>> --- a/lib/efi_loader/efi_console.c
>>>> +++ b/lib/efi_loader/efi_console.c
>>>> @@ -5,6 +5,8 @@
>>>>     *  Copyright (c) 2016 Alexander Graf
>>>>     */
>>>> +#define LOG_CATEGORY LOGC_EFI
>>>> +
>>>>    #include <common.h>
>>>>    #include <charset.h>
>>>>    #include <malloc.h>
>>>> @@ -12,6 +14,7 @@
>>>>    #include <dm/device.h>
>>>>    #include <efi_loader.h>
>>>>    #include <env.h>
>>>> +#include <log.h>
>>>>    #include <stdio_dev.h>
>>>>    #include <video_console.h>
>>>>    #include <linux/delay.h>
>>>> @@ -58,7 +61,12 @@ const efi_guid_t efi_guid_text_output_protocol =
>>>>    #define cESC '\x1b'
>>>>    #define ESC "\x1b"
>>>> -/* Default to mode 0 */
>>>> +/*
>>>> + * efi_con_mode - mode information of the Simple Text Output Protocol
>>>> + *
>>>> + * Use safe settings before efi_setup_console_size() is called.
>>>> + * By default enable only the 80x25 mode which must always exist.
>>>> + */
>>>>    static struct simple_text_output_mode efi_con_mode = {
>>>>    	.max_mode = 1,
>>>>    	.mode = 0,
>>>> @@ -333,13 +341,13 @@ static int __maybe_unused query_vidconsole(int *rows, int *cols)
>>>>    }
>>>>    /**
>>>> - * query_console_size() - update the mode table.
>>>> + * 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.
>>>>     */
>>>> -static void query_console_size(void)
>>>> +void efi_setup_console_size(void)
>>>>    {
>>>>    	int rows = 25, cols = 80;
>>>>    	int ret = -ENODEV;
>>>> @@ -351,6 +359,8 @@ static void query_console_size(void)
>>>>    	if (ret)
>>>>    		return;
>>>> +	log_debug("Console size %dx%d\n", rows, cols);
>>>> +
>>>>    	/* Test if we can have Mode 1 */
>>>>    	if (cols >= 80 && rows >= 50) {
>>>>    		efi_cout_modes[1].present = 1;
>>>> @@ -371,7 +381,6 @@ static void query_console_size(void)
>>>>    	}
>>>>    }
>>>> -
>>>>    /**
>>>>     * efi_cout_query_mode() - get terminal size for a text mode
>>>>     *
>>>> @@ -1262,9 +1271,6 @@ efi_status_t efi_console_register(void)
>>>>    	efi_status_t r;
>>>>    	struct efi_device_path *dp;
>>>> -	/* Set up mode information */
>>>> -	query_console_size();
>>>> -
>>>>    	/* Install protocols on root node */
>>>>    	r = EFI_CALL(efi_install_multiple_protocol_interfaces
>>>>    		     (&efi_root,
>>>> diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
>>>> index 250eeb2fcd..492ecf4cb1 100644
>>>> --- a/lib/efi_loader/efi_setup.c
>>>> +++ b/lib/efi_loader/efi_setup.c
>>>> @@ -243,6 +243,10 @@ efi_status_t efi_init_obj_list(void)
>>>>    			goto out;
>>>>    	}
>>>> +	/* Set up console modes */
>>>> +	efi_setup_console_size();
>>>> +
>>>> +	/* Install EFI_RNG_PROTOCOL */
>>>>    	if (IS_ENABLED(CONFIG_EFI_RNG_PROTOCOL)) {
>>>>    		ret = efi_rng_register();
>>>>    		if (ret != EFI_SUCCESS)
>>>> -- 
>>>> 2.36.1
>>>>