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;
MIME-Version: 1.0
References: <20230628125612.1.I5cc6c3d916195f5bcfdf5b75d823f2037707f5dc@changeid>
 <20230629164809.GA2872423@aspen.lan>
In-Reply-To: <20230629164809.GA2872423@aspen.lan>
From:   Doug Anderson <dianders@chromium.org>
Date:   Thu, 29 Jun 2023 10:49:26 -0700
Message-ID: <CAD=FV=UFEhyqtSM9H4kEj72c5UEpeREZBszULhFsDAMnyLNCoA@mail.gmail.com>
Subject: Re: [PATCH] kdb: Handle LF in the command parser
To:     Daniel Thompson <daniel.thompson@linaro.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
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi,

On Thu, Jun 29, 2023 at 9:48=E2=80=AFAM Daniel Thompson
<daniel.thompson@linaro.org> wrote:
>
> 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 ;-)).

Fair enough. I haven't done massive amounts of serial communications
across lots of platforms, but I do remember the history of line
endings in text files and so I wanted to document my findings a bit.
;-)


> > 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' =3D> '\r\n' conversion. I would argue that perhaps we should
> >   eventually do the opposite and move the '\n' =3D> '\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?

Yeah, it should be OK. It's really pretty straightforward. Thanks!

-Doug