On Thu, 13 Aug 2020 at 05:29, Doug Anderson [off-list ref] wrote:

quoted

Hi,

On Tue, Jul 21, 2020 at 5:11 AM Sumit Garg [off-list ref] wrote:

quoted

Add NMI framework APIs in serial core which can be leveraged by serial
drivers to have NMI driven serial transfers. These APIs are kept under
CONFIG_CONSOLE_POLL as currently kgdb initializing uart in polling mode
is the only known user to enable NMI driven serial port.

The general idea is to intercept RX characters in NMI context, if those
are specific to magic sysrq then allow corresponding handler to run in
NMI context. Otherwise defer all other RX and TX operations to IRQ work
queue in order to run those in normal interrupt context.

Also, since magic sysrq entry APIs will need to be invoked from NMI
context, so make those APIs NMI safe via deferring NMI unsafe work to
IRQ work queue.

Signed-off-by: Sumit Garg <redacted>
---
 drivers/tty/serial/serial_core.c | 120 ++++++++++++++++++++++++++++++++++++++-
 include/linux/serial_core.h      |  67 ++++++++++++++++++++++
 2 files changed, 185 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 57840cf..6342e90 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c

@@ -3181,8 +3181,14 @@ static bool uart_try_toggle_sysrq(struct uart_port *port, unsigned int ch)
                return true;
        }

+#ifdef CONFIG_CONSOLE_POLL
+       if (in_nmi())
+               irq_work_queue(&port->nmi_state.sysrq_toggle_work);
+       else
+               schedule_work(&sysrq_enable_work);
+#else
        schedule_work(&sysrq_enable_work);
-
+#endif

It should be a very high bar to have #ifdefs inside functions.  I
don't think this meets it.  Instead maybe something like this
(untested and maybe slightly wrong syntax, but hopefully makes
sense?):

Outside the function:

#ifdef CONFIG_CONSOLE_POLL
#define queue_port_nmi_work(port, work_type)
irq_work_queue(&port->nmi_state.work_type)
#else
#define queue_port_nmi_work(port, work_type)
#endif

...and then:

if (IS_ENABLED(CONFIG_CONSOLE_POLL) && in_nmi())
  queue_port_nmi_work(port, sysrq_toggle_work);
else
  schedule_work(&sysrq_enable_work);

---

The whole double-hopping is really quite annoying.  I guess
schedule_work() can't be called from NMI context but can be called
from IRQ context?  So you need to first transition from NMI context to
IRQ context and then go and schedule the work?  Almost feels like we
should just fix schedule_work() to do this double-hop for you if
called from NMI context.  Seems like you could even re-use the list
pointers in the work_struct to keep the queue of people who need to be
scheduled from the next irq_work?  Worst case it seems like you could
add a schedule_work_nmi() that would do all the hoops for you.  ...but
I also know very little about NMI so maybe I'm being naive.

Thanks for this suggestion and yes indeed we could make
schedule_work() NMI safe and in turn get rid of all this #ifdefs. Have
a look at below changes:

diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
index 26de0ca..1daf1b4 100644
--- a/include/linux/workqueue.h
+++ b/include/linux/workqueue.h

@@ -14,6 +14,7 @@
 #include <linux/atomic.h>
 #include <linux/cpumask.h>
 #include <linux/rcupdate.h>
+#include <linux/irq_work.h>

 struct workqueue_struct;

@@ -106,6 +107,7 @@ struct work_struct {
 #ifdef CONFIG_LOCKDEP
        struct lockdep_map lockdep_map;
 #endif
+       struct irq_work iw;