Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 29 Jun 2023 17:48:09 +0100
From:   Daniel Thompson <daniel.thompson@linaro.org>
To:     Douglas Anderson <dianders@chromium.org>
Cc:     Aaron Tomlin <atomlin@atomlin.com>,
        Jason Wessel <jason.wessel@windriver.com>,
        John Ogness <john.ogness@linutronix.de>,
        Petr Mladek <pmladek@suse.com>,
        kgdb-bugreport@lists.sourceforge.net, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] kdb: Handle LF in the command parser
Message-ID: <20230629164809.GA2872423@aspen.lan>
References: <20230628125612.1.I5cc6c3d916195f5bcfdf5b75d823f2037707f5dc@changeid>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230628125612.1.I5cc6c3d916195f5bcfdf5b75d823f2037707f5dc@changeid>
Precedence: bulk

On Wed, Jun 28, 2023 at 12:56:17PM -0700, Douglas Anderson wrote:
> The main kdb command parser only handles CR (ASCII 13 AKA '\r') today,
> but not LF (ASCII 10 AKA '\n'). That means that the kdb command parser
> can handle terminals that send just CR or that send CR+LF but can't
> handle terminals that send just LF.
>
> The fact that kdb didn't handle LF in the command parser tripped up a
> tool I tried to use with it. Specifically, I was trying to send a
> command to my device to resume it from kdb using a ChromeOS tool like:
>   dut-control cpu_uart_cmd:"g"
> That tool only terminates lines with LF, not CR+LF.
>
> Arguably the ChromeOS tool should be fixed. After all, officially kdb
> seems to be designed such that CR+LF is the official line ending
> transmitted over the wire and that internally a line ending is just
> '\n' (LF). Some evidence:
> * uart_poll_put_char(), which is used by kdb, notices a '\n' and
>   converts it to '\r\n'.
> * kdb functions specifically use '\r' to get a carriage return without
>   a newline. You can see this in the pager where kdb will write a '\r'
>   and then write over the pager prompt.

I'd view this as simply "what you have to do drive a terminal correctly"
rather than indicating what the official newline protocol for kdb is.
With a human and a terminal emulator I would expect the typical input to
be CR-only (and that's what I setup the test suite to send ;-)).


> However, all that being said there's no real harm in accepting LF as a
> command terminator in the kdb parser and doing so seems like it would
> improve compatibility. After this, I'd expect that things would work
> OK-ish with a remote terminal that used any of CR, CR+LF, or LF as a
> line ending. Someone using CR as a line ending might get some ugliness
> where kdb wasn't able to overwrite the last line, but basic commands
> would work. Someone using just LF as a line ending would probably also
> work OK.
>
> A few other notes:
> - It can be noted that "bash" running on an "agetty" handles LF as a
>   line termination with no complaints.
> - Historically, kdb's "pager" actually handled either CR or LF fine. A
>   very quick inspection would make one think that kdb's pager actually
>   could have paged down two lines instead of one for anyone using
>   CR+LF, but this is generally avoided because of kdb_input_flush().

These are very convincing though!

> - Conceivably one could argue that some of this special case logic
>   belongs in uart_poll_get_char() since uart_poll_put_char() handles
>   the '\n' => '\r\n' conversion. I would argue that perhaps we should
>   eventually do the opposite and move the '\n' => '\r\n' out of
>   uart_poll_put_char(). Having that conversion at such a low level
>   could interfere if we ever want to transfer binary data. In
>   addition, if we truly made uart_poll_get_char() the inverse of
>   uart_poll_put_char() it would convert back to '\n' and (ironically)
>   kdb's parser currently only looks for '\r' to find the end of a
>   command.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

This arrived just as I was gathering up the patches (I know... running
late). I've added a couple of cases to the test suite to cover the
new feature.

The code looks good to me. Are you happy for me to take it this
merge cycle?


Daniel.

> ---
>
>  kernel/debug/kdb/kdb_io.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
> index 5c7e9ba7cd6b..813cb6cf72d6 100644
> --- a/kernel/debug/kdb/kdb_io.c
> +++ b/kernel/debug/kdb/kdb_io.c
> @@ -131,6 +131,7 @@ char kdb_getchar(void)
>  	int escape_delay = 0;
>  	get_char_func *f, *f_prev = NULL;
>  	int key;
> +	static bool last_char_was_cr;
>
>  	for (f = &kdb_poll_funcs[0]; ; ++f) {
>  		if (*f == NULL) {
> @@ -149,6 +150,18 @@ char kdb_getchar(void)
>  			continue;
>  		}
>
> +		/*
> +		 * The caller expects that newlines are either CR or LF. However
> +		 * some terminals send _both_ CR and LF. Avoid having to handle
> +		 * this in the caller by stripping the LF if we saw a CR right
> +		 * before.
> +		 */
> +		if (last_char_was_cr && key == '\n') {
> +			last_char_was_cr = false;
> +			continue;
> +		}
> +		last_char_was_cr = (key == '\r');
> +
>  		/*
>  		 * When the first character is received (or we get a change
>  		 * input source) we set ourselves up to handle an escape
> @@ -244,7 +257,8 @@ static char *kdb_read(char *buffer, size_t bufsize)
>  			*cp = tmp;
>  		}
>  		break;
> -	case 13: /* enter */
> +	case 10: /* linefeed */
> +	case 13: /* carriage return */
>  		*lastchar++ = '\n';
>  		*lastchar++ = '\0';
>  		if (!KDB_STATE(KGDB_TRANS)) {
> --
> 2.41.0.162.gfafddb0af9-goog
>