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

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