* [PATCH v3 0/2] kdb code refactoring @ 2021-05-13 11:28 Sumit Garg 2021-05-13 11:28 ` [PATCH v3 1/2] kdb: Get rid of redundant kdb_register_flags() Sumit Garg 2021-05-13 11:28 ` [PATCH v3 2/2] kdb: Simplify kdb_defcmd macro logic Sumit Garg 0 siblings, 2 replies; 8+ messages in thread From: Sumit Garg @ 2021-05-13 11:28 UTC (permalink / raw) To: kgdb-bugreport Cc: daniel.thompson, jason.wessel, dianders, rostedt, mingo, linux-kernel, Sumit Garg Some more kdb code refactoring related to: - Removal of redundant kdb_register_flags(). - Simplification of kdb defcmd macro logic. Tested with kgdbtest on arm64, doesn't show any regressions. Changes in v3: - Split patch into 2 for ease of review. - Get rid of kdb_register_flags() completely via switching all user to register pre-allocated kdb commands. Changes in v2: - Define new structs: kdb_macro_t and kdb_macro_cmd_t instead of modifying existing kdb command struct and struct kdb_subcmd. - Reword commit message. Sumit Garg (2): kdb: Get rid of redundant kdb_register_flags() kdb: Simplify kdb_defcmd macro logic include/linux/kdb.h | 27 ++-- kernel/debug/kdb/kdb_main.c | 271 +++++++++++++-------------------- kernel/debug/kdb/kdb_private.h | 13 -- kernel/trace/trace_kdb.c | 12 +- samples/kdb/kdb_hello.c | 20 ++- 5 files changed, 141 insertions(+), 202 deletions(-) -- 2.25.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v3 1/2] kdb: Get rid of redundant kdb_register_flags() 2021-05-13 11:28 [PATCH v3 0/2] kdb code refactoring Sumit Garg @ 2021-05-13 11:28 ` Sumit Garg 2021-05-13 11:31 ` Sumit Garg ` (2 more replies) 2021-05-13 11:28 ` [PATCH v3 2/2] kdb: Simplify kdb_defcmd macro logic Sumit Garg 1 sibling, 3 replies; 8+ messages in thread From: Sumit Garg @ 2021-05-13 11:28 UTC (permalink / raw) To: kgdb-bugreport Cc: daniel.thompson, jason.wessel, dianders, rostedt, mingo, linux-kernel, Sumit Garg Commit e4f291b3f7bb ("kdb: Simplify kdb commands registration") allowed registration of pre-allocated kdb commands with pointer to struct kdbtab_t. Lets switch other users as well to register pre- allocated kdb commands via: - Changing prototype for kdb_register() to pass a pointer to struct kdbtab_t instead. - Embed kdbtab_t structure in defcmd_set rather than individual params. So while at it rename struct defcmd_set to struct kdb_macro_t as that sounds more appropriate given its purpose. With these changes kdb_register_flags() becomes redundant and hence removed. Also, since we have switched all users to register pre-allocated commands, "is_dynamic" flag in struct kdbtab_t becomes redundant and hence removed as well. Signed-off-by: Sumit Garg <sumit.garg@linaro.org> --- include/linux/kdb.h | 27 +++-- kernel/debug/kdb/kdb_main.c | 206 +++++++++++---------------------- kernel/debug/kdb/kdb_private.h | 13 --- kernel/trace/trace_kdb.c | 12 +- samples/kdb/kdb_hello.c | 20 ++-- 5 files changed, 104 insertions(+), 174 deletions(-) diff --git a/include/linux/kdb.h b/include/linux/kdb.h index 0125a677b67f..9e893279b2ea 100644 --- a/include/linux/kdb.h +++ b/include/linux/kdb.h @@ -13,6 +13,8 @@ * Copyright (C) 2009 Jason Wessel <jason.wessel@windriver.com> */ +#include <linux/list.h> + /* Shifted versions of the command enable bits are be used if the command * has no arguments (see kdb_check_flags). This allows commands, such as * go, to have different permissions depending upon whether it is called @@ -64,6 +66,17 @@ typedef enum { typedef int (*kdb_func_t)(int, const char **); +/* The KDB shell command table */ +typedef struct _kdbtab { + char *cmd_name; /* Command name */ + kdb_func_t cmd_func; /* Function to execute command */ + char *cmd_usage; /* Usage String for this command */ + char *cmd_help; /* Help message for this command */ + short cmd_minlen; /* Minimum legal # cmd chars required */ + kdb_cmdflags_t cmd_flags; /* Command behaviour flags */ + struct list_head list_node; /* Command list */ +} kdbtab_t; + #ifdef CONFIG_KGDB_KDB #include <linux/init.h> #include <linux/sched.h> @@ -193,19 +206,13 @@ static inline const char *kdb_walk_kallsyms(loff_t *pos) #endif /* ! CONFIG_KALLSYMS */ /* Dynamic kdb shell command registration */ -extern int kdb_register(char *, kdb_func_t, char *, char *, short); -extern int kdb_register_flags(char *, kdb_func_t, char *, char *, - short, kdb_cmdflags_t); -extern int kdb_unregister(char *); +extern int kdb_register(kdbtab_t *cmd); +extern int kdb_unregister(kdbtab_t *cmd); #else /* ! CONFIG_KGDB_KDB */ static inline __printf(1, 2) int kdb_printf(const char *fmt, ...) { return 0; } static inline void kdb_init(int level) {} -static inline int kdb_register(char *cmd, kdb_func_t func, char *usage, - char *help, short minlen) { return 0; } -static inline int kdb_register_flags(char *cmd, kdb_func_t func, char *usage, - char *help, short minlen, - kdb_cmdflags_t flags) { return 0; } -static inline int kdb_unregister(char *cmd) { return 0; } +static inline int kdb_register(kdbtab_t *cmd) { return 0; } +static inline int kdb_unregister(kdbtab_t *cmd) { return 0; } #endif /* CONFIG_KGDB_KDB */ enum { KDB_NOT_INITIALIZED, diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c index 1baa96a2ecb8..de685b2a8ce7 100644 --- a/kernel/debug/kdb/kdb_main.c +++ b/kernel/debug/kdb/kdb_main.c @@ -33,7 +33,6 @@ #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> @@ -654,16 +653,14 @@ static void kdb_cmderror(int diag) * Returns: * zero for success, a kdb diagnostic if error */ -struct defcmd_set { +struct kdb_macro_t { int count; bool usable; - char *name; - char *usage; - char *help; + kdbtab_t cmd; char **command; }; -static struct defcmd_set *defcmd_set; -static int defcmd_set_count; +static struct kdb_macro_t *kdb_macro; +static int kdb_macro_count; static bool defcmd_in_progress; /* Forward references */ @@ -671,20 +668,14 @@ static int kdb_exec_defcmd(int argc, const char **argv); static int kdb_defcmd2(const char *cmdstr, const char *argv0) { - struct defcmd_set *s = defcmd_set + defcmd_set_count - 1; + struct kdb_macro_t *s = kdb_macro + kdb_macro_count - 1; char **save_command = s->command; if (strcmp(argv0, "endefcmd") == 0) { defcmd_in_progress = false; if (!s->count) s->usable = false; if (s->usable) - /* macros are always safe because when executed each - * internal command re-enters kdb_parse() and is - * safety checked individually. - */ - kdb_register_flags(s->name, kdb_exec_defcmd, s->usage, - s->help, 0, - KDB_ENABLE_ALWAYS_SAFE); + kdb_register(&s->cmd); return 0; } if (!s->usable) @@ -704,7 +695,9 @@ static int kdb_defcmd2(const char *cmdstr, const char *argv0) static int kdb_defcmd(int argc, const char **argv) { - struct defcmd_set *save_defcmd_set = defcmd_set, *s; + struct kdb_macro_t *save_kdb_macro = kdb_macro, *s; + kdbtab_t *mp; + if (defcmd_in_progress) { kdb_printf("kdb: nested defcmd detected, assuming missing " "endefcmd\n"); @@ -712,9 +705,9 @@ static int kdb_defcmd(int argc, const char **argv) } if (argc == 0) { int i; - for (s = defcmd_set; s < defcmd_set + defcmd_set_count; ++s) { - kdb_printf("defcmd %s \"%s\" \"%s\"\n", s->name, - s->usage, s->help); + for (s = kdb_macro; s < kdb_macro + kdb_macro_count; ++s) { + kdb_printf("defcmd %s \"%s\" \"%s\"\n", s->cmd.cmd_name, + s->cmd.cmd_usage, s->cmd.cmd_help); for (i = 0; i < s->count; ++i) kdb_printf("%s", s->command[i]); kdb_printf("endefcmd\n"); @@ -727,45 +720,50 @@ static int kdb_defcmd(int argc, const char **argv) kdb_printf("Command only available during kdb_init()\n"); return KDB_NOTIMP; } - defcmd_set = kmalloc_array(defcmd_set_count + 1, sizeof(*defcmd_set), + kdb_macro = kmalloc_array(kdb_macro_count + 1, sizeof(*kdb_macro), GFP_KDB); - if (!defcmd_set) + if (!kdb_macro) goto fail_defcmd; - memcpy(defcmd_set, save_defcmd_set, - defcmd_set_count * sizeof(*defcmd_set)); - s = defcmd_set + defcmd_set_count; + memcpy(kdb_macro, save_kdb_macro, + kdb_macro_count * sizeof(*kdb_macro)); + s = kdb_macro + kdb_macro_count; memset(s, 0, sizeof(*s)); s->usable = true; - s->name = kdb_strdup(argv[1], GFP_KDB); - if (!s->name) + + mp = &s->cmd; + mp->cmd_func = kdb_exec_defcmd; + mp->cmd_minlen = 0; + mp->cmd_flags = KDB_ENABLE_ALWAYS_SAFE; + mp->cmd_name = kdb_strdup(argv[1], GFP_KDB); + if (!mp->cmd_name) goto fail_name; - s->usage = kdb_strdup(argv[2], GFP_KDB); - if (!s->usage) + mp->cmd_usage = kdb_strdup(argv[2], GFP_KDB); + if (!mp->cmd_usage) goto fail_usage; - s->help = kdb_strdup(argv[3], GFP_KDB); - if (!s->help) + mp->cmd_help = kdb_strdup(argv[3], GFP_KDB); + if (!mp->cmd_help) goto fail_help; - if (s->usage[0] == '"') { - strcpy(s->usage, argv[2]+1); - s->usage[strlen(s->usage)-1] = '\0'; + if (mp->cmd_usage[0] == '"') { + strcpy(mp->cmd_usage, argv[2]+1); + mp->cmd_usage[strlen(mp->cmd_usage)-1] = '\0'; } - if (s->help[0] == '"') { - strcpy(s->help, argv[3]+1); - s->help[strlen(s->help)-1] = '\0'; + if (mp->cmd_help[0] == '"') { + strcpy(mp->cmd_help, argv[3]+1); + mp->cmd_help[strlen(mp->cmd_help)-1] = '\0'; } - ++defcmd_set_count; + ++kdb_macro_count; defcmd_in_progress = true; - kfree(save_defcmd_set); + kfree(save_kdb_macro); return 0; fail_help: - kfree(s->usage); + kfree(mp->cmd_usage); fail_usage: - kfree(s->name); + kfree(mp->cmd_name); fail_name: - kfree(defcmd_set); + kfree(kdb_macro); fail_defcmd: - kdb_printf("Could not allocate new defcmd_set entry for %s\n", argv[1]); - defcmd_set = save_defcmd_set; + kdb_printf("Could not allocate new kdb_macro entry for %s\n", argv[1]); + kdb_macro = save_kdb_macro; return KDB_NOTIMP; } @@ -781,14 +779,14 @@ static int kdb_defcmd(int argc, const char **argv) static int kdb_exec_defcmd(int argc, const char **argv) { int i, ret; - struct defcmd_set *s; + struct kdb_macro_t *s; if (argc != 0) return KDB_ARGCOUNT; - for (s = defcmd_set, i = 0; i < defcmd_set_count; ++i, ++s) { - if (strcmp(s->name, argv[0]) == 0) + for (s = kdb_macro, i = 0; i < kdb_macro_count; ++i, ++s) { + if (strcmp(s->cmd.cmd_name, argv[0]) == 0) break; } - if (i == defcmd_set_count) { + if (i == kdb_macro_count) { kdb_printf("kdb_exec_defcmd: could not find commands for %s\n", argv[0]); return KDB_NOTIMP; @@ -797,7 +795,7 @@ static int kdb_exec_defcmd(int argc, const char **argv) /* Recursive use of kdb_parse, do not use argv after * this point */ argv = NULL; - kdb_printf("[%s]kdb> %s\n", s->name, s->command[i]); + kdb_printf("[%s]kdb> %s\n", s->cmd.cmd_name, s->command[i]); ret = kdb_parse(s->command[i]); if (ret) return ret; @@ -2620,55 +2618,6 @@ static int kdb_grep_help(int argc, const char **argv) return 0; } -/* - * kdb_register_flags - This function is used to register a kernel - * debugger command. - * Inputs: - * cmd Command name - * func Function to execute the command - * usage A simple usage string showing arguments - * help A simple help string describing command - * repeat Does the command auto repeat on enter? - * Returns: - * zero for success, one if a duplicate command. - */ -int kdb_register_flags(char *cmd, - kdb_func_t func, - char *usage, - char *help, - short minlen, - kdb_cmdflags_t flags) -{ - kdbtab_t *kp; - - 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; - } - } - - kp = kmalloc(sizeof(*kp), GFP_KDB); - if (!kp) { - kdb_printf("Could not allocate new kdb_command table\n"); - return 1; - } - - kp->cmd_name = cmd; - kp->cmd_func = func; - kp->cmd_usage = usage; - 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. @@ -2684,54 +2633,37 @@ void kdb_register_table(kdbtab_t *kp, size_t len) } /* - * kdb_register - Compatibility register function for commands that do - * not need to specify a repeat state. Equivalent to - * kdb_register_flags with flags set to 0. - * Inputs: - * cmd Command name - * func Function to execute the command - * usage A simple usage string showing arguments - * help A simple help string describing command - * Returns: - * zero for success, one if a duplicate command. + * kdb_register() - This function is used to register a kernel debugger + * command. + * @cmd: pointer to kdb command */ -int kdb_register(char *cmd, - kdb_func_t func, - char *usage, - char *help, - short minlen) -{ - return kdb_register_flags(cmd, func, usage, help, minlen, 0); -} -EXPORT_SYMBOL_GPL(kdb_register); - -/* - * kdb_unregister - This function is used to unregister a kernel - * debugger command. It is generally called when a module which - * implements kdb commands is unloaded. - * Inputs: - * cmd Command name - * Returns: - * zero for success, one command not registered. - */ -int kdb_unregister(char *cmd) +int kdb_register(kdbtab_t *cmd) { kdbtab_t *kp; - /* - * find the command. - */ 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; + if (strcmp(kp->cmd_name, cmd->cmd_name) == 0) { + kdb_printf("Duplicate kdb cmd: %s, func %p help %s\n", + cmd->cmd_name, cmd->cmd_func, cmd->cmd_help); + return 1; } } - /* Couldn't find it. */ - return 1; + list_add_tail(&cmd->list_node, &kdb_cmds_head); + return 0; +} +EXPORT_SYMBOL_GPL(kdb_register); + +/* + * kdb_unregister() - This function is used to unregister a kernel debugger + * command. It is generally called when a module which + * implements kdb command is unloaded. + * @cmd: pointer to kdb command + */ +int kdb_unregister(kdbtab_t *cmd) +{ + list_del(&cmd->list_node); + return 0; } EXPORT_SYMBOL_GPL(kdb_unregister); diff --git a/kernel/debug/kdb/kdb_private.h b/kernel/debug/kdb/kdb_private.h index 95d2b730af4d..608d57f3c419 100644 --- a/kernel/debug/kdb/kdb_private.h +++ b/kernel/debug/kdb/kdb_private.h @@ -164,19 +164,6 @@ typedef struct _kdb_bp { #ifdef CONFIG_KGDB_KDB extern kdb_bp_t kdb_breakpoints[/* KDB_MAXBPT */]; -/* The KDB shell command table */ -typedef struct _kdbtab { - char *cmd_name; /* Command name */ - kdb_func_t cmd_func; /* Function to execute command */ - char *cmd_usage; /* Usage String for this command */ - char *cmd_help; /* Help message for this command */ - 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 */ diff --git a/kernel/trace/trace_kdb.c b/kernel/trace/trace_kdb.c index 9da76104f7a2..6c4f92c79e43 100644 --- a/kernel/trace/trace_kdb.c +++ b/kernel/trace/trace_kdb.c @@ -147,11 +147,17 @@ static int kdb_ftdump(int argc, const char **argv) return 0; } +static kdbtab_t ftdump_cmd = { + .cmd_name = "ftdump", + .cmd_func = kdb_ftdump, + .cmd_usage = "[skip_#entries] [cpu]", + .cmd_help = "Dump ftrace log; -skip dumps last #entries", + .cmd_flags = KDB_ENABLE_ALWAYS_SAFE, +}; + static __init int kdb_ftrace_register(void) { - kdb_register_flags("ftdump", kdb_ftdump, "[skip_#entries] [cpu]", - "Dump ftrace log; -skip dumps last #entries", 0, - KDB_ENABLE_ALWAYS_SAFE); + kdb_register(&ftdump_cmd); return 0; } diff --git a/samples/kdb/kdb_hello.c b/samples/kdb/kdb_hello.c index c1c2fa0f62c2..9ad514a6648b 100644 --- a/samples/kdb/kdb_hello.c +++ b/samples/kdb/kdb_hello.c @@ -28,28 +28,26 @@ static int kdb_hello_cmd(int argc, const char **argv) return 0; } +static kdbtab_t hello_cmd = { + .cmd_name = "hello", + .cmd_func = kdb_hello_cmd, + .cmd_usage = "[string]", + .cmd_help = "Say Hello World or Hello [string]", +}; static int __init kdb_hello_cmd_init(void) { /* * Registration of a dynamically added kdb command is done with - * kdb_register() with the arguments being: - * 1: The name of the shell command - * 2: The function that processes the command - * 3: Description of the usage of any arguments - * 4: Descriptive text when you run help - * 5: Number of characters to complete the command - * 0 == type the whole command - * 1 == match both "g" and "go" for example + * kdb_register(). */ - kdb_register("hello", kdb_hello_cmd, "[string]", - "Say Hello World or Hello [string]", 0); + kdb_register(&hello_cmd); return 0; } static void __exit kdb_hello_cmd_exit(void) { - kdb_unregister("hello"); + kdb_unregister(&hello_cmd); } module_init(kdb_hello_cmd_init); -- 2.25.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v3 1/2] kdb: Get rid of redundant kdb_register_flags() 2021-05-13 11:28 ` [PATCH v3 1/2] kdb: Get rid of redundant kdb_register_flags() Sumit Garg @ 2021-05-13 11:31 ` Sumit Garg 2021-05-13 13:19 ` Steven Rostedt 2021-05-20 17:20 ` Doug Anderson 2 siblings, 0 replies; 8+ messages in thread From: Sumit Garg @ 2021-05-13 11:31 UTC (permalink / raw) To: kgdb-bugreport Cc: Daniel Thompson, Jason Wessel, Douglas Anderson, Steven Rostedt, mingo, Linux Kernel Mailing List On Thu, 13 May 2021 at 16:59, Sumit Garg <sumit.garg@linaro.org> wrote: > > Commit e4f291b3f7bb ("kdb: Simplify kdb commands registration") > allowed registration of pre-allocated kdb commands with pointer to > struct kdbtab_t. Lets switch other users as well to register pre- > allocated kdb commands via: > - Changing prototype for kdb_register() to pass a pointer to struct > kdbtab_t instead. > - Embed kdbtab_t structure in defcmd_set rather than individual params. > So while at it rename struct defcmd_set to struct kdb_macro_t as that > sounds more appropriate given its purpose. > > With these changes kdb_register_flags() becomes redundant and hence > removed. Also, since we have switched all users to register > pre-allocated commands, "is_dynamic" flag in struct kdbtab_t becomes > redundant and hence removed as well. > Missed Daniel's suggested by tag, so here it is: Suggested-by: Daniel Thompson <daniel.thompson@linaro.org> > Signed-off-by: Sumit Garg <sumit.garg@linaro.org> > --- > include/linux/kdb.h | 27 +++-- > kernel/debug/kdb/kdb_main.c | 206 +++++++++++---------------------- > kernel/debug/kdb/kdb_private.h | 13 --- > kernel/trace/trace_kdb.c | 12 +- > samples/kdb/kdb_hello.c | 20 ++-- > 5 files changed, 104 insertions(+), 174 deletions(-) > > diff --git a/include/linux/kdb.h b/include/linux/kdb.h > index 0125a677b67f..9e893279b2ea 100644 > --- a/include/linux/kdb.h > +++ b/include/linux/kdb.h > @@ -13,6 +13,8 @@ > * Copyright (C) 2009 Jason Wessel <jason.wessel@windriver.com> > */ > > +#include <linux/list.h> > + > /* Shifted versions of the command enable bits are be used if the command > * has no arguments (see kdb_check_flags). This allows commands, such as > * go, to have different permissions depending upon whether it is called > @@ -64,6 +66,17 @@ typedef enum { > > typedef int (*kdb_func_t)(int, const char **); > > +/* The KDB shell command table */ > +typedef struct _kdbtab { > + char *cmd_name; /* Command name */ > + kdb_func_t cmd_func; /* Function to execute command */ > + char *cmd_usage; /* Usage String for this command */ > + char *cmd_help; /* Help message for this command */ > + short cmd_minlen; /* Minimum legal # cmd chars required */ > + kdb_cmdflags_t cmd_flags; /* Command behaviour flags */ > + struct list_head list_node; /* Command list */ > +} kdbtab_t; > + > #ifdef CONFIG_KGDB_KDB > #include <linux/init.h> > #include <linux/sched.h> > @@ -193,19 +206,13 @@ static inline const char *kdb_walk_kallsyms(loff_t *pos) > #endif /* ! CONFIG_KALLSYMS */ > > /* Dynamic kdb shell command registration */ > -extern int kdb_register(char *, kdb_func_t, char *, char *, short); > -extern int kdb_register_flags(char *, kdb_func_t, char *, char *, > - short, kdb_cmdflags_t); > -extern int kdb_unregister(char *); > +extern int kdb_register(kdbtab_t *cmd); > +extern int kdb_unregister(kdbtab_t *cmd); > #else /* ! CONFIG_KGDB_KDB */ > static inline __printf(1, 2) int kdb_printf(const char *fmt, ...) { return 0; } > static inline void kdb_init(int level) {} > -static inline int kdb_register(char *cmd, kdb_func_t func, char *usage, > - char *help, short minlen) { return 0; } > -static inline int kdb_register_flags(char *cmd, kdb_func_t func, char *usage, > - char *help, short minlen, > - kdb_cmdflags_t flags) { return 0; } > -static inline int kdb_unregister(char *cmd) { return 0; } > +static inline int kdb_register(kdbtab_t *cmd) { return 0; } > +static inline int kdb_unregister(kdbtab_t *cmd) { return 0; } > #endif /* CONFIG_KGDB_KDB */ > enum { > KDB_NOT_INITIALIZED, > diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c > index 1baa96a2ecb8..de685b2a8ce7 100644 > --- a/kernel/debug/kdb/kdb_main.c > +++ b/kernel/debug/kdb/kdb_main.c > @@ -33,7 +33,6 @@ > #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> > @@ -654,16 +653,14 @@ static void kdb_cmderror(int diag) > * Returns: > * zero for success, a kdb diagnostic if error > */ > -struct defcmd_set { > +struct kdb_macro_t { > int count; > bool usable; > - char *name; > - char *usage; > - char *help; > + kdbtab_t cmd; > char **command; > }; > -static struct defcmd_set *defcmd_set; > -static int defcmd_set_count; > +static struct kdb_macro_t *kdb_macro; > +static int kdb_macro_count; > static bool defcmd_in_progress; > > /* Forward references */ > @@ -671,20 +668,14 @@ static int kdb_exec_defcmd(int argc, const char **argv); > > static int kdb_defcmd2(const char *cmdstr, const char *argv0) > { > - struct defcmd_set *s = defcmd_set + defcmd_set_count - 1; > + struct kdb_macro_t *s = kdb_macro + kdb_macro_count - 1; > char **save_command = s->command; > if (strcmp(argv0, "endefcmd") == 0) { > defcmd_in_progress = false; > if (!s->count) > s->usable = false; > if (s->usable) > - /* macros are always safe because when executed each > - * internal command re-enters kdb_parse() and is > - * safety checked individually. > - */ > - kdb_register_flags(s->name, kdb_exec_defcmd, s->usage, > - s->help, 0, > - KDB_ENABLE_ALWAYS_SAFE); > + kdb_register(&s->cmd); > return 0; > } > if (!s->usable) > @@ -704,7 +695,9 @@ static int kdb_defcmd2(const char *cmdstr, const char *argv0) > > static int kdb_defcmd(int argc, const char **argv) > { > - struct defcmd_set *save_defcmd_set = defcmd_set, *s; > + struct kdb_macro_t *save_kdb_macro = kdb_macro, *s; > + kdbtab_t *mp; > + > if (defcmd_in_progress) { > kdb_printf("kdb: nested defcmd detected, assuming missing " > "endefcmd\n"); > @@ -712,9 +705,9 @@ static int kdb_defcmd(int argc, const char **argv) > } > if (argc == 0) { > int i; > - for (s = defcmd_set; s < defcmd_set + defcmd_set_count; ++s) { > - kdb_printf("defcmd %s \"%s\" \"%s\"\n", s->name, > - s->usage, s->help); > + for (s = kdb_macro; s < kdb_macro + kdb_macro_count; ++s) { > + kdb_printf("defcmd %s \"%s\" \"%s\"\n", s->cmd.cmd_name, > + s->cmd.cmd_usage, s->cmd.cmd_help); > for (i = 0; i < s->count; ++i) > kdb_printf("%s", s->command[i]); > kdb_printf("endefcmd\n"); > @@ -727,45 +720,50 @@ static int kdb_defcmd(int argc, const char **argv) > kdb_printf("Command only available during kdb_init()\n"); > return KDB_NOTIMP; > } > - defcmd_set = kmalloc_array(defcmd_set_count + 1, sizeof(*defcmd_set), > + kdb_macro = kmalloc_array(kdb_macro_count + 1, sizeof(*kdb_macro), > GFP_KDB); > - if (!defcmd_set) > + if (!kdb_macro) > goto fail_defcmd; > - memcpy(defcmd_set, save_defcmd_set, > - defcmd_set_count * sizeof(*defcmd_set)); > - s = defcmd_set + defcmd_set_count; > + memcpy(kdb_macro, save_kdb_macro, > + kdb_macro_count * sizeof(*kdb_macro)); > + s = kdb_macro + kdb_macro_count; > memset(s, 0, sizeof(*s)); > s->usable = true; > - s->name = kdb_strdup(argv[1], GFP_KDB); > - if (!s->name) > + > + mp = &s->cmd; > + mp->cmd_func = kdb_exec_defcmd; > + mp->cmd_minlen = 0; > + mp->cmd_flags = KDB_ENABLE_ALWAYS_SAFE; > + mp->cmd_name = kdb_strdup(argv[1], GFP_KDB); > + if (!mp->cmd_name) > goto fail_name; > - s->usage = kdb_strdup(argv[2], GFP_KDB); > - if (!s->usage) > + mp->cmd_usage = kdb_strdup(argv[2], GFP_KDB); > + if (!mp->cmd_usage) > goto fail_usage; > - s->help = kdb_strdup(argv[3], GFP_KDB); > - if (!s->help) > + mp->cmd_help = kdb_strdup(argv[3], GFP_KDB); > + if (!mp->cmd_help) > goto fail_help; > - if (s->usage[0] == '"') { > - strcpy(s->usage, argv[2]+1); > - s->usage[strlen(s->usage)-1] = '\0'; > + if (mp->cmd_usage[0] == '"') { > + strcpy(mp->cmd_usage, argv[2]+1); > + mp->cmd_usage[strlen(mp->cmd_usage)-1] = '\0'; > } > - if (s->help[0] == '"') { > - strcpy(s->help, argv[3]+1); > - s->help[strlen(s->help)-1] = '\0'; > + if (mp->cmd_help[0] == '"') { > + strcpy(mp->cmd_help, argv[3]+1); > + mp->cmd_help[strlen(mp->cmd_help)-1] = '\0'; > } > - ++defcmd_set_count; > + ++kdb_macro_count; > defcmd_in_progress = true; > - kfree(save_defcmd_set); > + kfree(save_kdb_macro); > return 0; > fail_help: > - kfree(s->usage); > + kfree(mp->cmd_usage); > fail_usage: > - kfree(s->name); > + kfree(mp->cmd_name); > fail_name: > - kfree(defcmd_set); > + kfree(kdb_macro); > fail_defcmd: > - kdb_printf("Could not allocate new defcmd_set entry for %s\n", argv[1]); > - defcmd_set = save_defcmd_set; > + kdb_printf("Could not allocate new kdb_macro entry for %s\n", argv[1]); > + kdb_macro = save_kdb_macro; > return KDB_NOTIMP; > } > > @@ -781,14 +779,14 @@ static int kdb_defcmd(int argc, const char **argv) > static int kdb_exec_defcmd(int argc, const char **argv) > { > int i, ret; > - struct defcmd_set *s; > + struct kdb_macro_t *s; > if (argc != 0) > return KDB_ARGCOUNT; > - for (s = defcmd_set, i = 0; i < defcmd_set_count; ++i, ++s) { > - if (strcmp(s->name, argv[0]) == 0) > + for (s = kdb_macro, i = 0; i < kdb_macro_count; ++i, ++s) { > + if (strcmp(s->cmd.cmd_name, argv[0]) == 0) > break; > } > - if (i == defcmd_set_count) { > + if (i == kdb_macro_count) { > kdb_printf("kdb_exec_defcmd: could not find commands for %s\n", > argv[0]); > return KDB_NOTIMP; > @@ -797,7 +795,7 @@ static int kdb_exec_defcmd(int argc, const char **argv) > /* Recursive use of kdb_parse, do not use argv after > * this point */ > argv = NULL; > - kdb_printf("[%s]kdb> %s\n", s->name, s->command[i]); > + kdb_printf("[%s]kdb> %s\n", s->cmd.cmd_name, s->command[i]); > ret = kdb_parse(s->command[i]); > if (ret) > return ret; > @@ -2620,55 +2618,6 @@ static int kdb_grep_help(int argc, const char **argv) > return 0; > } > > -/* > - * kdb_register_flags - This function is used to register a kernel > - * debugger command. > - * Inputs: > - * cmd Command name > - * func Function to execute the command > - * usage A simple usage string showing arguments > - * help A simple help string describing command > - * repeat Does the command auto repeat on enter? > - * Returns: > - * zero for success, one if a duplicate command. > - */ > -int kdb_register_flags(char *cmd, > - kdb_func_t func, > - char *usage, > - char *help, > - short minlen, > - kdb_cmdflags_t flags) > -{ > - kdbtab_t *kp; > - > - 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; > - } > - } > - > - kp = kmalloc(sizeof(*kp), GFP_KDB); > - if (!kp) { > - kdb_printf("Could not allocate new kdb_command table\n"); > - return 1; > - } > - > - kp->cmd_name = cmd; > - kp->cmd_func = func; > - kp->cmd_usage = usage; > - 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. > @@ -2684,54 +2633,37 @@ void kdb_register_table(kdbtab_t *kp, size_t len) > } > > /* > - * kdb_register - Compatibility register function for commands that do > - * not need to specify a repeat state. Equivalent to > - * kdb_register_flags with flags set to 0. > - * Inputs: > - * cmd Command name > - * func Function to execute the command > - * usage A simple usage string showing arguments > - * help A simple help string describing command > - * Returns: > - * zero for success, one if a duplicate command. > + * kdb_register() - This function is used to register a kernel debugger > + * command. > + * @cmd: pointer to kdb command > */ > -int kdb_register(char *cmd, > - kdb_func_t func, > - char *usage, > - char *help, > - short minlen) > -{ > - return kdb_register_flags(cmd, func, usage, help, minlen, 0); > -} > -EXPORT_SYMBOL_GPL(kdb_register); > - > -/* > - * kdb_unregister - This function is used to unregister a kernel > - * debugger command. It is generally called when a module which > - * implements kdb commands is unloaded. > - * Inputs: > - * cmd Command name > - * Returns: > - * zero for success, one command not registered. > - */ > -int kdb_unregister(char *cmd) > +int kdb_register(kdbtab_t *cmd) > { > kdbtab_t *kp; > > - /* > - * find the command. > - */ > 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; > + if (strcmp(kp->cmd_name, cmd->cmd_name) == 0) { > + kdb_printf("Duplicate kdb cmd: %s, func %p help %s\n", > + cmd->cmd_name, cmd->cmd_func, cmd->cmd_help); > + return 1; > } > } > > - /* Couldn't find it. */ > - return 1; > + list_add_tail(&cmd->list_node, &kdb_cmds_head); > + return 0; > +} > +EXPORT_SYMBOL_GPL(kdb_register); > + > +/* > + * kdb_unregister() - This function is used to unregister a kernel debugger > + * command. It is generally called when a module which > + * implements kdb command is unloaded. > + * @cmd: pointer to kdb command > + */ > +int kdb_unregister(kdbtab_t *cmd) > +{ > + list_del(&cmd->list_node); > + return 0; > } > EXPORT_SYMBOL_GPL(kdb_unregister); > > diff --git a/kernel/debug/kdb/kdb_private.h b/kernel/debug/kdb/kdb_private.h > index 95d2b730af4d..608d57f3c419 100644 > --- a/kernel/debug/kdb/kdb_private.h > +++ b/kernel/debug/kdb/kdb_private.h > @@ -164,19 +164,6 @@ typedef struct _kdb_bp { > #ifdef CONFIG_KGDB_KDB > extern kdb_bp_t kdb_breakpoints[/* KDB_MAXBPT */]; > > -/* The KDB shell command table */ > -typedef struct _kdbtab { > - char *cmd_name; /* Command name */ > - kdb_func_t cmd_func; /* Function to execute command */ > - char *cmd_usage; /* Usage String for this command */ > - char *cmd_help; /* Help message for this command */ > - 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 */ > > diff --git a/kernel/trace/trace_kdb.c b/kernel/trace/trace_kdb.c > index 9da76104f7a2..6c4f92c79e43 100644 > --- a/kernel/trace/trace_kdb.c > +++ b/kernel/trace/trace_kdb.c > @@ -147,11 +147,17 @@ static int kdb_ftdump(int argc, const char **argv) > return 0; > } > > +static kdbtab_t ftdump_cmd = { > + .cmd_name = "ftdump", > + .cmd_func = kdb_ftdump, > + .cmd_usage = "[skip_#entries] [cpu]", > + .cmd_help = "Dump ftrace log; -skip dumps last #entries", > + .cmd_flags = KDB_ENABLE_ALWAYS_SAFE, > +}; > + > static __init int kdb_ftrace_register(void) > { > - kdb_register_flags("ftdump", kdb_ftdump, "[skip_#entries] [cpu]", > - "Dump ftrace log; -skip dumps last #entries", 0, > - KDB_ENABLE_ALWAYS_SAFE); > + kdb_register(&ftdump_cmd); > return 0; > } > > diff --git a/samples/kdb/kdb_hello.c b/samples/kdb/kdb_hello.c > index c1c2fa0f62c2..9ad514a6648b 100644 > --- a/samples/kdb/kdb_hello.c > +++ b/samples/kdb/kdb_hello.c > @@ -28,28 +28,26 @@ static int kdb_hello_cmd(int argc, const char **argv) > return 0; > } > > +static kdbtab_t hello_cmd = { > + .cmd_name = "hello", > + .cmd_func = kdb_hello_cmd, > + .cmd_usage = "[string]", > + .cmd_help = "Say Hello World or Hello [string]", > +}; > > static int __init kdb_hello_cmd_init(void) > { > /* > * Registration of a dynamically added kdb command is done with > - * kdb_register() with the arguments being: > - * 1: The name of the shell command > - * 2: The function that processes the command > - * 3: Description of the usage of any arguments > - * 4: Descriptive text when you run help > - * 5: Number of characters to complete the command > - * 0 == type the whole command > - * 1 == match both "g" and "go" for example > + * kdb_register(). > */ > - kdb_register("hello", kdb_hello_cmd, "[string]", > - "Say Hello World or Hello [string]", 0); > + kdb_register(&hello_cmd); > return 0; > } > > static void __exit kdb_hello_cmd_exit(void) > { > - kdb_unregister("hello"); > + kdb_unregister(&hello_cmd); > } > > module_init(kdb_hello_cmd_init); > -- > 2.25.1 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3 1/2] kdb: Get rid of redundant kdb_register_flags() 2021-05-13 11:28 ` [PATCH v3 1/2] kdb: Get rid of redundant kdb_register_flags() Sumit Garg 2021-05-13 11:31 ` Sumit Garg @ 2021-05-13 13:19 ` Steven Rostedt 2021-05-20 17:20 ` Doug Anderson 2 siblings, 0 replies; 8+ messages in thread From: Steven Rostedt @ 2021-05-13 13:19 UTC (permalink / raw) To: Sumit Garg Cc: kgdb-bugreport, daniel.thompson, jason.wessel, dianders, mingo, linux-kernel On Thu, 13 May 2021 16:58:41 +0530 Sumit Garg <sumit.garg@linaro.org> wrote: > Commit e4f291b3f7bb ("kdb: Simplify kdb commands registration") > allowed registration of pre-allocated kdb commands with pointer to > struct kdbtab_t. Lets switch other users as well to register pre- > allocated kdb commands via: > - Changing prototype for kdb_register() to pass a pointer to struct > kdbtab_t instead. > - Embed kdbtab_t structure in defcmd_set rather than individual params. > So while at it rename struct defcmd_set to struct kdb_macro_t as that > sounds more appropriate given its purpose. > > With these changes kdb_register_flags() becomes redundant and hence > removed. Also, since we have switched all users to register > pre-allocated commands, "is_dynamic" flag in struct kdbtab_t becomes > redundant and hence removed as well. > > Signed-off-by: Sumit Garg <sumit.garg@linaro.org> > --- > include/linux/kdb.h | 27 +++-- > kernel/debug/kdb/kdb_main.c | 206 +++++++++++---------------------- > kernel/debug/kdb/kdb_private.h | 13 --- > kernel/trace/trace_kdb.c | 12 +- For the tracing directory. Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org> -- Steve > samples/kdb/kdb_hello.c | 20 ++-- > 5 files changed, 104 insertions(+), 174 deletions(-) ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3 1/2] kdb: Get rid of redundant kdb_register_flags() 2021-05-13 11:28 ` [PATCH v3 1/2] kdb: Get rid of redundant kdb_register_flags() Sumit Garg 2021-05-13 11:31 ` Sumit Garg 2021-05-13 13:19 ` Steven Rostedt @ 2021-05-20 17:20 ` Doug Anderson 2021-06-17 11:49 ` Sumit Garg 2 siblings, 1 reply; 8+ messages in thread From: Doug Anderson @ 2021-05-20 17:20 UTC (permalink / raw) To: Sumit Garg Cc: kgdb-bugreport, Daniel Thompson, Jason Wessel, Steven Rostedt, Ingo Molnar, LKML Hi, On Thu, May 13, 2021 at 4:29 AM Sumit Garg <sumit.garg@linaro.org> wrote: > > Commit e4f291b3f7bb ("kdb: Simplify kdb commands registration") > allowed registration of pre-allocated kdb commands with pointer to > struct kdbtab_t. Lets switch other users as well to register pre- > allocated kdb commands via: > - Changing prototype for kdb_register() to pass a pointer to struct > kdbtab_t instead. > - Embed kdbtab_t structure in defcmd_set rather than individual params. > So while at it rename struct defcmd_set to struct kdb_macro_t as that > sounds more appropriate given its purpose. > > With these changes kdb_register_flags() becomes redundant and hence > removed. Also, since we have switched all users to register > pre-allocated commands, "is_dynamic" flag in struct kdbtab_t becomes > redundant and hence removed as well. > > Signed-off-by: Sumit Garg <sumit.garg@linaro.org> > --- > include/linux/kdb.h | 27 +++-- > kernel/debug/kdb/kdb_main.c | 206 +++++++++++---------------------- > kernel/debug/kdb/kdb_private.h | 13 --- > kernel/trace/trace_kdb.c | 12 +- > samples/kdb/kdb_hello.c | 20 ++-- > 5 files changed, 104 insertions(+), 174 deletions(-) > > diff --git a/include/linux/kdb.h b/include/linux/kdb.h > index 0125a677b67f..9e893279b2ea 100644 > --- a/include/linux/kdb.h > +++ b/include/linux/kdb.h > @@ -13,6 +13,8 @@ > * Copyright (C) 2009 Jason Wessel <jason.wessel@windriver.com> > */ > > +#include <linux/list.h> > + > /* Shifted versions of the command enable bits are be used if the command > * has no arguments (see kdb_check_flags). This allows commands, such as > * go, to have different permissions depending upon whether it is called > @@ -64,6 +66,17 @@ typedef enum { > > typedef int (*kdb_func_t)(int, const char **); > > +/* The KDB shell command table */ > +typedef struct _kdbtab { > + char *cmd_name; /* Command name */ > + kdb_func_t cmd_func; /* Function to execute command */ > + char *cmd_usage; /* Usage String for this command */ > + char *cmd_help; /* Help message for this command */ > + short cmd_minlen; /* Minimum legal # cmd chars required */ > + kdb_cmdflags_t cmd_flags; /* Command behaviour flags */ > + struct list_head list_node; /* Command list */ > +} kdbtab_t; IMO prefixing all of the members of the structure with "cmd_" just makes the code that references them more awkward. For instance, when I read: s->cmd.cmd_name I wonder why it can't just be: s->cmd.name > + > #ifdef CONFIG_KGDB_KDB > #include <linux/init.h> > #include <linux/sched.h> > @@ -193,19 +206,13 @@ static inline const char *kdb_walk_kallsyms(loff_t *pos) > #endif /* ! CONFIG_KALLSYMS */ > > /* Dynamic kdb shell command registration */ > -extern int kdb_register(char *, kdb_func_t, char *, char *, short); > -extern int kdb_register_flags(char *, kdb_func_t, char *, char *, > - short, kdb_cmdflags_t); > -extern int kdb_unregister(char *); > +extern int kdb_register(kdbtab_t *cmd); > +extern int kdb_unregister(kdbtab_t *cmd); I suggest changing kdb_unregister() to return "void". It can no longer fail and generally we can't really do anything if unregister calls fail anyway. > #else /* ! CONFIG_KGDB_KDB */ > static inline __printf(1, 2) int kdb_printf(const char *fmt, ...) { return 0; } > static inline void kdb_init(int level) {} > -static inline int kdb_register(char *cmd, kdb_func_t func, char *usage, > - char *help, short minlen) { return 0; } > -static inline int kdb_register_flags(char *cmd, kdb_func_t func, char *usage, > - char *help, short minlen, > - kdb_cmdflags_t flags) { return 0; } > -static inline int kdb_unregister(char *cmd) { return 0; } > +static inline int kdb_register(kdbtab_t *cmd) { return 0; } > +static inline int kdb_unregister(kdbtab_t *cmd) { return 0; } > #endif /* CONFIG_KGDB_KDB */ > enum { > KDB_NOT_INITIALIZED, > diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c > index 1baa96a2ecb8..de685b2a8ce7 100644 > --- a/kernel/debug/kdb/kdb_main.c > +++ b/kernel/debug/kdb/kdb_main.c > @@ -33,7 +33,6 @@ > #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> > @@ -654,16 +653,14 @@ static void kdb_cmderror(int diag) > * Returns: > * zero for success, a kdb diagnostic if error > */ > -struct defcmd_set { > +struct kdb_macro_t { > int count; > bool usable; > - char *name; > - char *usage; > - char *help; > + kdbtab_t cmd; > char **command; > }; > -static struct defcmd_set *defcmd_set; > -static int defcmd_set_count; > +static struct kdb_macro_t *kdb_macro; > +static int kdb_macro_count; It would have made my review job easier if the rename from "defcmd_set" to "kdb_macro" was in a separate no-op change... > static bool defcmd_in_progress; > > /* Forward references */ > @@ -671,20 +668,14 @@ static int kdb_exec_defcmd(int argc, const char **argv); > > static int kdb_defcmd2(const char *cmdstr, const char *argv0) > { > - struct defcmd_set *s = defcmd_set + defcmd_set_count - 1; > + struct kdb_macro_t *s = kdb_macro + kdb_macro_count - 1; > char **save_command = s->command; > if (strcmp(argv0, "endefcmd") == 0) { > defcmd_in_progress = false; > if (!s->count) > s->usable = false; > if (s->usable) > - /* macros are always safe because when executed each > - * internal command re-enters kdb_parse() and is > - * safety checked individually. > - */ You dropped this comment. Should it instead move into kdb_defcmd() where you set the KDB_ENABLE_ALWAYS_SAFE flag? > - kdb_register_flags(s->name, kdb_exec_defcmd, s->usage, > - s->help, 0, > - KDB_ENABLE_ALWAYS_SAFE); > + kdb_register(&s->cmd); > return 0; > } > if (!s->usable) > @@ -704,7 +695,9 @@ static int kdb_defcmd2(const char *cmdstr, const char *argv0) > > static int kdb_defcmd(int argc, const char **argv) > { > - struct defcmd_set *save_defcmd_set = defcmd_set, *s; > + struct kdb_macro_t *save_kdb_macro = kdb_macro, *s; > + kdbtab_t *mp; > + > if (defcmd_in_progress) { > kdb_printf("kdb: nested defcmd detected, assuming missing " > "endefcmd\n"); > @@ -712,9 +705,9 @@ static int kdb_defcmd(int argc, const char **argv) > } > if (argc == 0) { > int i; > - for (s = defcmd_set; s < defcmd_set + defcmd_set_count; ++s) { > - kdb_printf("defcmd %s \"%s\" \"%s\"\n", s->name, > - s->usage, s->help); > + for (s = kdb_macro; s < kdb_macro + kdb_macro_count; ++s) { > + kdb_printf("defcmd %s \"%s\" \"%s\"\n", s->cmd.cmd_name, > + s->cmd.cmd_usage, s->cmd.cmd_help); > for (i = 0; i < s->count; ++i) > kdb_printf("%s", s->command[i]); > kdb_printf("endefcmd\n"); > @@ -727,45 +720,50 @@ static int kdb_defcmd(int argc, const char **argv) > kdb_printf("Command only available during kdb_init()\n"); > return KDB_NOTIMP; > } > - defcmd_set = kmalloc_array(defcmd_set_count + 1, sizeof(*defcmd_set), > + kdb_macro = kmalloc_array(kdb_macro_count + 1, sizeof(*kdb_macro), > GFP_KDB); Looking at the code that follows makes me think we should have been using krealloc_array(), but I guess we don't need to bother since the next patch changes all of it. ;-) > - if (!defcmd_set) > + if (!kdb_macro) > goto fail_defcmd; > - memcpy(defcmd_set, save_defcmd_set, > - defcmd_set_count * sizeof(*defcmd_set)); > - s = defcmd_set + defcmd_set_count; > + memcpy(kdb_macro, save_kdb_macro, > + kdb_macro_count * sizeof(*kdb_macro)); > + s = kdb_macro + kdb_macro_count; > memset(s, 0, sizeof(*s)); > s->usable = true; > - s->name = kdb_strdup(argv[1], GFP_KDB); > - if (!s->name) > + > + mp = &s->cmd; > + mp->cmd_func = kdb_exec_defcmd; > + mp->cmd_minlen = 0; > + mp->cmd_flags = KDB_ENABLE_ALWAYS_SAFE; > + mp->cmd_name = kdb_strdup(argv[1], GFP_KDB); > + if (!mp->cmd_name) > goto fail_name; > - s->usage = kdb_strdup(argv[2], GFP_KDB); > - if (!s->usage) > + mp->cmd_usage = kdb_strdup(argv[2], GFP_KDB); > + if (!mp->cmd_usage) > goto fail_usage; > - s->help = kdb_strdup(argv[3], GFP_KDB); > - if (!s->help) > + mp->cmd_help = kdb_strdup(argv[3], GFP_KDB); > + if (!mp->cmd_help) > goto fail_help; > - if (s->usage[0] == '"') { > - strcpy(s->usage, argv[2]+1); > - s->usage[strlen(s->usage)-1] = '\0'; > + if (mp->cmd_usage[0] == '"') { > + strcpy(mp->cmd_usage, argv[2]+1); > + mp->cmd_usage[strlen(mp->cmd_usage)-1] = '\0'; > } > - if (s->help[0] == '"') { > - strcpy(s->help, argv[3]+1); > - s->help[strlen(s->help)-1] = '\0'; > + if (mp->cmd_help[0] == '"') { > + strcpy(mp->cmd_help, argv[3]+1); > + mp->cmd_help[strlen(mp->cmd_help)-1] = '\0'; > } > - ++defcmd_set_count; > + ++kdb_macro_count; > defcmd_in_progress = true; > - kfree(save_defcmd_set); > + kfree(save_kdb_macro); > return 0; > fail_help: > - kfree(s->usage); > + kfree(mp->cmd_usage); > fail_usage: > - kfree(s->name); > + kfree(mp->cmd_name); > fail_name: > - kfree(defcmd_set); > + kfree(kdb_macro); > fail_defcmd: > - kdb_printf("Could not allocate new defcmd_set entry for %s\n", argv[1]); > - defcmd_set = save_defcmd_set; > + kdb_printf("Could not allocate new kdb_macro entry for %s\n", argv[1]); > + kdb_macro = save_kdb_macro; > return KDB_NOTIMP; > } > > @@ -781,14 +779,14 @@ static int kdb_defcmd(int argc, const char **argv) > static int kdb_exec_defcmd(int argc, const char **argv) > { > int i, ret; > - struct defcmd_set *s; > + struct kdb_macro_t *s; > if (argc != 0) > return KDB_ARGCOUNT; > - for (s = defcmd_set, i = 0; i < defcmd_set_count; ++i, ++s) { > - if (strcmp(s->name, argv[0]) == 0) > + for (s = kdb_macro, i = 0; i < kdb_macro_count; ++i, ++s) { > + if (strcmp(s->cmd.cmd_name, argv[0]) == 0) > break; > } > - if (i == defcmd_set_count) { > + if (i == kdb_macro_count) { > kdb_printf("kdb_exec_defcmd: could not find commands for %s\n", > argv[0]); > return KDB_NOTIMP; > @@ -797,7 +795,7 @@ static int kdb_exec_defcmd(int argc, const char **argv) > /* Recursive use of kdb_parse, do not use argv after > * this point */ > argv = NULL; > - kdb_printf("[%s]kdb> %s\n", s->name, s->command[i]); > + kdb_printf("[%s]kdb> %s\n", s->cmd.cmd_name, s->command[i]); > ret = kdb_parse(s->command[i]); > if (ret) > return ret; > @@ -2620,55 +2618,6 @@ static int kdb_grep_help(int argc, const char **argv) > return 0; > } > > -/* > - * kdb_register_flags - This function is used to register a kernel > - * debugger command. > - * Inputs: > - * cmd Command name > - * func Function to execute the command > - * usage A simple usage string showing arguments > - * help A simple help string describing command > - * repeat Does the command auto repeat on enter? > - * Returns: > - * zero for success, one if a duplicate command. > - */ > -int kdb_register_flags(char *cmd, > - kdb_func_t func, > - char *usage, > - char *help, > - short minlen, > - kdb_cmdflags_t flags) > -{ > - kdbtab_t *kp; > - > - 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; > - } > - } > - > - kp = kmalloc(sizeof(*kp), GFP_KDB); > - if (!kp) { > - kdb_printf("Could not allocate new kdb_command table\n"); > - return 1; > - } > - > - kp->cmd_name = cmd; > - kp->cmd_func = func; > - kp->cmd_usage = usage; > - 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. > @@ -2684,54 +2633,37 @@ void kdb_register_table(kdbtab_t *kp, size_t len) > } > > /* You intended to make this kernel doc style, right? So "/**" here? > - * kdb_register - Compatibility register function for commands that do > - * not need to specify a repeat state. Equivalent to > - * kdb_register_flags with flags set to 0. > - * Inputs: > - * cmd Command name > - * func Function to execute the command > - * usage A simple usage string showing arguments > - * help A simple help string describing command > - * Returns: > - * zero for success, one if a duplicate command. > + * kdb_register() - This function is used to register a kernel debugger > + * command. > + * @cmd: pointer to kdb command You should document that it's the job of the caller to keep the memory for the cmd allocated until unregister is called. I'll also note that your diffs would be slightly easier to understand if you moved your new kdb_register() to before kdb_register_table(). That's because the new kdb_register() is more similar to the old kdb_register_flags(). > */ > -int kdb_register(char *cmd, > - kdb_func_t func, > - char *usage, > - char *help, > - short minlen) > -{ > - return kdb_register_flags(cmd, func, usage, help, minlen, 0); > -} > -EXPORT_SYMBOL_GPL(kdb_register); > - > -/* > - * kdb_unregister - This function is used to unregister a kernel > - * debugger command. It is generally called when a module which > - * implements kdb commands is unloaded. > - * Inputs: > - * cmd Command name > - * Returns: > - * zero for success, one command not registered. > - */ > -int kdb_unregister(char *cmd) > +int kdb_register(kdbtab_t *cmd) > { > kdbtab_t *kp; > > - /* > - * find the command. > - */ > 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; > + if (strcmp(kp->cmd_name, cmd->cmd_name) == 0) { > + kdb_printf("Duplicate kdb cmd: %s, func %p help %s\n", > + cmd->cmd_name, cmd->cmd_func, cmd->cmd_help); > + return 1; > } > } > > - /* Couldn't find it. */ > - return 1; > + list_add_tail(&cmd->list_node, &kdb_cmds_head); > + return 0; > +} > +EXPORT_SYMBOL_GPL(kdb_register); > + > +/* You intended to make this kernel doc style, right? So "/**" here? -Doug ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3 1/2] kdb: Get rid of redundant kdb_register_flags() 2021-05-20 17:20 ` Doug Anderson @ 2021-06-17 11:49 ` Sumit Garg 0 siblings, 0 replies; 8+ messages in thread From: Sumit Garg @ 2021-06-17 11:49 UTC (permalink / raw) To: Doug Anderson Cc: kgdb-bugreport, Daniel Thompson, Jason Wessel, Steven Rostedt, Ingo Molnar, LKML Hi Doug, Thanks for your comments on this series and apologies for my delayed reply as I was busy with other high priority stuff. So I will incorporate your comments in the next version. -Sumit On Thu, 20 May 2021 at 22:51, Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Thu, May 13, 2021 at 4:29 AM Sumit Garg <sumit.garg@linaro.org> wrote: > > > > Commit e4f291b3f7bb ("kdb: Simplify kdb commands registration") > > allowed registration of pre-allocated kdb commands with pointer to > > struct kdbtab_t. Lets switch other users as well to register pre- > > allocated kdb commands via: > > - Changing prototype for kdb_register() to pass a pointer to struct > > kdbtab_t instead. > > - Embed kdbtab_t structure in defcmd_set rather than individual params. > > So while at it rename struct defcmd_set to struct kdb_macro_t as that > > sounds more appropriate given its purpose. > > > > With these changes kdb_register_flags() becomes redundant and hence > > removed. Also, since we have switched all users to register > > pre-allocated commands, "is_dynamic" flag in struct kdbtab_t becomes > > redundant and hence removed as well. > > > > Signed-off-by: Sumit Garg <sumit.garg@linaro.org> > > --- > > include/linux/kdb.h | 27 +++-- > > kernel/debug/kdb/kdb_main.c | 206 +++++++++++---------------------- > > kernel/debug/kdb/kdb_private.h | 13 --- > > kernel/trace/trace_kdb.c | 12 +- > > samples/kdb/kdb_hello.c | 20 ++-- > > 5 files changed, 104 insertions(+), 174 deletions(-) > > > > diff --git a/include/linux/kdb.h b/include/linux/kdb.h > > index 0125a677b67f..9e893279b2ea 100644 > > --- a/include/linux/kdb.h > > +++ b/include/linux/kdb.h > > @@ -13,6 +13,8 @@ > > * Copyright (C) 2009 Jason Wessel <jason.wessel@windriver.com> > > */ > > > > +#include <linux/list.h> > > + > > /* Shifted versions of the command enable bits are be used if the command > > * has no arguments (see kdb_check_flags). This allows commands, such as > > * go, to have different permissions depending upon whether it is called > > @@ -64,6 +66,17 @@ typedef enum { > > > > typedef int (*kdb_func_t)(int, const char **); > > > > +/* The KDB shell command table */ > > +typedef struct _kdbtab { > > + char *cmd_name; /* Command name */ > > + kdb_func_t cmd_func; /* Function to execute command */ > > + char *cmd_usage; /* Usage String for this command */ > > + char *cmd_help; /* Help message for this command */ > > + short cmd_minlen; /* Minimum legal # cmd chars required */ > > + kdb_cmdflags_t cmd_flags; /* Command behaviour flags */ > > + struct list_head list_node; /* Command list */ > > +} kdbtab_t; > > IMO prefixing all of the members of the structure with "cmd_" just > makes the code that references them more awkward. For instance, when I > read: > > s->cmd.cmd_name > > I wonder why it can't just be: > > s->cmd.name > > > > + > > #ifdef CONFIG_KGDB_KDB > > #include <linux/init.h> > > #include <linux/sched.h> > > @@ -193,19 +206,13 @@ static inline const char *kdb_walk_kallsyms(loff_t *pos) > > #endif /* ! CONFIG_KALLSYMS */ > > > > /* Dynamic kdb shell command registration */ > > -extern int kdb_register(char *, kdb_func_t, char *, char *, short); > > -extern int kdb_register_flags(char *, kdb_func_t, char *, char *, > > - short, kdb_cmdflags_t); > > -extern int kdb_unregister(char *); > > +extern int kdb_register(kdbtab_t *cmd); > > +extern int kdb_unregister(kdbtab_t *cmd); > > I suggest changing kdb_unregister() to return "void". It can no longer > fail and generally we can't really do anything if unregister calls > fail anyway. > > > > #else /* ! CONFIG_KGDB_KDB */ > > static inline __printf(1, 2) int kdb_printf(const char *fmt, ...) { return 0; } > > static inline void kdb_init(int level) {} > > -static inline int kdb_register(char *cmd, kdb_func_t func, char *usage, > > - char *help, short minlen) { return 0; } > > -static inline int kdb_register_flags(char *cmd, kdb_func_t func, char *usage, > > - char *help, short minlen, > > - kdb_cmdflags_t flags) { return 0; } > > -static inline int kdb_unregister(char *cmd) { return 0; } > > +static inline int kdb_register(kdbtab_t *cmd) { return 0; } > > +static inline int kdb_unregister(kdbtab_t *cmd) { return 0; } > > #endif /* CONFIG_KGDB_KDB */ > > enum { > > KDB_NOT_INITIALIZED, > > diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c > > index 1baa96a2ecb8..de685b2a8ce7 100644 > > --- a/kernel/debug/kdb/kdb_main.c > > +++ b/kernel/debug/kdb/kdb_main.c > > @@ -33,7 +33,6 @@ > > #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> > > @@ -654,16 +653,14 @@ static void kdb_cmderror(int diag) > > * Returns: > > * zero for success, a kdb diagnostic if error > > */ > > -struct defcmd_set { > > +struct kdb_macro_t { > > int count; > > bool usable; > > - char *name; > > - char *usage; > > - char *help; > > + kdbtab_t cmd; > > char **command; > > }; > > -static struct defcmd_set *defcmd_set; > > -static int defcmd_set_count; > > +static struct kdb_macro_t *kdb_macro; > > +static int kdb_macro_count; > > It would have made my review job easier if the rename from > "defcmd_set" to "kdb_macro" was in a separate no-op change... > > > > static bool defcmd_in_progress; > > > > /* Forward references */ > > @@ -671,20 +668,14 @@ static int kdb_exec_defcmd(int argc, const char **argv); > > > > static int kdb_defcmd2(const char *cmdstr, const char *argv0) > > { > > - struct defcmd_set *s = defcmd_set + defcmd_set_count - 1; > > + struct kdb_macro_t *s = kdb_macro + kdb_macro_count - 1; > > char **save_command = s->command; > > if (strcmp(argv0, "endefcmd") == 0) { > > defcmd_in_progress = false; > > if (!s->count) > > s->usable = false; > > if (s->usable) > > - /* macros are always safe because when executed each > > - * internal command re-enters kdb_parse() and is > > - * safety checked individually. > > - */ > > You dropped this comment. Should it instead move into kdb_defcmd() > where you set the KDB_ENABLE_ALWAYS_SAFE flag? > > > > - kdb_register_flags(s->name, kdb_exec_defcmd, s->usage, > > - s->help, 0, > > - KDB_ENABLE_ALWAYS_SAFE); > > + kdb_register(&s->cmd); > > return 0; > > } > > if (!s->usable) > > @@ -704,7 +695,9 @@ static int kdb_defcmd2(const char *cmdstr, const char *argv0) > > > > static int kdb_defcmd(int argc, const char **argv) > > { > > - struct defcmd_set *save_defcmd_set = defcmd_set, *s; > > + struct kdb_macro_t *save_kdb_macro = kdb_macro, *s; > > + kdbtab_t *mp; > > + > > if (defcmd_in_progress) { > > kdb_printf("kdb: nested defcmd detected, assuming missing " > > "endefcmd\n"); > > @@ -712,9 +705,9 @@ static int kdb_defcmd(int argc, const char **argv) > > } > > if (argc == 0) { > > int i; > > - for (s = defcmd_set; s < defcmd_set + defcmd_set_count; ++s) { > > - kdb_printf("defcmd %s \"%s\" \"%s\"\n", s->name, > > - s->usage, s->help); > > + for (s = kdb_macro; s < kdb_macro + kdb_macro_count; ++s) { > > + kdb_printf("defcmd %s \"%s\" \"%s\"\n", s->cmd.cmd_name, > > + s->cmd.cmd_usage, s->cmd.cmd_help); > > for (i = 0; i < s->count; ++i) > > kdb_printf("%s", s->command[i]); > > kdb_printf("endefcmd\n"); > > @@ -727,45 +720,50 @@ static int kdb_defcmd(int argc, const char **argv) > > kdb_printf("Command only available during kdb_init()\n"); > > return KDB_NOTIMP; > > } > > - defcmd_set = kmalloc_array(defcmd_set_count + 1, sizeof(*defcmd_set), > > + kdb_macro = kmalloc_array(kdb_macro_count + 1, sizeof(*kdb_macro), > > GFP_KDB); > > Looking at the code that follows makes me think we should have been > using krealloc_array(), but I guess we don't need to bother since the > next patch changes all of it. ;-) > > > > - if (!defcmd_set) > > + if (!kdb_macro) > > goto fail_defcmd; > > - memcpy(defcmd_set, save_defcmd_set, > > - defcmd_set_count * sizeof(*defcmd_set)); > > - s = defcmd_set + defcmd_set_count; > > + memcpy(kdb_macro, save_kdb_macro, > > + kdb_macro_count * sizeof(*kdb_macro)); > > + s = kdb_macro + kdb_macro_count; > > memset(s, 0, sizeof(*s)); > > s->usable = true; > > - s->name = kdb_strdup(argv[1], GFP_KDB); > > - if (!s->name) > > + > > + mp = &s->cmd; > > + mp->cmd_func = kdb_exec_defcmd; > > + mp->cmd_minlen = 0; > > + mp->cmd_flags = KDB_ENABLE_ALWAYS_SAFE; > > + mp->cmd_name = kdb_strdup(argv[1], GFP_KDB); > > + if (!mp->cmd_name) > > goto fail_name; > > - s->usage = kdb_strdup(argv[2], GFP_KDB); > > - if (!s->usage) > > + mp->cmd_usage = kdb_strdup(argv[2], GFP_KDB); > > + if (!mp->cmd_usage) > > goto fail_usage; > > - s->help = kdb_strdup(argv[3], GFP_KDB); > > - if (!s->help) > > + mp->cmd_help = kdb_strdup(argv[3], GFP_KDB); > > + if (!mp->cmd_help) > > goto fail_help; > > - if (s->usage[0] == '"') { > > - strcpy(s->usage, argv[2]+1); > > - s->usage[strlen(s->usage)-1] = '\0'; > > + if (mp->cmd_usage[0] == '"') { > > + strcpy(mp->cmd_usage, argv[2]+1); > > + mp->cmd_usage[strlen(mp->cmd_usage)-1] = '\0'; > > } > > - if (s->help[0] == '"') { > > - strcpy(s->help, argv[3]+1); > > - s->help[strlen(s->help)-1] = '\0'; > > + if (mp->cmd_help[0] == '"') { > > + strcpy(mp->cmd_help, argv[3]+1); > > + mp->cmd_help[strlen(mp->cmd_help)-1] = '\0'; > > } > > - ++defcmd_set_count; > > + ++kdb_macro_count; > > defcmd_in_progress = true; > > - kfree(save_defcmd_set); > > + kfree(save_kdb_macro); > > return 0; > > fail_help: > > - kfree(s->usage); > > + kfree(mp->cmd_usage); > > fail_usage: > > - kfree(s->name); > > + kfree(mp->cmd_name); > > fail_name: > > - kfree(defcmd_set); > > + kfree(kdb_macro); > > fail_defcmd: > > - kdb_printf("Could not allocate new defcmd_set entry for %s\n", argv[1]); > > - defcmd_set = save_defcmd_set; > > + kdb_printf("Could not allocate new kdb_macro entry for %s\n", argv[1]); > > + kdb_macro = save_kdb_macro; > > return KDB_NOTIMP; > > } > > > > @@ -781,14 +779,14 @@ static int kdb_defcmd(int argc, const char **argv) > > static int kdb_exec_defcmd(int argc, const char **argv) > > { > > int i, ret; > > - struct defcmd_set *s; > > + struct kdb_macro_t *s; > > if (argc != 0) > > return KDB_ARGCOUNT; > > - for (s = defcmd_set, i = 0; i < defcmd_set_count; ++i, ++s) { > > - if (strcmp(s->name, argv[0]) == 0) > > + for (s = kdb_macro, i = 0; i < kdb_macro_count; ++i, ++s) { > > + if (strcmp(s->cmd.cmd_name, argv[0]) == 0) > > break; > > } > > - if (i == defcmd_set_count) { > > + if (i == kdb_macro_count) { > > kdb_printf("kdb_exec_defcmd: could not find commands for %s\n", > > argv[0]); > > return KDB_NOTIMP; > > @@ -797,7 +795,7 @@ static int kdb_exec_defcmd(int argc, const char **argv) > > /* Recursive use of kdb_parse, do not use argv after > > * this point */ > > argv = NULL; > > - kdb_printf("[%s]kdb> %s\n", s->name, s->command[i]); > > + kdb_printf("[%s]kdb> %s\n", s->cmd.cmd_name, s->command[i]); > > ret = kdb_parse(s->command[i]); > > if (ret) > > return ret; > > @@ -2620,55 +2618,6 @@ static int kdb_grep_help(int argc, const char **argv) > > return 0; > > } > > > > -/* > > - * kdb_register_flags - This function is used to register a kernel > > - * debugger command. > > - * Inputs: > > - * cmd Command name > > - * func Function to execute the command > > - * usage A simple usage string showing arguments > > - * help A simple help string describing command > > - * repeat Does the command auto repeat on enter? > > - * Returns: > > - * zero for success, one if a duplicate command. > > - */ > > -int kdb_register_flags(char *cmd, > > - kdb_func_t func, > > - char *usage, > > - char *help, > > - short minlen, > > - kdb_cmdflags_t flags) > > -{ > > - kdbtab_t *kp; > > - > > - 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; > > - } > > - } > > - > > - kp = kmalloc(sizeof(*kp), GFP_KDB); > > - if (!kp) { > > - kdb_printf("Could not allocate new kdb_command table\n"); > > - return 1; > > - } > > - > > - kp->cmd_name = cmd; > > - kp->cmd_func = func; > > - kp->cmd_usage = usage; > > - 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. > > @@ -2684,54 +2633,37 @@ void kdb_register_table(kdbtab_t *kp, size_t len) > > } > > > > /* > > You intended to make this kernel doc style, right? So "/**" here? > > > > - * kdb_register - Compatibility register function for commands that do > > - * not need to specify a repeat state. Equivalent to > > - * kdb_register_flags with flags set to 0. > > - * Inputs: > > - * cmd Command name > > - * func Function to execute the command > > - * usage A simple usage string showing arguments > > - * help A simple help string describing command > > - * Returns: > > - * zero for success, one if a duplicate command. > > + * kdb_register() - This function is used to register a kernel debugger > > + * command. > > + * @cmd: pointer to kdb command > > You should document that it's the job of the caller to keep the memory > for the cmd allocated until unregister is called. > > I'll also note that your diffs would be slightly easier to understand > if you moved your new kdb_register() to before kdb_register_table(). > That's because the new kdb_register() is more similar to the old > kdb_register_flags(). > > > > */ > > -int kdb_register(char *cmd, > > - kdb_func_t func, > > - char *usage, > > - char *help, > > - short minlen) > > -{ > > - return kdb_register_flags(cmd, func, usage, help, minlen, 0); > > -} > > -EXPORT_SYMBOL_GPL(kdb_register); > > - > > -/* > > - * kdb_unregister - This function is used to unregister a kernel > > - * debugger command. It is generally called when a module which > > - * implements kdb commands is unloaded. > > - * Inputs: > > - * cmd Command name > > - * Returns: > > - * zero for success, one command not registered. > > - */ > > -int kdb_unregister(char *cmd) > > +int kdb_register(kdbtab_t *cmd) > > { > > kdbtab_t *kp; > > > > - /* > > - * find the command. > > - */ > > 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; > > + if (strcmp(kp->cmd_name, cmd->cmd_name) == 0) { > > + kdb_printf("Duplicate kdb cmd: %s, func %p help %s\n", > > + cmd->cmd_name, cmd->cmd_func, cmd->cmd_help); > > + return 1; > > } > > } > > > > - /* Couldn't find it. */ > > - return 1; > > + list_add_tail(&cmd->list_node, &kdb_cmds_head); > > + return 0; > > +} > > +EXPORT_SYMBOL_GPL(kdb_register); > > + > > +/* > > You intended to make this kernel doc style, right? So "/**" here? > > > -Doug ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v3 2/2] kdb: Simplify kdb_defcmd macro logic 2021-05-13 11:28 [PATCH v3 0/2] kdb code refactoring Sumit Garg 2021-05-13 11:28 ` [PATCH v3 1/2] kdb: Get rid of redundant kdb_register_flags() Sumit Garg @ 2021-05-13 11:28 ` Sumit Garg 2021-05-20 17:21 ` Doug Anderson 1 sibling, 1 reply; 8+ messages in thread From: Sumit Garg @ 2021-05-13 11:28 UTC (permalink / raw) To: kgdb-bugreport Cc: daniel.thompson, jason.wessel, dianders, rostedt, mingo, linux-kernel, Sumit Garg Switch to use a linked list instead of dynamic array which makes allocation of kdb macro and traversing the kdb macro commands list simpler. Suggested-by: Daniel Thompson <daniel.thompson@linaro.org> Signed-off-by: Sumit Garg <sumit.garg@linaro.org> --- kernel/debug/kdb/kdb_main.c | 107 +++++++++++++++++++----------------- 1 file changed, 58 insertions(+), 49 deletions(-) diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c index de685b2a8ce7..ce7f4c71992d 100644 --- a/kernel/debug/kdb/kdb_main.c +++ b/kernel/debug/kdb/kdb_main.c @@ -654,13 +654,16 @@ static void kdb_cmderror(int diag) * zero for success, a kdb diagnostic if error */ struct kdb_macro_t { - int count; - bool usable; - kdbtab_t cmd; - char **command; + kdbtab_t cmd; /* Macro command name */ + struct list_head commands; /* Associated command list */ }; + +struct kdb_macro_cmd_t { + char *cmd; /* Command name */ + struct list_head list_node; /* Command list node */ +}; + static struct kdb_macro_t *kdb_macro; -static int kdb_macro_count; static bool defcmd_in_progress; /* Forward references */ @@ -668,34 +671,33 @@ static int kdb_exec_defcmd(int argc, const char **argv); static int kdb_defcmd2(const char *cmdstr, const char *argv0) { - struct kdb_macro_t *s = kdb_macro + kdb_macro_count - 1; - char **save_command = s->command; + struct kdb_macro_cmd_t *kmc; + + if (!kdb_macro) + return KDB_NOTIMP; + if (strcmp(argv0, "endefcmd") == 0) { defcmd_in_progress = false; - if (!s->count) - s->usable = false; - if (s->usable) - kdb_register(&s->cmd); + if (!list_empty(&kdb_macro->commands)) + kdb_register(&kdb_macro->cmd); return 0; } - if (!s->usable) - return KDB_NOTIMP; - s->command = kcalloc(s->count + 1, sizeof(*(s->command)), GFP_KDB); - if (!s->command) { - kdb_printf("Could not allocate new kdb_defcmd table for %s\n", + + kmc = kmalloc(sizeof(*kmc), GFP_KDB); + if (!kmc) { + kdb_printf("Could not allocate new kdb macro command: %s\n", cmdstr); - s->usable = false; return KDB_NOTIMP; } - memcpy(s->command, save_command, s->count * sizeof(*(s->command))); - s->command[s->count++] = kdb_strdup(cmdstr, GFP_KDB); - kfree(save_command); + + kmc->cmd = kdb_strdup(cmdstr, GFP_KDB); + list_add_tail(&kmc->list_node, &kdb_macro->commands); + return 0; } static int kdb_defcmd(int argc, const char **argv) { - struct kdb_macro_t *save_kdb_macro = kdb_macro, *s; kdbtab_t *mp; if (defcmd_in_progress) { @@ -704,13 +706,21 @@ static int kdb_defcmd(int argc, const char **argv) kdb_defcmd2("endefcmd", "endefcmd"); } if (argc == 0) { - int i; - for (s = kdb_macro; s < kdb_macro + kdb_macro_count; ++s) { - kdb_printf("defcmd %s \"%s\" \"%s\"\n", s->cmd.cmd_name, - s->cmd.cmd_usage, s->cmd.cmd_help); - for (i = 0; i < s->count; ++i) - kdb_printf("%s", s->command[i]); - kdb_printf("endefcmd\n"); + kdbtab_t *kp; + struct kdb_macro_t *kmp; + struct kdb_macro_cmd_t *kmc; + + list_for_each_entry(kp, &kdb_cmds_head, list_node) { + if (kp->cmd_func == kdb_exec_defcmd) { + kdb_printf("defcmd %s \"%s\" \"%s\"\n", + kp->cmd_name, kp->cmd_usage, + kp->cmd_help); + kmp = container_of(kp, struct kdb_macro_t, cmd); + list_for_each_entry(kmc, &kmp->commands, + list_node) + kdb_printf("%s", kmc->cmd); + kdb_printf("endefcmd\n"); + } } return 0; } @@ -720,17 +730,11 @@ static int kdb_defcmd(int argc, const char **argv) kdb_printf("Command only available during kdb_init()\n"); return KDB_NOTIMP; } - kdb_macro = kmalloc_array(kdb_macro_count + 1, sizeof(*kdb_macro), - GFP_KDB); + kdb_macro = kzalloc(sizeof(*kdb_macro), GFP_KDB); if (!kdb_macro) goto fail_defcmd; - memcpy(kdb_macro, save_kdb_macro, - kdb_macro_count * sizeof(*kdb_macro)); - s = kdb_macro + kdb_macro_count; - memset(s, 0, sizeof(*s)); - s->usable = true; - mp = &s->cmd; + mp = &kdb_macro->cmd; mp->cmd_func = kdb_exec_defcmd; mp->cmd_minlen = 0; mp->cmd_flags = KDB_ENABLE_ALWAYS_SAFE; @@ -751,9 +755,9 @@ static int kdb_defcmd(int argc, const char **argv) strcpy(mp->cmd_help, argv[3]+1); mp->cmd_help[strlen(mp->cmd_help)-1] = '\0'; } - ++kdb_macro_count; + + INIT_LIST_HEAD(&kdb_macro->commands); defcmd_in_progress = true; - kfree(save_kdb_macro); return 0; fail_help: kfree(mp->cmd_usage); @@ -763,7 +767,6 @@ static int kdb_defcmd(int argc, const char **argv) kfree(kdb_macro); fail_defcmd: kdb_printf("Could not allocate new kdb_macro entry for %s\n", argv[1]); - kdb_macro = save_kdb_macro; return KDB_NOTIMP; } @@ -778,25 +781,31 @@ static int kdb_defcmd(int argc, const char **argv) */ static int kdb_exec_defcmd(int argc, const char **argv) { - int i, ret; - struct kdb_macro_t *s; + int ret; + kdbtab_t *kp; + struct kdb_macro_t *kmp; + struct kdb_macro_cmd_t *kmc; + if (argc != 0) return KDB_ARGCOUNT; - for (s = kdb_macro, i = 0; i < kdb_macro_count; ++i, ++s) { - if (strcmp(s->cmd.cmd_name, argv[0]) == 0) + + list_for_each_entry(kp, &kdb_cmds_head, list_node) { + if (strcmp(kp->cmd_name, argv[0]) == 0) break; } - if (i == kdb_macro_count) { + if (list_entry_is_head(kp, &kdb_cmds_head, list_node)) { kdb_printf("kdb_exec_defcmd: could not find commands for %s\n", argv[0]); return KDB_NOTIMP; } - for (i = 0; i < s->count; ++i) { - /* Recursive use of kdb_parse, do not use argv after - * this point */ + kmp = container_of(kp, struct kdb_macro_t, cmd); + list_for_each_entry(kmc, &kmp->commands, list_node) { + /* + * Recursive use of kdb_parse, do not use argv after this point. + */ argv = NULL; - kdb_printf("[%s]kdb> %s\n", s->cmd.cmd_name, s->command[i]); - ret = kdb_parse(s->command[i]); + kdb_printf("[%s]kdb> %s\n", kmp->cmd.cmd_name, kmc->cmd); + ret = kdb_parse(kmc->cmd); if (ret) return ret; } -- 2.25.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v3 2/2] kdb: Simplify kdb_defcmd macro logic 2021-05-13 11:28 ` [PATCH v3 2/2] kdb: Simplify kdb_defcmd macro logic Sumit Garg @ 2021-05-20 17:21 ` Doug Anderson 0 siblings, 0 replies; 8+ messages in thread From: Doug Anderson @ 2021-05-20 17:21 UTC (permalink / raw) To: Sumit Garg Cc: kgdb-bugreport, Daniel Thompson, Jason Wessel, Steven Rostedt, Ingo Molnar, LKML Hi, On Thu, May 13, 2021 at 4:29 AM Sumit Garg <sumit.garg@linaro.org> wrote: > > Switch to use a linked list instead of dynamic array which makes > allocation of kdb macro and traversing the kdb macro commands list > simpler. > > Suggested-by: Daniel Thompson <daniel.thompson@linaro.org> > Signed-off-by: Sumit Garg <sumit.garg@linaro.org> > --- > kernel/debug/kdb/kdb_main.c | 107 +++++++++++++++++++----------------- > 1 file changed, 58 insertions(+), 49 deletions(-) > > diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c > index de685b2a8ce7..ce7f4c71992d 100644 > --- a/kernel/debug/kdb/kdb_main.c > +++ b/kernel/debug/kdb/kdb_main.c > @@ -654,13 +654,16 @@ static void kdb_cmderror(int diag) > * zero for success, a kdb diagnostic if error > */ > struct kdb_macro_t { > - int count; > - bool usable; > - kdbtab_t cmd; > - char **command; > + kdbtab_t cmd; /* Macro command name */ It's more than just the name, right? > + struct list_head commands; /* Associated command list */ I get confused between the two different usages of "command". Can we call the individual entries of a macro "statements". So this would be: struct list_head statements; /* Associated statement list */ ...and the structure below would get renamed as well. > }; > + > +struct kdb_macro_cmd_t { > + char *cmd; /* Command name */ This isn't the "name" of the command, is it? It's the actual statement that the user entered? Other than that, this looks like a nice patch to me. -Doug ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-06-17 11:49 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-05-13 11:28 [PATCH v3 0/2] kdb code refactoring Sumit Garg 2021-05-13 11:28 ` [PATCH v3 1/2] kdb: Get rid of redundant kdb_register_flags() Sumit Garg 2021-05-13 11:31 ` Sumit Garg 2021-05-13 13:19 ` Steven Rostedt 2021-05-20 17:20 ` Doug Anderson 2021-06-17 11:49 ` Sumit Garg 2021-05-13 11:28 ` [PATCH v3 2/2] kdb: Simplify kdb_defcmd macro logic Sumit Garg 2021-05-20 17:21 ` Doug Anderson
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).