[PATCH v6 0/6] Implement fastboot multiresponce

[PATCH v6 0/6] Implement fastboot multiresponce

[Svyatoslav Ryhel]

Currently u-boot fastboot can only send one message back to host,
so if there is a need to print more than one line messages must be
kept sending until all the required data is obtained. This behavior
can be adjusted using multiresponce ability (getting multiple lines
of response) proposed in this patch set.

Implementation of multiresponce leads to ability to dump content of
console buffer which, with use of "oem run", allows to entirely avoid
need in UART.

---
Changes in v6:
 - remove prev console changes
 - add console_record_isempty helper
 - set record flag on init

Changes in v5:
 - restored membuff_readline behavior changed in v4

Changes in v4:
 - adjust membuff_readline behavior with overflow

Changes in v3:
 - fix missing function calls if CONFIG_CONSOLE_RECORD is not enabled

Changes in v2:
 - changed variables to static
 - fixed multiresponce for udp
 - documented use of "MORE"
 - converted #if to if (...)
---

Ion Agorria (6):
  fastboot: multiresponse support
  fastboot: implement "getvar all"
  common: console: introduce console_record_isempty helper
  common: console: record console from the beginning
  lib: membuff: fix readline not returning line in case of overflow
  fastboot: add oem console command support

 boot/bootmeth_extlinux.c          |  2 +-
 common/console.c                  | 10 +++-
 doc/android/fastboot-protocol.rst |  3 ++
 doc/android/fastboot.rst          |  1 +
 drivers/fastboot/Kconfig          |  7 +++
 drivers/fastboot/fb_command.c     | 52 +++++++++++++++++++++
 drivers/fastboot/fb_getvar.c      | 77 +++++++++++++++++++++++++------
 drivers/usb/gadget/f_fastboot.c   | 29 ++++++++++++
 include/console.h                 | 13 ++++++
 include/fastboot-internal.h       |  7 +++
 include/fastboot.h                | 19 ++++++++
 include/membuff.h                 |  5 +-
 lib/membuff.c                     |  4 +-
 net/fastboot_udp.c                | 29 +++++++++---
 14 files changed, 233 insertions(+), 25 deletions(-)

-- 
2.40.1

[PATCH v6 1/6] fastboot: multiresponse support

[Svyatoslav Ryhel]

From: Ion Agorria <ion@agorria.com>

Currently u-boot fastboot can only send one message back to host,
so if there is a need to print more than one line messages must be
kept sending until all the required data is obtained. This behavior
can be adjusted using multiresponce ability (getting multiple lines
of response) proposed in this patch.

Signed-off-by: Ion Agorria <ion@agorria.com>
Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
---
 drivers/fastboot/fb_command.c   | 10 ++++++++++
 drivers/usb/gadget/f_fastboot.c | 29 +++++++++++++++++++++++++++++
 include/fastboot.h              | 18 ++++++++++++++++++
 net/fastboot_udp.c              | 29 +++++++++++++++++++++++------
 4 files changed, 80 insertions(+), 6 deletions(-)

diff --git a/drivers/fastboot/fb_command.c b/drivers/fastboot/fb_command.c
index 5fcadcdf50..ab72d8c781 100644
--- a/drivers/fastboot/fb_command.c
+++ b/drivers/fastboot/fb_command.c
@@ -5,6 +5,7 @@
 
 #include <common.h>
 #include <command.h>
+#include <console.h>
 #include <env.h>
 #include <fastboot.h>
 #include <fastboot-internal.h>
@@ -152,6 +153,15 @@ int fastboot_handle_command(char *cmd_string, char *response)
 	return -1;
 }
 
+void fastboot_multiresponse(int cmd, char *response)
+{
+	switch (cmd) {
+	default:
+		fastboot_fail("Unknown multiresponse command", response);
+		break;
+	}
+}
+
 /**
  * okay() - Send bare OKAY response
  *
diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c
index 9f322c9550..09e740cc96 100644
--- a/drivers/usb/gadget/f_fastboot.c
+++ b/drivers/usb/gadget/f_fastboot.c
@@ -497,6 +497,25 @@ static void do_bootm_on_complete(struct usb_ep *ep, struct usb_request *req)
 	do_exit_on_complete(ep, req);
 }
 
+static int multiresponse_cmd = -1;
+static void multiresponse_on_complete(struct usb_ep *ep, struct usb_request *req)
+{
+	char response[FASTBOOT_RESPONSE_LEN] = {0};
+
+	if (multiresponse_cmd == -1)
+		return;
+
+	/* Call handler to obtain next response */
+	fastboot_multiresponse(multiresponse_cmd, response);
+	fastboot_tx_write_str(response);
+
+	/* If response is final OKAY/FAIL response disconnect this handler and unset cmd */
+	if (!strncmp("OKAY", response, 4) || !strncmp("FAIL", response, 4)) {
+		multiresponse_cmd = -1;
+		fastboot_func->in_req->complete = fastboot_complete;
+	}
+}
+
 static void do_acmd_complete(struct usb_ep *ep, struct usb_request *req)
 {
 	/* When usb dequeue complete will be called
@@ -524,6 +543,16 @@ static void rx_handler_command(struct usb_ep *ep, struct usb_request *req)
 		fastboot_fail("buffer overflow", response);
 	}
 
+	if (!strncmp(FASTBOOT_MULTIRESPONSE_START, response, 4)) {
+		multiresponse_cmd = cmd;
+		fastboot_multiresponse(multiresponse_cmd, response);
+
+		/* Only add complete callback if first is not a final OKAY/FAIL response */
+		if (strncmp("OKAY", response, 4) && strncmp("FAIL", response, 4)) {
+			fastboot_func->in_req->complete = multiresponse_on_complete;
+		}
+	}
+
 	if (!strncmp("DATA", response, 4)) {
 		req->complete = rx_handler_dl_image;
 		req->length = rx_bytes_expected(ep);
diff --git a/include/fastboot.h b/include/fastboot.h
index 296451f89d..59cbea61ec 100644
--- a/include/fastboot.h
+++ b/include/fastboot.h
@@ -14,6 +14,16 @@
 
 #define FASTBOOT_VERSION	"0.4"
 
+/*
+ * Signals u-boot fastboot code to send multiple responses by
+ * calling response generating function repeatedly until a OKAY/FAIL
+ * is generated as final response.
+ *
+ * This status code is only used internally to signal, must NOT
+ * be sent to host.
+ */
+#define FASTBOOT_MULTIRESPONSE_START	("MORE")
+
 /* The 64 defined bytes plus \0 */
 #define FASTBOOT_COMMAND_LEN	(64 + 1)
 #define FASTBOOT_RESPONSE_LEN	(64 + 1)
@@ -172,5 +182,13 @@ void fastboot_data_download(const void *fastboot_data,
  */
 void fastboot_data_complete(char *response);
 
+/**
+ * fastboot_handle_multiresponse() - Called for each response to send
+ *
+ * @cmd: Command id that requested multiresponse
+ * @response: Pointer to fastboot response buffer
+ */
+void fastboot_multiresponse(int cmd, char *response);
+
 void fastboot_acmd_complete(void);
 #endif /* _FASTBOOT_H_ */
diff --git a/net/fastboot_udp.c b/net/fastboot_udp.c
index d690787478..6fee441ab3 100644
--- a/net/fastboot_udp.c
+++ b/net/fastboot_udp.c
@@ -42,16 +42,15 @@ static int fastboot_remote_port;
 static int fastboot_our_port;
 
 /**
- * fastboot_udp_send_info() - Send an INFO packet during long commands.
+ * fastboot_udp_send_response() - Send an response into UDP
  *
- * @msg: String describing the reason for waiting
+ * @response: Response to send
  */
-static void fastboot_udp_send_info(const char *msg)
+static void fastboot_udp_send_response(const char *response)
 {
 	uchar *packet;
 	uchar *packet_base;
 	int len = 0;
-	char response[FASTBOOT_RESPONSE_LEN] = {0};
 
 	struct fastboot_header response_header = {
 		.id = FASTBOOT_FASTBOOT,
@@ -66,7 +65,6 @@ static void fastboot_udp_send_info(const char *msg)
 	memcpy(packet, &response_header, sizeof(response_header));
 	packet += sizeof(response_header);
 	/* Write response */
-	fastboot_response("INFO", response, "%s", msg);
 	memcpy(packet, response, strlen(response));
 	packet += strlen(response);
 
@@ -91,6 +89,7 @@ static void fastboot_udp_send_info(const char *msg)
 static void fastboot_timed_send_info(const char *msg)
 {
 	static ulong start;
+	char response[FASTBOOT_RESPONSE_LEN] = {0};
 
 	/* Initialize timer */
 	if (start == 0)
@@ -99,7 +98,8 @@ static void fastboot_timed_send_info(const char *msg)
 	/* Send INFO packet to host every 30 seconds */
 	if (time >= 30000) {
 		start = get_timer(0);
-		fastboot_udp_send_info(msg);
+		fastboot_response("INFO", response, "%s", msg);
+		fastboot_udp_send_response(response);
 	}
 }
 
@@ -180,6 +180,23 @@ static void fastboot_send(struct fastboot_header header, char *fastboot_data,
 		} else {
 			cmd = fastboot_handle_command(command, response);
 			pending_command = false;
+
+			if (!strncmp(FASTBOOT_MULTIRESPONSE_START, response, 4)) {
+				while (1) {
+					/* Call handler to obtain next response */
+					fastboot_multiresponse(cmd, response);
+
+					/*
+					 * Send more responses or break to send
+					 * final OKAY/FAIL response
+					 */
+					if (strncmp("OKAY", response, 4) &&
+					    strncmp("FAIL", response, 4))
+						fastboot_udp_send_response(response);
+					else
+						break;
+				}
+			}
 		}
 		/*
 		 * Sent some INFO packets, need to update sequence number in
-- 
2.40.1

[PATCH v6 2/6] fastboot: implement "getvar all"

[Svyatoslav Ryhel]

From: Ion Agorria <ion@agorria.com>

This commit implements "fastboot getvar all" listing
by iterating the existing dispatchers that don't require
parameters (as we pass NULL), uses fastboot multiresponse.

Signed-off-by: Ion Agorria <ion@agorria.com>
Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
---
 doc/android/fastboot-protocol.rst |  3 ++
 drivers/fastboot/fb_command.c     |  3 ++
 drivers/fastboot/fb_getvar.c      | 77 +++++++++++++++++++++++++------
 include/fastboot-internal.h       |  7 +++
 4 files changed, 77 insertions(+), 13 deletions(-)

diff --git a/doc/android/fastboot-protocol.rst b/doc/android/fastboot-protocol.rst
index e8cbd7f24e..8bd6d7168f 100644
--- a/doc/android/fastboot-protocol.rst
+++ b/doc/android/fastboot-protocol.rst
@@ -173,6 +173,9 @@ The various currently defined names are::
                       bootloader requiring a signature before
                       it will install or boot images.
 
+  all                 Provides all info from commands above as
+                      they were called one by one
+
 Names starting with a lowercase character are reserved by this
 specification.  OEM-specific names should not start with lowercase
 characters.
diff --git a/drivers/fastboot/fb_command.c b/drivers/fastboot/fb_command.c
index ab72d8c781..6f621df074 100644
--- a/drivers/fastboot/fb_command.c
+++ b/drivers/fastboot/fb_command.c
@@ -156,6 +156,9 @@ int fastboot_handle_command(char *cmd_string, char *response)
 void fastboot_multiresponse(int cmd, char *response)
 {
 	switch (cmd) {
+	case FASTBOOT_COMMAND_GETVAR:
+		fastboot_getvar_all(response);
+		break;
 	default:
 		fastboot_fail("Unknown multiresponse command", response);
 		break;
diff --git a/drivers/fastboot/fb_getvar.c b/drivers/fastboot/fb_getvar.c
index 8cb8ffa2c6..f65519c57b 100644
--- a/drivers/fastboot/fb_getvar.c
+++ b/drivers/fastboot/fb_getvar.c
@@ -29,53 +29,67 @@ static void getvar_is_userspace(char *var_parameter, char *response);
 
 static const struct {
 	const char *variable;
+	bool list;
 	void (*dispatch)(char *var_parameter, char *response);
 } getvar_dispatch[] = {
 	{
 		.variable = "version",
-		.dispatch = getvar_version
+		.dispatch = getvar_version,
+		.list = true,
 	}, {
 		.variable = "version-bootloader",
-		.dispatch = getvar_version_bootloader
+		.dispatch = getvar_version_bootloader,
+		.list = true
 	}, {
 		.variable = "downloadsize",
-		.dispatch = getvar_downloadsize
+		.dispatch = getvar_downloadsize,
+		.list = true
 	}, {
 		.variable = "max-download-size",
-		.dispatch = getvar_downloadsize
+		.dispatch = getvar_downloadsize,
+		.list = true
 	}, {
 		.variable = "serialno",
-		.dispatch = getvar_serialno
+		.dispatch = getvar_serialno,
+		.list = true
 	}, {
 		.variable = "version-baseband",
-		.dispatch = getvar_version_baseband
+		.dispatch = getvar_version_baseband,
+		.list = true
 	}, {
 		.variable = "product",
-		.dispatch = getvar_product
+		.dispatch = getvar_product,
+		.list = true
 	}, {
 		.variable = "platform",
-		.dispatch = getvar_platform
+		.dispatch = getvar_platform,
+		.list = true
 	}, {
 		.variable = "current-slot",
-		.dispatch = getvar_current_slot
+		.dispatch = getvar_current_slot,
+		.list = true
 #if IS_ENABLED(CONFIG_FASTBOOT_FLASH)
 	}, {
 		.variable = "has-slot",
-		.dispatch = getvar_has_slot
+		.dispatch = getvar_has_slot,
+		.list = false
 #endif
 #if IS_ENABLED(CONFIG_FASTBOOT_FLASH_MMC)
 	}, {
 		.variable = "partition-type",
-		.dispatch = getvar_partition_type
+		.dispatch = getvar_partition_type,
+		.list = false
 #endif
 #if IS_ENABLED(CONFIG_FASTBOOT_FLASH)
 	}, {
 		.variable = "partition-size",
-		.dispatch = getvar_partition_size
+		.dispatch = getvar_partition_size,
+		.list = false
 #endif
 	}, {
 		.variable = "is-userspace",
-		.dispatch = getvar_is_userspace
+		.dispatch = getvar_is_userspace,
+		.list = true
 	}
 };
 
@@ -237,6 +251,40 @@ static void getvar_is_userspace(char *var_parameter, char *response)
 	fastboot_okay("no", response);
 }
 
+static int current_all_dispatch;
+void fastboot_getvar_all(char *response)
+{
+	/*
+	 * Find a dispatch getvar that can be listed and send
+	 * it as INFO until we reach the end.
+	 */
+	while (current_all_dispatch < ARRAY_SIZE(getvar_dispatch)) {
+		if (!getvar_dispatch[current_all_dispatch].list) {
+			current_all_dispatch++;
+			continue;
+		}
+
+		char envstr[FASTBOOT_RESPONSE_LEN] = { 0 };
+
+		getvar_dispatch[current_all_dispatch].dispatch(NULL, envstr);
+
+		char *envstr_start = envstr;
+
+		if (!strncmp("OKAY", envstr, 4) || !strncmp("FAIL", envstr, 4))
+			envstr_start += 4;
+
+		fastboot_response("INFO", response, "%s: %s",
+				  getvar_dispatch[current_all_dispatch].variable,
+				  envstr_start);
+
+		current_all_dispatch++;
+		return;
+	}
+
+	fastboot_response("OKAY", response, NULL);
+	current_all_dispatch = 0;
+}
+
 /**
  * fastboot_getvar() - Writes variable indicated by cmd_parameter to response.
  *
@@ -254,6 +302,9 @@ void fastboot_getvar(char *cmd_parameter, char *response)
 {
 	if (!cmd_parameter) {
 		fastboot_fail("missing var", response);
+	} else if (!strncmp("all", cmd_parameter, 3) && strlen(cmd_parameter) == 3) {
+		current_all_dispatch = 0;
+		fastboot_response(FASTBOOT_MULTIRESPONSE_START, response, NULL);
 	} else {
 #define FASTBOOT_ENV_PREFIX	"fastboot."
 		int i;
diff --git a/include/fastboot-internal.h b/include/fastboot-internal.h
index bf2f2b3c89..610d4f9141 100644
--- a/include/fastboot-internal.h
+++ b/include/fastboot-internal.h
@@ -18,6 +18,13 @@ extern u32 fastboot_buf_size;
  */
 extern void (*fastboot_progress_callback)(const char *msg);
 
+/**
+ * fastboot_getvar_all() - Writes current variable being listed from "all" to response.
+ *
+ * @response: Pointer to fastboot response buffer
+ */
+void fastboot_getvar_all(char *response);
+
 /**
  * fastboot_getvar() - Writes variable indicated by cmd_parameter to response.
  *
-- 
2.40.1

[PATCH v6 3/6] common: console: introduce console_record_isempty helper

[Svyatoslav Ryhel]

From: Ion Agorria <ion@agorria.com>

Add console_record_isempty to check if console record buffer
contains any data.

Signed-off-by: Ion Agorria <ion@agorria.com>
Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
---
 common/console.c  |  5 +++++
 include/console.h | 13 +++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/common/console.c b/common/console.c
index 1ffda49c87..6f2089caa0 100644
--- a/common/console.c
+++ b/common/console.c
@@ -853,6 +853,11 @@ int console_record_avail(void)
 	return membuff_avail((struct membuff *)&gd->console_out);
 }
 
+bool console_record_isempty(void)
+{
+	return membuff_isempty((struct membuff *)&gd->console_out);
+}
+
 int console_in_puts(const char *str)
 {
 	return membuff_put((struct membuff *)&gd->console_in, str, strlen(str));
diff --git a/include/console.h b/include/console.h
index e29817e57b..2617e16007 100644
--- a/include/console.h
+++ b/include/console.h
@@ -84,6 +84,13 @@ int console_record_readline(char *str, int maxlen);
  */
 int console_record_avail(void);
 
+/**
+ * console_record_isempty() - Returns if console output is empty
+ *
+ * Return: true if empty
+ */
+bool console_record_isempty(void);
+
 /**
  * console_in_puts() - Write a string to the console input buffer
  *
@@ -131,6 +138,12 @@ static inline int console_in_puts(const char *str)
 	return 0;
 }
 
+static inline bool console_record_isempty(void)
+{
+	/* Always empty */
+	return true;
+}
+
 #endif /* !CONFIG_CONSOLE_RECORD */
 
 /**
-- 
2.40.1

Re: [PATCH v6 3/6] common: console: introduce console_record_isempty helper

[Simon Glass]

On Thu, Dec 28, 2023 at 6:02 PM Svyatoslav Ryhel <clamor95@gmail.com> wrote:
>
> From: Ion Agorria <ion@agorria.com>
>
> Add console_record_isempty to check if console record buffer
> contains any data.
>
> Signed-off-by: Ion Agorria <ion@agorria.com>
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
> ---
>  common/console.c  |  5 +++++
>  include/console.h | 13 +++++++++++++
>  2 files changed, 18 insertions(+)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH v6 4/6] common: console: record console from the beginning

[Svyatoslav Ryhel]

From: Ion Agorria <ion@agorria.com>

Set flag to enable console record on console_record_init
and not only on console_record_reset_enable. This fixes
missing start of U-Boot log for fastboot oem console
command.

Signed-off-by: Ion Agorria <ion@agorria.com>
Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
---
 common/console.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/common/console.c b/common/console.c
index 6f2089caa0..e6d7ebe935 100644
--- a/common/console.c
+++ b/common/console.c
@@ -821,6 +821,9 @@ int console_record_init(void)
 	ret = membuff_new((struct membuff *)&gd->console_in,
 			  CONFIG_CONSOLE_RECORD_IN_SIZE);
 
+	/* Start recording from the beginning */
+	gd->flags |= GD_FLG_RECORD;
+
 	return ret;
 }
 
-- 
2.40.1

Re: [PATCH v6 4/6] common: console: record console from the beginning

[Simon Glass]

On Thu, Dec 28, 2023 at 6:02 PM Svyatoslav Ryhel <clamor95@gmail.com> wrote:
>
> From: Ion Agorria <ion@agorria.com>
>
> Set flag to enable console record on console_record_init
> and not only on console_record_reset_enable. This fixes
> missing start of U-Boot log for fastboot oem console
> command.
>
> Signed-off-by: Ion Agorria <ion@agorria.com>
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
> ---
>  common/console.c | 3 +++
>  1 file changed, 3 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

OK, I can see the use of this...but I wonder if we can now get rid of
the same line of code from console_record_reset_enable() ?

>
> diff --git a/common/console.c b/common/console.c
> index 6f2089caa0..e6d7ebe935 100644
> --- a/common/console.c
> +++ b/common/console.c
> @@ -821,6 +821,9 @@ int console_record_init(void)
>         ret = membuff_new((struct membuff *)&gd->console_in,
>                           CONFIG_CONSOLE_RECORD_IN_SIZE);
>
> +       /* Start recording from the beginning */
> +       gd->flags |= GD_FLG_RECORD;
> +
>         return ret;
>  }
>
> --
> 2.40.1
>

Regards,
Simon

Re: [PATCH v6 4/6] common: console: record console from the beginning

[Svyatoslav Ryhel]

чт, 28 груд. 2023 р. о 21:48 Simon Glass <sjg@chromium.org> пише:
>
> On Thu, Dec 28, 2023 at 6:02 PM Svyatoslav Ryhel <clamor95@gmail.com> wrote:
> >
> > From: Ion Agorria <ion@agorria.com>
> >
> > Set flag to enable console record on console_record_init
> > and not only on console_record_reset_enable. This fixes
> > missing start of U-Boot log for fastboot oem console
> > command.
> >
> > Signed-off-by: Ion Agorria <ion@agorria.com>
> > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
> > ---
> >  common/console.c | 3 +++
> >  1 file changed, 3 insertions(+)
>
> Reviewed-by: Simon Glass <sjg@chromium.org>
>
> OK, I can see the use of this...but I wonder if we can now get rid of
> the same line of code from console_record_reset_enable() ?
>

Interesting question but let's leave it to a dedicated patch :)

Best Regards,
Svyatoslav R.

> >
> > diff --git a/common/console.c b/common/console.c
> > index 6f2089caa0..e6d7ebe935 100644
> > --- a/common/console.c
> > +++ b/common/console.c
> > @@ -821,6 +821,9 @@ int console_record_init(void)
> >         ret = membuff_new((struct membuff *)&gd->console_in,
> >                           CONFIG_CONSOLE_RECORD_IN_SIZE);
> >
> > +       /* Start recording from the beginning */
> > +       gd->flags |= GD_FLG_RECORD;
> > +
> >         return ret;
> >  }
> >
> > --
> > 2.40.1
> >
>
> Regards,
> Simon

Re: [PATCH v6 4/6] common: console: record console from the beginning

[Mattijs Korpershoek]

Hi Simon, Svyatoslav,

On Thu, Dec 28, 2023 at 21:52, Svyatoslav Ryhel <clamor95@gmail.com> wrote:

> чт, 28 груд. 2023 р. о 21:48 Simon Glass <sjg@chromium.org> пише:
>>
>> On Thu, Dec 28, 2023 at 6:02 PM Svyatoslav Ryhel <clamor95@gmail.com> wrote:
>> >
>> > From: Ion Agorria <ion@agorria.com>
>> >
>> > Set flag to enable console record on console_record_init
>> > and not only on console_record_reset_enable. This fixes
>> > missing start of U-Boot log for fastboot oem console
>> > command.
>> >
>> > Signed-off-by: Ion Agorria <ion@agorria.com>
>> > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
>> > Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
>> > ---
>> >  common/console.c | 3 +++
>> >  1 file changed, 3 insertions(+)
>>
>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>
>> OK, I can see the use of this...but I wonder if we can now get rid of
>> the same line of code from console_record_reset_enable() ?
>>
>
> Interesting question but let's leave it to a dedicated patch :)

I've looked a little more into to this, and I'm not so sure we can get
rid of the gd->flags |= GD_FLG_RECORD; in console_record_reset_enable().

Removing the flag seems to break quite some tests in
test/py/tests/test_ut.py.

The breakage can be explained that various unit tests clear the
GD_FLG_RECORD with:

	gd->flags &= ~GD_FLG_RECORD;

Therefore, I would suggest we keep the flag in
console_record_reset_enable().

>
> Best Regards,
> Svyatoslav R.
>
>> >
>> > diff --git a/common/console.c b/common/console.c
>> > index 6f2089caa0..e6d7ebe935 100644
>> > --- a/common/console.c
>> > +++ b/common/console.c
>> > @@ -821,6 +821,9 @@ int console_record_init(void)
>> >         ret = membuff_new((struct membuff *)&gd->console_in,
>> >                           CONFIG_CONSOLE_RECORD_IN_SIZE);
>> >
>> > +       /* Start recording from the beginning */
>> > +       gd->flags |= GD_FLG_RECORD;
>> > +
>> >         return ret;
>> >  }
>> >
>> > --
>> > 2.40.1
>> >
>>
>> Regards,
>> Simon

Re: [PATCH v6 4/6] common: console: record console from the beginning

[Simon Glass]

Hi Mattijs,

On Tue, Jan 2, 2024 at 2:52 AM Mattijs Korpershoek
<mkorpershoek@baylibre.com> wrote:
>
> Hi Simon, Svyatoslav,
>
> On Thu, Dec 28, 2023 at 21:52, Svyatoslav Ryhel <clamor95@gmail.com> wrote:
>
> > чт, 28 груд. 2023 р. о 21:48 Simon Glass <sjg@chromium.org> пише:
> >>
> >> On Thu, Dec 28, 2023 at 6:02 PM Svyatoslav Ryhel <clamor95@gmail.com> wrote:
> >> >
> >> > From: Ion Agorria <ion@agorria.com>
> >> >
> >> > Set flag to enable console record on console_record_init
> >> > and not only on console_record_reset_enable. This fixes
> >> > missing start of U-Boot log for fastboot oem console
> >> > command.
> >> >
> >> > Signed-off-by: Ion Agorria <ion@agorria.com>
> >> > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> >> > Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
> >> > ---
> >> >  common/console.c | 3 +++
> >> >  1 file changed, 3 insertions(+)
> >>
> >> Reviewed-by: Simon Glass <sjg@chromium.org>
> >>
> >> OK, I can see the use of this...but I wonder if we can now get rid of
> >> the same line of code from console_record_reset_enable() ?
> >>
> >
> > Interesting question but let's leave it to a dedicated patch :)
>
> I've looked a little more into to this, and I'm not so sure we can get
> rid of the gd->flags |= GD_FLG_RECORD; in console_record_reset_enable().
>
> Removing the flag seems to break quite some tests in
> test/py/tests/test_ut.py.
>
> The breakage can be explained that various unit tests clear the
> GD_FLG_RECORD with:
>
>         gd->flags &= ~GD_FLG_RECORD;
>
> Therefore, I would suggest we keep the flag in
> console_record_reset_enable().

From my look all of those are not needed in tests, i.e. are bugs. If
you are able to do a patch to remove those lines, it would avoid the
confusion.

Also, by setting UT_TESTF_CONSOLE_REC for a test, console recording is
set up automatically, so the console_record_reset_enable() is not
needed at the start of the test.

Regards,
Simon

Re: [PATCH v6 4/6] common: console: record console from the beginning

[Mattijs Korpershoek]

Hi Simon,

On Tue, Jan 02, 2024 at 07:06, Simon Glass <sjg@chromium.org> wrote:

> Hi Mattijs,
>
> On Tue, Jan 2, 2024 at 2:52 AM Mattijs Korpershoek
> <mkorpershoek@baylibre.com> wrote:
>>
>> Hi Simon, Svyatoslav,
>>
>> On Thu, Dec 28, 2023 at 21:52, Svyatoslav Ryhel <clamor95@gmail.com> wrote:
>>
>> > чт, 28 груд. 2023 р. о 21:48 Simon Glass <sjg@chromium.org> пише:
>> >>
>> >> On Thu, Dec 28, 2023 at 6:02 PM Svyatoslav Ryhel <clamor95@gmail.com> wrote:
>> >> >
>> >> > From: Ion Agorria <ion@agorria.com>
>> >> >
>> >> > Set flag to enable console record on console_record_init
>> >> > and not only on console_record_reset_enable. This fixes
>> >> > missing start of U-Boot log for fastboot oem console
>> >> > command.
>> >> >
>> >> > Signed-off-by: Ion Agorria <ion@agorria.com>
>> >> > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
>> >> > Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
>> >> > ---
>> >> >  common/console.c | 3 +++
>> >> >  1 file changed, 3 insertions(+)
>> >>
>> >> Reviewed-by: Simon Glass <sjg@chromium.org>
>> >>
>> >> OK, I can see the use of this...but I wonder if we can now get rid of
>> >> the same line of code from console_record_reset_enable() ?
>> >>
>> >
>> > Interesting question but let's leave it to a dedicated patch :)
>>
>> I've looked a little more into to this, and I'm not so sure we can get
>> rid of the gd->flags |= GD_FLG_RECORD; in console_record_reset_enable().
>>
>> Removing the flag seems to break quite some tests in
>> test/py/tests/test_ut.py.
>>
>> The breakage can be explained that various unit tests clear the
>> GD_FLG_RECORD with:
>>
>>         gd->flags &= ~GD_FLG_RECORD;
>>
>> Therefore, I would suggest we keep the flag in
>> console_record_reset_enable().
>
> From my look all of those are not needed in tests, i.e. are bugs. If
> you are able to do a patch to remove those lines, it would avoid the
> confusion.

With gd->flags |= GD_FLG_RECORD removed from
console_record_reset_enable(),

When I run:
$ ./test/py/test.py --bd sandbox --build -k ut

It produces this list of the the tests that fail:
https://paste.debian.net/1302906/

I can also reproduce individually with a bootflow test, for example:
$ ./test/py/test.py --bd sandbox --build -k ut_bootstd_bootflow_cmd_boot

Produces:
https://paste.debian.net/1302907/

I did not investigate more on detail but it seems not trivial to me.

I can continue the investigation in the coming weeks but I would like
to apply this series this week.

>
> Also, by setting UT_TESTF_CONSOLE_REC for a test, console recording is
> set up automatically, so the console_record_reset_enable() is not
> needed at the start of the test.

I was not aware of that, thank you.

>
> Regards,
> Simon

Re: [PATCH v6 4/6] common: console: record console from the beginning

[Simon Glass]

Hi Mattijs,

On Wed, Jan 3, 2024 at 5:41 AM Mattijs Korpershoek
<mkorpershoek@baylibre.com> wrote:
>
> Hi Simon,
>
> On Tue, Jan 02, 2024 at 07:06, Simon Glass <sjg@chromium.org> wrote:
>
> > Hi Mattijs,
> >
> > On Tue, Jan 2, 2024 at 2:52 AM Mattijs Korpershoek
> > <mkorpershoek@baylibre.com> wrote:
> >>
> >> Hi Simon, Svyatoslav,
> >>
> >> On Thu, Dec 28, 2023 at 21:52, Svyatoslav Ryhel <clamor95@gmail.com> wrote:
> >>
> >> > чт, 28 груд. 2023 р. о 21:48 Simon Glass <sjg@chromium.org> пише:
> >> >>
> >> >> On Thu, Dec 28, 2023 at 6:02 PM Svyatoslav Ryhel <clamor95@gmail.com> wrote:
> >> >> >
> >> >> > From: Ion Agorria <ion@agorria.com>
> >> >> >
> >> >> > Set flag to enable console record on console_record_init
> >> >> > and not only on console_record_reset_enable. This fixes
> >> >> > missing start of U-Boot log for fastboot oem console
> >> >> > command.
> >> >> >
> >> >> > Signed-off-by: Ion Agorria <ion@agorria.com>
> >> >> > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> >> >> > Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
> >> >> > ---
> >> >> >  common/console.c | 3 +++
> >> >> >  1 file changed, 3 insertions(+)
> >> >>
> >> >> Reviewed-by: Simon Glass <sjg@chromium.org>
> >> >>
> >> >> OK, I can see the use of this...but I wonder if we can now get rid of
> >> >> the same line of code from console_record_reset_enable() ?
> >> >>
> >> >
> >> > Interesting question but let's leave it to a dedicated patch :)
> >>
> >> I've looked a little more into to this, and I'm not so sure we can get
> >> rid of the gd->flags |= GD_FLG_RECORD; in console_record_reset_enable().
> >>
> >> Removing the flag seems to break quite some tests in
> >> test/py/tests/test_ut.py.
> >>
> >> The breakage can be explained that various unit tests clear the
> >> GD_FLG_RECORD with:
> >>
> >>         gd->flags &= ~GD_FLG_RECORD;
> >>
> >> Therefore, I would suggest we keep the flag in
> >> console_record_reset_enable().
> >
> > From my look all of those are not needed in tests, i.e. are bugs. If
> > you are able to do a patch to remove those lines, it would avoid the
> > confusion.
>
> With gd->flags |= GD_FLG_RECORD removed from
> console_record_reset_enable(),
>
> When I run:
> $ ./test/py/test.py --bd sandbox --build -k ut
>
> It produces this list of the the tests that fail:
> https://paste.debian.net/1302906/
>
> I can also reproduce individually with a bootflow test, for example:
> $ ./test/py/test.py --bd sandbox --build -k ut_bootstd_bootflow_cmd_boot
>
> Produces:
> https://paste.debian.net/1302907/
>
> I did not investigate more on detail but it seems not trivial to me.

Did you add UT_TESTF_CONSOLE_REC to each test as well?


>
> I can continue the investigation in the coming weeks but I would like
> to apply this series this week.
>
> >
> > Also, by setting UT_TESTF_CONSOLE_REC for a test, console recording is
> > set up automatically, so the console_record_reset_enable() is not
> > needed at the start of the test.
>
> I was not aware of that, thank you.
>
> >
> > Regards,
> > Simon

Re: [PATCH v6 4/6] common: console: record console from the beginning

[Mattijs Korpershoek]

Hi Simon,

On mer., janv. 03, 2024 at 18:38, Simon Glass <sjg@chromium.org> wrote:

> Hi Mattijs,
>
> On Wed, Jan 3, 2024 at 5:41 AM Mattijs Korpershoek
> <mkorpershoek@baylibre.com> wrote:
>>
>> Hi Simon,
>>
>> On Tue, Jan 02, 2024 at 07:06, Simon Glass <sjg@chromium.org> wrote:
>>
>> > Hi Mattijs,
>> >
>> > On Tue, Jan 2, 2024 at 2:52 AM Mattijs Korpershoek
>> > <mkorpershoek@baylibre.com> wrote:
>> >>
>> >> Hi Simon, Svyatoslav,
>> >>
>> >> On Thu, Dec 28, 2023 at 21:52, Svyatoslav Ryhel <clamor95@gmail.com> wrote:
>> >>
>> >> > чт, 28 груд. 2023 р. о 21:48 Simon Glass <sjg@chromium.org> пише:
>> >> >>
>> >> >> On Thu, Dec 28, 2023 at 6:02 PM Svyatoslav Ryhel <clamor95@gmail.com> wrote:
>> >> >> >
>> >> >> > From: Ion Agorria <ion@agorria.com>
>> >> >> >
>> >> >> > Set flag to enable console record on console_record_init
>> >> >> > and not only on console_record_reset_enable. This fixes
>> >> >> > missing start of U-Boot log for fastboot oem console
>> >> >> > command.
>> >> >> >
>> >> >> > Signed-off-by: Ion Agorria <ion@agorria.com>
>> >> >> > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
>> >> >> > Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
>> >> >> > ---
>> >> >> >  common/console.c | 3 +++
>> >> >> >  1 file changed, 3 insertions(+)
>> >> >>
>> >> >> Reviewed-by: Simon Glass <sjg@chromium.org>
>> >> >>
>> >> >> OK, I can see the use of this...but I wonder if we can now get rid of
>> >> >> the same line of code from console_record_reset_enable() ?
>> >> >>
>> >> >
>> >> > Interesting question but let's leave it to a dedicated patch :)
>> >>
>> >> I've looked a little more into to this, and I'm not so sure we can get
>> >> rid of the gd->flags |= GD_FLG_RECORD; in console_record_reset_enable().
>> >>
>> >> Removing the flag seems to break quite some tests in
>> >> test/py/tests/test_ut.py.
>> >>
>> >> The breakage can be explained that various unit tests clear the
>> >> GD_FLG_RECORD with:
>> >>
>> >>         gd->flags &= ~GD_FLG_RECORD;
>> >>
>> >> Therefore, I would suggest we keep the flag in
>> >> console_record_reset_enable().
>> >
>> > From my look all of those are not needed in tests, i.e. are bugs. If
>> > you are able to do a patch to remove those lines, it would avoid the
>> > confusion.
>>
>> With gd->flags |= GD_FLG_RECORD removed from
>> console_record_reset_enable(),
>>
>> When I run:
>> $ ./test/py/test.py --bd sandbox --build -k ut
>>
>> It produces this list of the the tests that fail:
>> https://paste.debian.net/1302906/
>>
>> I can also reproduce individually with a bootflow test, for example:
>> $ ./test/py/test.py --bd sandbox --build -k ut_bootstd_bootflow_cmd_boot
>>
>> Produces:
>> https://paste.debian.net/1302907/
>>
>> I did not investigate more on detail but it seems not trivial to me.
>
> Did you add UT_TESTF_CONSOLE_REC to each test as well?

Here are the steps I did:
1. Take base commit dffa6d0210f5 ("Merge tag 'dm-next-1124' of https://source.denx.de/u-boot/custodians/u-boot-dm into next")
2. Apply series: b4 shazam 20231228180154.50473-1-clamor95@gmail.com
3. Run bootflow_cmd_boot test, which passes.
4. Apply the following diff:

diff --git a/common/console.c b/common/console.c
index cad65891fc9f..1bff80029266 100644
--- a/common/console.c
+++ b/common/console.c
@@ -837,7 +837,6 @@ void console_record_reset(void)
 int console_record_reset_enable(void)
 {
        console_record_reset();
-       gd->flags |= GD_FLG_RECORD;
 
        return 0;
 }
diff --git a/test/boot/bootflow.c b/test/boot/bootflow.c
index 104f49deef2a..6a98f42f2707 100644
--- a/test/boot/bootflow.c
+++ b/test/boot/bootflow.c
@@ -482,7 +482,6 @@ BOOTSTD_TEST(bootflow_scan_glob_bootmeth, UT_TESTF_DM | UT_TESTF_SCAN_FDT);
 /* Check 'bootflow boot' to boot a selected bootflow */
 static int bootflow_cmd_boot(struct unit_test_state *uts)
 {
-       console_record_reset_enable();
        ut_assertok(run_command("bootdev select 1", 0));
        ut_assert_console_end();
        ut_assertok(run_command("bootflow scan", 0));
@@ -506,7 +505,7 @@ static int bootflow_cmd_boot(struct unit_test_state *uts)
 
        return 0;
 }
-BOOTSTD_TEST(bootflow_cmd_boot, UT_TESTF_DM | UT_TESTF_SCAN_FDT);
+BOOTSTD_TEST(bootflow_cmd_boot, UT_TESTF_DM | UT_TESTF_SCAN_FDT | UT_TESTF_CONSOLE_REC);
 
 /**
  * prep_mmc_bootdev() - Set up an mmc bootdev so we can access other distros

5. Run bootflow_cmd_boot test, which now fails.

>
>
>>
>> I can continue the investigation in the coming weeks but I would like
>> to apply this series this week.
>>
>> >
>> > Also, by setting UT_TESTF_CONSOLE_REC for a test, console recording is
>> > set up automatically, so the console_record_reset_enable() is not
>> > needed at the start of the test.
>>
>> I was not aware of that, thank you.
>>
>> >
>> > Regards,
>> > Simon

[PATCH v6 5/6] lib: membuff: fix readline not returning line in case of overflow

[Svyatoslav Ryhel]

From: Ion Agorria <ion@agorria.com>

If line overflows readline it will not be returned, fix this behavior,
make it optional and documented properly.

Signed-off-by: Ion Agorria <ion@agorria.com>
Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
 boot/bootmeth_extlinux.c | 2 +-
 common/console.c         | 2 +-
 include/membuff.h        | 5 +++--
 lib/membuff.c            | 4 ++--
 4 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/boot/bootmeth_extlinux.c b/boot/bootmeth_extlinux.c
index aa2a4591eb..ae0ad1d53e 100644
--- a/boot/bootmeth_extlinux.c
+++ b/boot/bootmeth_extlinux.c
@@ -82,7 +82,7 @@ static int extlinux_fill_info(struct bootflow *bflow)
 	log_debug("parsing bflow file size %x\n", bflow->size);
 	membuff_init(&mb, bflow->buf, bflow->size);
 	membuff_putraw(&mb, bflow->size, true, &data);
-	while (len = membuff_readline(&mb, line, sizeof(line) - 1, ' '), len) {
+	while (len = membuff_readline(&mb, line, sizeof(line) - 1, ' ', true), len) {
 		char *tok, *p = line;
 
 		tok = strsep(&p, " ");
diff --git a/common/console.c b/common/console.c
index e6d7ebe935..cad65891fc 100644
--- a/common/console.c
+++ b/common/console.c
@@ -848,7 +848,7 @@ int console_record_readline(char *str, int maxlen)
 		return -ENOSPC;
 
 	return membuff_readline((struct membuff *)&gd->console_out, str,
-				maxlen, '\0');
+				maxlen, '\0', false);
 }
 
 int console_record_avail(void)
diff --git a/include/membuff.h b/include/membuff.h
index 21051b0c54..4eba626ce1 100644
--- a/include/membuff.h
+++ b/include/membuff.h
@@ -192,10 +192,11 @@ int membuff_free(struct membuff *mb);
  * @mb: membuff to adjust
  * @str: Place to put the line
  * @maxlen: Maximum line length (excluding terminator)
+ * @must_fit: If true then str is empty if line doesn't fit
  * Return: number of bytes read (including terminator) if a line has been
- *	   read, 0 if nothing was there
+ *	   read, 0 if nothing was there or line didn't fit when must_fit is set
  */
-int membuff_readline(struct membuff *mb, char *str, int maxlen, int minch);
+int membuff_readline(struct membuff *mb, char *str, int maxlen, int minch, bool must_fit);
 
 /**
  * membuff_extend_by() - expand a membuff
diff --git a/lib/membuff.c b/lib/membuff.c
index 3c6c0ae125..b242a38ff1 100644
--- a/lib/membuff.c
+++ b/lib/membuff.c
@@ -287,7 +287,7 @@ int membuff_free(struct membuff *mb)
 			(mb->end - mb->start) - 1 - membuff_avail(mb);
 }
 
-int membuff_readline(struct membuff *mb, char *str, int maxlen, int minch)
+int membuff_readline(struct membuff *mb, char *str, int maxlen, int minch, bool must_fit)
 {
 	int len;  /* number of bytes read (!= string length) */
 	char *s, *end;
@@ -309,7 +309,7 @@ int membuff_readline(struct membuff *mb, char *str, int maxlen, int minch)
 	}
 
 	/* couldn't get the whole string */
-	if (!ok) {
+	if (!ok && must_fit) {
 		if (maxlen)
 			*orig = '\0';
 		return 0;
-- 
2.40.1

[PATCH v6 6/6] fastboot: add oem console command support

[Svyatoslav Ryhel]

From: Ion Agorria <ion@agorria.com>

"oem console" serves to read console record buffer.

Signed-off-by: Ion Agorria <ion@agorria.com>
Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
---
 doc/android/fastboot.rst      |  1 +
 drivers/fastboot/Kconfig      |  7 +++++++
 drivers/fastboot/fb_command.c | 39 +++++++++++++++++++++++++++++++++++
 include/fastboot.h            |  1 +
 4 files changed, 48 insertions(+)

diff --git a/doc/android/fastboot.rst b/doc/android/fastboot.rst
index 1ad8a897c8..05d8f77759 100644
--- a/doc/android/fastboot.rst
+++ b/doc/android/fastboot.rst
@@ -29,6 +29,7 @@ The following OEM commands are supported (if enabled):
   with <arg> = boot_ack boot_partition
 - ``oem bootbus``  - this executes ``mmc bootbus %x %s`` to configure eMMC
 - ``oem run`` - this executes an arbitrary U-Boot command
+- ``oem console`` - this dumps U-Boot console record buffer
 
 Support for both eMMC and NAND devices is included.
 
diff --git a/drivers/fastboot/Kconfig b/drivers/fastboot/Kconfig
index 11fc0fe1c8..5e5855a76c 100644
--- a/drivers/fastboot/Kconfig
+++ b/drivers/fastboot/Kconfig
@@ -242,6 +242,13 @@ config FASTBOOT_OEM_RUN
 	  this feature if you are using verified boot, as it will allow an
 	  attacker to bypass any restrictions you have in place.
 
+config FASTBOOT_CMD_OEM_CONSOLE
+	bool "Enable the 'oem console' command"
+	depends on CONSOLE_RECORD
+	help
+	  Add support for the "oem console" command to input and read console
+	  record buffer.
+
 endif # FASTBOOT
 
 endmenu
diff --git a/drivers/fastboot/fb_command.c b/drivers/fastboot/fb_command.c
index 6f621df074..f95f4e4ae1 100644
--- a/drivers/fastboot/fb_command.c
+++ b/drivers/fastboot/fb_command.c
@@ -41,6 +41,7 @@ static void reboot_recovery(char *, char *);
 static void oem_format(char *, char *);
 static void oem_partconf(char *, char *);
 static void oem_bootbus(char *, char *);
+static void oem_console(char *, char *);
 static void run_ucmd(char *, char *);
 static void run_acmd(char *, char *);
 
@@ -108,6 +109,10 @@ static const struct {
 		.command = "oem run",
 		.dispatch = CONFIG_IS_ENABLED(FASTBOOT_OEM_RUN, (run_ucmd), (NULL))
 	},
+	[FASTBOOT_COMMAND_OEM_CONSOLE] = {
+		.command = "oem console",
+		.dispatch = CONFIG_IS_ENABLED(FASTBOOT_CMD_OEM_CONSOLE, (oem_console), (NULL))
+	},
 	[FASTBOOT_COMMAND_UCMD] = {
 		.command = "UCmd",
 		.dispatch = CONFIG_IS_ENABLED(FASTBOOT_UUU_SUPPORT, (run_ucmd), (NULL))
@@ -159,6 +164,23 @@ void fastboot_multiresponse(int cmd, char *response)
 	case FASTBOOT_COMMAND_GETVAR:
 		fastboot_getvar_all(response);
 		break;
+	case FASTBOOT_COMMAND_OEM_CONSOLE:
+		if (CONFIG_IS_ENABLED(FASTBOOT_CMD_OEM_CONSOLE)) {
+			char buf[FASTBOOT_RESPONSE_LEN] = { 0 };
+
+			if (console_record_isempty()) {
+				console_record_reset();
+				fastboot_okay(NULL, response);
+			} else {
+				int ret = console_record_readline(buf, sizeof(buf) - 5);
+
+				if (ret < 0)
+					fastboot_fail("Error reading console", response);
+				else
+					fastboot_response("INFO", response, "%s", buf);
+			}
+			break;
+		}
 	default:
 		fastboot_fail("Unknown multiresponse command", response);
 		break;
@@ -503,3 +525,20 @@ static void __maybe_unused oem_bootbus(char *cmd_parameter, char *response)
 	else
 		fastboot_okay(NULL, response);
 }
+
+/**
+ * oem_console() - Execute the OEM console command
+ *
+ * @cmd_parameter: Pointer to command parameter
+ * @response: Pointer to fastboot response buffer
+ */
+static void __maybe_unused oem_console(char *cmd_parameter, char *response)
+{
+	if (cmd_parameter)
+		console_in_puts(cmd_parameter);
+
+	if (console_record_isempty())
+		fastboot_fail("Empty console", response);
+	else
+		fastboot_response(FASTBOOT_MULTIRESPONSE_START, response, NULL);
+}
diff --git a/include/fastboot.h b/include/fastboot.h
index 59cbea61ec..1e7920eb91 100644
--- a/include/fastboot.h
+++ b/include/fastboot.h
@@ -47,6 +47,7 @@ enum {
 	FASTBOOT_COMMAND_OEM_PARTCONF,
 	FASTBOOT_COMMAND_OEM_BOOTBUS,
 	FASTBOOT_COMMAND_OEM_RUN,
+	FASTBOOT_COMMAND_OEM_CONSOLE,
 	FASTBOOT_COMMAND_ACMD,
 	FASTBOOT_COMMAND_UCMD,
 	FASTBOOT_COMMAND_COUNT
-- 
2.40.1

Re: [PATCH v6 0/6] Implement fastboot multiresponce

[Mattijs Korpershoek]

Hello Svyatoslav,

On jeu., déc. 28, 2023 at 20:01, Svyatoslav Ryhel <clamor95@gmail.com> wrote:

> Currently u-boot fastboot can only send one message back to host,
> so if there is a need to print more than one line messages must be
> kept sending until all the required data is obtained. This behavior
> can be adjusted using multiresponce ability (getting multiple lines
> of response) proposed in this patch set.
>
> Implementation of multiresponce leads to ability to dump content of
> console buffer which, with use of "oem run", allows to entirely avoid
> need in UART.
>
> ---
> Changes in v6:
>  - remove prev console changes
>  - add console_record_isempty helper
>  - set record flag on init
>
> Changes in v5:
>  - restored membuff_readline behavior changed in v4
>
> Changes in v4:
>  - adjust membuff_readline behavior with overflow
>
> Changes in v3:
>  - fix missing function calls if CONFIG_CONSOLE_RECORD is not enabled
>
> Changes in v2:
>  - changed variables to static
>  - fixed multiresponce for udp
>  - documented use of "MORE"
>  - converted #if to if (...)
> ---
>
> Ion Agorria (6):
>   fastboot: multiresponse support
>   fastboot: implement "getvar all"
>   common: console: introduce console_record_isempty helper
>   common: console: record console from the beginning
>   lib: membuff: fix readline not returning line in case of overflow
>   fastboot: add oem console command support

I went on to apply the series, however CI detected a regression in the
unit tests:

https://source.denx.de/u-boot/custodians/u-boot-dfu/-/jobs/764396

This can be reproduced with:
$ ./test/py/test.py --bd sandbox --build -k ut_hush_hush_test_simple_dollar

I've narrowed this down to:
 [PATCH v6 5/6] lib: membuff: fix readline not returning line in case of overflow

Could you please have a look?

Thank you!

>
>  boot/bootmeth_extlinux.c          |  2 +-
>  common/console.c                  | 10 +++-
>  doc/android/fastboot-protocol.rst |  3 ++
>  doc/android/fastboot.rst          |  1 +
>  drivers/fastboot/Kconfig          |  7 +++
>  drivers/fastboot/fb_command.c     | 52 +++++++++++++++++++++
>  drivers/fastboot/fb_getvar.c      | 77 +++++++++++++++++++++++++------
>  drivers/usb/gadget/f_fastboot.c   | 29 ++++++++++++
>  include/console.h                 | 13 ++++++
>  include/fastboot-internal.h       |  7 +++
>  include/fastboot.h                | 19 ++++++++
>  include/membuff.h                 |  5 +-
>  lib/membuff.c                     |  4 +-
>  net/fastboot_udp.c                | 29 +++++++++---
>  14 files changed, 233 insertions(+), 25 deletions(-)
>
> -- 
> 2.40.1

Re: [PATCH v6 0/6] Implement fastboot multiresponce

[Svyatoslav Ryhel]

чт, 4 січ. 2024 р. о 17:00 Mattijs Korpershoek <mkorpershoek@baylibre.com> пише:
>
> Hello Svyatoslav,
>
> On jeu., déc. 28, 2023 at 20:01, Svyatoslav Ryhel <clamor95@gmail.com> wrote:
>
> > Currently u-boot fastboot can only send one message back to host,
> > so if there is a need to print more than one line messages must be
> > kept sending until all the required data is obtained. This behavior
> > can be adjusted using multiresponce ability (getting multiple lines
> > of response) proposed in this patch set.
> >
> > Implementation of multiresponce leads to ability to dump content of
> > console buffer which, with use of "oem run", allows to entirely avoid
> > need in UART.
> >
> > ---
> > Changes in v6:
> >  - remove prev console changes
> >  - add console_record_isempty helper
> >  - set record flag on init
> >
> > Changes in v5:
> >  - restored membuff_readline behavior changed in v4
> >
> > Changes in v4:
> >  - adjust membuff_readline behavior with overflow
> >
> > Changes in v3:
> >  - fix missing function calls if CONFIG_CONSOLE_RECORD is not enabled
> >
> > Changes in v2:
> >  - changed variables to static
> >  - fixed multiresponce for udp
> >  - documented use of "MORE"
> >  - converted #if to if (...)
> > ---
> >
> > Ion Agorria (6):
> >   fastboot: multiresponse support
> >   fastboot: implement "getvar all"
> >   common: console: introduce console_record_isempty helper
> >   common: console: record console from the beginning
> >   lib: membuff: fix readline not returning line in case of overflow
> >   fastboot: add oem console command support
>
> I went on to apply the series, however CI detected a regression in the
> unit tests:
>
> https://source.denx.de/u-boot/custodians/u-boot-dfu/-/jobs/764396
>
> This can be reproduced with:
> $ ./test/py/test.py --bd sandbox --build -k ut_hush_hush_test_simple_dollar
>
> I've narrowed this down to:
>  [PATCH v6 5/6] lib: membuff: fix readline not returning line in case of overflow
>
> Could you please have a look?
>
> Thank you!
>

Test contains 2 skiplines, commenting one fixes test

ut_asserteq(1, run_command("dollar_foo=bar quux\"", 0));
        /* Two next lines contain error message */
        ut_assert_skipline();
        ut_assert_skipline();

> >
> >  boot/bootmeth_extlinux.c          |  2 +-
> >  common/console.c                  | 10 +++-
> >  doc/android/fastboot-protocol.rst |  3 ++
> >  doc/android/fastboot.rst          |  1 +
> >  drivers/fastboot/Kconfig          |  7 +++
> >  drivers/fastboot/fb_command.c     | 52 +++++++++++++++++++++
> >  drivers/fastboot/fb_getvar.c      | 77 +++++++++++++++++++++++++------
> >  drivers/usb/gadget/f_fastboot.c   | 29 ++++++++++++
> >  include/console.h                 | 13 ++++++
> >  include/fastboot-internal.h       |  7 +++
> >  include/fastboot.h                | 19 ++++++++
> >  include/membuff.h                 |  5 +-
> >  lib/membuff.c                     |  4 +-
> >  net/fastboot_udp.c                | 29 +++++++++---
> >  14 files changed, 233 insertions(+), 25 deletions(-)
> >
> > --
> > 2.40.1

Re: [PATCH v6 0/6] Implement fastboot multiresponce

[Ion Agorria]

Hello,

It seems like without the fix the ut_assert_skipline(); didn't clear
console and running ut_assert_skipline(); many times would give always
OK. With my fix the line is cleared correctly and next assert fails
because now there is nothing to clean which is correct if we look the
this a bit above the failing assert:

       if (gd->flags & GD_FLG_HUSH_MODERN_PARSER) {
               /*
                * For some strange reasons, the console is not empty after
                * running above command.
                * So, we reset it to not have side effects for other tests.
                */
               console_record_reset_enable();
       } else if (gd->flags & GD_FLG_HUSH_OLD_PARSER) {
               ut_assert_console_end();
       }

Which further confirms that tests workaround the old problem and now
that problem is fixed we can remove the whole if blocks and simply
place ut_assert_console_end() right after ut_assert_skipline() without
any conditional and will pass green.

So this part of code goes from:
       ut_assert_skipline();
       ut_assert_skipline();

       if (gd->flags & GD_FLG_HUSH_MODERN_PARSER) {
               /* See above comments. */
               console_record_reset_enable();
       } else if (gd->flags & GD_FLG_HUSH_OLD_PARSER) {
               ut_assert_console_end();
       }

to become:
       ut_assert_skipline();
       ut_assert_console_end();

Same thing should be done with the if block mentioned at start of
email that calls console_record_reset_enable().

Regards,
Ion

El jue, 4 ene 2024 a las 17:15, Svyatoslav Ryhel
(<clamor95@gmail.com>) escribió:
>
> чт, 4 січ. 2024 р. о 17:00 Mattijs Korpershoek <mkorpershoek@baylibre.com> пише:
> >
> > Hello Svyatoslav,
> >
> > On jeu., déc. 28, 2023 at 20:01, Svyatoslav Ryhel <clamor95@gmail.com> wrote:
> >
> > > Currently u-boot fastboot can only send one message back to host,
> > > so if there is a need to print more than one line messages must be
> > > kept sending until all the required data is obtained. This behavior
> > > can be adjusted using multiresponce ability (getting multiple lines
> > > of response) proposed in this patch set.
> > >
> > > Implementation of multiresponce leads to ability to dump content of
> > > console buffer which, with use of "oem run", allows to entirely avoid
> > > need in UART.
> > >
> > > ---
> > > Changes in v6:
> > >  - remove prev console changes
> > >  - add console_record_isempty helper
> > >  - set record flag on init
> > >
> > > Changes in v5:
> > >  - restored membuff_readline behavior changed in v4
> > >
> > > Changes in v4:
> > >  - adjust membuff_readline behavior with overflow
> > >
> > > Changes in v3:
> > >  - fix missing function calls if CONFIG_CONSOLE_RECORD is not enabled
> > >
> > > Changes in v2:
> > >  - changed variables to static
> > >  - fixed multiresponce for udp
> > >  - documented use of "MORE"
> > >  - converted #if to if (...)
> > > ---
> > >
> > > Ion Agorria (6):
> > >   fastboot: multiresponse support
> > >   fastboot: implement "getvar all"
> > >   common: console: introduce console_record_isempty helper
> > >   common: console: record console from the beginning
> > >   lib: membuff: fix readline not returning line in case of overflow
> > >   fastboot: add oem console command support
> >
> > I went on to apply the series, however CI detected a regression in the
> > unit tests:
> >
> > https://source.denx.de/u-boot/custodians/u-boot-dfu/-/jobs/764396
> >
> > This can be reproduced with:
> > $ ./test/py/test.py --bd sandbox --build -k ut_hush_hush_test_simple_dollar
> >
> > I've narrowed this down to:
> >  [PATCH v6 5/6] lib: membuff: fix readline not returning line in case of overflow
> >
> > Could you please have a look?
> >
> > Thank you!
> >
>
> Test contains 2 skiplines, commenting one fixes test
>
> ut_asserteq(1, run_command("dollar_foo=bar quux\"", 0));
>         /* Two next lines contain error message */
>         ut_assert_skipline();
>         ut_assert_skipline();
>
> > >
> > >  boot/bootmeth_extlinux.c          |  2 +-
> > >  common/console.c                  | 10 +++-
> > >  doc/android/fastboot-protocol.rst |  3 ++
> > >  doc/android/fastboot.rst          |  1 +
> > >  drivers/fastboot/Kconfig          |  7 +++
> > >  drivers/fastboot/fb_command.c     | 52 +++++++++++++++++++++
> > >  drivers/fastboot/fb_getvar.c      | 77 +++++++++++++++++++++++++------
> > >  drivers/usb/gadget/f_fastboot.c   | 29 ++++++++++++
> > >  include/console.h                 | 13 ++++++
> > >  include/fastboot-internal.h       |  7 +++
> > >  include/fastboot.h                | 19 ++++++++
> > >  include/membuff.h                 |  5 +-
> > >  lib/membuff.c                     |  4 +-
> > >  net/fastboot_udp.c                | 29 +++++++++---
> > >  14 files changed, 233 insertions(+), 25 deletions(-)
> > >
> > > --
> > > 2.40.1

Re: [PATCH v6 0/6] Implement fastboot multiresponce

[Mattijs Korpershoek]

Hi Ion,

On jeu., janv. 04, 2024 at 17:44, Ion Agorria <ionpl9@gmail.com> wrote:

> Hello,
>
> It seems like without the fix the ut_assert_skipline(); didn't clear
> console and running ut_assert_skipline(); many times would give always
> OK. With my fix the line is cleared correctly and next assert fails
> because now there is nothing to clean which is correct if we look the
> this a bit above the failing assert:
>
>        if (gd->flags & GD_FLG_HUSH_MODERN_PARSER) {
>                /*
>                 * For some strange reasons, the console is not empty after
>                 * running above command.
>                 * So, we reset it to not have side effects for other tests.
>                 */
>                console_record_reset_enable();
>        } else if (gd->flags & GD_FLG_HUSH_OLD_PARSER) {
>                ut_assert_console_end();
>        }
>
> Which further confirms that tests workaround the old problem and now
> that problem is fixed we can remove the whole if blocks and simply
> place ut_assert_console_end() right after ut_assert_skipline() without
> any conditional and will pass green.
>
> So this part of code goes from:
>        ut_assert_skipline();
>        ut_assert_skipline();
>
>        if (gd->flags & GD_FLG_HUSH_MODERN_PARSER) {
>                /* See above comments. */
>                console_record_reset_enable();
>        } else if (gd->flags & GD_FLG_HUSH_OLD_PARSER) {
>                ut_assert_console_end();
>        }
>
> to become:
>        ut_assert_skipline();
>        ut_assert_console_end();
>
> Same thing should be done with the if block mentioned at start of
> email that calls console_record_reset_enable().

That makes sense. Thank you for looking into this. I see that Svyatoslav
included your suggestion in

https://patchwork.ozlabs.org/project/uboot/patch/20240105072212.6615-8-clamor95@gmail.com/

I will review it there.

>
> Regards,
> Ion
>
> El jue, 4 ene 2024 a las 17:15, Svyatoslav Ryhel
> (<clamor95@gmail.com>) escribió:
>>
>> чт, 4 січ. 2024 р. о 17:00 Mattijs Korpershoek <mkorpershoek@baylibre.com> пише:
>> >
>> > Hello Svyatoslav,
>> >
>> > On jeu., déc. 28, 2023 at 20:01, Svyatoslav Ryhel <clamor95@gmail.com> wrote:
>> >
>> > > Currently u-boot fastboot can only send one message back to host,
>> > > so if there is a need to print more than one line messages must be
>> > > kept sending until all the required data is obtained. This behavior
>> > > can be adjusted using multiresponce ability (getting multiple lines
>> > > of response) proposed in this patch set.
>> > >
>> > > Implementation of multiresponce leads to ability to dump content of
>> > > console buffer which, with use of "oem run", allows to entirely avoid
>> > > need in UART.
>> > >
>> > > ---
>> > > Changes in v6:
>> > >  - remove prev console changes
>> > >  - add console_record_isempty helper
>> > >  - set record flag on init
>> > >
>> > > Changes in v5:
>> > >  - restored membuff_readline behavior changed in v4
>> > >
>> > > Changes in v4:
>> > >  - adjust membuff_readline behavior with overflow
>> > >
>> > > Changes in v3:
>> > >  - fix missing function calls if CONFIG_CONSOLE_RECORD is not enabled
>> > >
>> > > Changes in v2:
>> > >  - changed variables to static
>> > >  - fixed multiresponce for udp
>> > >  - documented use of "MORE"
>> > >  - converted #if to if (...)
>> > > ---
>> > >
>> > > Ion Agorria (6):
>> > >   fastboot: multiresponse support
>> > >   fastboot: implement "getvar all"
>> > >   common: console: introduce console_record_isempty helper
>> > >   common: console: record console from the beginning
>> > >   lib: membuff: fix readline not returning line in case of overflow
>> > >   fastboot: add oem console command support
>> >
>> > I went on to apply the series, however CI detected a regression in the
>> > unit tests:
>> >
>> > https://source.denx.de/u-boot/custodians/u-boot-dfu/-/jobs/764396
>> >
>> > This can be reproduced with:
>> > $ ./test/py/test.py --bd sandbox --build -k ut_hush_hush_test_simple_dollar
>> >
>> > I've narrowed this down to:
>> >  [PATCH v6 5/6] lib: membuff: fix readline not returning line in case of overflow
>> >
>> > Could you please have a look?
>> >
>> > Thank you!
>> >
>>
>> Test contains 2 skiplines, commenting one fixes test
>>
>> ut_asserteq(1, run_command("dollar_foo=bar quux\"", 0));
>>         /* Two next lines contain error message */
>>         ut_assert_skipline();
>>         ut_assert_skipline();
>>
>> > >
>> > >  boot/bootmeth_extlinux.c          |  2 +-
>> > >  common/console.c                  | 10 +++-
>> > >  doc/android/fastboot-protocol.rst |  3 ++
>> > >  doc/android/fastboot.rst          |  1 +
>> > >  drivers/fastboot/Kconfig          |  7 +++
>> > >  drivers/fastboot/fb_command.c     | 52 +++++++++++++++++++++
>> > >  drivers/fastboot/fb_getvar.c      | 77 +++++++++++++++++++++++++------
>> > >  drivers/usb/gadget/f_fastboot.c   | 29 ++++++++++++
>> > >  include/console.h                 | 13 ++++++
>> > >  include/fastboot-internal.h       |  7 +++
>> > >  include/fastboot.h                | 19 ++++++++
>> > >  include/membuff.h                 |  5 +-
>> > >  lib/membuff.c                     |  4 +-
>> > >  net/fastboot_udp.c                | 29 +++++++++---
>> > >  14 files changed, 233 insertions(+), 25 deletions(-)
>> > >
>> > > --
>> > > 2.40.1