From b356e89b89cc950dc142e906c1ad5c2d4eb1fa13 Mon Sep 17 00:00:00 2001 From: Douglas Anderson Date: Sat, 9 Nov 2019 11:16:40 -0800 Subject: [PATCH 1/7] MIPS: kdb: Remove old workaround for backtracing on other CPUs As of commit 2277b492582d ("kdb: Fix stack crawling on 'running' CPUs that aren't the master") we no longer need any special case for doing stack dumps on CPUs that are not the kdb master. Let's remove. Signed-off-by: Douglas Anderson Link: https://lore.kernel.org/r/20191109111623.1.I30a0cac4d9880040c8d41495bd9a567fe3e24989@changeid Signed-off-by: Daniel Thompson --- arch/mips/kernel/traps.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/arch/mips/kernel/traps.c b/arch/mips/kernel/traps.c index 83f2a437d9e2..31968cbd6464 100644 --- a/arch/mips/kernel/traps.c +++ b/arch/mips/kernel/traps.c @@ -210,11 +210,6 @@ void show_stack(struct task_struct *task, unsigned long *sp) regs.regs[29] = task->thread.reg29; regs.regs[31] = 0; regs.cp0_epc = task->thread.reg31; -#ifdef CONFIG_KGDB_KDB - } else if (atomic_read(&kgdb_active) != -1 && - kdb_current_regs) { - memcpy(®s, kdb_current_regs, sizeof(regs)); -#endif /* CONFIG_KGDB_KDB */ } else { prepare_frametrace(®s); } From c67c10a67f6b2edcc7804317947cbfdeab71048c Mon Sep 17 00:00:00 2001 From: Douglas Anderson Date: Sat, 9 Nov 2019 11:16:41 -0800 Subject: [PATCH 2/7] kdb: kdb_current_regs should be private As of the patch ("MIPS: kdb: Remove old workaround for backtracing on other CPUs") there is no reason for kdb_current_regs to be in the public "kdb.h". Let's move it next to kdb_current_task. Signed-off-by: Douglas Anderson Link: https://lore.kernel.org/r/20191109111623.2.Iadbfb484e90b557cc4b5ac9890bfca732cd99d77@changeid Signed-off-by: Daniel Thompson --- include/linux/kdb.h | 2 -- kernel/debug/kdb/kdb_private.h | 1 + 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/include/linux/kdb.h b/include/linux/kdb.h index 68bd88223417..24cd447659e0 100644 --- a/include/linux/kdb.h +++ b/include/linux/kdb.h @@ -183,8 +183,6 @@ int kdb_process_cpu(const struct task_struct *p) return cpu; } -/* kdb access to register set for stack dumping */ -extern struct pt_regs *kdb_current_regs; #ifdef CONFIG_KALLSYMS extern const char *kdb_walk_kallsyms(loff_t *pos); #else /* ! CONFIG_KALLSYMS */ diff --git a/kernel/debug/kdb/kdb_private.h b/kernel/debug/kdb/kdb_private.h index 55d052061ef9..e829b22f3946 100644 --- a/kernel/debug/kdb/kdb_private.h +++ b/kernel/debug/kdb/kdb_private.h @@ -242,6 +242,7 @@ extern void debug_kusage(void); extern void kdb_set_current_task(struct task_struct *); extern struct task_struct *kdb_current_task; +extern struct pt_regs *kdb_current_regs; #ifdef CONFIG_KDB_KEYBOARD extern void kdb_kbd_cleanup_state(void); From a8649fb0a8c1de9c842c9c492f189cabbc468272 Mon Sep 17 00:00:00 2001 From: Douglas Anderson Date: Sat, 9 Nov 2019 11:16:42 -0800 Subject: [PATCH 3/7] kdb: kdb_current_task shouldn't be exported The kdb_current_task variable has been declared in "kernel/debug/kdb/kdb_private.h" since 2010 when kdb was added to the mainline kernel. This is not a public header. There should be no reason that kdb_current_task should be exported and there are no in-kernel users that need it. Remove the export. Signed-off-by: Douglas Anderson Link: https://lore.kernel.org/r/20191109111623.3.I14b22b5eb15ca8f3812ab33e96621231304dc1f7@changeid Signed-off-by: Daniel Thompson --- kernel/debug/kdb/kdb_main.c | 1 - 1 file changed, 1 deletion(-) diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c index 4567fe998c30..4d44b3746836 100644 --- a/kernel/debug/kdb/kdb_main.c +++ b/kernel/debug/kdb/kdb_main.c @@ -73,7 +73,6 @@ int kdb_nextline = 1; int kdb_state; /* General KDB state */ struct task_struct *kdb_current_task; -EXPORT_SYMBOL(kdb_current_task); struct pt_regs *kdb_current_regs; const char *kdb_diemsg; From 9441d5f6b77770ee388884f04b14a99b028a15e6 Mon Sep 17 00:00:00 2001 From: Douglas Anderson Date: Sat, 9 Nov 2019 11:16:43 -0800 Subject: [PATCH 4/7] kdb: Gid rid of implicit setting of the current task / regs Some (but not all?) of the kdb backtrace paths would cause the kdb_current_task and kdb_current_regs to remain changed. As discussed in a review of a previous patch [1], this doesn't seem intuitive, so let's fix that. ...but, it turns out that there's actually no longer any reason to set the current task / current regs while backtracing anymore anyway. As of commit 2277b492582d ("kdb: Fix stack crawling on 'running' CPUs that aren't the master") if we're backtracing on a task running on a CPU we ask that CPU to do the backtrace itself. Linux can do that without anything fancy. If we're doing backtrace on a sleeping task we can also do that fine without updating globals. So this patch mostly just turns into deleting a bunch of code. [1] https://lore.kernel.org/r/20191010150735.dhrj3pbjgmjrdpwr@holly.lan Signed-off-by: Douglas Anderson Link: https://lore.kernel.org/r/20191109111624.4.Ibc3d982bbeb9e46872d43973ba808cd4c79537c7@changeid Signed-off-by: Daniel Thompson --- kernel/debug/kdb/kdb_bt.c | 8 +------- kernel/debug/kdb/kdb_main.c | 2 +- kernel/debug/kdb/kdb_private.h | 1 - 3 files changed, 2 insertions(+), 9 deletions(-) diff --git a/kernel/debug/kdb/kdb_bt.c b/kernel/debug/kdb/kdb_bt.c index 4af48ac53625..3de0cc780c16 100644 --- a/kernel/debug/kdb/kdb_bt.c +++ b/kernel/debug/kdb/kdb_bt.c @@ -119,7 +119,6 @@ kdb_bt_cpu(unsigned long cpu) return; } - kdb_set_current_task(kdb_tsk); kdb_bt1(kdb_tsk, ~0UL, false); } @@ -166,10 +165,8 @@ kdb_bt(int argc, const char **argv) if (diag) return diag; p = find_task_by_pid_ns(pid, &init_pid_ns); - if (p) { - kdb_set_current_task(p); + if (p) return kdb_bt1(p, ~0UL, false); - } kdb_printf("No process with pid == %ld found\n", pid); return 0; } else if (strcmp(argv[0], "btt") == 0) { @@ -178,11 +175,9 @@ kdb_bt(int argc, const char **argv) diag = kdbgetularg((char *)argv[1], &addr); if (diag) return diag; - kdb_set_current_task((struct task_struct *)addr); return kdb_bt1((struct task_struct *)addr, ~0UL, false); } else if (strcmp(argv[0], "btc") == 0) { unsigned long cpu = ~0; - struct task_struct *save_current_task = kdb_current_task; if (argc > 1) return KDB_ARGCOUNT; if (argc == 1) { @@ -204,7 +199,6 @@ kdb_bt(int argc, const char **argv) kdb_bt_cpu(cpu); touch_nmi_watchdog(); } - kdb_set_current_task(save_current_task); } return 0; } else { diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c index 4d44b3746836..ba12e9f4661e 100644 --- a/kernel/debug/kdb/kdb_main.c +++ b/kernel/debug/kdb/kdb_main.c @@ -1138,7 +1138,7 @@ static void kdb_dumpregs(struct pt_regs *regs) console_loglevel = old_lvl; } -void kdb_set_current_task(struct task_struct *p) +static void kdb_set_current_task(struct task_struct *p) { kdb_current_task = p; diff --git a/kernel/debug/kdb/kdb_private.h b/kernel/debug/kdb/kdb_private.h index e829b22f3946..2e296e4a234c 100644 --- a/kernel/debug/kdb/kdb_private.h +++ b/kernel/debug/kdb/kdb_private.h @@ -240,7 +240,6 @@ extern void *debug_kmalloc(size_t size, gfp_t flags); extern void debug_kfree(void *); extern void debug_kusage(void); -extern void kdb_set_current_task(struct task_struct *); extern struct task_struct *kdb_current_task; extern struct pt_regs *kdb_current_regs; From bbfceba15f8d1260c328a254efc2b3f2deae4904 Mon Sep 17 00:00:00 2001 From: Douglas Anderson Date: Sat, 9 Nov 2019 11:16:44 -0800 Subject: [PATCH 5/7] kdb: Get rid of confusing diag msg from "rd" if current task has no regs If you switch to a sleeping task with the "pid" command and then type "rd", kdb tells you this: No current kdb registers. You may need to select another task diag: -17: Invalid register name The first message makes sense, but not the second. Fix it by just returning 0 after commands accessing the current registers finish if we've already printed the "No current kdb registers" error. While fixing kdb_rd(), change the function to use "if" rather than "ifdef". It cleans the function up a bit and any modern compiler will have no trouble handling still producing good code. Signed-off-by: Douglas Anderson Link: https://lore.kernel.org/r/20191109111624.5.I121f4c6f0c19266200bf6ef003de78841e5bfc3d@changeid Signed-off-by: Daniel Thompson --- kernel/debug/kdb/kdb_main.c | 28 +++++++++++++--------------- 1 file changed, 13 insertions(+), 15 deletions(-) diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c index ba12e9f4661e..b22292b649c4 100644 --- a/kernel/debug/kdb/kdb_main.c +++ b/kernel/debug/kdb/kdb_main.c @@ -543,9 +543,8 @@ int kdbgetaddrarg(int argc, const char **argv, int *nextarg, if (diag) return diag; } else if (symname[0] == '%') { - diag = kdb_check_regs(); - if (diag) - return diag; + if (kdb_check_regs()) + return 0; /* Implement register values with % at a later time as it is * arch optional. */ @@ -1836,8 +1835,7 @@ static int kdb_go(int argc, const char **argv) */ static int kdb_rd(int argc, const char **argv) { - int len = kdb_check_regs(); -#if DBG_MAX_REG_NUM > 0 + int len = 0; int i; char *rname; int rsize; @@ -1846,8 +1844,14 @@ static int kdb_rd(int argc, const char **argv) u16 reg16; u8 reg8; - if (len) - return len; + if (kdb_check_regs()) + return 0; + + /* Fallback to Linux showregs() if we don't have DBG_MAX_REG_NUM */ + if (DBG_MAX_REG_NUM <= 0) { + kdb_dumpregs(kdb_current_regs); + return 0; + } for (i = 0; i < DBG_MAX_REG_NUM; i++) { rsize = dbg_reg_def[i].size * 2; @@ -1889,12 +1893,7 @@ static int kdb_rd(int argc, const char **argv) } } kdb_printf("\n"); -#else - if (len) - return len; - kdb_dumpregs(kdb_current_regs); -#endif return 0; } @@ -1928,9 +1927,8 @@ static int kdb_rm(int argc, const char **argv) if (diag) return diag; - diag = kdb_check_regs(); - if (diag) - return diag; + if (kdb_check_regs()) + return 0; diag = KDB_BADREG; for (i = 0; i < DBG_MAX_REG_NUM; i++) { From a4f8a7fb1963bc02fbd40a0a28e128bb56d2fcc9 Mon Sep 17 00:00:00 2001 From: Colin Ian King Date: Thu, 28 Nov 2019 13:07:53 +0000 Subject: [PATCH 6/7] kdb: remove redundant assignment to pointer bp The point bp is assigned a value that is never read, it is being re-assigned later to bp = &kdb_breakpoints[lowbp] in a for-loop. Remove the redundant assignment. Addresses-Coverity ("Unused value") Signed-off-by: Colin Ian King Link: https://lore.kernel.org/r/20191128130753.181246-1-colin.king@canonical.com Signed-off-by: Daniel Thompson --- kernel/debug/kdb/kdb_bp.c | 1 - 1 file changed, 1 deletion(-) diff --git a/kernel/debug/kdb/kdb_bp.c b/kernel/debug/kdb/kdb_bp.c index 62c301ad0773..d7ebb2c79cb8 100644 --- a/kernel/debug/kdb/kdb_bp.c +++ b/kernel/debug/kdb/kdb_bp.c @@ -412,7 +412,6 @@ static int kdb_bc(int argc, const char **argv) * assume that the breakpoint number is desired. */ if (addr < KDB_MAXBPT) { - bp = &kdb_breakpoints[addr]; lowbp = highbp = addr; highbp++; } else { From dc2c733e65848b1df8d55c83eea79fc4a868c800 Mon Sep 17 00:00:00 2001 From: Andy Shevchenko Date: Fri, 24 Jan 2020 18:14:40 +0200 Subject: [PATCH 7/7] kdb: Use for_each_console() helper Replace open coded single-linked list iteration loop with for_each_console() helper in use. Signed-off-by: Andy Shevchenko Signed-off-by: Daniel Thompson --- kernel/debug/kdb/kdb_io.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c index 8bcdded5d61f..924bc9298a42 100644 --- a/kernel/debug/kdb/kdb_io.c +++ b/kernel/debug/kdb/kdb_io.c @@ -553,7 +553,7 @@ 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 = console_drivers; + struct console *c; unsigned long uninitialized_var(flags); /* Serialize kdb_printf if multiple cpus try to write at once. @@ -698,10 +698,9 @@ kdb_printit: cp2++; } } - while (c) { + for_each_console(c) { c->write(c, cp, retlen - (cp - kdb_buffer)); touch_nmi_watchdog(); - c = c->next; } } if (logging) { @@ -752,7 +751,6 @@ kdb_printit: moreprompt = "more> "; kdb_input_flush(); - c = console_drivers; if (dbg_io_ops && !dbg_io_ops->is_console) { len = strlen(moreprompt); @@ -762,10 +760,9 @@ kdb_printit: cp++; } } - while (c) { + for_each_console(c) { c->write(c, moreprompt, strlen(moreprompt)); touch_nmi_watchdog(); - c = c->next; } if (logging)