Hi, On Tue, Oct 8, 2019 at 6:21 AM Daniel Thompson <daniel.thomp...@linaro.org> wrote: > > kdb_read() contains special case logic to force it exit after reading > a single character. We can remove all the special case logic by directly > calling the function to read a single character instead. This also > allows us to tidy up the function prototype which, because it now matches > getchar(), we can also rename in order to make its role clearer.
nit: since you're doing the rename, should you rename kdb_read_handle_escape() to match? > Signed-off-by: Daniel Thompson <daniel.thomp...@linaro.org> > --- > kernel/debug/kdb/kdb_io.c | 56 ++++++++++++++++----------------------- > 1 file changed, 23 insertions(+), 33 deletions(-) > > diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c > index 78cb6e339408..a9e73bc9d1c3 100644 > --- a/kernel/debug/kdb/kdb_io.c > +++ b/kernel/debug/kdb/kdb_io.c > @@ -106,7 +106,19 @@ static int kdb_read_handle_escape(char *buf, size_t sz) > return -1; > } > > -static int kdb_read_get_key(char *buffer, size_t bufsize) > +/* > + * kdb_getchar > + * > + * Read a single character from kdb console (or consoles). nit: should we start moving to the standard kernel convention of kernel-doc style comments? See "Documentation/doc-guide/kernel-doc.rst" > + * > + * An escape key could be the start of a vt100 control sequence such as \e[D > + * (left arrow) or it could be a character in its own right. The standard > + * method for detecting the difference is to wait for 2 seconds to see if > there > + * are any other characters. kdb is complicated by the lack of a timer > service > + * (interrupts are off), by multiple input sources. Escape sequence > processing > + * has to be done as states in the polling loop. Before your paragraph, maybe add: "Most of the work of this function is dealing with escape sequences." to give it a little bit of context. > + */ > +static int kdb_getchar(void) Is "int" the right return type here, or "unsigned char"? You never return EOF, right? Always a valid character? NOTE: if you do change this to "unsigned char" I think you still need to keep the local "key" variable as an "int" since -1 shouldn't be confused with the character 255. > { > #define ESCAPE_UDELAY 1000 > #define ESCAPE_DELAY (2*1000000/ESCAPE_UDELAY) /* 2 seconds worth of udelays > */ > @@ -124,7 +136,6 @@ static int kdb_read_get_key(char *buffer, size_t bufsize) > } > > key = (*f)(); > - > if (key == -1) { > if (escape_delay) { > udelay(ESCAPE_UDELAY); > @@ -134,14 +145,6 @@ static int kdb_read_get_key(char *buffer, size_t bufsize) > continue; > } > > - if (bufsize <= 2) { > - if (key == '\r') > - key = '\n'; > - *buffer++ = key; > - *buffer = '\0'; > - return -1; > - } > - > if (escape_delay == 0 && key == '\e') { > escape_delay = ESCAPE_DELAY; > ped = escape_data; > @@ -183,17 +186,7 @@ static int kdb_read_get_key(char *buffer, size_t bufsize) > * function. It is not reentrant - it relies on the fact > * that while kdb is running on only one "master debug" cpu. > * Remarks: > - * > - * The buffer size must be >= 2. A buffer size of 2 means that the caller > only > - * wants a single key. By removing this you broke "BTAPROMPT". So doing: set BTAPROMPT=1 bta It's now impossible to quit out. Not that I've ever used BTAPROMPT, but seems like we should either get rid of it or keep it working. > - * > - * An escape key could be the start of a vt100 control sequence such as \e[D > - * (left arrow) or it could be a character in its own right. The standard > - * method for detecting the difference is to wait for 2 seconds to see if > there > - * are any other characters. kdb is complicated by the lack of a timer > service > - * (interrupts are off), by multiple input sources and by the need to > sometimes > - * return after just one key. Escape sequence processing has to be done as > - * states in the polling loop. > + * The buffer size must be >= 2. > */ > > static char *kdb_read(char *buffer, size_t bufsize) > @@ -228,9 +221,7 @@ static char *kdb_read(char *buffer, size_t bufsize) > *cp = '\0'; > kdb_printf("%s", buffer); > poll_again: > - key = kdb_read_get_key(buffer, bufsize); > - if (key == -1) > - return buffer; > + key = kdb_getchar(); > if (key != 9) > tab = 0; > switch (key) { > @@ -741,7 +732,7 @@ int vkdb_printf(enum kdb_msgsrc src, const char *fmt, > va_list ap) > > /* check for having reached the LINES number of printed lines */ > if (kdb_nextline >= linecount) { > - char buf1[16] = ""; > + char ch; The type of "ch" should be the same as returned by kdb_getchar()? Either "int" if you're keeping it "int" or "unsigned char"? > /* Watch out for recursion here. Any routine that calls > * kdb_printf will come back through here. And kdb_read > @@ -776,39 +767,38 @@ int vkdb_printf(enum kdb_msgsrc src, const char *fmt, > va_list ap) > if (logging) > printk("%s", moreprompt); > > - kdb_read(buf1, 2); /* '2' indicates to return > - * immediately after getting one key. */ > + ch = kdb_getchar(); > kdb_nextline = 1; /* Really set output line 1 */ > > /* empty and reset the buffer: */ > kdb_buffer[0] = '\0'; > next_avail = kdb_buffer; > size_avail = sizeof(kdb_buffer); > - if ((buf1[0] == 'q') || (buf1[0] == 'Q')) { > + if ((ch == 'q') || (ch == 'Q')) { > /* user hit q or Q */ > KDB_FLAG_SET(CMD_INTERRUPT); /* command interrupted */ > KDB_STATE_CLEAR(PAGER); > /* end of command output; back to normal mode */ > kdb_grepping_flag = 0; > kdb_printf("\n"); > - } else if (buf1[0] == ' ') { > + } else if (ch == ' ') { > kdb_printf("\r"); > suspend_grep = 1; /* for this recursion */ > - } else if (buf1[0] == '\n') { > + } else if (ch == '\n' || ch == '\r') { > kdb_nextline = linecount - 1; > kdb_printf("\r"); > suspend_grep = 1; /* for this recursion */ > - } else if (buf1[0] == '/' && !kdb_grepping_flag) { > + } else if (ch == '/' && !kdb_grepping_flag) { > kdb_printf("\r"); > kdb_getstr(kdb_grep_string, KDB_GREP_STRLEN, > kdbgetenv("SEARCHPROMPT") ?: "search> "); > *strchrnul(kdb_grep_string, '\n') = '\0'; > kdb_grepping_flag += KDB_GREPPING_FLAG_SEARCH; > suspend_grep = 1; /* for this recursion */ > - } else if (buf1[0] && buf1[0] != '\n') { > + } else if (ch && ch != '\n') { Remove "&& ch != '\n'". We would have hit an earlier case in the if/else anyway. If you really want to keep it here for some reason, I guess you should also handle '\r' ? -Doug