Re: kgdb: core

[Ingo Molnar <mingo@elte.hu>]

* Andrew Morton <akpm@linux-foundation.org> 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