Messages in this thread | | | From | Sumit Garg <> | Date | Thu, 28 Jan 2021 10:42:18 +0530 | Subject | Re: [PATCH v2] kdb: Simplify kdb commands registration |
| |
On Mon, 25 Jan 2021 at 21:32, Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Sun, Jan 24, 2021 at 11:06 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 v2: > > - Remove redundant NULL check for "cmd_name". > > - Incorporate misc. comment. > > > > kernel/debug/kdb/kdb_main.c | 119 ++++++++++++----------------------------- > > kernel/debug/kdb/kdb_private.h | 1 + > > 2 files changed, 34 insertions(+), 86 deletions(-) > > > > diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c > > index 930ac1b..a0989a0 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,18 @@ 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; > > - } > > - } > > - > > - if (strcmp(argv[0], tp->cmd_name) == 0) > > + 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)) { > > + if (strncmp(argv[0], tp->cmd_name, tp->cmd_minlen) == 0) > > break; > > } > > The old code had the same problem, but since you're touching it you could fix? > > if (a) { > if (b) > break; > } > > ...is the same as: > > if (a && b) > break; >
Sure, I will fix it in the next version.
> In any case, this looks like quite a nice cleanup, so: > > Reviewed-by: Douglas Anderson <dianders@chromium.org>
Thanks, Sumit
| |