Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20191008132043.7966-1-daniel.thompson@linaro.org> <20191008132043.7966-4-daniel.thompson@linaro.org>
In-Reply-To: <20191008132043.7966-4-daniel.thompson@linaro.org>
From:   Doug Anderson <dianders@chromium.org>
Date:   Tue, 8 Oct 2019 15:21:02 -0700
Message-ID: <CAD=FV=W9Tdh2hPekzgSYnCqoTX_ms1GY-FXgnxd-uEW0SWbyuw@mail.gmail.com>
Subject: Re: [PATCH v2 3/5] kdb: Remove special case logic from kdb_read()
To:     Daniel Thompson <daniel.thompson@linaro.org>
Cc:     Jason Wessel <jason.wessel@windriver.com>,
        kgdb-bugreport@lists.sourceforge.net,
        LKML <linux-kernel@vger.kernel.org>,
        Patch Tracking <patches@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi,

On Tue, Oct 8, 2019 at 6:21 AM Daniel Thompson
<daniel.thompson@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.thompson@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