Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210513112842.707103-1-sumit.garg@linaro.org>
 <20210513112842.707103-2-sumit.garg@linaro.org> <CAD=FV=UkQoUFzYa3OKPSwQo51V09-EXTdp=GNo=H+ZMcjX-ovw@mail.gmail.com>
In-Reply-To: <CAD=FV=UkQoUFzYa3OKPSwQo51V09-EXTdp=GNo=H+ZMcjX-ovw@mail.gmail.com>
From:   Sumit Garg <sumit.garg@linaro.org>
Date:   Thu, 17 Jun 2021 17:19:39 +0530
Message-ID: <CAFA6WYP6X+VRY=A6DBYcRUDVAFZT=FwmzB_Dc0XMjtsNNoa3jQ@mail.gmail.com>
Subject: Re: [PATCH v3 1/2] kdb: Get rid of redundant kdb_register_flags()
To:     Doug Anderson <dianders@chromium.org>
Cc:     kgdb-bugreport@lists.sourceforge.net,
        Daniel Thompson <daniel.thompson@linaro.org>,
        Jason Wessel <jason.wessel@windriver.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        Ingo Molnar <mingo@redhat.com>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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