LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Christoph Hellwig <hch@infradead.org>
To: jason.wessel@windriver.com
Cc: linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/8] kgdb: core API and gdb protocol handler
Date: Sat, 9 Feb 2008 12:27:13 -0500	[thread overview]
Message-ID: <20080209172713.GD15568@infradead.org> (raw)
In-Reply-To: <1202564114-18587-2-git-send-email-jason.wessel@windriver.com>

On Sat, Feb 09, 2008 at 07:35:07AM -0600, jason.wessel@windriver.com wrote:
> index 0000000..24e0b6c
> --- /dev/null
> +++ b/include/asm-generic/kgdb.h
> @@ -0,0 +1,105 @@
> +/*
> + * include/asm-generic/kgdb.h

Please don't mention the file name in the top-of-file comments.  This
information is redundant and will easily get out of date when moving
files around or copying them.  Note that this applies to basically
any file in this patch.

> +#ifdef CONFIG_X86
> +/**
> + *	kgdb_skipexception - Bail of 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.
> + */
> +int kgdb_skipexception(int exception, struct pt_regs *regs);
> +#else
> +static inline int kgdb_skipexception(int exception, struct pt_regs *regs)
> +{
> +	return 0;
> +}
> +#endif
> +
> +#if defined(CONFIG_X86)

arch ifdefs don't belong into an asm-generic/ file.  Please have a
proper asm-x86/kgdb.h that defines these things.

> +/**
> + *	kgdb_post_master_code - Save error vector/code numbers.
> + *	@regs: Original pt_regs.
> + *	@e_vector: Original error vector.
> + *	@err_code: Original error code.
> + *
> + *	This is needed on architectures which support SMP and KGDB.
> + *	This function is called after all the slave cpus have been put
> + *	to a know spin state and the master CPU has control over KGDB.
> + */
> +extern void kgdb_post_master_code(struct pt_regs *regs, int e_vector,
> +				  int err_code);

Kerneldoc comments don't belong above the prototype of a function but
the function body.

> +#ifdef CONFIG_KGDB_ARCH_HAS_SHADOW_INFO
> +/**
> + *	kgdb_shadowinfo - Get shadowed information on @threadid.
> + *	@regs: The &struct pt_regs of the current process.
> + *	@buffer: A buffer of %BUFMAX size.
> + *	@threadid: The thread id of the shadowed process to get information on.
> + */
> +extern void kgdb_shadowinfo(struct pt_regs *regs, char *buffer,
> +			    unsigned threadid);

I don't really thing this belongs into an asm-generic header, again.
ARchitectures having shadow info should just provide this in their
own asm-foo/kgdb.h.  Or better yet just kill it for the first
submission.

> +struct debuggerinfo_struct {
> +	void			*debuggerinfo;
> +	struct task_struct	*task;
> +} kgdb_info[NR_CPUS];

shouldn't this use per-cpu data?  Or is that in some way to
fragile for a debugger?

> +/* reboot notifier block */
> +static struct notifier_block kgdb_reboot_notifier = {
> +	.notifier_call		= kgdb_notify_reboot,
> +	.next			= NULL,
> +	.priority		= INT_MAX,
> +};

No need to initialize fields to 0 or NULL in static variables.

> +{
> +	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;

lots of superflous braces.  More of them later in this file
in the same style.

> +#ifdef __BIG_ENDIAN
> +		*buf++ = hexchars[(tmp_s >> 12) & 0xf];
> +		*buf++ = hexchars[(tmp_s >> 8) & 0xf];
> +		*buf++ = hexchars[(tmp_s >> 4) & 0xf];
> +		*buf++ = hexchars[tmp_s & 0xf];
> +#else
> +		*buf++ = hexchars[(tmp_s >> 4) & 0xf];
> +		*buf++ = hexchars[tmp_s & 0xf];
> +		*buf++ = hexchars[(tmp_s >> 12) & 0xf];
> +		*buf++ = hexchars[(tmp_s >> 8) & 0xf];
> +#endif

This is really ugly, but I don't really know a good way around it
either.

> +	if (arch_kgdb_ops.shadowth &&
> +			ks->kgdb_usethreadid >= pid_max + num_online_cpus()) {

	if (arch_kgdb_ops.shadowth &&
	    ks->kgdb_usethreadid >= pid_max + num_online_cpus()) {

similar odd indentation in a few other spots.

> +menuconfig KGDB
> +	bool "KGDB: kernel debugging with remote gdb"
> +	select KGDB_ARCH_HAS_SHADOW_INFO if X86_64

Why can't this be set in the X86_64 config?

> +	select DEBUG_INFO
> +	select FRAME_POINTER

I think these two would be better as depends on


  parent reply	other threads:[~2008-02-09 17:27 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-02-09 13:35 [PATCH 0/8] kgdb 2.6.25 version jason.wessel
2008-02-09 13:35 ` [PATCH 1/8] kgdb: core API and gdb protocol handler jason.wessel
2008-02-09 13:35   ` [PATCH 2/8] pid, kgdb: add pid_max prototype jason.wessel
2008-02-09 13:35     ` [PATCH 3/8] kgdb, modules: Always allow module sect info for KGDB jason.wessel
2008-02-09 13:35       ` [PATCH 4/8] kgdb: COPTIMIZE flag jason.wessel
2008-02-09 13:35         ` [PATCH 5/8] kgdb, x86: Add arch specfic kgdb support jason.wessel
2008-02-09 13:35           ` [PATCH 6/8] kgdb, sysrq_bugfix jason.wessel
2008-02-09 13:35             ` [PATCH][7/8] kgdb: exclusive use kgdb8250 uart I/O driver jason.wessel
2008-02-09 13:35               ` [PATCH 8/8] kgdb: kgdboc 8250 I/O module jason.wessel
2008-02-09 14:53               ` [PATCH][7/8] kgdb: exclusive use kgdb8250 uart I/O driver Jan Kiszka
2008-02-09 18:45                 ` Jason Wessel
2008-02-09 16:40               ` Jan Kiszka
2008-02-09 18:41                 ` Jason Wessel
2008-02-10 15:26               ` Pavel Machek
2008-02-09 14:33           ` [PATCH 5/8] kgdb, x86: Add arch specfic kgdb support Jan Kiszka
2008-02-09 17:16         ` [PATCH 4/8] kgdb: COPTIMIZE flag Christoph Hellwig
2008-02-09 17:15       ` [PATCH 3/8] kgdb, modules: Always allow module sect info for KGDB Christoph Hellwig
2008-02-09 17:10     ` [PATCH 2/8] pid, kgdb: add pid_max prototype Christoph Hellwig
2008-02-09 14:27   ` [PATCH 1/8] kgdb: core API and gdb protocol handler Jan Kiszka
2008-02-09 15:29   ` Sam Ravnborg
2008-02-09 17:27   ` Christoph Hellwig [this message]
2008-02-09 19:46     ` Ray Lee
2008-02-09 21:51       ` Ray Lee
2008-02-09 17:38 ` [PATCH 0/8] kgdb 2.6.25 version Christoph Hellwig

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20080209172713.GD15568@infradead.org \
    --to=hch@infradead.org \
    --cc=jason.wessel@windriver.com \
    --cc=linux-kernel@vger.kernel.org \
    --subject='Re: [PATCH 1/8] kgdb: core API and gdb protocol handler' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).