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