On Mon, 22 Jun 2020 at 21:33, Daniel Thompson
[off-list ref] wrote:

On Mon, Jun 22, 2020 at 07:56:20PM +0530, Sumit Garg wrote:

quoted

From: Daniel Thompson <redacted>

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 <redacted>
Co-developed-by: Sumit Garg <redacted>
Signed-off-by: Sumit Garg <redacted>
---
 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

quoted

+     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.

quoted

+             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