[PATCH] menu: fix the logic checking whether ESC key is pressed
It's observed that the bootmenu on a serial console sometimes incorrectly quitted with superfluous characters filled to command line input:
*** U-Boot Boot Menu ***
1. Startup system (Default) 2. Upgrade firmware 3. Upgrade ATF BL2 4. Upgrade ATF FIP 5. Load image 0. U-Boot consolePress UP/DOWN to move, ENTER to select, ESC to quit MT7988> [B
Analysis shows it was caused by the wrong logic of bootmenu_loop:
At first the bootmenu_loop received the first ESC char correctly.
However, during the second call to bootmenu_loop, there's no data in the UART Rx FIFO. Due to the low baudrate, the second char of the down array key sequence hasn't be fully received.
But bootmenu_loop just did a mdelay(10), and then treated it as a single ESC key press event. It didn't even try tstc() again after the 10ms timeout.
This patch fixes this issue by letting bootmenu_loop check tstc() twice.
Signed-off-by: Weijie Gao weijie.gao@mediatek.com --- common/menu.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/common/menu.c b/common/menu.c index 5fd705039fc..bd2ac3e0ed9 100644 --- a/common/menu.c +++ b/common/menu.c @@ -566,7 +566,7 @@ enum bootmenu_key bootmenu_loop(struct bootmenu_data *menu, struct cli_ch_state *cch) { enum bootmenu_key key; - int c; + int c, errchar = 0; int choice;
c = cli_ch_process(cch, 0); @@ -574,7 +574,8 @@ enum bootmenu_key bootmenu_loop(struct bootmenu_data *menu, while (!c && !tstc()) { schedule(); mdelay(10); - c = cli_ch_process(cch, -ETIMEDOUT); + c = cli_ch_process(cch, errchar); + errchar = -ETIMEDOUT; } if (!c) { c = getchar();
Hi Weijie,
On Fri, 18 Oct 2024 at 02:38, Weijie Gao weijie.gao@mediatek.com wrote:
It's observed that the bootmenu on a serial console sometimes incorrectly quitted with superfluous characters filled to command line input:
*** U-Boot Boot Menu ***
1. Startup system (Default) 2. Upgrade firmware 3. Upgrade ATF BL2 4. Upgrade ATF FIP 5. Load image 0. U-Boot consolePress UP/DOWN to move, ENTER to select, ESC to quit MT7988> [B
Analysis shows it was caused by the wrong logic of bootmenu_loop:
At first the bootmenu_loop received the first ESC char correctly.
However, during the second call to bootmenu_loop, there's no data in the UART Rx FIFO. Due to the low baudrate, the second char of the down array key sequence hasn't be fully received.
But bootmenu_loop just did a mdelay(10), and then treated it as a single ESC key press event. It didn't even try tstc() again after the 10ms timeout.
This patch fixes this issue by letting bootmenu_loop check tstc() twice.
Signed-off-by: Weijie Gao weijie.gao@mediatek.com
common/menu.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/common/menu.c b/common/menu.c index 5fd705039fc..bd2ac3e0ed9 100644 --- a/common/menu.c +++ b/common/menu.c @@ -566,7 +566,7 @@ enum bootmenu_key bootmenu_loop(struct bootmenu_data *menu, struct cli_ch_state *cch) { enum bootmenu_key key;
int c;
int c, errchar = 0; int choice; c = cli_ch_process(cch, 0);@@ -574,7 +574,8 @@ enum bootmenu_key bootmenu_loop(struct bootmenu_data *menu, while (!c && !tstc()) { schedule(); mdelay(10);
c = cli_ch_process(cch, -ETIMEDOUT);
c = cli_ch_process(cch, errchar);errchar = -ETIMEDOUT; } if (!c) { c = getchar();-- 2.34.1
This looks reasonable from a first glance, but please can you rebase on -master ?
Regards, Simon
Hi,
On Fri, Oct 18, 2024 at 8:03 AM Simon Glass sjg@chromium.org wrote:
Hi Weijie,
On Fri, 18 Oct 2024 at 02:38, Weijie Gao weijie.gao@mediatek.com wrote:
It's observed that the bootmenu on a serial console sometimes incorrectly quitted with superfluous characters filled to command line input:
*** U-Boot Boot Menu ***
1. Startup system (Default) 2. Upgrade firmware 3. Upgrade ATF BL2 4. Upgrade ATF FIP 5. Load image 0. U-Boot consolePress UP/DOWN to move, ENTER to select, ESC to quit MT7988> [B
Analysis shows it was caused by the wrong logic of bootmenu_loop:
At first the bootmenu_loop received the first ESC char correctly.
However, during the second call to bootmenu_loop, there's no data in the UART Rx FIFO. Due to the low baudrate, the second char of the down array key sequence hasn't be fully received.
But bootmenu_loop just did a mdelay(10), and then treated it as a single ESC key press event. It didn't even try tstc() again after the 10ms timeout.
This patch fixes this issue by letting bootmenu_loop check tstc() twice.
Signed-off-by: Weijie Gao weijie.gao@mediatek.com
common/menu.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/common/menu.c b/common/menu.c index 5fd705039fc..bd2ac3e0ed9 100644 --- a/common/menu.c +++ b/common/menu.c @@ -566,7 +566,7 @@ enum bootmenu_key bootmenu_loop(struct bootmenu_data *menu, struct cli_ch_state *cch) { enum bootmenu_key key;
int c;
int c, errchar = 0; int choice; c = cli_ch_process(cch, 0);@@ -574,7 +574,8 @@ enum bootmenu_key bootmenu_loop(struct bootmenu_data *menu, while (!c && !tstc()) { schedule(); mdelay(10);
c = cli_ch_process(cch, -ETIMEDOUT);
c = cli_ch_process(cch, errchar);errchar = -ETIMEDOUT; } if (!c) { c = getchar();-- 2.34.1
This looks reasonable from a first glance, but please can you rebase on -master ?
Regards, Simon
Does this affect eficonfig menu ? I was having some intermittent trouble about the eficonfig menu quitting suddenly before this patch. With this fix applied it's no worse but maybe better, I tried for some minutes to reproduce the previous behaviour and was not able to make the glitch happen.
Tested-By: E Shattow lucent@gmail.com
On Mon, 2024-10-21 at 02:07 -0700, E Shattow wrote:
Hi,
On Fri, Oct 18, 2024 at 8:03 AM Simon Glass sjg@chromium.org wrote:
Hi Weijie,
On Fri, 18 Oct 2024 at 02:38, Weijie Gao weijie.gao@mediatek.com
wrote:
It's observed that the bootmenu on a serial console sometimes incorrectly quitted with superfluous characters filled to command line input:
*** U-Boot Boot Menu ***
1. Startup system (Default) 2. Upgrade firmware 3. Upgrade ATF BL2 4. Upgrade ATF FIP 5. Load image 0. U-Boot consolePress UP/DOWN to move, ENTER to select, ESC to quit MT7988> [B
Analysis shows it was caused by the wrong logic of bootmenu_loop:
At first the bootmenu_loop received the first ESC char correctly.
However, during the second call to bootmenu_loop, there's no data in the UART Rx FIFO. Due to the low baudrate, the second char of the down array key sequence hasn't be fully received.
But bootmenu_loop just did a mdelay(10), and then treated it as a single ESC key press event. It didn't even try tstc() again after the 10ms timeout.
This patch fixes this issue by letting bootmenu_loop check tstc() twice.
Signed-off-by: Weijie Gao weijie.gao@mediatek.com
common/menu.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/common/menu.c b/common/menu.c index 5fd705039fc..bd2ac3e0ed9 100644 --- a/common/menu.c +++ b/common/menu.c @@ -566,7 +566,7 @@ enum bootmenu_key bootmenu_loop(struct
bootmenu_data *menu,
struct cli_ch_state *cch){ enum bootmenu_key key;
int c;
int c, errchar = 0; int choice; c = cli_ch_process(cch, 0);@@ -574,7 +574,8 @@ enum bootmenu_key bootmenu_loop(struct
bootmenu_data *menu,
while (!c && !tstc()) { schedule(); mdelay(10);
c = cli_ch_process(cch, -ETIMEDOUT);
c = cli_ch_process(cch, errchar);errchar = -ETIMEDOUT; } if (!c) { c = getchar();-- 2.34.1
This looks reasonable from a first glance, but please can you
rebase
on -master ?
Regards, Simon
Does this affect eficonfig menu ? I was having some intermittent trouble about the eficonfig menu quitting suddenly before this patch. With this fix applied it's no worse but maybe better, I tried for some minutes to reproduce the previous behaviour and was not able to make the glitch happen.
I don't have efi environment to test. But I do see this function be called in eficonfig.c (eficonfig_choice_change_boot_order). So I believe this will affect eficonfig menu.
Tested-By: E Shattow lucent@gmail.com
participants (4)
-
E Shattow -
Simon Glass -
Weijie Gao -
Weijie Gao (高惟杰)