[PATCH 0/4] serial: kgdb_nmi: Allow ttyNMIX to act as primary console

[PATCH 0/4] serial: kgdb_nmi: Allow ttyNMIX to act as primary console

[Daniel Thompson]

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 to support it.

The first patch is a generic improvement to make the behaviour of
poll_put_char consistent for all UARTs. This is especially important
for kgdb_nmi, which makes unusually heavy use of poll_put_char,
although this fix also benefits other users of polled I/O.

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.

Daniel Thompson (4):
  serial: core: Consistent LF handling for poll_put_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/8250/8250_core.c         |  5 ---
 drivers/tty/serial/cpm_uart/cpm_uart_core.c |  8 ++--
 drivers/tty/serial/kgdb_nmi.c               | 59 +++++++++++++----------------
 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 ---
 7 files changed, 33 insertions(+), 56 deletions(-)

-- 
1.9.0

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

[Daniel Thompson]

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(-)

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
-- 
1.9.0

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

[Daniel Thompson]

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(-)

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
-- 
1.9.0

[PATCH 2/4] serial: kgdb_nmi: Use container_of() to locate private data

[Daniel Thompson]

This corrects a crash in kgdb_nmi_tty_shutdown() which occurs when
the function is called with port->tty set to NULL.

All conversions between struct tty_port and struct kgdb_nmi_tty_priv
have been switched to direct calls to container_of() to improve code
clarity and consistancy.

Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
---
 drivers/tty/serial/kgdb_nmi.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/tty/serial/kgdb_nmi.c b/drivers/tty/serial/kgdb_nmi.c
index 5f673b7..d51b2a1 100644
--- a/drivers/tty/serial/kgdb_nmi.c
+++ b/drivers/tty/serial/kgdb_nmi.c
@@ -84,11 +84,6 @@ struct kgdb_nmi_tty_priv {
 	STRUCT_KFIFO(char, KGDB_NMI_FIFO_SIZE) fifo;
 };
 
-static struct kgdb_nmi_tty_priv *kgdb_nmi_port_to_priv(struct tty_port *port)
-{
-	return container_of(port, struct kgdb_nmi_tty_priv, port);
-}
-
 /*
  * Our debugging console is polled in a tasklet, so we'll check for input
  * every tick. In HZ-less mode, we should program the next tick.  We have
@@ -118,7 +113,7 @@ static void kgdb_tty_recv(int ch)
 	 * do that, and actually, we can't: we're in NMI context, no locks are
 	 * possible.
 	 */
-	priv = kgdb_nmi_port_to_priv(kgdb_nmi_port);
+	priv = container_of(kgdb_nmi_port, struct kgdb_nmi_tty_priv, port);
 	kfifo_in(&priv->fifo, &c, 1);
 	kgdb_tty_poke();
 }
@@ -216,7 +211,8 @@ static void kgdb_nmi_tty_receiver(unsigned long data)
 
 static int kgdb_nmi_tty_activate(struct tty_port *port, struct tty_struct *tty)
 {
-	struct kgdb_nmi_tty_priv *priv = tty->driver_data;
+	struct kgdb_nmi_tty_priv *priv =
+	    container_of(port, struct kgdb_nmi_tty_priv, port);
 
 	kgdb_nmi_port = port;
 	tasklet_schedule(&priv->tlet);
@@ -225,7 +221,8 @@ static int kgdb_nmi_tty_activate(struct tty_port *port, struct tty_struct *tty)
 
 static void kgdb_nmi_tty_shutdown(struct tty_port *port)
 {
-	struct kgdb_nmi_tty_priv *priv = port->tty->driver_data;
+	struct kgdb_nmi_tty_priv *priv =
+	    container_of(port, struct kgdb_nmi_tty_priv, port);
 
 	tasklet_kill(&priv->tlet);
 	kgdb_nmi_port = NULL;
-- 
1.9.0

[PATCH 3/4] serial: kgdb_nmi: Switch from tasklets to real timers

[Daniel Thompson]

kgdb_nmi uses tasklets on the assumption they will not be scheduled
until the next timer tick. This assumption is invalid and can lead to
live lock, continually servicing the kgdb_nmi tasklet. This is fixed
by using the timer API instead.

Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
---
 drivers/tty/serial/kgdb_nmi.c | 30 +++++++++---------------------
 1 file changed, 9 insertions(+), 21 deletions(-)

diff --git a/drivers/tty/serial/kgdb_nmi.c b/drivers/tty/serial/kgdb_nmi.c
index d51b2a1..20d21d0 100644
--- a/drivers/tty/serial/kgdb_nmi.c
+++ b/drivers/tty/serial/kgdb_nmi.c
@@ -80,24 +80,10 @@ static struct console kgdb_nmi_console = {
 
 struct kgdb_nmi_tty_priv {
 	struct tty_port port;
-	struct tasklet_struct tlet;
+	struct timer_list timer;
 	STRUCT_KFIFO(char, KGDB_NMI_FIFO_SIZE) fifo;
 };
 
-/*
- * Our debugging console is polled in a tasklet, so we'll check for input
- * every tick. In HZ-less mode, we should program the next tick.  We have
- * to use the lowlevel stuff as no locks should be grabbed.
- */
-#ifdef CONFIG_HIGH_RES_TIMERS
-static void kgdb_tty_poke(void)
-{
-	tick_program_event(ktime_get(), 0);
-}
-#else
-static inline void kgdb_tty_poke(void) {}
-#endif
-
 static struct tty_port *kgdb_nmi_port;
 
 static void kgdb_tty_recv(int ch)
@@ -108,14 +94,13 @@ static void kgdb_tty_recv(int ch)
 	if (!kgdb_nmi_port || ch < 0)
 		return;
 	/*
-	 * Can't use port->tty->driver_data as tty might be not there. Tasklet
+	 * Can't use port->tty->driver_data as tty might be not there. Timer
 	 * will check for tty and will get the ref, but here we don't have to
 	 * do that, and actually, we can't: we're in NMI context, no locks are
 	 * possible.
 	 */
 	priv = container_of(kgdb_nmi_port, struct kgdb_nmi_tty_priv, port);
 	kfifo_in(&priv->fifo, &c, 1);
-	kgdb_tty_poke();
 }
 
 static int kgdb_nmi_poll_one_knock(void)
@@ -199,7 +184,8 @@ static void kgdb_nmi_tty_receiver(unsigned long data)
 	struct kgdb_nmi_tty_priv *priv = (void *)data;
 	char ch;
 
-	tasklet_schedule(&priv->tlet);
+	priv->timer.expires = jiffies + (HZ/100);
+	add_timer(&priv->timer);
 
 	if (likely(!kgdb_nmi_tty_enabled || !kfifo_len(&priv->fifo)))
 		return;
@@ -215,7 +201,9 @@ static int kgdb_nmi_tty_activate(struct tty_port *port, struct tty_struct *tty)
 	    container_of(port, struct kgdb_nmi_tty_priv, port);
 
 	kgdb_nmi_port = port;
-	tasklet_schedule(&priv->tlet);
+	priv->timer.expires = jiffies + (HZ/100);
+	add_timer(&priv->timer);
+
 	return 0;
 }
 
@@ -224,7 +212,7 @@ static void kgdb_nmi_tty_shutdown(struct tty_port *port)
 	struct kgdb_nmi_tty_priv *priv =
 	    container_of(port, struct kgdb_nmi_tty_priv, port);
 
-	tasklet_kill(&priv->tlet);
+	del_timer(&priv->timer);
 	kgdb_nmi_port = NULL;
 }
 
@@ -243,7 +231,7 @@ static int kgdb_nmi_tty_install(struct tty_driver *drv, struct tty_struct *tty)
 		return -ENOMEM;
 
 	INIT_KFIFO(priv->fifo);
-	tasklet_init(&priv->tlet, kgdb_nmi_tty_receiver, (unsigned long)priv);
+	setup_timer(&priv->timer, kgdb_nmi_tty_receiver, (unsigned long)priv);
 	tty_port_init(&priv->port);
 	priv->port.ops = &kgdb_nmi_tty_port_ops;
 	tty->driver_data = priv;
-- 
1.9.0

[PATCH 4/4] serial: kgdb_nmi: Improve console integration with KDB I/O

[Daniel Thompson]

kgdb_nmi_tty_enabled is used for two unrelated purposes, namely to
suppress normal TTY input handling and to suppress console output
(although it has no effect at all on TTY output). A much better way to
handle muting the console is to not have to mute it in the first place!
That's what this patch does.

Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
---
 drivers/tty/serial/kgdb_nmi.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/kgdb_nmi.c b/drivers/tty/serial/kgdb_nmi.c
index 20d21d0..cfadf29 100644
--- a/drivers/tty/serial/kgdb_nmi.c
+++ b/drivers/tty/serial/kgdb_nmi.c
@@ -44,13 +44,22 @@ MODULE_PARM_DESC(magic, "magic sequence to enter NMI debugger (default $3#33)");
 
 static bool kgdb_nmi_tty_enabled;
 
+static int kgdb_nmi_console_setup(struct console *co, char *options)
+{
+	/* The NMI console uses the dbg_io_ops to issue console messages. To
+	 * avoid duplicate messages during kdb sessions we must inform kdb's
+	 * I/O utilities that messages sent to the console will automatically
+	 * be displayed on the dbg_io.
+	 */
+	dbg_io_ops->is_console = true;
+
+	return 0;
+}
+
 static void kgdb_nmi_console_write(struct console *co, const char *s, uint c)
 {
 	int i;
 
-	if (!kgdb_nmi_tty_enabled || atomic_read(&kgdb_active) >= 0)
-		return;
-
 	for (i = 0; i < c; i++)
 		dbg_io_ops->write_char(s[i]);
 }
@@ -65,6 +74,7 @@ static struct tty_driver *kgdb_nmi_console_device(struct console *co, int *idx)
 
 static struct console kgdb_nmi_console = {
 	.name	= "ttyNMI",
+	.setup  = kgdb_nmi_console_setup,
 	.write	= kgdb_nmi_console_write,
 	.device	= kgdb_nmi_console_device,
 	.flags	= CON_PRINTBUFFER | CON_ANYTIME | CON_ENABLED,
-- 
1.9.0

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

[Jason Wessel]

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.

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

[Jason Wessel]

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.

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

[Daniel Thompson]

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.

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

[Greg Kroah-Hartman]

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

[PATCH tty-next v2 0/4] serial: kgdb_nmi: Allow ttyNMIX to act as primary console

[Daniel Thompson]

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(-)

-- 
1.9.0

[PATCH tty-next v2 1/4] serial: cpm_uart: No LF conversion in put_poll_char()

[Daniel Thompson]

In (c7d44a02a serial_core: Commonalize crlf when working w/ a non open
console port) the core was modified to make the UART poll_put_char()
automatically convert LF to CRLF. This driver's poll_put_char() adds a
CR itself and this was not disabled by the above patch meaning
currently it sends two CR characters.

The code to issue a character is shared by the console write code (where
driver must do LF to CRLF conversion, although it can make use of the
uart_console_write() helper function) and the poll_put_char (where
driver must not do the conversion). For that reason we add a flag rather
than simply rip out the conversion code.

Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
Cc: Doug Anderson <dianders@chromium.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jslaby@suse.cz>
Cc: Christophe Leroy <christophe.leroy@c-s.fr>
Cc: linux-serial@vger.kernel.org
---
 drivers/tty/serial/cpm_uart/cpm_uart_core.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

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);
-- 
1.9.0

[PATCH tty-next v2 2/4] serial: kgdb_nmi: Use container_of() to locate private data

[Daniel Thompson]

This corrects a crash in kgdb_nmi_tty_shutdown() which occurs when
the function is called with port->tty set to NULL.

All conversions between struct tty_port and struct kgdb_nmi_tty_priv
have been switched to direct calls to container_of() to improve code
clarity and consistancy.

Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
---
 drivers/tty/serial/kgdb_nmi.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/tty/serial/kgdb_nmi.c b/drivers/tty/serial/kgdb_nmi.c
index 5f673b7..d51b2a1 100644
--- a/drivers/tty/serial/kgdb_nmi.c
+++ b/drivers/tty/serial/kgdb_nmi.c
@@ -84,11 +84,6 @@ struct kgdb_nmi_tty_priv {
 	STRUCT_KFIFO(char, KGDB_NMI_FIFO_SIZE) fifo;
 };
 
-static struct kgdb_nmi_tty_priv *kgdb_nmi_port_to_priv(struct tty_port *port)
-{
-	return container_of(port, struct kgdb_nmi_tty_priv, port);
-}
-
 /*
  * Our debugging console is polled in a tasklet, so we'll check for input
  * every tick. In HZ-less mode, we should program the next tick.  We have
@@ -118,7 +113,7 @@ static void kgdb_tty_recv(int ch)
 	 * do that, and actually, we can't: we're in NMI context, no locks are
 	 * possible.
 	 */
-	priv = kgdb_nmi_port_to_priv(kgdb_nmi_port);
+	priv = container_of(kgdb_nmi_port, struct kgdb_nmi_tty_priv, port);
 	kfifo_in(&priv->fifo, &c, 1);
 	kgdb_tty_poke();
 }
@@ -216,7 +211,8 @@ static void kgdb_nmi_tty_receiver(unsigned long data)
 
 static int kgdb_nmi_tty_activate(struct tty_port *port, struct tty_struct *tty)
 {
-	struct kgdb_nmi_tty_priv *priv = tty->driver_data;
+	struct kgdb_nmi_tty_priv *priv =
+	    container_of(port, struct kgdb_nmi_tty_priv, port);
 
 	kgdb_nmi_port = port;
 	tasklet_schedule(&priv->tlet);
@@ -225,7 +221,8 @@ static int kgdb_nmi_tty_activate(struct tty_port *port, struct tty_struct *tty)
 
 static void kgdb_nmi_tty_shutdown(struct tty_port *port)
 {
-	struct kgdb_nmi_tty_priv *priv = port->tty->driver_data;
+	struct kgdb_nmi_tty_priv *priv =
+	    container_of(port, struct kgdb_nmi_tty_priv, port);
 
 	tasklet_kill(&priv->tlet);
 	kgdb_nmi_port = NULL;
-- 
1.9.0

[PATCH tty-next v2 3/4] serial: kgdb_nmi: Switch from tasklets to real timers

[Daniel Thompson]

kgdb_nmi uses tasklets on the assumption they will not be scheduled
until the next timer tick. This assumption is invalid and can lead to
live lock, continually servicing the kgdb_nmi tasklet. This is fixed
by using the timer API instead.

Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
---
 drivers/tty/serial/kgdb_nmi.c | 30 +++++++++---------------------
 1 file changed, 9 insertions(+), 21 deletions(-)

diff --git a/drivers/tty/serial/kgdb_nmi.c b/drivers/tty/serial/kgdb_nmi.c
index d51b2a1..20d21d0 100644
--- a/drivers/tty/serial/kgdb_nmi.c
+++ b/drivers/tty/serial/kgdb_nmi.c
@@ -80,24 +80,10 @@ static struct console kgdb_nmi_console = {
 
 struct kgdb_nmi_tty_priv {
 	struct tty_port port;
-	struct tasklet_struct tlet;
+	struct timer_list timer;
 	STRUCT_KFIFO(char, KGDB_NMI_FIFO_SIZE) fifo;
 };
 
-/*
- * Our debugging console is polled in a tasklet, so we'll check for input
- * every tick. In HZ-less mode, we should program the next tick.  We have
- * to use the lowlevel stuff as no locks should be grabbed.
- */
-#ifdef CONFIG_HIGH_RES_TIMERS
-static void kgdb_tty_poke(void)
-{
-	tick_program_event(ktime_get(), 0);
-}
-#else
-static inline void kgdb_tty_poke(void) {}
-#endif
-
 static struct tty_port *kgdb_nmi_port;
 
 static void kgdb_tty_recv(int ch)
@@ -108,14 +94,13 @@ static void kgdb_tty_recv(int ch)
 	if (!kgdb_nmi_port || ch < 0)
 		return;
 	/*
-	 * Can't use port->tty->driver_data as tty might be not there. Tasklet
+	 * Can't use port->tty->driver_data as tty might be not there. Timer
 	 * will check for tty and will get the ref, but here we don't have to
 	 * do that, and actually, we can't: we're in NMI context, no locks are
 	 * possible.
 	 */
 	priv = container_of(kgdb_nmi_port, struct kgdb_nmi_tty_priv, port);
 	kfifo_in(&priv->fifo, &c, 1);
-	kgdb_tty_poke();
 }
 
 static int kgdb_nmi_poll_one_knock(void)
@@ -199,7 +184,8 @@ static void kgdb_nmi_tty_receiver(unsigned long data)
 	struct kgdb_nmi_tty_priv *priv = (void *)data;
 	char ch;
 
-	tasklet_schedule(&priv->tlet);
+	priv->timer.expires = jiffies + (HZ/100);
+	add_timer(&priv->timer);
 
 	if (likely(!kgdb_nmi_tty_enabled || !kfifo_len(&priv->fifo)))
 		return;
@@ -215,7 +201,9 @@ static int kgdb_nmi_tty_activate(struct tty_port *port, struct tty_struct *tty)
 	    container_of(port, struct kgdb_nmi_tty_priv, port);
 
 	kgdb_nmi_port = port;
-	tasklet_schedule(&priv->tlet);
+	priv->timer.expires = jiffies + (HZ/100);
+	add_timer(&priv->timer);
+
 	return 0;
 }
 
@@ -224,7 +212,7 @@ static void kgdb_nmi_tty_shutdown(struct tty_port *port)
 	struct kgdb_nmi_tty_priv *priv =
 	    container_of(port, struct kgdb_nmi_tty_priv, port);
 
-	tasklet_kill(&priv->tlet);
+	del_timer(&priv->timer);
 	kgdb_nmi_port = NULL;
 }
 
@@ -243,7 +231,7 @@ static int kgdb_nmi_tty_install(struct tty_driver *drv, struct tty_struct *tty)
 		return -ENOMEM;
 
 	INIT_KFIFO(priv->fifo);
-	tasklet_init(&priv->tlet, kgdb_nmi_tty_receiver, (unsigned long)priv);
+	setup_timer(&priv->timer, kgdb_nmi_tty_receiver, (unsigned long)priv);
 	tty_port_init(&priv->port);
 	priv->port.ops = &kgdb_nmi_tty_port_ops;
 	tty->driver_data = priv;
-- 
1.9.0

[PATCH tty-next v2 4/4] serial: kgdb_nmi: Improve console integration with KDB I/O

[Daniel Thompson]

kgdb_nmi_tty_enabled is used for two unrelated purposes, namely to
suppress normal TTY input handling and to suppress console output
(although it has no effect at all on TTY output). A much better way to
handle muting the console is to not have to mute it in the first place!
That's what this patch does.

Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
---
 drivers/tty/serial/kgdb_nmi.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/kgdb_nmi.c b/drivers/tty/serial/kgdb_nmi.c
index 20d21d0..cfadf29 100644
--- a/drivers/tty/serial/kgdb_nmi.c
+++ b/drivers/tty/serial/kgdb_nmi.c
@@ -44,13 +44,22 @@ MODULE_PARM_DESC(magic, "magic sequence to enter NMI debugger (default $3#33)");
 
 static bool kgdb_nmi_tty_enabled;
 
+static int kgdb_nmi_console_setup(struct console *co, char *options)
+{
+	/* The NMI console uses the dbg_io_ops to issue console messages. To
+	 * avoid duplicate messages during kdb sessions we must inform kdb's
+	 * I/O utilities that messages sent to the console will automatically
+	 * be displayed on the dbg_io.
+	 */
+	dbg_io_ops->is_console = true;
+
+	return 0;
+}
+
 static void kgdb_nmi_console_write(struct console *co, const char *s, uint c)
 {
 	int i;
 
-	if (!kgdb_nmi_tty_enabled || atomic_read(&kgdb_active) >= 0)
-		return;
-
 	for (i = 0; i < c; i++)
 		dbg_io_ops->write_char(s[i]);
 }
@@ -65,6 +74,7 @@ static struct tty_driver *kgdb_nmi_console_device(struct console *co, int *idx)
 
 static struct console kgdb_nmi_console = {
 	.name	= "ttyNMI",
+	.setup  = kgdb_nmi_console_setup,
 	.write	= kgdb_nmi_console_write,
 	.device	= kgdb_nmi_console_device,
 	.flags	= CON_PRINTBUFFER | CON_ANYTIME | CON_ENABLED,
-- 
1.9.0