* [PATCH v2] kdb: Simplify kdb commands registration
@ 2021-01-25 7:06 Sumit Garg
2021-01-25 16:01 ` Doug Anderson
0 siblings, 1 reply; 3+ messages in thread
From: Sumit Garg @ 2021-01-25 7:06 UTC (permalink / raw)
To: kgdb-bugreport
Cc: jason.wessel, daniel.thompson, 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 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;
}
+
+ if (strcmp(argv[0], tp->cmd_name) == 0)
+ break;
}
/*
@@ -1037,19 +1024,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 +2411,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)
@@ -2667,49 +2647,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;
@@ -2719,6 +2670,8 @@ int kdb_register_flags(char *cmd,
kp->cmd_minlen = minlen;
kp->cmd_flags = flags;
+ list_add_tail(&kp->list_node, &kdb_cmds_head);
+
return 0;
}
EXPORT_SYMBOL_GPL(kdb_register_flags);
@@ -2757,15 +2710,15 @@ 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);
+ kfree(kp);
return 0;
}
}
@@ -2778,12 +2731,6 @@ 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);
diff --git a/kernel/debug/kdb/kdb_private.h b/kernel/debug/kdb/kdb_private.h
index a4281fb..75ceafa 100644
--- a/kernel/debug/kdb/kdb_private.h
+++ b/kernel/debug/kdb/kdb_private.h
@@ -174,6 +174,7 @@ 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 */
} kdbtab_t;
extern int kdb_bt(int, const char **); /* KDB display back trace */
--
2.7.4
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] kdb: Simplify kdb commands registration
2021-01-25 7:06 [PATCH v2] kdb: Simplify kdb commands registration Sumit Garg
@ 2021-01-25 16:01 ` Doug Anderson
2021-01-28 5:12 ` Sumit Garg
0 siblings, 1 reply; 3+ messages in thread
From: Doug Anderson @ 2021-01-25 16:01 UTC (permalink / raw)
To: Sumit Garg; +Cc: kgdb-bugreport, Jason Wessel, Daniel Thompson, LKML
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;
In any case, this looks like quite a nice cleanup, so:
Reviewed-by: Douglas Anderson <dianders@chromium.org>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] kdb: Simplify kdb commands registration
2021-01-25 16:01 ` Doug Anderson
@ 2021-01-28 5:12 ` Sumit Garg
0 siblings, 0 replies; 3+ messages in thread
From: Sumit Garg @ 2021-01-28 5:12 UTC (permalink / raw)
To: Doug Anderson; +Cc: kgdb-bugreport, Jason Wessel, Daniel Thompson, LKML
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-01-28 5:13 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-25 7:06 [PATCH v2] kdb: Simplify kdb commands registration Sumit Garg
2021-01-25 16:01 ` Doug Anderson
2021-01-28 5:12 ` Sumit Garg
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).