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