Messages in this thread | | | Date | Mon, 21 Apr 2008 16:12:52 +0200 | From | Ingo Molnar <> | Subject | Re: kgdb: core |
| |
* 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
| |