Thanks Doug for your review.
On Sat, 23 Jan 2021 at 00:12, Doug Anderson <diand...@chromium.org> wrote:
>
> Hi,
>
> On Tue, Jan 19, 2021 at 2:50 AM Sumit Garg <sumit.g...@linaro.org> wrote:
> >
> > Simplify kdb commands registration via using linked list instead of
> > static array for commands storage.
> >
> > Signed-off-by: Sumit Garg <sumit.g...@linaro.org>
> > ---
> > kernel/debug/kdb/kdb_main.c | 78
> > ++++++++++--------------------------------
> > kernel/debug/kdb/kdb_private.h | 1 +
> > 2 files changed, 20 insertions(+), 59 deletions(-)
>
> Wow, nice. It should have been done this way from the start! ;-)
>
>
> > @@ -1011,7 +1005,7 @@ int kdb_parse(const char *cmdstr)
> > ++argv[0];
> > }
> >
> > - for_each_kdbcmd(tp, i) {
> > + list_for_each_entry(tp, &kdb_cmds_head, list_node) {
> > if (tp->cmd_name) {
>
> So I think here (and elsewhere in this file) you can remove this "if
> (...->cmd_name)" stuff now, right? That was all there because the old
> "remove" used to just NULL out the name to handle gaps and that's no
> longer possible. If you are really paranoid you could error-check
> kdb_register_flags()
>
Agree, will get rid of this check.
>
> > --- 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;
>
> nit: every other entry in this struct has a comment but not yours..
>
Will add a comment here.
>
> Other than those small nits I think this is a great improvement, thanks!
>
Thanks,
Sumit
> -Doug
