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

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