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

[Sumit Garg <sumit.garg@linaro.org>]

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