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

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

[matthias.bgg at kernel.org]

From: Matthias Brugger <mbrugger@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@suse.com>
---
 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;
+}
+
 /*
  * 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;
+	while (!tstc())
+		if (timer_get_us() > timeout)
+			return -1;
+
+	if (term_get_char(&c) || c != cESC)
 		return -1;
-	c = getc();
-	if (c != '[')
+
+	if (term_get_char(&c) || c != '[')
 		return -1;
 
 	n[0] = 0;
 	while (1) {
-		c = getc();
-		if (c == ';') {
-			i++;
-			if (i >= num)
+		if (!term_get_char(&c)) {
+			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;
-- 
2.20.1

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

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

[Matthias Brugger]

On 26/02/2019 17:58, Heinrich Schuchardt wrote:
> On 2/26/19 12:46 PM, matthias.bgg at kernel.org wrote:
>> From: Matthias Brugger <mbrugger@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@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.
> 

Ok, I'll do that in v2.

>> +}
>> +
>>  /*
>>   * 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.
> 

So we don't have to wait up to one second for the first character to arrive as
we did in query_console_serial() before this patch?

>> +	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().

I don't understand, if the character is not yet present, we will loop in the
while until it arrives or we hit the timeout.

Regards,
Matthias

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

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

[Heinrich Schuchardt]

On 2/27/19 10:55 AM, Matthias Brugger wrote:
> 
> 
> On 26/02/2019 17:58, Heinrich Schuchardt wrote:
>> On 2/26/19 12:46 PM, matthias.bgg at kernel.org wrote:
>>> From: Matthias Brugger <mbrugger@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@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.
>>
> 
> Ok, I'll do that in v2.
> 
>>> +}
>>> +
>>>  /*
>>>   * 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.
>>
> 
> So we don't have to wait up to one second for the first character to arrive as
> we did in query_console_serial() before this patch?

Teterminal will respond immediately when we query the size. Even at 110
baud it should be sufficient to wait for a maximum of 100 ms for the
first character.

> 
>>> +	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().
> 
> I don't understand, if the character is not yet present, we will loop in the
> while until it arrives or we hit the timeout.

We should not loop forever. If the next character is not received within
100 ms we should error out.

Best regards

Heinrich

> 
> Regards,
> Matthias
> 
>>
>> 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;
>>>
>>
>