Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 22 Jun 2020 17:03:00 +0100
From:   Daniel Thompson <daniel.thompson@linaro.org>
To:     Sumit Garg <sumit.garg@linaro.org>
Cc:     kgdb-bugreport@lists.sourceforge.net, linux-serial@vger.kernel.org,
        gregkh@linuxfoundation.org, jason.wessel@windriver.com,
        dianders@chromium.org, jslaby@suse.com, linux@armlinux.org.uk,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 3/7] kgdb: Add request_nmi() to the io ops table for
 kgdboc
Message-ID: <20200622160300.avgfhnfkpqzqqtsr@holly.lan>
References: <1592835984-28613-1-git-send-email-sumit.garg@linaro.org>
 <1592835984-28613-4-git-send-email-sumit.garg@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1592835984-28613-4-git-send-email-sumit.garg@linaro.org>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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?


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


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
>