On Mon, 22 Jun 2020 at 21:33, Daniel Thompson <daniel.thompson@xxxxxxxxxx> wrote: > > On Mon, Jun 22, 2020 at 07:56:20PM +0530, Sumit Garg wrote: > > From: Daniel Thompson <daniel.thompson@xxxxxxxxxx> > > > > 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@xxxxxxxxxx> > > Co-developed-by: Sumit Garg <sumit.garg@xxxxxxxxxx> > > Signed-off-by: Sumit Garg <sumit.garg@xxxxxxxxxx> > > --- > > 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 > >