[PATCH v3 0/4] kdb: Improve console handling

[PATCH v3 0/4] kdb: Improve console handling

[Sumit Garg]

This patch-set is aimed to improve console handling especially when kdb
operates in NMI context.

Brief description of enhancements:
- Add status check for console prior to invoking corresponding handler.
- Fixup to avoid possible deadlock in NMI context due to usage of locks
  in the console handlers.
- Prefer usage of polling I/O driver mode (lockless APIs) over invocation
  of console handlers.

Changes in v3:
- Split patch to have separate patch for console status check.
- New patch to re-factor kdb message emit code.
- New patch to prefer polling I/O over console mode.
- Add code comments to describe usage of oops_in_progress.

Changes in v2:
- Use oops_in_progress directly instead of bust_spinlocks().

Sumit Garg (4):
  kdb: Re-factor kdb_printf() message write code
  kdb: Check status of console prior to invoking handlers
  kdb: Make kdb_printf robust to run in NMI context
  kdb: Switch kdb_msg_write() to use safer polling I/O

 drivers/tty/serial/kgdboc.c | 17 ++++-----
 include/linux/kgdb.h        |  2 +
 kernel/debug/kdb/kdb_io.c   | 91 ++++++++++++++++++++++++++++++---------------
 3 files changed, 72 insertions(+), 38 deletions(-)

-- 
2.7.4

[PATCH v3 1/4] kdb: Re-factor kdb_printf() message write code

[Sumit Garg]

Re-factor kdb_printf() message write code in order to avoid duplication
of code and thereby increase readability.

Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
---
 kernel/debug/kdb/kdb_io.c | 61 +++++++++++++++++++++++++----------------------
 1 file changed, 32 insertions(+), 29 deletions(-)

diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
index 924bc92..f6b4d47 100644
--- a/kernel/debug/kdb/kdb_io.c
+++ b/kernel/debug/kdb/kdb_io.c
@@ -542,6 +542,33 @@ static int kdb_search_string(char *searched, char *searchfor)
 	return 0;
 }
 
+static void kdb_io_write(char *cp, int len, void (*io_put_char)(u8 ch))
+{
+	if (len <= 0)
+		return;
+
+	while (len--) {
+		io_put_char(*cp);
+		cp++;
+	}
+}
+
+static void kdb_msg_write(char *msg, int msg_len)
+{
+	struct console *c;
+
+	if (msg_len <= 0)
+		return;
+
+	if (dbg_io_ops && !dbg_io_ops->is_console)
+		kdb_io_write(msg, msg_len, dbg_io_ops->write_char);
+
+	for_each_console(c) {
+		c->write(c, msg, msg_len);
+		touch_nmi_watchdog();
+	}
+}
+
 int vkdb_printf(enum kdb_msgsrc src, const char *fmt, va_list ap)
 {
 	int diag;
@@ -553,7 +580,6 @@ int vkdb_printf(enum kdb_msgsrc src, const char *fmt, va_list ap)
 	int this_cpu, old_cpu;
 	char *cp, *cp2, *cphold = NULL, replaced_byte = ' ';
 	char *moreprompt = "more> ";
-	struct console *c;
 	unsigned long uninitialized_var(flags);
 
 	/* Serialize kdb_printf if multiple cpus try to write at once.
@@ -687,22 +713,11 @@ int vkdb_printf(enum kdb_msgsrc src, const char *fmt, va_list ap)
 	 */
 	retlen = strlen(kdb_buffer);
 	cp = (char *) printk_skip_headers(kdb_buffer);
-	if (!dbg_kdb_mode && kgdb_connected) {
+	if (!dbg_kdb_mode && kgdb_connected)
 		gdbstub_msg_write(cp, retlen - (cp - kdb_buffer));
-	} else {
-		if (dbg_io_ops && !dbg_io_ops->is_console) {
-			len = retlen - (cp - kdb_buffer);
-			cp2 = cp;
-			while (len--) {
-				dbg_io_ops->write_char(*cp2);
-				cp2++;
-			}
-		}
-		for_each_console(c) {
-			c->write(c, cp, retlen - (cp - kdb_buffer));
-			touch_nmi_watchdog();
-		}
-	}
+	else
+		kdb_msg_write(cp, retlen - (cp - kdb_buffer));
+
 	if (logging) {
 		saved_loglevel = console_loglevel;
 		console_loglevel = CONSOLE_LOGLEVEL_SILENT;
@@ -751,19 +766,7 @@ int vkdb_printf(enum kdb_msgsrc src, const char *fmt, va_list ap)
 			moreprompt = "more> ";
 
 		kdb_input_flush();
-
-		if (dbg_io_ops && !dbg_io_ops->is_console) {
-			len = strlen(moreprompt);
-			cp = moreprompt;
-			while (len--) {
-				dbg_io_ops->write_char(*cp);
-				cp++;
-			}
-		}
-		for_each_console(c) {
-			c->write(c, moreprompt, strlen(moreprompt));
-			touch_nmi_watchdog();
-		}
+		kdb_msg_write(moreprompt, strlen(moreprompt));
 
 		if (logging)
 			printk("%s", moreprompt);
-- 
2.7.4

[PATCH v3 2/4] kdb: Check status of console prior to invoking handlers

[Sumit Garg]

Check if a console is enabled prior to invoking corresponding write
handler.

Suggested-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
---
 kernel/debug/kdb/kdb_io.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
index f6b4d47..349dfcc 100644
--- a/kernel/debug/kdb/kdb_io.c
+++ b/kernel/debug/kdb/kdb_io.c
@@ -564,6 +564,8 @@ static void kdb_msg_write(char *msg, int msg_len)
 		kdb_io_write(msg, msg_len, dbg_io_ops->write_char);
 
 	for_each_console(c) {
+		if (!(c->flags & CON_ENABLED))
+			continue;
 		c->write(c, msg, msg_len);
 		touch_nmi_watchdog();
 	}
-- 
2.7.4

[PATCH v3 3/4] kdb: Make kdb_printf robust to run in NMI context

[Sumit Garg]

While rounding up CPUs via NMIs, its possible that a rounded up CPU
maybe holding a console port lock leading to kgdb master CPU stuck in
a deadlock during invocation of console write operations. So in order
to avoid such a deadlock, enable oops_in_progress prior to invocation
of console handlers.

Suggested-by: Petr Mladek <pmladek@suse.com>
Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
---
 kernel/debug/kdb/kdb_io.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
index 349dfcc..f848482 100644
--- a/kernel/debug/kdb/kdb_io.c
+++ b/kernel/debug/kdb/kdb_io.c
@@ -566,7 +566,17 @@ static void kdb_msg_write(char *msg, int msg_len)
 	for_each_console(c) {
 		if (!(c->flags & CON_ENABLED))
 			continue;
+		/*
+		 * While rounding up CPUs via NMIs, its possible that
+		 * a rounded up CPU maybe holding a console port lock
+		 * leading to kgdb master CPU stuck in a deadlock during
+		 * invocation of console write operations. So in order
+		 * to avoid such a deadlock, enable oops_in_progress
+		 * prior to invocation of console handlers.
+		 */
+		++oops_in_progress;
 		c->write(c, msg, msg_len);
+		--oops_in_progress;
 		touch_nmi_watchdog();
 	}
 }
-- 
2.7.4

[PATCH v3 4/4] kdb: Switch kdb_msg_write() to use safer polling I/O

[Sumit Garg]

In kgdb NMI context, calling console handlers isn't safe due to locks
used in those handlers which could 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.

Suggested-by: Daniel Thompson <daniel.thompson@linaro.org>
Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
---
 drivers/tty/serial/kgdboc.c | 17 ++++++++---------
 include/linux/kgdb.h        |  2 ++
 kernel/debug/kdb/kdb_io.c   | 46 +++++++++++++++++++++++++++++++--------------
 3 files changed, 42 insertions(+), 23 deletions(-)

diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c
index c9f94fa..6199fe1 100644
--- a/drivers/tty/serial/kgdboc.c
+++ b/drivers/tty/serial/kgdboc.c
@@ -35,7 +35,6 @@ static struct kparam_string kps = {
 };
 
 static int kgdboc_use_kms;  /* 1 if we use kernel mode switching */
-static struct tty_driver	*kgdb_tty_driver;
 static int			kgdb_tty_line;
 
 #ifdef CONFIG_KDB_KEYBOARD
@@ -154,7 +153,7 @@ static int configure_kgdboc(void)
 	}
 
 	kgdboc_io_ops.is_console = 0;
-	kgdb_tty_driver = NULL;
+	kgdboc_io_ops.tty_drv = NULL;
 
 	kgdboc_use_kms = 0;
 	if (strncmp(cptr, "kms,", 4) == 0) {
@@ -178,7 +177,7 @@ static int configure_kgdboc(void)
 		}
 	}
 
-	kgdb_tty_driver = p;
+	kgdboc_io_ops.tty_drv = p;
 	kgdb_tty_line = tty_line;
 
 do_register:
@@ -216,18 +215,18 @@ static int __init init_kgdboc(void)
 
 static int kgdboc_get_char(void)
 {
-	if (!kgdb_tty_driver)
+	if (!kgdboc_io_ops.tty_drv)
 		return -1;
-	return kgdb_tty_driver->ops->poll_get_char(kgdb_tty_driver,
-						kgdb_tty_line);
+	return kgdboc_io_ops.tty_drv->ops->poll_get_char(kgdboc_io_ops.tty_drv,
+							 kgdb_tty_line);
 }
 
 static void kgdboc_put_char(u8 chr)
 {
-	if (!kgdb_tty_driver)
+	if (!kgdboc_io_ops.tty_drv)
 		return;
-	kgdb_tty_driver->ops->poll_put_char(kgdb_tty_driver,
-					kgdb_tty_line, chr);
+	kgdboc_io_ops.tty_drv->ops->poll_put_char(kgdboc_io_ops.tty_drv,
+						  kgdb_tty_line, chr);
 }
 
 static int param_set_kgdboc_var(const char *kmessage,
diff --git a/include/linux/kgdb.h b/include/linux/kgdb.h
index b072aeb..05d165d 100644
--- a/include/linux/kgdb.h
+++ b/include/linux/kgdb.h
@@ -275,6 +275,7 @@ struct kgdb_arch {
  * 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
+ * @tty_drv: Pointer to polling tty driver.
  */
 struct kgdb_io {
 	const char		*name;
@@ -285,6 +286,7 @@ struct kgdb_io {
 	void			(*pre_exception) (void);
 	void			(*post_exception) (void);
 	int			is_console;
+	struct tty_driver	*tty_drv;
 };
 
 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 f848482..c2efa52 100644
--- a/kernel/debug/kdb/kdb_io.c
+++ b/kernel/debug/kdb/kdb_io.c
@@ -24,6 +24,7 @@
 #include <linux/kgdb.h>
 #include <linux/kdb.h>
 #include <linux/kallsyms.h>
+#include <linux/tty_driver.h>
 #include "kdb_private.h"
 
 #define CMD_BUFLEN 256
@@ -542,13 +543,18 @@ static int kdb_search_string(char *searched, char *searchfor)
 	return 0;
 }
 
-static void kdb_io_write(char *cp, int len, void (*io_put_char)(u8 ch))
+static void kdb_io_write(char *cp, int len, void (*io_put_char)(u8),
+			 struct tty_driver *p, int line,
+			 void (*poll_put_char)(struct tty_driver *, int, char))
 {
 	if (len <= 0)
 		return;
 
 	while (len--) {
-		io_put_char(*cp);
+		if (io_put_char)
+			io_put_char(*cp);
+		if (poll_put_char)
+			poll_put_char(p, line, *cp);
 		cp++;
 	}
 }
@@ -561,22 +567,34 @@ static void kdb_msg_write(char *msg, int msg_len)
 		return;
 
 	if (dbg_io_ops && !dbg_io_ops->is_console)
-		kdb_io_write(msg, msg_len, dbg_io_ops->write_char);
+		kdb_io_write(msg, msg_len, dbg_io_ops->write_char,
+			     NULL, 0, NULL);
 
 	for_each_console(c) {
+		int line;
+		struct tty_driver *p;
+
 		if (!(c->flags & CON_ENABLED))
 			continue;
-		/*
-		 * While rounding up CPUs via NMIs, its possible that
-		 * a rounded up CPU maybe holding a console port lock
-		 * leading to kgdb master CPU stuck in a deadlock during
-		 * invocation of console write operations. So in order
-		 * to avoid such a deadlock, enable oops_in_progress
-		 * prior to invocation of console handlers.
-		 */
-		++oops_in_progress;
-		c->write(c, msg, msg_len);
-		--oops_in_progress;
+
+		p = c->device ? c->device(c, &line) : NULL;
+		if (p && dbg_io_ops && p == dbg_io_ops->tty_drv && p->ops &&
+		    p->ops->poll_put_char) {
+			kdb_io_write(msg, msg_len, NULL, p, line,
+				     p->ops->poll_put_char);
+		} else {
+			/*
+			 * While rounding up CPUs via NMIs, its possible that
+			 * a rounded up CPU maybe holding a console port lock
+			 * leading to kgdb master CPU stuck in a deadlock during
+			 * invocation of console write operations. So in order
+			 * to avoid such a deadlock, enable oops_in_progress
+			 * prior to invocation of console handlers.
+			 */
+			++oops_in_progress;
+			c->write(c, msg, msg_len);
+			--oops_in_progress;
+		}
 		touch_nmi_watchdog();
 	}
 }
-- 
2.7.4

Re: [PATCH v3 1/4] kdb: Re-factor kdb_printf() message write code

[Daniel Thompson]

On Wed, May 27, 2020 at 11:55:56AM +0530, Sumit Garg wrote:
> Re-factor kdb_printf() message write code in order to avoid duplication
> of code and thereby increase readability.
> 
> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> ---
>  kernel/debug/kdb/kdb_io.c | 61 +++++++++++++++++++++++++----------------------
>  1 file changed, 32 insertions(+), 29 deletions(-)
> 
> diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
> index 924bc92..f6b4d47 100644
> --- a/kernel/debug/kdb/kdb_io.c
> +++ b/kernel/debug/kdb/kdb_io.c
> @@ -542,6 +542,33 @@ static int kdb_search_string(char *searched, char *searchfor)
>  	return 0;
>  }
>  
> +static void kdb_io_write(char *cp, int len, void (*io_put_char)(u8 ch))

Don't use a function pointer here. Just pick it up from dbg_io_ops as
usual.


> +{
> +	if (len <= 0)
> +		return;

How can len ever be negative?


> +
> +	while (len--) {
> +		io_put_char(*cp);
> +		cp++;
> +	}
> +}
> +
> +static void kdb_msg_write(char *msg, int msg_len)
> +{
> +	struct console *c;
> +
> +	if (msg_len <= 0)
> +		return;

How can msg_len ever be negative?


> +
> +	if (dbg_io_ops && !dbg_io_ops->is_console)
> +		kdb_io_write(msg, msg_len, dbg_io_ops->write_char);
> +
> +	for_each_console(c) {
> +		c->write(c, msg, msg_len);
> +		touch_nmi_watchdog();
> +	}
> +}
> +
>  int vkdb_printf(enum kdb_msgsrc src, const char *fmt, va_list ap)
>  {
>  	int diag;
> @@ -553,7 +580,6 @@ int vkdb_printf(enum kdb_msgsrc src, const char *fmt, va_list ap)
>  	int this_cpu, old_cpu;
>  	char *cp, *cp2, *cphold = NULL, replaced_byte = ' ';
>  	char *moreprompt = "more> ";
> -	struct console *c;
>  	unsigned long uninitialized_var(flags);
>  
>  	/* Serialize kdb_printf if multiple cpus try to write at once.
> @@ -687,22 +713,11 @@ int vkdb_printf(enum kdb_msgsrc src, const char *fmt, va_list ap)
>  	 */
>  	retlen = strlen(kdb_buffer);
>  	cp = (char *) printk_skip_headers(kdb_buffer);
> -	if (!dbg_kdb_mode && kgdb_connected) {
> +	if (!dbg_kdb_mode && kgdb_connected)
>  		gdbstub_msg_write(cp, retlen - (cp - kdb_buffer));
> -	} else {
> -		if (dbg_io_ops && !dbg_io_ops->is_console) {
> -			len = retlen - (cp - kdb_buffer);
> -			cp2 = cp;
> -			while (len--) {
> -				dbg_io_ops->write_char(*cp2);
> -				cp2++;
> -			}
> -		}
> -		for_each_console(c) {
> -			c->write(c, cp, retlen - (cp - kdb_buffer));
> -			touch_nmi_watchdog();
> -		}
> -	}
> +	else
> +		kdb_msg_write(cp, retlen - (cp - kdb_buffer));
> +
>  	if (logging) {
>  		saved_loglevel = console_loglevel;
>  		console_loglevel = CONSOLE_LOGLEVEL_SILENT;
> @@ -751,19 +766,7 @@ int vkdb_printf(enum kdb_msgsrc src, const char *fmt, va_list ap)
>  			moreprompt = "more> ";
>  
>  		kdb_input_flush();
> -
> -		if (dbg_io_ops && !dbg_io_ops->is_console) {
> -			len = strlen(moreprompt);
> -			cp = moreprompt;
> -			while (len--) {
> -				dbg_io_ops->write_char(*cp);
> -				cp++;
> -			}
> -		}
> -		for_each_console(c) {
> -			c->write(c, moreprompt, strlen(moreprompt));
> -			touch_nmi_watchdog();
> -		}
> +		kdb_msg_write(moreprompt, strlen(moreprompt));
>  
>  		if (logging)
>  			printk("%s", moreprompt);
> -- 
> 2.7.4
> 

Re: [PATCH v3 1/4] kdb: Re-factor kdb_printf() message write code

[Sumit Garg]

On Wed, 27 May 2020 at 13:59, Daniel Thompson
<daniel.thompson@linaro.org> wrote:
>
> On Wed, May 27, 2020 at 11:55:56AM +0530, Sumit Garg wrote:
> > Re-factor kdb_printf() message write code in order to avoid duplication
> > of code and thereby increase readability.
> >
> > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> > ---
> >  kernel/debug/kdb/kdb_io.c | 61 +++++++++++++++++++++++++----------------------
> >  1 file changed, 32 insertions(+), 29 deletions(-)
> >
> > diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
> > index 924bc92..f6b4d47 100644
> > --- a/kernel/debug/kdb/kdb_io.c
> > +++ b/kernel/debug/kdb/kdb_io.c
> > @@ -542,6 +542,33 @@ static int kdb_search_string(char *searched, char *searchfor)
> >       return 0;
> >  }
> >
> > +static void kdb_io_write(char *cp, int len, void (*io_put_char)(u8 ch))
>
> Don't use a function pointer here. Just pick it up from dbg_io_ops as
> usual.

My initial intent to use function pointer here was to extend this API
in patch #4 for poll_put_char() as well. But it just came to my mind
after your comment that internally dbg_io_ops->write_char() fallbacks
to tty_drv->ops->poll_put_char() API only. So I don't need to do any
crazy things with function pointers here in order to avoid a duplicate
loop but can simply invoke dbg_io_ops->write_char() here instead.

>
> > +{
> > +     if (len <= 0)
> > +             return;
>
> How can len ever be negative?
>

The only rationale to have this check is for completeness as the type
of variable: "len" being "int". If you don't prefer such checks, then
I can replace it with an "==" check.

>
> > +
> > +     while (len--) {
> > +             io_put_char(*cp);
> > +             cp++;
> > +     }
> > +}
> > +
> > +static void kdb_msg_write(char *msg, int msg_len)
> > +{
> > +     struct console *c;
> > +
> > +     if (msg_len <= 0)
> > +             return;
>
> How can msg_len ever be negative?
>

Same as above.

-Sumit

>
> > +
> > +     if (dbg_io_ops && !dbg_io_ops->is_console)
> > +             kdb_io_write(msg, msg_len, dbg_io_ops->write_char);
> > +
> > +     for_each_console(c) {
> > +             c->write(c, msg, msg_len);
> > +             touch_nmi_watchdog();
> > +     }
> > +}
> > +
> >  int vkdb_printf(enum kdb_msgsrc src, const char *fmt, va_list ap)
> >  {
> >       int diag;
> > @@ -553,7 +580,6 @@ int vkdb_printf(enum kdb_msgsrc src, const char *fmt, va_list ap)
> >       int this_cpu, old_cpu;
> >       char *cp, *cp2, *cphold = NULL, replaced_byte = ' ';
> >       char *moreprompt = "more> ";
> > -     struct console *c;
> >       unsigned long uninitialized_var(flags);
> >
> >       /* Serialize kdb_printf if multiple cpus try to write at once.
> > @@ -687,22 +713,11 @@ int vkdb_printf(enum kdb_msgsrc src, const char *fmt, va_list ap)
> >        */
> >       retlen = strlen(kdb_buffer);
> >       cp = (char *) printk_skip_headers(kdb_buffer);
> > -     if (!dbg_kdb_mode && kgdb_connected) {
> > +     if (!dbg_kdb_mode && kgdb_connected)
> >               gdbstub_msg_write(cp, retlen - (cp - kdb_buffer));
> > -     } else {
> > -             if (dbg_io_ops && !dbg_io_ops->is_console) {
> > -                     len = retlen - (cp - kdb_buffer);
> > -                     cp2 = cp;
> > -                     while (len--) {
> > -                             dbg_io_ops->write_char(*cp2);
> > -                             cp2++;
> > -                     }
> > -             }
> > -             for_each_console(c) {
> > -                     c->write(c, cp, retlen - (cp - kdb_buffer));
> > -                     touch_nmi_watchdog();
> > -             }
> > -     }
> > +     else
> > +             kdb_msg_write(cp, retlen - (cp - kdb_buffer));
> > +
> >       if (logging) {
> >               saved_loglevel = console_loglevel;
> >               console_loglevel = CONSOLE_LOGLEVEL_SILENT;
> > @@ -751,19 +766,7 @@ int vkdb_printf(enum kdb_msgsrc src, const char *fmt, va_list ap)
> >                       moreprompt = "more> ";
> >
> >               kdb_input_flush();
> > -
> > -             if (dbg_io_ops && !dbg_io_ops->is_console) {
> > -                     len = strlen(moreprompt);
> > -                     cp = moreprompt;
> > -                     while (len--) {
> > -                             dbg_io_ops->write_char(*cp);
> > -                             cp++;
> > -                     }
> > -             }
> > -             for_each_console(c) {
> > -                     c->write(c, moreprompt, strlen(moreprompt));
> > -                     touch_nmi_watchdog();
> > -             }
> > +             kdb_msg_write(moreprompt, strlen(moreprompt));
> >
> >               if (logging)
> >                       printk("%s", moreprompt);
> > --
> > 2.7.4
> >

Re: [PATCH v3 4/4] kdb: Switch kdb_msg_write() to use safer polling I/O

[Daniel Thompson]

On Wed, May 27, 2020 at 11:55:59AM +0530, Sumit Garg wrote:
> In kgdb NMI context, calling console handlers isn't safe due to locks
> used in those handlers which could 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.

Not sure I would have predicted all those changes to kgdboc.c based on
this patch description. I assume this is to help identify which console
matches our dbg_io_ops but it would be good to spell this out.


> Suggested-by: Daniel Thompson <daniel.thompson@linaro.org>
> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> ---
>  drivers/tty/serial/kgdboc.c | 17 ++++++++---------
>  include/linux/kgdb.h        |  2 ++
>  kernel/debug/kdb/kdb_io.c   | 46 +++++++++++++++++++++++++++++++--------------
>  3 files changed, 42 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c
> index c9f94fa..6199fe1 100644
> --- a/drivers/tty/serial/kgdboc.c
> +++ b/drivers/tty/serial/kgdboc.c
> @@ -35,7 +35,6 @@ static struct kparam_string kps = {
>  };
>  
>  static int kgdboc_use_kms;  /* 1 if we use kernel mode switching */
> -static struct tty_driver	*kgdb_tty_driver;
>  static int			kgdb_tty_line;
>  
>  #ifdef CONFIG_KDB_KEYBOARD
> @@ -154,7 +153,7 @@ static int configure_kgdboc(void)
>  	}
>  
>  	kgdboc_io_ops.is_console = 0;
> -	kgdb_tty_driver = NULL;
> +	kgdboc_io_ops.tty_drv = NULL;
>  
>  	kgdboc_use_kms = 0;
>  	if (strncmp(cptr, "kms,", 4) == 0) {
> @@ -178,7 +177,7 @@ static int configure_kgdboc(void)
>  		}
>  	}
>  
> -	kgdb_tty_driver = p;
> +	kgdboc_io_ops.tty_drv = p;
>  	kgdb_tty_line = tty_line;
>  
>  do_register:
> @@ -216,18 +215,18 @@ static int __init init_kgdboc(void)
>  
>  static int kgdboc_get_char(void)
>  {
> -	if (!kgdb_tty_driver)
> +	if (!kgdboc_io_ops.tty_drv)
>  		return -1;
> -	return kgdb_tty_driver->ops->poll_get_char(kgdb_tty_driver,
> -						kgdb_tty_line);
> +	return kgdboc_io_ops.tty_drv->ops->poll_get_char(kgdboc_io_ops.tty_drv,
> +							 kgdb_tty_line);
>  }
>  
>  static void kgdboc_put_char(u8 chr)
>  {
> -	if (!kgdb_tty_driver)
> +	if (!kgdboc_io_ops.tty_drv)
>  		return;
> -	kgdb_tty_driver->ops->poll_put_char(kgdb_tty_driver,
> -					kgdb_tty_line, chr);
> +	kgdboc_io_ops.tty_drv->ops->poll_put_char(kgdboc_io_ops.tty_drv,
> +						  kgdb_tty_line, chr);
>  }
>  
>  static int param_set_kgdboc_var(const char *kmessage,
> diff --git a/include/linux/kgdb.h b/include/linux/kgdb.h
> index b072aeb..05d165d 100644
> --- a/include/linux/kgdb.h
> +++ b/include/linux/kgdb.h
> @@ -275,6 +275,7 @@ struct kgdb_arch {
>   * 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
> + * @tty_drv: Pointer to polling tty driver.
>   */
>  struct kgdb_io {
>  	const char		*name;
> @@ -285,6 +286,7 @@ struct kgdb_io {
>  	void			(*pre_exception) (void);
>  	void			(*post_exception) (void);
>  	int			is_console;
> +	struct tty_driver	*tty_drv;

Should this be a struct tty_driver or a struct console?

In other words if the lifetime the console structure is the same as the
tty_driver then isn't it better to capture the console instead
(easier to compare and works with non-tty devices such as the
USB debug mode).


>  };
>  
>  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 f848482..c2efa52 100644
> --- a/kernel/debug/kdb/kdb_io.c
> +++ b/kernel/debug/kdb/kdb_io.c
> @@ -24,6 +24,7 @@
>  #include <linux/kgdb.h>
>  #include <linux/kdb.h>
>  #include <linux/kallsyms.h>
> +#include <linux/tty_driver.h>
>  #include "kdb_private.h"
>  
>  #define CMD_BUFLEN 256
> @@ -542,13 +543,18 @@ static int kdb_search_string(char *searched, char *searchfor)
>  	return 0;
>  }
>  
> -static void kdb_io_write(char *cp, int len, void (*io_put_char)(u8 ch))
> +static void kdb_io_write(char *cp, int len, void (*io_put_char)(u8),
> +			 struct tty_driver *p, int line,
> +			 void (*poll_put_char)(struct tty_driver *, int, char))

Judging from your reply to comment 1 I guess this is already on the list
to eliminate ;-).


Daniel.


>  {
>  	if (len <= 0)
>  		return;
>  
>  	while (len--) {
> -		io_put_char(*cp);
> +		if (io_put_char)
> +			io_put_char(*cp);
> +		if (poll_put_char)
> +			poll_put_char(p, line, *cp);
>  		cp++;
>  	}
>  }
> @@ -561,22 +567,34 @@ static void kdb_msg_write(char *msg, int msg_len)
>  		return;
>  
>  	if (dbg_io_ops && !dbg_io_ops->is_console)
> -		kdb_io_write(msg, msg_len, dbg_io_ops->write_char);
> +		kdb_io_write(msg, msg_len, dbg_io_ops->write_char,
> +			     NULL, 0, NULL);
>  
>  	for_each_console(c) {
> +		int line;
> +		struct tty_driver *p;
> +
>  		if (!(c->flags & CON_ENABLED))
>  			continue;
> -		/*
> -		 * While rounding up CPUs via NMIs, its possible that
> -		 * a rounded up CPU maybe holding a console port lock
> -		 * leading to kgdb master CPU stuck in a deadlock during
> -		 * invocation of console write operations. So in order
> -		 * to avoid such a deadlock, enable oops_in_progress
> -		 * prior to invocation of console handlers.
> -		 */
> -		++oops_in_progress;
> -		c->write(c, msg, msg_len);
> -		--oops_in_progress;
> +
> +		p = c->device ? c->device(c, &line) : NULL;
> +		if (p && dbg_io_ops && p == dbg_io_ops->tty_drv && p->ops &&
> +		    p->ops->poll_put_char) {
> +			kdb_io_write(msg, msg_len, NULL, p, line,
> +				     p->ops->poll_put_char);
> +		} else {
> +			/*
> +			 * While rounding up CPUs via NMIs, its possible that
> +			 * a rounded up CPU maybe holding a console port lock
> +			 * leading to kgdb master CPU stuck in a deadlock during
> +			 * invocation of console write operations. So in order
> +			 * to avoid such a deadlock, enable oops_in_progress
> +			 * prior to invocation of console handlers.
> +			 */
> +			++oops_in_progress;
> +			c->write(c, msg, msg_len);
> +			--oops_in_progress;
> +		}
>  		touch_nmi_watchdog();
>  	}
>  }
> -- 
> 2.7.4
> 

Re: [PATCH v3 3/4] kdb: Make kdb_printf robust to run in NMI context

[Daniel Thompson]

On Wed, May 27, 2020 at 11:55:58AM +0530, Sumit Garg wrote:
> While rounding up CPUs via NMIs, its possible that a rounded up CPU

This problem does not just impact NMI roundup (breakpoints, including
implicit breakpoint-on-oops can have the same effect).


> maybe holding a console port lock leading to kgdb master CPU stuck in
> a deadlock during invocation of console write operations. So in order
> to avoid such a deadlock, enable oops_in_progress prior to invocation
> of console handlers.
> 
> Suggested-by: Petr Mladek <pmladek@suse.com>
> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> ---
>  kernel/debug/kdb/kdb_io.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
> index 349dfcc..f848482 100644
> --- a/kernel/debug/kdb/kdb_io.c
> +++ b/kernel/debug/kdb/kdb_io.c
> @@ -566,7 +566,17 @@ static void kdb_msg_write(char *msg, int msg_len)
>  	for_each_console(c) {
>  		if (!(c->flags & CON_ENABLED))
>  			continue;
> +		/*
> +		 * While rounding up CPUs via NMIs, its possible that

Ditto.

> +		 * a rounded up CPU maybe holding a console port lock
> +		 * leading to kgdb master CPU stuck in a deadlock during
> +		 * invocation of console write operations. So in order
> +		 * to avoid such a deadlock, enable oops_in_progress
> +		 * prior to invocation of console handlers.

Actually looking at this comment as a whole I think it spends to many
words on what and not enough on why (e.g. what the tradeoffs are and
why we are not using bust_spinlocks() which would be a more obvious
approach).

  Set oops_in_progress to encourage the console drivers to disregard
  their internal spin locks: in the current calling context
  the risk of deadlock is a bigger problem than risks due to
  re-entering the console driver. We operate directly on
  oops_in_progress rather than using bust_spinlocks() because
  the calls bust_spinlocks() makes on exit are not appropriate
  for this calling context.


Daniel.


> +		 */
> +		++oops_in_progress;
>  		c->write(c, msg, msg_len);
> +		--oops_in_progress;
>  		touch_nmi_watchdog();
>  	}
>  }
> -- 
> 2.7.4
> 

Re: [PATCH v3 2/4] kdb: Check status of console prior to invoking handlers

[Daniel Thompson]

On Wed, May 27, 2020 at 11:55:57AM +0530, Sumit Garg wrote:
> Check if a console is enabled prior to invoking corresponding write
> handler.
> 
> Suggested-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>

Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>


> ---
>  kernel/debug/kdb/kdb_io.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
> index f6b4d47..349dfcc 100644
> --- a/kernel/debug/kdb/kdb_io.c
> +++ b/kernel/debug/kdb/kdb_io.c
> @@ -564,6 +564,8 @@ static void kdb_msg_write(char *msg, int msg_len)
>  		kdb_io_write(msg, msg_len, dbg_io_ops->write_char);
>  
>  	for_each_console(c) {
> +		if (!(c->flags & CON_ENABLED))
> +			continue;
>  		c->write(c, msg, msg_len);
>  		touch_nmi_watchdog();
>  	}
> -- 
> 2.7.4
> 

Re: [PATCH v3 4/4] kdb: Switch kdb_msg_write() to use safer polling I/O

[Sumit Garg]

On Wed, 27 May 2020 at 19:01, Daniel Thompson
<daniel.thompson@linaro.org> wrote:
>
> On Wed, May 27, 2020 at 11:55:59AM +0530, Sumit Garg wrote:
> > In kgdb NMI context, calling console handlers isn't safe due to locks
> > used in those handlers which could 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.
>
> Not sure I would have predicted all those changes to kgdboc.c based on
> this patch description. I assume this is to help identify which console
> matches our dbg_io_ops but it would be good to spell this out.
>

Okay, will add the corresponding description.

>
> > Suggested-by: Daniel Thompson <daniel.thompson@linaro.org>
> > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> > ---
> >  drivers/tty/serial/kgdboc.c | 17 ++++++++---------
> >  include/linux/kgdb.h        |  2 ++
> >  kernel/debug/kdb/kdb_io.c   | 46 +++++++++++++++++++++++++++++++--------------
> >  3 files changed, 42 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c
> > index c9f94fa..6199fe1 100644
> > --- a/drivers/tty/serial/kgdboc.c
> > +++ b/drivers/tty/serial/kgdboc.c
> > @@ -35,7 +35,6 @@ static struct kparam_string kps = {
> >  };
> >
> >  static int kgdboc_use_kms;  /* 1 if we use kernel mode switching */
> > -static struct tty_driver     *kgdb_tty_driver;
> >  static int                   kgdb_tty_line;
> >
> >  #ifdef CONFIG_KDB_KEYBOARD
> > @@ -154,7 +153,7 @@ static int configure_kgdboc(void)
> >       }
> >
> >       kgdboc_io_ops.is_console = 0;
> > -     kgdb_tty_driver = NULL;
> > +     kgdboc_io_ops.tty_drv = NULL;
> >
> >       kgdboc_use_kms = 0;
> >       if (strncmp(cptr, "kms,", 4) == 0) {
> > @@ -178,7 +177,7 @@ static int configure_kgdboc(void)
> >               }
> >       }
> >
> > -     kgdb_tty_driver = p;
> > +     kgdboc_io_ops.tty_drv = p;
> >       kgdb_tty_line = tty_line;
> >
> >  do_register:
> > @@ -216,18 +215,18 @@ static int __init init_kgdboc(void)
> >
> >  static int kgdboc_get_char(void)
> >  {
> > -     if (!kgdb_tty_driver)
> > +     if (!kgdboc_io_ops.tty_drv)
> >               return -1;
> > -     return kgdb_tty_driver->ops->poll_get_char(kgdb_tty_driver,
> > -                                             kgdb_tty_line);
> > +     return kgdboc_io_ops.tty_drv->ops->poll_get_char(kgdboc_io_ops.tty_drv,
> > +                                                      kgdb_tty_line);
> >  }
> >
> >  static void kgdboc_put_char(u8 chr)
> >  {
> > -     if (!kgdb_tty_driver)
> > +     if (!kgdboc_io_ops.tty_drv)
> >               return;
> > -     kgdb_tty_driver->ops->poll_put_char(kgdb_tty_driver,
> > -                                     kgdb_tty_line, chr);
> > +     kgdboc_io_ops.tty_drv->ops->poll_put_char(kgdboc_io_ops.tty_drv,
> > +                                               kgdb_tty_line, chr);
> >  }
> >
> >  static int param_set_kgdboc_var(const char *kmessage,
> > diff --git a/include/linux/kgdb.h b/include/linux/kgdb.h
> > index b072aeb..05d165d 100644
> > --- a/include/linux/kgdb.h
> > +++ b/include/linux/kgdb.h
> > @@ -275,6 +275,7 @@ struct kgdb_arch {
> >   * 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
> > + * @tty_drv: Pointer to polling tty driver.
> >   */
> >  struct kgdb_io {
> >       const char              *name;
> > @@ -285,6 +286,7 @@ struct kgdb_io {
> >       void                    (*pre_exception) (void);
> >       void                    (*post_exception) (void);
> >       int                     is_console;
> > +     struct tty_driver       *tty_drv;
>
> Should this be a struct tty_driver or a struct console?
>
> In other words if the lifetime the console structure is the same as the
> tty_driver then isn't it better to capture the console instead
> (easier to compare and works with non-tty devices such as the
> USB debug mode).
>

IIUC, you mean to say we can easily replace "is_console" with "struct
console". This sounds feasible and should be a straightforward
comparison in order to prefer "dbg_io_ops" over console handlers. So I
will switch to use "struct console" instead.

>
> >  };
> >
> >  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 f848482..c2efa52 100644
> > --- a/kernel/debug/kdb/kdb_io.c
> > +++ b/kernel/debug/kdb/kdb_io.c
> > @@ -24,6 +24,7 @@
> >  #include <linux/kgdb.h>
> >  #include <linux/kdb.h>
> >  #include <linux/kallsyms.h>
> > +#include <linux/tty_driver.h>
> >  #include "kdb_private.h"
> >
> >  #define CMD_BUFLEN 256
> > @@ -542,13 +543,18 @@ static int kdb_search_string(char *searched, char *searchfor)
> >       return 0;
> >  }
> >
> > -static void kdb_io_write(char *cp, int len, void (*io_put_char)(u8 ch))
> > +static void kdb_io_write(char *cp, int len, void (*io_put_char)(u8),
> > +                      struct tty_driver *p, int line,
> > +                      void (*poll_put_char)(struct tty_driver *, int, char))
>
> Judging from your reply to comment 1 I guess this is already on the list
> to eliminate ;-).
>

Yeah.

-Sumit

>
> Daniel.
>
>
> >  {
> >       if (len <= 0)
> >               return;
> >
> >       while (len--) {
> > -             io_put_char(*cp);
> > +             if (io_put_char)
> > +                     io_put_char(*cp);
> > +             if (poll_put_char)
> > +                     poll_put_char(p, line, *cp);
> >               cp++;
> >       }
> >  }
> > @@ -561,22 +567,34 @@ static void kdb_msg_write(char *msg, int msg_len)
> >               return;
> >
> >       if (dbg_io_ops && !dbg_io_ops->is_console)
> > -             kdb_io_write(msg, msg_len, dbg_io_ops->write_char);
> > +             kdb_io_write(msg, msg_len, dbg_io_ops->write_char,
> > +                          NULL, 0, NULL);
> >
> >       for_each_console(c) {
> > +             int line;
> > +             struct tty_driver *p;
> > +
> >               if (!(c->flags & CON_ENABLED))
> >                       continue;
> > -             /*
> > -              * While rounding up CPUs via NMIs, its possible that
> > -              * a rounded up CPU maybe holding a console port lock
> > -              * leading to kgdb master CPU stuck in a deadlock during
> > -              * invocation of console write operations. So in order
> > -              * to avoid such a deadlock, enable oops_in_progress
> > -              * prior to invocation of console handlers.
> > -              */
> > -             ++oops_in_progress;
> > -             c->write(c, msg, msg_len);
> > -             --oops_in_progress;
> > +
> > +             p = c->device ? c->device(c, &line) : NULL;
> > +             if (p && dbg_io_ops && p == dbg_io_ops->tty_drv && p->ops &&
> > +                 p->ops->poll_put_char) {
> > +                     kdb_io_write(msg, msg_len, NULL, p, line,
> > +                                  p->ops->poll_put_char);
> > +             } else {
> > +                     /*
> > +                      * While rounding up CPUs via NMIs, its possible that
> > +                      * a rounded up CPU maybe holding a console port lock
> > +                      * leading to kgdb master CPU stuck in a deadlock during
> > +                      * invocation of console write operations. So in order
> > +                      * to avoid such a deadlock, enable oops_in_progress
> > +                      * prior to invocation of console handlers.
> > +                      */
> > +                     ++oops_in_progress;
> > +                     c->write(c, msg, msg_len);
> > +                     --oops_in_progress;
> > +             }
> >               touch_nmi_watchdog();
> >       }
> >  }
> > --
> > 2.7.4
> >

Re: [PATCH v3 3/4] kdb: Make kdb_printf robust to run in NMI context

[Sumit Garg]

On Wed, 27 May 2020 at 19:56, Daniel Thompson
<daniel.thompson@linaro.org> wrote:
>
> On Wed, May 27, 2020 at 11:55:58AM +0530, Sumit Garg wrote:
> > While rounding up CPUs via NMIs, its possible that a rounded up CPU
>
> This problem does not just impact NMI roundup (breakpoints,

I guess here via breakpoints you meant if we add a compiled breakpoint
or runtime breakpoint in console handler code while its holding the
spin lock could lead to a deadlock, correct?

> including
> implicit breakpoint-on-oops can have the same effect).
>

Isn't the breakpoint-on-oops case already handled via bust_spinlocks()
usage in panic handler here [1]?

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/panic.c#n207

>
> > maybe holding a console port lock leading to kgdb master CPU stuck in
> > a deadlock during invocation of console write operations. So in order
> > to avoid such a deadlock, enable oops_in_progress prior to invocation
> > of console handlers.
> >
> > Suggested-by: Petr Mladek <pmladek@suse.com>
> > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> > ---
> >  kernel/debug/kdb/kdb_io.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
> > index 349dfcc..f848482 100644
> > --- a/kernel/debug/kdb/kdb_io.c
> > +++ b/kernel/debug/kdb/kdb_io.c
> > @@ -566,7 +566,17 @@ static void kdb_msg_write(char *msg, int msg_len)
> >       for_each_console(c) {
> >               if (!(c->flags & CON_ENABLED))
> >                       continue;
> > +             /*
> > +              * While rounding up CPUs via NMIs, its possible that
>
> Ditto.
>
> > +              * a rounded up CPU maybe holding a console port lock
> > +              * leading to kgdb master CPU stuck in a deadlock during
> > +              * invocation of console write operations. So in order
> > +              * to avoid such a deadlock, enable oops_in_progress
> > +              * prior to invocation of console handlers.
>
> Actually looking at this comment as a whole I think it spends to many
> words on what and not enough on why (e.g. what the tradeoffs are and
> why we are not using bust_spinlocks() which would be a more obvious
> approach).
>
>   Set oops_in_progress to encourage the console drivers to disregard
>   their internal spin locks: in the current calling context
>   the risk of deadlock is a bigger problem than risks due to
>   re-entering the console driver. We operate directly on
>   oops_in_progress rather than using bust_spinlocks() because
>   the calls bust_spinlocks() makes on exit are not appropriate
>   for this calling context.

Sounds reasonable, will use it instead.

-Sumit

>
>
> Daniel.
>
>
> > +              */
> > +             ++oops_in_progress;
> >               c->write(c, msg, msg_len);
> > +             --oops_in_progress;
> >               touch_nmi_watchdog();
> >       }
> >  }
> > --
> > 2.7.4
> >

Re: [PATCH v3 4/4] kdb: Switch kdb_msg_write() to use safer polling I/O

[Daniel Thompson]

On Thu, May 28, 2020 at 11:48:48AM +0530, Sumit Garg wrote:
> On Wed, 27 May 2020 at 19:01, Daniel Thompson
> <daniel.thompson@linaro.org> wrote:
> >
> > On Wed, May 27, 2020 at 11:55:59AM +0530, Sumit Garg wrote:
> > > In kgdb NMI context, calling console handlers isn't safe due to locks
> > > used in those handlers which could 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.
> >
> > Not sure I would have predicted all those changes to kgdboc.c based on
> > this patch description. I assume this is to help identify which console
> > matches our dbg_io_ops but it would be good to spell this out.
> >
> 
> Okay, will add the corresponding description.
> 
> >
> > > Suggested-by: Daniel Thompson <daniel.thompson@linaro.org>
> > > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> > > ---
> > >  drivers/tty/serial/kgdboc.c | 17 ++++++++---------
> > >  include/linux/kgdb.h        |  2 ++
> > >  kernel/debug/kdb/kdb_io.c   | 46 +++++++++++++++++++++++++++++++--------------
> > >  3 files changed, 42 insertions(+), 23 deletions(-)
> > >
> > > diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c
> > > index c9f94fa..6199fe1 100644
> > > --- a/drivers/tty/serial/kgdboc.c
> > > +++ b/drivers/tty/serial/kgdboc.c
> > > @@ -35,7 +35,6 @@ static struct kparam_string kps = {
> > >  };
> > >
> > >  static int kgdboc_use_kms;  /* 1 if we use kernel mode switching */
> > > -static struct tty_driver     *kgdb_tty_driver;
> > >  static int                   kgdb_tty_line;
> > >
> > >  #ifdef CONFIG_KDB_KEYBOARD
> > > @@ -154,7 +153,7 @@ static int configure_kgdboc(void)
> > >       }
> > >
> > >       kgdboc_io_ops.is_console = 0;
> > > -     kgdb_tty_driver = NULL;
> > > +     kgdboc_io_ops.tty_drv = NULL;
> > >
> > >       kgdboc_use_kms = 0;
> > >       if (strncmp(cptr, "kms,", 4) == 0) {
> > > @@ -178,7 +177,7 @@ static int configure_kgdboc(void)
> > >               }
> > >       }
> > >
> > > -     kgdb_tty_driver = p;
> > > +     kgdboc_io_ops.tty_drv = p;
> > >       kgdb_tty_line = tty_line;
> > >
> > >  do_register:
> > > @@ -216,18 +215,18 @@ static int __init init_kgdboc(void)
> > >
> > >  static int kgdboc_get_char(void)
> > >  {
> > > -     if (!kgdb_tty_driver)
> > > +     if (!kgdboc_io_ops.tty_drv)
> > >               return -1;
> > > -     return kgdb_tty_driver->ops->poll_get_char(kgdb_tty_driver,
> > > -                                             kgdb_tty_line);
> > > +     return kgdboc_io_ops.tty_drv->ops->poll_get_char(kgdboc_io_ops.tty_drv,
> > > +                                                      kgdb_tty_line);
> > >  }
> > >
> > >  static void kgdboc_put_char(u8 chr)
> > >  {
> > > -     if (!kgdb_tty_driver)
> > > +     if (!kgdboc_io_ops.tty_drv)
> > >               return;
> > > -     kgdb_tty_driver->ops->poll_put_char(kgdb_tty_driver,
> > > -                                     kgdb_tty_line, chr);
> > > +     kgdboc_io_ops.tty_drv->ops->poll_put_char(kgdboc_io_ops.tty_drv,
> > > +                                               kgdb_tty_line, chr);
> > >  }
> > >
> > >  static int param_set_kgdboc_var(const char *kmessage,
> > > diff --git a/include/linux/kgdb.h b/include/linux/kgdb.h
> > > index b072aeb..05d165d 100644
> > > --- a/include/linux/kgdb.h
> > > +++ b/include/linux/kgdb.h
> > > @@ -275,6 +275,7 @@ struct kgdb_arch {
> > >   * 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
> > > + * @tty_drv: Pointer to polling tty driver.
> > >   */
> > >  struct kgdb_io {
> > >       const char              *name;
> > > @@ -285,6 +286,7 @@ struct kgdb_io {
> > >       void                    (*pre_exception) (void);
> > >       void                    (*post_exception) (void);
> > >       int                     is_console;
> > > +     struct tty_driver       *tty_drv;
> >
> > Should this be a struct tty_driver or a struct console?
> >
> > In other words if the lifetime the console structure is the same as the
> > tty_driver then isn't it better to capture the console instead
> > (easier to compare and works with non-tty devices such as the
> > USB debug mode).
> >
> 
> IIUC, you mean to say we can easily replace "is_console" with "struct
> console". This sounds feasible and should be a straightforward
> comparison in order to prefer "dbg_io_ops" over console handlers. So I
> will switch to use "struct console" instead.

My comment contains an if ("if the lifetime of the console structure is
the same") so you need to check that it is true before sharing a patch to
make the change.


Daniel.

> 
> >
> > >  };
> > >
> > >  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 f848482..c2efa52 100644
> > > --- a/kernel/debug/kdb/kdb_io.c
> > > +++ b/kernel/debug/kdb/kdb_io.c
> > > @@ -24,6 +24,7 @@
> > >  #include <linux/kgdb.h>
> > >  #include <linux/kdb.h>
> > >  #include <linux/kallsyms.h>
> > > +#include <linux/tty_driver.h>
> > >  #include "kdb_private.h"
> > >
> > >  #define CMD_BUFLEN 256
> > > @@ -542,13 +543,18 @@ static int kdb_search_string(char *searched, char *searchfor)
> > >       return 0;
> > >  }
> > >
> > > -static void kdb_io_write(char *cp, int len, void (*io_put_char)(u8 ch))
> > > +static void kdb_io_write(char *cp, int len, void (*io_put_char)(u8),
> > > +                      struct tty_driver *p, int line,
> > > +                      void (*poll_put_char)(struct tty_driver *, int, char))
> >
> > Judging from your reply to comment 1 I guess this is already on the list
> > to eliminate ;-).
> >
> 
> Yeah.
> 
> -Sumit
> 
> >
> > Daniel.
> >
> >
> > >  {
> > >       if (len <= 0)
> > >               return;
> > >
> > >       while (len--) {
> > > -             io_put_char(*cp);
> > > +             if (io_put_char)
> > > +                     io_put_char(*cp);
> > > +             if (poll_put_char)
> > > +                     poll_put_char(p, line, *cp);
> > >               cp++;
> > >       }
> > >  }
> > > @@ -561,22 +567,34 @@ static void kdb_msg_write(char *msg, int msg_len)
> > >               return;
> > >
> > >       if (dbg_io_ops && !dbg_io_ops->is_console)
> > > -             kdb_io_write(msg, msg_len, dbg_io_ops->write_char);
> > > +             kdb_io_write(msg, msg_len, dbg_io_ops->write_char,
> > > +                          NULL, 0, NULL);
> > >
> > >       for_each_console(c) {
> > > +             int line;
> > > +             struct tty_driver *p;
> > > +
> > >               if (!(c->flags & CON_ENABLED))
> > >                       continue;
> > > -             /*
> > > -              * While rounding up CPUs via NMIs, its possible that
> > > -              * a rounded up CPU maybe holding a console port lock
> > > -              * leading to kgdb master CPU stuck in a deadlock during
> > > -              * invocation of console write operations. So in order
> > > -              * to avoid such a deadlock, enable oops_in_progress
> > > -              * prior to invocation of console handlers.
> > > -              */
> > > -             ++oops_in_progress;
> > > -             c->write(c, msg, msg_len);
> > > -             --oops_in_progress;
> > > +
> > > +             p = c->device ? c->device(c, &line) : NULL;
> > > +             if (p && dbg_io_ops && p == dbg_io_ops->tty_drv && p->ops &&
> > > +                 p->ops->poll_put_char) {
> > > +                     kdb_io_write(msg, msg_len, NULL, p, line,
> > > +                                  p->ops->poll_put_char);
> > > +             } else {
> > > +                     /*
> > > +                      * While rounding up CPUs via NMIs, its possible that
> > > +                      * a rounded up CPU maybe holding a console port lock
> > > +                      * leading to kgdb master CPU stuck in a deadlock during
> > > +                      * invocation of console write operations. So in order
> > > +                      * to avoid such a deadlock, enable oops_in_progress
> > > +                      * prior to invocation of console handlers.
> > > +                      */
> > > +                     ++oops_in_progress;
> > > +                     c->write(c, msg, msg_len);
> > > +                     --oops_in_progress;
> > > +             }
> > >               touch_nmi_watchdog();
> > >       }
> > >  }
> > > --
> > > 2.7.4
> > >

Re: [PATCH v3 4/4] kdb: Switch kdb_msg_write() to use safer polling I/O

[Petr Mladek]

On Thu 2020-05-28 12:26:20, Daniel Thompson wrote:
> On Thu, May 28, 2020 at 11:48:48AM +0530, Sumit Garg wrote:
> > On Wed, 27 May 2020 at 19:01, Daniel Thompson
> > <daniel.thompson@linaro.org> wrote:
> > >
> > > On Wed, May 27, 2020 at 11:55:59AM +0530, Sumit Garg wrote:
> > > > In kgdb NMI context, calling console handlers isn't safe due to locks
> > > > used in those handlers which could 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.
> > >
> > > > diff --git a/include/linux/kgdb.h b/include/linux/kgdb.h
> > > > index b072aeb..05d165d 100644
> > > > --- a/include/linux/kgdb.h
> > > > +++ b/include/linux/kgdb.h
> > > > @@ -275,6 +275,7 @@ struct kgdb_arch {
> > > >   * 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
> > > > + * @tty_drv: Pointer to polling tty driver.
> > > >   */
> > > >  struct kgdb_io {
> > > >       const char              *name;
> > > > @@ -285,6 +286,7 @@ struct kgdb_io {
> > > >       void                    (*pre_exception) (void);
> > > >       void                    (*post_exception) (void);
> > > >       int                     is_console;
> > > > +     struct tty_driver       *tty_drv;
> > >
> > > Should this be a struct tty_driver or a struct console?
> > >
> > > In other words if the lifetime the console structure is the same as the
> > > tty_driver then isn't it better to capture the console instead
> > > (easier to compare and works with non-tty devices such as the
> > > USB debug mode).
> > >
> > 
> > IIUC, you mean to say we can easily replace "is_console" with "struct
> > console". This sounds feasible and should be a straightforward
> > comparison in order to prefer "dbg_io_ops" over console handlers. So I
> > will switch to use "struct console" instead.
> 
> My comment contains an if ("if the lifetime of the console structure is
> the same") so you need to check that it is true before sharing a patch to
> make the change.

Honestly, I am not completely familiar with the console an tty drivers
code.

Anyway, struct console is typically statically defined by the console
driver code. It is not must to have but I am not aware of any
driver where it would be dynamically defined.

On the other hand, struct tty_driver is dynamically allocated
when the driver gets initialized.

So I would say that it is pretty safe to store struct console.
Well, you need to call con->device() to see if the tty_driver
is actually initialized.

Best Regards,
Petr

Re: [PATCH v3 4/4] kdb: Switch kdb_msg_write() to use safer polling I/O

[Sumit Garg]

On Thu, 28 May 2020 at 20:27, Petr Mladek <pmladek@suse.com> wrote:
>
> On Thu 2020-05-28 12:26:20, Daniel Thompson wrote:
> > On Thu, May 28, 2020 at 11:48:48AM +0530, Sumit Garg wrote:
> > > On Wed, 27 May 2020 at 19:01, Daniel Thompson
> > > <daniel.thompson@linaro.org> wrote:
> > > >
> > > > On Wed, May 27, 2020 at 11:55:59AM +0530, Sumit Garg wrote:
> > > > > In kgdb NMI context, calling console handlers isn't safe due to locks
> > > > > used in those handlers which could 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.
> > > >
> > > > > diff --git a/include/linux/kgdb.h b/include/linux/kgdb.h
> > > > > index b072aeb..05d165d 100644
> > > > > --- a/include/linux/kgdb.h
> > > > > +++ b/include/linux/kgdb.h
> > > > > @@ -275,6 +275,7 @@ struct kgdb_arch {
> > > > >   * 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
> > > > > + * @tty_drv: Pointer to polling tty driver.
> > > > >   */
> > > > >  struct kgdb_io {
> > > > >       const char              *name;
> > > > > @@ -285,6 +286,7 @@ struct kgdb_io {
> > > > >       void                    (*pre_exception) (void);
> > > > >       void                    (*post_exception) (void);
> > > > >       int                     is_console;
> > > > > +     struct tty_driver       *tty_drv;
> > > >
> > > > Should this be a struct tty_driver or a struct console?
> > > >
> > > > In other words if the lifetime the console structure is the same as the
> > > > tty_driver then isn't it better to capture the console instead
> > > > (easier to compare and works with non-tty devices such as the
> > > > USB debug mode).
> > > >
> > >
> > > IIUC, you mean to say we can easily replace "is_console" with "struct
> > > console". This sounds feasible and should be a straightforward
> > > comparison in order to prefer "dbg_io_ops" over console handlers. So I
> > > will switch to use "struct console" instead.
> >
> > My comment contains an if ("if the lifetime of the console structure is
> > the same") so you need to check that it is true before sharing a patch to
> > make the change.
>
> Honestly, I am not completely familiar with the console an tty drivers
> code.
>
> Anyway, struct console is typically statically defined by the console
> driver code. It is not must to have but I am not aware of any
> driver where it would be dynamically defined.
>

Yes this is mine understanding as well.

> On the other hand, struct tty_driver is dynamically allocated
> when the driver gets initialized.
>
> So I would say that it is pretty safe to store struct console.

Okay.

> Well, you need to call con->device() to see if the tty_driver
> is actually initialized.

Agree and con->device() is already invoked here [1]. So we only need
to store struct console if con->device() invocation returns success.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/tty/serial/kgdboc.c#n174

-Sumit

>
> Best Regards,
> Petr