[1/4] serial: core: Consistent LF handling for poll_put_char

Message ID 1400079335-32125-2-git-send-email-daniel.thompson@linaro.org
State New
Headers show

Commit Message

Daniel Thompson May 14, 2014, 2:55 p.m.
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(-)

Comments

Jason Wessel May 14, 2014, 3:08 p.m. | #1
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.
Daniel Thompson May 15, 2014, 11:06 a.m. | #2
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.
Greg Kroah-Hartman May 28, 2014, 8:03 p.m. | #3
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
Daniel Thompson May 29, 2014, 8:48 a.m. | #4
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(-)

Patch hide | download patch | download mbox

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