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 v5:
- Rebased on top of tags/kgdb-5.8-rc1.
- Get rid of kdb_io_write().
- Fixed issue reported by kbuild test robot.
- Remove is_console from kgdboc_earlycon_io_ops as well.
- Misc. comments.
- Added Doug's review tag.
Changes in v4:
- Use dbg_io_ops->write_char() directly instead of passing it as a
function pointer.
- Use "struct console" rather than "struct tty_driver" for comparison.
- Make commit descriptions and comments more informative.
- Add review tag for patch #2.
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() console handling more robust
kdb: Switch to use safer dbg_io_ops over console APIs
drivers/tty/serial/kgdb_nmi.c | 2 +-
drivers/tty/serial/kgdboc.c | 6 ++--
drivers/usb/early/ehci-dbgp.c | 3 +-
include/linux/kgdb.h | 5 ++-
kernel/debug/kdb/kdb_io.c | 72 ++++++++++++++++++++++++++-----------------
5 files changed, 51 insertions(+), 37 deletions(-)
--
2.7.4
Re-factor kdb_printf() message write code in order to avoid duplication
of code and thereby increase readability.
Signed-off-by: Sumit Garg <[email protected]>
Reviewed-by: Douglas Anderson <[email protected]>
---
kernel/debug/kdb/kdb_io.c | 57 +++++++++++++++++++++++------------------------
1 file changed, 28 insertions(+), 29 deletions(-)
diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
index 924bc92..2d42a02 100644
--- a/kernel/debug/kdb/kdb_io.c
+++ b/kernel/debug/kdb/kdb_io.c
@@ -542,6 +542,29 @@ static int kdb_search_string(char *searched, char *searchfor)
return 0;
}
+static void kdb_msg_write(const char *msg, int msg_len)
+{
+ struct console *c;
+
+ if (msg_len == 0)
+ return;
+
+ if (dbg_io_ops && !dbg_io_ops->is_console) {
+ const char *cp = msg;
+ int len = msg_len;
+
+ while (len--) {
+ dbg_io_ops->write_char(*cp);
+ cp++;
+ }
+ }
+
+ 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 +576,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 +709,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 +762,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
Check if a console is enabled prior to invoking corresponding write
handler.
Suggested-by: Sergey Senozhatsky <[email protected]>
Signed-off-by: Sumit Garg <[email protected]>
Reviewed-by: Daniel Thompson <[email protected]>
Reviewed-by: Douglas Anderson <[email protected]>
---
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 2d42a02..58b7d25 100644
--- a/kernel/debug/kdb/kdb_io.c
+++ b/kernel/debug/kdb/kdb_io.c
@@ -560,6 +560,8 @@ static void kdb_msg_write(const char *msg, int msg_len)
}
for_each_console(c) {
+ if (!(c->flags & CON_ENABLED))
+ continue;
c->write(c, msg, msg_len);
touch_nmi_watchdog();
}
--
2.7.4
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 <[email protected]>
Signed-off-by: Sumit Garg <[email protected]>
Reviewed-by: Douglas Anderson <[email protected]>
---
drivers/tty/serial/kgdb_nmi.c | 2 +-
drivers/tty/serial/kgdboc.c | 6 +++---
drivers/usb/early/ehci-dbgp.c | 3 ++-
include/linux/kgdb.h | 5 ++---
kernel/debug/kdb/kdb_io.c | 4 +++-
5 files changed, 11 insertions(+), 9 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..6e182aa 100644
--- a/drivers/tty/serial/kgdboc.c
+++ b/drivers/tty/serial/kgdboc.c
@@ -178,7 +178,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 +198,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;
}
}
@@ -511,7 +511,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)
@@ -558,6 +557,7 @@ static int __init kgdboc_earlycon_init(char *opt)
}
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;
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
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. A similar
deadlock could also be possible while using synchronous breakpoints.
So in order to avoid such a deadlock, 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.
Suggested-by: Petr Mladek <[email protected]>
Signed-off-by: Sumit Garg <[email protected]>
Reviewed-by: Douglas Anderson <[email protected]>
---
kernel/debug/kdb/kdb_io.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
index 58b7d25..0e4f2ed 100644
--- a/kernel/debug/kdb/kdb_io.c
+++ b/kernel/debug/kdb/kdb_io.c
@@ -562,7 +562,18 @@ static void kdb_msg_write(const char *msg, int msg_len)
for_each_console(c) {
if (!(c->flags & CON_ENABLED))
continue;
+ /*
+ * 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.
+ */
+ ++oops_in_progress;
c->write(c, msg, msg_len);
+ --oops_in_progress;
touch_nmi_watchdog();
}
}
--
2.7.4
On Wed 2020-06-03 12:52:12, 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 <[email protected]>
> Reviewed-by: Douglas Anderson <[email protected]>
Revieved-by: Petr Mladek <[email protected]>
Best Regards,
Petr
On Wed 2020-06-03 12:52:14, Sumit Garg wrote:
> 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. A similar
> deadlock could also be possible while using synchronous breakpoints.
>
> So in order to avoid such a deadlock, 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.
>
> Suggested-by: Petr Mladek <[email protected]>
I think that this was actually suggested by Sergey.
> Signed-off-by: Sumit Garg <[email protected]>
> Reviewed-by: Douglas Anderson <[email protected]>
Otherwise, it looks good. With updated suggested by:
Reviewed-by: Petr Mladek <[email protected]>
Best Regards,
Petr
On Wed 2020-06-03 12:52:13, Sumit Garg wrote:
> Check if a console is enabled prior to invoking corresponding write
> handler.
>
> Suggested-by: Sergey Senozhatsky <[email protected]>
> Signed-off-by: Sumit Garg <[email protected]>
> Reviewed-by: Daniel Thompson <[email protected]>
> Reviewed-by: Douglas Anderson <[email protected]>
Reviewed-by: Petr Mladek <[email protected]>
Best Regards,
Petr
On Wed 2020-06-03 12:52:15, 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.
>
> diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c
> index 4139698..6e182aa 100644
> --- a/drivers/tty/serial/kgdboc.c
> +++ b/drivers/tty/serial/kgdboc.c
> @@ -558,6 +557,7 @@ static int __init kgdboc_earlycon_init(char *opt)
> }
>
> 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;
Should we clear kgdboc_earlycon_io_ops.cons here when
kgdb_register_io_module() failed?
> 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;
Nit: I would call it "con". The trailing 's' makes me feel that that the
variable points to an array or list of consoles.
> };
>
Otherwise, it looks pretty straightforward.
Best Regards,
Petr
On Wed, Jun 03, 2020 at 10:25:04AM +0200, Petr Mladek wrote:
> On Wed 2020-06-03 12:52:15, 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.
> >
> > diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c
> > index 4139698..6e182aa 100644
> > --- a/drivers/tty/serial/kgdboc.c
> > +++ b/drivers/tty/serial/kgdboc.c
> > @@ -558,6 +557,7 @@ static int __init kgdboc_earlycon_init(char *opt)
> > }
> >
> > 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;
>
> Should we clear kgdboc_earlycon_io_ops.cons here when
> kgdb_register_io_module() failed?
>
> > 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;
>
> Nit: I would call it "con". The trailing 's' makes me feel that that the
> variable points to an array or list of consoles.
How strongly do you feel about it?
I'd probably agree with you except that the uart subsystem, which is by
far the most prolific supplier of consoles for kgdb to use, calls
pointers to single consoles "cons" so I'd prefer to be aligned on
terminology.
Daniel.
On Wed, 3 Jun 2020 at 13:55, Petr Mladek <[email protected]> wrote:
>
> On Wed 2020-06-03 12:52:15, 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.
> >
> > diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c
> > index 4139698..6e182aa 100644
> > --- a/drivers/tty/serial/kgdboc.c
> > +++ b/drivers/tty/serial/kgdboc.c
> > @@ -558,6 +557,7 @@ static int __init kgdboc_earlycon_init(char *opt)
> > }
> >
> > 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;
>
> Should we clear kgdboc_earlycon_io_ops.cons here when
> kgdb_register_io_module() failed?
>
AFAIK, kgdboc_earlycon_io_ops won't be used at any later stage in case
registration fails. So IMO, it would be a redundant assignment unless
I missed something.
-Sumit
> > 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;
>
> Nit: I would call it "con". The trailing 's' makes me feel that that the
> variable points to an array or list of consoles.
>
> > };
> >
>
> Otherwise, it looks pretty straightforward.
>
> Best Regards,
> Petr
On Wed, Jun 03, 2020 at 03:02:02PM +0530, Sumit Garg wrote:
> On Wed, 3 Jun 2020 at 13:55, Petr Mladek <[email protected]> wrote:
> >
> > On Wed 2020-06-03 12:52:15, 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.
> > >
> > > diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c
> > > index 4139698..6e182aa 100644
> > > --- a/drivers/tty/serial/kgdboc.c
> > > +++ b/drivers/tty/serial/kgdboc.c
> > > @@ -558,6 +557,7 @@ static int __init kgdboc_earlycon_init(char *opt)
> > > }
> > >
> > > 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;
> >
> > Should we clear kgdboc_earlycon_io_ops.cons here when
> > kgdb_register_io_module() failed?
> >
>
> AFAIK, kgdboc_earlycon_io_ops won't be used at any later stage in case
> registration fails. So IMO, it would be a redundant assignment unless
> I missed something.
Or, putting it another way, earlycon is a redundant (albeit better
maintained) copy of kgdboc_earlycon_io_ops.cons. So I think the best
thing to do is entirely replace earlycon with
kgdboc_earlycon_io_ops.cons and then properly set it to NULL!
Daniel.
On Wed 2020-06-03 10:18:30, Daniel Thompson wrote:
> On Wed, Jun 03, 2020 at 10:25:04AM +0200, Petr Mladek wrote:
> > On Wed 2020-06-03 12:52:15, 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.
> > >
> > > diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c
> > > index 4139698..6e182aa 100644
> > > --- a/drivers/tty/serial/kgdboc.c
> > > +++ b/drivers/tty/serial/kgdboc.c
> > > @@ -558,6 +557,7 @@ static int __init kgdboc_earlycon_init(char *opt)
> > > }
> > >
> > > 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;
> >
> > Should we clear kgdboc_earlycon_io_ops.cons here when
> > kgdb_register_io_module() failed?
> >
> > > 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;
> >
> > Nit: I would call it "con". The trailing 's' makes me feel that that the
> > variable points to an array or list of consoles.
>
> How strongly do you feel about it?
I do not have strong opinion about it.
> I'd probably agree with you except that the uart subsystem, which is by
> far the most prolific supplier of consoles for kgdb to use, calls
> pointers to single consoles "cons" so I'd prefer to be aligned on
> terminology.
You made me curious ;-) I tried to find what names are used for
struct console variables.
$linux> git grep "struct console \*c" | sed -e "s/^.*\(struct console[[:blank:]]*\*c[a-z]*\).*$/\1/" | sort | uniq -c
26 struct console *c
181 struct console *co
68 struct console *con
7 struct console *cons
28 struct console *console
1 struct console *cs
and from tty subdirectory:
linux/drivers/tty> git grep "struct console \*c" | sed -e "s/^.*\(struct console[[:blank:]]*\*c[a-z]*\).*$/\1/" | sort | uniq -c
8 struct console *c
136 struct console *co
35 struct console *con
4 struct console *cons
10 struct console *console
1 struct console *cs
Anyway, feel free to use whatever you want. I prefer "con".
But "cons" still looks better than "co" ;-)
Best Regards,
Petr
On Wed, 3 Jun 2020 at 17:12, Daniel Thompson <[email protected]> wrote:
>
> On Wed, Jun 03, 2020 at 03:02:02PM +0530, Sumit Garg wrote:
> > On Wed, 3 Jun 2020 at 13:55, Petr Mladek <[email protected]> wrote:
> > >
> > > On Wed 2020-06-03 12:52:15, 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.
> > > >
> > > > diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c
> > > > index 4139698..6e182aa 100644
> > > > --- a/drivers/tty/serial/kgdboc.c
> > > > +++ b/drivers/tty/serial/kgdboc.c
> > > > @@ -558,6 +557,7 @@ static int __init kgdboc_earlycon_init(char *opt)
> > > > }
> > > >
> > > > 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;
> > >
> > > Should we clear kgdboc_earlycon_io_ops.cons here when
> > > kgdb_register_io_module() failed?
> > >
> >
> > AFAIK, kgdboc_earlycon_io_ops won't be used at any later stage in case
> > registration fails. So IMO, it would be a redundant assignment unless
> > I missed something.
>
> Or, putting it another way, earlycon is a redundant (albeit better
> maintained) copy of kgdboc_earlycon_io_ops.cons. So I think the best
> thing to do is entirely replace earlycon with
> kgdboc_earlycon_io_ops.cons and then properly set it to NULL!
>
Sounds reasonable, will replace earlycon instead.
-Sumit
>
> Daniel.