Message-ID: <CAD=FV=WjQjGgGeH5BiC7cc93OVuka+Fx+0BVHjdem2px_N=Y=g@mail.gmail.com>
Date: Tue, 18 Jun 2024 07:42:15 -0700
From: Doug Anderson <dianders@...omium.org>
To: Daniel Thompson <daniel.thompson@...aro.org>
Cc: kgdb-bugreport@...ts.sourceforge.net, 
	Christophe JAILLET <christophe.jaillet@...adoo.fr>, Jason Wessel <jason.wessel@...driver.com>, 
	Thorsten Blum <thorsten.blum@...lux.com>, Yuran Pereira <yuran.pereira@...mail.com>, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 02/13] kdb: Document the various "md" commands better

Hi,

On Tue, Jun 18, 2024 at 4:24 AM Daniel Thompson
<daniel.thompson@...aro.org> wrote:
>
> On Mon, Jun 17, 2024 at 05:34:36PM -0700, Douglas Anderson wrote:
> > The documentation for the variouis "md" commands was inconsistent
> > about documenting the command arguments. It was also hard to figure
> > out what the differences between the "phys", "raw", and "symbolic"
> > versions was.
> >
> > Update the help strings to make things more obvious.
> >
> > As part of this, add "bogus" commands to the table for "mdW" and
> > "mdWcN" so we don't have to obscurely reference them in the normal
> > "md" help. These bogus commands don't really hurt since kdb_md()
> > validates argv[0] enough.
> >
> > Signed-off-by: Douglas Anderson <dianders@...omium.org>
> > ---
> >
> >  kernel/debug/kdb/kdb_main.c | 39 ++++++++++++++++++++++---------------
> >  1 file changed, 23 insertions(+), 16 deletions(-)
> >
> > diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
> > index cbeb203785b4..47e037c3c002 100644
> > --- a/kernel/debug/kdb/kdb_main.c
> > +++ b/kernel/debug/kdb/kdb_main.c
> > @@ -1516,14 +1516,9 @@ static int kdb_mdr(unsigned long addr, unsigned int count)
> >  }
> >
> >  /*
> > - * kdb_md - This function implements the 'md', 'md1', 'md2', 'md4',
> > - *   'md8' 'mdr' and 'mds' commands.
> > + * kdb_md - This function implements the guts of the various 'md' commands.
> >   *
> > - *   md|mds  [<addr arg> [<line count> [<radix>]]]
> > - *   mdWcN   [<addr arg> [<line count> [<radix>]]]
> > - *           where W = is the width (1, 2, 4 or 8) and N is the count.
> > - *           for eg., md1c20 reads 20 bytes, 1 at a time.
> > - *   mdr  <addr arg>,<byte count>
> > + * See the kdb help for syntax.
> >   */
> >  static void kdb_md_line(const char *fmtstr, unsigned long addr,
> >                       int symbolic, int nosect, int bytesperword,
> > @@ -2677,26 +2672,38 @@ EXPORT_SYMBOL_GPL(kdb_unregister);
> >  static kdbtab_t maintab[] = {
> >       {       .name = "md",
> >               .func = kdb_md,
> > -             .usage = "<vaddr>",
> > -             .help = "Display Memory Contents, also mdWcN, e.g. md8c1",
> > +             .usage = "<vaddr> [<lines> [<radix>]]",
> > +             .help = "Display RAM using BYTESPERWORD; show MDCOUNT lines",
>
> I'd prefer "memory" over "RAM" because it's what the mnemonic is
> abbreviating. This applies to all of the below but I won't be adding a
> "same here" for all of them.
>
> Where we have to crush something to fit into one line we'd than have to
> break the pattern and choose from thing like:
>
> 1. Show memory
> 2. Display RAM
> 3. Display mem
>
> Personally I prefer #1 but could probably cope with #2.

I'm not dead set on RAM, but at least for me RAM was more clarifying.
Specifically when it said "memory" I always assumed it would take in
any memory address and when I first looked at this I tried to figure
out why memory addresses in the IO space didn't work with these
commands. I figured I was holding it wrong only to find out that the
commands specifically limit you to just the RAM range of memory
addresses.

That being said, I don't feel strongly so if you really like "memory"
I'll change it back.


> >               .flags = KDB_ENABLE_MEM_READ | KDB_REPEAT_NO_ARGS,
> >       },
> > -     {       .name = "mdr",
> > +     {       .name = "mdW",
> >               .func = kdb_md,
> > -             .usage = "<vaddr> <bytes>",
> > -             .help = "Display Raw Memory",
> > +             .usage = "<vaddr> [<lines> [<radix>]]",
> > +             .help = "Display RAM using word size (W) of 1, 2, 4, or 8",
>
> We need an "e.g. md8" in here somewhere. Otherwise it is not at all
> obvious that W is a wildcard.
>
> I guess that alternatively you could also try naming the command with
> hint that W is a wild card (what happens if you register a command
> called md<W>?).
>
>
> > +             .flags = KDB_ENABLE_MEM_READ | KDB_REPEAT_NO_ARGS,
> > +     },
> > +     {       .name = "mdWcN",
> > +             .func = kdb_md,
> > +             .usage = "<vaddr> [<lines> [<radix>]]",
> > +             .help = "Display RAM using word size (W); show N words",
>
> Same here.

Sure, so:

.name = "md<W>",
.help = "Display RAM using word size 1, 2, 4, or 8; e.g. md8",

.name = "md<W>c<N>",
.help = "Display RAM using word size W; show N words; e.g. md4c6",

...or changing RAM to "memory" if you don't buy my argument above.

We're definitely ending up over the 80 character mark here, but I
assume that's OK. We were even before my change.

I'll assume that I don't need the "e.g." for all the followup (mdp,
mdi) variants introduced in later patches?


Random question: for the mdWcN variant, should I make specifying
<lines> illegal? It's pretty silly to let the user specify a word
count and then immediately override it. In that case, do I bump
"<radix>" to the 2nd argument or just don't allow "<radix>" for mdWcN?
That would need to be done in a later patch, obviously...

-Doug

Powered by blists - more mailing lists