[U-Boot] [PATCH] efi_loader: Fix serial console size detection

[Heinrich Schuchardt]

On 2/26/19 12:46 PM, matthias.bgg at kernel.org wrote:
> From: Matthias Brugger <mbrugger at suse.com>
> 
> Function term_read_reply tries to read from the serial console until
> the end_char was read. This can hang forever if we are, for some reason,
> not be able to read the full response (e.g. serial buffer too small,
> frame error). This patch moves the timeout detection into
> term_read_reply to assure we will make progress.
> 
> Fixes: 6bb591f704 ("efi_loader: query serial console size reliably")
> Signed-off-by: Matthias Brugger <mbrugger at suse.com>

Thanks Matthias for the patch.

The general direction is right. Yet I would prefer if you could move the
waiting loop as described below.

> ---
>  lib/efi_loader/efi_console.c | 63 +++++++++++++++++++++---------------
>  1 file changed, 37 insertions(+), 26 deletions(-)
> 
> diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c
> index 66c33a551d..817220455f 100644
> --- a/lib/efi_loader/efi_console.c
> +++ b/lib/efi_loader/efi_console.c
> @@ -62,6 +62,16 @@ static struct simple_text_output_mode efi_con_mode = {
>  	.cursor_visible = 1,
>  };
>  
> +static int term_get_char(char *c)
> +{
> +	if (tstc()) {
> +		*c = getc();
> +		return 0;
> +	}
> +
> +	return 1;

The function signals an error if the character to read is not yet in the
UART buffer. I think it would be preferable to put the waiting time (100
ms is sufficient at 110 baud and above) into this function instead. This
has the following advantages:

- We would need only one waiting loop.
- We could reuse the function in function efi_cin_read_key() where we
  have another chance to hang waiting for a character.

> +}
> +
>  /*
>   * Receive and parse a reply from the terminal.
>   *
> @@ -74,32 +84,42 @@ static int term_read_reply(int *n, int num, char end_char)
>  {
>  	char c;
>  	int i = 0;
> +	u64 timeout;
>  
> -	c = getc();
> -	if (c != cESC)
> +	/* Allow up to one second for the response */
> +	timeout = timer_get_us() + 1000000;

Even at 110 baud a waiting time of 100 ms is sufficient.

> +	while (!tstc())
> +		if (timer_get_us() > timeout)
> +			return -1;

This loop could be moved to term_get_char().

> +
> +	if (term_get_char(&c) || c != cESC)
>  		return -1;
> -	c = getc();
> -	if (c != '[')
> +
> +	if (term_get_char(&c) || c != '[')

We should allow time for this character to arrive.

>  		return -1;
>  
>  	n[0] = 0;
>  	while (1) {
> -		c = getc();
> -		if (c == ';') {
> -			i++;
> -			if (i >= num)> +		if (!term_get_char(&c)) {

On loop entry there is no wait before this term_get_char().

Best regards

Heinrich

> +			if (c == ';') {
> +				i++;
> +				if (i >= num)
> +					return -1;
> +				n[i] = 0;
> +				continue;
> +			} else if (c == end_char) {
> +				break;
> +			} else if (c > '9' || c < '0') {
>  				return -1;
> -			n[i] = 0;
> -			continue;
> -		} else if (c == end_char) {
> -			break;
> -		} else if (c > '9' || c < '0') {
> -			return -1;
> +			}
> +
> +			/* Read one more decimal position */
> +			n[i] *= 10;
> +			n[i] += c - '0';
>  		}
>  
> -		/* Read one more decimal position */
> -		n[i] *= 10;
> -		n[i] += c - '0';
> +		if (timer_get_us() > timeout)
> +			return -1;
>  	}
>  	if (i != num - 1)
>  		return -1;
> @@ -196,7 +216,6 @@ static int query_console_serial(int *rows, int *cols)
>  {
>  	int ret = 0;
>  	int n[2];
> -	u64 timeout;
>  
>  	/* Empty input buffer */
>  	while (tstc())
> @@ -216,14 +235,6 @@ static int query_console_serial(int *rows, int *cols)
>  	       ESC "[999;999H"	/* Move to bottom right corner */
>  	       ESC "[6n");	/* Query cursor position */
>  
> -	/* Allow up to one second for a response */
> -	timeout = timer_get_us() + 1000000;
> -	while (!tstc())
> -		if (timer_get_us() > timeout) {
> -			ret = -1;
> -			goto out;
> -		}
> -
>  	/* Read {rows,cols} */
>  	if (term_read_reply(n, 2, 'R')) {
>  		ret = 1;
>