On Mon, 25 Jan 2021 at 22:44, Doug Anderson <diand...@chromium.org> wrote:
>
> Hi,
>
> On Mon, Jan 25, 2021 at 6:30 AM Sumit Garg <sumit.g...@linaro.org> wrote:
> >
> > diff --git a/kernel/debug/kdb/kdb_env.c b/kernel/debug/kdb/kdb_env.c
> > new file mode 100644
> > index 0000000..33ab5e6
> > --- /dev/null
> > +++ b/kernel/debug/kdb/kdb_env.c
> > @@ -0,0 +1,229 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Kernel Debugger Architecture Independent Environment Functions
> > + *
> > + * Copyright (c) 1999-2004 Silicon Graphics, Inc. All Rights Reserved.
> > + * Copyright (c) 2009 Wind River Systems, Inc. All Rights Reserved.
> > + * 03/02/13 added new 2.5 kallsyms <xavier....@bull.net>
>
> I'm not sure the policy for copying over copyright notices like this,
> but I would have expected them to get copied over from the file you
> got the code from? These are slightly different.
>
Okay, I will match them exactly.
> > + */
> > +
> > +#include <linux/kdb.h>
> > +#include <linux/string.h>
> > +#include "kdb_private.h"
> > +
> > +/*
> > + * Initial environment. This is all kept static and local to
> > + * this file. We don't want to rely on the memory allocation
> > + * mechanisms in the kernel, so we use a very limited allocate-only
> > + * heap for new and altered environment variables. The entire
> > + * environment is limited to a fixed number of entries (add more
> > + * to __env[] if required) and a fixed amount of heap (add more to
> > + * KDB_ENVBUFSIZE if required).
> > + */
> > +static char *__env[] = {
> > +#if defined(CONFIG_SMP)
> > + "PROMPT=[%d]kdb> ",
> > +#else
> > + "PROMPT=kdb> ",
> > +#endif
> > + "MOREPROMPT=more> ",
> > + "RADIX=16",
> > + "MDCOUNT=8", /* lines of md output */
> > + KDB_PLATFORM_ENV,
> > + "DTABCOUNT=30",
> > + "NOSECT=1",
> > + (char *)0,
>
> In a follow-up patch, I guess these could move from 0 to NULL and
> remove the cast?
>
Sure, I will remove the cast.
>
> > +/*
> > + * kdbgetenv - This function will return the character string value of
> > + * an environment variable.
> > + * Parameters:
> > + * match A character string representing an environment variable.
> > + * Returns:
> > + * NULL No environment variable matches 'match'
> > + * char* Pointer to string value of environment variable.
> > + */
>
> In a follow-up patch, the above could be moved to kernel-doc format,
> which we're trying to move to for kgdb when we touch code.
>
> I will leave it up to you about whether the new functions introduced
> in this patch are introduced with the proper kernel doc format or move
> to the right format in the same follow-up patch.
>
Okay, I will move these to kernel-doc format.
>
> > +/*
> > + * kdb_prienv - Display the current environment variables.
> > + */
> > +void kdb_prienv(void)
>
> IMO saving the two characters in the function name isn't worth it,
> especially since this function is called in only one place. Use
> kdb_printenv()
Sure, I will rename it as kdb_printenv().
-Sumit
>
> -Doug
