On Fri, Nov 08, 2024 at 09:37:32AM -0800, Doug Anderson wrote: > Hi, > > On Fri, Nov 8, 2024 at 8:17 AM Omar Sandoval <osandov@xxxxxxxxxxx> wrote: > > > > On Fri, Nov 08, 2024 at 07:31:19AM -0800, Doug Anderson wrote: > > > Hi, > > > > > > On Thu, Nov 7, 2024 at 5:13 PM Omar Sandoval <osandov@xxxxxxxxxxx> wrote: > > > > > > > > On Thu, Nov 07, 2024 at 05:08:58PM -0800, Doug Anderson wrote: > > > > > Hi, > > > > > > > > > > On Thu, Nov 7, 2024 at 2:23 PM Omar Sandoval <osandov@xxxxxxxxxxx> wrote: > > > > > > > > > > > > Hi everyone, > > > > > > > > > > > > Amal is working on adding a custom query packet to kgdb for getting the > > > > > > kernel's vmcoreinfo. The rationale and details are available here: > > > > > > https://github.com/osandov/drgn/wiki/GDB-Remote-Protocol-proposal:-linux.vmcoreinfo-query-packet > > > > > > > > > > > > vmcoreinfo is about 3kB, so we were hoping to avoid hex-encoding the > > > > > > response and doubling the time it takes to transmit over a slow serial > > > > > > connection. Instead, we were hoping to use the escaped binary format, > > > > > > which escapes the characters #$}* and leaves other bytes untouched. > > > > > > > > > > > > We ran into a problem, though: vmcoreinfo contains newline characters, > > > > > > which the serial core replaces with CRLF; see commit c7d44a02ac60 > > > > > > ("serial_core: Commonalize crlf when working w/ a non open console > > > > > > port"). > > > > > > > > > > FWIW, the problem predates that commit, but that commit at least moved > > > > > it to be someplace common. Before that some serial drivers were > > > > > hardcoding it... ;-) > > > > > > > > > > > > > > > > This effectively corrupts the data and causes a checksum > > > > > > mismatch. > > > > > > > > > > > > We'd love some input on how to work around this, especially from the > > > > > > kgdb maintainers. Here are a few options, in descending order of my > > > > > > preference: > > > > > > > > > > > > 1. Disable the LF -> CRLF replacement while sending binary data. > > > > > > 2. Escape the newlines using some other custom scheme. > > > > > > 3. Give up and hex-encode the response. > > > > > > > > > > I haven't tried prototyping it, but what about moving the LR -> CRLF > > > > > code to kdb_msg_write(). It would be really easy to do this in the > > > > > case where we're doing "dbg_io_ops->write_char()" since we're already > > > > > processing character at a time. It would be harder to do this when > > > > > also sending the output to the various console, but may not _too_ > > > > > hard? You could loop searching for "\n" and send all the characters > > > > > before the "\n", then send a "\r", then send the "\n" and all the > > > > > characters up to the next "\n". > > Actually, looking at this again, I wonder if we even need to do any > transformation before sending it to the various consoles. Probably > not. I think it's _just_ the write_char() path that needed it? > > > > > > > If you did this then you'd lose the "\n" to "\r\n" combination in the > > > > > gdb stub, but _probably_ that doesn't matter? > > > > > > > > That sounds reasonable. I was concerned whether this would affect > > > > anything else using the ->poll_put_char() tty operation, but kgdb seems > > > > to be the only user, does that sound right? > > > > > > Right. As far as I can tell, kgdb is the only user of poll_put_char(). > > > > Ah, one other concern, though: only uart_poll_put_char() does the CRLF > > replacement, but there are other tty_operations that don't, like > > hvc_poll_put_char(). So if we move that to kdb_msg_write(), then we'll > > be adding extra '\r' for other tty types. Would that be a problem? > > I honestly don't know. I guess also it can be noted that if we do it > in kdb_msg_write() then the write_char() path can even take us to > places that don't invoke uart_poll_put_char(). For instance > "ehci-dbgp.c" registers its own io_module... > > Bleh. It _probably_ wouldn't be a big deal to do the LF -> CRLF for > all these, but I don't know for sure. I guess worst case you could add > some flag in the "dbg_io_ops" and figure out how to set it just for > UARTs? I find myself in a similar position to Doug. It sounds reasonable to move the CR synthesis into kdb_msg_write() but I also am not certain the other polling backends will handle this correctly. However I did apply the following patch and run a few tests and it all looks good. Specifically there are no regressions from the kgdb test suite although that's unsurprising since that suite exclusively tests the serial port. I also fired up the kdb,kms polling backends on x86 and verified that I didn't get double line feeds in kdb (I did not). That means I'd certainly accept patches based on Doug's idea and if it proves later that we have to revert and add a new dbg_io_ops pointer to switch the handler between binary and ascii modes then so be it! Daniel. diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c index d94d73e45fb6d..fff1269c55498 100644 --- a/drivers/tty/serial/serial_core.c +++ b/drivers/tty/serial/serial_core.c @@ -2738,8 +2738,10 @@ static void uart_poll_put_char(struct tty_driver *driver, int line, char ch) if (!port) return; +#if 0 if (ch == '\n') port->ops->poll_put_char(port, '\r'); +#endif port->ops->poll_put_char(port, ch); uart_port_deref(port); } diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c index 6a77f1c779c4c..43a7c8ad741ac 100644 --- a/kernel/debug/kdb/kdb_io.c +++ b/kernel/debug/kdb/kdb_io.c @@ -572,6 +572,8 @@ static void kdb_msg_write(const char *msg, int msg_len) len = msg_len; while (len--) { + if (*cp == '\n') + dbg_io_ops->write_char('\r'); dbg_io_ops->write_char(*cp); cp++; }
