[PATCH u-boot 0/3] Revert broken Bootmenu commits

[PATCH u-boot 0/3] Revert broken Bootmenu commits

[Pali Rohár]

These 3 commits broke support for U-Boot Bootmenu on Nokia N900.
Issues were reported more than month ago, but nobody reacted to them.
So revert these broken commits for now, to fix U-Boot Bootmenu support.

With these revered commits, U-Boot Bootmenu from master branch is
working fine again on Nokia N900.

Pali Rohár (3):
  Revert "video: Enable VIDEO_ANSI by default only with EFI"
  Revert "menu: Factor out menu-keypress decoding"
  Revert "menu: Make use of CLI character processing"

 cmd/bootmenu.c        |   9 ++--
 cmd/eficonfig.c       |  12 ++---
 common/cli_getch.c    |  12 ++---
 common/menu.c         | 122 ++++++++++++++++++++++++++----------------
 drivers/video/Kconfig |   7 +--
 include/cli.h         |   4 +-
 include/menu.h        |  17 +-----
 7 files changed, 91 insertions(+), 92 deletions(-)

-- 
2.20.1

[PATCH u-boot 1/3] Revert "video: Enable VIDEO_ANSI by default only with EFI"

[Pali Rohár]

This reverts commit 72a0dd8bed010bef78028ae528763f9807758e6b.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 drivers/video/Kconfig | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index 1e2f4e6de4a5..52f7eac1e6a3 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -141,13 +141,10 @@ config VIDEO_BPP32
 
 config VIDEO_ANSI
 	bool "Support ANSI escape sequences in video console"
-	default y if EFI_LOADER
+	default y
 	help
 	  Enable ANSI escape sequence decoding for a more fully functional
-	  console. Functionality includes changing the text colour and moving
-	  the cursor. These date from the 1970s and are still widely used today
-	  to control a text terminal. U-Boot implements these by decoding the
-	  sequences and performing the appropriate operation.
+	  console.
 
 config VIDEO_MIPI_DSI
 	bool "Support MIPI DSI interface"
-- 
2.20.1

[PATCH u-boot 2/3] Revert "menu: Factor out menu-keypress decoding"

[Pali Rohár]

This reverts commit 9e7ac0b0be5cb663e539716554d66f8f0890ca83.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 common/menu.c  | 48 ++++++++++++++++++------------------------------
 include/menu.h | 10 ----------
 2 files changed, 18 insertions(+), 40 deletions(-)

diff --git a/common/menu.c b/common/menu.c
index 94514177e4e9..b6ec2e9c616c 100644
--- a/common/menu.c
+++ b/common/menu.c
@@ -483,11 +483,26 @@ enum bootmenu_key bootmenu_autoboot_loop(struct bootmenu_data *menu,
 	return key;
 }
 
-enum bootmenu_key bootmenu_conv_key(int ichar)
+enum bootmenu_key bootmenu_loop(struct bootmenu_data *menu,
+				struct cli_ch_state *cch)
 {
-	enum bootmenu_key key;
+	enum bootmenu_key key = BKEY_NONE;
+	int c;
+
+	c = cli_ch_process(cch, 0);
+	if (!c) {
+		while (!c && !tstc()) {
+			schedule();
+			mdelay(10);
+			c = cli_ch_process(cch, -ETIMEDOUT);
+		}
+		if (!c) {
+			c = getchar();
+			c = cli_ch_process(cch, c);
+		}
+	}
 
-	switch (ichar) {
+	switch (c) {
 	case '\n':
 		/* enter key was pressed */
 		key = BKEY_SELECT;
@@ -515,34 +530,7 @@ enum bootmenu_key bootmenu_conv_key(int ichar)
 	case ' ':
 		key = BKEY_SPACE;
 		break;
-	default:
-		key = BKEY_NONE;
-		break;
-	}
-
-	return key;
-}
-
-enum bootmenu_key bootmenu_loop(struct bootmenu_data *menu,
-				struct cli_ch_state *cch)
-{
-	enum bootmenu_key key;
-	int c;
-
-	c = cli_ch_process(cch, 0);
-	if (!c) {
-		while (!c && !tstc()) {
-			schedule();
-			mdelay(10);
-			c = cli_ch_process(cch, -ETIMEDOUT);
-		}
-		if (!c) {
-			c = getchar();
-			c = cli_ch_process(cch, c);
-		}
 	}
 
-	key = bootmenu_conv_key(c);
-
 	return key;
 }
diff --git a/include/menu.h b/include/menu.h
index 64ce89b7d263..5e54f033dfa4 100644
--- a/include/menu.h
+++ b/include/menu.h
@@ -54,8 +54,6 @@ enum bootmenu_key {
 	BKEY_MINUS,
 	BKEY_SPACE,
 	BKEY_SAVE,
-
-	BKEY_COUNT,
 };
 
 /**
@@ -104,12 +102,4 @@ enum bootmenu_key bootmenu_autoboot_loop(struct bootmenu_data *menu,
 enum bootmenu_key bootmenu_loop(struct bootmenu_data *menu,
 				struct cli_ch_state *cch);
 
-/**
- * bootmenu_conv_key() - Convert a U-Boot keypress into a menu key
- *
- * @ichar: Keypress to convert (ASCII, including control characters)
- * Returns: Menu key that corresponds to @ichar, or BKEY_NONE if none
- */
-enum bootmenu_key bootmenu_conv_key(int ichar);
-
 #endif /* __MENU_H__ */
-- 
2.20.1

[PATCH u-boot 3/3] Revert "menu: Make use of CLI character processing"

[Pali Rohár]

This reverts commit 32bab0eae51b55898d1e2804e6614d9143840581.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 cmd/bootmenu.c     |  9 ++---
 cmd/eficonfig.c    | 12 ++----
 common/cli_getch.c | 12 ++----
 common/menu.c      | 92 +++++++++++++++++++++++++++++++++-------------
 include/cli.h      |  4 +-
 include/menu.h     |  7 +---
 6 files changed, 80 insertions(+), 56 deletions(-)

diff --git a/cmd/bootmenu.c b/cmd/bootmenu.c
index 6baeedc69f91..c02dc6269283 100644
--- a/cmd/bootmenu.c
+++ b/cmd/bootmenu.c
@@ -4,7 +4,6 @@
  */
 
 #include <charset.h>
-#include <cli.h>
 #include <common.h>
 #include <command.h>
 #include <ansi.h>
@@ -85,21 +84,19 @@ static void bootmenu_print_entry(void *data)
 
 static char *bootmenu_choice_entry(void *data)
 {
-	struct cli_ch_state s_cch, *cch = &s_cch;
 	struct bootmenu_data *menu = data;
 	struct bootmenu_entry *iter;
 	enum bootmenu_key key = BKEY_NONE;
+	int esc = 0;
 	int i;
 
-	cli_ch_init(cch);
-
 	while (1) {
 		if (menu->delay >= 0) {
 			/* Autoboot was not stopped */
-			key = bootmenu_autoboot_loop(menu, cch);
+			key = bootmenu_autoboot_loop(menu, &esc);
 		} else {
 			/* Some key was pressed, so autoboot was stopped */
-			key = bootmenu_loop(menu, cch);
+			key = bootmenu_loop(menu, &esc);
 		}
 
 		switch (key) {
diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
index 720f52b48b8c..5c3ed76e78f5 100644
--- a/cmd/eficonfig.c
+++ b/cmd/eficonfig.c
@@ -6,7 +6,6 @@
  */
 
 #include <ansi.h>
-#include <cli.h>
 #include <common.h>
 #include <charset.h>
 #include <efi_loader.h>
@@ -230,16 +229,14 @@ void eficonfig_display_statusline(struct menu *m)
  */
 char *eficonfig_choice_entry(void *data)
 {
-	struct cli_ch_state s_cch, *cch = &s_cch;
+	int esc = 0;
 	struct list_head *pos, *n;
 	struct eficonfig_entry *entry;
 	enum bootmenu_key key = BKEY_NONE;
 	struct efimenu *efi_menu = data;
 
-	cli_ch_init(cch);
-
 	while (1) {
-		key = bootmenu_loop((struct bootmenu_data *)efi_menu, cch);
+		key = bootmenu_loop((struct bootmenu_data *)efi_menu, &esc);
 
 		switch (key) {
 		case BKEY_UP:
@@ -1929,15 +1926,14 @@ static void eficonfig_print_change_boot_order_entry(void *data)
  */
 char *eficonfig_choice_change_boot_order(void *data)
 {
-	struct cli_ch_state s_cch, *cch = &s_cch;
+	int esc = 0;
 	struct list_head *pos, *n;
 	struct efimenu *efi_menu = data;
 	enum bootmenu_key key = BKEY_NONE;
 	struct eficonfig_entry *entry, *tmp;
 
-	cli_ch_init(cch);
 	while (1) {
-		key = bootmenu_loop(NULL, cch);
+		key = bootmenu_loop(NULL, &esc);
 
 		switch (key) {
 		case BKEY_PLUS:
diff --git a/common/cli_getch.c b/common/cli_getch.c
index 61d4cb261b81..8080ff814eff 100644
--- a/common/cli_getch.c
+++ b/common/cli_getch.c
@@ -140,11 +140,10 @@ int cli_ch_process(struct cli_ch_state *cch, int ichar)
 	 * sequence
 	 */
 	if (!ichar) {
-		if (cch->emitting) {
+		if (cch->emit_upto) {
 			if (cch->emit_upto < cch->esc_len)
 				return cch->esc_save[cch->emit_upto++];
 			cch->emit_upto = 0;
-			cch->emitting = false;
 			cch->esc_len = 0;
 		}
 		return 0;
@@ -176,21 +175,18 @@ int cli_ch_process(struct cli_ch_state *cch, int ichar)
 		case ESC_SAVE:
 			/* save this character and return nothing */
 			cch->esc_save[cch->esc_len++] = ichar;
-			ichar = 0;
-			break;
+			return 0;
 		case ESC_REJECT:
 			/*
 			 * invalid escape sequence, start returning the
 			 * characters in it
 			 */
 			cch->esc_save[cch->esc_len++] = ichar;
-			ichar = cch->esc_save[cch->emit_upto++];
-			cch->emitting = true;
-			return ichar;
+			return cch->esc_save[cch->emit_upto++];
 		case ESC_CONVERTED:
 			/* valid escape sequence, return the resulting char */
 			cch->esc_len = 0;
-			break;
+			return ichar;
 		}
 	}
 
diff --git a/common/menu.c b/common/menu.c
index b6ec2e9c616c..5b614eafa413 100644
--- a/common/menu.c
+++ b/common/menu.c
@@ -15,8 +15,6 @@
 
 #include "menu.h"
 
-#define ansi 0
-
 /*
  * Internally, each item in a menu is represented by a struct menu_item.
  *
@@ -427,19 +425,15 @@ int menu_destroy(struct menu *m)
 	return 1;
 }
 
-enum bootmenu_key bootmenu_autoboot_loop(struct bootmenu_data *menu,
-					 struct cli_ch_state *cch)
+enum bootmenu_key bootmenu_autoboot_loop(struct bootmenu_data *menu, int *esc)
 {
 	enum bootmenu_key key = BKEY_NONE;
 	int i, c;
 
 	while (menu->delay > 0) {
-		if (ansi)
-			printf(ANSI_CURSOR_POSITION, menu->count + 5, 3);
+		printf(ANSI_CURSOR_POSITION, menu->count + 5, 3);
 		printf("Hit any key to stop autoboot: %d ", menu->delay);
 		for (i = 0; i < 100; ++i) {
-			int ichar;
-
 			if (!tstc()) {
 				schedule();
 				mdelay(10);
@@ -449,13 +443,12 @@ enum bootmenu_key bootmenu_autoboot_loop(struct bootmenu_data *menu,
 			menu->delay = -1;
 			c = getchar();
 
-			ichar = cli_ch_process(cch, c);
-
-			switch (ichar) {
-			case '\0':
+			switch (c) {
+			case '\e':
+				*esc = 1;
 				key = BKEY_NONE;
 				break;
-			case '\n':
+			case '\r':
 				key = BKEY_SELECT;
 				break;
 			case 0x3: /* ^C */
@@ -465,6 +458,7 @@ enum bootmenu_key bootmenu_autoboot_loop(struct bootmenu_data *menu,
 				key = BKEY_NONE;
 				break;
 			}
+
 			break;
 		}
 
@@ -474,8 +468,7 @@ enum bootmenu_key bootmenu_autoboot_loop(struct bootmenu_data *menu,
 		--menu->delay;
 	}
 
-	if (ansi)
-		printf(ANSI_CURSOR_POSITION ANSI_CLEAR_LINE, menu->count + 5, 1);
+	printf(ANSI_CURSOR_POSITION ANSI_CLEAR_LINE, menu->count + 5, 1);
 
 	if (menu->delay == 0)
 		key = BKEY_SELECT;
@@ -483,32 +476,79 @@ enum bootmenu_key bootmenu_autoboot_loop(struct bootmenu_data *menu,
 	return key;
 }
 
-enum bootmenu_key bootmenu_loop(struct bootmenu_data *menu,
-				struct cli_ch_state *cch)
+enum bootmenu_key bootmenu_loop(struct bootmenu_data *menu, int *esc)
 {
 	enum bootmenu_key key = BKEY_NONE;
 	int c;
 
-	c = cli_ch_process(cch, 0);
-	if (!c) {
-		while (!c && !tstc()) {
+	if (*esc == 1) {
+		if (tstc()) {
+			c = getchar();
+		} else {
 			schedule();
 			mdelay(10);
-			c = cli_ch_process(cch, -ETIMEDOUT);
+			if (tstc())
+				c = getchar();
+			else
+				c = '\e';
 		}
-		if (!c) {
-			c = getchar();
-			c = cli_ch_process(cch, c);
+	} else {
+		while (!tstc()) {
+			schedule();
+			mdelay(10);
 		}
+		c = getchar();
+	}
+
+	switch (*esc) {
+	case 0:
+		/* First char of ANSI escape sequence '\e' */
+		if (c == '\e') {
+			*esc = 1;
+			key = BKEY_NONE;
+		}
+		break;
+	case 1:
+		/* Second char of ANSI '[' */
+		if (c == '[') {
+			*esc = 2;
+			key = BKEY_NONE;
+		} else {
+		/* Alone ESC key was pressed */
+			key = BKEY_QUIT;
+			*esc = (c == '\e') ? 1 : 0;
+		}
+		break;
+	case 2:
+	case 3:
+		/* Third char of ANSI (number '1') - optional */
+		if (*esc == 2 && c == '1') {
+			*esc = 3;
+			key = BKEY_NONE;
+			break;
+		}
+
+		*esc = 0;
+
+		/* ANSI 'A' - key up was pressed */
+		if (c == 'A')
+			key = BKEY_UP;
+		/* ANSI 'B' - key down was pressed */
+		else if (c == 'B')
+			key = BKEY_DOWN;
+		/* other key was pressed */
+		else
+			key = BKEY_NONE;
+
+		break;
 	}
 
 	switch (c) {
-	case '\n':
+	case '\r':
 		/* enter key was pressed */
 		key = BKEY_SELECT;
 		break;
 	case CTL_CH('c'):
-	case '\e':
 		/* ^C was pressed */
 		key = BKEY_QUIT;
 		break;
diff --git a/include/cli.h b/include/cli.h
index 094a6602d70e..0c7995fb90a4 100644
--- a/include/cli.h
+++ b/include/cli.h
@@ -14,14 +14,12 @@
  *
  * @esc_len: Number of escape characters read so far
  * @esc_save: Escape characters collected so far
- * @emit_upto: Next index to emit from esc_save
- * @emitting: true if emitting from esc_save
+ * @emit_upto: Next character to emit from esc_save (0 if not emitting)
  */
 struct cli_ch_state {
 	int esc_len;
 	char esc_save[8];
 	int emit_upto;
-	bool emitting;
 };
 
 /**
diff --git a/include/menu.h b/include/menu.h
index 5e54f033dfa4..e4fae283f74f 100644
--- a/include/menu.h
+++ b/include/menu.h
@@ -6,7 +6,6 @@
 #ifndef __MENU_H__
 #define __MENU_H__
 
-struct cli_ch_state;
 struct menu;
 
 struct menu *menu_create(char *title, int timeout, int prompt,
@@ -73,8 +72,7 @@ enum bootmenu_key {
  *	Ctrl-C: KEY_QUIT
  *	anything else: KEY_NONE
  */
-enum bootmenu_key bootmenu_autoboot_loop(struct bootmenu_data *menu,
-					 struct cli_ch_state *cch);
+enum bootmenu_key bootmenu_autoboot_loop(struct bootmenu_data *menu, int *esc);
 
 /**
  * bootmenu_loop() - handle waiting for a keypress when autoboot is disabled
@@ -99,7 +97,6 @@ enum bootmenu_key bootmenu_autoboot_loop(struct bootmenu_data *menu,
  *	Minus: BKEY_MINUS
  *	Space: BKEY_SPACE
  */
-enum bootmenu_key bootmenu_loop(struct bootmenu_data *menu,
-				struct cli_ch_state *cch);
+enum bootmenu_key bootmenu_loop(struct bootmenu_data *menu, int *esc);
 
 #endif /* __MENU_H__ */
-- 
2.20.1

Re: [PATCH u-boot 0/3] Revert broken Bootmenu commits

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1471 bytes --]

On Sun, Jun 11, 2023 at 02:53:21PM +0200, Pali Rohár wrote:

> These 3 commits broke support for U-Boot Bootmenu on Nokia N900.
> Issues were reported more than month ago, but nobody reacted to them.
> So revert these broken commits for now, to fix U-Boot Bootmenu support.
> 
> With these revered commits, U-Boot Bootmenu from master branch is
> working fine again on Nokia N900.
> 
> Pali Rohár (3):
>   Revert "video: Enable VIDEO_ANSI by default only with EFI"
>   Revert "menu: Factor out menu-keypress decoding"
>   Revert "menu: Make use of CLI character processing"
> 
>  cmd/bootmenu.c        |   9 ++--
>  cmd/eficonfig.c       |  12 ++---
>  common/cli_getch.c    |  12 ++---
>  common/menu.c         | 122 ++++++++++++++++++++++++++----------------
>  drivers/video/Kconfig |   7 +--
>  include/cli.h         |   4 +-
>  include/menu.h        |  17 +-----
>  7 files changed, 91 insertions(+), 92 deletions(-)

Following up over here, while
https://patchwork.ozlabs.org/project/uboot/patch/20230612201434.861700-1-sjg@chromium.org/
addresses some of the issues, but not all (as it clearly isn't working
in all of the cases Pali has explained), looking in to the VIDEO_ANSI
part of this too, all three of these reverts are related seemingly to
something escape-character related.  I'm not taking any of the revert
commits in just yet, but will by the next -rc if we don't have something
that fixes all of the issues.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH u-boot 0/3] Revert broken Bootmenu commits

[Simon Glass]

Hi Tom, Pali,

On Wed, 14 Jun 2023 at 20:51, Tom Rini <trini@konsulko.com> wrote:
>
> On Sun, Jun 11, 2023 at 02:53:21PM +0200, Pali Rohár wrote:
>
> > These 3 commits broke support for U-Boot Bootmenu on Nokia N900.
> > Issues were reported more than month ago, but nobody reacted to them.
> > So revert these broken commits for now, to fix U-Boot Bootmenu support.
> >
> > With these revered commits, U-Boot Bootmenu from master branch is
> > working fine again on Nokia N900.
> >
> > Pali Rohár (3):
> >   Revert "video: Enable VIDEO_ANSI by default only with EFI"
> >   Revert "menu: Factor out menu-keypress decoding"
> >   Revert "menu: Make use of CLI character processing"
> >
> >  cmd/bootmenu.c        |   9 ++--
> >  cmd/eficonfig.c       |  12 ++---
> >  common/cli_getch.c    |  12 ++---
> >  common/menu.c         | 122 ++++++++++++++++++++++++++----------------
> >  drivers/video/Kconfig |   7 +--
> >  include/cli.h         |   4 +-
> >  include/menu.h        |  17 +-----
> >  7 files changed, 91 insertions(+), 92 deletions(-)
>
> Following up over here, while
> https://patchwork.ozlabs.org/project/uboot/patch/20230612201434.861700-1-sjg@chromium.org/
> addresses some of the issues, but not all (as it clearly isn't working
> in all of the cases Pali has explained), looking in to the VIDEO_ANSI
> part of this too, all three of these reverts are related seemingly to
> something escape-character related.  I'm not taking any of the revert
> commits in just yet, but will by the next -rc if we don't have something
> that fixes all of the issues.

I did send a series [1] with a fix for the nokia_rx51 keyboard driver,
including the previous ansi-code patch. Given that:

- this problem doesn't seem to manifest on other boards
- it does not cause any CI test to fail
- there seem to be bugs in the nokia_rx51 implementation which are a
possible/likely cause
- the nokia_rx51 CI uses a 10-year-old out-of-tree QEMU
- the problem goes away if debugging is added, suggesting it is
related to timing

...I don't think a revert is appropriate.

Pali, can you please take a look and see if you can debug what is
actually going on? Here is my guess:

1. When an arrow key is pressed, cli_ch_process() handles it by being
passed the codes in sequence: \e [ B
2. Calling cli_ch_process() with ichar = 0 causes it to think the
sequence is finished: \e [ \0 B   (this doesn't work since the \0 ends
the sequence)
3. nokia_rx51 keyboard driver is returning '\0' even when key codes
are pending, causing cli_ch_process() to be told that the sequence is
done

It would help to move the keyboard driver into drivers/input/ so it is
easier to find.

Regards,
Simon

[1] https://patchwork.ozlabs.org/project/uboot/list/?series=360134

Re: [PATCH u-boot 0/3] Revert broken Bootmenu commits

[Pali Rohár]

On Tuesday 20 June 2023 11:20:35 Simon Glass wrote:
> Hi Tom, Pali,
> 
> On Wed, 14 Jun 2023 at 20:51, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Sun, Jun 11, 2023 at 02:53:21PM +0200, Pali Rohár wrote:
> >
> > > These 3 commits broke support for U-Boot Bootmenu on Nokia N900.
> > > Issues were reported more than month ago, but nobody reacted to them.
> > > So revert these broken commits for now, to fix U-Boot Bootmenu support.
> > >
> > > With these revered commits, U-Boot Bootmenu from master branch is
> > > working fine again on Nokia N900.
> > >
> > > Pali Rohár (3):
> > >   Revert "video: Enable VIDEO_ANSI by default only with EFI"
> > >   Revert "menu: Factor out menu-keypress decoding"
> > >   Revert "menu: Make use of CLI character processing"
> > >
> > >  cmd/bootmenu.c        |   9 ++--
> > >  cmd/eficonfig.c       |  12 ++---
> > >  common/cli_getch.c    |  12 ++---
> > >  common/menu.c         | 122 ++++++++++++++++++++++++++----------------
> > >  drivers/video/Kconfig |   7 +--
> > >  include/cli.h         |   4 +-
> > >  include/menu.h        |  17 +-----
> > >  7 files changed, 91 insertions(+), 92 deletions(-)
> >
> > Following up over here, while
> > https://patchwork.ozlabs.org/project/uboot/patch/20230612201434.861700-1-sjg@chromium.org/
> > addresses some of the issues, but not all (as it clearly isn't working
> > in all of the cases Pali has explained), looking in to the VIDEO_ANSI
> > part of this too, all three of these reverts are related seemingly to
> > something escape-character related.  I'm not taking any of the revert
> > commits in just yet, but will by the next -rc if we don't have something
> > that fixes all of the issues.
> 
> I did send a series [1] with a fix for the nokia_rx51 keyboard driver,
> including the previous ansi-code patch. Given that:
> 
> - this problem doesn't seem to manifest on other boards
> - it does not cause any CI test to fail
> - there seem to be bugs in the nokia_rx51 implementation which are a
> possible/likely cause
> - the nokia_rx51 CI uses a 10-year-old out-of-tree QEMU
> - the problem goes away if debugging is added, suggesting it is
> related to timing
> 
> ...I don't think a revert is appropriate.

Unfortunately it does not fix this issue :-( New patch series from [1]
applied on top of the master branch has still issue with DOWN key on
emulated UART terminal.

> Pali, can you please take a look and see if you can debug what is
> actually going on? Here is my guess:
> 
> 1. When an arrow key is pressed, cli_ch_process() handles it by being
> passed the codes in sequence: \e [ B
> 2. Calling cli_ch_process() with ichar = 0 causes it to think the
> sequence is finished: \e [ \0 B   (this doesn't work since the \0 ends
> the sequence)
> 3. nokia_rx51 keyboard driver is returning '\0' even when key codes
> are pending, causing cli_ch_process() to be told that the sequence is
> done

I can look at it later, but I'm loosing motivation to do whole debugging
for another issue again, because for the last year my fixes and other
patches were stalled and u-boot devs show me that they are not
interested even in commenting them. I do not want to work again on
something which would be ignored.

Hence, now I did only smaller - but still important - task: Locate exact
commits which broke particular feature and prepare reverts on top of the
master branch which _really_ fix the broken functionality - and make
code working again.

> It would help to move the keyboard driver into drivers/input/ so it is
> easier to find.
> 
> Regards,
> Simon
> 
> [1] https://patchwork.ozlabs.org/project/uboot/list/?series=360134

Re: [PATCH u-boot 0/3] Revert broken Bootmenu commits

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 4079 bytes --]

On Sat, Jun 24, 2023 at 10:50:41AM +0200, Pali Rohár wrote:
> On Tuesday 20 June 2023 11:20:35 Simon Glass wrote:
> > Hi Tom, Pali,
> > 
> > On Wed, 14 Jun 2023 at 20:51, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Sun, Jun 11, 2023 at 02:53:21PM +0200, Pali Rohár wrote:
> > >
> > > > These 3 commits broke support for U-Boot Bootmenu on Nokia N900.
> > > > Issues were reported more than month ago, but nobody reacted to them.
> > > > So revert these broken commits for now, to fix U-Boot Bootmenu support.
> > > >
> > > > With these revered commits, U-Boot Bootmenu from master branch is
> > > > working fine again on Nokia N900.
> > > >
> > > > Pali Rohár (3):
> > > >   Revert "video: Enable VIDEO_ANSI by default only with EFI"
> > > >   Revert "menu: Factor out menu-keypress decoding"
> > > >   Revert "menu: Make use of CLI character processing"
> > > >
> > > >  cmd/bootmenu.c        |   9 ++--
> > > >  cmd/eficonfig.c       |  12 ++---
> > > >  common/cli_getch.c    |  12 ++---
> > > >  common/menu.c         | 122 ++++++++++++++++++++++++++----------------
> > > >  drivers/video/Kconfig |   7 +--
> > > >  include/cli.h         |   4 +-
> > > >  include/menu.h        |  17 +-----
> > > >  7 files changed, 91 insertions(+), 92 deletions(-)
> > >
> > > Following up over here, while
> > > https://patchwork.ozlabs.org/project/uboot/patch/20230612201434.861700-1-sjg@chromium.org/
> > > addresses some of the issues, but not all (as it clearly isn't working
> > > in all of the cases Pali has explained), looking in to the VIDEO_ANSI
> > > part of this too, all three of these reverts are related seemingly to
> > > something escape-character related.  I'm not taking any of the revert
> > > commits in just yet, but will by the next -rc if we don't have something
> > > that fixes all of the issues.
> > 
> > I did send a series [1] with a fix for the nokia_rx51 keyboard driver,
> > including the previous ansi-code patch. Given that:
> > 
> > - this problem doesn't seem to manifest on other boards
> > - it does not cause any CI test to fail
> > - there seem to be bugs in the nokia_rx51 implementation which are a
> > possible/likely cause
> > - the nokia_rx51 CI uses a 10-year-old out-of-tree QEMU
> > - the problem goes away if debugging is added, suggesting it is
> > related to timing
> > 
> > ...I don't think a revert is appropriate.
> 
> Unfortunately it does not fix this issue :-( New patch series from [1]
> applied on top of the master branch has still issue with DOWN key on
> emulated UART terminal.
> 
> > Pali, can you please take a look and see if you can debug what is
> > actually going on? Here is my guess:
> > 
> > 1. When an arrow key is pressed, cli_ch_process() handles it by being
> > passed the codes in sequence: \e [ B
> > 2. Calling cli_ch_process() with ichar = 0 causes it to think the
> > sequence is finished: \e [ \0 B   (this doesn't work since the \0 ends
> > the sequence)
> > 3. nokia_rx51 keyboard driver is returning '\0' even when key codes
> > are pending, causing cli_ch_process() to be told that the sequence is
> > done
> 
> I can look at it later, but I'm loosing motivation to do whole debugging
> for another issue again, because for the last year my fixes and other
> patches were stalled and u-boot devs show me that they are not
> interested even in commenting them. I do not want to work again on
> something which would be ignored.

Well, unless your changes here break something else, I don't think a fix
for your problem will be ignored.

> Hence, now I did only smaller - but still important - task: Locate exact
> commits which broke particular feature and prepare reverts on top of the
> master branch which _really_ fix the broken functionality - and make
> code working again.

Unfortunately you're also the only person able to replicate the problem,
and Simon has tried (and posted fixes to your platform for other issues,
and debugging patches to hopefully making finding the problem easier).

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH u-boot 0/3] Revert broken Bootmenu commits

[Pali Rohár]

On Saturday 24 June 2023 12:58:07 Tom Rini wrote:
> On Sat, Jun 24, 2023 at 10:50:41AM +0200, Pali Rohár wrote:
> > On Tuesday 20 June 2023 11:20:35 Simon Glass wrote:
> > > Hi Tom, Pali,
> > > 
> > > On Wed, 14 Jun 2023 at 20:51, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Sun, Jun 11, 2023 at 02:53:21PM +0200, Pali Rohár wrote:
> > > >
> > > > > These 3 commits broke support for U-Boot Bootmenu on Nokia N900.
> > > > > Issues were reported more than month ago, but nobody reacted to them.
> > > > > So revert these broken commits for now, to fix U-Boot Bootmenu support.
> > > > >
> > > > > With these revered commits, U-Boot Bootmenu from master branch is
> > > > > working fine again on Nokia N900.
> > > > >
> > > > > Pali Rohár (3):
> > > > >   Revert "video: Enable VIDEO_ANSI by default only with EFI"
> > > > >   Revert "menu: Factor out menu-keypress decoding"
> > > > >   Revert "menu: Make use of CLI character processing"
> > > > >
> > > > >  cmd/bootmenu.c        |   9 ++--
> > > > >  cmd/eficonfig.c       |  12 ++---
> > > > >  common/cli_getch.c    |  12 ++---
> > > > >  common/menu.c         | 122 ++++++++++++++++++++++++++----------------
> > > > >  drivers/video/Kconfig |   7 +--
> > > > >  include/cli.h         |   4 +-
> > > > >  include/menu.h        |  17 +-----
> > > > >  7 files changed, 91 insertions(+), 92 deletions(-)
> > > >
> > > > Following up over here, while
> > > > https://patchwork.ozlabs.org/project/uboot/patch/20230612201434.861700-1-sjg@chromium.org/
> > > > addresses some of the issues, but not all (as it clearly isn't working
> > > > in all of the cases Pali has explained), looking in to the VIDEO_ANSI
> > > > part of this too, all three of these reverts are related seemingly to
> > > > something escape-character related.  I'm not taking any of the revert
> > > > commits in just yet, but will by the next -rc if we don't have something
> > > > that fixes all of the issues.
> > > 
> > > I did send a series [1] with a fix for the nokia_rx51 keyboard driver,
> > > including the previous ansi-code patch. Given that:
> > > 
> > > - this problem doesn't seem to manifest on other boards
> > > - it does not cause any CI test to fail
> > > - there seem to be bugs in the nokia_rx51 implementation which are a
> > > possible/likely cause
> > > - the nokia_rx51 CI uses a 10-year-old out-of-tree QEMU
> > > - the problem goes away if debugging is added, suggesting it is
> > > related to timing
> > > 
> > > ...I don't think a revert is appropriate.
> > 
> > Unfortunately it does not fix this issue :-( New patch series from [1]
> > applied on top of the master branch has still issue with DOWN key on
> > emulated UART terminal.
> > 
> > > Pali, can you please take a look and see if you can debug what is
> > > actually going on? Here is my guess:
> > > 
> > > 1. When an arrow key is pressed, cli_ch_process() handles it by being
> > > passed the codes in sequence: \e [ B
> > > 2. Calling cli_ch_process() with ichar = 0 causes it to think the
> > > sequence is finished: \e [ \0 B   (this doesn't work since the \0 ends
> > > the sequence)
> > > 3. nokia_rx51 keyboard driver is returning '\0' even when key codes
> > > are pending, causing cli_ch_process() to be told that the sequence is
> > > done
> > 
> > I can look at it later, but I'm loosing motivation to do whole debugging
> > for another issue again, because for the last year my fixes and other
> > patches were stalled and u-boot devs show me that they are not
> > interested even in commenting them. I do not want to work again on
> > something which would be ignored.
> 
> Well, unless your changes here break something else, I don't think a fix
> for your problem will be ignored.

This is something which I read here more times in the past... and
reality was different.

Should I remind you that there are waiting eMMC fixes for mvebu and
again discussion about them stalled? Or rather should I say that it is
again ignored?

I have already spend time on this and have done everything needed to
make bootmenu working again. I have also prepared patches which are
fixing this problem and which were also tested.

And if you want something more from me then you show me why this time it
would be different, and again empty promises.

> > Hence, now I did only smaller - but still important - task: Locate exact
> > commits which broke particular feature and prepare reverts on top of the
> > master branch which _really_ fix the broken functionality - and make
> > code working again.
> 
> Unfortunately you're also the only person able to replicate the problem,
> and Simon has tried (and posted fixes to your platform for other issues,
> and debugging patches to hopefully making finding the problem easier).

Have somebody else even tried to reproduce this issue on any other HW
board or any other qemu board?

Re: [PATCH u-boot 0/3] Revert broken Bootmenu commits

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 5982 bytes --]

On Sun, Jun 25, 2023 at 09:50:39AM +0200, Pali Rohár wrote:
> On Saturday 24 June 2023 12:58:07 Tom Rini wrote:
> > On Sat, Jun 24, 2023 at 10:50:41AM +0200, Pali Rohár wrote:
> > > On Tuesday 20 June 2023 11:20:35 Simon Glass wrote:
> > > > Hi Tom, Pali,
> > > > 
> > > > On Wed, 14 Jun 2023 at 20:51, Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > On Sun, Jun 11, 2023 at 02:53:21PM +0200, Pali Rohár wrote:
> > > > >
> > > > > > These 3 commits broke support for U-Boot Bootmenu on Nokia N900.
> > > > > > Issues were reported more than month ago, but nobody reacted to them.
> > > > > > So revert these broken commits for now, to fix U-Boot Bootmenu support.
> > > > > >
> > > > > > With these revered commits, U-Boot Bootmenu from master branch is
> > > > > > working fine again on Nokia N900.
> > > > > >
> > > > > > Pali Rohár (3):
> > > > > >   Revert "video: Enable VIDEO_ANSI by default only with EFI"
> > > > > >   Revert "menu: Factor out menu-keypress decoding"
> > > > > >   Revert "menu: Make use of CLI character processing"
> > > > > >
> > > > > >  cmd/bootmenu.c        |   9 ++--
> > > > > >  cmd/eficonfig.c       |  12 ++---
> > > > > >  common/cli_getch.c    |  12 ++---
> > > > > >  common/menu.c         | 122 ++++++++++++++++++++++++++----------------
> > > > > >  drivers/video/Kconfig |   7 +--
> > > > > >  include/cli.h         |   4 +-
> > > > > >  include/menu.h        |  17 +-----
> > > > > >  7 files changed, 91 insertions(+), 92 deletions(-)
> > > > >
> > > > > Following up over here, while
> > > > > https://patchwork.ozlabs.org/project/uboot/patch/20230612201434.861700-1-sjg@chromium.org/
> > > > > addresses some of the issues, but not all (as it clearly isn't working
> > > > > in all of the cases Pali has explained), looking in to the VIDEO_ANSI
> > > > > part of this too, all three of these reverts are related seemingly to
> > > > > something escape-character related.  I'm not taking any of the revert
> > > > > commits in just yet, but will by the next -rc if we don't have something
> > > > > that fixes all of the issues.
> > > > 
> > > > I did send a series [1] with a fix for the nokia_rx51 keyboard driver,
> > > > including the previous ansi-code patch. Given that:
> > > > 
> > > > - this problem doesn't seem to manifest on other boards
> > > > - it does not cause any CI test to fail
> > > > - there seem to be bugs in the nokia_rx51 implementation which are a
> > > > possible/likely cause
> > > > - the nokia_rx51 CI uses a 10-year-old out-of-tree QEMU
> > > > - the problem goes away if debugging is added, suggesting it is
> > > > related to timing
> > > > 
> > > > ...I don't think a revert is appropriate.
> > > 
> > > Unfortunately it does not fix this issue :-( New patch series from [1]
> > > applied on top of the master branch has still issue with DOWN key on
> > > emulated UART terminal.
> > > 
> > > > Pali, can you please take a look and see if you can debug what is
> > > > actually going on? Here is my guess:
> > > > 
> > > > 1. When an arrow key is pressed, cli_ch_process() handles it by being
> > > > passed the codes in sequence: \e [ B
> > > > 2. Calling cli_ch_process() with ichar = 0 causes it to think the
> > > > sequence is finished: \e [ \0 B   (this doesn't work since the \0 ends
> > > > the sequence)
> > > > 3. nokia_rx51 keyboard driver is returning '\0' even when key codes
> > > > are pending, causing cli_ch_process() to be told that the sequence is
> > > > done
> > > 
> > > I can look at it later, but I'm loosing motivation to do whole debugging
> > > for another issue again, because for the last year my fixes and other
> > > patches were stalled and u-boot devs show me that they are not
> > > interested even in commenting them. I do not want to work again on
> > > something which would be ignored.
> > 
> > Well, unless your changes here break something else, I don't think a fix
> > for your problem will be ignored.
> 
> This is something which I read here more times in the past... and
> reality was different.
> 
> Should I remind you that there are waiting eMMC fixes for mvebu and
> again discussion about them stalled? Or rather should I say that it is
> again ignored?

No, you should just say they're still waiting for review.  Since they're
waiting for review. The MMC custodian has been asked to review them, and
hasn't yet. And they don't appear to be super critical changes, and they
conflict with other series, so yes, they'll get picked up when the
custodian has time to review everything.

> I have already spend time on this and have done everything needed to
> make bootmenu working again. I have also prepared patches which are
> fixing this problem and which were also tested.

Note that "I reverted the commit" is not a fix.

> And if you want something more from me then you show me why this time it
> would be different, and again empty promises.

What I'd like is for you to not assume worst-faith. If you can fix the
problem quickly at this point it'll get reviewed (assuming Simon has
time, next week is EOSS) and merged if it's a safe enough looking fix.
Otherwise it'll get in v2023.10.

> > > Hence, now I did only smaller - but still important - task: Locate exact
> > > commits which broke particular feature and prepare reverts on top of the
> > > master branch which _really_ fix the broken functionality - and make
> > > code working again.
> > 
> > Unfortunately you're also the only person able to replicate the problem,
> > and Simon has tried (and posted fixes to your platform for other issues,
> > and debugging patches to hopefully making finding the problem easier).
> 
> Have somebody else even tried to reproduce this issue on any other HW
> board or any other qemu board?

The summary of Simon's thread where he tried to fix this problem is that
only your platform still has a problem here.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH u-boot 0/3] Revert broken Bootmenu commits

[Pali Rohár]

On Sunday 25 June 2023 10:52:13 Tom Rini wrote:
> On Sun, Jun 25, 2023 at 09:50:39AM +0200, Pali Rohár wrote:
> > On Saturday 24 June 2023 12:58:07 Tom Rini wrote:
> > > On Sat, Jun 24, 2023 at 10:50:41AM +0200, Pali Rohár wrote:
> > > > On Tuesday 20 June 2023 11:20:35 Simon Glass wrote:
> > > > > Hi Tom, Pali,
> > > > > 
> > > > > On Wed, 14 Jun 2023 at 20:51, Tom Rini <trini@konsulko.com> wrote:
> > > > > >
> > > > > > On Sun, Jun 11, 2023 at 02:53:21PM +0200, Pali Rohár wrote:
> > > > > >
> > > > > > > These 3 commits broke support for U-Boot Bootmenu on Nokia N900.
> > > > > > > Issues were reported more than month ago, but nobody reacted to them.
> > > > > > > So revert these broken commits for now, to fix U-Boot Bootmenu support.
> > > > > > >
> > > > > > > With these revered commits, U-Boot Bootmenu from master branch is
> > > > > > > working fine again on Nokia N900.
> > > > > > >
> > > > > > > Pali Rohár (3):
> > > > > > >   Revert "video: Enable VIDEO_ANSI by default only with EFI"
> > > > > > >   Revert "menu: Factor out menu-keypress decoding"
> > > > > > >   Revert "menu: Make use of CLI character processing"
> > > > > > >
> > > > > > >  cmd/bootmenu.c        |   9 ++--
> > > > > > >  cmd/eficonfig.c       |  12 ++---
> > > > > > >  common/cli_getch.c    |  12 ++---
> > > > > > >  common/menu.c         | 122 ++++++++++++++++++++++++++----------------
> > > > > > >  drivers/video/Kconfig |   7 +--
> > > > > > >  include/cli.h         |   4 +-
> > > > > > >  include/menu.h        |  17 +-----
> > > > > > >  7 files changed, 91 insertions(+), 92 deletions(-)
> > > > > >
> > > > > > Following up over here, while
> > > > > > https://patchwork.ozlabs.org/project/uboot/patch/20230612201434.861700-1-sjg@chromium.org/
> > > > > > addresses some of the issues, but not all (as it clearly isn't working
> > > > > > in all of the cases Pali has explained), looking in to the VIDEO_ANSI
> > > > > > part of this too, all three of these reverts are related seemingly to
> > > > > > something escape-character related.  I'm not taking any of the revert
> > > > > > commits in just yet, but will by the next -rc if we don't have something
> > > > > > that fixes all of the issues.
> > > > > 
> > > > > I did send a series [1] with a fix for the nokia_rx51 keyboard driver,
> > > > > including the previous ansi-code patch. Given that:
> > > > > 
> > > > > - this problem doesn't seem to manifest on other boards
> > > > > - it does not cause any CI test to fail
> > > > > - there seem to be bugs in the nokia_rx51 implementation which are a
> > > > > possible/likely cause
> > > > > - the nokia_rx51 CI uses a 10-year-old out-of-tree QEMU
> > > > > - the problem goes away if debugging is added, suggesting it is
> > > > > related to timing
> > > > > 
> > > > > ...I don't think a revert is appropriate.
> > > > 
> > > > Unfortunately it does not fix this issue :-( New patch series from [1]
> > > > applied on top of the master branch has still issue with DOWN key on
> > > > emulated UART terminal.
> > > > 
> > > > > Pali, can you please take a look and see if you can debug what is
> > > > > actually going on? Here is my guess:
> > > > > 
> > > > > 1. When an arrow key is pressed, cli_ch_process() handles it by being
> > > > > passed the codes in sequence: \e [ B
> > > > > 2. Calling cli_ch_process() with ichar = 0 causes it to think the
> > > > > sequence is finished: \e [ \0 B   (this doesn't work since the \0 ends
> > > > > the sequence)
> > > > > 3. nokia_rx51 keyboard driver is returning '\0' even when key codes
> > > > > are pending, causing cli_ch_process() to be told that the sequence is
> > > > > done
> > > > 
> > > > I can look at it later, but I'm loosing motivation to do whole debugging
> > > > for another issue again, because for the last year my fixes and other
> > > > patches were stalled and u-boot devs show me that they are not
> > > > interested even in commenting them. I do not want to work again on
> > > > something which would be ignored.
> > > 
> > > Well, unless your changes here break something else, I don't think a fix
> > > for your problem will be ignored.
> > 
> > This is something which I read here more times in the past... and
> > reality was different.
> > 
> > Should I remind you that there are waiting eMMC fixes for mvebu and
> > again discussion about them stalled? Or rather should I say that it is
> > again ignored?
> 
> No, you should just say they're still waiting for review.  Since they're
> waiting for review. The MMC custodian has been asked to review them, and
> hasn't yet. And they don't appear to be super critical changes, and they
> conflict with other series, so yes, they'll get picked up when the
> custodian has time to review everything.

And what is "critical change" if it is not fixing booting (from eMMC)?

> > I have already spend time on this and have done everything needed to
> > make bootmenu working again. I have also prepared patches which are
> > fixing this problem and which were also tested.
> 
> Note that "I reverted the commit" is not a fix.

And what is the "change which makes feature X working again after it was
broken" if not a fix? Of course, it is _not_ fix because it is fixing
the problem.

> > And if you want something more from me then you show me why this time it
> > would be different, and again empty promises.
> 
> What I'd like is for you to not assume worst-faith.

Of course not. I'm just assuming that same thing happen as in the past.
From realistic point of view, this is the best approximation what to
assume.

Also you just few lines above wrote that fixing broken booting is not a
critical change at all, so you are just expressed that you are not going
to take fixes.

> If you can fix the
> problem quickly at this point it'll get reviewed (assuming Simon has
> time, next week is EOSS) and merged if it's a safe enough looking fix.
> Otherwise it'll get in v2023.10.

And here you wrote that "change maybe land in v2023.10 version". But at
that time u-boot would move and again it would not land because change
from this time wound not applied on top of the master branch which would
be in the future.

I'm not newbie here, I see what is happening for more years with my
patches.

Or should I remind you what is the average time how long rx51 patches
have been waiting on the list until they were merged?

I have already provided a fix for this problem. What happened? Nothing,
I was just ask to do another fix. Do you think, that I'm a total idiot,
who is going to prepare a new patch, which would be ignored or rejected
like the previous one, or other other work which I done?

> > > > Hence, now I did only smaller - but still important - task: Locate exact
> > > > commits which broke particular feature and prepare reverts on top of the
> > > > master branch which _really_ fix the broken functionality - and make
> > > > code working again.
> > > 
> > > Unfortunately you're also the only person able to replicate the problem,
> > > and Simon has tried (and posted fixes to your platform for other issues,
> > > and debugging patches to hopefully making finding the problem easier).
> > 
> > Have somebody else even tried to reproduce this issue on any other HW
> > board or any other qemu board?
> 
> The summary of Simon's thread where he tried to fix this problem is that
> only your platform still has a problem here.

This is not answer to the question.

Re: [PATCH u-boot 0/3] Revert broken Bootmenu commits

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 10751 bytes --]

On Sun, Jun 25, 2023 at 05:15:34PM +0200, Pali Rohár wrote:
> On Sunday 25 June 2023 10:52:13 Tom Rini wrote:
> > On Sun, Jun 25, 2023 at 09:50:39AM +0200, Pali Rohár wrote:
> > > On Saturday 24 June 2023 12:58:07 Tom Rini wrote:
> > > > On Sat, Jun 24, 2023 at 10:50:41AM +0200, Pali Rohár wrote:
> > > > > On Tuesday 20 June 2023 11:20:35 Simon Glass wrote:
> > > > > > Hi Tom, Pali,
> > > > > > 
> > > > > > On Wed, 14 Jun 2023 at 20:51, Tom Rini <trini@konsulko.com> wrote:
> > > > > > >
> > > > > > > On Sun, Jun 11, 2023 at 02:53:21PM +0200, Pali Rohár wrote:
> > > > > > >
> > > > > > > > These 3 commits broke support for U-Boot Bootmenu on Nokia N900.
> > > > > > > > Issues were reported more than month ago, but nobody reacted to them.
> > > > > > > > So revert these broken commits for now, to fix U-Boot Bootmenu support.
> > > > > > > >
> > > > > > > > With these revered commits, U-Boot Bootmenu from master branch is
> > > > > > > > working fine again on Nokia N900.
> > > > > > > >
> > > > > > > > Pali Rohár (3):
> > > > > > > >   Revert "video: Enable VIDEO_ANSI by default only with EFI"
> > > > > > > >   Revert "menu: Factor out menu-keypress decoding"
> > > > > > > >   Revert "menu: Make use of CLI character processing"
> > > > > > > >
> > > > > > > >  cmd/bootmenu.c        |   9 ++--
> > > > > > > >  cmd/eficonfig.c       |  12 ++---
> > > > > > > >  common/cli_getch.c    |  12 ++---
> > > > > > > >  common/menu.c         | 122 ++++++++++++++++++++++++++----------------
> > > > > > > >  drivers/video/Kconfig |   7 +--
> > > > > > > >  include/cli.h         |   4 +-
> > > > > > > >  include/menu.h        |  17 +-----
> > > > > > > >  7 files changed, 91 insertions(+), 92 deletions(-)
> > > > > > >
> > > > > > > Following up over here, while
> > > > > > > https://patchwork.ozlabs.org/project/uboot/patch/20230612201434.861700-1-sjg@chromium.org/
> > > > > > > addresses some of the issues, but not all (as it clearly isn't working
> > > > > > > in all of the cases Pali has explained), looking in to the VIDEO_ANSI
> > > > > > > part of this too, all three of these reverts are related seemingly to
> > > > > > > something escape-character related.  I'm not taking any of the revert
> > > > > > > commits in just yet, but will by the next -rc if we don't have something
> > > > > > > that fixes all of the issues.
> > > > > > 
> > > > > > I did send a series [1] with a fix for the nokia_rx51 keyboard driver,
> > > > > > including the previous ansi-code patch. Given that:
> > > > > > 
> > > > > > - this problem doesn't seem to manifest on other boards
> > > > > > - it does not cause any CI test to fail
> > > > > > - there seem to be bugs in the nokia_rx51 implementation which are a
> > > > > > possible/likely cause
> > > > > > - the nokia_rx51 CI uses a 10-year-old out-of-tree QEMU
> > > > > > - the problem goes away if debugging is added, suggesting it is
> > > > > > related to timing
> > > > > > 
> > > > > > ...I don't think a revert is appropriate.
> > > > > 
> > > > > Unfortunately it does not fix this issue :-( New patch series from [1]
> > > > > applied on top of the master branch has still issue with DOWN key on
> > > > > emulated UART terminal.
> > > > > 
> > > > > > Pali, can you please take a look and see if you can debug what is
> > > > > > actually going on? Here is my guess:
> > > > > > 
> > > > > > 1. When an arrow key is pressed, cli_ch_process() handles it by being
> > > > > > passed the codes in sequence: \e [ B
> > > > > > 2. Calling cli_ch_process() with ichar = 0 causes it to think the
> > > > > > sequence is finished: \e [ \0 B   (this doesn't work since the \0 ends
> > > > > > the sequence)
> > > > > > 3. nokia_rx51 keyboard driver is returning '\0' even when key codes
> > > > > > are pending, causing cli_ch_process() to be told that the sequence is
> > > > > > done
> > > > > 
> > > > > I can look at it later, but I'm loosing motivation to do whole debugging
> > > > > for another issue again, because for the last year my fixes and other
> > > > > patches were stalled and u-boot devs show me that they are not
> > > > > interested even in commenting them. I do not want to work again on
> > > > > something which would be ignored.
> > > > 
> > > > Well, unless your changes here break something else, I don't think a fix
> > > > for your problem will be ignored.
> > > 
> > > This is something which I read here more times in the past... and
> > > reality was different.
> > > 
> > > Should I remind you that there are waiting eMMC fixes for mvebu and
> > > again discussion about them stalled? Or rather should I say that it is
> > > again ignored?
> > 
> > No, you should just say they're still waiting for review.  Since they're
> > waiting for review. The MMC custodian has been asked to review them, and
> > hasn't yet. And they don't appear to be super critical changes, and they
> > conflict with other series, so yes, they'll get picked up when the
> > custodian has time to review everything.
> 
> And what is "critical change" if it is not fixing booting (from eMMC)?

The when and where, and since when. Since re-reading everything in
https://patchwork.ozlabs.org/project/uboot/list/?submitter=78810 that's
not this revert series doesn't say "fix booting on $platform that was
broken by ...".  It says clean up.  Clean up can wait.

> > > I have already spend time on this and have done everything needed to
> > > make bootmenu working again. I have also prepared patches which are
> > > fixing this problem and which were also tested.
> > 
> > Note that "I reverted the commit" is not a fix.
> 
> And what is the "change which makes feature X working again after it was
> broken" if not a fix? Of course, it is _not_ fix because it is fixing
> the problem.

The part where you're reverting changes that were made intentionally
rather than fixing whatever problem they introduce or expose. There is a
problem in here, and Simon's patch that I have since merged fixes that.
There's seemingly something else further going on with N900 and only
N900 and you're saying the fix is to revert changes rather than explain
what's wrong in any of the changes, still. The first change 'Revert
"video: Enable VIDEO_ANSI by default only with EFI"' as a fix would be
to add whatever conditional should also cause this to be "default y".
And I tried doing that, but after reading the code, it doesn't make any
sense why a few functions change on N900 when I do so. It's also the
only platform where there's a change.

> > > And if you want something more from me then you show me why this time it
> > > would be different, and again empty promises.
> > 
> > What I'd like is for you to not assume worst-faith.
> 
> Of course not. I'm just assuming that same thing happen as in the past.
> From realistic point of view, this is the best approximation what to
> assume.
> 
> Also you just few lines above wrote that fixing broken booting is not a
> critical change at all, so you are just expressed that you are not going
> to take fixes.

The only time your patches aren't accepted in a timely manner are when
either (a) they get delayed because of custodians who don't have a lot
of time to review an area or (b) you refuse to take feedback on a
change. The feedback right now on this 3 part revert series is to take
what Simon posted after trying things in QEMU and use that additional
debug information to figure it out or post more detailed failure logs.

> > If you can fix the
> > problem quickly at this point it'll get reviewed (assuming Simon has
> > time, next week is EOSS) and merged if it's a safe enough looking fix.
> > Otherwise it'll get in v2023.10.
> 
> And here you wrote that "change maybe land in v2023.10 version". But at
> that time u-boot would move and again it would not land because change
> from this time wound not applied on top of the master branch which would
> be in the future.

Yes, if the change to fix exactly one platform is 10 lines, I'm going to
be iffy about taking it a week before release.  If the change is 1 or 2
obviously correct lines, I'll likely take the gamble. But since at this
point there's exactly one known broken platform I'm not going to delay
the release. There were more broken platforms when I said I'd take the 3
part revert but Simon came along and fixed the problems everyone else
reported and made a good faith attempt and fixing your platform too.

> I'm not newbie here, I see what is happening for more years with my
> patches.
> 
> Or should I remind you what is the average time how long rx51 patches
> have been waiting on the list until they were merged?

I mean, what's the average over the last year or two? Yes, they got
delayed when I delegated them to the TI custodian who then had no real
time to deal with U-Boot.

> I have already provided a fix for this problem. What happened? Nothing,
> I was just ask to do another fix. Do you think, that I'm a total idiot,
> who is going to prepare a new patch, which would be ignored or rejected
> like the previous one, or other other work which I done?

Narrowing down a problem to a few commits and reverting them is a debug
aide, unless the commits themselves are simply wholly broken and wrong.
So no, you didn't fix the issue.  You narrowed it down.  I was willing
to take that even if Simon was unable to figure out what was wrong as it
was on multiple platforms, even.  So now you've been asked to dig more
on your platform as it doesn't fail for anyone else anywhere else.

> > > > > Hence, now I did only smaller - but still important - task: Locate exact
> > > > > commits which broke particular feature and prepare reverts on top of the
> > > > > master branch which _really_ fix the broken functionality - and make
> > > > > code working again.
> > > > 
> > > > Unfortunately you're also the only person able to replicate the problem,
> > > > and Simon has tried (and posted fixes to your platform for other issues,
> > > > and debugging patches to hopefully making finding the problem easier).
> > > 
> > > Have somebody else even tried to reproduce this issue on any other HW
> > > board or any other qemu board?
> > 
> > The summary of Simon's thread where he tried to fix this problem is that
> > only your platform still has a problem here.
> 
> This is not answer to the question.

Simon.  Simon tested this on N900 in QEMU, found some problems, reported
them, was unable to find further problems and pushed it back on you to
review the patches for your platform to fix the problem he did see and
was able to fix.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH u-boot 0/3] Revert broken Bootmenu commits

[Pali Rohár]

On Sunday 25 June 2023 21:08:19 Tom Rini wrote:
> On Sun, Jun 25, 2023 at 05:15:34PM +0200, Pali Rohár wrote:
> > On Sunday 25 June 2023 10:52:13 Tom Rini wrote:
> > > On Sun, Jun 25, 2023 at 09:50:39AM +0200, Pali Rohár wrote:
> > > > On Saturday 24 June 2023 12:58:07 Tom Rini wrote:
> > > > > On Sat, Jun 24, 2023 at 10:50:41AM +0200, Pali Rohár wrote:
> > > > > > On Tuesday 20 June 2023 11:20:35 Simon Glass wrote:
> > > > > > > Hi Tom, Pali,
> > > > > > > 
> > > > > > > On Wed, 14 Jun 2023 at 20:51, Tom Rini <trini@konsulko.com> wrote:
> > > > > > > >
> > > > > > > > On Sun, Jun 11, 2023 at 02:53:21PM +0200, Pali Rohár wrote:
> > > > > > > >
> > > > > > > > > These 3 commits broke support for U-Boot Bootmenu on Nokia N900.
> > > > > > > > > Issues were reported more than month ago, but nobody reacted to them.
> > > > > > > > > So revert these broken commits for now, to fix U-Boot Bootmenu support.
> > > > > > > > >
> > > > > > > > > With these revered commits, U-Boot Bootmenu from master branch is
> > > > > > > > > working fine again on Nokia N900.
> > > > > > > > >
> > > > > > > > > Pali Rohár (3):
> > > > > > > > >   Revert "video: Enable VIDEO_ANSI by default only with EFI"
> > > > > > > > >   Revert "menu: Factor out menu-keypress decoding"
> > > > > > > > >   Revert "menu: Make use of CLI character processing"
> > > > > > > > >
> > > > > > > > >  cmd/bootmenu.c        |   9 ++--
> > > > > > > > >  cmd/eficonfig.c       |  12 ++---
> > > > > > > > >  common/cli_getch.c    |  12 ++---
> > > > > > > > >  common/menu.c         | 122 ++++++++++++++++++++++++++----------------
> > > > > > > > >  drivers/video/Kconfig |   7 +--
> > > > > > > > >  include/cli.h         |   4 +-
> > > > > > > > >  include/menu.h        |  17 +-----
> > > > > > > > >  7 files changed, 91 insertions(+), 92 deletions(-)
> > > > > > > >
> > > > > > > > Following up over here, while
> > > > > > > > https://patchwork.ozlabs.org/project/uboot/patch/20230612201434.861700-1-sjg@chromium.org/
> > > > > > > > addresses some of the issues, but not all (as it clearly isn't working
> > > > > > > > in all of the cases Pali has explained), looking in to the VIDEO_ANSI
> > > > > > > > part of this too, all three of these reverts are related seemingly to
> > > > > > > > something escape-character related.  I'm not taking any of the revert
> > > > > > > > commits in just yet, but will by the next -rc if we don't have something
> > > > > > > > that fixes all of the issues.
> > > > > > > 
> > > > > > > I did send a series [1] with a fix for the nokia_rx51 keyboard driver,
> > > > > > > including the previous ansi-code patch. Given that:
> > > > > > > 
> > > > > > > - this problem doesn't seem to manifest on other boards
> > > > > > > - it does not cause any CI test to fail
> > > > > > > - there seem to be bugs in the nokia_rx51 implementation which are a
> > > > > > > possible/likely cause
> > > > > > > - the nokia_rx51 CI uses a 10-year-old out-of-tree QEMU
> > > > > > > - the problem goes away if debugging is added, suggesting it is
> > > > > > > related to timing
> > > > > > > 
> > > > > > > ...I don't think a revert is appropriate.
> > > > > > 
> > > > > > Unfortunately it does not fix this issue :-( New patch series from [1]
> > > > > > applied on top of the master branch has still issue with DOWN key on
> > > > > > emulated UART terminal.
> > > > > > 
> > > > > > > Pali, can you please take a look and see if you can debug what is
> > > > > > > actually going on? Here is my guess:
> > > > > > > 
> > > > > > > 1. When an arrow key is pressed, cli_ch_process() handles it by being
> > > > > > > passed the codes in sequence: \e [ B
> > > > > > > 2. Calling cli_ch_process() with ichar = 0 causes it to think the
> > > > > > > sequence is finished: \e [ \0 B   (this doesn't work since the \0 ends
> > > > > > > the sequence)
> > > > > > > 3. nokia_rx51 keyboard driver is returning '\0' even when key codes
> > > > > > > are pending, causing cli_ch_process() to be told that the sequence is
> > > > > > > done
> > > > > > 
> > > > > > I can look at it later, but I'm loosing motivation to do whole debugging
> > > > > > for another issue again, because for the last year my fixes and other
> > > > > > patches were stalled and u-boot devs show me that they are not
> > > > > > interested even in commenting them. I do not want to work again on
> > > > > > something which would be ignored.
> > > > > 
> > > > > Well, unless your changes here break something else, I don't think a fix
> > > > > for your problem will be ignored.
> > > > 
> > > > This is something which I read here more times in the past... and
> > > > reality was different.
> > > > 
> > > > Should I remind you that there are waiting eMMC fixes for mvebu and
> > > > again discussion about them stalled? Or rather should I say that it is
> > > > again ignored?
> > > 
> > > No, you should just say they're still waiting for review.  Since they're
> > > waiting for review. The MMC custodian has been asked to review them, and
> > > hasn't yet. And they don't appear to be super critical changes, and they
> > > conflict with other series, so yes, they'll get picked up when the
> > > custodian has time to review everything.
> > 
> > And what is "critical change" if it is not fixing booting (from eMMC)?
> 
> The when and where, and since when. Since re-reading everything in
> https://patchwork.ozlabs.org/project/uboot/list/?submitter=78810 that's
> not this revert series doesn't say "fix booting on $platform that was
> broken by ...".  It says clean up.  Clean up can wait.

"Fix eMMC boot" - This is not clear that it is a "fix"???

Or are you again going to play a game "I do not see your patches"?
Ok, then here is direct link:

https://lore.kernel.org/u-boot/20230413205750.10641-1-pali@kernel.org/

> > > > I have already spend time on this and have done everything needed to
> > > > make bootmenu working again. I have also prepared patches which are
> > > > fixing this problem and which were also tested.
> > > 
> > > Note that "I reverted the commit" is not a fix.
> > 
> > And what is the "change which makes feature X working again after it was
> > broken" if not a fix? Of course, it is _not_ fix because it is fixing
> > the problem.
> 
> The part where you're reverting changes that were made intentionally
> rather than fixing whatever problem they introduce or expose. There is a
> problem in here, and Simon's patch that I have since merged fixes that.
> There's seemingly something else further going on with N900 and only
> N900

It is not "only N900", see below.

> and you're saying the fix is to revert changes rather than explain
> what's wrong in any of the changes, still. The first change 'Revert
> "video: Enable VIDEO_ANSI by default only with EFI"' as a fix would be
> to add whatever conditional should also cause this to be "default y".
> And I tried doing that, but after reading the code, it doesn't make any
> sense why a few functions change on N900 when I do so. It's also the
> only platform where there's a change.
> 
> > > > And if you want something more from me then you show me why this time it
> > > > would be different, and again empty promises.
> > > 
> > > What I'd like is for you to not assume worst-faith.
> > 
> > Of course not. I'm just assuming that same thing happen as in the past.
> > From realistic point of view, this is the best approximation what to
> > assume.
> > 
> > Also you just few lines above wrote that fixing broken booting is not a
> > critical change at all, so you are just expressed that you are not going
> > to take fixes.
> 
> The only time your patches aren't accepted in a timely manner are when
> either (a) they get delayed because of custodians who don't have a lot
> of time to review an area or (b) you refuse to take feedback on a
> change. The feedback right now on this 3 part revert series is to take
> what Simon posted after trying things in QEMU and use that additional
> debug information to figure it out or post more detailed failure logs.
> 
> > > If you can fix the
> > > problem quickly at this point it'll get reviewed (assuming Simon has
> > > time, next week is EOSS) and merged if it's a safe enough looking fix.
> > > Otherwise it'll get in v2023.10.
> > 
> > And here you wrote that "change maybe land in v2023.10 version". But at
> > that time u-boot would move and again it would not land because change
> > from this time wound not applied on top of the master branch which would
> > be in the future.
> 
> Yes, if the change to fix exactly one platform is 10 lines, I'm going to
> be iffy about taking it a week before release.  If the change is 1 or 2
> obviously correct lines, I'll likely take the gamble. But since at this
> point there's exactly one known broken platform I'm not going to delay
> the release. There were more broken platforms when I said I'd take the 3
> part revert but Simon came along and fixed the problems everyone else
> reported and made a good faith attempt and fixing your platform too.
> 
> > I'm not newbie here, I see what is happening for more years with my
> > patches.
> > 
> > Or should I remind you what is the average time how long rx51 patches
> > have been waiting on the list until they were merged?
> 
> I mean, what's the average over the last year or two? Yes, they got
> delayed when I delegated them to the TI custodian who then had no real
> time to deal with U-Boot.
> 
> > I have already provided a fix for this problem. What happened? Nothing,
> > I was just ask to do another fix. Do you think, that I'm a total idiot,
> > who is going to prepare a new patch, which would be ignored or rejected
> > like the previous one, or other other work which I done?
> 
> Narrowing down a problem to a few commits and reverting them is a debug
> aide, unless the commits themselves are simply wholly broken and wrong.
> So no, you didn't fix the issue.  You narrowed it down.  I was willing
> to take that even if Simon was unable to figure out what was wrong as it
> was on multiple platforms, even.  So now you've been asked to dig more
> on your platform as it doesn't fail for anyone else anywhere else.

I have not found any issue on rx51 code, see below...

> > > > > > Hence, now I did only smaller - but still important - task: Locate exact
> > > > > > commits which broke particular feature and prepare reverts on top of the
> > > > > > master branch which _really_ fix the broken functionality - and make
> > > > > > code working again.
> > > > > 
> > > > > Unfortunately you're also the only person able to replicate the problem,
> > > > > and Simon has tried (and posted fixes to your platform for other issues,
> > > > > and debugging patches to hopefully making finding the problem easier).
> > > > 
> > > > Have somebody else even tried to reproduce this issue on any other HW
> > > > board or any other qemu board?
> > > 
> > > The summary of Simon's thread where he tried to fix this problem is that
> > > only your platform still has a problem here.
> > 
> > This is not answer to the question.
> 
> Simon.  Simon tested this on N900 in QEMU, found some problems, reported
> them, was unable to find further problems and pushed it back on you to
> review the patches for your platform to fix the problem he did see and
> was able to fix.
> 
> -- 
> Tom

I take some time, starting debugging this issue and...

1) I have not found any issue in n900 code. So if there is one, please
exactly show where and how.

2) This problem is possible to reproduce _without_ n900 board code,
without n900 keyboard driver.

So I'm asking again question:

> Have somebody else even tried to reproduce this issue on any other HW
> board or any other qemu board?

Because you have not answered to it, I will do it. Answer is NO.

Nobody tried to reproduce it, otherwise would be able to hit this issue.
And your arguments "I'm lazy in building n900 qemu version" is pointless
because this can be reproduce without n900 keyboard driver. Just on the
UART terminal.


Normally people who introduced regression should take care of them. And
if are not able to do it in time, then revert of the broken
functionality must be applied.

Nobody took any action here, I wrote some details where can be problems,
everybody completely ignored them; so I sent those reverts.


Now I have also look at those 3 problematic commits. First one is
obviously broken, I think it does not need more comments that what I
already wrote.

Second one is probably correct but it somehow interference with the
third one (and disallow to revert the third one). And the third one is
the problem. I have tried to trace what that original commit is doing,
but I have not understand. So I cannot debug this code.

So I'm again asking, revert these 3 commits, or show me proof of their
validity. I have already showed you they they are incorrect by simple
regression testing.


I'm really disappointed, that even after getting explicit information
about breakage, regression and identification what is wrong; no steps
were taken and you intentionally released broken u-boot for rx51.

Should I take it as a clear "we do not care about reporting
regressions" or "fuck you we do not need anymore, go way"?

Its pretty obvious now, that you do not want to do anything with it, so
I'm NOT going to debug & send patches.

Re: [PATCH u-boot 0/3] Revert broken Bootmenu commits

[Simon Glass]

Hi Pali,

On Mon, 10 Jul 2023 at 08:10, Pali Rohár <pali@kernel.org> wrote:
>
> On Sunday 25 June 2023 21:08:19 Tom Rini wrote:
> > On Sun, Jun 25, 2023 at 05:15:34PM +0200, Pali Rohár wrote:
> > > On Sunday 25 June 2023 10:52:13 Tom Rini wrote:
> > > > On Sun, Jun 25, 2023 at 09:50:39AM +0200, Pali Rohár wrote:
> > > > > On Saturday 24 June 2023 12:58:07 Tom Rini wrote:
> > > > > > On Sat, Jun 24, 2023 at 10:50:41AM +0200, Pali Rohár wrote:
> > > > > > > On Tuesday 20 June 2023 11:20:35 Simon Glass wrote:
> > > > > > > > Hi Tom, Pali,
> > > > > > > >
> > > > > > > > On Wed, 14 Jun 2023 at 20:51, Tom Rini <trini@konsulko.com> wrote:
> > > > > > > > >
> > > > > > > > > On Sun, Jun 11, 2023 at 02:53:21PM +0200, Pali Rohár wrote:
> > > > > > > > >
> > > > > > > > > > These 3 commits broke support for U-Boot Bootmenu on Nokia N900.
> > > > > > > > > > Issues were reported more than month ago, but nobody reacted to them.
> > > > > > > > > > So revert these broken commits for now, to fix U-Boot Bootmenu support.
> > > > > > > > > >
> > > > > > > > > > With these revered commits, U-Boot Bootmenu from master branch is
> > > > > > > > > > working fine again on Nokia N900.
> > > > > > > > > >
> > > > > > > > > > Pali Rohár (3):
> > > > > > > > > >   Revert "video: Enable VIDEO_ANSI by default only with EFI"
> > > > > > > > > >   Revert "menu: Factor out menu-keypress decoding"
> > > > > > > > > >   Revert "menu: Make use of CLI character processing"
> > > > > > > > > >
> > > > > > > > > >  cmd/bootmenu.c        |   9 ++--
> > > > > > > > > >  cmd/eficonfig.c       |  12 ++---
> > > > > > > > > >  common/cli_getch.c    |  12 ++---
> > > > > > > > > >  common/menu.c         | 122 ++++++++++++++++++++++++++----------------
> > > > > > > > > >  drivers/video/Kconfig |   7 +--
> > > > > > > > > >  include/cli.h         |   4 +-
> > > > > > > > > >  include/menu.h        |  17 +-----
> > > > > > > > > >  7 files changed, 91 insertions(+), 92 deletions(-)
> > > > > > > > >
> > > > > > > > > Following up over here, while
> > > > > > > > > https://patchwork.ozlabs.org/project/uboot/patch/20230612201434.861700-1-sjg@chromium.org/
> > > > > > > > > addresses some of the issues, but not all (as it clearly isn't working
> > > > > > > > > in all of the cases Pali has explained), looking in to the VIDEO_ANSI
> > > > > > > > > part of this too, all three of these reverts are related seemingly to
> > > > > > > > > something escape-character related.  I'm not taking any of the revert
> > > > > > > > > commits in just yet, but will by the next -rc if we don't have something
> > > > > > > > > that fixes all of the issues.
> > > > > > > >
> > > > > > > > I did send a series [1] with a fix for the nokia_rx51 keyboard driver,
> > > > > > > > including the previous ansi-code patch. Given that:
> > > > > > > >
> > > > > > > > - this problem doesn't seem to manifest on other boards
> > > > > > > > - it does not cause any CI test to fail
> > > > > > > > - there seem to be bugs in the nokia_rx51 implementation which are a
> > > > > > > > possible/likely cause
> > > > > > > > - the nokia_rx51 CI uses a 10-year-old out-of-tree QEMU
> > > > > > > > - the problem goes away if debugging is added, suggesting it is
> > > > > > > > related to timing
> > > > > > > >
> > > > > > > > ...I don't think a revert is appropriate.
> > > > > > >
> > > > > > > Unfortunately it does not fix this issue :-( New patch series from [1]
> > > > > > > applied on top of the master branch has still issue with DOWN key on
> > > > > > > emulated UART terminal.
> > > > > > >
> > > > > > > > Pali, can you please take a look and see if you can debug what is
> > > > > > > > actually going on? Here is my guess:
> > > > > > > >
> > > > > > > > 1. When an arrow key is pressed, cli_ch_process() handles it by being
> > > > > > > > passed the codes in sequence: \e [ B
> > > > > > > > 2. Calling cli_ch_process() with ichar = 0 causes it to think the
> > > > > > > > sequence is finished: \e [ \0 B   (this doesn't work since the \0 ends
> > > > > > > > the sequence)
> > > > > > > > 3. nokia_rx51 keyboard driver is returning '\0' even when key codes
> > > > > > > > are pending, causing cli_ch_process() to be told that the sequence is
> > > > > > > > done
> > > > > > >
> > > > > > > I can look at it later, but I'm loosing motivation to do whole debugging
> > > > > > > for another issue again, because for the last year my fixes and other
> > > > > > > patches were stalled and u-boot devs show me that they are not
> > > > > > > interested even in commenting them. I do not want to work again on
> > > > > > > something which would be ignored.
> > > > > >
> > > > > > Well, unless your changes here break something else, I don't think a fix
> > > > > > for your problem will be ignored.
> > > > >
> > > > > This is something which I read here more times in the past... and
> > > > > reality was different.
> > > > >
> > > > > Should I remind you that there are waiting eMMC fixes for mvebu and
> > > > > again discussion about them stalled? Or rather should I say that it is
> > > > > again ignored?
> > > >
> > > > No, you should just say they're still waiting for review.  Since they're
> > > > waiting for review. The MMC custodian has been asked to review them, and
> > > > hasn't yet. And they don't appear to be super critical changes, and they
> > > > conflict with other series, so yes, they'll get picked up when the
> > > > custodian has time to review everything.
> > >
> > > And what is "critical change" if it is not fixing booting (from eMMC)?
> >
> > The when and where, and since when. Since re-reading everything in
> > https://patchwork.ozlabs.org/project/uboot/list/?submitter=78810 that's
> > not this revert series doesn't say "fix booting on $platform that was
> > broken by ...".  It says clean up.  Clean up can wait.
>
> "Fix eMMC boot" - This is not clear that it is a "fix"???
>
> Or are you again going to play a game "I do not see your patches"?
> Ok, then here is direct link:
>
> https://lore.kernel.org/u-boot/20230413205750.10641-1-pali@kernel.org/
>
> > > > > I have already spend time on this and have done everything needed to
> > > > > make bootmenu working again. I have also prepared patches which are
> > > > > fixing this problem and which were also tested.
> > > >
> > > > Note that "I reverted the commit" is not a fix.
> > >
> > > And what is the "change which makes feature X working again after it was
> > > broken" if not a fix? Of course, it is _not_ fix because it is fixing
> > > the problem.
> >
> > The part where you're reverting changes that were made intentionally
> > rather than fixing whatever problem they introduce or expose. There is a
> > problem in here, and Simon's patch that I have since merged fixes that.
> > There's seemingly something else further going on with N900 and only
> > N900
>
> It is not "only N900", see below.
>
> > and you're saying the fix is to revert changes rather than explain
> > what's wrong in any of the changes, still. The first change 'Revert
> > "video: Enable VIDEO_ANSI by default only with EFI"' as a fix would be
> > to add whatever conditional should also cause this to be "default y".
> > And I tried doing that, but after reading the code, it doesn't make any
> > sense why a few functions change on N900 when I do so. It's also the
> > only platform where there's a change.
> >
> > > > > And if you want something more from me then you show me why this time it
> > > > > would be different, and again empty promises.
> > > >
> > > > What I'd like is for you to not assume worst-faith.
> > >
> > > Of course not. I'm just assuming that same thing happen as in the past.
> > > From realistic point of view, this is the best approximation what to
> > > assume.
> > >
> > > Also you just few lines above wrote that fixing broken booting is not a
> > > critical change at all, so you are just expressed that you are not going
> > > to take fixes.
> >
> > The only time your patches aren't accepted in a timely manner are when
> > either (a) they get delayed because of custodians who don't have a lot
> > of time to review an area or (b) you refuse to take feedback on a
> > change. The feedback right now on this 3 part revert series is to take
> > what Simon posted after trying things in QEMU and use that additional
> > debug information to figure it out or post more detailed failure logs.
> >
> > > > If you can fix the
> > > > problem quickly at this point it'll get reviewed (assuming Simon has
> > > > time, next week is EOSS) and merged if it's a safe enough looking fix.
> > > > Otherwise it'll get in v2023.10.
> > >
> > > And here you wrote that "change maybe land in v2023.10 version". But at
> > > that time u-boot would move and again it would not land because change
> > > from this time wound not applied on top of the master branch which would
> > > be in the future.
> >
> > Yes, if the change to fix exactly one platform is 10 lines, I'm going to
> > be iffy about taking it a week before release.  If the change is 1 or 2
> > obviously correct lines, I'll likely take the gamble. But since at this
> > point there's exactly one known broken platform I'm not going to delay
> > the release. There were more broken platforms when I said I'd take the 3
> > part revert but Simon came along and fixed the problems everyone else
> > reported and made a good faith attempt and fixing your platform too.
> >
> > > I'm not newbie here, I see what is happening for more years with my
> > > patches.
> > >
> > > Or should I remind you what is the average time how long rx51 patches
> > > have been waiting on the list until they were merged?
> >
> > I mean, what's the average over the last year or two? Yes, they got
> > delayed when I delegated them to the TI custodian who then had no real
> > time to deal with U-Boot.
> >
> > > I have already provided a fix for this problem. What happened? Nothing,
> > > I was just ask to do another fix. Do you think, that I'm a total idiot,
> > > who is going to prepare a new patch, which would be ignored or rejected
> > > like the previous one, or other other work which I done?
> >
> > Narrowing down a problem to a few commits and reverting them is a debug
> > aide, unless the commits themselves are simply wholly broken and wrong.
> > So no, you didn't fix the issue.  You narrowed it down.  I was willing
> > to take that even if Simon was unable to figure out what was wrong as it
> > was on multiple platforms, even.  So now you've been asked to dig more
> > on your platform as it doesn't fail for anyone else anywhere else.
>
> I have not found any issue on rx51 code, see below...
>
> > > > > > > Hence, now I did only smaller - but still important - task: Locate exact
> > > > > > > commits which broke particular feature and prepare reverts on top of the
> > > > > > > master branch which _really_ fix the broken functionality - and make
> > > > > > > code working again.
> > > > > >
> > > > > > Unfortunately you're also the only person able to replicate the problem,
> > > > > > and Simon has tried (and posted fixes to your platform for other issues,
> > > > > > and debugging patches to hopefully making finding the problem easier).
> > > > >
> > > > > Have somebody else even tried to reproduce this issue on any other HW
> > > > > board or any other qemu board?
> > > >
> > > > The summary of Simon's thread where he tried to fix this problem is that
> > > > only your platform still has a problem here.
> > >
> > > This is not answer to the question.
> >
> > Simon.  Simon tested this on N900 in QEMU, found some problems, reported
> > them, was unable to find further problems and pushed it back on you to
> > review the patches for your platform to fix the problem he did see and
> > was able to fix.
> >
> > --
> > Tom
>
> I take some time, starting debugging this issue and...
>
> 1) I have not found any issue in n900 code. So if there is one, please
> exactly show where and how.

nokia_rx51 keyboard driver (rx51_kp_tstc) is returning '\0' even when key codes
are pending, causing cli_ch_process() to be told that the sequence is
done

If you can fix that, it will probably spring into life, but at least I
might be able to figure out what is wrong.

>
> 2) This problem is possible to reproduce _without_ n900 board code,
> without n900 keyboard driver.

How can this problem be reproduced? I can see the problem in qemu on
n900 but not on sandbox. Does it happen on another board?

Regards,
Simon

[..]

Re: [PATCH u-boot 0/3] Revert broken Bootmenu commits

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 13262 bytes --]

On Mon, Jul 10, 2023 at 04:10:39PM +0200, Pali Rohár wrote:
> On Sunday 25 June 2023 21:08:19 Tom Rini wrote:
> > On Sun, Jun 25, 2023 at 05:15:34PM +0200, Pali Rohár wrote:
> > > On Sunday 25 June 2023 10:52:13 Tom Rini wrote:
> > > > On Sun, Jun 25, 2023 at 09:50:39AM +0200, Pali Rohár wrote:
> > > > > On Saturday 24 June 2023 12:58:07 Tom Rini wrote:
> > > > > > On Sat, Jun 24, 2023 at 10:50:41AM +0200, Pali Rohár wrote:
> > > > > > > On Tuesday 20 June 2023 11:20:35 Simon Glass wrote:
> > > > > > > > Hi Tom, Pali,
> > > > > > > > 
> > > > > > > > On Wed, 14 Jun 2023 at 20:51, Tom Rini <trini@konsulko.com> wrote:
> > > > > > > > >
> > > > > > > > > On Sun, Jun 11, 2023 at 02:53:21PM +0200, Pali Rohár wrote:
> > > > > > > > >
> > > > > > > > > > These 3 commits broke support for U-Boot Bootmenu on Nokia N900.
> > > > > > > > > > Issues were reported more than month ago, but nobody reacted to them.
> > > > > > > > > > So revert these broken commits for now, to fix U-Boot Bootmenu support.
> > > > > > > > > >
> > > > > > > > > > With these revered commits, U-Boot Bootmenu from master branch is
> > > > > > > > > > working fine again on Nokia N900.
> > > > > > > > > >
> > > > > > > > > > Pali Rohár (3):
> > > > > > > > > >   Revert "video: Enable VIDEO_ANSI by default only with EFI"
> > > > > > > > > >   Revert "menu: Factor out menu-keypress decoding"
> > > > > > > > > >   Revert "menu: Make use of CLI character processing"
> > > > > > > > > >
> > > > > > > > > >  cmd/bootmenu.c        |   9 ++--
> > > > > > > > > >  cmd/eficonfig.c       |  12 ++---
> > > > > > > > > >  common/cli_getch.c    |  12 ++---
> > > > > > > > > >  common/menu.c         | 122 ++++++++++++++++++++++++++----------------
> > > > > > > > > >  drivers/video/Kconfig |   7 +--
> > > > > > > > > >  include/cli.h         |   4 +-
> > > > > > > > > >  include/menu.h        |  17 +-----
> > > > > > > > > >  7 files changed, 91 insertions(+), 92 deletions(-)
> > > > > > > > >
> > > > > > > > > Following up over here, while
> > > > > > > > > https://patchwork.ozlabs.org/project/uboot/patch/20230612201434.861700-1-sjg@chromium.org/
> > > > > > > > > addresses some of the issues, but not all (as it clearly isn't working
> > > > > > > > > in all of the cases Pali has explained), looking in to the VIDEO_ANSI
> > > > > > > > > part of this too, all three of these reverts are related seemingly to
> > > > > > > > > something escape-character related.  I'm not taking any of the revert
> > > > > > > > > commits in just yet, but will by the next -rc if we don't have something
> > > > > > > > > that fixes all of the issues.
> > > > > > > > 
> > > > > > > > I did send a series [1] with a fix for the nokia_rx51 keyboard driver,
> > > > > > > > including the previous ansi-code patch. Given that:
> > > > > > > > 
> > > > > > > > - this problem doesn't seem to manifest on other boards
> > > > > > > > - it does not cause any CI test to fail
> > > > > > > > - there seem to be bugs in the nokia_rx51 implementation which are a
> > > > > > > > possible/likely cause
> > > > > > > > - the nokia_rx51 CI uses a 10-year-old out-of-tree QEMU
> > > > > > > > - the problem goes away if debugging is added, suggesting it is
> > > > > > > > related to timing
> > > > > > > > 
> > > > > > > > ...I don't think a revert is appropriate.
> > > > > > > 
> > > > > > > Unfortunately it does not fix this issue :-( New patch series from [1]
> > > > > > > applied on top of the master branch has still issue with DOWN key on
> > > > > > > emulated UART terminal.
> > > > > > > 
> > > > > > > > Pali, can you please take a look and see if you can debug what is
> > > > > > > > actually going on? Here is my guess:
> > > > > > > > 
> > > > > > > > 1. When an arrow key is pressed, cli_ch_process() handles it by being
> > > > > > > > passed the codes in sequence: \e [ B
> > > > > > > > 2. Calling cli_ch_process() with ichar = 0 causes it to think the
> > > > > > > > sequence is finished: \e [ \0 B   (this doesn't work since the \0 ends
> > > > > > > > the sequence)
> > > > > > > > 3. nokia_rx51 keyboard driver is returning '\0' even when key codes
> > > > > > > > are pending, causing cli_ch_process() to be told that the sequence is
> > > > > > > > done
> > > > > > > 
> > > > > > > I can look at it later, but I'm loosing motivation to do whole debugging
> > > > > > > for another issue again, because for the last year my fixes and other
> > > > > > > patches were stalled and u-boot devs show me that they are not
> > > > > > > interested even in commenting them. I do not want to work again on
> > > > > > > something which would be ignored.
> > > > > > 
> > > > > > Well, unless your changes here break something else, I don't think a fix
> > > > > > for your problem will be ignored.
> > > > > 
> > > > > This is something which I read here more times in the past... and
> > > > > reality was different.
> > > > > 
> > > > > Should I remind you that there are waiting eMMC fixes for mvebu and
> > > > > again discussion about them stalled? Or rather should I say that it is
> > > > > again ignored?
> > > > 
> > > > No, you should just say they're still waiting for review.  Since they're
> > > > waiting for review. The MMC custodian has been asked to review them, and
> > > > hasn't yet. And they don't appear to be super critical changes, and they
> > > > conflict with other series, so yes, they'll get picked up when the
> > > > custodian has time to review everything.
> > > 
> > > And what is "critical change" if it is not fixing booting (from eMMC)?
> > 
> > The when and where, and since when. Since re-reading everything in
> > https://patchwork.ozlabs.org/project/uboot/list/?submitter=78810 that's
> > not this revert series doesn't say "fix booting on $platform that was
> > broken by ...".  It says clean up.  Clean up can wait.
> 
> "Fix eMMC boot" - This is not clear that it is a "fix"???
> 
> Or are you again going to play a game "I do not see your patches"?
> Ok, then here is direct link:
> 
> https://lore.kernel.org/u-boot/20230413205750.10641-1-pali@kernel.org/

Oh, oh, I think I get it now. You're wondering why I guess:
https://patchwork.ozlabs.org/project/uboot/patch/20230505193710.n35h2ofq6fogk4bq@pali/
https://patchwork.ozlabs.org/project/uboot/patch/20230516184943.7206-1-pali@kernel.org/
haven't been picked up? They're assigned to Stefan.

I thought you were talking about your other eMMC related patches.

> > > > > I have already spend time on this and have done everything needed to
> > > > > make bootmenu working again. I have also prepared patches which are
> > > > > fixing this problem and which were also tested.
> > > > 
> > > > Note that "I reverted the commit" is not a fix.
> > > 
> > > And what is the "change which makes feature X working again after it was
> > > broken" if not a fix? Of course, it is _not_ fix because it is fixing
> > > the problem.
> > 
> > The part where you're reverting changes that were made intentionally
> > rather than fixing whatever problem they introduce or expose. There is a
> > problem in here, and Simon's patch that I have since merged fixes that.
> > There's seemingly something else further going on with N900 and only
> > N900
> 
> It is not "only N900", see below.
> 
> > and you're saying the fix is to revert changes rather than explain
> > what's wrong in any of the changes, still. The first change 'Revert
> > "video: Enable VIDEO_ANSI by default only with EFI"' as a fix would be
> > to add whatever conditional should also cause this to be "default y".
> > And I tried doing that, but after reading the code, it doesn't make any
> > sense why a few functions change on N900 when I do so. It's also the
> > only platform where there's a change.
> > 
> > > > > And if you want something more from me then you show me why this time it
> > > > > would be different, and again empty promises.
> > > > 
> > > > What I'd like is for you to not assume worst-faith.
> > > 
> > > Of course not. I'm just assuming that same thing happen as in the past.
> > > From realistic point of view, this is the best approximation what to
> > > assume.
> > > 
> > > Also you just few lines above wrote that fixing broken booting is not a
> > > critical change at all, so you are just expressed that you are not going
> > > to take fixes.
> > 
> > The only time your patches aren't accepted in a timely manner are when
> > either (a) they get delayed because of custodians who don't have a lot
> > of time to review an area or (b) you refuse to take feedback on a
> > change. The feedback right now on this 3 part revert series is to take
> > what Simon posted after trying things in QEMU and use that additional
> > debug information to figure it out or post more detailed failure logs.
> > 
> > > > If you can fix the
> > > > problem quickly at this point it'll get reviewed (assuming Simon has
> > > > time, next week is EOSS) and merged if it's a safe enough looking fix.
> > > > Otherwise it'll get in v2023.10.
> > > 
> > > And here you wrote that "change maybe land in v2023.10 version". But at
> > > that time u-boot would move and again it would not land because change
> > > from this time wound not applied on top of the master branch which would
> > > be in the future.
> > 
> > Yes, if the change to fix exactly one platform is 10 lines, I'm going to
> > be iffy about taking it a week before release.  If the change is 1 or 2
> > obviously correct lines, I'll likely take the gamble. But since at this
> > point there's exactly one known broken platform I'm not going to delay
> > the release. There were more broken platforms when I said I'd take the 3
> > part revert but Simon came along and fixed the problems everyone else
> > reported and made a good faith attempt and fixing your platform too.
> > 
> > > I'm not newbie here, I see what is happening for more years with my
> > > patches.
> > > 
> > > Or should I remind you what is the average time how long rx51 patches
> > > have been waiting on the list until they were merged?
> > 
> > I mean, what's the average over the last year or two? Yes, they got
> > delayed when I delegated them to the TI custodian who then had no real
> > time to deal with U-Boot.
> > 
> > > I have already provided a fix for this problem. What happened? Nothing,
> > > I was just ask to do another fix. Do you think, that I'm a total idiot,
> > > who is going to prepare a new patch, which would be ignored or rejected
> > > like the previous one, or other other work which I done?
> > 
> > Narrowing down a problem to a few commits and reverting them is a debug
> > aide, unless the commits themselves are simply wholly broken and wrong.
> > So no, you didn't fix the issue.  You narrowed it down.  I was willing
> > to take that even if Simon was unable to figure out what was wrong as it
> > was on multiple platforms, even.  So now you've been asked to dig more
> > on your platform as it doesn't fail for anyone else anywhere else.
> 
> I have not found any issue on rx51 code, see below...
> 
> > > > > > > Hence, now I did only smaller - but still important - task: Locate exact
> > > > > > > commits which broke particular feature and prepare reverts on top of the
> > > > > > > master branch which _really_ fix the broken functionality - and make
> > > > > > > code working again.
> > > > > > 
> > > > > > Unfortunately you're also the only person able to replicate the problem,
> > > > > > and Simon has tried (and posted fixes to your platform for other issues,
> > > > > > and debugging patches to hopefully making finding the problem easier).
> > > > > 
> > > > > Have somebody else even tried to reproduce this issue on any other HW
> > > > > board or any other qemu board?
> > > > 
> > > > The summary of Simon's thread where he tried to fix this problem is that
> > > > only your platform still has a problem here.
> > > 
> > > This is not answer to the question.
> > 
> > Simon.  Simon tested this on N900 in QEMU, found some problems, reported
> > them, was unable to find further problems and pushed it back on you to
> > review the patches for your platform to fix the problem he did see and
> > was able to fix.
> > 
> > -- 
> > Tom
> 
> I take some time, starting debugging this issue and...
> 
> 1) I have not found any issue in n900 code. So if there is one, please
> exactly show where and how.
> 
> 2) This problem is possible to reproduce _without_ n900 board code,
> without n900 keyboard driver.
> 
> So I'm asking again question:
> 
> > Have somebody else even tried to reproduce this issue on any other HW
> > board or any other qemu board?
> 
> Because you have not answered to it, I will do it. Answer is NO.

I'm just going to leave at Simon's answer to this, after repeating
myself. He introduced the issues, debugged the issues, fixed the issues
on other platforms and pointed out problems in the n900 code. The only
other problems that were visible on other platforms have been resolved
on those other platforms (and tested-by the reporter).

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH u-boot 0/3] Revert broken Bootmenu commits

[Stefan Roese]

On 7/10/23 17:17, Tom Rini wrote:
> On Mon, Jul 10, 2023 at 04:10:39PM +0200, Pali Rohár wrote:
>> On Sunday 25 June 2023 21:08:19 Tom Rini wrote:
>>> On Sun, Jun 25, 2023 at 05:15:34PM +0200, Pali Rohár wrote:
>>>> On Sunday 25 June 2023 10:52:13 Tom Rini wrote:
>>>>> On Sun, Jun 25, 2023 at 09:50:39AM +0200, Pali Rohár wrote:
>>>>>> On Saturday 24 June 2023 12:58:07 Tom Rini wrote:
>>>>>>> On Sat, Jun 24, 2023 at 10:50:41AM +0200, Pali Rohár wrote:
>>>>>>>> On Tuesday 20 June 2023 11:20:35 Simon Glass wrote:
>>>>>>>>> Hi Tom, Pali,
>>>>>>>>>
>>>>>>>>> On Wed, 14 Jun 2023 at 20:51, Tom Rini <trini@konsulko.com> wrote:
>>>>>>>>>>
>>>>>>>>>> On Sun, Jun 11, 2023 at 02:53:21PM +0200, Pali Rohár wrote:
>>>>>>>>>>
>>>>>>>>>>> These 3 commits broke support for U-Boot Bootmenu on Nokia N900.
>>>>>>>>>>> Issues were reported more than month ago, but nobody reacted to them.
>>>>>>>>>>> So revert these broken commits for now, to fix U-Boot Bootmenu support.
>>>>>>>>>>>
>>>>>>>>>>> With these revered commits, U-Boot Bootmenu from master branch is
>>>>>>>>>>> working fine again on Nokia N900.
>>>>>>>>>>>
>>>>>>>>>>> Pali Rohár (3):
>>>>>>>>>>>    Revert "video: Enable VIDEO_ANSI by default only with EFI"
>>>>>>>>>>>    Revert "menu: Factor out menu-keypress decoding"
>>>>>>>>>>>    Revert "menu: Make use of CLI character processing"
>>>>>>>>>>>
>>>>>>>>>>>   cmd/bootmenu.c        |   9 ++--
>>>>>>>>>>>   cmd/eficonfig.c       |  12 ++---
>>>>>>>>>>>   common/cli_getch.c    |  12 ++---
>>>>>>>>>>>   common/menu.c         | 122 ++++++++++++++++++++++++++----------------
>>>>>>>>>>>   drivers/video/Kconfig |   7 +--
>>>>>>>>>>>   include/cli.h         |   4 +-
>>>>>>>>>>>   include/menu.h        |  17 +-----
>>>>>>>>>>>   7 files changed, 91 insertions(+), 92 deletions(-)
>>>>>>>>>>
>>>>>>>>>> Following up over here, while
>>>>>>>>>> https://patchwork.ozlabs.org/project/uboot/patch/20230612201434.861700-1-sjg@chromium.org/
>>>>>>>>>> addresses some of the issues, but not all (as it clearly isn't working
>>>>>>>>>> in all of the cases Pali has explained), looking in to the VIDEO_ANSI
>>>>>>>>>> part of this too, all three of these reverts are related seemingly to
>>>>>>>>>> something escape-character related.  I'm not taking any of the revert
>>>>>>>>>> commits in just yet, but will by the next -rc if we don't have something
>>>>>>>>>> that fixes all of the issues.
>>>>>>>>>
>>>>>>>>> I did send a series [1] with a fix for the nokia_rx51 keyboard driver,
>>>>>>>>> including the previous ansi-code patch. Given that:
>>>>>>>>>
>>>>>>>>> - this problem doesn't seem to manifest on other boards
>>>>>>>>> - it does not cause any CI test to fail
>>>>>>>>> - there seem to be bugs in the nokia_rx51 implementation which are a
>>>>>>>>> possible/likely cause
>>>>>>>>> - the nokia_rx51 CI uses a 10-year-old out-of-tree QEMU
>>>>>>>>> - the problem goes away if debugging is added, suggesting it is
>>>>>>>>> related to timing
>>>>>>>>>
>>>>>>>>> ...I don't think a revert is appropriate.
>>>>>>>>
>>>>>>>> Unfortunately it does not fix this issue :-( New patch series from [1]
>>>>>>>> applied on top of the master branch has still issue with DOWN key on
>>>>>>>> emulated UART terminal.
>>>>>>>>
>>>>>>>>> Pali, can you please take a look and see if you can debug what is
>>>>>>>>> actually going on? Here is my guess:
>>>>>>>>>
>>>>>>>>> 1. When an arrow key is pressed, cli_ch_process() handles it by being
>>>>>>>>> passed the codes in sequence: \e [ B
>>>>>>>>> 2. Calling cli_ch_process() with ichar = 0 causes it to think the
>>>>>>>>> sequence is finished: \e [ \0 B   (this doesn't work since the \0 ends
>>>>>>>>> the sequence)
>>>>>>>>> 3. nokia_rx51 keyboard driver is returning '\0' even when key codes
>>>>>>>>> are pending, causing cli_ch_process() to be told that the sequence is
>>>>>>>>> done
>>>>>>>>
>>>>>>>> I can look at it later, but I'm loosing motivation to do whole debugging
>>>>>>>> for another issue again, because for the last year my fixes and other
>>>>>>>> patches were stalled and u-boot devs show me that they are not
>>>>>>>> interested even in commenting them. I do not want to work again on
>>>>>>>> something which would be ignored.
>>>>>>>
>>>>>>> Well, unless your changes here break something else, I don't think a fix
>>>>>>> for your problem will be ignored.
>>>>>>
>>>>>> This is something which I read here more times in the past... and
>>>>>> reality was different.
>>>>>>
>>>>>> Should I remind you that there are waiting eMMC fixes for mvebu and
>>>>>> again discussion about them stalled? Or rather should I say that it is
>>>>>> again ignored?
>>>>>
>>>>> No, you should just say they're still waiting for review.  Since they're
>>>>> waiting for review. The MMC custodian has been asked to review them, and
>>>>> hasn't yet. And they don't appear to be super critical changes, and they
>>>>> conflict with other series, so yes, they'll get picked up when the
>>>>> custodian has time to review everything.
>>>>
>>>> And what is "critical change" if it is not fixing booting (from eMMC)?
>>>
>>> The when and where, and since when. Since re-reading everything in
>>> https://patchwork.ozlabs.org/project/uboot/list/?submitter=78810 that's
>>> not this revert series doesn't say "fix booting on $platform that was
>>> broken by ...".  It says clean up.  Clean up can wait.
>>
>> "Fix eMMC boot" - This is not clear that it is a "fix"???
>>
>> Or are you again going to play a game "I do not see your patches"?
>> Ok, then here is direct link:
>>
>> https://lore.kernel.org/u-boot/20230413205750.10641-1-pali@kernel.org/
> 
> Oh, oh, I think I get it now. You're wondering why I guess:
> https://patchwork.ozlabs.org/project/uboot/patch/20230505193710.n35h2ofq6fogk4bq@pali/
> https://patchwork.ozlabs.org/project/uboot/patch/20230516184943.7206-1-pali@kernel.org/
> haven't been picked up? They're assigned to Stefan.

Hmmm, I might have missed something here. I'm traveling right now, so
my time is limited this week. Perhaps by end of this week. Or the
patches should be delegated to some other custodian if this makes
sense.

Thanks,
Stefan

> I thought you were talking about your other eMMC related patches.
> 
>>>>>> I have already spend time on this and have done everything needed to
>>>>>> make bootmenu working again. I have also prepared patches which are
>>>>>> fixing this problem and which were also tested.
>>>>>
>>>>> Note that "I reverted the commit" is not a fix.
>>>>
>>>> And what is the "change which makes feature X working again after it was
>>>> broken" if not a fix? Of course, it is _not_ fix because it is fixing
>>>> the problem.
>>>
>>> The part where you're reverting changes that were made intentionally
>>> rather than fixing whatever problem they introduce or expose. There is a
>>> problem in here, and Simon's patch that I have since merged fixes that.
>>> There's seemingly something else further going on with N900 and only
>>> N900
>>
>> It is not "only N900", see below.
>>
>>> and you're saying the fix is to revert changes rather than explain
>>> what's wrong in any of the changes, still. The first change 'Revert
>>> "video: Enable VIDEO_ANSI by default only with EFI"' as a fix would be
>>> to add whatever conditional should also cause this to be "default y".
>>> And I tried doing that, but after reading the code, it doesn't make any
>>> sense why a few functions change on N900 when I do so. It's also the
>>> only platform where there's a change.
>>>
>>>>>> And if you want something more from me then you show me why this time it
>>>>>> would be different, and again empty promises.
>>>>>
>>>>> What I'd like is for you to not assume worst-faith.
>>>>
>>>> Of course not. I'm just assuming that same thing happen as in the past.
>>>>  From realistic point of view, this is the best approximation what to
>>>> assume.
>>>>
>>>> Also you just few lines above wrote that fixing broken booting is not a
>>>> critical change at all, so you are just expressed that you are not going
>>>> to take fixes.
>>>
>>> The only time your patches aren't accepted in a timely manner are when
>>> either (a) they get delayed because of custodians who don't have a lot
>>> of time to review an area or (b) you refuse to take feedback on a
>>> change. The feedback right now on this 3 part revert series is to take
>>> what Simon posted after trying things in QEMU and use that additional
>>> debug information to figure it out or post more detailed failure logs.
>>>
>>>>> If you can fix the
>>>>> problem quickly at this point it'll get reviewed (assuming Simon has
>>>>> time, next week is EOSS) and merged if it's a safe enough looking fix.
>>>>> Otherwise it'll get in v2023.10.
>>>>
>>>> And here you wrote that "change maybe land in v2023.10 version". But at
>>>> that time u-boot would move and again it would not land because change
>>>> from this time wound not applied on top of the master branch which would
>>>> be in the future.
>>>
>>> Yes, if the change to fix exactly one platform is 10 lines, I'm going to
>>> be iffy about taking it a week before release.  If the change is 1 or 2
>>> obviously correct lines, I'll likely take the gamble. But since at this
>>> point there's exactly one known broken platform I'm not going to delay
>>> the release. There were more broken platforms when I said I'd take the 3
>>> part revert but Simon came along and fixed the problems everyone else
>>> reported and made a good faith attempt and fixing your platform too.
>>>
>>>> I'm not newbie here, I see what is happening for more years with my
>>>> patches.
>>>>
>>>> Or should I remind you what is the average time how long rx51 patches
>>>> have been waiting on the list until they were merged?
>>>
>>> I mean, what's the average over the last year or two? Yes, they got
>>> delayed when I delegated them to the TI custodian who then had no real
>>> time to deal with U-Boot.
>>>
>>>> I have already provided a fix for this problem. What happened? Nothing,
>>>> I was just ask to do another fix. Do you think, that I'm a total idiot,
>>>> who is going to prepare a new patch, which would be ignored or rejected
>>>> like the previous one, or other other work which I done?
>>>
>>> Narrowing down a problem to a few commits and reverting them is a debug
>>> aide, unless the commits themselves are simply wholly broken and wrong.
>>> So no, you didn't fix the issue.  You narrowed it down.  I was willing
>>> to take that even if Simon was unable to figure out what was wrong as it
>>> was on multiple platforms, even.  So now you've been asked to dig more
>>> on your platform as it doesn't fail for anyone else anywhere else.
>>
>> I have not found any issue on rx51 code, see below...
>>
>>>>>>>> Hence, now I did only smaller - but still important - task: Locate exact
>>>>>>>> commits which broke particular feature and prepare reverts on top of the
>>>>>>>> master branch which _really_ fix the broken functionality - and make
>>>>>>>> code working again.
>>>>>>>
>>>>>>> Unfortunately you're also the only person able to replicate the problem,
>>>>>>> and Simon has tried (and posted fixes to your platform for other issues,
>>>>>>> and debugging patches to hopefully making finding the problem easier).
>>>>>>
>>>>>> Have somebody else even tried to reproduce this issue on any other HW
>>>>>> board or any other qemu board?
>>>>>
>>>>> The summary of Simon's thread where he tried to fix this problem is that
>>>>> only your platform still has a problem here.
>>>>
>>>> This is not answer to the question.
>>>
>>> Simon.  Simon tested this on N900 in QEMU, found some problems, reported
>>> them, was unable to find further problems and pushed it back on you to
>>> review the patches for your platform to fix the problem he did see and
>>> was able to fix.
>>>
>>> -- 
>>> Tom
>>
>> I take some time, starting debugging this issue and...
>>
>> 1) I have not found any issue in n900 code. So if there is one, please
>> exactly show where and how.
>>
>> 2) This problem is possible to reproduce _without_ n900 board code,
>> without n900 keyboard driver.
>>
>> So I'm asking again question:
>>
>>> Have somebody else even tried to reproduce this issue on any other HW
>>> board or any other qemu board?
>>
>> Because you have not answered to it, I will do it. Answer is NO.
> 
> I'm just going to leave at Simon's answer to this, after repeating
> myself. He introduced the issues, debugged the issues, fixed the issues
> on other platforms and pointed out problems in the n900 code. The only
> other problems that were visible on other platforms have been resolved
> on those other platforms (and tested-by the reporter).
> 

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH u-boot 0/3] Revert broken Bootmenu commits

[Stefan Roese]

Hi,

(added Jaehoon Chung to Cc)

On 7/10/23 18:03, Stefan Roese wrote:
> On 7/10/23 17:17, Tom Rini wrote:
>> On Mon, Jul 10, 2023 at 04:10:39PM +0200, Pali Rohár wrote:
>>> On Sunday 25 June 2023 21:08:19 Tom Rini wrote:
>>>> On Sun, Jun 25, 2023 at 05:15:34PM +0200, Pali Rohár wrote:
>>>>> On Sunday 25 June 2023 10:52:13 Tom Rini wrote:
>>>>>> On Sun, Jun 25, 2023 at 09:50:39AM +0200, Pali Rohár wrote:
>>>>>>> On Saturday 24 June 2023 12:58:07 Tom Rini wrote:
>>>>>>>> On Sat, Jun 24, 2023 at 10:50:41AM +0200, Pali Rohár wrote:
>>>>>>>>> On Tuesday 20 June 2023 11:20:35 Simon Glass wrote:
>>>>>>>>>> Hi Tom, Pali,
>>>>>>>>>>
>>>>>>>>>> On Wed, 14 Jun 2023 at 20:51, Tom Rini <trini@konsulko.com> 
>>>>>>>>>> wrote:
>>>>>>>>>>>
>>>>>>>>>>> On Sun, Jun 11, 2023 at 02:53:21PM +0200, Pali Rohár wrote:
>>>>>>>>>>>
>>>>>>>>>>>> These 3 commits broke support for U-Boot Bootmenu on Nokia 
>>>>>>>>>>>> N900.
>>>>>>>>>>>> Issues were reported more than month ago, but nobody reacted 
>>>>>>>>>>>> to them.
>>>>>>>>>>>> So revert these broken commits for now, to fix U-Boot 
>>>>>>>>>>>> Bootmenu support.
>>>>>>>>>>>>
>>>>>>>>>>>> With these revered commits, U-Boot Bootmenu from master 
>>>>>>>>>>>> branch is
>>>>>>>>>>>> working fine again on Nokia N900.
>>>>>>>>>>>>
>>>>>>>>>>>> Pali Rohár (3):
>>>>>>>>>>>>    Revert "video: Enable VIDEO_ANSI by default only with EFI"
>>>>>>>>>>>>    Revert "menu: Factor out menu-keypress decoding"
>>>>>>>>>>>>    Revert "menu: Make use of CLI character processing"
>>>>>>>>>>>>
>>>>>>>>>>>>   cmd/bootmenu.c        |   9 ++--
>>>>>>>>>>>>   cmd/eficonfig.c       |  12 ++---
>>>>>>>>>>>>   common/cli_getch.c    |  12 ++---
>>>>>>>>>>>>   common/menu.c         | 122 
>>>>>>>>>>>> ++++++++++++++++++++++++++----------------
>>>>>>>>>>>>   drivers/video/Kconfig |   7 +--
>>>>>>>>>>>>   include/cli.h         |   4 +-
>>>>>>>>>>>>   include/menu.h        |  17 +-----
>>>>>>>>>>>>   7 files changed, 91 insertions(+), 92 deletions(-)
>>>>>>>>>>>
>>>>>>>>>>> Following up over here, while
>>>>>>>>>>> https://patchwork.ozlabs.org/project/uboot/patch/20230612201434.861700-1-sjg@chromium.org/
>>>>>>>>>>> addresses some of the issues, but not all (as it clearly 
>>>>>>>>>>> isn't working
>>>>>>>>>>> in all of the cases Pali has explained), looking in to the 
>>>>>>>>>>> VIDEO_ANSI
>>>>>>>>>>> part of this too, all three of these reverts are related 
>>>>>>>>>>> seemingly to
>>>>>>>>>>> something escape-character related.  I'm not taking any of 
>>>>>>>>>>> the revert
>>>>>>>>>>> commits in just yet, but will by the next -rc if we don't 
>>>>>>>>>>> have something
>>>>>>>>>>> that fixes all of the issues.
>>>>>>>>>>
>>>>>>>>>> I did send a series [1] with a fix for the nokia_rx51 keyboard 
>>>>>>>>>> driver,
>>>>>>>>>> including the previous ansi-code patch. Given that:
>>>>>>>>>>
>>>>>>>>>> - this problem doesn't seem to manifest on other boards
>>>>>>>>>> - it does not cause any CI test to fail
>>>>>>>>>> - there seem to be bugs in the nokia_rx51 implementation which 
>>>>>>>>>> are a
>>>>>>>>>> possible/likely cause
>>>>>>>>>> - the nokia_rx51 CI uses a 10-year-old out-of-tree QEMU
>>>>>>>>>> - the problem goes away if debugging is added, suggesting it is
>>>>>>>>>> related to timing
>>>>>>>>>>
>>>>>>>>>> ...I don't think a revert is appropriate.
>>>>>>>>>
>>>>>>>>> Unfortunately it does not fix this issue :-( New patch series 
>>>>>>>>> from [1]
>>>>>>>>> applied on top of the master branch has still issue with DOWN 
>>>>>>>>> key on
>>>>>>>>> emulated UART terminal.
>>>>>>>>>
>>>>>>>>>> Pali, can you please take a look and see if you can debug what is
>>>>>>>>>> actually going on? Here is my guess:
>>>>>>>>>>
>>>>>>>>>> 1. When an arrow key is pressed, cli_ch_process() handles it 
>>>>>>>>>> by being
>>>>>>>>>> passed the codes in sequence: \e [ B
>>>>>>>>>> 2. Calling cli_ch_process() with ichar = 0 causes it to think the
>>>>>>>>>> sequence is finished: \e [ \0 B   (this doesn't work since the 
>>>>>>>>>> \0 ends
>>>>>>>>>> the sequence)
>>>>>>>>>> 3. nokia_rx51 keyboard driver is returning '\0' even when key 
>>>>>>>>>> codes
>>>>>>>>>> are pending, causing cli_ch_process() to be told that the 
>>>>>>>>>> sequence is
>>>>>>>>>> done
>>>>>>>>>
>>>>>>>>> I can look at it later, but I'm loosing motivation to do whole 
>>>>>>>>> debugging
>>>>>>>>> for another issue again, because for the last year my fixes and 
>>>>>>>>> other
>>>>>>>>> patches were stalled and u-boot devs show me that they are not
>>>>>>>>> interested even in commenting them. I do not want to work again on
>>>>>>>>> something which would be ignored.
>>>>>>>>
>>>>>>>> Well, unless your changes here break something else, I don't 
>>>>>>>> think a fix
>>>>>>>> for your problem will be ignored.
>>>>>>>
>>>>>>> This is something which I read here more times in the past... and
>>>>>>> reality was different.
>>>>>>>
>>>>>>> Should I remind you that there are waiting eMMC fixes for mvebu and
>>>>>>> again discussion about them stalled? Or rather should I say that 
>>>>>>> it is
>>>>>>> again ignored?
>>>>>>
>>>>>> No, you should just say they're still waiting for review.  Since 
>>>>>> they're
>>>>>> waiting for review. The MMC custodian has been asked to review 
>>>>>> them, and
>>>>>> hasn't yet. And they don't appear to be super critical changes, 
>>>>>> and they
>>>>>> conflict with other series, so yes, they'll get picked up when the
>>>>>> custodian has time to review everything.
>>>>>
>>>>> And what is "critical change" if it is not fixing booting (from eMMC)?
>>>>
>>>> The when and where, and since when. Since re-reading everything in
>>>> https://patchwork.ozlabs.org/project/uboot/list/?submitter=78810 that's
>>>> not this revert series doesn't say "fix booting on $platform that was
>>>> broken by ...".  It says clean up.  Clean up can wait.
>>>
>>> "Fix eMMC boot" - This is not clear that it is a "fix"???
>>>
>>> Or are you again going to play a game "I do not see your patches"?
>>> Ok, then here is direct link:
>>>
>>> https://lore.kernel.org/u-boot/20230413205750.10641-1-pali@kernel.org/
>>
>> Oh, oh, I think I get it now. You're wondering why I guess:
>> https://patchwork.ozlabs.org/project/uboot/patch/20230505193710.n35h2ofq6fogk4bq@pali/
>> https://patchwork.ozlabs.org/project/uboot/patch/20230516184943.7206-1-pali@kernel.org/
>> haven't been picked up? They're assigned to Stefan.
> 
> Hmmm, I might have missed something here. I'm traveling right now, so
> my time is limited this week. Perhaps by end of this week. Or the
> patches should be delegated to some other custodian if this makes
> sense.

So I've worked though a few of the pending patches and also the 2 eMMC
related ones mentioned above. They depend on this series AFAICT:

mmc: Explain and cleanup partition selection

Which is currently being reviewed by Jaehoon Chung AFAIU. Once this is
applied I'll take care of these other 2 patches above (if possible).

Thanks,
Stefan