| Message ID | 1400079335-32125-2-git-send-email-daniel.thompson@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 05/14/2014 09:55 AM, Daniel Thompson wrote: > The behaviour of the UART poll_put_char infrastructure is inconsistent > with respect to linefeed conversions. This in turn leads to difficulty > using kdb on serial ports that are not also consoles > (e.g. console=ttyAMA0,115200 kgdboc=ttyAMA1,115200). > > The following drivers automatically convert '\n' to '\r\n' inside their > own poll functions but the remaining seventeen do not: > > serial8250, cpm, pch_uart, serial_pxa, serial_txx9, > > This can be made fully consistent but performing the conversion in > uart_poll_put_char(). A similar conversion is already made inside > uart_console_write() but it is optional for drivers to use this > function. Fortunately we can be confident the translation is safe > because the (very common) 8250 already does this translation. I'll have to take a look at some of the other drivers. If all the instances of the function calls are going to coded per driver, it might make more sense to add variable to struct uart_port, vs changing the number of arguments to uart_poll_put_char. And then the default can simply be coded in the struct initialization to the most common need. Cheers, Jason.
On 14/05/14 16:08, Jason Wessel wrote: > On 05/14/2014 09:55 AM, Daniel Thompson wrote: >> The behaviour of the UART poll_put_char infrastructure is inconsistent >> with respect to linefeed conversions. This in turn leads to difficulty >> using kdb on serial ports that are not also consoles >> (e.g. console=ttyAMA0,115200 kgdboc=ttyAMA1,115200). >> >> The following drivers automatically convert '\n' to '\r\n' inside their >> own poll functions but the remaining seventeen do not: >> >> serial8250, cpm, pch_uart, serial_pxa, serial_txx9, >> >> This can be made fully consistent but performing the conversion in >> uart_poll_put_char(). A similar conversion is already made inside >> uart_console_write() but it is optional for drivers to use this >> function. Fortunately we can be confident the translation is safe >> because the (very common) 8250 already does this translation. > > > I'll have to take a look at some of the other drivers. If all the > instances of the function calls are going to coded per driver, it might > make more sense to add variable to struct uart_port, vs changing the > number of arguments to uart_poll_put_char. And then the default can > simply be coded in the struct initialization to the most common need. I'm proposing a very simply approach: unconditionally make all serial drivers behave like the 8250. Detailed reasoning is: 1. Making the polled serial drivers behave consistently is good for transferring mainstream testing to less commonly used UARTs, 2. The 8250 gets best test coverage so it is probably the best behaviour to standardize on, 3. kdb normally sends characters to the user using console I/O rather than polled I/O and almost all serial drivers automatically convert linefeeds in the console I/O, 4. kgdb never generates a linefeed character. If it did kgdb would not currently work on 8250 UARTs (its true that I have assumed, whilst reasoning about potential regressions, that is does). To be absolutely sure of #4 I did a full code review this morning and confirmed that all code that sends arbitrary data to the gdbserver converts the raw data to hex first. Note also that if, in the future, kgdb does ever implement the "modern" gdbserver commands that utilize raw 8-bit data it would still be possible for kgdb to escape linefeeds to ensure arbitrary binary data can be safely transferred. Daniel.
On Wed, May 14, 2014 at 03:55:32PM +0100, Daniel Thompson wrote: > The behaviour of the UART poll_put_char infrastructure is inconsistent > with respect to linefeed conversions. This in turn leads to difficulty > using kdb on serial ports that are not also consoles > (e.g. console=ttyAMA0,115200 kgdboc=ttyAMA1,115200). > > The following drivers automatically convert '\n' to '\r\n' inside their > own poll functions but the remaining seventeen do not: > > serial8250, cpm, pch_uart, serial_pxa, serial_txx9, > > This can be made fully consistent but performing the conversion in > uart_poll_put_char(). A similar conversion is already made inside > uart_console_write() but it is optional for drivers to use this > function. Fortunately we can be confident the translation is safe > because the (very common) 8250 already does this translation. > > Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org> > Cc: Kumar Gala <galak@kernel.crashing.org> > Cc: Pantelis Antoniou <panto@intracom.gr> > Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com> > Cc: Joe Schultz <jschultz@xes-inc.com> > Cc: Loic Poulain <loic.poulain@intel.com> > Cc: Kyle McMartin <kyle@infradead.org> > Cc: Stephen Warren <swarren@nvidia.com> > Cc: Ingo Molnar <mingo@elte.hu> > Cc: Paul Gortmaker <paul.gortmaker@windriver.com> > Cc: Grant Likely <grant.likely@linaro.org> > Cc: Rob Herring <rob.herring@calxeda.com> > Cc: Jingoo Han <jg1.han@samsung.com> > Cc: Christophe Leroy <christophe.leroy@c-s.fr> > --- > drivers/tty/serial/8250/8250_core.c | 5 ----- This patch doesn't apply to my tty-next branch anymore, as it seems that part of it is already there, right? Can you respin this whole series and resend? thanks, greg k-h
This patchset contains a number of fixes to make it possible to use ttyNMIX as the primary console (providing you also have that the additional architecture specific code which is proposed in a different patch set). The first patch fixes a bug in the cpm poll_put_char() driver. This is not strictly related to ttyNMIX but appears in this patch series for historical reasons (it was part of a patch that did impact ttyNMIX but most of that patch is already in the kernel thanks to an independent patch from Doug Anderson). The remaining patches are specific to kgdb_nmi and have no effect on any other part of the kernel. The series has been runtime tested on ARM and compile tested on x86 and powerpc. Changes since v1: - Trimmed down the first patch to the remaining fragments as described above. - Rebased on tty-next as requested by Greg KH. Daniel Thompson (4): serial: cpm_uart: No LF conversion in put_poll_char() serial: kgdb_nmi: Use container_of() to locate private data serial: kgdb_nmi: Switch from tasklets to real timers serial: kgdb_nmi: Improve console integration with KDB I/O drivers/tty/serial/cpm_uart/cpm_uart_core.c | 8 ++-- drivers/tty/serial/kgdb_nmi.c | 59 +++++++++++++---------------- 2 files changed, 31 insertions(+), 36 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c index 2d4bd39..053c200 100644 --- a/drivers/tty/serial/8250/8250_core.c +++ b/drivers/tty/serial/8250/8250_core.c @@ -1926,13 +1926,8 @@ static void serial8250_put_poll_char(struct uart_port *port, wait_for_xmitr(up, BOTH_EMPTY); /* * Send the character out. - * If a LF, also do CR... */ serial_port_out(port, UART_TX, c); - if (c == 10) { - wait_for_xmitr(up, BOTH_EMPTY); - serial_port_out(port, UART_TX, 13); - } /* * Finally, wait for transmitter to become empty diff --git a/drivers/tty/serial/cpm_uart/cpm_uart_core.c b/drivers/tty/serial/cpm_uart/cpm_uart_core.c index 7d76214..aa60e6d 100644 --- a/drivers/tty/serial/cpm_uart/cpm_uart_core.c +++ b/drivers/tty/serial/cpm_uart/cpm_uart_core.c @@ -971,7 +971,7 @@ static void cpm_uart_config_port(struct uart_port *port, int flags) * Note that this is called with interrupts already disabled */ static void cpm_uart_early_write(struct uart_cpm_port *pinfo, - const char *string, u_int count) + const char *string, u_int count, bool handle_linefeed) { unsigned int i; cbd_t __iomem *bdp, *bdbase; @@ -1013,7 +1013,7 @@ static void cpm_uart_early_write(struct uart_cpm_port *pinfo, bdp++; /* if a LF, also do CR... */ - if (*string == 10) { + if (handle_linefeed && *string == 10) { while ((in_be16(&bdp->cbd_sc) & BD_SC_READY) != 0) ; @@ -1111,7 +1111,7 @@ static void cpm_put_poll_char(struct uart_port *port, static char ch[2]; ch[0] = (char)c; - cpm_uart_early_write(pinfo, ch, 1); + cpm_uart_early_write(pinfo, ch, 1, false); } #endif /* CONFIG_CONSOLE_POLL */ @@ -1275,7 +1275,7 @@ static void cpm_uart_console_write(struct console *co, const char *s, spin_lock_irqsave(&pinfo->port.lock, flags); } - cpm_uart_early_write(pinfo, s, count); + cpm_uart_early_write(pinfo, s, count, true); if (unlikely(nolock)) { local_irq_restore(flags); diff --git a/drivers/tty/serial/pch_uart.c b/drivers/tty/serial/pch_uart.c index 0931b3f..11e631d 100644 --- a/drivers/tty/serial/pch_uart.c +++ b/drivers/tty/serial/pch_uart.c @@ -1588,13 +1588,8 @@ static void pch_uart_put_poll_char(struct uart_port *port, wait_for_xmitr(priv, UART_LSR_THRE); /* * Send the character out. - * If a LF, also do CR... */ iowrite8(c, priv->membase + PCH_UART_THR); - if (c == 10) { - wait_for_xmitr(priv, UART_LSR_THRE); - iowrite8(13, priv->membase + PCH_UART_THR); - } /* * Finally, wait for transmitter to become empty diff --git a/drivers/tty/serial/pxa.c b/drivers/tty/serial/pxa.c index f9f20f3..9e7ee39 100644 --- a/drivers/tty/serial/pxa.c +++ b/drivers/tty/serial/pxa.c @@ -711,13 +711,8 @@ static void serial_pxa_put_poll_char(struct uart_port *port, wait_for_xmitr(up); /* * Send the character out. - * If a LF, also do CR... */ serial_out(up, UART_TX, c); - if (c == 10) { - wait_for_xmitr(up); - serial_out(up, UART_TX, 13); - } /* * Finally, wait for transmitter to become empty diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c index b68550d..50167bf 100644 --- a/drivers/tty/serial/serial_core.c +++ b/drivers/tty/serial/serial_core.c @@ -2239,6 +2239,8 @@ static void uart_poll_put_char(struct tty_driver *driver, int line, char ch) return; port = state->uart_port; + if (ch == '\n') + port->ops->poll_put_char(port, '\r'); port->ops->poll_put_char(port, ch); } #endif diff --git a/drivers/tty/serial/serial_txx9.c b/drivers/tty/serial/serial_txx9.c index 90a080b..60f49b9 100644 --- a/drivers/tty/serial/serial_txx9.c +++ b/drivers/tty/serial/serial_txx9.c @@ -535,13 +535,8 @@ static void serial_txx9_put_poll_char(struct uart_port *port, unsigned char c) wait_for_xmitr(up); /* * Send the character out. - * If a LF, also do CR... */ sio_out(up, TXX9_SITFIFO, c); - if (c == 10) { - wait_for_xmitr(up); - sio_out(up, TXX9_SITFIFO, 13); - } /* * Finally, wait for transmitter to become empty
The behaviour of the UART poll_put_char infrastructure is inconsistent with respect to linefeed conversions. This in turn leads to difficulty using kdb on serial ports that are not also consoles (e.g. console=ttyAMA0,115200 kgdboc=ttyAMA1,115200). The following drivers automatically convert '\n' to '\r\n' inside their own poll functions but the remaining seventeen do not: serial8250, cpm, pch_uart, serial_pxa, serial_txx9, This can be made fully consistent but performing the conversion in uart_poll_put_char(). A similar conversion is already made inside uart_console_write() but it is optional for drivers to use this function. Fortunately we can be confident the translation is safe because the (very common) 8250 already does this translation. Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org> Cc: Kumar Gala <galak@kernel.crashing.org> Cc: Pantelis Antoniou <panto@intracom.gr> Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com> Cc: Joe Schultz <jschultz@xes-inc.com> Cc: Loic Poulain <loic.poulain@intel.com> Cc: Kyle McMartin <kyle@infradead.org> Cc: Stephen Warren <swarren@nvidia.com> Cc: Ingo Molnar <mingo@elte.hu> Cc: Paul Gortmaker <paul.gortmaker@windriver.com> Cc: Grant Likely <grant.likely@linaro.org> Cc: Rob Herring <rob.herring@calxeda.com> Cc: Jingoo Han <jg1.han@samsung.com> Cc: Christophe Leroy <christophe.leroy@c-s.fr> --- drivers/tty/serial/8250/8250_core.c | 5 ----- drivers/tty/serial/cpm_uart/cpm_uart_core.c | 8 ++++---- drivers/tty/serial/pch_uart.c | 5 ----- drivers/tty/serial/pxa.c | 5 ----- drivers/tty/serial/serial_core.c | 2 ++ drivers/tty/serial/serial_txx9.c | 5 ----- 6 files changed, 6 insertions(+), 24 deletions(-)