Re: kgdb: core

From: Ingo Molnar
Date: Mon Apr 21 2008 - 10:13:19 EST



* Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:

> > +/*
> > + * kgdb_skipexception - Bail out of KGDB when we've been triggered.
> > + * @exception: Exception vector number
> > + * @regs: Current &struct pt_regs.
> > + *
> > + * On some architectures we need to skip a breakpoint exception when
> > + * it occurs after a breakpoint has been removed.
> > + */
> > +extern int kgdb_skipexception(int exception, struct pt_regs *regs);
>
> Please just nuke all the interface comments in the header files. They
> duplicate the kernedoc comments at the definition site and we don't
> want to have to update both versions whenever we change something.

well that way we'll have to update _all_ arch versions whenever we
change something - while the reference prototype in kgdb.h should all
cover it. Do we really want to do that?

> > +/*
> > + * Functions each KGDB-supporting architecture must provide:
> > + */
> > +
> > +/*
> > + * kgdb_arch_init - Perform any architecture specific initalization.
> > + *
> > + * This function will handle the initalization of any architecture
> > + * specific callbacks.
> > + */
> > +extern int kgdb_arch_init(void);
>
> Well, these are trickier because there is an implementation of this
> function within each architecture. So I think that in this case it
> _does_ make sense to document the function in a common place, and the
> only common place is this header file.
>
> So please
>
> a) make this a kerneldoc comment and
>
> b) remove the kerneldoc at the definition site(s).
>
> (alternative: teach the kerneldoc system to go fishing in the various
> arch directories to find the appropriate documentation, but I don't
> know enough about kerneldoc to be able say anything about that).

well there's lkml feedback ping-pong effect here. It was pointed out in
earlier kgdb review that it's an "error" to put kerneldoc into header
files. I pointed out that it makes no sense to do otherwise but removed
the kerneldoc annotation to resolve the "objection".

> This should become a kernedoc comment, as this is the only place we
> can document it. So please add the leading /**

same deal - it was objected to in review.

> > +static const char hexchars[] = "0123456789abcdef";
> > +
> > +static int hex(char ch)
> > +{
> > + if ((ch >= 'a') && (ch <= 'f'))
> > + return ch - 'a' + 10;
> > + if ((ch >= '0') && (ch <= '9'))
> > + return ch - '0';
> > + if ((ch >= 'A') && (ch <= 'F'))
> > + return ch - 'A' + 10;
> > + return -1;
> > +}
>
> How many are we up to now?
>
> akpm:/usr/src/linux-2.6.25> grep -ri '"0123456789abcdef"' . | wc -l
> 40
>
> lol.

okay, hex_asc() it should use. Probably KGDB's code predates that of
kernel.h though ;-)

> Nice-looking code - kgb has improved rather a lot. I'm glad we
> finally got it in. [...]

thanks :)

> [...] Maybe one day I'll get to use it again :(

/me duly notes this request to break Andrew's systems even more frequently ;-)

Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/