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

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