linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v5] kdb: Simplify kdb commands registration
@ 2021-02-24  7:08 Sumit Garg
  2021-03-01 18:40 ` Doug Anderson
  2021-03-22 16:15 ` Daniel Thompson
  0 siblings, 2 replies; 4+ messages in thread
From: Sumit Garg @ 2021-02-24  7:08 UTC (permalink / raw)
  To: kgdb-bugreport
  Cc: daniel.thompson, jason.wessel, dianders, linux-kernel, Sumit Garg

Simplify kdb commands registration via using linked list instead of
static array for commands storage.

Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
---

Changes in v5:
- Introduce new method: kdb_register_table() to register static kdb
  main and breakpoint command tables instead of using statically
  allocated commands.

Changes in v4:
- Fix kdb commands memory allocation issue prior to slab being available
  with an array of statically allocated commands. Now it works fine with
  kgdbwait.
- Fix a misc checkpatch warning.
- I have dropped Doug's review tag as I think this version includes a
  major fix that should be reviewed again.

Changes in v3:
- Remove redundant "if" check.
- Pick up review tag from Doug.

Changes in v2:
- Remove redundant NULL check for "cmd_name".
- Incorporate misc. comment.

 kernel/debug/kdb/kdb_bp.c      |  81 ++++--
 kernel/debug/kdb/kdb_main.c    | 472 ++++++++++++++++++++-------------
 kernel/debug/kdb/kdb_private.h |   3 +
 3 files changed, 343 insertions(+), 213 deletions(-)

diff --git a/kernel/debug/kdb/kdb_bp.c b/kernel/debug/kdb/kdb_bp.c
index ec4940146612..c15a1c6abfd6 100644
--- a/kernel/debug/kdb/kdb_bp.c
+++ b/kernel/debug/kdb/kdb_bp.c
@@ -522,6 +522,60 @@ static int kdb_ss(int argc, const char **argv)
 	return KDB_CMD_SS;
 }
 
+static kdbtab_t bptab[] = {
+	{	.cmd_name = "bp",
+		.cmd_func = kdb_bp,
+		.cmd_usage = "[<vaddr>]",
+		.cmd_help = "Set/Display breakpoints",
+		.cmd_minlen = 0,
+		.cmd_flags = KDB_ENABLE_FLOW_CTRL | KDB_REPEAT_NO_ARGS,
+	},
+	{	.cmd_name = "bl",
+		.cmd_func = kdb_bp,
+		.cmd_usage = "[<vaddr>]",
+		.cmd_help = "Display breakpoints",
+		.cmd_minlen = 0,
+		.cmd_flags = KDB_ENABLE_FLOW_CTRL | KDB_REPEAT_NO_ARGS,
+	},
+	{	.cmd_name = "bc",
+		.cmd_func = kdb_bc,
+		.cmd_usage = "<bpnum>",
+		.cmd_help = "Clear Breakpoint",
+		.cmd_minlen = 0,
+		.cmd_flags = KDB_ENABLE_FLOW_CTRL,
+	},
+	{	.cmd_name = "be",
+		.cmd_func = kdb_bc,
+		.cmd_usage = "<bpnum>",
+		.cmd_help = "Enable Breakpoint",
+		.cmd_minlen = 0,
+		.cmd_flags = KDB_ENABLE_FLOW_CTRL,
+	},
+	{	.cmd_name = "bd",
+		.cmd_func = kdb_bc,
+		.cmd_usage = "<bpnum>",
+		.cmd_help = "Disable Breakpoint",
+		.cmd_minlen = 0,
+		.cmd_flags = KDB_ENABLE_FLOW_CTRL,
+	},
+	{	.cmd_name = "ss",
+		.cmd_func = kdb_ss,
+		.cmd_usage = "",
+		.cmd_help = "Single Step",
+		.cmd_minlen = 1,
+		.cmd_flags = KDB_ENABLE_FLOW_CTRL | KDB_REPEAT_NO_ARGS,
+	},
+};
+
+static kdbtab_t bphcmd = {
+	.cmd_name = "bph",
+	.cmd_func = kdb_bp,
+	.cmd_usage = "[<vaddr>]",
+	.cmd_help = "[datar [length]|dataw [length]]   Set hw brk",
+	.cmd_minlen = 0,
+	.cmd_flags = KDB_ENABLE_FLOW_CTRL | KDB_REPEAT_NO_ARGS,
+};
+
 /* Initialize the breakpoint table and register	breakpoint commands. */
 
 void __init kdb_initbptab(void)
@@ -537,30 +591,7 @@ void __init kdb_initbptab(void)
 	for (i = 0, bp = kdb_breakpoints; i < KDB_MAXBPT; i++, bp++)
 		bp->bp_free = 1;
 
-	kdb_register_flags("bp", kdb_bp, "[<vaddr>]",
-		"Set/Display breakpoints", 0,
-		KDB_ENABLE_FLOW_CTRL | KDB_REPEAT_NO_ARGS);
-	kdb_register_flags("bl", kdb_bp, "[<vaddr>]",
-		"Display breakpoints", 0,
-		KDB_ENABLE_FLOW_CTRL | KDB_REPEAT_NO_ARGS);
+	kdb_register_table(bptab, ARRAY_SIZE(bptab));
 	if (arch_kgdb_ops.flags & KGDB_HW_BREAKPOINT)
-		kdb_register_flags("bph", kdb_bp, "[<vaddr>]",
-		"[datar [length]|dataw [length]]   Set hw brk", 0,
-		KDB_ENABLE_FLOW_CTRL | KDB_REPEAT_NO_ARGS);
-	kdb_register_flags("bc", kdb_bc, "<bpnum>",
-		"Clear Breakpoint", 0,
-		KDB_ENABLE_FLOW_CTRL);
-	kdb_register_flags("be", kdb_bc, "<bpnum>",
-		"Enable Breakpoint", 0,
-		KDB_ENABLE_FLOW_CTRL);
-	kdb_register_flags("bd", kdb_bc, "<bpnum>",
-		"Disable Breakpoint", 0,
-		KDB_ENABLE_FLOW_CTRL);
-
-	kdb_register_flags("ss", kdb_ss, "",
-		"Single Step", 1,
-		KDB_ENABLE_FLOW_CTRL | KDB_REPEAT_NO_ARGS);
-	/*
-	 * Architecture dependent initialization.
-	 */
+		kdb_register_table(&bphcmd, 1);
 }
diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
index 930ac1b25ec7..1e0c2c37df94 100644
--- a/kernel/debug/kdb/kdb_main.c
+++ b/kernel/debug/kdb/kdb_main.c
@@ -33,6 +33,7 @@
 #include <linux/kallsyms.h>
 #include <linux/kgdb.h>
 #include <linux/kdb.h>
+#include <linux/list.h>
 #include <linux/notifier.h>
 #include <linux/interrupt.h>
 #include <linux/delay.h>
@@ -84,15 +85,8 @@ static unsigned int kdb_continue_catastrophic =
 static unsigned int kdb_continue_catastrophic;
 #endif
 
-/* kdb_commands describes the available commands. */
-static kdbtab_t *kdb_commands;
-#define KDB_BASE_CMD_MAX 50
-static int kdb_max_commands = KDB_BASE_CMD_MAX;
-static kdbtab_t kdb_base_commands[KDB_BASE_CMD_MAX];
-#define for_each_kdbcmd(cmd, num)					\
-	for ((cmd) = kdb_base_commands, (num) = 0;			\
-	     num < kdb_max_commands;					\
-	     num++, num == KDB_BASE_CMD_MAX ? cmd = kdb_commands : cmd++)
+/* kdb_cmds_head describes the available commands. */
+static LIST_HEAD(kdb_cmds_head);
 
 typedef struct _kdbmsg {
 	int	km_diag;	/* kdb diagnostic */
@@ -921,7 +915,7 @@ int kdb_parse(const char *cmdstr)
 	char *cp;
 	char *cpp, quoted;
 	kdbtab_t *tp;
-	int i, escaped, ignore_errors = 0, check_grep = 0;
+	int escaped, ignore_errors = 0, check_grep = 0;
 
 	/*
 	 * First tokenize the command string.
@@ -1011,25 +1005,17 @@ int kdb_parse(const char *cmdstr)
 		++argv[0];
 	}
 
-	for_each_kdbcmd(tp, i) {
-		if (tp->cmd_name) {
-			/*
-			 * If this command is allowed to be abbreviated,
-			 * check to see if this is it.
-			 */
-
-			if (tp->cmd_minlen
-			 && (strlen(argv[0]) <= tp->cmd_minlen)) {
-				if (strncmp(argv[0],
-					    tp->cmd_name,
-					    tp->cmd_minlen) == 0) {
-					break;
-				}
-			}
+	list_for_each_entry(tp, &kdb_cmds_head, list_node) {
+		/*
+		 * If this command is allowed to be abbreviated,
+		 * check to see if this is it.
+		 */
+		if (tp->cmd_minlen && (strlen(argv[0]) <= tp->cmd_minlen) &&
+		    (strncmp(argv[0], tp->cmd_name, tp->cmd_minlen) == 0))
+			break;
 
-			if (strcmp(argv[0], tp->cmd_name) == 0)
-				break;
-		}
+		if (strcmp(argv[0], tp->cmd_name) == 0)
+			break;
 	}
 
 	/*
@@ -1037,19 +1023,15 @@ int kdb_parse(const char *cmdstr)
 	 * few characters of this match any of the known commands.
 	 * e.g., md1c20 should match md.
 	 */
-	if (i == kdb_max_commands) {
-		for_each_kdbcmd(tp, i) {
-			if (tp->cmd_name) {
-				if (strncmp(argv[0],
-					    tp->cmd_name,
-					    strlen(tp->cmd_name)) == 0) {
-					break;
-				}
-			}
+	if (list_entry_is_head(tp, &kdb_cmds_head, list_node)) {
+		list_for_each_entry(tp, &kdb_cmds_head, list_node) {
+			if (strncmp(argv[0], tp->cmd_name,
+				    strlen(tp->cmd_name)) == 0)
+				break;
 		}
 	}
 
-	if (i < kdb_max_commands) {
+	if (!list_entry_is_head(tp, &kdb_cmds_head, list_node)) {
 		int result;
 
 		if (!kdb_check_flags(tp->cmd_flags, kdb_cmd_enabled, argc <= 1))
@@ -2428,17 +2410,14 @@ static int kdb_kgdb(int argc, const char **argv)
 static int kdb_help(int argc, const char **argv)
 {
 	kdbtab_t *kt;
-	int i;
 
 	kdb_printf("%-15.15s %-20.20s %s\n", "Command", "Usage", "Description");
 	kdb_printf("-----------------------------"
 		   "-----------------------------\n");
-	for_each_kdbcmd(kt, i) {
+	list_for_each_entry(kt, &kdb_cmds_head, list_node) {
 		char *space = "";
 		if (KDB_FLAG(CMD_INTERRUPT))
 			return 0;
-		if (!kt->cmd_name)
-			continue;
 		if (!kdb_check_flags(kt->cmd_flags, kdb_cmd_enabled, true))
 			continue;
 		if (strlen(kt->cmd_usage) > 20)
@@ -2659,7 +2638,6 @@ static int kdb_grep_help(int argc, const char **argv)
  * Returns:
  *	zero for success, one if a duplicate command.
  */
-#define kdb_command_extend 50	/* arbitrary */
 int kdb_register_flags(char *cmd,
 		       kdb_func_t func,
 		       char *usage,
@@ -2667,49 +2645,20 @@ int kdb_register_flags(char *cmd,
 		       short minlen,
 		       kdb_cmdflags_t flags)
 {
-	int i;
 	kdbtab_t *kp;
 
-	/*
-	 *  Brute force method to determine duplicates
-	 */
-	for_each_kdbcmd(kp, i) {
-		if (kp->cmd_name && (strcmp(kp->cmd_name, cmd) == 0)) {
+	list_for_each_entry(kp, &kdb_cmds_head, list_node) {
+		if (strcmp(kp->cmd_name, cmd) == 0) {
 			kdb_printf("Duplicate kdb command registered: "
 				"%s, func %px help %s\n", cmd, func, help);
 			return 1;
 		}
 	}
 
-	/*
-	 * Insert command into first available location in table
-	 */
-	for_each_kdbcmd(kp, i) {
-		if (kp->cmd_name == NULL)
-			break;
-	}
-
-	if (i >= kdb_max_commands) {
-		kdbtab_t *new = kmalloc_array(kdb_max_commands -
-						KDB_BASE_CMD_MAX +
-						kdb_command_extend,
-					      sizeof(*new),
-					      GFP_KDB);
-		if (!new) {
-			kdb_printf("Could not allocate new kdb_command "
-				   "table\n");
-			return 1;
-		}
-		if (kdb_commands) {
-			memcpy(new, kdb_commands,
-			  (kdb_max_commands - KDB_BASE_CMD_MAX) * sizeof(*new));
-			kfree(kdb_commands);
-		}
-		memset(new + kdb_max_commands - KDB_BASE_CMD_MAX, 0,
-		       kdb_command_extend * sizeof(*new));
-		kdb_commands = new;
-		kp = kdb_commands + kdb_max_commands - KDB_BASE_CMD_MAX;
-		kdb_max_commands += kdb_command_extend;
+	kp = kmalloc(sizeof(*kp), GFP_KDB);
+	if (!kp) {
+		kdb_printf("Could not allocate new kdb_command table\n");
+		return 1;
 	}
 
 	kp->cmd_name   = cmd;
@@ -2718,11 +2667,27 @@ int kdb_register_flags(char *cmd,
 	kp->cmd_help   = help;
 	kp->cmd_minlen = minlen;
 	kp->cmd_flags  = flags;
+	kp->is_dynamic = true;
+
+	list_add_tail(&kp->list_node, &kdb_cmds_head);
 
 	return 0;
 }
 EXPORT_SYMBOL_GPL(kdb_register_flags);
 
+/*
+ * kdb_register_table() - This function is used to register a kdb command
+ *                        table.
+ * @kp: pointer to kdb command table
+ * @len: length of kdb command table
+ */
+void kdb_register_table(kdbtab_t *kp, size_t len)
+{
+	while (len--) {
+		list_add_tail(&kp->list_node, &kdb_cmds_head);
+		kp++;
+	}
+}
 
 /*
  * kdb_register - Compatibility register function for commands that do
@@ -2757,15 +2722,16 @@ EXPORT_SYMBOL_GPL(kdb_register);
  */
 int kdb_unregister(char *cmd)
 {
-	int i;
 	kdbtab_t *kp;
 
 	/*
 	 *  find the command.
 	 */
-	for_each_kdbcmd(kp, i) {
-		if (kp->cmd_name && (strcmp(kp->cmd_name, cmd) == 0)) {
-			kp->cmd_name = NULL;
+	list_for_each_entry(kp, &kdb_cmds_head, list_node) {
+		if (strcmp(kp->cmd_name, cmd) == 0) {
+			list_del(&kp->list_node);
+			if (kp->is_dynamic)
+				kfree(kp);
 			return 0;
 		}
 	}
@@ -2775,118 +2741,248 @@ int kdb_unregister(char *cmd)
 }
 EXPORT_SYMBOL_GPL(kdb_unregister);
 
-/* Initialize the kdb command table. */
-static void __init kdb_inittab(void)
-{
-	int i;
-	kdbtab_t *kp;
-
-	for_each_kdbcmd(kp, i)
-		kp->cmd_name = NULL;
-
-	kdb_register_flags("md", kdb_md, "<vaddr>",
-	  "Display Memory Contents, also mdWcN, e.g. md8c1", 1,
-	  KDB_ENABLE_MEM_READ | KDB_REPEAT_NO_ARGS);
-	kdb_register_flags("mdr", kdb_md, "<vaddr> <bytes>",
-	  "Display Raw Memory", 0,
-	  KDB_ENABLE_MEM_READ | KDB_REPEAT_NO_ARGS);
-	kdb_register_flags("mdp", kdb_md, "<paddr> <bytes>",
-	  "Display Physical Memory", 0,
-	  KDB_ENABLE_MEM_READ | KDB_REPEAT_NO_ARGS);
-	kdb_register_flags("mds", kdb_md, "<vaddr>",
-	  "Display Memory Symbolically", 0,
-	  KDB_ENABLE_MEM_READ | KDB_REPEAT_NO_ARGS);
-	kdb_register_flags("mm", kdb_mm, "<vaddr> <contents>",
-	  "Modify Memory Contents", 0,
-	  KDB_ENABLE_MEM_WRITE | KDB_REPEAT_NO_ARGS);
-	kdb_register_flags("go", kdb_go, "[<vaddr>]",
-	  "Continue Execution", 1,
-	  KDB_ENABLE_REG_WRITE | KDB_ENABLE_ALWAYS_SAFE_NO_ARGS);
-	kdb_register_flags("rd", kdb_rd, "",
-	  "Display Registers", 0,
-	  KDB_ENABLE_REG_READ);
-	kdb_register_flags("rm", kdb_rm, "<reg> <contents>",
-	  "Modify Registers", 0,
-	  KDB_ENABLE_REG_WRITE);
-	kdb_register_flags("ef", kdb_ef, "<vaddr>",
-	  "Display exception frame", 0,
-	  KDB_ENABLE_MEM_READ);
-	kdb_register_flags("bt", kdb_bt, "[<vaddr>]",
-	  "Stack traceback", 1,
-	  KDB_ENABLE_MEM_READ | KDB_ENABLE_INSPECT_NO_ARGS);
-	kdb_register_flags("btp", kdb_bt, "<pid>",
-	  "Display stack for process <pid>", 0,
-	  KDB_ENABLE_INSPECT);
-	kdb_register_flags("bta", kdb_bt, "[D|R|S|T|C|Z|E|U|I|M|A]",
-	  "Backtrace all processes matching state flag", 0,
-	  KDB_ENABLE_INSPECT);
-	kdb_register_flags("btc", kdb_bt, "",
-	  "Backtrace current process on each cpu", 0,
-	  KDB_ENABLE_INSPECT);
-	kdb_register_flags("btt", kdb_bt, "<vaddr>",
-	  "Backtrace process given its struct task address", 0,
-	  KDB_ENABLE_MEM_READ | KDB_ENABLE_INSPECT_NO_ARGS);
-	kdb_register_flags("env", kdb_env, "",
-	  "Show environment variables", 0,
-	  KDB_ENABLE_ALWAYS_SAFE);
-	kdb_register_flags("set", kdb_set, "",
-	  "Set environment variables", 0,
-	  KDB_ENABLE_ALWAYS_SAFE);
-	kdb_register_flags("help", kdb_help, "",
-	  "Display Help Message", 1,
-	  KDB_ENABLE_ALWAYS_SAFE);
-	kdb_register_flags("?", kdb_help, "",
-	  "Display Help Message", 0,
-	  KDB_ENABLE_ALWAYS_SAFE);
-	kdb_register_flags("cpu", kdb_cpu, "<cpunum>",
-	  "Switch to new cpu", 0,
-	  KDB_ENABLE_ALWAYS_SAFE_NO_ARGS);
-	kdb_register_flags("kgdb", kdb_kgdb, "",
-	  "Enter kgdb mode", 0, 0);
-	kdb_register_flags("ps", kdb_ps, "[<flags>|A]",
-	  "Display active task list", 0,
-	  KDB_ENABLE_INSPECT);
-	kdb_register_flags("pid", kdb_pid, "<pidnum>",
-	  "Switch to another task", 0,
-	  KDB_ENABLE_INSPECT);
-	kdb_register_flags("reboot", kdb_reboot, "",
-	  "Reboot the machine immediately", 0,
-	  KDB_ENABLE_REBOOT);
+static kdbtab_t maintab[] = {
+	{	.cmd_name = "md",
+		.cmd_func = kdb_md,
+		.cmd_usage = "<vaddr>",
+		.cmd_help = "Display Memory Contents, also mdWcN, e.g. md8c1",
+		.cmd_minlen = 1,
+		.cmd_flags = KDB_ENABLE_MEM_READ | KDB_REPEAT_NO_ARGS,
+	},
+	{	.cmd_name = "mdr",
+		.cmd_func = kdb_md,
+		.cmd_usage = "<vaddr> <bytes>",
+		.cmd_help = "Display Raw Memory",
+		.cmd_minlen = 0,
+		.cmd_flags = KDB_ENABLE_MEM_READ | KDB_REPEAT_NO_ARGS,
+	},
+	{	.cmd_name = "mdp",
+		.cmd_func = kdb_md,
+		.cmd_usage = "<paddr> <bytes>",
+		.cmd_help = "Display Physical Memory",
+		.cmd_minlen = 0,
+		.cmd_flags = KDB_ENABLE_MEM_READ | KDB_REPEAT_NO_ARGS,
+	},
+	{	.cmd_name = "mds",
+		.cmd_func = kdb_md,
+		.cmd_usage = "<vaddr>",
+		.cmd_help = "Display Memory Symbolically",
+		.cmd_minlen = 0,
+		.cmd_flags = KDB_ENABLE_MEM_READ | KDB_REPEAT_NO_ARGS,
+	},
+	{	.cmd_name = "mm",
+		.cmd_func = kdb_mm,
+		.cmd_usage = "<vaddr> <contents>",
+		.cmd_help = "Modify Memory Contents",
+		.cmd_minlen = 0,
+		.cmd_flags = KDB_ENABLE_MEM_WRITE | KDB_REPEAT_NO_ARGS,
+	},
+	{	.cmd_name = "go",
+		.cmd_func = kdb_go,
+		.cmd_usage = "[<vaddr>]",
+		.cmd_help = "Continue Execution",
+		.cmd_minlen = 1,
+		.cmd_flags = KDB_ENABLE_REG_WRITE |
+			     KDB_ENABLE_ALWAYS_SAFE_NO_ARGS,
+	},
+	{	.cmd_name = "rd",
+		.cmd_func = kdb_rd,
+		.cmd_usage = "",
+		.cmd_help = "Display Registers",
+		.cmd_minlen = 0,
+		.cmd_flags = KDB_ENABLE_REG_READ,
+	},
+	{	.cmd_name = "rm",
+		.cmd_func = kdb_rm,
+		.cmd_usage = "<reg> <contents>",
+		.cmd_help = "Modify Registers",
+		.cmd_minlen = 0,
+		.cmd_flags = KDB_ENABLE_REG_WRITE,
+	},
+	{	.cmd_name = "ef",
+		.cmd_func = kdb_ef,
+		.cmd_usage = "<vaddr>",
+		.cmd_help = "Display exception frame",
+		.cmd_minlen = 0,
+		.cmd_flags = KDB_ENABLE_MEM_READ,
+	},
+	{	.cmd_name = "bt",
+		.cmd_func = kdb_bt,
+		.cmd_usage = "[<vaddr>]",
+		.cmd_help = "Stack traceback",
+		.cmd_minlen = 1,
+		.cmd_flags = KDB_ENABLE_MEM_READ | KDB_ENABLE_INSPECT_NO_ARGS,
+	},
+	{	.cmd_name = "btp",
+		.cmd_func = kdb_bt,
+		.cmd_usage = "<pid>",
+		.cmd_help = "Display stack for process <pid>",
+		.cmd_minlen = 0,
+		.cmd_flags = KDB_ENABLE_INSPECT,
+	},
+	{	.cmd_name = "bta",
+		.cmd_func = kdb_bt,
+		.cmd_usage = "[D|R|S|T|C|Z|E|U|I|M|A]",
+		.cmd_help = "Backtrace all processes matching state flag",
+		.cmd_minlen = 0,
+		.cmd_flags = KDB_ENABLE_INSPECT,
+	},
+	{	.cmd_name = "btc",
+		.cmd_func = kdb_bt,
+		.cmd_usage = "",
+		.cmd_help = "Backtrace current process on each cpu",
+		.cmd_minlen = 0,
+		.cmd_flags = KDB_ENABLE_INSPECT,
+	},
+	{	.cmd_name = "btt",
+		.cmd_func = kdb_bt,
+		.cmd_usage = "<vaddr>",
+		.cmd_help = "Backtrace process given its struct task address",
+		.cmd_minlen = 0,
+		.cmd_flags = KDB_ENABLE_MEM_READ | KDB_ENABLE_INSPECT_NO_ARGS,
+	},
+	{	.cmd_name = "env",
+		.cmd_func = kdb_env,
+		.cmd_usage = "",
+		.cmd_help = "Show environment variables",
+		.cmd_minlen = 0,
+		.cmd_flags = KDB_ENABLE_ALWAYS_SAFE,
+	},
+	{	.cmd_name = "set",
+		.cmd_func = kdb_set,
+		.cmd_usage = "",
+		.cmd_help = "Set environment variables",
+		.cmd_minlen = 0,
+		.cmd_flags = KDB_ENABLE_ALWAYS_SAFE,
+	},
+	{	.cmd_name = "help",
+		.cmd_func = kdb_help,
+		.cmd_usage = "",
+		.cmd_help = "Display Help Message",
+		.cmd_minlen = 1,
+		.cmd_flags = KDB_ENABLE_ALWAYS_SAFE,
+	},
+	{	.cmd_name = "?",
+		.cmd_func = kdb_help,
+		.cmd_usage = "",
+		.cmd_help = "Display Help Message",
+		.cmd_minlen = 0,
+		.cmd_flags = KDB_ENABLE_ALWAYS_SAFE,
+	},
+	{	.cmd_name = "cpu",
+		.cmd_func = kdb_cpu,
+		.cmd_usage = "<cpunum>",
+		.cmd_help = "Switch to new cpu",
+		.cmd_minlen = 0,
+		.cmd_flags = KDB_ENABLE_ALWAYS_SAFE_NO_ARGS,
+	},
+	{	.cmd_name = "kgdb",
+		.cmd_func = kdb_kgdb,
+		.cmd_usage = "",
+		.cmd_help = "Enter kgdb mode",
+		.cmd_minlen = 0,
+		.cmd_flags = 0,
+	},
+	{	.cmd_name = "ps",
+		.cmd_func = kdb_ps,
+		.cmd_usage = "[<flags>|A]",
+		.cmd_help = "Display active task list",
+		.cmd_minlen = 0,
+		.cmd_flags = KDB_ENABLE_INSPECT,
+	},
+	{	.cmd_name = "pid",
+		.cmd_func = kdb_pid,
+		.cmd_usage = "<pidnum>",
+		.cmd_help = "Switch to another task",
+		.cmd_minlen = 0,
+		.cmd_flags = KDB_ENABLE_INSPECT,
+	},
+	{	.cmd_name = "reboot",
+		.cmd_func = kdb_reboot,
+		.cmd_usage = "",
+		.cmd_help = "Reboot the machine immediately",
+		.cmd_minlen = 0,
+		.cmd_flags = KDB_ENABLE_REBOOT,
+	},
 #if defined(CONFIG_MODULES)
-	kdb_register_flags("lsmod", kdb_lsmod, "",
-	  "List loaded kernel modules", 0,
-	  KDB_ENABLE_INSPECT);
+	{	.cmd_name = "lsmod",
+		.cmd_func = kdb_lsmod,
+		.cmd_usage = "",
+		.cmd_help = "List loaded kernel modules",
+		.cmd_minlen = 0,
+		.cmd_flags = KDB_ENABLE_INSPECT,
+	},
 #endif
 #if defined(CONFIG_MAGIC_SYSRQ)
-	kdb_register_flags("sr", kdb_sr, "<key>",
-	  "Magic SysRq key", 0,
-	  KDB_ENABLE_ALWAYS_SAFE);
+	{	.cmd_name = "sr",
+		.cmd_func = kdb_sr,
+		.cmd_usage = "<key>",
+		.cmd_help = "Magic SysRq key",
+		.cmd_minlen = 0,
+		.cmd_flags = KDB_ENABLE_ALWAYS_SAFE,
+	},
 #endif
 #if defined(CONFIG_PRINTK)
-	kdb_register_flags("dmesg", kdb_dmesg, "[lines]",
-	  "Display syslog buffer", 0,
-	  KDB_ENABLE_ALWAYS_SAFE);
+	{	.cmd_name = "dmesg",
+		.cmd_func = kdb_dmesg,
+		.cmd_usage = "[lines]",
+		.cmd_help = "Display syslog buffer",
+		.cmd_minlen = 0,
+		.cmd_flags = KDB_ENABLE_ALWAYS_SAFE,
+	},
 #endif
-	if (arch_kgdb_ops.enable_nmi) {
-		kdb_register_flags("disable_nmi", kdb_disable_nmi, "",
-		  "Disable NMI entry to KDB", 0,
-		  KDB_ENABLE_ALWAYS_SAFE);
-	}
-	kdb_register_flags("defcmd", kdb_defcmd, "name \"usage\" \"help\"",
-	  "Define a set of commands, down to endefcmd", 0,
-	  KDB_ENABLE_ALWAYS_SAFE);
-	kdb_register_flags("kill", kdb_kill, "<-signal> <pid>",
-	  "Send a signal to a process", 0,
-	  KDB_ENABLE_SIGNAL);
-	kdb_register_flags("summary", kdb_summary, "",
-	  "Summarize the system", 4,
-	  KDB_ENABLE_ALWAYS_SAFE);
-	kdb_register_flags("per_cpu", kdb_per_cpu, "<sym> [<bytes>] [<cpu>]",
-	  "Display per_cpu variables", 3,
-	  KDB_ENABLE_MEM_READ);
-	kdb_register_flags("grephelp", kdb_grep_help, "",
-	  "Display help on | grep", 0,
-	  KDB_ENABLE_ALWAYS_SAFE);
+	{	.cmd_name = "defcmd",
+		.cmd_func = kdb_defcmd,
+		.cmd_usage = "name \"usage\" \"help\"",
+		.cmd_help = "Define a set of commands, down to endefcmd",
+		.cmd_minlen = 0,
+		.cmd_flags = KDB_ENABLE_ALWAYS_SAFE,
+	},
+	{	.cmd_name = "kill",
+		.cmd_func = kdb_kill,
+		.cmd_usage = "<-signal> <pid>",
+		.cmd_help = "Send a signal to a process",
+		.cmd_minlen = 0,
+		.cmd_flags = KDB_ENABLE_SIGNAL,
+	},
+	{	.cmd_name = "summary",
+		.cmd_func = kdb_summary,
+		.cmd_usage = "",
+		.cmd_help = "Summarize the system",
+		.cmd_minlen = 4,
+		.cmd_flags = KDB_ENABLE_ALWAYS_SAFE,
+	},
+	{	.cmd_name = "per_cpu",
+		.cmd_func = kdb_per_cpu,
+		.cmd_usage = "<sym> [<bytes>] [<cpu>]",
+		.cmd_help = "Display per_cpu variables",
+		.cmd_minlen = 3,
+		.cmd_flags = KDB_ENABLE_MEM_READ,
+	},
+	{	.cmd_name = "grephelp",
+		.cmd_func = kdb_grep_help,
+		.cmd_usage = "",
+		.cmd_help = "Display help on | grep",
+		.cmd_minlen = 0,
+		.cmd_flags = KDB_ENABLE_ALWAYS_SAFE,
+	},
+};
+
+static kdbtab_t nmicmd = {
+	.cmd_name = "disable_nmi",
+	.cmd_func = kdb_disable_nmi,
+	.cmd_usage = "",
+	.cmd_help = "Disable NMI entry to KDB",
+	.cmd_minlen = 0,
+	.cmd_flags = KDB_ENABLE_ALWAYS_SAFE,
+};
+
+/* Initialize the kdb command table. */
+static void __init kdb_inittab(void)
+{
+	kdb_register_table(maintab, ARRAY_SIZE(maintab));
+	if (arch_kgdb_ops.enable_nmi)
+		kdb_register_table(&nmicmd, 1);
 }
 
 /* Execute any commands defined in kdb_cmds.  */
diff --git a/kernel/debug/kdb/kdb_private.h b/kernel/debug/kdb/kdb_private.h
index a4281fb99299..2d4030fa56cf 100644
--- a/kernel/debug/kdb/kdb_private.h
+++ b/kernel/debug/kdb/kdb_private.h
@@ -174,8 +174,11 @@ typedef struct _kdbtab {
 	short    cmd_minlen;		/* Minimum legal # command
 					 * chars required */
 	kdb_cmdflags_t cmd_flags;	/* Command behaviour flags */
+	struct list_head list_node;	/* Command list */
+	bool    is_dynamic;		/* Command table allocation type */
 } kdbtab_t;
 
+extern void kdb_register_table(kdbtab_t *kp, size_t len);
 extern int kdb_bt(int, const char **);	/* KDB display back trace */
 
 /* KDB breakpoint management functions */
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH v5] kdb: Simplify kdb commands registration
  2021-02-24  7:08 [PATCH v5] kdb: Simplify kdb commands registration Sumit Garg
@ 2021-03-01 18:40 ` Doug Anderson
  2021-03-04 10:58   ` Sumit Garg
  2021-03-22 16:15 ` Daniel Thompson
  1 sibling, 1 reply; 4+ messages in thread
From: Doug Anderson @ 2021-03-01 18:40 UTC (permalink / raw)
  To: Sumit Garg; +Cc: kgdb-bugreport, Daniel Thompson, Jason Wessel, LKML

Hi,

On Tue, Feb 23, 2021 at 11:08 PM Sumit Garg <sumit.garg@linaro.org> wrote:
>
> Simplify kdb commands registration via using linked list instead of
> static array for commands storage.
>
> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> ---
>
> Changes in v5:
> - Introduce new method: kdb_register_table() to register static kdb
>   main and breakpoint command tables instead of using statically
>   allocated commands.
>
> Changes in v4:
> - Fix kdb commands memory allocation issue prior to slab being available
>   with an array of statically allocated commands. Now it works fine with
>   kgdbwait.
> - Fix a misc checkpatch warning.
> - I have dropped Doug's review tag as I think this version includes a
>   major fix that should be reviewed again.
>
> Changes in v3:
> - Remove redundant "if" check.
> - Pick up review tag from Doug.
>
> Changes in v2:
> - Remove redundant NULL check for "cmd_name".
> - Incorporate misc. comment.
>
>  kernel/debug/kdb/kdb_bp.c      |  81 ++++--
>  kernel/debug/kdb/kdb_main.c    | 472 ++++++++++++++++++++-------------
>  kernel/debug/kdb/kdb_private.h |   3 +
>  3 files changed, 343 insertions(+), 213 deletions(-)

This looks good to me, thanks!

Random notes:

* We no longer check for "duplicate" commands for any of these
statically allocated ones, but I guess that's fine.

* Presumably nothing outside of kdb/kgdb itself needs the ability to
allocate commands statically.  The only user I see now is ftrace and
it looks like it runs late enough that it should be fine.

Reviewed-by: Douglas Anderson <dianders@chromium.org>


-Doug

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v5] kdb: Simplify kdb commands registration
  2021-03-01 18:40 ` Doug Anderson
@ 2021-03-04 10:58   ` Sumit Garg
  0 siblings, 0 replies; 4+ messages in thread
From: Sumit Garg @ 2021-03-04 10:58 UTC (permalink / raw)
  To: Doug Anderson; +Cc: kgdb-bugreport, Daniel Thompson, Jason Wessel, LKML

Hi Doug,

On Tue, 2 Mar 2021 at 00:10, Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Tue, Feb 23, 2021 at 11:08 PM Sumit Garg <sumit.garg@linaro.org> wrote:
> >
> > Simplify kdb commands registration via using linked list instead of
> > static array for commands storage.
> >
> > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> > ---
> >
> > Changes in v5:
> > - Introduce new method: kdb_register_table() to register static kdb
> >   main and breakpoint command tables instead of using statically
> >   allocated commands.
> >
> > Changes in v4:
> > - Fix kdb commands memory allocation issue prior to slab being available
> >   with an array of statically allocated commands. Now it works fine with
> >   kgdbwait.
> > - Fix a misc checkpatch warning.
> > - I have dropped Doug's review tag as I think this version includes a
> >   major fix that should be reviewed again.
> >
> > Changes in v3:
> > - Remove redundant "if" check.
> > - Pick up review tag from Doug.
> >
> > Changes in v2:
> > - Remove redundant NULL check for "cmd_name".
> > - Incorporate misc. comment.
> >
> >  kernel/debug/kdb/kdb_bp.c      |  81 ++++--
> >  kernel/debug/kdb/kdb_main.c    | 472 ++++++++++++++++++++-------------
> >  kernel/debug/kdb/kdb_private.h |   3 +
> >  3 files changed, 343 insertions(+), 213 deletions(-)
>
> This looks good to me, thanks!
>
> Random notes:
>
> * We no longer check for "duplicate" commands for any of these
> statically allocated ones, but I guess that's fine.

Yeah, I think that check is redundant for static ones.

>
> * Presumably nothing outside of kdb/kgdb itself needs the ability to
> allocate commands statically.  The only user I see now is ftrace and
> it looks like it runs late enough that it should be fine.

Agree.

>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
>

Thanks,
-Sumit

>
> -Doug

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v5] kdb: Simplify kdb commands registration
  2021-02-24  7:08 [PATCH v5] kdb: Simplify kdb commands registration Sumit Garg
  2021-03-01 18:40 ` Doug Anderson
@ 2021-03-22 16:15 ` Daniel Thompson
  1 sibling, 0 replies; 4+ messages in thread
From: Daniel Thompson @ 2021-03-22 16:15 UTC (permalink / raw)
  To: Sumit Garg; +Cc: kgdb-bugreport, jason.wessel, dianders, linux-kernel

On Wed, Feb 24, 2021 at 12:38:27PM +0530, Sumit Garg wrote:
> Simplify kdb commands registration via using linked list instead of
> static array for commands storage.
> 
> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>

Applied, thanks!


> ---
> 
> Changes in v5:
> - Introduce new method: kdb_register_table() to register static kdb
>   main and breakpoint command tables instead of using statically
>   allocated commands.
> 
> Changes in v4:
> - Fix kdb commands memory allocation issue prior to slab being available
>   with an array of statically allocated commands. Now it works fine with
>   kgdbwait.
> - Fix a misc checkpatch warning.
> - I have dropped Doug's review tag as I think this version includes a
>   major fix that should be reviewed again.
> 
> Changes in v3:
> - Remove redundant "if" check.
> - Pick up review tag from Doug.
> 
> Changes in v2:
> - Remove redundant NULL check for "cmd_name".
> - Incorporate misc. comment.
> 
>  kernel/debug/kdb/kdb_bp.c      |  81 ++++--
>  kernel/debug/kdb/kdb_main.c    | 472 ++++++++++++++++++++-------------
>  kernel/debug/kdb/kdb_private.h |   3 +
>  3 files changed, 343 insertions(+), 213 deletions(-)
> 
> diff --git a/kernel/debug/kdb/kdb_bp.c b/kernel/debug/kdb/kdb_bp.c
> index ec4940146612..c15a1c6abfd6 100644
> --- a/kernel/debug/kdb/kdb_bp.c
> +++ b/kernel/debug/kdb/kdb_bp.c
> @@ -522,6 +522,60 @@ static int kdb_ss(int argc, const char **argv)
>  	return KDB_CMD_SS;
>  }
>  
> +static kdbtab_t bptab[] = {
> +	{	.cmd_name = "bp",
> +		.cmd_func = kdb_bp,
> +		.cmd_usage = "[<vaddr>]",
> +		.cmd_help = "Set/Display breakpoints",
> +		.cmd_minlen = 0,
> +		.cmd_flags = KDB_ENABLE_FLOW_CTRL | KDB_REPEAT_NO_ARGS,
> +	},
> +	{	.cmd_name = "bl",
> +		.cmd_func = kdb_bp,
> +		.cmd_usage = "[<vaddr>]",
> +		.cmd_help = "Display breakpoints",
> +		.cmd_minlen = 0,
> +		.cmd_flags = KDB_ENABLE_FLOW_CTRL | KDB_REPEAT_NO_ARGS,
> +	},
> +	{	.cmd_name = "bc",
> +		.cmd_func = kdb_bc,
> +		.cmd_usage = "<bpnum>",
> +		.cmd_help = "Clear Breakpoint",
> +		.cmd_minlen = 0,
> +		.cmd_flags = KDB_ENABLE_FLOW_CTRL,
> +	},
> +	{	.cmd_name = "be",
> +		.cmd_func = kdb_bc,
> +		.cmd_usage = "<bpnum>",
> +		.cmd_help = "Enable Breakpoint",
> +		.cmd_minlen = 0,
> +		.cmd_flags = KDB_ENABLE_FLOW_CTRL,
> +	},
> +	{	.cmd_name = "bd",
> +		.cmd_func = kdb_bc,
> +		.cmd_usage = "<bpnum>",
> +		.cmd_help = "Disable Breakpoint",
> +		.cmd_minlen = 0,
> +		.cmd_flags = KDB_ENABLE_FLOW_CTRL,
> +	},
> +	{	.cmd_name = "ss",
> +		.cmd_func = kdb_ss,
> +		.cmd_usage = "",
> +		.cmd_help = "Single Step",
> +		.cmd_minlen = 1,
> +		.cmd_flags = KDB_ENABLE_FLOW_CTRL | KDB_REPEAT_NO_ARGS,
> +	},
> +};
> +
> +static kdbtab_t bphcmd = {
> +	.cmd_name = "bph",
> +	.cmd_func = kdb_bp,
> +	.cmd_usage = "[<vaddr>]",
> +	.cmd_help = "[datar [length]|dataw [length]]   Set hw brk",
> +	.cmd_minlen = 0,
> +	.cmd_flags = KDB_ENABLE_FLOW_CTRL | KDB_REPEAT_NO_ARGS,
> +};
> +
>  /* Initialize the breakpoint table and register	breakpoint commands. */
>  
>  void __init kdb_initbptab(void)
> @@ -537,30 +591,7 @@ void __init kdb_initbptab(void)
>  	for (i = 0, bp = kdb_breakpoints; i < KDB_MAXBPT; i++, bp++)
>  		bp->bp_free = 1;
>  
> -	kdb_register_flags("bp", kdb_bp, "[<vaddr>]",
> -		"Set/Display breakpoints", 0,
> -		KDB_ENABLE_FLOW_CTRL | KDB_REPEAT_NO_ARGS);
> -	kdb_register_flags("bl", kdb_bp, "[<vaddr>]",
> -		"Display breakpoints", 0,
> -		KDB_ENABLE_FLOW_CTRL | KDB_REPEAT_NO_ARGS);
> +	kdb_register_table(bptab, ARRAY_SIZE(bptab));
>  	if (arch_kgdb_ops.flags & KGDB_HW_BREAKPOINT)
> -		kdb_register_flags("bph", kdb_bp, "[<vaddr>]",
> -		"[datar [length]|dataw [length]]   Set hw brk", 0,
> -		KDB_ENABLE_FLOW_CTRL | KDB_REPEAT_NO_ARGS);
> -	kdb_register_flags("bc", kdb_bc, "<bpnum>",
> -		"Clear Breakpoint", 0,
> -		KDB_ENABLE_FLOW_CTRL);
> -	kdb_register_flags("be", kdb_bc, "<bpnum>",
> -		"Enable Breakpoint", 0,
> -		KDB_ENABLE_FLOW_CTRL);
> -	kdb_register_flags("bd", kdb_bc, "<bpnum>",
> -		"Disable Breakpoint", 0,
> -		KDB_ENABLE_FLOW_CTRL);
> -
> -	kdb_register_flags("ss", kdb_ss, "",
> -		"Single Step", 1,
> -		KDB_ENABLE_FLOW_CTRL | KDB_REPEAT_NO_ARGS);
> -	/*
> -	 * Architecture dependent initialization.
> -	 */
> +		kdb_register_table(&bphcmd, 1);
>  }
> diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
> index 930ac1b25ec7..1e0c2c37df94 100644
> --- a/kernel/debug/kdb/kdb_main.c
> +++ b/kernel/debug/kdb/kdb_main.c
> @@ -33,6 +33,7 @@
>  #include <linux/kallsyms.h>
>  #include <linux/kgdb.h>
>  #include <linux/kdb.h>
> +#include <linux/list.h>
>  #include <linux/notifier.h>
>  #include <linux/interrupt.h>
>  #include <linux/delay.h>
> @@ -84,15 +85,8 @@ static unsigned int kdb_continue_catastrophic =
>  static unsigned int kdb_continue_catastrophic;
>  #endif
>  
> -/* kdb_commands describes the available commands. */
> -static kdbtab_t *kdb_commands;
> -#define KDB_BASE_CMD_MAX 50
> -static int kdb_max_commands = KDB_BASE_CMD_MAX;
> -static kdbtab_t kdb_base_commands[KDB_BASE_CMD_MAX];
> -#define for_each_kdbcmd(cmd, num)					\
> -	for ((cmd) = kdb_base_commands, (num) = 0;			\
> -	     num < kdb_max_commands;					\
> -	     num++, num == KDB_BASE_CMD_MAX ? cmd = kdb_commands : cmd++)
> +/* kdb_cmds_head describes the available commands. */
> +static LIST_HEAD(kdb_cmds_head);
>  
>  typedef struct _kdbmsg {
>  	int	km_diag;	/* kdb diagnostic */
> @@ -921,7 +915,7 @@ int kdb_parse(const char *cmdstr)
>  	char *cp;
>  	char *cpp, quoted;
>  	kdbtab_t *tp;
> -	int i, escaped, ignore_errors = 0, check_grep = 0;
> +	int escaped, ignore_errors = 0, check_grep = 0;
>  
>  	/*
>  	 * First tokenize the command string.
> @@ -1011,25 +1005,17 @@ int kdb_parse(const char *cmdstr)
>  		++argv[0];
>  	}
>  
> -	for_each_kdbcmd(tp, i) {
> -		if (tp->cmd_name) {
> -			/*
> -			 * If this command is allowed to be abbreviated,
> -			 * check to see if this is it.
> -			 */
> -
> -			if (tp->cmd_minlen
> -			 && (strlen(argv[0]) <= tp->cmd_minlen)) {
> -				if (strncmp(argv[0],
> -					    tp->cmd_name,
> -					    tp->cmd_minlen) == 0) {
> -					break;
> -				}
> -			}
> +	list_for_each_entry(tp, &kdb_cmds_head, list_node) {
> +		/*
> +		 * If this command is allowed to be abbreviated,
> +		 * check to see if this is it.
> +		 */
> +		if (tp->cmd_minlen && (strlen(argv[0]) <= tp->cmd_minlen) &&
> +		    (strncmp(argv[0], tp->cmd_name, tp->cmd_minlen) == 0))
> +			break;
>  
> -			if (strcmp(argv[0], tp->cmd_name) == 0)
> -				break;
> -		}
> +		if (strcmp(argv[0], tp->cmd_name) == 0)
> +			break;
>  	}
>  
>  	/*
> @@ -1037,19 +1023,15 @@ int kdb_parse(const char *cmdstr)
>  	 * few characters of this match any of the known commands.
>  	 * e.g., md1c20 should match md.
>  	 */
> -	if (i == kdb_max_commands) {
> -		for_each_kdbcmd(tp, i) {
> -			if (tp->cmd_name) {
> -				if (strncmp(argv[0],
> -					    tp->cmd_name,
> -					    strlen(tp->cmd_name)) == 0) {
> -					break;
> -				}
> -			}
> +	if (list_entry_is_head(tp, &kdb_cmds_head, list_node)) {
> +		list_for_each_entry(tp, &kdb_cmds_head, list_node) {
> +			if (strncmp(argv[0], tp->cmd_name,
> +				    strlen(tp->cmd_name)) == 0)
> +				break;
>  		}
>  	}
>  
> -	if (i < kdb_max_commands) {
> +	if (!list_entry_is_head(tp, &kdb_cmds_head, list_node)) {
>  		int result;
>  
>  		if (!kdb_check_flags(tp->cmd_flags, kdb_cmd_enabled, argc <= 1))
> @@ -2428,17 +2410,14 @@ static int kdb_kgdb(int argc, const char **argv)
>  static int kdb_help(int argc, const char **argv)
>  {
>  	kdbtab_t *kt;
> -	int i;
>  
>  	kdb_printf("%-15.15s %-20.20s %s\n", "Command", "Usage", "Description");
>  	kdb_printf("-----------------------------"
>  		   "-----------------------------\n");
> -	for_each_kdbcmd(kt, i) {
> +	list_for_each_entry(kt, &kdb_cmds_head, list_node) {
>  		char *space = "";
>  		if (KDB_FLAG(CMD_INTERRUPT))
>  			return 0;
> -		if (!kt->cmd_name)
> -			continue;
>  		if (!kdb_check_flags(kt->cmd_flags, kdb_cmd_enabled, true))
>  			continue;
>  		if (strlen(kt->cmd_usage) > 20)
> @@ -2659,7 +2638,6 @@ static int kdb_grep_help(int argc, const char **argv)
>   * Returns:
>   *	zero for success, one if a duplicate command.
>   */
> -#define kdb_command_extend 50	/* arbitrary */
>  int kdb_register_flags(char *cmd,
>  		       kdb_func_t func,
>  		       char *usage,
> @@ -2667,49 +2645,20 @@ int kdb_register_flags(char *cmd,
>  		       short minlen,
>  		       kdb_cmdflags_t flags)
>  {
> -	int i;
>  	kdbtab_t *kp;
>  
> -	/*
> -	 *  Brute force method to determine duplicates
> -	 */
> -	for_each_kdbcmd(kp, i) {
> -		if (kp->cmd_name && (strcmp(kp->cmd_name, cmd) == 0)) {
> +	list_for_each_entry(kp, &kdb_cmds_head, list_node) {
> +		if (strcmp(kp->cmd_name, cmd) == 0) {
>  			kdb_printf("Duplicate kdb command registered: "
>  				"%s, func %px help %s\n", cmd, func, help);
>  			return 1;
>  		}
>  	}
>  
> -	/*
> -	 * Insert command into first available location in table
> -	 */
> -	for_each_kdbcmd(kp, i) {
> -		if (kp->cmd_name == NULL)
> -			break;
> -	}
> -
> -	if (i >= kdb_max_commands) {
> -		kdbtab_t *new = kmalloc_array(kdb_max_commands -
> -						KDB_BASE_CMD_MAX +
> -						kdb_command_extend,
> -					      sizeof(*new),
> -					      GFP_KDB);
> -		if (!new) {
> -			kdb_printf("Could not allocate new kdb_command "
> -				   "table\n");
> -			return 1;
> -		}
> -		if (kdb_commands) {
> -			memcpy(new, kdb_commands,
> -			  (kdb_max_commands - KDB_BASE_CMD_MAX) * sizeof(*new));
> -			kfree(kdb_commands);
> -		}
> -		memset(new + kdb_max_commands - KDB_BASE_CMD_MAX, 0,
> -		       kdb_command_extend * sizeof(*new));
> -		kdb_commands = new;
> -		kp = kdb_commands + kdb_max_commands - KDB_BASE_CMD_MAX;
> -		kdb_max_commands += kdb_command_extend;
> +	kp = kmalloc(sizeof(*kp), GFP_KDB);
> +	if (!kp) {
> +		kdb_printf("Could not allocate new kdb_command table\n");
> +		return 1;
>  	}
>  
>  	kp->cmd_name   = cmd;
> @@ -2718,11 +2667,27 @@ int kdb_register_flags(char *cmd,
>  	kp->cmd_help   = help;
>  	kp->cmd_minlen = minlen;
>  	kp->cmd_flags  = flags;
> +	kp->is_dynamic = true;
> +
> +	list_add_tail(&kp->list_node, &kdb_cmds_head);
>  
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(kdb_register_flags);
>  
> +/*
> + * kdb_register_table() - This function is used to register a kdb command
> + *                        table.
> + * @kp: pointer to kdb command table
> + * @len: length of kdb command table
> + */
> +void kdb_register_table(kdbtab_t *kp, size_t len)
> +{
> +	while (len--) {
> +		list_add_tail(&kp->list_node, &kdb_cmds_head);
> +		kp++;
> +	}
> +}
>  
>  /*
>   * kdb_register - Compatibility register function for commands that do
> @@ -2757,15 +2722,16 @@ EXPORT_SYMBOL_GPL(kdb_register);
>   */
>  int kdb_unregister(char *cmd)
>  {
> -	int i;
>  	kdbtab_t *kp;
>  
>  	/*
>  	 *  find the command.
>  	 */
> -	for_each_kdbcmd(kp, i) {
> -		if (kp->cmd_name && (strcmp(kp->cmd_name, cmd) == 0)) {
> -			kp->cmd_name = NULL;
> +	list_for_each_entry(kp, &kdb_cmds_head, list_node) {
> +		if (strcmp(kp->cmd_name, cmd) == 0) {
> +			list_del(&kp->list_node);
> +			if (kp->is_dynamic)
> +				kfree(kp);
>  			return 0;
>  		}
>  	}
> @@ -2775,118 +2741,248 @@ int kdb_unregister(char *cmd)
>  }
>  EXPORT_SYMBOL_GPL(kdb_unregister);
>  
> -/* Initialize the kdb command table. */
> -static void __init kdb_inittab(void)
> -{
> -	int i;
> -	kdbtab_t *kp;
> -
> -	for_each_kdbcmd(kp, i)
> -		kp->cmd_name = NULL;
> -
> -	kdb_register_flags("md", kdb_md, "<vaddr>",
> -	  "Display Memory Contents, also mdWcN, e.g. md8c1", 1,
> -	  KDB_ENABLE_MEM_READ | KDB_REPEAT_NO_ARGS);
> -	kdb_register_flags("mdr", kdb_md, "<vaddr> <bytes>",
> -	  "Display Raw Memory", 0,
> -	  KDB_ENABLE_MEM_READ | KDB_REPEAT_NO_ARGS);
> -	kdb_register_flags("mdp", kdb_md, "<paddr> <bytes>",
> -	  "Display Physical Memory", 0,
> -	  KDB_ENABLE_MEM_READ | KDB_REPEAT_NO_ARGS);
> -	kdb_register_flags("mds", kdb_md, "<vaddr>",
> -	  "Display Memory Symbolically", 0,
> -	  KDB_ENABLE_MEM_READ | KDB_REPEAT_NO_ARGS);
> -	kdb_register_flags("mm", kdb_mm, "<vaddr> <contents>",
> -	  "Modify Memory Contents", 0,
> -	  KDB_ENABLE_MEM_WRITE | KDB_REPEAT_NO_ARGS);
> -	kdb_register_flags("go", kdb_go, "[<vaddr>]",
> -	  "Continue Execution", 1,
> -	  KDB_ENABLE_REG_WRITE | KDB_ENABLE_ALWAYS_SAFE_NO_ARGS);
> -	kdb_register_flags("rd", kdb_rd, "",
> -	  "Display Registers", 0,
> -	  KDB_ENABLE_REG_READ);
> -	kdb_register_flags("rm", kdb_rm, "<reg> <contents>",
> -	  "Modify Registers", 0,
> -	  KDB_ENABLE_REG_WRITE);
> -	kdb_register_flags("ef", kdb_ef, "<vaddr>",
> -	  "Display exception frame", 0,
> -	  KDB_ENABLE_MEM_READ);
> -	kdb_register_flags("bt", kdb_bt, "[<vaddr>]",
> -	  "Stack traceback", 1,
> -	  KDB_ENABLE_MEM_READ | KDB_ENABLE_INSPECT_NO_ARGS);
> -	kdb_register_flags("btp", kdb_bt, "<pid>",
> -	  "Display stack for process <pid>", 0,
> -	  KDB_ENABLE_INSPECT);
> -	kdb_register_flags("bta", kdb_bt, "[D|R|S|T|C|Z|E|U|I|M|A]",
> -	  "Backtrace all processes matching state flag", 0,
> -	  KDB_ENABLE_INSPECT);
> -	kdb_register_flags("btc", kdb_bt, "",
> -	  "Backtrace current process on each cpu", 0,
> -	  KDB_ENABLE_INSPECT);
> -	kdb_register_flags("btt", kdb_bt, "<vaddr>",
> -	  "Backtrace process given its struct task address", 0,
> -	  KDB_ENABLE_MEM_READ | KDB_ENABLE_INSPECT_NO_ARGS);
> -	kdb_register_flags("env", kdb_env, "",
> -	  "Show environment variables", 0,
> -	  KDB_ENABLE_ALWAYS_SAFE);
> -	kdb_register_flags("set", kdb_set, "",
> -	  "Set environment variables", 0,
> -	  KDB_ENABLE_ALWAYS_SAFE);
> -	kdb_register_flags("help", kdb_help, "",
> -	  "Display Help Message", 1,
> -	  KDB_ENABLE_ALWAYS_SAFE);
> -	kdb_register_flags("?", kdb_help, "",
> -	  "Display Help Message", 0,
> -	  KDB_ENABLE_ALWAYS_SAFE);
> -	kdb_register_flags("cpu", kdb_cpu, "<cpunum>",
> -	  "Switch to new cpu", 0,
> -	  KDB_ENABLE_ALWAYS_SAFE_NO_ARGS);
> -	kdb_register_flags("kgdb", kdb_kgdb, "",
> -	  "Enter kgdb mode", 0, 0);
> -	kdb_register_flags("ps", kdb_ps, "[<flags>|A]",
> -	  "Display active task list", 0,
> -	  KDB_ENABLE_INSPECT);
> -	kdb_register_flags("pid", kdb_pid, "<pidnum>",
> -	  "Switch to another task", 0,
> -	  KDB_ENABLE_INSPECT);
> -	kdb_register_flags("reboot", kdb_reboot, "",
> -	  "Reboot the machine immediately", 0,
> -	  KDB_ENABLE_REBOOT);
> +static kdbtab_t maintab[] = {
> +	{	.cmd_name = "md",
> +		.cmd_func = kdb_md,
> +		.cmd_usage = "<vaddr>",
> +		.cmd_help = "Display Memory Contents, also mdWcN, e.g. md8c1",
> +		.cmd_minlen = 1,
> +		.cmd_flags = KDB_ENABLE_MEM_READ | KDB_REPEAT_NO_ARGS,
> +	},
> +	{	.cmd_name = "mdr",
> +		.cmd_func = kdb_md,
> +		.cmd_usage = "<vaddr> <bytes>",
> +		.cmd_help = "Display Raw Memory",
> +		.cmd_minlen = 0,
> +		.cmd_flags = KDB_ENABLE_MEM_READ | KDB_REPEAT_NO_ARGS,
> +	},
> +	{	.cmd_name = "mdp",
> +		.cmd_func = kdb_md,
> +		.cmd_usage = "<paddr> <bytes>",
> +		.cmd_help = "Display Physical Memory",
> +		.cmd_minlen = 0,
> +		.cmd_flags = KDB_ENABLE_MEM_READ | KDB_REPEAT_NO_ARGS,
> +	},
> +	{	.cmd_name = "mds",
> +		.cmd_func = kdb_md,
> +		.cmd_usage = "<vaddr>",
> +		.cmd_help = "Display Memory Symbolically",
> +		.cmd_minlen = 0,
> +		.cmd_flags = KDB_ENABLE_MEM_READ | KDB_REPEAT_NO_ARGS,
> +	},
> +	{	.cmd_name = "mm",
> +		.cmd_func = kdb_mm,
> +		.cmd_usage = "<vaddr> <contents>",
> +		.cmd_help = "Modify Memory Contents",
> +		.cmd_minlen = 0,
> +		.cmd_flags = KDB_ENABLE_MEM_WRITE | KDB_REPEAT_NO_ARGS,
> +	},
> +	{	.cmd_name = "go",
> +		.cmd_func = kdb_go,
> +		.cmd_usage = "[<vaddr>]",
> +		.cmd_help = "Continue Execution",
> +		.cmd_minlen = 1,
> +		.cmd_flags = KDB_ENABLE_REG_WRITE |
> +			     KDB_ENABLE_ALWAYS_SAFE_NO_ARGS,
> +	},
> +	{	.cmd_name = "rd",
> +		.cmd_func = kdb_rd,
> +		.cmd_usage = "",
> +		.cmd_help = "Display Registers",
> +		.cmd_minlen = 0,
> +		.cmd_flags = KDB_ENABLE_REG_READ,
> +	},
> +	{	.cmd_name = "rm",
> +		.cmd_func = kdb_rm,
> +		.cmd_usage = "<reg> <contents>",
> +		.cmd_help = "Modify Registers",
> +		.cmd_minlen = 0,
> +		.cmd_flags = KDB_ENABLE_REG_WRITE,
> +	},
> +	{	.cmd_name = "ef",
> +		.cmd_func = kdb_ef,
> +		.cmd_usage = "<vaddr>",
> +		.cmd_help = "Display exception frame",
> +		.cmd_minlen = 0,
> +		.cmd_flags = KDB_ENABLE_MEM_READ,
> +	},
> +	{	.cmd_name = "bt",
> +		.cmd_func = kdb_bt,
> +		.cmd_usage = "[<vaddr>]",
> +		.cmd_help = "Stack traceback",
> +		.cmd_minlen = 1,
> +		.cmd_flags = KDB_ENABLE_MEM_READ | KDB_ENABLE_INSPECT_NO_ARGS,
> +	},
> +	{	.cmd_name = "btp",
> +		.cmd_func = kdb_bt,
> +		.cmd_usage = "<pid>",
> +		.cmd_help = "Display stack for process <pid>",
> +		.cmd_minlen = 0,
> +		.cmd_flags = KDB_ENABLE_INSPECT,
> +	},
> +	{	.cmd_name = "bta",
> +		.cmd_func = kdb_bt,
> +		.cmd_usage = "[D|R|S|T|C|Z|E|U|I|M|A]",
> +		.cmd_help = "Backtrace all processes matching state flag",
> +		.cmd_minlen = 0,
> +		.cmd_flags = KDB_ENABLE_INSPECT,
> +	},
> +	{	.cmd_name = "btc",
> +		.cmd_func = kdb_bt,
> +		.cmd_usage = "",
> +		.cmd_help = "Backtrace current process on each cpu",
> +		.cmd_minlen = 0,
> +		.cmd_flags = KDB_ENABLE_INSPECT,
> +	},
> +	{	.cmd_name = "btt",
> +		.cmd_func = kdb_bt,
> +		.cmd_usage = "<vaddr>",
> +		.cmd_help = "Backtrace process given its struct task address",
> +		.cmd_minlen = 0,
> +		.cmd_flags = KDB_ENABLE_MEM_READ | KDB_ENABLE_INSPECT_NO_ARGS,
> +	},
> +	{	.cmd_name = "env",
> +		.cmd_func = kdb_env,
> +		.cmd_usage = "",
> +		.cmd_help = "Show environment variables",
> +		.cmd_minlen = 0,
> +		.cmd_flags = KDB_ENABLE_ALWAYS_SAFE,
> +	},
> +	{	.cmd_name = "set",
> +		.cmd_func = kdb_set,
> +		.cmd_usage = "",
> +		.cmd_help = "Set environment variables",
> +		.cmd_minlen = 0,
> +		.cmd_flags = KDB_ENABLE_ALWAYS_SAFE,
> +	},
> +	{	.cmd_name = "help",
> +		.cmd_func = kdb_help,
> +		.cmd_usage = "",
> +		.cmd_help = "Display Help Message",
> +		.cmd_minlen = 1,
> +		.cmd_flags = KDB_ENABLE_ALWAYS_SAFE,
> +	},
> +	{	.cmd_name = "?",
> +		.cmd_func = kdb_help,
> +		.cmd_usage = "",
> +		.cmd_help = "Display Help Message",
> +		.cmd_minlen = 0,
> +		.cmd_flags = KDB_ENABLE_ALWAYS_SAFE,
> +	},
> +	{	.cmd_name = "cpu",
> +		.cmd_func = kdb_cpu,
> +		.cmd_usage = "<cpunum>",
> +		.cmd_help = "Switch to new cpu",
> +		.cmd_minlen = 0,
> +		.cmd_flags = KDB_ENABLE_ALWAYS_SAFE_NO_ARGS,
> +	},
> +	{	.cmd_name = "kgdb",
> +		.cmd_func = kdb_kgdb,
> +		.cmd_usage = "",
> +		.cmd_help = "Enter kgdb mode",
> +		.cmd_minlen = 0,
> +		.cmd_flags = 0,
> +	},
> +	{	.cmd_name = "ps",
> +		.cmd_func = kdb_ps,
> +		.cmd_usage = "[<flags>|A]",
> +		.cmd_help = "Display active task list",
> +		.cmd_minlen = 0,
> +		.cmd_flags = KDB_ENABLE_INSPECT,
> +	},
> +	{	.cmd_name = "pid",
> +		.cmd_func = kdb_pid,
> +		.cmd_usage = "<pidnum>",
> +		.cmd_help = "Switch to another task",
> +		.cmd_minlen = 0,
> +		.cmd_flags = KDB_ENABLE_INSPECT,
> +	},
> +	{	.cmd_name = "reboot",
> +		.cmd_func = kdb_reboot,
> +		.cmd_usage = "",
> +		.cmd_help = "Reboot the machine immediately",
> +		.cmd_minlen = 0,
> +		.cmd_flags = KDB_ENABLE_REBOOT,
> +	},
>  #if defined(CONFIG_MODULES)
> -	kdb_register_flags("lsmod", kdb_lsmod, "",
> -	  "List loaded kernel modules", 0,
> -	  KDB_ENABLE_INSPECT);
> +	{	.cmd_name = "lsmod",
> +		.cmd_func = kdb_lsmod,
> +		.cmd_usage = "",
> +		.cmd_help = "List loaded kernel modules",
> +		.cmd_minlen = 0,
> +		.cmd_flags = KDB_ENABLE_INSPECT,
> +	},
>  #endif
>  #if defined(CONFIG_MAGIC_SYSRQ)
> -	kdb_register_flags("sr", kdb_sr, "<key>",
> -	  "Magic SysRq key", 0,
> -	  KDB_ENABLE_ALWAYS_SAFE);
> +	{	.cmd_name = "sr",
> +		.cmd_func = kdb_sr,
> +		.cmd_usage = "<key>",
> +		.cmd_help = "Magic SysRq key",
> +		.cmd_minlen = 0,
> +		.cmd_flags = KDB_ENABLE_ALWAYS_SAFE,
> +	},
>  #endif
>  #if defined(CONFIG_PRINTK)
> -	kdb_register_flags("dmesg", kdb_dmesg, "[lines]",
> -	  "Display syslog buffer", 0,
> -	  KDB_ENABLE_ALWAYS_SAFE);
> +	{	.cmd_name = "dmesg",
> +		.cmd_func = kdb_dmesg,
> +		.cmd_usage = "[lines]",
> +		.cmd_help = "Display syslog buffer",
> +		.cmd_minlen = 0,
> +		.cmd_flags = KDB_ENABLE_ALWAYS_SAFE,
> +	},
>  #endif
> -	if (arch_kgdb_ops.enable_nmi) {
> -		kdb_register_flags("disable_nmi", kdb_disable_nmi, "",
> -		  "Disable NMI entry to KDB", 0,
> -		  KDB_ENABLE_ALWAYS_SAFE);
> -	}
> -	kdb_register_flags("defcmd", kdb_defcmd, "name \"usage\" \"help\"",
> -	  "Define a set of commands, down to endefcmd", 0,
> -	  KDB_ENABLE_ALWAYS_SAFE);
> -	kdb_register_flags("kill", kdb_kill, "<-signal> <pid>",
> -	  "Send a signal to a process", 0,
> -	  KDB_ENABLE_SIGNAL);
> -	kdb_register_flags("summary", kdb_summary, "",
> -	  "Summarize the system", 4,
> -	  KDB_ENABLE_ALWAYS_SAFE);
> -	kdb_register_flags("per_cpu", kdb_per_cpu, "<sym> [<bytes>] [<cpu>]",
> -	  "Display per_cpu variables", 3,
> -	  KDB_ENABLE_MEM_READ);
> -	kdb_register_flags("grephelp", kdb_grep_help, "",
> -	  "Display help on | grep", 0,
> -	  KDB_ENABLE_ALWAYS_SAFE);
> +	{	.cmd_name = "defcmd",
> +		.cmd_func = kdb_defcmd,
> +		.cmd_usage = "name \"usage\" \"help\"",
> +		.cmd_help = "Define a set of commands, down to endefcmd",
> +		.cmd_minlen = 0,
> +		.cmd_flags = KDB_ENABLE_ALWAYS_SAFE,
> +	},
> +	{	.cmd_name = "kill",
> +		.cmd_func = kdb_kill,
> +		.cmd_usage = "<-signal> <pid>",
> +		.cmd_help = "Send a signal to a process",
> +		.cmd_minlen = 0,
> +		.cmd_flags = KDB_ENABLE_SIGNAL,
> +	},
> +	{	.cmd_name = "summary",
> +		.cmd_func = kdb_summary,
> +		.cmd_usage = "",
> +		.cmd_help = "Summarize the system",
> +		.cmd_minlen = 4,
> +		.cmd_flags = KDB_ENABLE_ALWAYS_SAFE,
> +	},
> +	{	.cmd_name = "per_cpu",
> +		.cmd_func = kdb_per_cpu,
> +		.cmd_usage = "<sym> [<bytes>] [<cpu>]",
> +		.cmd_help = "Display per_cpu variables",
> +		.cmd_minlen = 3,
> +		.cmd_flags = KDB_ENABLE_MEM_READ,
> +	},
> +	{	.cmd_name = "grephelp",
> +		.cmd_func = kdb_grep_help,
> +		.cmd_usage = "",
> +		.cmd_help = "Display help on | grep",
> +		.cmd_minlen = 0,
> +		.cmd_flags = KDB_ENABLE_ALWAYS_SAFE,
> +	},
> +};
> +
> +static kdbtab_t nmicmd = {
> +	.cmd_name = "disable_nmi",
> +	.cmd_func = kdb_disable_nmi,
> +	.cmd_usage = "",
> +	.cmd_help = "Disable NMI entry to KDB",
> +	.cmd_minlen = 0,
> +	.cmd_flags = KDB_ENABLE_ALWAYS_SAFE,
> +};
> +
> +/* Initialize the kdb command table. */
> +static void __init kdb_inittab(void)
> +{
> +	kdb_register_table(maintab, ARRAY_SIZE(maintab));
> +	if (arch_kgdb_ops.enable_nmi)
> +		kdb_register_table(&nmicmd, 1);
>  }
>  
>  /* Execute any commands defined in kdb_cmds.  */
> diff --git a/kernel/debug/kdb/kdb_private.h b/kernel/debug/kdb/kdb_private.h
> index a4281fb99299..2d4030fa56cf 100644
> --- a/kernel/debug/kdb/kdb_private.h
> +++ b/kernel/debug/kdb/kdb_private.h
> @@ -174,8 +174,11 @@ typedef struct _kdbtab {
>  	short    cmd_minlen;		/* Minimum legal # command
>  					 * chars required */
>  	kdb_cmdflags_t cmd_flags;	/* Command behaviour flags */
> +	struct list_head list_node;	/* Command list */
> +	bool    is_dynamic;		/* Command table allocation type */
>  } kdbtab_t;
>  
> +extern void kdb_register_table(kdbtab_t *kp, size_t len);
>  extern int kdb_bt(int, const char **);	/* KDB display back trace */
>  
>  /* KDB breakpoint management functions */
> -- 
> 2.25.1

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2021-03-22 16:16 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-24  7:08 [PATCH v5] kdb: Simplify kdb commands registration Sumit Garg
2021-03-01 18:40 ` Doug Anderson
2021-03-04 10:58   ` Sumit Garg
2021-03-22 16:15 ` Daniel Thompson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).