On 2/26/19 12:46 PM, matthias....@kernel.org wrote:
> From: Matthias Brugger <mbrug...@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 <mbrug...@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 mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
