[PATCH v1 0/5] Implement fastboot multiresponce

[PATCH v1 0/5] 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.

Ion Agorria (5):
  fastboot: multiresponse support
  fastboot: implement "getvar all"
  commonn: console: introduce overflow and isempty calls
  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                  | 17 +++++--
 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      | 75 +++++++++++++++++++++++++------
 drivers/usb/gadget/f_fastboot.c   | 29 ++++++++++++
 include/console.h                 | 14 ++++++
 include/fastboot-internal.h       |  7 +++
 include/fastboot.h                |  9 ++++
 include/membuff.h                 |  5 ++-
 lib/membuff.c                     |  4 +-
 net/fastboot_udp.c                | 25 ++++++++---
 test/ut.c                         |  9 ++--
 15 files changed, 226 insertions(+), 33 deletions(-)

-- 
2.40.1

[PATCH v1 1/5] 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>
---
 drivers/fastboot/fb_command.c   | 10 ++++++++++
 drivers/usb/gadget/f_fastboot.c | 29 +++++++++++++++++++++++++++++
 include/fastboot.h              |  8 ++++++++
 net/fastboot_udp.c              | 25 +++++++++++++++++++------
 4 files changed, 66 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 741775a7bc..7d8c22b948 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);
 }
 
+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 not an INFO disconnect this handler and unset cmd */
+	if (strncmp("INFO", response, 4) != 0) {
+		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("MORE", response, 4)) {
+		multiresponse_cmd = cmd;
+		fastboot_multiresponse(multiresponse_cmd, response);
+
+		/* Only add if first is a INFO */
+		if (!strncmp("INFO", 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..d1a2b74b2f 100644
--- a/include/fastboot.h
+++ b/include/fastboot.h
@@ -172,5 +172,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..ca3278f7b4 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,19 @@ static void fastboot_send(struct fastboot_header header, char *fastboot_data,
 		} else {
 			cmd = fastboot_handle_command(command, response);
 			pending_command = false;
+
+			if (!strncmp("MORE", response, 4)) {
+				while (1) {
+					/* Call handler to obtain next response */
+					fastboot_multiresponse(multiresponse_cmd, response);
+
+					/* Send INFO or break to send final response */
+					if (!strncmp("INFO", response, 4))
+						fastboot_udp_send_response(response);
+					else
+						break;
+				}
+			}
 		}
 		/*
 		 * Sent some INFO packets, need to update sequence number in
-- 
2.40.1

Re: [PATCH v1 1/5] fastboot: multiresponse support

[Mattijs Korpershoek]

Hi Svyatoslav,

Thank you for your patch.

On mar., nov. 07, 2023 at 14:42, Svyatoslav Ryhel <clamor95@gmail.com> wrote:

> 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>
> ---
>  drivers/fastboot/fb_command.c   | 10 ++++++++++
>  drivers/usb/gadget/f_fastboot.c | 29 +++++++++++++++++++++++++++++
>  include/fastboot.h              |  8 ++++++++
>  net/fastboot_udp.c              | 25 +++++++++++++++++++------
>  4 files changed, 66 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 741775a7bc..7d8c22b948 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);
>  }
>  
> +int multiresponse_cmd = -1;

This is non static, because it is also used in net/fastboot_udp.c

However, if we:
1. disable CONFIG_USB_FUNCTION_FASTBOOT
2. enable CONFIG_UDP_FUNCTION_FASTBOOT

The build will break with:
net/fastboot_udp.c: In function 'fastboot_send':
net/fastboot_udp.c:187:64: error: 'multiresponse_cmd' undeclared (first use in this function)
  187 |                                         fastboot_multiresponse(multiresponse_cmd, response);
      |                                                                ^~~~~~~~~~~~~~~~~
net/fastboot_udp.c:187:64: note: each undeclared identifier is reported only once for each function it appears in

Since having only CONFIG_UDP_FUNCTION_FASTBOOT is valid (there is no
dependency on CONFIG_USB_FASTBOOT), can we think of something else to
handle multi-response commands without breaking UDP-only fastboot
support ?


> +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 not an INFO disconnect this handler and unset cmd */
> +	if (strncmp("INFO", response, 4) != 0) {
> +		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("MORE", response, 4)) {

As discussed in [1], please use TEXT instead of MORE

[1] https://lore.kernel.org/all/CAPVz0n2fOXeHQOOZ-obPWUCQ7LdFsKZFGxZw5bp-qF8SaxLSMA@mail.gmail.com/

> +		multiresponse_cmd = cmd;
> +		fastboot_multiresponse(multiresponse_cmd, response);
> +
> +		/* Only add if first is a INFO */
> +		if (!strncmp("INFO", 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..d1a2b74b2f 100644
> --- a/include/fastboot.h
> +++ b/include/fastboot.h
> @@ -172,5 +172,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..ca3278f7b4 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,19 @@ static void fastboot_send(struct fastboot_header header, char *fastboot_data,
>  		} else {
>  			cmd = fastboot_handle_command(command, response);
>  			pending_command = false;
> +
> +			if (!strncmp("MORE", response, 4)) {

MORE -> TEXT

> +				while (1) {
> +					/* Call handler to obtain next response */
> +					fastboot_multiresponse(multiresponse_cmd, response);

As written previously, multiresponse_cmd is undefined when:
CONFIG_USB_FUNCTION_FASTBOOT=n && CONFIG_UDP_FUNCTION_FASTBOOT=y

> +
> +					/* Send INFO or break to send final response */
> +					if (!strncmp("INFO", response, 4))
> +						fastboot_udp_send_response(response);
> +					else
> +						break;
> +				}
> +			}
>  		}
>  		/*
>  		 * Sent some INFO packets, need to update sequence number in
> -- 
> 2.40.1

[PATCH v1 2/5] 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>
---
 doc/android/fastboot-protocol.rst |  3 ++
 drivers/fastboot/fb_command.c     |  3 ++
 drivers/fastboot/fb_getvar.c      | 75 +++++++++++++++++++++++++------
 include/fastboot-internal.h       |  7 +++
 4 files changed, 75 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..fc5e1dac87 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,38 @@ static void getvar_is_userspace(char *var_parameter, char *response)
 	fastboot_okay("no", response);
 }
 
+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 +300,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("MORE", 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

Re: [PATCH v1 2/5] fastboot: implement "getvar all"

[Mattijs Korpershoek]

Hi Svyatoslav,

Thank you for your patch.

On mar., nov. 07, 2023 at 14:42, Svyatoslav Ryhel <clamor95@gmail.com> wrote:

> 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>

Some small comments below.

With those addressed, please add:

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

> ---
>  doc/android/fastboot-protocol.rst |  3 ++
>  drivers/fastboot/fb_command.c     |  3 ++
>  drivers/fastboot/fb_getvar.c      | 75 +++++++++++++++++++++++++------
>  include/fastboot-internal.h       |  7 +++
>  4 files changed, 75 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..fc5e1dac87 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,38 @@ static void getvar_is_userspace(char *var_parameter, char *response)
>  	fastboot_okay("no", response);
>  }
>  
> +int current_all_dispatch;

static please, as this is only used in drivers/fastboot/fb_getvar.c

> +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);

./scripts/checkpatch.pl --strict --u-boot complains a missing line here

> +
> +		char *envstr_start = envstr;

./scripts/checkpatch.pl --strict --u-boot complains a missing line here

> +		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 +300,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("MORE", 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

Re: [PATCH v1 2/5] fastboot: implement "getvar all"

[Svyatoslav Ryhel]

14 листопада 2023 р. 11:32:35 GMT+02:00, Mattijs Korpershoek <mkorpershoek@baylibre.com> написав(-ла):
>Hi Svyatoslav,
>
>Thank you for your patch.
>
>On mar., nov. 07, 2023 at 14:42, Svyatoslav Ryhel <clamor95@gmail.com> wrote:
>
>> 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>
>
>Some small comments below.
>
>With those addressed, please add:
>
>Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
>
>> ---
>>  doc/android/fastboot-protocol.rst |  3 ++
>>  drivers/fastboot/fb_command.c     |  3 ++
>>  drivers/fastboot/fb_getvar.c      | 75 +++++++++++++++++++++++++------
>>  include/fastboot-internal.h       |  7 +++
>>  4 files changed, 75 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..fc5e1dac87 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,38 @@ static void getvar_is_userspace(char *var_parameter, char *response)
>>  	fastboot_okay("no", response);
>>  }
>>  
>> +int current_all_dispatch;
>
>static please, as this is only used in drivers/fastboot/fb_getvar.c
>
>> +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);
>
>./scripts/checkpatch.pl --strict --u-boot complains a missing line here
>
>> +
>> +		char *envstr_start = envstr;
>
>./scripts/checkpatch.pl --strict --u-boot complains a missing line here
>

About missing lines, I have intentionally not added them since it does not make reading code harder while having a line-space-line IMHO makes it more unpleasant to read. I would like to leave it as is if possible. Everything else will be adjusted. Thanks!

>> +		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 +300,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("MORE", 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 v1 3/5] commonn: console: introduce overflow and isempty calls

[Svyatoslav Ryhel]

From: Ion Agorria <ion@agorria.com>

Separate record overflow logic and add console_record_isempty
as available calls don't serve to know output has been read
fully with readline's.

Signed-off-by: Ion Agorria <ion@agorria.com>
Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
 common/console.c  | 15 ++++++++++++---
 include/console.h | 14 ++++++++++++++
 test/ut.c         |  9 ++++-----
 3 files changed, 30 insertions(+), 8 deletions(-)

diff --git a/common/console.c b/common/console.c
index 98c3ee6ca6..8a869b137e 100644
--- a/common/console.c
+++ b/common/console.c
@@ -818,6 +818,8 @@ int console_record_init(void)
 	ret = membuff_new((struct membuff *)&gd->console_in,
 			  CONFIG_CONSOLE_RECORD_IN_SIZE);
 
+	gd->flags |= GD_FLG_RECORD;
+
 	return ret;
 }
 
@@ -836,11 +838,13 @@ int console_record_reset_enable(void)
 	return 0;
 }
 
-int console_record_readline(char *str, int maxlen)
+bool console_record_overflow(void)
 {
-	if (gd->flags & GD_FLG_RECORD_OVF)
-		return -ENOSPC;
+	return gd->flags & GD_FLG_RECORD_OVF ? true : false;
+}
 
+int console_record_readline(char *str, int maxlen)
+{
 	return membuff_readline((struct membuff *)&gd->console_out, str,
 				maxlen, '\0');
 }
@@ -850,6 +854,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 ceb733b5cb..c053bc9ba8 100644
--- a/include/console.h
+++ b/include/console.h
@@ -64,6 +64,13 @@ void console_record_reset(void);
  */
 int console_record_reset_enable(void);
 
+/**
+ * console_record_overflow() - returns state of buffers overflow
+ *
+ * Return: true if the console buffer was overflowed
+ */
+bool console_record_overflow(void);
+
 /**
  * console_record_readline() - Read a line from the console output
  *
@@ -84,6 +91,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
  *
diff --git a/test/ut.c b/test/ut.c
index 28da417686..d202644a15 100644
--- a/test/ut.c
+++ b/test/ut.c
@@ -53,15 +53,14 @@ long ut_check_delta(ulong last)
 
 static int readline_check(struct unit_test_state *uts)
 {
-	int ret;
-
-	ret = console_record_readline(uts->actual_str, sizeof(uts->actual_str));
-	if (ret == -ENOSPC) {
+	if (console_record_overflow()) {
 		ut_fail(uts, __FILE__, __LINE__, __func__,
 			"Console record buffer too small - increase CONFIG_CONSOLE_RECORD_OUT_SIZE");
-		return ret;
+		return -ENOSPC;
 	}
 
+	console_record_readline(uts->actual_str, sizeof(uts->actual_str));
+
 	return 0;
 }
 
-- 
2.40.1

[PATCH v1 4/5] 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>
---
 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 8a869b137e..cd56035171 100644
--- a/common/console.c
+++ b/common/console.c
@@ -846,7 +846,7 @@ bool console_record_overflow(void)
 int console_record_readline(char *str, int maxlen)
 {
 	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 36dc43a523..964e504d5b 100644
--- a/lib/membuff.c
+++ b/lib/membuff.c
@@ -288,7 +288,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;
@@ -310,7 +310,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 v1 5/5] 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>
---
 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 837c6f1180..58b08120a4 100644
--- a/drivers/fastboot/Kconfig
+++ b/drivers/fastboot/Kconfig
@@ -241,6 +241,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..acf5971108 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;
+#if CONFIG_IS_ENABLED(FASTBOOT_CMD_OEM_CONSOLE)
+	case FASTBOOT_COMMAND_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;
+#endif
 	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("MORE", response, NULL);
+}
diff --git a/include/fastboot.h b/include/fastboot.h
index d1a2b74b2f..23d26fb4be 100644
--- a/include/fastboot.h
+++ b/include/fastboot.h
@@ -37,6 +37,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 v1 5/5] fastboot: add oem console command support

[Mattijs Korpershoek]

Hi Svyatoslav,

Thank you for your patch.

On mar., nov. 07, 2023 at 14:42, Svyatoslav Ryhel <clamor95@gmail.com> wrote:

> 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>
> ---
>  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 837c6f1180..58b08120a4 100644
> --- a/drivers/fastboot/Kconfig
> +++ b/drivers/fastboot/Kconfig
> @@ -241,6 +241,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..acf5971108 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;
> +#if CONFIG_IS_ENABLED(FASTBOOT_CMD_OEM_CONSOLE)

Checkpatch also complains about this.

Can we rewrite this using if (IS_ENABLED(CONFIG...)) please ?


> +	case FASTBOOT_COMMAND_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;
> +#endif
>  	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("MORE", response, NULL);

MORE -> TEXT

> +}
> diff --git a/include/fastboot.h b/include/fastboot.h
> index d1a2b74b2f..23d26fb4be 100644
> --- a/include/fastboot.h
> +++ b/include/fastboot.h
> @@ -37,6 +37,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 v1 5/5] fastboot: add oem console command support

[Svyatoslav Ryhel]

14 листопада 2023 р. 12:24:52 GMT+02:00, Mattijs Korpershoek <mkorpershoek@baylibre.com> написав(-ла):
>Hi Svyatoslav,
>
>Thank you for your patch.
>
>On mar., nov. 07, 2023 at 14:42, Svyatoslav Ryhel <clamor95@gmail.com> wrote:
>
>> 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>
>> ---
>>  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 837c6f1180..58b08120a4 100644
>> --- a/drivers/fastboot/Kconfig
>> +++ b/drivers/fastboot/Kconfig
>> @@ -241,6 +241,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..acf5971108 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;
>> +#if CONFIG_IS_ENABLED(FASTBOOT_CMD_OEM_CONSOLE)
>
>Checkpatch also complains about this.
>
>Can we rewrite this using if (IS_ENABLED(CONFIG...)) please ?
>

Please, do not relay on checkpatch that much. In this case #ifdef is better since in this case all under ifdef will be cut off while using if(...) requires all code under the if to be able to be run even if config is not enabled. Thanks.

>> +	case FASTBOOT_COMMAND_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;
>> +#endif
>>  	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("MORE", response, NULL);
>
>MORE -> TEXT
>
>> +}
>> diff --git a/include/fastboot.h b/include/fastboot.h
>> index d1a2b74b2f..23d26fb4be 100644
>> --- a/include/fastboot.h
>> +++ b/include/fastboot.h
>> @@ -37,6 +37,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 v1 5/5] fastboot: add oem console command support

[Mattijs Korpershoek]

Hi Svyatoslav,

On mar., nov. 14, 2023 at 12:30, Svyatoslav Ryhel <clamor95@gmail.com> wrote:

> 14 листопада 2023 р. 12:24:52 GMT+02:00, Mattijs Korpershoek <mkorpershoek@baylibre.com> написав(-ла):
>>Hi Svyatoslav,
>>
>>Thank you for your patch.
>>
>>On mar., nov. 07, 2023 at 14:42, Svyatoslav Ryhel <clamor95@gmail.com> wrote:
>>
>>> 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>
>>> ---
>>>  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 837c6f1180..58b08120a4 100644
>>> --- a/drivers/fastboot/Kconfig
>>> +++ b/drivers/fastboot/Kconfig
>>> @@ -241,6 +241,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..acf5971108 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;
>>> +#if CONFIG_IS_ENABLED(FASTBOOT_CMD_OEM_CONSOLE)
>>
>>Checkpatch also complains about this.
>>
>>Can we rewrite this using if (IS_ENABLED(CONFIG...)) please ?
>>
>
> Please, do not relay on checkpatch that much. In this case #ifdef is better since in this case all under ifdef will be cut off while using if(...) requires all code under the if to be able to be run even if config is not enabled. Thanks.

I understand that sometimes, checkpatch generates false positives or bad
suggestions.
I also understand the differences between #ifdef and if (IS_ENABLED()).

I did not measure whether the binary size is bigger when switching from
#ifdef to "if IS_ENABLED()" but I suspect that the compiler can
optimize this out as it's known at compile-time.

This file (and the fastboot code in general) mostly uses
"if (IS_ENABLED())" and to keep the code consistent I recommend using it.

Therefore, can we please use if (IS_ENABLED()) here ?

Thank you

Mattijs

>
>>> +	case FASTBOOT_COMMAND_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;
>>> +#endif
>>>  	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("MORE", response, NULL);
>>
>>MORE -> TEXT
>>
>>> +}
>>> diff --git a/include/fastboot.h b/include/fastboot.h
>>> index d1a2b74b2f..23d26fb4be 100644
>>> --- a/include/fastboot.h
>>> +++ b/include/fastboot.h
>>> @@ -37,6 +37,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 v1 0/5] Implement fastboot multiresponce

[Mattijs Korpershoek]

Hi Svyatoslav, Ion,

Thank you for these series.

On mar., nov. 07, 2023 at 14:42, 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.

I'm happy to fastboot getvar all implemented since it's standardly
supported on Android phones.

fastboot oem console seems like a nice debug tool as well when no
console is available.

I do have some concerns with the series, mainly because the introduction
of the "MORE" byte response.
"MORE" is not part of the fastboot specification [1].

However, that specification documents "TEXT", which, per my
understanding, should allow to send multiple responses as well.

Could we implement this using "TEXT" instead ?

[1] https://android.googlesource.com/platform/system/core/+/master/fastboot/#transport-and-framing

>
> Ion Agorria (5):
>   fastboot: multiresponse support
>   fastboot: implement "getvar all"
>   commonn: console: introduce overflow and isempty calls
>   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                  | 17 +++++--
>  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      | 75 +++++++++++++++++++++++++------
>  drivers/usb/gadget/f_fastboot.c   | 29 ++++++++++++
>  include/console.h                 | 14 ++++++
>  include/fastboot-internal.h       |  7 +++
>  include/fastboot.h                |  9 ++++
>  include/membuff.h                 |  5 ++-
>  lib/membuff.c                     |  4 +-
>  net/fastboot_udp.c                | 25 ++++++++---
>  test/ut.c                         |  9 ++--
>  15 files changed, 226 insertions(+), 33 deletions(-)
>
> -- 
> 2.40.1

Re: [PATCH v1 0/5] Implement fastboot multiresponce

[Svyatoslav Ryhel]

9 листопада 2023 р. 10:41:30 GMT+02:00, Mattijs Korpershoek <mkorpershoek@baylibre.com> написав(-ла):
>Hi Svyatoslav, Ion,
>
>Thank you for these series.
>
>On mar., nov. 07, 2023 at 14:42, 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.
>

Hello Mattijs!
Thank you for your suggestions and so rapid responce!

>I'm happy to fastboot getvar all implemented since it's standardly
>supported on Android phones.
>
>fastboot oem console seems like a nice debug tool as well when no
>console is available.
>
>I do have some concerns with the series, mainly because the introduction
>of the "MORE" byte response.
>"MORE" is not part of the fastboot specification [1].
>
>However, that specification documents "TEXT", which, per my
>understanding, should allow to send multiple responses as well.
>
>Could we implement this using "TEXT" instead ?

I see no issues with replacing MORE with TEXT whatsoever.

These patches were mainly developed and used for our internal development needs but since they served extremely well, I have decided to push them upstream so other developers could benefit as well.

If you have no other suggestions then swap more > text, please send your reviewed-by to patch and I will reload v2 with swap included.

Best regards,
Svyatoslav R.

>[1] https://android.googlesource.com/platform/system/core/+/master/fastboot/#transport-and-framing
>
>>
>> Ion Agorria (5):
>>   fastboot: multiresponse support
>>   fastboot: implement "getvar all"
>>   commonn: console: introduce overflow and isempty calls
>>   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                  | 17 +++++--
>>  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      | 75 +++++++++++++++++++++++++------
>>  drivers/usb/gadget/f_fastboot.c   | 29 ++++++++++++
>>  include/console.h                 | 14 ++++++
>>  include/fastboot-internal.h       |  7 +++
>>  include/fastboot.h                |  9 ++++
>>  include/membuff.h                 |  5 ++-
>>  lib/membuff.c                     |  4 +-
>>  net/fastboot_udp.c                | 25 ++++++++---
>>  test/ut.c                         |  9 ++--
>>  15 files changed, 226 insertions(+), 33 deletions(-)
>>
>> -- 
>> 2.40.1

Re: [PATCH v1 0/5] Implement fastboot multiresponce

[Mattijs Korpershoek]

Hi Svyatoslav,

On jeu., nov. 09, 2023 at 11:01, Svyatoslav Ryhel <clamor95@gmail.com> wrote:

> 9 листопада 2023 р. 10:41:30 GMT+02:00, Mattijs Korpershoek <mkorpershoek@baylibre.com> написав(-ла):
>>Hi Svyatoslav, Ion,
>>
>>Thank you for these series.
>>
>>On mar., nov. 07, 2023 at 14:42, 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.
>>
>
> Hello Mattijs!
> Thank you for your suggestions and so rapid responce!
>
>>I'm happy to fastboot getvar all implemented since it's standardly
>>supported on Android phones.
>>
>>fastboot oem console seems like a nice debug tool as well when no
>>console is available.
>>
>>I do have some concerns with the series, mainly because the introduction
>>of the "MORE" byte response.
>>"MORE" is not part of the fastboot specification [1].
>>
>>However, that specification documents "TEXT", which, per my
>>understanding, should allow to send multiple responses as well.
>>
>>Could we implement this using "TEXT" instead ?
>
> I see no issues with replacing MORE with TEXT whatsoever.
>
> These patches were mainly developed and used for our internal development needs but since they served extremely well, I have decided to push them upstream so other developers could benefit as well.

Thank you for taking the time to upstream.

>
> If you have no other suggestions then swap more > text, please send your reviewed-by to patch and I will reload v2 with swap included.

I will do a more thorough review in the coming days, I wanted to get
feedback on MORE->TEXT first.

>
> Best regards,
> Svyatoslav R.
>
>>[1] https://android.googlesource.com/platform/system/core/+/master/fastboot/#transport-and-framing
>>
>>>
>>> Ion Agorria (5):
>>>   fastboot: multiresponse support
>>>   fastboot: implement "getvar all"
>>>   commonn: console: introduce overflow and isempty calls
>>>   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                  | 17 +++++--
>>>  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      | 75 +++++++++++++++++++++++++------
>>>  drivers/usb/gadget/f_fastboot.c   | 29 ++++++++++++
>>>  include/console.h                 | 14 ++++++
>>>  include/fastboot-internal.h       |  7 +++
>>>  include/fastboot.h                |  9 ++++
>>>  include/membuff.h                 |  5 ++-
>>>  lib/membuff.c                     |  4 +-
>>>  net/fastboot_udp.c                | 25 ++++++++---
>>>  test/ut.c                         |  9 ++--
>>>  15 files changed, 226 insertions(+), 33 deletions(-)
>>>
>>> -- 
>>> 2.40.1

Re: [PATCH v1 0/5] Implement fastboot multiresponce

[Svyatoslav Ryhel]

чт, 9 лист. 2023 р. о 13:00 Mattijs Korpershoek
<mkorpershoek@baylibre.com> пише:
>
> Hi Svyatoslav,
>
> On jeu., nov. 09, 2023 at 11:01, Svyatoslav Ryhel <clamor95@gmail.com> wrote:
>
> > 9 листопада 2023 р. 10:41:30 GMT+02:00, Mattijs Korpershoek <mkorpershoek@baylibre.com> написав(-ла):
> >>Hi Svyatoslav, Ion,
> >>
> >>Thank you for these series.
> >>
> >>On mar., nov. 07, 2023 at 14:42, 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.
> >>
> >
> > Hello Mattijs!
> > Thank you for your suggestions and so rapid responce!
> >
> >>I'm happy to fastboot getvar all implemented since it's standardly
> >>supported on Android phones.
> >>
> >>fastboot oem console seems like a nice debug tool as well when no
> >>console is available.
> >>
> >>I do have some concerns with the series, mainly because the introduction
> >>of the "MORE" byte response.
> >>"MORE" is not part of the fastboot specification [1].
> >>
> >>However, that specification documents "TEXT", which, per my
> >>understanding, should allow to send multiple responses as well.
> >>
> >>Could we implement this using "TEXT" instead ?
> >
> > I see no issues with replacing MORE with TEXT whatsoever.
> >
> > These patches were mainly developed and used for our internal development needs but since they served extremely well, I have decided to push them upstream so other developers could benefit as well.
>
> Thank you for taking the time to upstream.
>
> >
> > If you have no other suggestions then swap more > text, please send your reviewed-by to patch and I will reload v2 with swap included.
>
> I will do a more thorough review in the coming days, I wanted to get
> feedback on MORE->TEXT first.
>

I have just tested with TEXT and fastboot works exactly the same way
it worked with MORE. I will upload v2 with swap alongside any other
changes, if such would be needed.

> >
> > Best regards,
> > Svyatoslav R.
> >
> >>[1] https://android.googlesource.com/platform/system/core/+/master/fastboot/#transport-and-framing
> >>
> >>>
> >>> Ion Agorria (5):
> >>>   fastboot: multiresponse support
> >>>   fastboot: implement "getvar all"
> >>>   commonn: console: introduce overflow and isempty calls
> >>>   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                  | 17 +++++--
> >>>  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      | 75 +++++++++++++++++++++++++------
> >>>  drivers/usb/gadget/f_fastboot.c   | 29 ++++++++++++
> >>>  include/console.h                 | 14 ++++++
> >>>  include/fastboot-internal.h       |  7 +++
> >>>  include/fastboot.h                |  9 ++++
> >>>  include/membuff.h                 |  5 ++-
> >>>  lib/membuff.c                     |  4 +-
> >>>  net/fastboot_udp.c                | 25 ++++++++---
> >>>  test/ut.c                         |  9 ++--
> >>>  15 files changed, 226 insertions(+), 33 deletions(-)
> >>>
> >>> --
> >>> 2.40.1