[PATCH v2 0/3] eficonfig: add vertical scroll support

[PATCH v2 0/3] eficonfig: add vertical scroll support

[Masahisa Kojima]

This series aims to add the vertical scroll for the eficonfig menu.
Before adding scroll support, this series does the refactoring
of change boot order implementation since it has own menu handling
and it should be removed to improve maintenanceability.

The eficonfig menu handles file selection for EFI load option
and secure boot keys, it likely to enumerate tens of files.
User can not select the file without scroll if theare are
many files under the target directory.

This series only modifies the eficonfig menus. Other menus
such as bootmenu is not yet done. We need to enhance the
U-Boot menu framework itself if we support other menus.

Masahisa Kojima (3):
  eficonfig: refactor eficonfig_process_common function
  eficonfig: refactor change boot order implementation
  eficonfig: add vertical scroll support

 cmd/eficonfig.c       | 383 +++++++++++++++++++++++++++++-------------
 cmd/eficonfig_sbkey.c |  18 +-
 include/efi_config.h  |  17 +-
 include/efi_loader.h  |   1 +
 4 files changed, 296 insertions(+), 123 deletions(-)

-- 
2.17.1

[PATCH v2 1/3] eficonfig: refactor eficonfig_process_common function

[Masahisa Kojima]

Current change boot order implementation does not call
eficonfig_process_common() and call own menu functions
for display_statusline, item_data_print and item_choice.
Change boot order functionality should call
eficonfig_process_common() to improve maintenanceability.

This commit is a preparation to remove the change boot
order specific implementation. The menu functions
(display_statusline, item_data_print and item_choice) are
added as argument of eficonfig_process_common().
The menu description string displayed at the bottom of
the menu is also added as argument.

Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
---
Changes in v2:
- add const qualifier to eficonfig_menu_desc, change it to pointer

 cmd/eficonfig.c       | 69 +++++++++++++++++++++++++++++++++----------
 cmd/eficonfig_sbkey.c | 18 +++++++++--
 include/efi_config.h  | 13 +++++++-
 3 files changed, 81 insertions(+), 19 deletions(-)

diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
index ce7175a566..2fc486dac2 100644
--- a/cmd/eficonfig.c
+++ b/cmd/eficonfig.c
@@ -21,6 +21,8 @@
 #include <linux/delay.h>
 
 static struct efi_simple_text_input_protocol *cin;
+const char *eficonfig_menu_desc =
+	"  Press UP/DOWN to move, ENTER to select, ESC/CTRL+C to quit";
 
 #define EFICONFIG_DESCRIPTION_MAX 32
 #define EFICONFIG_OPTIONAL_DATA_MAX 64
@@ -133,7 +135,7 @@ void eficonfig_print_msg(char *msg)
  *
  * @data:	pointer to the data associated with each menu entry
  */
-static void eficonfig_print_entry(void *data)
+void eficonfig_print_entry(void *data)
 {
 	struct eficonfig_entry *entry = data;
 	int reverse = (entry->efi_menu->active == entry->num);
@@ -160,7 +162,7 @@ static void eficonfig_print_entry(void *data)
  *
  * @m:	pointer to the menu structure
  */
-static void eficonfig_display_statusline(struct menu *m)
+void eficonfig_display_statusline(struct menu *m)
 {
 	struct eficonfig_entry *entry;
 
@@ -170,10 +172,11 @@ static void eficonfig_display_statusline(struct menu *m)
 	printf(ANSI_CURSOR_POSITION
 	      "\n%s\n"
 	       ANSI_CURSOR_POSITION ANSI_CLEAR_LINE ANSI_CURSOR_POSITION
-	       "  Press UP/DOWN to move, ENTER to select, ESC/CTRL+C to quit"
+	       "%s"
 	       ANSI_CLEAR_LINE_TO_END ANSI_CURSOR_POSITION ANSI_CLEAR_LINE,
 	       1, 1, entry->efi_menu->menu_header, entry->efi_menu->count + 5, 1,
-	       entry->efi_menu->count + 6, 1, entry->efi_menu->count + 7, 1);
+	       entry->efi_menu->count + 6, 1, entry->efi_menu->menu_desc,
+	       entry->efi_menu->count + 7, 1);
 }
 
 /**
@@ -182,7 +185,7 @@ static void eficonfig_display_statusline(struct menu *m)
  * @data:	pointer to the efimenu structure
  * Return:	key string to identify the selected entry
  */
-static char *eficonfig_choice_entry(void *data)
+char *eficonfig_choice_entry(void *data)
 {
 	int esc = 0;
 	struct list_head *pos, *n;
@@ -358,9 +361,17 @@ out:
  *
  * @efi_menu:		pointer to the efimenu structure
  * @menu_header:	pointer to the menu header string
+ * @menu_desc:		pointer to the menu description
+ * @display_statusline:	function pointer to draw statusline
+ * @item_data_print:	function pointer to draw the menu item
+ * @item_choice:	function pointer to handle the key press
  * Return:		status code
  */
-efi_status_t eficonfig_process_common(struct efimenu *efi_menu, char *menu_header)
+efi_status_t eficonfig_process_common(struct efimenu *efi_menu,
+				      char *menu_header, const char *menu_desc,
+				      void (*display_statusline)(struct menu *),
+				      void (*item_data_print)(void *),
+				      char *(*item_choice)(void *))
 {
 	struct menu *menu;
 	void *choice = NULL;
@@ -379,10 +390,11 @@ efi_status_t eficonfig_process_common(struct efimenu *efi_menu, char *menu_heade
 		if (!efi_menu->menu_header)
 			return EFI_OUT_OF_RESOURCES;
 	}
+	if (menu_desc)
+		efi_menu->menu_desc = menu_desc;
 
-	menu = menu_create(NULL, 0, 1, eficonfig_display_statusline,
-			   eficonfig_print_entry, eficonfig_choice_entry,
-			   efi_menu);
+	menu = menu_create(NULL, 0, 1, display_statusline, item_data_print,
+			   item_choice, efi_menu);
 	if (!menu)
 		return EFI_INVALID_PARAMETER;
 
@@ -641,7 +653,12 @@ static efi_status_t eficonfig_select_volume(struct eficonfig_select_file_info *f
 	if (ret != EFI_SUCCESS)
 		goto out;
 
-	ret = eficonfig_process_common(efi_menu, "  ** Select Volume **");
+	ret = eficonfig_process_common(efi_menu, "  ** Select Volume **",
+				       eficonfig_menu_desc,
+				       eficonfig_display_statusline,
+				       eficonfig_print_entry,
+				       eficonfig_choice_entry);
+
 out:
 	efi_free_pool(volume_handles);
 	list_for_each_safe(pos, n, &efi_menu->list) {
@@ -816,7 +833,11 @@ static efi_status_t eficonfig_show_file_selection(struct eficonfig_select_file_i
 		if (ret != EFI_SUCCESS)
 			goto err;
 
-		ret = eficonfig_process_common(efi_menu, "  ** Select File **");
+		ret = eficonfig_process_common(efi_menu, "  ** Select File **",
+					       eficonfig_menu_desc,
+					       eficonfig_display_statusline,
+					       eficonfig_print_entry,
+					       eficonfig_choice_entry);
 err:
 		EFI_CALL(f->close(f));
 		eficonfig_destroy(efi_menu);
@@ -977,7 +998,11 @@ efi_status_t eficonfig_process_show_file_option(void *data)
 	if (!efi_menu)
 		return EFI_OUT_OF_RESOURCES;
 
-	ret = eficonfig_process_common(efi_menu, "  ** Update File **");
+	ret = eficonfig_process_common(efi_menu, "  ** Update File **",
+				       eficonfig_menu_desc,
+				       eficonfig_display_statusline,
+				       eficonfig_print_entry,
+				       eficonfig_choice_entry);
 	if (ret != EFI_SUCCESS) /* User selects "Clear" or "Quit" */
 		ret = EFI_NOT_READY;
 
@@ -1323,7 +1348,12 @@ static efi_status_t eficonfig_show_boot_option(struct eficonfig_boot_option *bo,
 	if (ret != EFI_SUCCESS)
 		goto out;
 
-	ret = eficonfig_process_common(efi_menu, header_str);
+	ret = eficonfig_process_common(efi_menu, header_str,
+				       eficonfig_menu_desc,
+				       eficonfig_display_statusline,
+				       eficonfig_print_entry,
+				       eficonfig_choice_entry);
+
 out:
 	eficonfig_destroy(efi_menu);
 
@@ -1742,7 +1772,11 @@ static efi_status_t eficonfig_show_boot_selection(unsigned int *selected)
 	if (ret != EFI_SUCCESS)
 		goto out;
 
-	ret = eficonfig_process_common(efi_menu, "  ** Select Boot Option **");
+	ret = eficonfig_process_common(efi_menu, "  ** Select Boot Option **",
+				       eficonfig_menu_desc,
+				       eficonfig_display_statusline,
+				       eficonfig_print_entry,
+				       eficonfig_choice_entry);
 out:
 	list_for_each_safe(pos, n, &efi_menu->list) {
 		entry = list_entry(pos, struct eficonfig_entry, list);
@@ -2563,7 +2597,12 @@ static int do_eficonfig(struct cmd_tbl *cmdtp, int flag, int argc, char *const a
 		if (!efi_menu)
 			return CMD_RET_FAILURE;
 
-		ret = eficonfig_process_common(efi_menu, "  ** UEFI Maintenance Menu **");
+		ret = eficonfig_process_common(efi_menu,
+					       "  ** UEFI Maintenance Menu **",
+					       eficonfig_menu_desc,
+					       eficonfig_display_statusline,
+					       eficonfig_print_entry,
+					       eficonfig_choice_entry);
 		eficonfig_destroy(efi_menu);
 
 		if (ret == EFI_ABORTED)
diff --git a/cmd/eficonfig_sbkey.c b/cmd/eficonfig_sbkey.c
index ed39aab081..caca27495e 100644
--- a/cmd/eficonfig_sbkey.c
+++ b/cmd/eficonfig_sbkey.c
@@ -410,7 +410,10 @@ static efi_status_t enumerate_and_show_signature_database(void *varname)
 		goto out;
 
 	snprintf(buf, sizeof(buf), "  ** Show Signature Database (%ls) **", (u16 *)varname);
-	ret = eficonfig_process_common(efi_menu, buf);
+	ret = eficonfig_process_common(efi_menu, buf, eficonfig_menu_desc,
+				       eficonfig_display_statusline,
+				       eficonfig_print_entry,
+				       eficonfig_choice_entry);
 out:
 	list_for_each_safe(pos, n, &efi_menu->list) {
 		entry = list_entry(pos, struct eficonfig_entry, list);
@@ -472,7 +475,11 @@ static efi_status_t eficonfig_process_set_secure_boot_key(void *data)
 		efi_menu = eficonfig_create_fixed_menu(key_config_menu_items,
 						       ARRAY_SIZE(key_config_menu_items));
 
-		ret = eficonfig_process_common(efi_menu, header_str);
+		ret = eficonfig_process_common(efi_menu, header_str,
+					       eficonfig_menu_desc,
+					       eficonfig_display_statusline,
+					       eficonfig_print_entry,
+					       eficonfig_choice_entry);
 		eficonfig_destroy(efi_menu);
 
 		if (ret == EFI_ABORTED)
@@ -518,7 +525,12 @@ efi_status_t eficonfig_process_secure_boot_config(void *data)
 			break;
 		}
 
-		ret = eficonfig_process_common(efi_menu, header_str);
+		ret = eficonfig_process_common(efi_menu, header_str,
+					       eficonfig_menu_desc,
+					       eficonfig_display_statusline,
+					       eficonfig_print_entry,
+					       eficonfig_choice_entry);
+
 		eficonfig_destroy(efi_menu);
 
 		if (ret == EFI_ABORTED)
diff --git a/include/efi_config.h b/include/efi_config.h
index fd69926343..cec5715f84 100644
--- a/include/efi_config.h
+++ b/include/efi_config.h
@@ -9,12 +9,14 @@
 #define _EFI_CONFIG_H
 
 #include <efi_loader.h>
+#include <menu.h>
 
 #define EFICONFIG_ENTRY_NUM_MAX 99
 #define EFICONFIG_VOLUME_PATH_MAX 512
 #define EFICONFIG_FILE_PATH_MAX 512
 #define EFICONFIG_FILE_PATH_BUF_SIZE (EFICONFIG_FILE_PATH_MAX * sizeof(u16))
 
+extern const char *eficonfig_menu_desc;
 typedef efi_status_t (*eficonfig_entry_func)(void *data);
 
 /**
@@ -45,6 +47,7 @@ struct eficonfig_entry {
  * @active:		active menu entry index
  * @count:		total count of menu entry
  * @menu_header:	menu header string
+ * @menu_desc:		menu description string
  * @list:		menu entry list structure
  */
 struct efimenu {
@@ -52,6 +55,7 @@ struct efimenu {
 	int active;
 	int count;
 	char *menu_header;
+	const char *menu_desc;
 	struct list_head list;
 };
 
@@ -86,9 +90,16 @@ struct eficonfig_select_file_info {
 };
 
 void eficonfig_print_msg(char *msg);
+void eficonfig_print_entry(void *data);
+void eficonfig_display_statusline(struct menu *m);
+char *eficonfig_choice_entry(void *data);
 void eficonfig_destroy(struct efimenu *efi_menu);
 efi_status_t eficonfig_process_quit(void *data);
-efi_status_t eficonfig_process_common(struct efimenu *efi_menu, char *menu_header);
+efi_status_t eficonfig_process_common(struct efimenu *efi_menu,
+				      char *menu_header, const char *menu_desc,
+				      void (*display_statusline)(struct menu *),
+				      void (*item_data_print)(void *),
+				      char *(*item_choice)(void *));
 efi_status_t eficonfig_process_select_file(void *data);
 efi_status_t eficonfig_get_unused_bootoption(u16 *buf,
 					     efi_uintn_t buf_size, u32 *index);
-- 
2.17.1

Re: [PATCH v2 1/3] eficonfig: refactor eficonfig_process_common function

[Ilias Apalodimas]

Hi Kojima-san


Overall I think the cleanup is nice and easier to maintain in the long
run.

On Sat, Dec 24, 2022 at 07:57:42AM +0900, Masahisa Kojima wrote:
> Current change boot order implementation does not call
> eficonfig_process_common() and call own menu functions
> for display_statusline, item_data_print and item_choice.
> Change boot order functionality should call
> eficonfig_process_common() to improve maintenanceability.
>
> This commit is a preparation to remove the change boot
> order specific implementation. The menu functions
> (display_statusline, item_data_print and item_choice) are
> added as argument of eficonfig_process_common().
> The menu description string displayed at the bottom of
> the menu is also added as argument.
>
> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> ---
> Changes in v2:
> - add const qualifier to eficonfig_menu_desc, change it to pointer
>
>  cmd/eficonfig.c       | 69 +++++++++++++++++++++++++++++++++----------
>  cmd/eficonfig_sbkey.c | 18 +++++++++--
>  include/efi_config.h  | 13 +++++++-
>  3 files changed, 81 insertions(+), 19 deletions(-)
>
> diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
> index ce7175a566..2fc486dac2 100644
> --- a/cmd/eficonfig.c
> +++ b/cmd/eficonfig.c
> @@ -21,6 +21,8 @@
>  #include <linux/delay.h>
>
>  static struct efi_simple_text_input_protocol *cin;
> +const char *eficonfig_menu_desc =
> +	"  Press UP/DOWN to move, ENTER to select, ESC/CTRL+C to quit";
>
>  #define EFICONFIG_DESCRIPTION_MAX 32
>  #define EFICONFIG_OPTIONAL_DATA_MAX 64
> @@ -133,7 +135,7 @@ void eficonfig_print_msg(char *msg)
>   *
>   * @data:	pointer to the data associated with each menu entry
>   */
> -static void eficonfig_print_entry(void *data)
> +void eficonfig_print_entry(void *data)
>  {
>  	struct eficonfig_entry *entry = data;
>  	int reverse = (entry->efi_menu->active == entry->num);
> @@ -160,7 +162,7 @@ static void eficonfig_print_entry(void *data)
>   *
>   * @m:	pointer to the menu structure
>   */
> -static void eficonfig_display_statusline(struct menu *m)
> +void eficonfig_display_statusline(struct menu *m)
>  {
>  	struct eficonfig_entry *entry;
>
> @@ -170,10 +172,11 @@ static void eficonfig_display_statusline(struct menu *m)
>  	printf(ANSI_CURSOR_POSITION
>  	      "\n%s\n"
>  	       ANSI_CURSOR_POSITION ANSI_CLEAR_LINE ANSI_CURSOR_POSITION
> -	       "  Press UP/DOWN to move, ENTER to select, ESC/CTRL+C to quit"
> +	       "%s"
>  	       ANSI_CLEAR_LINE_TO_END ANSI_CURSOR_POSITION ANSI_CLEAR_LINE,
>  	       1, 1, entry->efi_menu->menu_header, entry->efi_menu->count + 5, 1,
> -	       entry->efi_menu->count + 6, 1, entry->efi_menu->count + 7, 1);
> +	       entry->efi_menu->count + 6, 1, entry->efi_menu->menu_desc,
> +	       entry->efi_menu->count + 7, 1);
>  }
>
>  /**
> @@ -182,7 +185,7 @@ static void eficonfig_display_statusline(struct menu *m)
>   * @data:	pointer to the efimenu structure
>   * Return:	key string to identify the selected entry
>   */
> -static char *eficonfig_choice_entry(void *data)
> +char *eficonfig_choice_entry(void *data)
>  {
>  	int esc = 0;
>  	struct list_head *pos, *n;
> @@ -358,9 +361,17 @@ out:
>   *
>   * @efi_menu:		pointer to the efimenu structure
>   * @menu_header:	pointer to the menu header string
> + * @menu_desc:		pointer to the menu description
> + * @display_statusline:	function pointer to draw statusline
> + * @item_data_print:	function pointer to draw the menu item
> + * @item_choice:	function pointer to handle the key press
>   * Return:		status code
>   */
> -efi_status_t eficonfig_process_common(struct efimenu *efi_menu, char *menu_header)
> +efi_status_t eficonfig_process_common(struct efimenu *efi_menu,
> +				      char *menu_header, const char *menu_desc,
> +				      void (*display_statusline)(struct menu *),
> +				      void (*item_data_print)(void *),
> +				      char *(*item_choice)(void *))
>  {
>  	struct menu *menu;
>  	void *choice = NULL;
> @@ -379,10 +390,11 @@ efi_status_t eficonfig_process_common(struct efimenu *efi_menu, char *menu_heade
>  		if (!efi_menu->menu_header)
>  			return EFI_OUT_OF_RESOURCES;
>  	}
> +	if (menu_desc)
> +		efi_menu->menu_desc = menu_desc;
>
> -	menu = menu_create(NULL, 0, 1, eficonfig_display_statusline,
> -			   eficonfig_print_entry, eficonfig_choice_entry,
> -			   efi_menu);
> +	menu = menu_create(NULL, 0, 1, display_statusline, item_data_print,
> +			   item_choice, efi_menu);

menu_create doesn't check any pointers for item_data_print, item_choice, efi_menu
Should we check any of these here?

>  	if (!menu)
>  		return EFI_INVALID_PARAMETER;

[...]

Regards
/Ilias

Re: [PATCH v2 1/3] eficonfig: refactor eficonfig_process_common function

[Masahisa Kojima]

Hi Ilias,

On Tue, 27 Dec 2022 at 23:41, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Kojima-san
>
>
> Overall I think the cleanup is nice and easier to maintain in the long
> run.
>
> On Sat, Dec 24, 2022 at 07:57:42AM +0900, Masahisa Kojima wrote:
> > Current change boot order implementation does not call
> > eficonfig_process_common() and call own menu functions
> > for display_statusline, item_data_print and item_choice.
> > Change boot order functionality should call
> > eficonfig_process_common() to improve maintenanceability.
> >
> > This commit is a preparation to remove the change boot
> > order specific implementation. The menu functions
> > (display_statusline, item_data_print and item_choice) are
> > added as argument of eficonfig_process_common().
> > The menu description string displayed at the bottom of
> > the menu is also added as argument.
> >
> > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > ---
> > Changes in v2:
> > - add const qualifier to eficonfig_menu_desc, change it to pointer
> >
> >  cmd/eficonfig.c       | 69 +++++++++++++++++++++++++++++++++----------
> >  cmd/eficonfig_sbkey.c | 18 +++++++++--
> >  include/efi_config.h  | 13 +++++++-
> >  3 files changed, 81 insertions(+), 19 deletions(-)
> >
> > diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
> > index ce7175a566..2fc486dac2 100644
> > --- a/cmd/eficonfig.c
> > +++ b/cmd/eficonfig.c
> > @@ -21,6 +21,8 @@
> >  #include <linux/delay.h>
> >
> >  static struct efi_simple_text_input_protocol *cin;
> > +const char *eficonfig_menu_desc =
> > +     "  Press UP/DOWN to move, ENTER to select, ESC/CTRL+C to quit";
> >
> >  #define EFICONFIG_DESCRIPTION_MAX 32
> >  #define EFICONFIG_OPTIONAL_DATA_MAX 64
> > @@ -133,7 +135,7 @@ void eficonfig_print_msg(char *msg)
> >   *
> >   * @data:    pointer to the data associated with each menu entry
> >   */
> > -static void eficonfig_print_entry(void *data)
> > +void eficonfig_print_entry(void *data)
> >  {
> >       struct eficonfig_entry *entry = data;
> >       int reverse = (entry->efi_menu->active == entry->num);
> > @@ -160,7 +162,7 @@ static void eficonfig_print_entry(void *data)
> >   *
> >   * @m:       pointer to the menu structure
> >   */
> > -static void eficonfig_display_statusline(struct menu *m)
> > +void eficonfig_display_statusline(struct menu *m)
> >  {
> >       struct eficonfig_entry *entry;
> >
> > @@ -170,10 +172,11 @@ static void eficonfig_display_statusline(struct menu *m)
> >       printf(ANSI_CURSOR_POSITION
> >             "\n%s\n"
> >              ANSI_CURSOR_POSITION ANSI_CLEAR_LINE ANSI_CURSOR_POSITION
> > -            "  Press UP/DOWN to move, ENTER to select, ESC/CTRL+C to quit"
> > +            "%s"
> >              ANSI_CLEAR_LINE_TO_END ANSI_CURSOR_POSITION ANSI_CLEAR_LINE,
> >              1, 1, entry->efi_menu->menu_header, entry->efi_menu->count + 5, 1,
> > -            entry->efi_menu->count + 6, 1, entry->efi_menu->count + 7, 1);
> > +            entry->efi_menu->count + 6, 1, entry->efi_menu->menu_desc,
> > +            entry->efi_menu->count + 7, 1);
> >  }
> >
> >  /**
> > @@ -182,7 +185,7 @@ static void eficonfig_display_statusline(struct menu *m)
> >   * @data:    pointer to the efimenu structure
> >   * Return:   key string to identify the selected entry
> >   */
> > -static char *eficonfig_choice_entry(void *data)
> > +char *eficonfig_choice_entry(void *data)
> >  {
> >       int esc = 0;
> >       struct list_head *pos, *n;
> > @@ -358,9 +361,17 @@ out:
> >   *
> >   * @efi_menu:                pointer to the efimenu structure
> >   * @menu_header:     pointer to the menu header string
> > + * @menu_desc:               pointer to the menu description
> > + * @display_statusline:      function pointer to draw statusline
> > + * @item_data_print: function pointer to draw the menu item
> > + * @item_choice:     function pointer to handle the key press
> >   * Return:           status code
> >   */
> > -efi_status_t eficonfig_process_common(struct efimenu *efi_menu, char *menu_header)
> > +efi_status_t eficonfig_process_common(struct efimenu *efi_menu,
> > +                                   char *menu_header, const char *menu_desc,
> > +                                   void (*display_statusline)(struct menu *),
> > +                                   void (*item_data_print)(void *),
> > +                                   char *(*item_choice)(void *))
> >  {
> >       struct menu *menu;
> >       void *choice = NULL;
> > @@ -379,10 +390,11 @@ efi_status_t eficonfig_process_common(struct efimenu *efi_menu, char *menu_heade
> >               if (!efi_menu->menu_header)
> >                       return EFI_OUT_OF_RESOURCES;
> >       }
> > +     if (menu_desc)
> > +             efi_menu->menu_desc = menu_desc;
> >
> > -     menu = menu_create(NULL, 0, 1, eficonfig_display_statusline,
> > -                        eficonfig_print_entry, eficonfig_choice_entry,
> > -                        efi_menu);
> > +     menu = menu_create(NULL, 0, 1, display_statusline, item_data_print,
> > +                        item_choice, efi_menu);
>
> menu_create doesn't check any pointers for item_data_print, item_choice, efi_menu
> Should we check any of these here?

This is as I intended.
I think we don't need to check these pointers, U-Boot menu has default behavior
when these pointers are NULL.
For example, if item_data_print is NULL, U-Boot menu prints the "key" string
of each menu entry.

Thanks,
Masahisa Kojima

>
> >       if (!menu)
> >               return EFI_INVALID_PARAMETER;
>
> [...]
>
> Regards
> /Ilias

[PATCH v2 2/3] eficonfig: refactor change boot order implementation

[Masahisa Kojima]

This commit removes the change boot order specific
menu implementation. The change boot order implementation
calls eficonfig_process_common() same as other menus.

The change boot order menu requires own item_data_print
and item_choice implementation, but display_statusline
function can be a same function as other menus.

Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
---
Changes in v2:
- add comment when the user key press is not valid
- add const qualifier to eficonfig_change_boot_order_desc

 cmd/eficonfig.c | 245 +++++++++++++++++++++++++++++-------------------
 1 file changed, 150 insertions(+), 95 deletions(-)

diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
index 2fc486dac2..13929cb003 100644
--- a/cmd/eficonfig.c
+++ b/cmd/eficonfig.c
@@ -24,6 +24,11 @@ static struct efi_simple_text_input_protocol *cin;
 const char *eficonfig_menu_desc =
 	"  Press UP/DOWN to move, ENTER to select, ESC/CTRL+C to quit";
 
+static const char *eficonfig_change_boot_order_desc =
+	"  Press UP/DOWN to move, +/- to change orde\n"
+	"  Press SPACE to activate or deactivate the entry\n"
+	"  Select [Save] to complete, ESC/CTRL+C to quit";
+
 #define EFICONFIG_DESCRIPTION_MAX 32
 #define EFICONFIG_OPTIONAL_DATA_MAX 64
 
@@ -105,6 +110,17 @@ struct eficonfig_boot_order_data {
 	bool active;
 };
 
+/**
+ * struct eficonfig_save_boot_order_data - structure to be used to change boot order
+ *
+ * @efi_menu:		pointer to efimenu structure
+ * @selected:		flag to indicate user selects "Save" entry
+ */
+struct eficonfig_save_boot_order_data {
+	struct efimenu *efi_menu;
+	bool selected;
+};
+
 /**
  * eficonfig_print_msg() - print message
  *
@@ -173,10 +189,9 @@ void eficonfig_display_statusline(struct menu *m)
 	      "\n%s\n"
 	       ANSI_CURSOR_POSITION ANSI_CLEAR_LINE ANSI_CURSOR_POSITION
 	       "%s"
-	       ANSI_CLEAR_LINE_TO_END ANSI_CURSOR_POSITION ANSI_CLEAR_LINE,
+	       ANSI_CLEAR_LINE_TO_END,
 	       1, 1, entry->efi_menu->menu_header, entry->efi_menu->count + 5, 1,
-	       entry->efi_menu->count + 6, 1, entry->efi_menu->menu_desc,
-	       entry->efi_menu->count + 7, 1);
+	       entry->efi_menu->count + 6, 1, entry->efi_menu->menu_desc);
 }
 
 /**
@@ -1841,63 +1856,44 @@ out:
 }
 
 /**
- * eficonfig_display_change_boot_order() - display the BootOrder list
+ * eficonfig_print_change_boot_order_entry() - print the boot option entry
  *
- * @efi_menu:	pointer to the efimenu structure
- * Return:	status code
+ * @data:	pointer to the data associated with each menu entry
  */
-static void eficonfig_display_change_boot_order(struct efimenu *efi_menu)
+static void eficonfig_print_change_boot_order_entry(void *data)
 {
-	bool reverse;
-	struct list_head *pos, *n;
-	struct eficonfig_entry *entry;
-
-	printf(ANSI_CLEAR_CONSOLE ANSI_CURSOR_POSITION
-	       "\n  ** Change Boot Order **\n"
-	       ANSI_CURSOR_POSITION
-	       "  Press UP/DOWN to move, +/- to change order"
-	       ANSI_CURSOR_POSITION
-	       "  Press SPACE to activate or deactivate the entry"
-	       ANSI_CURSOR_POSITION
-	       "  Select [Save] to complete, ESC/CTRL+C to quit"
-	       ANSI_CURSOR_POSITION ANSI_CLEAR_LINE,
-	       1, 1, efi_menu->count + 5, 1, efi_menu->count + 6, 1,
-	       efi_menu->count + 7, 1,  efi_menu->count + 8, 1);
-
-	/* draw boot option list */
-	list_for_each_safe(pos, n, &efi_menu->list) {
-		entry = list_entry(pos, struct eficonfig_entry, list);
-		reverse = (entry->num == efi_menu->active);
+	struct eficonfig_entry *entry = data;
+	int reverse = (entry->efi_menu->active == entry->num);
 
-		printf(ANSI_CURSOR_POSITION, entry->num + 4, 7);
+	printf(ANSI_CURSOR_POSITION, entry->num + 4, 7);
 
-		if (reverse)
-			puts(ANSI_COLOR_REVERSE);
+	if (reverse)
+		puts(ANSI_COLOR_REVERSE);
 
-		if (entry->num < efi_menu->count - 2) {
-			if (((struct eficonfig_boot_order_data *)entry->data)->active)
-				printf("[*]  ");
-			else
-				printf("[ ]  ");
-		}
+	if (entry->num < entry->efi_menu->count - 2) {
+		if (((struct eficonfig_boot_order_data *)entry->data)->active)
+			printf("[*]  ");
+		else
+			printf("[ ]  ");
+	}
 
-		printf("%s", entry->title);
+	printf("%s", entry->title);
 
-		if (reverse)
-			puts(ANSI_COLOR_RESET);
-	}
+	if (reverse)
+		puts(ANSI_COLOR_RESET);
 }
 
 /**
- * eficonfig_choice_change_boot_order() - handle the BootOrder update
+ * eficonfig_choice_change_boot_order() - user key input handler
  *
- * @efi_menu:	pointer to the efimenu structure
- * Return:	status code
+ * @data:	pointer to the menu entry
+ * Return:	key string to identify the selected entry
  */
-static efi_status_t eficonfig_choice_change_boot_order(struct efimenu *efi_menu)
+char *eficonfig_choice_change_boot_order(void *data)
 {
 	int esc = 0;
 	struct list_head *pos, *n;
+	struct efimenu *efi_menu = data;
 	enum bootmenu_key key = KEY_NONE;
 	struct eficonfig_entry *entry, *tmp;
 
@@ -1922,7 +1918,7 @@ static efi_status_t eficonfig_choice_change_boot_order(struct efimenu *efi_menu)
 		case KEY_UP:
 			if (efi_menu->active > 0)
 				--efi_menu->active;
-			return EFI_NOT_READY;
+			return NULL;
 		case KEY_MINUS:
 			if (efi_menu->active < efi_menu->count - 3) {
 				list_for_each_safe(pos, n, &efi_menu->list) {
@@ -1938,20 +1934,29 @@ static efi_status_t eficonfig_choice_change_boot_order(struct efimenu *efi_menu)
 
 				++efi_menu->active;
 			}
-			return EFI_NOT_READY;
+			return NULL;
 		case KEY_DOWN:
 			if (efi_menu->active < efi_menu->count - 1)
 				++efi_menu->active;
-			return EFI_NOT_READY;
+			return NULL;
 		case KEY_SELECT:
 			/* "Save" */
-			if (efi_menu->active == efi_menu->count - 2)
-				return EFI_SUCCESS;
-
+			if (efi_menu->active == efi_menu->count - 2) {
+				list_for_each_prev_safe(pos, n, &efi_menu->list) {
+					entry = list_entry(pos, struct eficonfig_entry, list);
+					if (entry->num == efi_menu->active)
+						break;
+				}
+				return entry->key;
+			}
 			/* "Quit" */
-			if (efi_menu->active == efi_menu->count - 1)
-				return EFI_ABORTED;
-
+			if (efi_menu->active == efi_menu->count - 1) {
+				entry = list_last_entry(&efi_menu->list,
+							struct eficonfig_entry,
+							list);
+				return entry->key;
+			}
+			/* Pressed key is not valid, wait next key press */
 			break;
 		case KEY_SPACE:
 			if (efi_menu->active < efi_menu->count - 2) {
@@ -1961,15 +1966,18 @@ static efi_status_t eficonfig_choice_change_boot_order(struct efimenu *efi_menu)
 						struct eficonfig_boot_order_data *data = entry->data;
 
 						data->active = !data->active;
-						return EFI_NOT_READY;
+						return NULL;
 					}
 				}
 			}
+			/* Pressed key is not valid, wait next key press */
 			break;
 		case KEY_QUIT:
-			return EFI_ABORTED;
+			entry = list_last_entry(&efi_menu->list,
+						struct eficonfig_entry, list);
+			return entry->key;
 		default:
-			/* Pressed key is not valid, no need to regenerate the menu */
+			/* Pressed key is not valid, wait next key press */
 			break;
 		}
 	}
@@ -2034,6 +2042,66 @@ out:
 	return ret;
 }
 
+/**
+ * eficonfig_process_save_boot_order() - callback function for "Save" entry
+ *
+ * @data:	pointer to the data
+ * Return:	status code
+ */
+static efi_status_t eficonfig_process_save_boot_order(void *data)
+{
+	u32 count = 0;
+	efi_status_t ret;
+	efi_uintn_t size;
+	struct list_head *pos, *n;
+	u16 *new_bootorder;
+	struct efimenu *efi_menu;
+	struct eficonfig_entry *entry;
+	struct eficonfig_save_boot_order_data *save_data = data;
+
+	efi_menu = save_data->efi_menu;
+
+	/*
+	 * The change boot order menu always has "Save" and "Quit" entries.
+	 * !(efi_menu->count - 2) means there is no user defined boot option.
+	 */
+	if (!(efi_menu->count - 2))
+		return EFI_SUCCESS;
+
+	new_bootorder = calloc(1, (efi_menu->count - 2) * sizeof(u16));
+	if (!new_bootorder) {
+		ret = EFI_OUT_OF_RESOURCES;
+		goto out;
+	}
+
+	/* create new BootOrder */
+	count = 0;
+	list_for_each_safe(pos, n, &efi_menu->list) {
+		struct eficonfig_boot_order_data *data;
+
+		entry = list_entry(pos, struct eficonfig_entry, list);
+		/* exit the loop when iteration reaches "Save" */
+		if (!strncmp(entry->title, "Save", strlen("Save")))
+			break;
+
+		data = entry->data;
+		if (data->active)
+			new_bootorder[count++] = data->boot_index;
+	}
+
+	size = count * sizeof(u16);
+	ret = efi_set_variable_int(u"BootOrder", &efi_global_variable_guid,
+				   EFI_VARIABLE_NON_VOLATILE |
+				   EFI_VARIABLE_BOOTSERVICE_ACCESS |
+				   EFI_VARIABLE_RUNTIME_ACCESS,
+				   size, new_bootorder, false);
+
+	save_data->selected = true;
+out:
+	free(new_bootorder);
+
+	return ret;
+}
 /**
  * eficonfig_create_change_boot_order_entry() - create boot order entry
  *
@@ -2050,6 +2118,7 @@ static efi_status_t eficonfig_create_change_boot_order_entry(struct efimenu *efi
 	efi_status_t ret;
 	u16 *var_name16 = NULL;
 	efi_uintn_t size, buf_size;
+	struct eficonfig_save_boot_order_data *save_data;
 
 	/* list the load option in the order of BootOrder variable */
 	for (i = 0; i < num; i++) {
@@ -2100,7 +2169,17 @@ static efi_status_t eficonfig_create_change_boot_order_entry(struct efimenu *efi
 		goto out;
 	}
 
-	ret = eficonfig_append_menu_entry(efi_menu, title, NULL, NULL);
+	save_data = malloc(sizeof(struct eficonfig_save_boot_order_data));
+	if (!save_data) {
+		ret = EFI_OUT_OF_RESOURCES;
+		goto out;
+	}
+	save_data->efi_menu = efi_menu;
+	save_data->selected = false;
+
+	ret = eficonfig_append_menu_entry(efi_menu, title,
+					  eficonfig_process_save_boot_order,
+					  save_data);
 	if (ret != EFI_SUCCESS)
 		goto out;
 
@@ -2123,7 +2202,6 @@ out:
  */
 static efi_status_t eficonfig_process_change_boot_order(void *data)
 {
-	u32 count;
 	u16 *bootorder;
 	efi_status_t ret;
 	efi_uintn_t num, size;
@@ -2144,47 +2222,24 @@ static efi_status_t eficonfig_process_change_boot_order(void *data)
 		goto out;
 
 	while (1) {
-		eficonfig_display_change_boot_order(efi_menu);
-
-		ret = eficonfig_choice_change_boot_order(efi_menu);
-		if (ret == EFI_SUCCESS) {
-			u16 *new_bootorder;
-
-			new_bootorder = calloc(1, (efi_menu->count - 2) * sizeof(u16));
-			if (!new_bootorder) {
-				ret = EFI_OUT_OF_RESOURCES;
-				goto out;
-			}
-
-			/* create new BootOrder  */
-			count = 0;
-			list_for_each_safe(pos, n, &efi_menu->list) {
-				struct eficonfig_boot_order_data *data;
-
+		ret = eficonfig_process_common(efi_menu,
+					       "  ** Change Boot Order **",
+					       eficonfig_change_boot_order_desc,
+					       eficonfig_display_statusline,
+					       eficonfig_print_change_boot_order_entry,
+					       eficonfig_choice_change_boot_order);
+		/* exit from the menu if user selects the "Save" entry. */
+		if (ret == EFI_SUCCESS && efi_menu->active == (efi_menu->count - 2)) {
+			list_for_each_prev_safe(pos, n, &efi_menu->list) {
 				entry = list_entry(pos, struct eficonfig_entry, list);
-				/* exit the loop when iteration reaches "Save" */
-				if (!strncmp(entry->title, "Save", strlen("Save")))
+				if (entry->num == efi_menu->active)
 					break;
-
-				data = entry->data;
-				if (data->active)
-					new_bootorder[count++] = data->boot_index;
 			}
-
-			size = count * sizeof(u16);
-			ret = efi_set_variable_int(u"BootOrder", &efi_global_variable_guid,
-						   EFI_VARIABLE_NON_VOLATILE |
-						   EFI_VARIABLE_BOOTSERVICE_ACCESS |
-						   EFI_VARIABLE_RUNTIME_ACCESS,
-						   size, new_bootorder, false);
-
-			free(new_bootorder);
-			goto out;
-		} else if (ret == EFI_NOT_READY) {
-			continue;
-		} else {
-			goto out;
+			if (((struct eficonfig_save_boot_order_data *)entry->data)->selected)
+				break;
 		}
+		if (ret != EFI_SUCCESS)
+			break;
 	}
 out:
 	free(bootorder);
-- 
2.17.1

Re: [PATCH v2 2/3] eficonfig: refactor change boot order implementation

[Ilias Apalodimas]

On Sat, Dec 24, 2022 at 07:57:43AM +0900, Masahisa Kojima wrote:
> This commit removes the change boot order specific
> menu implementation. The change boot order implementation
> calls eficonfig_process_common() same as other menus.
>
> The change boot order menu requires own item_data_print
> and item_choice implementation, but display_statusline
> function can be a same function as other menus.
>
> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> ---
> Changes in v2:
> - add comment when the user key press is not valid
> - add const qualifier to eficonfig_change_boot_order_desc
>
>  cmd/eficonfig.c | 245 +++++++++++++++++++++++++++++-------------------
>  1 file changed, 150 insertions(+), 95 deletions(-)
>
> diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
> index 2fc486dac2..13929cb003 100644
> --- a/cmd/eficonfig.c
> +++ b/cmd/eficonfig.c
> @@ -24,6 +24,11 @@ static struct efi_simple_text_input_protocol *cin;
>  const char *eficonfig_menu_desc =
>  	"  Press UP/DOWN to move, ENTER to select, ESC/CTRL+C to quit";
>
> +static const char *eficonfig_change_boot_order_desc =
> +	"  Press UP/DOWN to move, +/- to change orde\n"
> +	"  Press SPACE to activate or deactivate the entry\n"
> +	"  Select [Save] to complete, ESC/CTRL+C to quit";
> +
>  #define EFICONFIG_DESCRIPTION_MAX 32
>  #define EFICONFIG_OPTIONAL_DATA_MAX 64
>
> @@ -105,6 +110,17 @@ struct eficonfig_boot_order_data {
>  	bool active;
>  };
>
> +/**
> + * struct eficonfig_save_boot_order_data - structure to be used to change boot order
> + *
> + * @efi_menu:		pointer to efimenu structure
> + * @selected:		flag to indicate user selects "Save" entry
> + */
> +struct eficonfig_save_boot_order_data {
> +	struct efimenu *efi_menu;
> +	bool selected;
> +};
> +
>  /**
>   * eficonfig_print_msg() - print message
>   *
> @@ -173,10 +189,9 @@ void eficonfig_display_statusline(struct menu *m)
>  	      "\n%s\n"
>  	       ANSI_CURSOR_POSITION ANSI_CLEAR_LINE ANSI_CURSOR_POSITION
>  	       "%s"
> -	       ANSI_CLEAR_LINE_TO_END ANSI_CURSOR_POSITION ANSI_CLEAR_LINE,
> +	       ANSI_CLEAR_LINE_TO_END,
>  	       1, 1, entry->efi_menu->menu_header, entry->efi_menu->count + 5, 1,
> -	       entry->efi_menu->count + 6, 1, entry->efi_menu->menu_desc,
> -	       entry->efi_menu->count + 7, 1);
> +	       entry->efi_menu->count + 6, 1, entry->efi_menu->menu_desc);
>  }
>
>  /**
> @@ -1841,63 +1856,44 @@ out:
>  }
>
>  /**
> - * eficonfig_display_change_boot_order() - display the BootOrder list
> + * eficonfig_print_change_boot_order_entry() - print the boot option entry
>   *
> - * @efi_menu:	pointer to the efimenu structure
> - * Return:	status code
> + * @data:	pointer to the data associated with each menu entry
>   */
> -static void eficonfig_display_change_boot_order(struct efimenu *efi_menu)
> +static void eficonfig_print_change_boot_order_entry(void *data)
>  {
> -	bool reverse;
> -	struct list_head *pos, *n;
> -	struct eficonfig_entry *entry;
> -
> -	printf(ANSI_CLEAR_CONSOLE ANSI_CURSOR_POSITION
> -	       "\n  ** Change Boot Order **\n"
> -	       ANSI_CURSOR_POSITION
> -	       "  Press UP/DOWN to move, +/- to change order"
> -	       ANSI_CURSOR_POSITION
> -	       "  Press SPACE to activate or deactivate the entry"
> -	       ANSI_CURSOR_POSITION
> -	       "  Select [Save] to complete, ESC/CTRL+C to quit"
> -	       ANSI_CURSOR_POSITION ANSI_CLEAR_LINE,
> -	       1, 1, efi_menu->count + 5, 1, efi_menu->count + 6, 1,
> -	       efi_menu->count + 7, 1,  efi_menu->count + 8, 1);
> -
> -	/* draw boot option list */
> -	list_for_each_safe(pos, n, &efi_menu->list) {
> -		entry = list_entry(pos, struct eficonfig_entry, list);
> -		reverse = (entry->num == efi_menu->active);
> +	struct eficonfig_entry *entry = data;
> +	int reverse = (entry->efi_menu->active == entry->num);

Why did we change this from bool?

[...]

> -		printf(ANSI_CURSOR_POSITION, entry->num + 4, 7);
>  }
>
>  /**
> - * eficonfig_choice_change_boot_order() - handle the BootOrder update
> + * eficonfig_choice_change_boot_order() - user key input handler
>   *
> - * @efi_menu:	pointer to the efimenu structure
> - * Return:	status code
> + * @data:	pointer to the menu entry
> + * Return:	key string to identify the selected entry
>   */
> -static efi_status_t eficonfig_choice_change_boot_order(struct efimenu *efi_menu)
>  }
>
> +/**
> + * eficonfig_process_save_boot_order() - callback function for "Save" entry
> + *
> + * @data:	pointer to the data
> + * Return:	status code
> + */
> +static efi_status_t eficonfig_process_save_boot_order(void *data)
> +{
> +	u32 count = 0;
> +	efi_status_t ret;
> +	efi_uintn_t size;
> +	struct list_head *pos, *n;
> +	u16 *new_bootorder;
> +	struct efimenu *efi_menu;
> +	struct eficonfig_entry *entry;
> +	struct eficonfig_save_boot_order_data *save_data = data;
> +
> +	efi_menu = save_data->efi_menu;
> +
> +	/*
> +	 * The change boot order menu always has "Save" and "Quit" entries.
[...]


Acked-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Re: [PATCH v2 2/3] eficonfig: refactor change boot order implementation

[Masahisa Kojima]

On Wed, 28 Dec 2022 at 00:05, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> On Sat, Dec 24, 2022 at 07:57:43AM +0900, Masahisa Kojima wrote:
> > This commit removes the change boot order specific
> > menu implementation. The change boot order implementation
> > calls eficonfig_process_common() same as other menus.
> >
> > The change boot order menu requires own item_data_print
> > and item_choice implementation, but display_statusline
> > function can be a same function as other menus.
> >
> > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > ---
> > Changes in v2:
> > - add comment when the user key press is not valid
> > - add const qualifier to eficonfig_change_boot_order_desc
> >
> >  cmd/eficonfig.c | 245 +++++++++++++++++++++++++++++-------------------
> >  1 file changed, 150 insertions(+), 95 deletions(-)
> >
> > diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
> > index 2fc486dac2..13929cb003 100644
> > --- a/cmd/eficonfig.c
> > +++ b/cmd/eficonfig.c
> > @@ -24,6 +24,11 @@ static struct efi_simple_text_input_protocol *cin;
> >  const char *eficonfig_menu_desc =
> >       "  Press UP/DOWN to move, ENTER to select, ESC/CTRL+C to quit";
> >
> > +static const char *eficonfig_change_boot_order_desc =
> > +     "  Press UP/DOWN to move, +/- to change orde\n"
> > +     "  Press SPACE to activate or deactivate the entry\n"
> > +     "  Select [Save] to complete, ESC/CTRL+C to quit";
> > +
> >  #define EFICONFIG_DESCRIPTION_MAX 32
> >  #define EFICONFIG_OPTIONAL_DATA_MAX 64
> >
> > @@ -105,6 +110,17 @@ struct eficonfig_boot_order_data {
> >       bool active;
> >  };
> >
> > +/**
> > + * struct eficonfig_save_boot_order_data - structure to be used to change boot order
> > + *
> > + * @efi_menu:                pointer to efimenu structure
> > + * @selected:                flag to indicate user selects "Save" entry
> > + */
> > +struct eficonfig_save_boot_order_data {
> > +     struct efimenu *efi_menu;
> > +     bool selected;
> > +};
> > +
> >  /**
> >   * eficonfig_print_msg() - print message
> >   *
> > @@ -173,10 +189,9 @@ void eficonfig_display_statusline(struct menu *m)
> >             "\n%s\n"
> >              ANSI_CURSOR_POSITION ANSI_CLEAR_LINE ANSI_CURSOR_POSITION
> >              "%s"
> > -            ANSI_CLEAR_LINE_TO_END ANSI_CURSOR_POSITION ANSI_CLEAR_LINE,
> > +            ANSI_CLEAR_LINE_TO_END,
> >              1, 1, entry->efi_menu->menu_header, entry->efi_menu->count + 5, 1,
> > -            entry->efi_menu->count + 6, 1, entry->efi_menu->menu_desc,
> > -            entry->efi_menu->count + 7, 1);
> > +            entry->efi_menu->count + 6, 1, entry->efi_menu->menu_desc);
> >  }
> >
> >  /**
> > @@ -1841,63 +1856,44 @@ out:
> >  }
> >
> >  /**
> > - * eficonfig_display_change_boot_order() - display the BootOrder list
> > + * eficonfig_print_change_boot_order_entry() - print the boot option entry
> >   *
> > - * @efi_menu:        pointer to the efimenu structure
> > - * Return:   status code
> > + * @data:    pointer to the data associated with each menu entry
> >   */
> > -static void eficonfig_display_change_boot_order(struct efimenu *efi_menu)
> > +static void eficonfig_print_change_boot_order_entry(void *data)
> >  {
> > -     bool reverse;
> > -     struct list_head *pos, *n;
> > -     struct eficonfig_entry *entry;
> > -
> > -     printf(ANSI_CLEAR_CONSOLE ANSI_CURSOR_POSITION
> > -            "\n  ** Change Boot Order **\n"
> > -            ANSI_CURSOR_POSITION
> > -            "  Press UP/DOWN to move, +/- to change order"
> > -            ANSI_CURSOR_POSITION
> > -            "  Press SPACE to activate or deactivate the entry"
> > -            ANSI_CURSOR_POSITION
> > -            "  Select [Save] to complete, ESC/CTRL+C to quit"
> > -            ANSI_CURSOR_POSITION ANSI_CLEAR_LINE,
> > -            1, 1, efi_menu->count + 5, 1, efi_menu->count + 6, 1,
> > -            efi_menu->count + 7, 1,  efi_menu->count + 8, 1);
> > -
> > -     /* draw boot option list */
> > -     list_for_each_safe(pos, n, &efi_menu->list) {
> > -             entry = list_entry(pos, struct eficonfig_entry, list);
> > -             reverse = (entry->num == efi_menu->active);
> > +     struct eficonfig_entry *entry = data;
> > +     int reverse = (entry->efi_menu->active == entry->num);
>
> Why did we change this from bool?

This should be bool. I will fix it.

Thanks,
Masahisa Kojima

>
> [...]
>
> > -             printf(ANSI_CURSOR_POSITION, entry->num + 4, 7);
> >  }
> >
> >  /**
> > - * eficonfig_choice_change_boot_order() - handle the BootOrder update
> > + * eficonfig_choice_change_boot_order() - user key input handler
> >   *
> > - * @efi_menu:        pointer to the efimenu structure
> > - * Return:   status code
> > + * @data:    pointer to the menu entry
> > + * Return:   key string to identify the selected entry
> >   */
> > -static efi_status_t eficonfig_choice_change_boot_order(struct efimenu *efi_menu)
> >  }
> >
> > +/**
> > + * eficonfig_process_save_boot_order() - callback function for "Save" entry
> > + *
> > + * @data:    pointer to the data
> > + * Return:   status code
> > + */
> > +static efi_status_t eficonfig_process_save_boot_order(void *data)
> > +{
> > +     u32 count = 0;
> > +     efi_status_t ret;
> > +     efi_uintn_t size;
> > +     struct list_head *pos, *n;
> > +     u16 *new_bootorder;
> > +     struct efimenu *efi_menu;
> > +     struct eficonfig_entry *entry;
> > +     struct eficonfig_save_boot_order_data *save_data = data;
> > +
> > +     efi_menu = save_data->efi_menu;
> > +
> > +     /*
> > +      * The change boot order menu always has "Save" and "Quit" entries.
> [...]
>
>
> Acked-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>

[PATCH v2 3/3] eficonfig: add vertical scroll support

[Masahisa Kojima]

The current eficonfig menu does not support vertical scroll,
so it can not display the menu entries greater than
the console row size.

This commit add the vertial scroll support.
The console size is retrieved by
SIMPLE_TEXT_OUTPUT_PROTOCOL.QueryMode() service, then
calculates the row size for menu entry by subtracting
menu header and description row size from the console row size.
"start" and "end" are added in the efimenu structure.
"start" keeps the menu entry index at the top, "end" keeps
the bottom menu entry index. item_data_print() menu function
only draws the menu entry between "start" and "end".

Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
---
No update since v1

 cmd/eficonfig.c      | 79 ++++++++++++++++++++++++++++++++++++--------
 include/efi_config.h |  4 +++
 include/efi_loader.h |  1 +
 3 files changed, 70 insertions(+), 14 deletions(-)

diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
index 13929cb003..e289d95589 100644
--- a/cmd/eficonfig.c
+++ b/cmd/eficonfig.c
@@ -29,8 +29,13 @@ static const char *eficonfig_change_boot_order_desc =
 	"  Press SPACE to activate or deactivate the entry\n"
 	"  Select [Save] to complete, ESC/CTRL+C to quit";
 
+static struct efi_simple_text_output_protocol *cout;
+static int avail_row;
+
 #define EFICONFIG_DESCRIPTION_MAX 32
 #define EFICONFIG_OPTIONAL_DATA_MAX 64
+#define EFICONFIG_MENU_HEADER_ROW_NUM 3
+#define EFICONFIG_MENU_DESC_ROW_NUM 5
 
 /**
  * struct eficonfig_filepath_info - structure to be used to store file path
@@ -156,18 +161,16 @@ void eficonfig_print_entry(void *data)
 	struct eficonfig_entry *entry = data;
 	int reverse = (entry->efi_menu->active == entry->num);
 
-	/* TODO: support scroll or page for many entries */
+	if (entry->efi_menu->start > entry->num || entry->efi_menu->end < entry->num)
+		return;
 
-	/*
-	 * Move cursor to line where the entry will be drawn (entry->num)
-	 * First 3 lines(menu header) + 1 empty line
-	 */
-	printf(ANSI_CURSOR_POSITION, entry->num + 4, 7);
+	printf(ANSI_CURSOR_POSITION, (entry->num - entry->efi_menu->start) +
+	       EFICONFIG_MENU_HEADER_ROW_NUM + 1, 7);
 
 	if (reverse)
 		puts(ANSI_COLOR_REVERSE);
 
-	printf("%s", entry->title);
+	printf(ANSI_CLEAR_LINE "%s", entry->title);
 
 	if (reverse)
 		puts(ANSI_COLOR_RESET);
@@ -190,8 +193,8 @@ void eficonfig_display_statusline(struct menu *m)
 	       ANSI_CURSOR_POSITION ANSI_CLEAR_LINE ANSI_CURSOR_POSITION
 	       "%s"
 	       ANSI_CLEAR_LINE_TO_END,
-	       1, 1, entry->efi_menu->menu_header, entry->efi_menu->count + 5, 1,
-	       entry->efi_menu->count + 6, 1, entry->efi_menu->menu_desc);
+	       1, 1, entry->efi_menu->menu_header, avail_row + 4, 1,
+	       avail_row + 5, 1, entry->efi_menu->menu_desc);
 }
 
 /**
@@ -213,13 +216,23 @@ char *eficonfig_choice_entry(void *data)
 
 		switch (key) {
 		case KEY_UP:
-			if (efi_menu->active > 0)
+			if (efi_menu->active > 0) {
 				--efi_menu->active;
+				if (efi_menu->start > efi_menu->active) {
+					efi_menu->start--;
+					efi_menu->end--;
+				}
+			}
 			/* no menu key selected, regenerate menu */
 			return NULL;
 		case KEY_DOWN:
-			if (efi_menu->active < efi_menu->count - 1)
+			if (efi_menu->active < efi_menu->count - 1) {
 				++efi_menu->active;
+				if (efi_menu->end < efi_menu->active) {
+					efi_menu->start++;
+					efi_menu->end++;
+				}
+			}
 			/* no menu key selected, regenerate menu */
 			return NULL;
 		case KEY_SELECT:
@@ -399,6 +412,8 @@ efi_status_t eficonfig_process_common(struct efimenu *efi_menu,
 
 	efi_menu->delay = -1;
 	efi_menu->active = 0;
+	efi_menu->start = 0;
+	efi_menu->end = avail_row - 1;
 
 	if (menu_header) {
 		efi_menu->menu_header = strdup(menu_header);
@@ -1865,7 +1880,11 @@ static void eficonfig_print_change_boot_order_entry(void *data)
 	struct eficonfig_entry *entry = data;
 	int reverse = (entry->efi_menu->active == entry->num);
 
-	printf(ANSI_CURSOR_POSITION, entry->num + 4, 7);
+	if (entry->efi_menu->start > entry->num || entry->efi_menu->end < entry->num)
+		return;
+
+	printf(ANSI_CURSOR_POSITION ANSI_CLEAR_LINE,
+	       (entry->num - entry->efi_menu->start) + EFICONFIG_MENU_HEADER_ROW_NUM + 1, 7);
 
 	if (reverse)
 		puts(ANSI_COLOR_REVERSE);
@@ -1916,8 +1935,13 @@ char *eficonfig_choice_change_boot_order(void *data)
 			}
 			fallthrough;
 		case KEY_UP:
-			if (efi_menu->active > 0)
+			if (efi_menu->active > 0) {
 				--efi_menu->active;
+				if (efi_menu->start > efi_menu->active) {
+					efi_menu->start--;
+					efi_menu->end--;
+				}
+			}
 			return NULL;
 		case KEY_MINUS:
 			if (efi_menu->active < efi_menu->count - 3) {
@@ -1933,11 +1957,20 @@ char *eficonfig_choice_change_boot_order(void *data)
 				list_add(&entry->list, &tmp->list);
 
 				++efi_menu->active;
+				if (efi_menu->end < efi_menu->active) {
+					efi_menu->start++;
+					efi_menu->end++;
+				}
 			}
 			return NULL;
 		case KEY_DOWN:
-			if (efi_menu->active < efi_menu->count - 1)
+			if (efi_menu->active < efi_menu->count - 1) {
 				++efi_menu->active;
+				if (efi_menu->end < efi_menu->active) {
+					efi_menu->start++;
+					efi_menu->end++;
+				}
+			}
 			return NULL;
 		case KEY_SELECT:
 			/* "Save" */
@@ -2585,6 +2618,7 @@ static efi_status_t eficonfig_init(void)
 	efi_status_t ret = EFI_SUCCESS;
 	static bool init;
 	struct efi_handler *handler;
+	unsigned long columns, rows;
 
 	if (!init) {
 		ret = efi_search_protocol(efi_root, &efi_guid_text_input_protocol, &handler);
@@ -2595,6 +2629,23 @@ static efi_status_t eficonfig_init(void)
 					EFI_OPEN_PROTOCOL_GET_PROTOCOL);
 		if (ret != EFI_SUCCESS)
 			return ret;
+		ret = efi_search_protocol(efi_root, &efi_guid_text_output_protocol, &handler);
+		if (ret != EFI_SUCCESS)
+			return ret;
+
+		ret = efi_protocol_open(handler, (void **)&cout, efi_root, NULL,
+					EFI_OPEN_PROTOCOL_GET_PROTOCOL);
+		if (ret != EFI_SUCCESS)
+			return ret;
+
+		cout->query_mode(cout, cout->mode->mode, &columns, &rows);
+		avail_row = rows - (EFICONFIG_MENU_HEADER_ROW_NUM +
+				    EFICONFIG_MENU_DESC_ROW_NUM);
+		if (avail_row <= 0) {
+			eficonfig_print_msg("Console size is too small!");
+			return EFI_INVALID_PARAMETER;
+		}
+		/* TODO: Should we check the minimum column size? */
 	}
 
 	init = true;
diff --git a/include/efi_config.h b/include/efi_config.h
index cec5715f84..6a104e4b1d 100644
--- a/include/efi_config.h
+++ b/include/efi_config.h
@@ -49,6 +49,8 @@ struct eficonfig_entry {
  * @menu_header:	menu header string
  * @menu_desc:		menu description string
  * @list:		menu entry list structure
+ * @start:		top menu index to draw
+ * @end:		bottom menu index to draw
  */
 struct efimenu {
 	int delay;
@@ -57,6 +59,8 @@ struct efimenu {
 	char *menu_header;
 	const char *menu_desc;
 	struct list_head list;
+	int start;
+	int end;
 };
 
 /**
diff --git a/include/efi_loader.h b/include/efi_loader.h
index 699176872d..91d8a5ef0c 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -328,6 +328,7 @@ extern const efi_guid_t efi_esrt_guid;
 extern const efi_guid_t smbios_guid;
 /*GUID of console */
 extern const efi_guid_t efi_guid_text_input_protocol;
+extern const efi_guid_t efi_guid_text_output_protocol;
 
 extern char __efi_runtime_start[], __efi_runtime_stop[];
 extern char __efi_runtime_rel_start[], __efi_runtime_rel_stop[];
-- 
2.17.1

Re: [PATCH v2 3/3] eficonfig: add vertical scroll support

[Ilias Apalodimas]

This looks correct to me.
Heinrich can you check if that solves your scrolling issues?

On Sat, Dec 24, 2022 at 07:57:44AM +0900, Masahisa Kojima wrote:
> The current eficonfig menu does not support vertical scroll,
> so it can not display the menu entries greater than
> the console row size.
>
> This commit add the vertial scroll support.
> The console size is retrieved by
> SIMPLE_TEXT_OUTPUT_PROTOCOL.QueryMode() service, then
> calculates the row size for menu entry by subtracting
> menu header and description row size from the console row size.
> "start" and "end" are added in the efimenu structure.
> "start" keeps the menu entry index at the top, "end" keeps
> the bottom menu entry index. item_data_print() menu function
> only draws the menu entry between "start" and "end".
>
> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> ---
> No update since v1
>
>  cmd/eficonfig.c      | 79 ++++++++++++++++++++++++++++++++++++--------
>  include/efi_config.h |  4 +++
>  include/efi_loader.h |  1 +
>  3 files changed, 70 insertions(+), 14 deletions(-)
>
> diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
> index 13929cb003..e289d95589 100644
> --- a/cmd/eficonfig.c
> +++ b/cmd/eficonfig.c
> @@ -29,8 +29,13 @@ static const char *eficonfig_change_boot_order_desc =
>  	"  Press SPACE to activate or deactivate the entry\n"
>  	"  Select [Save] to complete, ESC/CTRL+C to quit";
>
> +static struct efi_simple_text_output_protocol *cout;
> +static int avail_row;
> +
>  #define EFICONFIG_DESCRIPTION_MAX 32
>  #define EFICONFIG_OPTIONAL_DATA_MAX 64
> +#define EFICONFIG_MENU_HEADER_ROW_NUM 3
> +#define EFICONFIG_MENU_DESC_ROW_NUM 5
>
>  /**
>   * struct eficonfig_filepath_info - structure to be used to store file path
> @@ -156,18 +161,16 @@ void eficonfig_print_entry(void *data)
>  	struct eficonfig_entry *entry = data;
>  	int reverse = (entry->efi_menu->active == entry->num);
>
> -	/* TODO: support scroll or page for many entries */
> +	if (entry->efi_menu->start > entry->num || entry->efi_menu->end < entry->num)
> +		return;
>
> -	/*
> -	 * Move cursor to line where the entry will be drawn (entry->num)
> -	 * First 3 lines(menu header) + 1 empty line
> -	 */
> -	printf(ANSI_CURSOR_POSITION, entry->num + 4, 7);
> +	printf(ANSI_CURSOR_POSITION, (entry->num - entry->efi_menu->start) +
> +	       EFICONFIG_MENU_HEADER_ROW_NUM + 1, 7);
>
>  	if (reverse)
>  		puts(ANSI_COLOR_REVERSE);
>
> -	printf("%s", entry->title);
> +	printf(ANSI_CLEAR_LINE "%s", entry->title);
>
>  	if (reverse)
>  		puts(ANSI_COLOR_RESET);
> @@ -190,8 +193,8 @@ void eficonfig_display_statusline(struct menu *m)
>  	       ANSI_CURSOR_POSITION ANSI_CLEAR_LINE ANSI_CURSOR_POSITION
>  	       "%s"
>  	       ANSI_CLEAR_LINE_TO_END,
> -	       1, 1, entry->efi_menu->menu_header, entry->efi_menu->count + 5, 1,
> -	       entry->efi_menu->count + 6, 1, entry->efi_menu->menu_desc);
> +	       1, 1, entry->efi_menu->menu_header, avail_row + 4, 1,
> +	       avail_row + 5, 1, entry->efi_menu->menu_desc);
>  }
>
>  /**
> @@ -213,13 +216,23 @@ char *eficonfig_choice_entry(void *data)
>
>  		switch (key) {
>  		case KEY_UP:
> -			if (efi_menu->active > 0)
> +			if (efi_menu->active > 0) {
>  				--efi_menu->active;
> +				if (efi_menu->start > efi_menu->active) {
> +					efi_menu->start--;
> +					efi_menu->end--;
> +				}
> +			}
>  			/* no menu key selected, regenerate menu */
>  			return NULL;
>  		case KEY_DOWN:
> -			if (efi_menu->active < efi_menu->count - 1)
> +			if (efi_menu->active < efi_menu->count - 1) {
>  				++efi_menu->active;
> +				if (efi_menu->end < efi_menu->active) {
> +					efi_menu->start++;
> +					efi_menu->end++;
> +				}
> +			}
>  			/* no menu key selected, regenerate menu */
>  			return NULL;
>  		case KEY_SELECT:
> @@ -399,6 +412,8 @@ efi_status_t eficonfig_process_common(struct efimenu *efi_menu,
>
>  	efi_menu->delay = -1;
>  	efi_menu->active = 0;
> +	efi_menu->start = 0;
> +	efi_menu->end = avail_row - 1;
>
>  	if (menu_header) {
>  		efi_menu->menu_header = strdup(menu_header);
> @@ -1865,7 +1880,11 @@ static void eficonfig_print_change_boot_order_entry(void *data)
>  	struct eficonfig_entry *entry = data;
>  	int reverse = (entry->efi_menu->active == entry->num);
>
> -	printf(ANSI_CURSOR_POSITION, entry->num + 4, 7);
> +	if (entry->efi_menu->start > entry->num || entry->efi_menu->end < entry->num)
> +		return;
> +
> +	printf(ANSI_CURSOR_POSITION ANSI_CLEAR_LINE,
> +	       (entry->num - entry->efi_menu->start) + EFICONFIG_MENU_HEADER_ROW_NUM + 1, 7);
>
>  	if (reverse)
>  		puts(ANSI_COLOR_REVERSE);
> @@ -1916,8 +1935,13 @@ char *eficonfig_choice_change_boot_order(void *data)
>  			}
>  			fallthrough;
>  		case KEY_UP:
> -			if (efi_menu->active > 0)
> +			if (efi_menu->active > 0) {
>  				--efi_menu->active;
> +				if (efi_menu->start > efi_menu->active) {
> +					efi_menu->start--;
> +					efi_menu->end--;
> +				}
> +			}
>  			return NULL;
>  		case KEY_MINUS:
>  			if (efi_menu->active < efi_menu->count - 3) {
> @@ -1933,11 +1957,20 @@ char *eficonfig_choice_change_boot_order(void *data)
>  				list_add(&entry->list, &tmp->list);
>
>  				++efi_menu->active;
> +				if (efi_menu->end < efi_menu->active) {
> +					efi_menu->start++;
> +					efi_menu->end++;
> +				}
>  			}
>  			return NULL;
>  		case KEY_DOWN:
> -			if (efi_menu->active < efi_menu->count - 1)
> +			if (efi_menu->active < efi_menu->count - 1) {
>  				++efi_menu->active;
> +				if (efi_menu->end < efi_menu->active) {
> +					efi_menu->start++;
> +					efi_menu->end++;
> +				}
> +			}
>  			return NULL;
>  		case KEY_SELECT:
>  			/* "Save" */
> @@ -2585,6 +2618,7 @@ static efi_status_t eficonfig_init(void)
>  	efi_status_t ret = EFI_SUCCESS;
>  	static bool init;
>  	struct efi_handler *handler;
> +	unsigned long columns, rows;
>
>  	if (!init) {
>  		ret = efi_search_protocol(efi_root, &efi_guid_text_input_protocol, &handler);
> @@ -2595,6 +2629,23 @@ static efi_status_t eficonfig_init(void)
>  					EFI_OPEN_PROTOCOL_GET_PROTOCOL);
>  		if (ret != EFI_SUCCESS)
>  			return ret;
> +		ret = efi_search_protocol(efi_root, &efi_guid_text_output_protocol, &handler);
> +		if (ret != EFI_SUCCESS)
> +			return ret;
> +
> +		ret = efi_protocol_open(handler, (void **)&cout, efi_root, NULL,
> +					EFI_OPEN_PROTOCOL_GET_PROTOCOL);

Shouldn't we be closing the protocol if eficonfig terminates?

> +		if (ret != EFI_SUCCESS)
> +			return ret;
> +
> +		cout->query_mode(cout, cout->mode->mode, &columns, &rows);

[...]

Regards
/Ilias

Re: [PATCH v2 3/3] eficonfig: add vertical scroll support

[Masahisa Kojima]

On Tue, 27 Dec 2022 at 23:55, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
>
> This looks correct to me.
> Heinrich can you check if that solves your scrolling issues?
>
> On Sat, Dec 24, 2022 at 07:57:44AM +0900, Masahisa Kojima wrote:
> > The current eficonfig menu does not support vertical scroll,
> > so it can not display the menu entries greater than
> > the console row size.
> >
> > This commit add the vertial scroll support.
> > The console size is retrieved by
> > SIMPLE_TEXT_OUTPUT_PROTOCOL.QueryMode() service, then
> > calculates the row size for menu entry by subtracting
> > menu header and description row size from the console row size.
> > "start" and "end" are added in the efimenu structure.
> > "start" keeps the menu entry index at the top, "end" keeps
> > the bottom menu entry index. item_data_print() menu function
> > only draws the menu entry between "start" and "end".
> >
> > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > ---
> > No update since v1
> >
> >  cmd/eficonfig.c      | 79 ++++++++++++++++++++++++++++++++++++--------
> >  include/efi_config.h |  4 +++
> >  include/efi_loader.h |  1 +
> >  3 files changed, 70 insertions(+), 14 deletions(-)
> >
> > diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
> > index 13929cb003..e289d95589 100644
> > --- a/cmd/eficonfig.c
> > +++ b/cmd/eficonfig.c
> > @@ -29,8 +29,13 @@ static const char *eficonfig_change_boot_order_desc =
> >       "  Press SPACE to activate or deactivate the entry\n"
> >       "  Select [Save] to complete, ESC/CTRL+C to quit";
> >
> > +static struct efi_simple_text_output_protocol *cout;
> > +static int avail_row;
> > +
> >  #define EFICONFIG_DESCRIPTION_MAX 32
> >  #define EFICONFIG_OPTIONAL_DATA_MAX 64
> > +#define EFICONFIG_MENU_HEADER_ROW_NUM 3
> > +#define EFICONFIG_MENU_DESC_ROW_NUM 5
> >
> >  /**
> >   * struct eficonfig_filepath_info - structure to be used to store file path
> > @@ -156,18 +161,16 @@ void eficonfig_print_entry(void *data)
> >       struct eficonfig_entry *entry = data;
> >       int reverse = (entry->efi_menu->active == entry->num);
> >
> > -     /* TODO: support scroll or page for many entries */
> > +     if (entry->efi_menu->start > entry->num || entry->efi_menu->end < entry->num)
> > +             return;
> >
> > -     /*
> > -      * Move cursor to line where the entry will be drawn (entry->num)
> > -      * First 3 lines(menu header) + 1 empty line
> > -      */
> > -     printf(ANSI_CURSOR_POSITION, entry->num + 4, 7);
> > +     printf(ANSI_CURSOR_POSITION, (entry->num - entry->efi_menu->start) +
> > +            EFICONFIG_MENU_HEADER_ROW_NUM + 1, 7);
> >
> >       if (reverse)
> >               puts(ANSI_COLOR_REVERSE);
> >
> > -     printf("%s", entry->title);
> > +     printf(ANSI_CLEAR_LINE "%s", entry->title);
> >
> >       if (reverse)
> >               puts(ANSI_COLOR_RESET);
> > @@ -190,8 +193,8 @@ void eficonfig_display_statusline(struct menu *m)
> >              ANSI_CURSOR_POSITION ANSI_CLEAR_LINE ANSI_CURSOR_POSITION
> >              "%s"
> >              ANSI_CLEAR_LINE_TO_END,
> > -            1, 1, entry->efi_menu->menu_header, entry->efi_menu->count + 5, 1,
> > -            entry->efi_menu->count + 6, 1, entry->efi_menu->menu_desc);
> > +            1, 1, entry->efi_menu->menu_header, avail_row + 4, 1,
> > +            avail_row + 5, 1, entry->efi_menu->menu_desc);
> >  }
> >
> >  /**
> > @@ -213,13 +216,23 @@ char *eficonfig_choice_entry(void *data)
> >
> >               switch (key) {
> >               case KEY_UP:
> > -                     if (efi_menu->active > 0)
> > +                     if (efi_menu->active > 0) {
> >                               --efi_menu->active;
> > +                             if (efi_menu->start > efi_menu->active) {
> > +                                     efi_menu->start--;
> > +                                     efi_menu->end--;
> > +                             }
> > +                     }
> >                       /* no menu key selected, regenerate menu */
> >                       return NULL;
> >               case KEY_DOWN:
> > -                     if (efi_menu->active < efi_menu->count - 1)
> > +                     if (efi_menu->active < efi_menu->count - 1) {
> >                               ++efi_menu->active;
> > +                             if (efi_menu->end < efi_menu->active) {
> > +                                     efi_menu->start++;
> > +                                     efi_menu->end++;
> > +                             }
> > +                     }
> >                       /* no menu key selected, regenerate menu */
> >                       return NULL;
> >               case KEY_SELECT:
> > @@ -399,6 +412,8 @@ efi_status_t eficonfig_process_common(struct efimenu *efi_menu,
> >
> >       efi_menu->delay = -1;
> >       efi_menu->active = 0;
> > +     efi_menu->start = 0;
> > +     efi_menu->end = avail_row - 1;
> >
> >       if (menu_header) {
> >               efi_menu->menu_header = strdup(menu_header);
> > @@ -1865,7 +1880,11 @@ static void eficonfig_print_change_boot_order_entry(void *data)
> >       struct eficonfig_entry *entry = data;
> >       int reverse = (entry->efi_menu->active == entry->num);
> >
> > -     printf(ANSI_CURSOR_POSITION, entry->num + 4, 7);
> > +     if (entry->efi_menu->start > entry->num || entry->efi_menu->end < entry->num)
> > +             return;
> > +
> > +     printf(ANSI_CURSOR_POSITION ANSI_CLEAR_LINE,
> > +            (entry->num - entry->efi_menu->start) + EFICONFIG_MENU_HEADER_ROW_NUM + 1, 7);
> >
> >       if (reverse)
> >               puts(ANSI_COLOR_REVERSE);
> > @@ -1916,8 +1935,13 @@ char *eficonfig_choice_change_boot_order(void *data)
> >                       }
> >                       fallthrough;
> >               case KEY_UP:
> > -                     if (efi_menu->active > 0)
> > +                     if (efi_menu->active > 0) {
> >                               --efi_menu->active;
> > +                             if (efi_menu->start > efi_menu->active) {
> > +                                     efi_menu->start--;
> > +                                     efi_menu->end--;
> > +                             }
> > +                     }
> >                       return NULL;
> >               case KEY_MINUS:
> >                       if (efi_menu->active < efi_menu->count - 3) {
> > @@ -1933,11 +1957,20 @@ char *eficonfig_choice_change_boot_order(void *data)
> >                               list_add(&entry->list, &tmp->list);
> >
> >                               ++efi_menu->active;
> > +                             if (efi_menu->end < efi_menu->active) {
> > +                                     efi_menu->start++;
> > +                                     efi_menu->end++;
> > +                             }
> >                       }
> >                       return NULL;
> >               case KEY_DOWN:
> > -                     if (efi_menu->active < efi_menu->count - 1)
> > +                     if (efi_menu->active < efi_menu->count - 1) {
> >                               ++efi_menu->active;
> > +                             if (efi_menu->end < efi_menu->active) {
> > +                                     efi_menu->start++;
> > +                                     efi_menu->end++;
> > +                             }
> > +                     }
> >                       return NULL;
> >               case KEY_SELECT:
> >                       /* "Save" */
> > @@ -2585,6 +2618,7 @@ static efi_status_t eficonfig_init(void)
> >       efi_status_t ret = EFI_SUCCESS;
> >       static bool init;
> >       struct efi_handler *handler;
> > +     unsigned long columns, rows;
> >
> >       if (!init) {
> >               ret = efi_search_protocol(efi_root, &efi_guid_text_input_protocol, &handler);
> > @@ -2595,6 +2629,23 @@ static efi_status_t eficonfig_init(void)
> >                                       EFI_OPEN_PROTOCOL_GET_PROTOCOL);
> >               if (ret != EFI_SUCCESS)
> >                       return ret;
> > +             ret = efi_search_protocol(efi_root, &efi_guid_text_output_protocol, &handler);
> > +             if (ret != EFI_SUCCESS)
> > +                     return ret;
> > +
> > +             ret = efi_protocol_open(handler, (void **)&cout, efi_root, NULL,
> > +                                     EFI_OPEN_PROTOCOL_GET_PROTOCOL);
>
> Shouldn't we be closing the protocol if eficonfig terminates?

This protocol is opened with the EFI_OPEN_PROTOCOL_GET_PROTOCOL attribute,
so we don't need to close the protocol.

Thanks,
Masahisa Kojima

>
> > +             if (ret != EFI_SUCCESS)
> > +                     return ret;
> > +
> > +             cout->query_mode(cout, cout->mode->mode, &columns, &rows);
>
> [...]
>
> Regards
> /Ilias