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:   Fri, 26 Jun 2020 13:13:48 +0100
From:   Daniel Thompson <daniel.thompson@linaro.org>
To:     Sumit Garg <sumit.garg@linaro.org>
Cc:     kgdb-bugreport@lists.sourceforge.net, jason.wessel@windriver.com,
        dianders@chromium.org, pmladek@suse.com,
        sergey.senozhatsky@gmail.com, gregkh@linuxfoundation.org,
        jslaby@suse.com, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v6 4/4] kdb: Switch to use safer dbg_io_ops over console
 APIs
Message-ID: <20200626121348.eeutt4py34f2yg3y@holly.lan>
References: <1591264879-25920-1-git-send-email-sumit.garg@linaro.org>
 <1591264879-25920-5-git-send-email-sumit.garg@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1591264879-25920-5-git-send-email-sumit.garg@linaro.org>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Greg

This patch touches some kgdb related code in both serial and usb trees.

Any objections to my queuing this via the kgdb tree?

After testing I've concluded that this, and its friends (which only
touch kgdb), fix enough problems that I plan to queue it for v5.8.


Daniel.


On Thu, Jun 04, 2020 at 03:31:19PM +0530, Sumit Garg wrote:
> In kgdb context, calling console handlers aren't safe due to locks used
> in those handlers which could in turn lead to a deadlock. Although, using
> oops_in_progress increases the chance to bypass locks in most console
> handlers but it might not be sufficient enough in case a console uses
> more locks (VT/TTY is good example).
> 
> Currently when a driver provides both polling I/O and a console then kdb
> will output using the console. We can increase robustness by using the
> currently active polling I/O driver (which should be lockless) instead
> of the corresponding console. For several common cases (e.g. an
> embedded system with a single serial port that is used both for console
> output and debugger I/O) this will result in no console handler being
> used.
> 
> In order to achieve this we need to reverse the order of preference to
> use dbg_io_ops (uses polling I/O mode) over console APIs. So we just
> store "struct console" that represents debugger I/O in dbg_io_ops and
> while emitting kdb messages, skip console that matches dbg_io_ops
> console in order to avoid duplicate messages. After this change,
> "is_console" param becomes redundant and hence removed.
> 
> Suggested-by: Daniel Thompson <daniel.thompson@linaro.org>
> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> ---
>  drivers/tty/serial/kgdb_nmi.c |  2 +-
>  drivers/tty/serial/kgdboc.c   | 32 ++++++++++++++++----------------
>  drivers/usb/early/ehci-dbgp.c |  3 ++-
>  include/linux/kgdb.h          |  5 ++---
>  kernel/debug/kdb/kdb_io.c     |  4 +++-
>  5 files changed, 24 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/tty/serial/kgdb_nmi.c b/drivers/tty/serial/kgdb_nmi.c
> index 5022447..6004c0c 100644
> --- a/drivers/tty/serial/kgdb_nmi.c
> +++ b/drivers/tty/serial/kgdb_nmi.c
> @@ -50,7 +50,7 @@ static int kgdb_nmi_console_setup(struct console *co, char *options)
>  	 * I/O utilities that messages sent to the console will automatically
>  	 * be displayed on the dbg_io.
>  	 */
> -	dbg_io_ops->is_console = true;
> +	dbg_io_ops->cons = co;
>  
>  	return 0;
>  }
> diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c
> index 4139698..84ffede 100644
> --- a/drivers/tty/serial/kgdboc.c
> +++ b/drivers/tty/serial/kgdboc.c
> @@ -45,7 +45,6 @@ static struct platform_device *kgdboc_pdev;
>  
>  #if IS_BUILTIN(CONFIG_KGDB_SERIAL_CONSOLE)
>  static struct kgdb_io		kgdboc_earlycon_io_ops;
> -static struct console		*earlycon;
>  static int                      (*earlycon_orig_exit)(struct console *con);
>  #endif /* IS_BUILTIN(CONFIG_KGDB_SERIAL_CONSOLE) */
>  
> @@ -145,7 +144,7 @@ static void kgdboc_unregister_kbd(void)
>  #if IS_BUILTIN(CONFIG_KGDB_SERIAL_CONSOLE)
>  static void cleanup_earlycon(void)
>  {
> -	if (earlycon)
> +	if (kgdboc_earlycon_io_ops.cons)
>  		kgdb_unregister_io_module(&kgdboc_earlycon_io_ops);
>  }
>  #else /* !IS_BUILTIN(CONFIG_KGDB_SERIAL_CONSOLE) */
> @@ -178,7 +177,7 @@ static int configure_kgdboc(void)
>  		goto noconfig;
>  	}
>  
> -	kgdboc_io_ops.is_console = 0;
> +	kgdboc_io_ops.cons = NULL;
>  	kgdb_tty_driver = NULL;
>  
>  	kgdboc_use_kms = 0;
> @@ -198,7 +197,7 @@ static int configure_kgdboc(void)
>  		int idx;
>  		if (cons->device && cons->device(cons, &idx) == p &&
>  		    idx == tty_line) {
> -			kgdboc_io_ops.is_console = 1;
> +			kgdboc_io_ops.cons = cons;
>  			break;
>  		}
>  	}
> @@ -433,7 +432,8 @@ static int kgdboc_earlycon_get_char(void)
>  {
>  	char c;
>  
> -	if (!earlycon->read(earlycon, &c, 1))
> +	if (!kgdboc_earlycon_io_ops.cons->read(kgdboc_earlycon_io_ops.cons,
> +					       &c, 1))
>  		return NO_POLL_CHAR;
>  
>  	return c;
> @@ -441,7 +441,8 @@ static int kgdboc_earlycon_get_char(void)
>  
>  static void kgdboc_earlycon_put_char(u8 chr)
>  {
> -	earlycon->write(earlycon, &chr, 1);
> +	kgdboc_earlycon_io_ops.cons->write(kgdboc_earlycon_io_ops.cons, &chr,
> +					   1);
>  }
>  
>  static void kgdboc_earlycon_pre_exp_handler(void)
> @@ -461,7 +462,7 @@ static void kgdboc_earlycon_pre_exp_handler(void)
>  	 * boot if we detect this case.
>  	 */
>  	for_each_console(con)
> -		if (con == earlycon)
> +		if (con == kgdboc_earlycon_io_ops.cons)
>  			return;
>  
>  	already_warned = true;
> @@ -484,25 +485,25 @@ static int kgdboc_earlycon_deferred_exit(struct console *con)
>  
>  static void kgdboc_earlycon_deinit(void)
>  {
> -	if (!earlycon)
> +	if (!kgdboc_earlycon_io_ops.cons)
>  		return;
>  
> -	if (earlycon->exit == kgdboc_earlycon_deferred_exit)
> +	if (kgdboc_earlycon_io_ops.cons->exit == kgdboc_earlycon_deferred_exit)
>  		/*
>  		 * kgdboc_earlycon is exiting but original boot console exit
>  		 * was never called (AKA kgdboc_earlycon_deferred_exit()
>  		 * didn't ever run).  Undo our trap.
>  		 */
> -		earlycon->exit = earlycon_orig_exit;
> -	else if (earlycon->exit)
> +		kgdboc_earlycon_io_ops.cons->exit = earlycon_orig_exit;
> +	else if (kgdboc_earlycon_io_ops.cons->exit)
>  		/*
>  		 * We skipped calling the exit() routine so we could try to
>  		 * keep using the boot console even after it went away.  We're
>  		 * finally done so call the function now.
>  		 */
> -		earlycon->exit(earlycon);
> +		kgdboc_earlycon_io_ops.cons->exit(kgdboc_earlycon_io_ops.cons);
>  
> -	earlycon = NULL;
> +	kgdboc_earlycon_io_ops.cons = NULL;
>  }
>  
>  static struct kgdb_io kgdboc_earlycon_io_ops = {
> @@ -511,7 +512,6 @@ static struct kgdb_io kgdboc_earlycon_io_ops = {
>  	.write_char		= kgdboc_earlycon_put_char,
>  	.pre_exception		= kgdboc_earlycon_pre_exp_handler,
>  	.deinit			= kgdboc_earlycon_deinit,
> -	.is_console		= true,
>  };
>  
>  #define MAX_CONSOLE_NAME_LEN (sizeof((struct console *) 0)->name)
> @@ -557,10 +557,10 @@ static int __init kgdboc_earlycon_init(char *opt)
>  		goto unlock;
>  	}
>  
> -	earlycon = con;
> +	kgdboc_earlycon_io_ops.cons = con;
>  	pr_info("Going to register kgdb with earlycon '%s'\n", con->name);
>  	if (kgdb_register_io_module(&kgdboc_earlycon_io_ops) != 0) {
> -		earlycon = NULL;
> +		kgdboc_earlycon_io_ops.cons = NULL;
>  		pr_info("Failed to register kgdb with earlycon\n");
>  	} else {
>  		/* Trap exit so we can keep earlycon longer if needed. */
> diff --git a/drivers/usb/early/ehci-dbgp.c b/drivers/usb/early/ehci-dbgp.c
> index ea0d531..775cf70 100644
> --- a/drivers/usb/early/ehci-dbgp.c
> +++ b/drivers/usb/early/ehci-dbgp.c
> @@ -1058,7 +1058,8 @@ static int __init kgdbdbgp_parse_config(char *str)
>  		kgdbdbgp_wait_time = simple_strtoul(ptr, &ptr, 10);
>  	}
>  	kgdb_register_io_module(&kgdbdbgp_io_ops);
> -	kgdbdbgp_io_ops.is_console = early_dbgp_console.index != -1;
> +	if (early_dbgp_console.index != -1)
> +		kgdbdbgp_io_ops.cons = &early_dbgp_console;
>  
>  	return 0;
>  }
> diff --git a/include/linux/kgdb.h b/include/linux/kgdb.h
> index c62d764..529116b 100644
> --- a/include/linux/kgdb.h
> +++ b/include/linux/kgdb.h
> @@ -276,8 +276,7 @@ struct kgdb_arch {
>   * the I/O driver.
>   * @post_exception: Pointer to a function that will do any cleanup work
>   * for the I/O driver.
> - * @is_console: 1 if the end device is a console 0 if the I/O device is
> - * not a console
> + * @cons: valid if the I/O device is a console; else NULL.
>   */
>  struct kgdb_io {
>  	const char		*name;
> @@ -288,7 +287,7 @@ struct kgdb_io {
>  	void			(*deinit) (void);
>  	void			(*pre_exception) (void);
>  	void			(*post_exception) (void);
> -	int			is_console;
> +	struct console		*cons;
>  };
>  
>  extern const struct kgdb_arch		arch_kgdb_ops;
> diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
> index 0e4f2ed..683a799 100644
> --- a/kernel/debug/kdb/kdb_io.c
> +++ b/kernel/debug/kdb/kdb_io.c
> @@ -549,7 +549,7 @@ static void kdb_msg_write(const char *msg, int msg_len)
>  	if (msg_len == 0)
>  		return;
>  
> -	if (dbg_io_ops && !dbg_io_ops->is_console) {
> +	if (dbg_io_ops) {
>  		const char *cp = msg;
>  		int len = msg_len;
>  
> @@ -562,6 +562,8 @@ static void kdb_msg_write(const char *msg, int msg_len)
>  	for_each_console(c) {
>  		if (!(c->flags & CON_ENABLED))
>  			continue;
> +		if (c == dbg_io_ops->cons)
> +			continue;
>  		/*
>  		 * Set oops_in_progress to encourage the console drivers to
>  		 * disregard their internal spin locks: in the current calling
> -- 
> 2.7.4