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-5-daniel.thompson@linaro.org>
In-Reply-To: <20191008132043.7966-5-daniel.thompson@linaro.org>
From:   Doug Anderson <dianders@chromium.org>
Date:   Tue, 8 Oct 2019 15:21:15 -0700
Message-ID: <CAD=FV=W008gO4HbtcSXH-uA4jyb6iJLYQJF5L++6CM0AtJmkAg@mail.gmail.com>
Subject: Re: [PATCH v2 4/5] kdb: Improve handling of characters from different
 input sources
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:
>
> Currently if an escape timer is interrupted by a character from a
> different input source then the new character is discarded and the
> function returns '\e' (which will be discarded by the level above).
> It is hard to see why this would ever be the desired behaviour.

I guess the 2nd input source would be if you enable keyboard input?
Personally I've never used this myself, but your functional change
seems OK to me.


> Fix this to return the new character rather then the '\e'.

s/then/than


> This is a bigger refactor that might be expected because the new
> character needs to go through escape sequence detection.
>
> Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
> ---
>  kernel/debug/kdb/kdb_io.c | 37 ++++++++++++++++++-------------------
>  1 file changed, 18 insertions(+), 19 deletions(-)
>
> diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
> index a9e73bc9d1c3..288dd1babf90 100644
> --- a/kernel/debug/kdb/kdb_io.c
> +++ b/kernel/debug/kdb/kdb_io.c
> @@ -122,8 +122,8 @@ static int kdb_getchar(void)
>  {
>  #define ESCAPE_UDELAY 1000
>  #define ESCAPE_DELAY (2*1000000/ESCAPE_UDELAY) /* 2 seconds worth of udelays */
> -       char escape_data[5];    /* longest vt100 escape sequence is 4 bytes */
> -       char *ped = escape_data;
> +       char buf[4];    /* longest vt100 escape sequence is 4 bytes */
> +       char *pbuf = buf;
>         int escape_delay = 0;
>         get_char_func *f, *f_escape = NULL;
>         int key;
> @@ -145,27 +145,26 @@ static int kdb_getchar(void)
>                         continue;
>                 }
>
> -               if (escape_delay == 0 && key == '\e') {
> -                       escape_delay = ESCAPE_DELAY;
> -                       ped = escape_data;
> +               /*
> +                * When the first character is received (or we get a change
> +                * input source) we set ourselves up to handle an escape
> +                * sequences (just in case).
> +                */
> +               if (f_escape != f) {
>                         f_escape = f;

Would it make sense to rename "f_escape" to "f_last" or "f_prev" now?
Essentially this logic now happens every time you change input
sources.


-Doug