[PATCH 0/7] Enable support for kgdb NMI console feature

[PATCH 0/7] Enable support for kgdb NMI console feature

[Sumit Garg]

This work is derived from Daniel's prior work here [1]. It has been
rebased (tag: kgdb-5.8-rc1 + console hardening patch-set[2]), reworked
to use serial RX interrupt as NMI (pseudo NMI on aarch64) in order to
drop into debugger and tested on Developerbox (using amba-pl011).

- Patch #1 is more of a fix required for NMI console to replace kgdb IO
  console.
- Patches #2 to #6 adds an architecture agnostic fallback mechanism to
  enable kgdb NMI console using serial RX interrupt as NMI.
- Patch #7 is an optimization patch that gets rid of inefficient timer
  based tasklet and rather uses irq_work.

Usage of kgdb NMI console:
- Enable "CONFIG_SERIAL_KGDB_NMI".
- Kernel cmdline modification for Developerbox:
   console=ttyNMI0 kgdboc=ttyAMA0

[1] https://git.linaro.org/people/daniel.thompson/linux.git/log/?h=kgdb/polled_request_irq
[2] https://lkml.org/lkml/2020/6/4/294

Daniel Thompson (5):
  tty: serial: Add poll_get_irq() to the polling interface
  kgdb: Add request_nmi() to the io ops table for kgdboc
  serial: kgdb_nmi: Add support for interrupt based fallback
  serial: 8250: Implement poll_get_irq() interface
  serial: kgdb_nmi: Replace hrtimer with irq_work ping

Sumit Garg (2):
  serial: kgdb_nmi: Allow NMI console to replace kgdb IO console
  serial: amba-pl011: Implement poll_get_irq() interface

 drivers/tty/serial/8250/8250_port.c |  16 ++++++
 drivers/tty/serial/amba-pl011.c     |  12 +++++
 drivers/tty/serial/kgdb_nmi.c       | 100 ++++++++++++++++++++++++------------
 drivers/tty/serial/kgdboc.c         |  35 +++++++++++++
 drivers/tty/serial/serial_core.c    |  18 +++++++
 include/linux/kgdb.h                |   7 +++
 include/linux/serial_core.h         |   1 +
 include/linux/tty_driver.h          |   1 +
 8 files changed, 158 insertions(+), 32 deletions(-)

-- 
2.7.4

[PATCH 1/7] serial: kgdb_nmi: Allow NMI console to replace kgdb IO console

[Sumit Garg]

Traditionally, kgdb NMI console relied on cmdline option "console=" to
enable/disable consoles. But it didn't took into account DT/ACPI modes
which can also provide default preferred console that can be enabled
prior to kgdb NMI console. And if that default preferred console is
used for debug IO operations as well then it will lead to duplicate
consoles representing same physical serial device which in turn leads
to duplicate printk messages.

In order to avoid this duplication, we need to disable/unregister debug
IO console in case the NMI console is enabled successfully. Also, we
wouldn't like to see beginning boot messages twice, so we need to
remove flag: CON_PRINTBUFFER prior to NMI console registration.

Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
---
 drivers/tty/serial/kgdb_nmi.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/tty/serial/kgdb_nmi.c b/drivers/tty/serial/kgdb_nmi.c
index 6004c0c..b32c6b1 100644
--- a/drivers/tty/serial/kgdb_nmi.c
+++ b/drivers/tty/serial/kgdb_nmi.c
@@ -40,6 +40,7 @@ module_param_named(magic, kgdb_nmi_magic, charp, 0600);
 MODULE_PARM_DESC(magic, "magic sequence to enter NMI debugger (default $3#33)");
 
 static atomic_t kgdb_nmi_num_readers = ATOMIC_INIT(0);
+static struct console *orig_dbg_cons;
 
 static int kgdb_nmi_console_setup(struct console *co, char *options)
 {
@@ -352,8 +353,22 @@ int kgdb_register_nmi_console(void)
 		goto err_drv_reg;
 	}
 
+	/*
+	 * If we already have an active debug IO console, and are switching
+	 * to a NMI console, don't print everything out again, since debug IO
+	 * console, and the NMI console are the same physical device, it's
+	 * annoying to see the beginning boot messages twice.
+	 */
+	if (dbg_io_ops->cons && (dbg_io_ops->cons->flags & CON_ENABLED)) {
+		orig_dbg_cons = dbg_io_ops->cons;
+		kgdb_nmi_console.flags &= ~CON_PRINTBUFFER;
+	}
+
 	register_console(&kgdb_nmi_console);
 
+	if (orig_dbg_cons && (kgdb_nmi_console.flags & CON_ENABLED))
+		unregister_console(orig_dbg_cons);
+
 	return 0;
 err_drv_reg:
 	put_tty_driver(kgdb_nmi_tty_driver);
@@ -373,6 +388,9 @@ int kgdb_unregister_nmi_console(void)
 	if (ret)
 		return ret;
 
+	if (orig_dbg_cons)
+		register_console(orig_dbg_cons);
+
 	ret = tty_unregister_driver(kgdb_nmi_tty_driver);
 	if (ret)
 		return ret;
-- 
2.7.4

[PATCH 2/7] tty: serial: Add poll_get_irq() to the polling interface

[Sumit Garg]

From: Daniel Thompson <daniel.thompson@linaro.org>

Add new API: poll_get_irq() to the polling interface in order for user
of polling interface to retrieve allocated IRQ corresponding to
underlying serial device.

Although, serial interface still works in polling mode but interrupt
associated with serial device can be leveraged for special purposes like
debugger(kgdb) entry.

Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
---
 drivers/tty/serial/serial_core.c | 18 ++++++++++++++++++
 include/linux/serial_core.h      |  1 +
 include/linux/tty_driver.h       |  1 +
 3 files changed, 20 insertions(+)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 66a5e2f..1bb033c 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2470,6 +2470,23 @@ static void uart_poll_put_char(struct tty_driver *driver, int line, char ch)
 	port->ops->poll_put_char(port, ch);
 	uart_port_deref(port);
 }
+
+static int uart_poll_get_irq(struct tty_driver *driver, int line)
+{
+	struct uart_driver *drv = driver->driver_state;
+	struct uart_state *state = drv->state + line;
+	struct uart_port *port;
+	int ret = -ENODEV;
+
+	port = uart_port_ref(state);
+	if (port && port->ops->poll_get_irq) {
+		ret = port->ops->poll_get_irq(port);
+		uart_port_deref(port);
+	}
+
+	return ret;
+}
+
 #endif
 
 static const struct tty_operations uart_ops = {
@@ -2505,6 +2522,7 @@ static const struct tty_operations uart_ops = {
 	.poll_init	= uart_poll_init,
 	.poll_get_char	= uart_poll_get_char,
 	.poll_put_char	= uart_poll_put_char,
+	.poll_get_irq	= uart_poll_get_irq,
 #endif
 };
 
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 92f5eba..8b132e6 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -78,6 +78,7 @@ struct uart_ops {
 	int		(*poll_init)(struct uart_port *);
 	void		(*poll_put_char)(struct uart_port *, unsigned char);
 	int		(*poll_get_char)(struct uart_port *);
+	int		(*poll_get_irq)(struct uart_port *);
 #endif
 };
 
diff --git a/include/linux/tty_driver.h b/include/linux/tty_driver.h
index 3584462..d6da5c5 100644
--- a/include/linux/tty_driver.h
+++ b/include/linux/tty_driver.h
@@ -295,6 +295,7 @@ struct tty_operations {
 	int (*poll_init)(struct tty_driver *driver, int line, char *options);
 	int (*poll_get_char)(struct tty_driver *driver, int line);
 	void (*poll_put_char)(struct tty_driver *driver, int line, char ch);
+	int (*poll_get_irq)(struct tty_driver *driver, int line);
 #endif
 	int (*proc_show)(struct seq_file *, void *);
 } __randomize_layout;
-- 
2.7.4

[PATCH 3/7] kgdb: Add request_nmi() to the io ops table for kgdboc

[Sumit Garg]

From: Daniel Thompson <daniel.thompson@linaro.org>

Add request_nmi() callback to install a non-maskable interrupt handler
corresponding to IRQ retrieved from polling interface. If NMI handler
installation fails due to missing support from underlying irqchip driver
then fallback to install it as normal interrupt handler.

Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
Co-developed-by: Sumit Garg <sumit.garg@linaro.org>
Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
---
 drivers/tty/serial/kgdboc.c | 35 +++++++++++++++++++++++++++++++++++
 include/linux/kgdb.h        |  7 +++++++
 2 files changed, 42 insertions(+)

diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c
index 84ffede..263afae 100644
--- a/drivers/tty/serial/kgdboc.c
+++ b/drivers/tty/serial/kgdboc.c
@@ -19,6 +19,9 @@
 #include <linux/console.h>
 #include <linux/vt_kern.h>
 #include <linux/input.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/irqdesc.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/serial_core.h>
@@ -390,12 +393,44 @@ static void kgdboc_post_exp_handler(void)
 	kgdboc_restore_input();
 }
 
+static int kgdb_tty_irq;
+
+static int kgdboc_request_nmi(irq_handler_t fn, void *dev_id)
+{
+	int irq, res;
+
+	/* Better to avoid double allocation in the tty driver! */
+	if (kgdb_tty_irq)
+		return 0;
+
+	if (!kgdb_tty_driver->ops->poll_get_irq)
+		return -ENODEV;
+
+	irq =
+	    kgdb_tty_driver->ops->poll_get_irq(kgdb_tty_driver, kgdb_tty_line);
+	if (irq <= 0)
+		return irq ? irq : -ENODEV;
+
+	irq_set_status_flags(irq, IRQ_NOAUTOEN);
+	res = request_nmi(irq, fn, IRQF_PERCPU, "kgdboc", dev_id);
+	if (res) {
+		res = request_irq(irq, fn, IRQF_SHARED, "kgdboc", dev_id);
+		WARN_ON(res);
+	}
+
+	enable_irq(irq);
+
+	kgdb_tty_irq = irq;
+	return 0;
+}
+
 static struct kgdb_io kgdboc_io_ops = {
 	.name			= "kgdboc",
 	.read_char		= kgdboc_get_char,
 	.write_char		= kgdboc_put_char,
 	.pre_exception		= kgdboc_pre_exp_handler,
 	.post_exception		= kgdboc_post_exp_handler,
+	.request_nmi		= kgdboc_request_nmi,
 };
 
 #if IS_BUILTIN(CONFIG_KGDB_SERIAL_CONSOLE)
diff --git a/include/linux/kgdb.h b/include/linux/kgdb.h
index 529116b..b32b044 100644
--- a/include/linux/kgdb.h
+++ b/include/linux/kgdb.h
@@ -16,6 +16,7 @@
 #include <linux/linkage.h>
 #include <linux/init.h>
 #include <linux/atomic.h>
+#include <linux/interrupt.h>
 #ifdef CONFIG_HAVE_ARCH_KGDB
 #include <asm/kgdb.h>
 #endif
@@ -276,6 +277,10 @@ struct kgdb_arch {
  * the I/O driver.
  * @post_exception: Pointer to a function that will do any cleanup work
  * for the I/O driver.
+ * @request_nmi: Pointer to a function that can install an non-maskable
+ * interrupt handler that will be called when a character is pending and that
+ * can be cleared by calling @read_char until it returns NO_POLL_CHAR. If NMI
+ * installation fails then fallback to install normal interrupt handler.
  * @cons: valid if the I/O device is a console; else NULL.
  */
 struct kgdb_io {
@@ -287,6 +292,8 @@ struct kgdb_io {
 	void			(*deinit) (void);
 	void			(*pre_exception) (void);
 	void			(*post_exception) (void);
+	int			(*request_nmi)(irq_handler_t nmi_handler,
+					       void *dev_id);
 	struct console		*cons;
 };
 
-- 
2.7.4

[PATCH 4/7] serial: kgdb_nmi: Add support for interrupt based fallback

[Sumit Garg]

From: Daniel Thompson <daniel.thompson@linaro.org>

Add a generic NMI fallback to support kgdb NMI console feature which can
be overridden by arch specific implementation.

This common fallback mechanism utilizes kgdb IO based interrupt in order
to support entry into kgdb if a user types in kgdb_nmi_magic sequence. So
during NMI console init, NMI handler is installed corresponding to kgdb
IO based NMI which is invoked when a character is pending and that can be
cleared by calling @read_char until it returns NO_POLL_CHAR.

Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
Co-developed-by: Sumit Garg <sumit.garg@linaro.org>
Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
---
 drivers/tty/serial/kgdb_nmi.c | 47 ++++++++++++++++++++++++++++++++++++-------
 1 file changed, 40 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/serial/kgdb_nmi.c b/drivers/tty/serial/kgdb_nmi.c
index b32c6b1..2580f39 100644
--- a/drivers/tty/serial/kgdb_nmi.c
+++ b/drivers/tty/serial/kgdb_nmi.c
@@ -42,9 +42,46 @@ MODULE_PARM_DESC(magic, "magic sequence to enter NMI debugger (default $3#33)");
 static atomic_t kgdb_nmi_num_readers = ATOMIC_INIT(0);
 static struct console *orig_dbg_cons;
 
+static int kgdb_nmi_poll_one_knock(void);
+
+static irqreturn_t kgdb_handle_nmi(int irq, void *dev_id)
+{
+	int ret;
+
+	if (kgdb_nmi_knock < 0) {
+		kgdb_breakpoint();
+		return IRQ_HANDLED;
+	}
+
+	ret = kgdb_nmi_poll_one_knock();
+	if (ret == NO_POLL_CHAR)
+		return IRQ_NONE;
+
+	while (ret != 1) {
+		ret = kgdb_nmi_poll_one_knock();
+		if (ret == NO_POLL_CHAR)
+			return IRQ_HANDLED;
+	}
+
+	kgdb_breakpoint();
+	return IRQ_HANDLED;
+}
+
 static int kgdb_nmi_console_setup(struct console *co, char *options)
 {
-	arch_kgdb_ops.enable_nmi(1);
+	int res;
+
+	if (arch_kgdb_ops.enable_nmi) {
+		arch_kgdb_ops.enable_nmi(1);
+	} else if (dbg_io_ops->request_nmi) {
+		res = dbg_io_ops->request_nmi(kgdb_handle_nmi, co);
+		if (res) {
+			pr_err("ttyNMI0: Cannot request nmi/irq\n");
+			return res;
+		}
+	} else {
+		return -ENODEV;
+	}
 
 	/* The NMI console uses the dbg_io_ops to issue console messages. To
 	 * avoid duplicate messages during kdb sessions we must inform kdb's
@@ -328,9 +365,6 @@ int kgdb_register_nmi_console(void)
 {
 	int ret;
 
-	if (!arch_kgdb_ops.enable_nmi)
-		return 0;
-
 	kgdb_nmi_tty_driver = alloc_tty_driver(1);
 	if (!kgdb_nmi_tty_driver) {
 		pr_err("%s: cannot allocate tty\n", __func__);
@@ -380,9 +414,8 @@ int kgdb_unregister_nmi_console(void)
 {
 	int ret;
 
-	if (!arch_kgdb_ops.enable_nmi)
-		return 0;
-	arch_kgdb_ops.enable_nmi(0);
+	if (arch_kgdb_ops.enable_nmi)
+		arch_kgdb_ops.enable_nmi(0);
 
 	ret = unregister_console(&kgdb_nmi_console);
 	if (ret)
-- 
2.7.4

[PATCH 5/7] serial: 8250: Implement poll_get_irq() interface

[Sumit Garg]

From: Daniel Thompson <daniel.thompson@linaro.org>

Support kgdb NMI console feature via implementing poll_get_irq()
interface. This will allow usage of RX interrupts to support kgdb entry
while serial device is operating in polling mode.

Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
---
 drivers/tty/serial/8250/8250_port.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index f77bf82..1473b1a 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -2138,6 +2138,21 @@ static void serial8250_put_poll_char(struct uart_port *port,
 	serial8250_rpm_put(up);
 }
 
+static int serial8250_get_poll_irq(struct uart_port *port)
+{
+	unsigned int ier;
+	struct uart_8250_port *up = up_to_u8250p(port);
+
+	serial8250_rpm_get(up);
+
+	ier = serial_port_in(port, UART_IER);
+	ier |= UART_IER_RLSI | UART_IER_RDI;
+	serial_port_out(port, UART_IER, ier);
+
+	serial8250_rpm_put(up);
+	return port->irq;
+}
+
 #endif /* CONFIG_CONSOLE_POLL */
 
 int serial8250_do_startup(struct uart_port *port)
@@ -3141,6 +3156,7 @@ static const struct uart_ops serial8250_pops = {
 #ifdef CONFIG_CONSOLE_POLL
 	.poll_get_char = serial8250_get_poll_char,
 	.poll_put_char = serial8250_put_poll_char,
+	.poll_get_irq  = serial8250_get_poll_irq,
 #endif
 };
 
-- 
2.7.4

[PATCH 6/7] serial: amba-pl011: Implement poll_get_irq() interface

[Sumit Garg]

Support kgdb NMI console feature via implementing poll_get_irq()
interface. This will allow usage of RX interrupts to support kgdb entry
while serial device is operating in polling mode.

Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
---
 drivers/tty/serial/amba-pl011.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index c010f63..d620d12 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -1637,6 +1637,16 @@ static void pl011_put_poll_char(struct uart_port *port,
 	pl011_write(ch, uap, REG_DR);
 }
 
+static int pl011_get_poll_irq(struct uart_port *port)
+{
+	struct uart_amba_port *uap =
+	    container_of(port, struct uart_amba_port, port);
+
+	pl011_write(UART011_RTIM | UART011_RXIM, uap, REG_IMSC);
+
+	return uap->port.irq;
+}
+
 #endif /* CONFIG_CONSOLE_POLL */
 
 static int pl011_hwinit(struct uart_port *port)
@@ -2145,6 +2155,7 @@ static const struct uart_ops amba_pl011_pops = {
 	.poll_init     = pl011_hwinit,
 	.poll_get_char = pl011_get_poll_char,
 	.poll_put_char = pl011_put_poll_char,
+	.poll_get_irq  = pl011_get_poll_irq,
 #endif
 };
 
@@ -2176,6 +2187,7 @@ static const struct uart_ops sbsa_uart_pops = {
 	.poll_init     = pl011_hwinit,
 	.poll_get_char = pl011_get_poll_char,
 	.poll_put_char = pl011_put_poll_char,
+	.poll_get_irq  = pl011_get_poll_irq,
 #endif
 };
 
-- 
2.7.4

[PATCH 7/7] serial: kgdb_nmi: Replace hrtimer with irq_work ping

[Sumit Garg]

From: Daniel Thompson <daniel.thompson@linaro.org>

Currently kgdb_nmi relies on a 100Hz timer to "defer" work from an NMI
handler. This is essentially doing the job that irq_work is designed to
do but in an odd and inefficient manner. Remove the timer code and
replace it with more appropriate irq_work related APIs.

Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
---
 drivers/tty/serial/kgdb_nmi.c | 35 ++++++++++-------------------------
 1 file changed, 10 insertions(+), 25 deletions(-)

diff --git a/drivers/tty/serial/kgdb_nmi.c b/drivers/tty/serial/kgdb_nmi.c
index 2580f39..ad84eef 100644
--- a/drivers/tty/serial/kgdb_nmi.c
+++ b/drivers/tty/serial/kgdb_nmi.c
@@ -21,7 +21,7 @@
 #include <linux/tty_flip.h>
 #include <linux/serial_core.h>
 #include <linux/interrupt.h>
-#include <linux/hrtimer.h>
+#include <linux/irq_work.h>
 #include <linux/tick.h>
 #include <linux/kfifo.h>
 #include <linux/kgdb.h>
@@ -127,7 +127,7 @@ static struct console kgdb_nmi_console = {
 
 struct kgdb_nmi_tty_priv {
 	struct tty_port port;
-	struct timer_list timer;
+	struct irq_work work;
 	STRUCT_KFIFO(char, KGDB_NMI_FIFO_SIZE) fifo;
 };
 
@@ -141,13 +141,12 @@ 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. 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.
+	 * Can't use port->tty->driver_data as tty might be not there. Also, we
+	 * can't even do that as 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);
+	irq_work_queue(&priv->work);
 }
 
 static int kgdb_nmi_poll_one_knock(void)
@@ -222,18 +221,12 @@ bool kgdb_nmi_poll_knock(void)
 	return true;
 }
 
-/*
- * The tasklet is cheap, it does not cause wakeups when reschedules itself,
- * instead it waits for the next tick.
- */
-static void kgdb_nmi_tty_receiver(struct timer_list *t)
+static void kgdb_nmi_tty_receiver(struct irq_work *work)
 {
-	struct kgdb_nmi_tty_priv *priv = from_timer(priv, t, timer);
+	struct kgdb_nmi_tty_priv *priv =
+	    container_of(work, struct kgdb_nmi_tty_priv, work);
 	char ch;
 
-	priv->timer.expires = jiffies + (HZ/100);
-	add_timer(&priv->timer);
-
 	if (likely(!atomic_read(&kgdb_nmi_num_readers) ||
 		   !kfifo_len(&priv->fifo)))
 		return;
@@ -245,22 +238,13 @@ static void kgdb_nmi_tty_receiver(struct timer_list *t)
 
 static int kgdb_nmi_tty_activate(struct tty_port *port, struct tty_struct *tty)
 {
-	struct kgdb_nmi_tty_priv *priv =
-	    container_of(port, struct kgdb_nmi_tty_priv, port);
-
 	kgdb_nmi_port = port;
-	priv->timer.expires = jiffies + (HZ/100);
-	add_timer(&priv->timer);
 
 	return 0;
 }
 
 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);
-
-	del_timer(&priv->timer);
 	kgdb_nmi_port = NULL;
 }
 
@@ -279,7 +263,8 @@ static int kgdb_nmi_tty_install(struct tty_driver *drv, struct tty_struct *tty)
 		return -ENOMEM;
 
 	INIT_KFIFO(priv->fifo);
-	timer_setup(&priv->timer, kgdb_nmi_tty_receiver, 0);
+	init_irq_work(&priv->work, kgdb_nmi_tty_receiver);
+	atomic_set(&priv->work.flags, IRQ_WORK_LAZY);
 	tty_port_init(&priv->port);
 	priv->port.ops = &kgdb_nmi_tty_port_ops;
 	tty->driver_data = priv;
-- 
2.7.4

Re: [PATCH 2/7] tty: serial: Add poll_get_irq() to the polling interface

[Daniel Thompson]

On Mon, Jun 22, 2020 at 07:56:19PM +0530, Sumit Garg wrote:
> From: Daniel Thompson <daniel.thompson@linaro.org>

Sumit, to some extent this mail is me yelling at myself two years ago
although, in my defence, at the time I choose not to post it because I
knew it wasn't right.

I'm a bit concerned to see the TODO: comment critiquing this interface
being removed (from patch 3) without this patch being changed to
address that comment.


> Add new API: poll_get_irq() to the polling interface in order for user
> of polling interface to retrieve allocated IRQ corresponding to
> underlying serial device.
> 
> Although, serial interface still works in polling mode but interrupt
> associated with serial device can be leveraged for special purposes like
> debugger(kgdb) entry.
> 
> Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> ---
>  drivers/tty/serial/serial_core.c | 18 ++++++++++++++++++
>  include/linux/serial_core.h      |  1 +
>  include/linux/tty_driver.h       |  1 +
>  3 files changed, 20 insertions(+)
> 
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 66a5e2f..1bb033c 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -2470,6 +2470,23 @@ static void uart_poll_put_char(struct tty_driver *driver, int line, char ch)
>  	port->ops->poll_put_char(port, ch);
>  	uart_port_deref(port);
>  }
> +
> +static int uart_poll_get_irq(struct tty_driver *driver, int line)
> +{
> +	struct uart_driver *drv = driver->driver_state;
> +	struct uart_state *state = drv->state + line;
> +	struct uart_port *port;
> +	int ret = -ENODEV;
> +
> +	port = uart_port_ref(state);
> +	if (port && port->ops->poll_get_irq) {
> +		ret = port->ops->poll_get_irq(port);
> +		uart_port_deref(port);
> +	}
> +
> +	return ret;
> +}
> +
>  #endif
>  
>  static const struct tty_operations uart_ops = {
> @@ -2505,6 +2522,7 @@ static const struct tty_operations uart_ops = {
>  	.poll_init	= uart_poll_init,
>  	.poll_get_char	= uart_poll_get_char,
>  	.poll_put_char	= uart_poll_put_char,
> +	.poll_get_irq	= uart_poll_get_irq,

The TODO comments claimed this API was bogus because it doesn't permit
a free and that can cause interoperation problems with the real serial
driver. I'll cover some of that in a reply to patch 3 but for now.

Anyhow, for this patch, what are the expected semantics for
uart_poll_get_irq().

In particular how do they ensure correct interlocking with the real
serial driver underlying it (if kgdb_nmi is active on a serial port
then the underlying driver better not be active at the same time).


Daniel.


>  #endif
>  };
>  
> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> index 92f5eba..8b132e6 100644
> --- a/include/linux/serial_core.h
> +++ b/include/linux/serial_core.h
> @@ -78,6 +78,7 @@ struct uart_ops {
>  	int		(*poll_init)(struct uart_port *);
>  	void		(*poll_put_char)(struct uart_port *, unsigned char);
>  	int		(*poll_get_char)(struct uart_port *);
> +	int		(*poll_get_irq)(struct uart_port *);
>  #endif
>  };
>  
> diff --git a/include/linux/tty_driver.h b/include/linux/tty_driver.h
> index 3584462..d6da5c5 100644
> --- a/include/linux/tty_driver.h
> +++ b/include/linux/tty_driver.h
> @@ -295,6 +295,7 @@ struct tty_operations {
>  	int (*poll_init)(struct tty_driver *driver, int line, char *options);
>  	int (*poll_get_char)(struct tty_driver *driver, int line);
>  	void (*poll_put_char)(struct tty_driver *driver, int line, char ch);
> +	int (*poll_get_irq)(struct tty_driver *driver, int line);
>  #endif
>  	int (*proc_show)(struct seq_file *, void *);
>  } __randomize_layout;
> -- 
> 2.7.4
> 

Re: [PATCH 3/7] kgdb: Add request_nmi() to the io ops table for kgdboc

[Daniel Thompson]

On Mon, Jun 22, 2020 at 07:56:20PM +0530, Sumit Garg wrote:
> From: Daniel Thompson <daniel.thompson@linaro.org>
> 
> Add request_nmi() callback to install a non-maskable interrupt handler
> corresponding to IRQ retrieved from polling interface. If NMI handler
> installation fails due to missing support from underlying irqchip driver
> then fallback to install it as normal interrupt handler.
> 
> Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
> Co-developed-by: Sumit Garg <sumit.garg@linaro.org>
> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> ---
>  drivers/tty/serial/kgdboc.c | 35 +++++++++++++++++++++++++++++++++++
>  include/linux/kgdb.h        |  7 +++++++
>  2 files changed, 42 insertions(+)
> 
> diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c
> index 84ffede..263afae 100644
> --- a/drivers/tty/serial/kgdboc.c
> +++ b/drivers/tty/serial/kgdboc.c
> @@ -19,6 +19,9 @@
>  #include <linux/console.h>
>  #include <linux/vt_kern.h>
>  #include <linux/input.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/irqdesc.h>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
>  #include <linux/serial_core.h>
> @@ -390,12 +393,44 @@ static void kgdboc_post_exp_handler(void)
>  	kgdboc_restore_input();
>  }
>  
> +static int kgdb_tty_irq;
> +
> +static int kgdboc_request_nmi(irq_handler_t fn, void *dev_id)
> +{
> +	int irq, res;
> +
> +	/* Better to avoid double allocation in the tty driver! */
> +	if (kgdb_tty_irq)
> +		return 0;
> +
> +	if (!kgdb_tty_driver->ops->poll_get_irq)
> +		return -ENODEV;
> +
> +	irq =
> +	    kgdb_tty_driver->ops->poll_get_irq(kgdb_tty_driver, kgdb_tty_line);
> +	if (irq <= 0)
> +		return irq ? irq : -ENODEV;
> +
> +	irq_set_status_flags(irq, IRQ_NOAUTOEN);
> +	res = request_nmi(irq, fn, IRQF_PERCPU, "kgdboc", dev_id);

Why do we need IRQF_PERCPU here. A UART interrupt is not normally
per-cpu?


> +	if (res) {
> +		res = request_irq(irq, fn, IRQF_SHARED, "kgdboc", dev_id);

IRQF_SHARED?

Currrently there is nothing that prevents concurrent activation of
ttyNMI0 and the underlying serial driver. Using IRQF_SHARED means it
becomes possible for both drivers to try to service the same interrupt.
That risks some rather "interesting" problems.


Daniel.


> +		WARN_ON(res);
> +	}
> +
> +	enable_irq(irq);
> +
> +	kgdb_tty_irq = irq;
> +	return 0;
> +}
> +
>  static struct kgdb_io kgdboc_io_ops = {
>  	.name			= "kgdboc",
>  	.read_char		= kgdboc_get_char,
>  	.write_char		= kgdboc_put_char,
>  	.pre_exception		= kgdboc_pre_exp_handler,
>  	.post_exception		= kgdboc_post_exp_handler,
> +	.request_nmi		= kgdboc_request_nmi,
>  };
>  
>  #if IS_BUILTIN(CONFIG_KGDB_SERIAL_CONSOLE)
> diff --git a/include/linux/kgdb.h b/include/linux/kgdb.h
> index 529116b..b32b044 100644
> --- a/include/linux/kgdb.h
> +++ b/include/linux/kgdb.h
> @@ -16,6 +16,7 @@
>  #include <linux/linkage.h>
>  #include <linux/init.h>
>  #include <linux/atomic.h>
> +#include <linux/interrupt.h>
>  #ifdef CONFIG_HAVE_ARCH_KGDB
>  #include <asm/kgdb.h>
>  #endif
> @@ -276,6 +277,10 @@ struct kgdb_arch {
>   * the I/O driver.
>   * @post_exception: Pointer to a function that will do any cleanup work
>   * for the I/O driver.
> + * @request_nmi: Pointer to a function that can install an non-maskable
> + * interrupt handler that will be called when a character is pending and that
> + * can be cleared by calling @read_char until it returns NO_POLL_CHAR. If NMI
> + * installation fails then fallback to install normal interrupt handler.
>   * @cons: valid if the I/O device is a console; else NULL.
>   */
>  struct kgdb_io {
> @@ -287,6 +292,8 @@ struct kgdb_io {
>  	void			(*deinit) (void);
>  	void			(*pre_exception) (void);
>  	void			(*post_exception) (void);
> +	int			(*request_nmi)(irq_handler_t nmi_handler,
> +					       void *dev_id);
>  	struct console		*cons;
>  };
>  
> -- 
> 2.7.4
> 

Re: [PATCH 4/7] serial: kgdb_nmi: Add support for interrupt based fallback

[Daniel Thompson]

On Mon, Jun 22, 2020 at 07:56:21PM +0530, Sumit Garg wrote:
> From: Daniel Thompson <daniel.thompson@linaro.org>
> 
> Add a generic NMI fallback to support kgdb NMI console feature which can
> be overridden by arch specific implementation.

arch_kgdb_ops.enable_nmi should probably be killed off. Given we now
have request_nmi() I'm dubious there are any good reasons to use this
API.


Daniel.


> This common fallback mechanism utilizes kgdb IO based interrupt in order
> to support entry into kgdb if a user types in kgdb_nmi_magic sequence. So
> during NMI console init, NMI handler is installed corresponding to kgdb
> IO based NMI which is invoked when a character is pending and that can be
> cleared by calling @read_char until it returns NO_POLL_CHAR.
> 
> Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
> Co-developed-by: Sumit Garg <sumit.garg@linaro.org>
> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> ---
>  drivers/tty/serial/kgdb_nmi.c | 47 ++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 40 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/tty/serial/kgdb_nmi.c b/drivers/tty/serial/kgdb_nmi.c
> index b32c6b1..2580f39 100644
> --- a/drivers/tty/serial/kgdb_nmi.c
> +++ b/drivers/tty/serial/kgdb_nmi.c
> @@ -42,9 +42,46 @@ MODULE_PARM_DESC(magic, "magic sequence to enter NMI debugger (default $3#33)");
>  static atomic_t kgdb_nmi_num_readers = ATOMIC_INIT(0);
>  static struct console *orig_dbg_cons;
>  
> +static int kgdb_nmi_poll_one_knock(void);
> +
> +static irqreturn_t kgdb_handle_nmi(int irq, void *dev_id)
> +{
> +	int ret;
> +
> +	if (kgdb_nmi_knock < 0) {
> +		kgdb_breakpoint();
> +		return IRQ_HANDLED;
> +	}
> +
> +	ret = kgdb_nmi_poll_one_knock();
> +	if (ret == NO_POLL_CHAR)
> +		return IRQ_NONE;
> +
> +	while (ret != 1) {
> +		ret = kgdb_nmi_poll_one_knock();
> +		if (ret == NO_POLL_CHAR)
> +			return IRQ_HANDLED;
> +	}
> +
> +	kgdb_breakpoint();
> +	return IRQ_HANDLED;
> +}
> +
>  static int kgdb_nmi_console_setup(struct console *co, char *options)
>  {
> -	arch_kgdb_ops.enable_nmi(1);
> +	int res;
> +
> +	if (arch_kgdb_ops.enable_nmi) {
> +		arch_kgdb_ops.enable_nmi(1);
> +	} else if (dbg_io_ops->request_nmi) {
> +		res = dbg_io_ops->request_nmi(kgdb_handle_nmi, co);
> +		if (res) {
> +			pr_err("ttyNMI0: Cannot request nmi/irq\n");
> +			return res;
> +		}
> +	} else {
> +		return -ENODEV;
> +	}
>  
>  	/* The NMI console uses the dbg_io_ops to issue console messages. To
>  	 * avoid duplicate messages during kdb sessions we must inform kdb's
> @@ -328,9 +365,6 @@ int kgdb_register_nmi_console(void)
>  {
>  	int ret;
>  
> -	if (!arch_kgdb_ops.enable_nmi)
> -		return 0;
> -
>  	kgdb_nmi_tty_driver = alloc_tty_driver(1);
>  	if (!kgdb_nmi_tty_driver) {
>  		pr_err("%s: cannot allocate tty\n", __func__);
> @@ -380,9 +414,8 @@ int kgdb_unregister_nmi_console(void)
>  {
>  	int ret;
>  
> -	if (!arch_kgdb_ops.enable_nmi)
> -		return 0;
> -	arch_kgdb_ops.enable_nmi(0);
> +	if (arch_kgdb_ops.enable_nmi)
> +		arch_kgdb_ops.enable_nmi(0);
>  
>  	ret = unregister_console(&kgdb_nmi_console);
>  	if (ret)
> -- 
> 2.7.4
> 

Re: [PATCH 2/7] tty: serial: Add poll_get_irq() to the polling interface

[Sumit Garg]

On Mon, 22 Jun 2020 at 21:26, Daniel Thompson
<daniel.thompson@linaro.org> wrote:
>
> On Mon, Jun 22, 2020 at 07:56:19PM +0530, Sumit Garg wrote:
> > From: Daniel Thompson <daniel.thompson@linaro.org>
>
> Sumit, to some extent this mail is me yelling at myself two years ago
> although, in my defence, at the time I choose not to post it because I
> knew it wasn't right.
>
> I'm a bit concerned to see the TODO: comment critiquing this interface
> being removed (from patch 3) without this patch being changed to
> address that comment.
>

I did consider that comment but I couldn't think of a normal scenario
where request_irq() should fail. And in case it fails as well, I did
put in "WARN_ON(res);" so that the user is notified of that particular
error scenario.

>
> > Add new API: poll_get_irq() to the polling interface in order for user
> > of polling interface to retrieve allocated IRQ corresponding to
> > underlying serial device.
> >
> > Although, serial interface still works in polling mode but interrupt
> > associated with serial device can be leveraged for special purposes like
> > debugger(kgdb) entry.
> >
> > Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
> > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> > ---
> >  drivers/tty/serial/serial_core.c | 18 ++++++++++++++++++
> >  include/linux/serial_core.h      |  1 +
> >  include/linux/tty_driver.h       |  1 +
> >  3 files changed, 20 insertions(+)
> >
> > diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> > index 66a5e2f..1bb033c 100644
> > --- a/drivers/tty/serial/serial_core.c
> > +++ b/drivers/tty/serial/serial_core.c
> > @@ -2470,6 +2470,23 @@ static void uart_poll_put_char(struct tty_driver *driver, int line, char ch)
> >       port->ops->poll_put_char(port, ch);
> >       uart_port_deref(port);
> >  }
> > +
> > +static int uart_poll_get_irq(struct tty_driver *driver, int line)
> > +{
> > +     struct uart_driver *drv = driver->driver_state;
> > +     struct uart_state *state = drv->state + line;
> > +     struct uart_port *port;
> > +     int ret = -ENODEV;
> > +
> > +     port = uart_port_ref(state);
> > +     if (port && port->ops->poll_get_irq) {
> > +             ret = port->ops->poll_get_irq(port);
> > +             uart_port_deref(port);
> > +     }
> > +
> > +     return ret;
> > +}
> > +
> >  #endif
> >
> >  static const struct tty_operations uart_ops = {
> > @@ -2505,6 +2522,7 @@ static const struct tty_operations uart_ops = {
> >       .poll_init      = uart_poll_init,
> >       .poll_get_char  = uart_poll_get_char,
> >       .poll_put_char  = uart_poll_put_char,
> > +     .poll_get_irq   = uart_poll_get_irq,
>
> The TODO comments claimed this API was bogus because it doesn't permit
> a free and that can cause interoperation problems with the real serial
> driver. I'll cover some of that in a reply to patch 3 but for now.
>
> Anyhow, for this patch, what are the expected semantics for
> uart_poll_get_irq().

Currently, the expected use for this API is to enable uart RX
interrupts and return corresponding IRQ id.

Although, we can make this interface modular as follows:

.poll_get_irq
.poll_enable_rx_int
.poll_disable_rx_int

Your views?

>
> In particular how do they ensure correct interlocking with the real
> serial driver underlying it (if kgdb_nmi is active on a serial port
> then the underlying driver better not be active at the same time).
>

AFAIU kgdb_nmi feature, it registers a new tty driver (ttyNMI0) which
is expected to work alongside underlying tty driver (eg. ttyAMA0 with
amba-pl011). So ttyAMA0 will only become active if user-space tries to
interact with /dev/ttyAMA0 like:

# echo "Hello World!" > /dev/ttyAMA0

So I would like to understand the downsides of having both of them
active at the same time using shared IRQ (although that won't be
possible with NMI as that doesn't support shared mode)?

-Sumit

>
> Daniel.
>
>
> >  #endif
> >  };
> >
> > diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> > index 92f5eba..8b132e6 100644
> > --- a/include/linux/serial_core.h
> > +++ b/include/linux/serial_core.h
> > @@ -78,6 +78,7 @@ struct uart_ops {
> >       int             (*poll_init)(struct uart_port *);
> >       void            (*poll_put_char)(struct uart_port *, unsigned char);
> >       int             (*poll_get_char)(struct uart_port *);
> > +     int             (*poll_get_irq)(struct uart_port *);
> >  #endif
> >  };
> >
> > diff --git a/include/linux/tty_driver.h b/include/linux/tty_driver.h
> > index 3584462..d6da5c5 100644
> > --- a/include/linux/tty_driver.h
> > +++ b/include/linux/tty_driver.h
> > @@ -295,6 +295,7 @@ struct tty_operations {
> >       int (*poll_init)(struct tty_driver *driver, int line, char *options);
> >       int (*poll_get_char)(struct tty_driver *driver, int line);
> >       void (*poll_put_char)(struct tty_driver *driver, int line, char ch);
> > +     int (*poll_get_irq)(struct tty_driver *driver, int line);
> >  #endif
> >       int (*proc_show)(struct seq_file *, void *);
> >  } __randomize_layout;
> > --
> > 2.7.4
> >

Re: [PATCH 3/7] kgdb: Add request_nmi() to the io ops table for kgdboc

[Sumit Garg]

On Mon, 22 Jun 2020 at 21:33, Daniel Thompson
<daniel.thompson@linaro.org> wrote:
>
> On Mon, Jun 22, 2020 at 07:56:20PM +0530, Sumit Garg wrote:
> > From: Daniel Thompson <daniel.thompson@linaro.org>
> >
> > Add request_nmi() callback to install a non-maskable interrupt handler
> > corresponding to IRQ retrieved from polling interface. If NMI handler
> > installation fails due to missing support from underlying irqchip driver
> > then fallback to install it as normal interrupt handler.
> >
> > Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
> > Co-developed-by: Sumit Garg <sumit.garg@linaro.org>
> > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> > ---
> >  drivers/tty/serial/kgdboc.c | 35 +++++++++++++++++++++++++++++++++++
> >  include/linux/kgdb.h        |  7 +++++++
> >  2 files changed, 42 insertions(+)
> >
> > diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c
> > index 84ffede..263afae 100644
> > --- a/drivers/tty/serial/kgdboc.c
> > +++ b/drivers/tty/serial/kgdboc.c
> > @@ -19,6 +19,9 @@
> >  #include <linux/console.h>
> >  #include <linux/vt_kern.h>
> >  #include <linux/input.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/irq.h>
> > +#include <linux/irqdesc.h>
> >  #include <linux/module.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/serial_core.h>
> > @@ -390,12 +393,44 @@ static void kgdboc_post_exp_handler(void)
> >       kgdboc_restore_input();
> >  }
> >
> > +static int kgdb_tty_irq;
> > +
> > +static int kgdboc_request_nmi(irq_handler_t fn, void *dev_id)
> > +{
> > +     int irq, res;
> > +
> > +     /* Better to avoid double allocation in the tty driver! */
> > +     if (kgdb_tty_irq)
> > +             return 0;
> > +
> > +     if (!kgdb_tty_driver->ops->poll_get_irq)
> > +             return -ENODEV;
> > +
> > +     irq =
> > +         kgdb_tty_driver->ops->poll_get_irq(kgdb_tty_driver, kgdb_tty_line);
> > +     if (irq <= 0)
> > +             return irq ? irq : -ENODEV;
> > +
> > +     irq_set_status_flags(irq, IRQ_NOAUTOEN);
> > +     res = request_nmi(irq, fn, IRQF_PERCPU, "kgdboc", dev_id);
>
> Why do we need IRQF_PERCPU here. A UART interrupt is not normally
> per-cpu?
>

Have a look at this comment [1] and corresponding check in
request_nmi(). So essentially yes UART interrupt is not normally
per-cpu but in order to make it an NMI, we need to request it in
per-cpu mode.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/irq/manage.c#n2112

>
> > +     if (res) {
> > +             res = request_irq(irq, fn, IRQF_SHARED, "kgdboc", dev_id);
>
> IRQF_SHARED?
>
> Currrently there is nothing that prevents concurrent activation of
> ttyNMI0 and the underlying serial driver. Using IRQF_SHARED means it
> becomes possible for both drivers to try to service the same interrupt.
> That risks some rather "interesting" problems.
>

Could you elaborate more on "interesting" problems?

BTW, I noticed one more problem with this patch that is IRQF_SHARED
doesn't go well with IRQ_NOAUTOEN status flag. Earlier I tested it
with auto enable set.

But if we agree that both shouldn't be active at the same time due to
some real problems(?) then I can rid of IRQF_SHARED as well. Also, I
think we should unregister underlying tty driver (eg. /dev/ttyAMA0) as
well as otherwise it would provide a broken interface to user-space.

-Sumit

>
> Daniel.
>
>
> > +             WARN_ON(res);
> > +     }
> > +
> > +     enable_irq(irq);
> > +
> > +     kgdb_tty_irq = irq;
> > +     return 0;
> > +}
> > +
> >  static struct kgdb_io kgdboc_io_ops = {
> >       .name                   = "kgdboc",
> >       .read_char              = kgdboc_get_char,
> >       .write_char             = kgdboc_put_char,
> >       .pre_exception          = kgdboc_pre_exp_handler,
> >       .post_exception         = kgdboc_post_exp_handler,
> > +     .request_nmi            = kgdboc_request_nmi,
> >  };
> >
> >  #if IS_BUILTIN(CONFIG_KGDB_SERIAL_CONSOLE)
> > diff --git a/include/linux/kgdb.h b/include/linux/kgdb.h
> > index 529116b..b32b044 100644
> > --- a/include/linux/kgdb.h
> > +++ b/include/linux/kgdb.h
> > @@ -16,6 +16,7 @@
> >  #include <linux/linkage.h>
> >  #include <linux/init.h>
> >  #include <linux/atomic.h>
> > +#include <linux/interrupt.h>
> >  #ifdef CONFIG_HAVE_ARCH_KGDB
> >  #include <asm/kgdb.h>
> >  #endif
> > @@ -276,6 +277,10 @@ struct kgdb_arch {
> >   * the I/O driver.
> >   * @post_exception: Pointer to a function that will do any cleanup work
> >   * for the I/O driver.
> > + * @request_nmi: Pointer to a function that can install an non-maskable
> > + * interrupt handler that will be called when a character is pending and that
> > + * can be cleared by calling @read_char until it returns NO_POLL_CHAR. If NMI
> > + * installation fails then fallback to install normal interrupt handler.
> >   * @cons: valid if the I/O device is a console; else NULL.
> >   */
> >  struct kgdb_io {
> > @@ -287,6 +292,8 @@ struct kgdb_io {
> >       void                    (*deinit) (void);
> >       void                    (*pre_exception) (void);
> >       void                    (*post_exception) (void);
> > +     int                     (*request_nmi)(irq_handler_t nmi_handler,
> > +                                            void *dev_id);
> >       struct console          *cons;
> >  };
> >
> > --
> > 2.7.4
> >

Re: [PATCH 4/7] serial: kgdb_nmi: Add support for interrupt based fallback

[Sumit Garg]

On Mon, 22 Jun 2020 at 22:06, Daniel Thompson
<daniel.thompson@linaro.org> wrote:
>
> On Mon, Jun 22, 2020 at 07:56:21PM +0530, Sumit Garg wrote:
> > From: Daniel Thompson <daniel.thompson@linaro.org>
> >
> > Add a generic NMI fallback to support kgdb NMI console feature which can
> > be overridden by arch specific implementation.
>
> arch_kgdb_ops.enable_nmi should probably be killed off. Given we now
> have request_nmi() I'm dubious there are any good reasons to use this
> API.
>

Sounds reasonable. Along with this, kgdb_nmi_poll_knock() seems to be
unused and can be removed as well.

-Sumit

>
> Daniel.
>
>
> > This common fallback mechanism utilizes kgdb IO based interrupt in order
> > to support entry into kgdb if a user types in kgdb_nmi_magic sequence. So
> > during NMI console init, NMI handler is installed corresponding to kgdb
> > IO based NMI which is invoked when a character is pending and that can be
> > cleared by calling @read_char until it returns NO_POLL_CHAR.
> >
> > Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
> > Co-developed-by: Sumit Garg <sumit.garg@linaro.org>
> > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> > ---
> >  drivers/tty/serial/kgdb_nmi.c | 47 ++++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 40 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/tty/serial/kgdb_nmi.c b/drivers/tty/serial/kgdb_nmi.c
> > index b32c6b1..2580f39 100644
> > --- a/drivers/tty/serial/kgdb_nmi.c
> > +++ b/drivers/tty/serial/kgdb_nmi.c
> > @@ -42,9 +42,46 @@ MODULE_PARM_DESC(magic, "magic sequence to enter NMI debugger (default $3#33)");
> >  static atomic_t kgdb_nmi_num_readers = ATOMIC_INIT(0);
> >  static struct console *orig_dbg_cons;
> >
> > +static int kgdb_nmi_poll_one_knock(void);
> > +
> > +static irqreturn_t kgdb_handle_nmi(int irq, void *dev_id)
> > +{
> > +     int ret;
> > +
> > +     if (kgdb_nmi_knock < 0) {
> > +             kgdb_breakpoint();
> > +             return IRQ_HANDLED;
> > +     }
> > +
> > +     ret = kgdb_nmi_poll_one_knock();
> > +     if (ret == NO_POLL_CHAR)
> > +             return IRQ_NONE;
> > +
> > +     while (ret != 1) {
> > +             ret = kgdb_nmi_poll_one_knock();
> > +             if (ret == NO_POLL_CHAR)
> > +                     return IRQ_HANDLED;
> > +     }
> > +
> > +     kgdb_breakpoint();
> > +     return IRQ_HANDLED;
> > +}
> > +
> >  static int kgdb_nmi_console_setup(struct console *co, char *options)
> >  {
> > -     arch_kgdb_ops.enable_nmi(1);
> > +     int res;
> > +
> > +     if (arch_kgdb_ops.enable_nmi) {
> > +             arch_kgdb_ops.enable_nmi(1);
> > +     } else if (dbg_io_ops->request_nmi) {
> > +             res = dbg_io_ops->request_nmi(kgdb_handle_nmi, co);
> > +             if (res) {
> > +                     pr_err("ttyNMI0: Cannot request nmi/irq\n");
> > +                     return res;
> > +             }
> > +     } else {
> > +             return -ENODEV;
> > +     }
> >
> >       /* The NMI console uses the dbg_io_ops to issue console messages. To
> >        * avoid duplicate messages during kdb sessions we must inform kdb's
> > @@ -328,9 +365,6 @@ int kgdb_register_nmi_console(void)
> >  {
> >       int ret;
> >
> > -     if (!arch_kgdb_ops.enable_nmi)
> > -             return 0;
> > -
> >       kgdb_nmi_tty_driver = alloc_tty_driver(1);
> >       if (!kgdb_nmi_tty_driver) {
> >               pr_err("%s: cannot allocate tty\n", __func__);
> > @@ -380,9 +414,8 @@ int kgdb_unregister_nmi_console(void)
> >  {
> >       int ret;
> >
> > -     if (!arch_kgdb_ops.enable_nmi)
> > -             return 0;
> > -     arch_kgdb_ops.enable_nmi(0);
> > +     if (arch_kgdb_ops.enable_nmi)
> > +             arch_kgdb_ops.enable_nmi(0);
> >
> >       ret = unregister_console(&kgdb_nmi_console);
> >       if (ret)
> > --
> > 2.7.4
> >

Re: [PATCH 2/7] tty: serial: Add poll_get_irq() to the polling interface

[Daniel Thompson]

On Tue, Jun 23, 2020 at 01:18:25PM +0530, Sumit Garg wrote:
> On Mon, 22 Jun 2020 at 21:26, Daniel Thompson
> <daniel.thompson@linaro.org> wrote:
> >
> > On Mon, Jun 22, 2020 at 07:56:19PM +0530, Sumit Garg wrote:
> > > From: Daniel Thompson <daniel.thompson@linaro.org>
> >
> > Sumit, to some extent this mail is me yelling at myself two years ago
> > although, in my defence, at the time I choose not to post it because I
> > knew it wasn't right.
> >
> > I'm a bit concerned to see the TODO: comment critiquing this interface
> > being removed (from patch 3) without this patch being changed to
> > address that comment.
> >
> 
> I did consider that comment but I couldn't think of a normal scenario
> where request_irq() should fail. And in case it fails as well, I did
> put in "WARN_ON(res);" so that the user is notified of that particular
> error scenario.
> 
> >
> > > Add new API: poll_get_irq() to the polling interface in order for user
> > > of polling interface to retrieve allocated IRQ corresponding to
> > > underlying serial device.
> > >
> > > Although, serial interface still works in polling mode but interrupt
> > > associated with serial device can be leveraged for special purposes like
> > > debugger(kgdb) entry.
> > >
> > > Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
> > > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> > > ---
> > >  drivers/tty/serial/serial_core.c | 18 ++++++++++++++++++
> > >  include/linux/serial_core.h      |  1 +
> > >  include/linux/tty_driver.h       |  1 +
> > >  3 files changed, 20 insertions(+)
> > >
> > > diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> > > index 66a5e2f..1bb033c 100644
> > > --- a/drivers/tty/serial/serial_core.c
> > > +++ b/drivers/tty/serial/serial_core.c
> > > @@ -2470,6 +2470,23 @@ static void uart_poll_put_char(struct tty_driver *driver, int line, char ch)
> > >       port->ops->poll_put_char(port, ch);
> > >       uart_port_deref(port);
> > >  }
> > > +
> > > +static int uart_poll_get_irq(struct tty_driver *driver, int line)
> > > +{
> > > +     struct uart_driver *drv = driver->driver_state;
> > > +     struct uart_state *state = drv->state + line;
> > > +     struct uart_port *port;
> > > +     int ret = -ENODEV;
> > > +
> > > +     port = uart_port_ref(state);
> > > +     if (port && port->ops->poll_get_irq) {
> > > +             ret = port->ops->poll_get_irq(port);
> > > +             uart_port_deref(port);
> > > +     }
> > > +
> > > +     return ret;
> > > +}
> > > +
> > >  #endif
> > >
> > >  static const struct tty_operations uart_ops = {
> > > @@ -2505,6 +2522,7 @@ static const struct tty_operations uart_ops = {
> > >       .poll_init      = uart_poll_init,
> > >       .poll_get_char  = uart_poll_get_char,
> > >       .poll_put_char  = uart_poll_put_char,
> > > +     .poll_get_irq   = uart_poll_get_irq,
> >
> > The TODO comments claimed this API was bogus because it doesn't permit
> > a free and that can cause interoperation problems with the real serial
> > driver. I'll cover some of that in a reply to patch 3 but for now.
> >
> > Anyhow, for this patch, what are the expected semantics for
> > uart_poll_get_irq().
> 
> Currently, the expected use for this API is to enable uart RX
> interrupts and return corresponding IRQ id.
> 
> Although, we can make this interface modular as follows:
> 
> .poll_get_irq
> .poll_enable_rx_int
> .poll_disable_rx_int
> 
> Your views?

A direct reply is to ask what the purpose of poll_enable_rx_int()
is? It appears to be making something modular without any reason to do
so.

More generally lets ask some more questions:

1. What serial hardware and/or driver state change happens when we call
   poll_get_irq()

2. After a call to poll_get_irq() are there any new restrictions on
   the underlying serial driver?

3. Does implementing poll_get_irq() create an implied change of
   contract to any other polling function (I think yes, because
   poll_read_char() will now have to acknowledge an interrupt if
   the interrupt flags are edge triggered)?

4. To what extent, if any, do we have to support disconnection of
   kgdb_nmi?

5. Is the API matched with how it will be used by the layers above?
   (top level uses a request_irq approach, low level is using a
   let me have the irq number approach)

An interesting idea to explore is using a different verb for
poll_get_irq(). Perhaps poll_borrow_irq()? It is now much clearer that
we are stealing the IRQ and making it unusable by the underlying serial
driver.


> > In particular how do they ensure correct interlocking with the real
> > serial driver underlying it (if kgdb_nmi is active on a serial port
> > then the underlying driver better not be active at the same time).
> >
> 
> AFAIU kgdb_nmi feature, it registers a new tty driver (ttyNMI0) which
> is expected to work alongside underlying tty driver (eg. ttyAMA0 with
> amba-pl011). So ttyAMA0 will only become active if user-space tries to
> interact with /dev/ttyAMA0 like:
> 
> # echo "Hello World!" > /dev/ttyAMA0
> 
> So I would like to understand the downsides of having both of them
> active at the same time using shared IRQ (although that won't be
> possible with NMI as that doesn't support shared mode)?

It is likely that one of the interrupt handlers will become unreachable
since the first handler to execute will clear the interrupt flags. This
will result in anything waiting for the broken driver getting stuck and
not reporting an error code.

That isn't acceptable.


Daniel.

Re: [PATCH 3/7] kgdb: Add request_nmi() to the io ops table for kgdboc

[Daniel Thompson]

On Tue, Jun 23, 2020 at 02:07:47PM +0530, Sumit Garg wrote:
> On Mon, 22 Jun 2020 at 21:33, Daniel Thompson
> <daniel.thompson@linaro.org> wrote:
> > > +     irq_set_status_flags(irq, IRQ_NOAUTOEN);
> > > +     res = request_nmi(irq, fn, IRQF_PERCPU, "kgdboc", dev_id);
> >
> > Why do we need IRQF_PERCPU here. A UART interrupt is not normally
> > per-cpu?
> >
> 
> Have a look at this comment [1] and corresponding check in
> request_nmi(). So essentially yes UART interrupt is not normally
> per-cpu but in order to make it an NMI, we need to request it in
> per-cpu mode.
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/irq/manage.c#n2112

Thanks! This is clear.

> > > +     if (res) {
> > > +             res = request_irq(irq, fn, IRQF_SHARED, "kgdboc", dev_id);
> >
> > IRQF_SHARED?
> >
> > Currrently there is nothing that prevents concurrent activation of
> > ttyNMI0 and the underlying serial driver. Using IRQF_SHARED means it
> > becomes possible for both drivers to try to service the same interrupt.
> > That risks some rather "interesting" problems.
> >
> 
> Could you elaborate more on "interesting" problems?

Er... one of the serial drivers we have allowed the userspace to open
will, at best, be stone dead and not passing any characters.


> BTW, I noticed one more problem with this patch that is IRQF_SHARED
> doesn't go well with IRQ_NOAUTOEN status flag. Earlier I tested it
> with auto enable set.
> 
> But if we agree that both shouldn't be active at the same time due to
> some real problems(?) then I can rid of IRQF_SHARED as well. Also, I
> think we should unregister underlying tty driver (eg. /dev/ttyAMA0) as
> well as otherwise it would provide a broken interface to user-space.

I don't have a particular strong opinion on whether IRQF_SHARED is
correct or not correct since I think that misses the point.

Firstly, using IRQF_SHARED shows us that there is no interlocking
between kgdb_nmi and the underlying serial driver. That probably tells
us about the importance of the interlock than about IRQF_SHARED.

To some extent I'm also unsure that kgdb_nmi could ever actually know
the correct flags to use in all cases (that was another reason for the
TODO comment about poll_get_irq() being a bogus API).


Daniel.

Re: [PATCH 3/7] kgdb: Add request_nmi() to the io ops table for kgdboc

[Doug Anderson]

Hi,

On Tue, Jun 23, 2020 at 3:59 AM Daniel Thompson
<daniel.thompson@linaro.org> wrote:
>
> On Tue, Jun 23, 2020 at 02:07:47PM +0530, Sumit Garg wrote:
> > On Mon, 22 Jun 2020 at 21:33, Daniel Thompson
> > <daniel.thompson@linaro.org> wrote:
> > > > +     irq_set_status_flags(irq, IRQ_NOAUTOEN);
> > > > +     res = request_nmi(irq, fn, IRQF_PERCPU, "kgdboc", dev_id);
> > >
> > > Why do we need IRQF_PERCPU here. A UART interrupt is not normally
> > > per-cpu?
> > >
> >
> > Have a look at this comment [1] and corresponding check in
> > request_nmi(). So essentially yes UART interrupt is not normally
> > per-cpu but in order to make it an NMI, we need to request it in
> > per-cpu mode.
> >
> > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/irq/manage.c#n2112
>
> Thanks! This is clear.
>
> > > > +     if (res) {
> > > > +             res = request_irq(irq, fn, IRQF_SHARED, "kgdboc", dev_id);
> > >
> > > IRQF_SHARED?
> > >
> > > Currrently there is nothing that prevents concurrent activation of
> > > ttyNMI0 and the underlying serial driver. Using IRQF_SHARED means it
> > > becomes possible for both drivers to try to service the same interrupt.
> > > That risks some rather "interesting" problems.
> > >
> >
> > Could you elaborate more on "interesting" problems?
>
> Er... one of the serial drivers we have allowed the userspace to open
> will, at best, be stone dead and not passing any characters.
>
>
> > BTW, I noticed one more problem with this patch that is IRQF_SHARED
> > doesn't go well with IRQ_NOAUTOEN status flag. Earlier I tested it
> > with auto enable set.
> >
> > But if we agree that both shouldn't be active at the same time due to
> > some real problems(?) then I can rid of IRQF_SHARED as well. Also, I
> > think we should unregister underlying tty driver (eg. /dev/ttyAMA0) as
> > well as otherwise it would provide a broken interface to user-space.
>
> I don't have a particular strong opinion on whether IRQF_SHARED is
> correct or not correct since I think that misses the point.
>
> Firstly, using IRQF_SHARED shows us that there is no interlocking
> between kgdb_nmi and the underlying serial driver. That probably tells
> us about the importance of the interlock than about IRQF_SHARED.
>
> To some extent I'm also unsure that kgdb_nmi could ever actually know
> the correct flags to use in all cases (that was another reason for the
> TODO comment about poll_get_irq() being a bogus API).

I do wonder a little bit if the architecture of the "kgdb_nmi_console"
should change.  I remember looking at it in the past and thinking it a
little weird that if I wanted to get it to work I'd need to change my
"console=" command line to go through this new driver and (I guess)
change the agetty I have running on my serial port to point to
ttyNMI0.  Is that how it's supposed to work?  Then if I want to do a
build without kgdb then I need to go in and change my agetty to point
back at my normal serial port?

It kinda feels like a better way to much of what the driver does would be to:

1. Allow kgdb to sniff incoming serial bytes on a port and look for
its characters.  We already have this feature in the kernel to a small
extent for sniffing a break / sysrq character.

2. If userspace doesn't happen to have the serial port open then
ideally we could open the port (using all the standard APIs that
already exist) from in the kernel and just throw away all the bytes
(since we already sniffed them).  As soon as userspace tried to open
the port when it would get ownership and if userspace ever closed the
port then we'd start reading / throwing away bytes again.

If we had a solution like that:

a) No serial drivers would need to change.

b) No kernel command line parameters would need to change.

Obviously that solution wouldn't magically get you an NMI, though.
For that I'd presume the right answer would be to add a parameter for
each serial driver that can support it to run its rx interrupt in NMI
mode.

Of course, perhaps I'm just confused and crazy and the above is a
really bad idea.


Speaking of confused: is there actually any way to use the existing
kgdb NMI driver (CONFIG_SERIAL_KGDB_NMI) in mainline without out of
tree patches?  When I looked before I assumed it was just me that was
outta luck because I didn't have NMI at the time, but I just did some
grepping and I can't find anyplace in mainline where
"arch_kgdb_ops.enable_nmi" would not be NULL.  Did I miss it, or do we
need out-of-tree patches to enable this?


-Doug

Re: [PATCH 3/7] kgdb: Add request_nmi() to the io ops table for kgdboc

[Daniel Thompson]

On Fri, Jun 26, 2020 at 12:44:15PM -0700, Doug Anderson wrote:
> Hi,
> 
> On Tue, Jun 23, 2020 at 3:59 AM Daniel Thompson
> <daniel.thompson@linaro.org> wrote:
> >
> > On Tue, Jun 23, 2020 at 02:07:47PM +0530, Sumit Garg wrote:
> > > On Mon, 22 Jun 2020 at 21:33, Daniel Thompson
> > > <daniel.thompson@linaro.org> wrote:
> > > > > +     irq_set_status_flags(irq, IRQ_NOAUTOEN);
> > > > > +     res = request_nmi(irq, fn, IRQF_PERCPU, "kgdboc", dev_id);
> > > >
> > > > Why do we need IRQF_PERCPU here. A UART interrupt is not normally
> > > > per-cpu?
> > > >
> > >
> > > Have a look at this comment [1] and corresponding check in
> > > request_nmi(). So essentially yes UART interrupt is not normally
> > > per-cpu but in order to make it an NMI, we need to request it in
> > > per-cpu mode.
> > >
> > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/irq/manage.c#n2112
> >
> > Thanks! This is clear.
> >
> > > > > +     if (res) {
> > > > > +             res = request_irq(irq, fn, IRQF_SHARED, "kgdboc", dev_id);
> > > >
> > > > IRQF_SHARED?
> > > >
> > > > Currrently there is nothing that prevents concurrent activation of
> > > > ttyNMI0 and the underlying serial driver. Using IRQF_SHARED means it
> > > > becomes possible for both drivers to try to service the same interrupt.
> > > > That risks some rather "interesting" problems.
> > > >
> > >
> > > Could you elaborate more on "interesting" problems?
> >
> > Er... one of the serial drivers we have allowed the userspace to open
> > will, at best, be stone dead and not passing any characters.
> >
> >
> > > BTW, I noticed one more problem with this patch that is IRQF_SHARED
> > > doesn't go well with IRQ_NOAUTOEN status flag. Earlier I tested it
> > > with auto enable set.
> > >
> > > But if we agree that both shouldn't be active at the same time due to
> > > some real problems(?) then I can rid of IRQF_SHARED as well. Also, I
> > > think we should unregister underlying tty driver (eg. /dev/ttyAMA0) as
> > > well as otherwise it would provide a broken interface to user-space.
> >
> > I don't have a particular strong opinion on whether IRQF_SHARED is
> > correct or not correct since I think that misses the point.
> >
> > Firstly, using IRQF_SHARED shows us that there is no interlocking
> > between kgdb_nmi and the underlying serial driver. That probably tells
> > us about the importance of the interlock than about IRQF_SHARED.
> >
> > To some extent I'm also unsure that kgdb_nmi could ever actually know
> > the correct flags to use in all cases (that was another reason for the
> > TODO comment about poll_get_irq() being a bogus API).
> 
> I do wonder a little bit if the architecture of the "kgdb_nmi_console"
> should change.  I remember looking at it in the past and thinking it a
> little weird that if I wanted to get it to work I'd need to change my
> "console=" command line to go through this new driver and (I guess)
> change the agetty I have running on my serial port to point to
> ttyNMI0.  Is that how it's supposed to work?  Then if I want to do a
> build without kgdb then I need to go in and change my agetty to point
> back at my normal serial port?
> 
> It kinda feels like a better way to much of what the driver does would be to:
> 
> 1. Allow kgdb to sniff incoming serial bytes on a port and look for
> its characters.  We already have this feature in the kernel to a small
> extent for sniffing a break / sysrq character.
> 
> 2. If userspace doesn't happen to have the serial port open then
> ideally we could open the port (using all the standard APIs that
> already exist) from in the kernel and just throw away all the bytes
> (since we already sniffed them).  As soon as userspace tried to open
> the port when it would get ownership and if userspace ever closed the
> port then we'd start reading / throwing away bytes again.
> 
> If we had a solution like that:
> 
> a) No serial drivers would need to change.
> 
> b) No kernel command line parameters would need to change.
> 
> Obviously that solution wouldn't magically get you an NMI, though.
> For that I'd presume the right answer would be to add a parameter for
> each serial driver that can support it to run its rx interrupt in NMI
> mode.

... or allow modal changes to the uart driver when kgdboc comes up?

We already allow UART drivers to de-optimize themselves and use
different code paths when polling is enabled so its not totally crazy
;-).


> Of course, perhaps I'm just confused and crazy and the above is a
> really bad idea.

Thanks for bringing this up.

Sumit and I were chatting last week and our discussion went in a similar
direction (I think not exactly the same which is why it is good to
see your thoughts too).

Personally I think it comes down to how intrusive adding NMI support is
to serial drivers. kgdb_nmi is rather hacky and feels a bit odd to
enable. It is clearly intended to avoid almost all changes to the UART
driver. On our side we have been wondering whether the serial core can
add helpers to make it easy for a serial driver to implement an simple,
safe but not optimal NMI implementation. Making it easy to have
safety-first might make NMI more palatable.


> Speaking of confused: is there actually any way to use the existing
> kgdb NMI driver (CONFIG_SERIAL_KGDB_NMI) in mainline without out of
> tree patches?  When I looked before I assumed it was just me that was
> outta luck because I didn't have NMI at the time, but I just did some
> grepping and I can't find anyplace in mainline where
> "arch_kgdb_ops.enable_nmi" would not be NULL.  Did I miss it, or do we
> need out-of-tree patches to enable this?

Out-of-tree...

If, after looking at other approaches, we do all agree to nuke kgdb_nmi
then there shouldn't be much impediment (nor that many tears).


Daniel.

Re: [PATCH 3/7] kgdb: Add request_nmi() to the io ops table for kgdboc

[Sumit Garg]

On Mon, 29 Jun 2020 at 17:15, Daniel Thompson
<daniel.thompson@linaro.org> wrote:
>
> On Fri, Jun 26, 2020 at 12:44:15PM -0700, Doug Anderson wrote:
> > Hi,
> >
> > On Tue, Jun 23, 2020 at 3:59 AM Daniel Thompson
> > <daniel.thompson@linaro.org> wrote:
> > >
> > > On Tue, Jun 23, 2020 at 02:07:47PM +0530, Sumit Garg wrote:
> > > > On Mon, 22 Jun 2020 at 21:33, Daniel Thompson
> > > > <daniel.thompson@linaro.org> wrote:
> > > > > > +     irq_set_status_flags(irq, IRQ_NOAUTOEN);
> > > > > > +     res = request_nmi(irq, fn, IRQF_PERCPU, "kgdboc", dev_id);
> > > > >
> > > > > Why do we need IRQF_PERCPU here. A UART interrupt is not normally
> > > > > per-cpu?
> > > > >
> > > >
> > > > Have a look at this comment [1] and corresponding check in
> > > > request_nmi(). So essentially yes UART interrupt is not normally
> > > > per-cpu but in order to make it an NMI, we need to request it in
> > > > per-cpu mode.
> > > >
> > > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/irq/manage.c#n2112
> > >
> > > Thanks! This is clear.
> > >
> > > > > > +     if (res) {
> > > > > > +             res = request_irq(irq, fn, IRQF_SHARED, "kgdboc", dev_id);
> > > > >
> > > > > IRQF_SHARED?
> > > > >
> > > > > Currrently there is nothing that prevents concurrent activation of
> > > > > ttyNMI0 and the underlying serial driver. Using IRQF_SHARED means it
> > > > > becomes possible for both drivers to try to service the same interrupt.
> > > > > That risks some rather "interesting" problems.
> > > > >
> > > >
> > > > Could you elaborate more on "interesting" problems?
> > >
> > > Er... one of the serial drivers we have allowed the userspace to open
> > > will, at best, be stone dead and not passing any characters.
> > >
> > >
> > > > BTW, I noticed one more problem with this patch that is IRQF_SHARED
> > > > doesn't go well with IRQ_NOAUTOEN status flag. Earlier I tested it
> > > > with auto enable set.
> > > >
> > > > But if we agree that both shouldn't be active at the same time due to
> > > > some real problems(?) then I can rid of IRQF_SHARED as well. Also, I
> > > > think we should unregister underlying tty driver (eg. /dev/ttyAMA0) as
> > > > well as otherwise it would provide a broken interface to user-space.
> > >
> > > I don't have a particular strong opinion on whether IRQF_SHARED is
> > > correct or not correct since I think that misses the point.
> > >
> > > Firstly, using IRQF_SHARED shows us that there is no interlocking
> > > between kgdb_nmi and the underlying serial driver. That probably tells
> > > us about the importance of the interlock than about IRQF_SHARED.
> > >
> > > To some extent I'm also unsure that kgdb_nmi could ever actually know
> > > the correct flags to use in all cases (that was another reason for the
> > > TODO comment about poll_get_irq() being a bogus API).
> >
> > I do wonder a little bit if the architecture of the "kgdb_nmi_console"
> > should change.  I remember looking at it in the past and thinking it a
> > little weird that if I wanted to get it to work I'd need to change my
> > "console=" command line to go through this new driver and (I guess)
> > change the agetty I have running on my serial port to point to
> > ttyNMI0.  Is that how it's supposed to work?  Then if I want to do a
> > build without kgdb then I need to go in and change my agetty to point
> > back at my normal serial port?
> >
> > It kinda feels like a better way to much of what the driver does would be to:
> >
> > 1. Allow kgdb to sniff incoming serial bytes on a port and look for
> > its characters.  We already have this feature in the kernel to a small
> > extent for sniffing a break / sysrq character.
> >
> > 2. If userspace doesn't happen to have the serial port open then
> > ideally we could open the port (using all the standard APIs that
> > already exist) from in the kernel and just throw away all the bytes
> > (since we already sniffed them).  As soon as userspace tried to open
> > the port when it would get ownership and if userspace ever closed the
> > port then we'd start reading / throwing away bytes again.
> >
> > If we had a solution like that:
> >
> > a) No serial drivers would need to change.
> >
> > b) No kernel command line parameters would need to change.
> >
> > Obviously that solution wouldn't magically get you an NMI, though.
> > For that I'd presume the right answer would be to add a parameter for
> > each serial driver that can support it to run its rx interrupt in NMI
> > mode.
>

Thanks Doug for the suggestions.

> ... or allow modal changes to the uart driver when kgdboc comes up?
>
> We already allow UART drivers to de-optimize themselves and use
> different code paths when polling is enabled so its not totally crazy
> ;-).
>
>
> > Of course, perhaps I'm just confused and crazy and the above is a
> > really bad idea.
>
> Thanks for bringing this up.
>
> Sumit and I were chatting last week and our discussion went in a similar
> direction (I think not exactly the same which is why it is good to
> see your thoughts too).
>
> Personally I think it comes down to how intrusive adding NMI support is
> to serial drivers. kgdb_nmi is rather hacky and feels a bit odd to
> enable. It is clearly intended to avoid almost all changes to the UART
> driver. On our side we have been wondering whether the serial core can
> add helpers to make it easy for a serial driver to implement an simple,
> safe but not optimal NMI implementation. Making it easy to have
> safety-first might make NMI more palatable.
>

I am currently working on a PoC in this direction and hopeful to come
up with least intrusive NMI support to serial drivers.

>
> > Speaking of confused: is there actually any way to use the existing
> > kgdb NMI driver (CONFIG_SERIAL_KGDB_NMI) in mainline without out of
> > tree patches?  When I looked before I assumed it was just me that was
> > outta luck because I didn't have NMI at the time, but I just did some
> > grepping and I can't find anyplace in mainline where
> > "arch_kgdb_ops.enable_nmi" would not be NULL.  Did I miss it, or do we
> > need out-of-tree patches to enable this?
>
> Out-of-tree...

Yeah and this patch-set derived from Daniel's work was one of them.

>
> If, after looking at other approaches, we do all agree to nuke kgdb_nmi
> then there shouldn't be much impediment (nor that many tears).
>

Makes sense.

-Sumit

>
> Daniel.