LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Sam Ravnborg <sam@ravnborg.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 16:29:20 +0100	[thread overview]
Message-ID: <20080209152920.GA7492@uranus.ravnborg.org> (raw)
In-Reply-To: <1202564114-18587-2-git-send-email-jason.wessel@windriver.com>

Hi Jason.

> --- /dev/null
> +++ b/kernel/kgdb.c
> +
> +struct debuggerinfo_struct {
> +	void			*debuggerinfo;
> +	struct task_struct	*task;
> +} kgdb_info[NR_CPUS];
static?

> +
> +/* Is a host GDB connected to us? */
> +int				kgdb_connected;
> +EXPORT_SYMBOL_GPL(kgdb_connected);
Drop additional spaces.
Add kernel-doc comments explaining the usage.

> +/* All the KGDB handlers are installed */
> +int				kgdb_io_module_registered;
static? drop spaces.

> +
> +/* Guard for recursive entry */
> +static int			exception_level;
drop spaces. In more places below - but they are obvious.

> +struct kgdb_bkpt		kgdb_break[KGDB_MAX_BREAKPOINTS] = {
> +	[0 ... KGDB_MAX_BREAKPOINTS-1] = { .state = BP_UNDEFINED }
> +};
static?

> +
> +extern int pid_max;

extern must be moved to a .h file.

> +atomic_t			kgdb_setting_breakpoint;
static?
Many more variables are static candidates. I will not repeat it.
Did you run this through sparse?

> +#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
small helper function?

> +	} else if ((count == 4) && (((long)mem & 3) == 0)) {
> +		u32 tmp_l;
> +		if (kgdb_mem_cpy(&tmp_l, mem, count))
> +			return ERR_PTR(-EINVAL);
> +
> +		mem += 4;
> +#ifdef __BIG_ENDIAN
> +		*buf++ = hexchars[(tmp_l >> 28) & 0xf];
> +		*buf++ = hexchars[(tmp_l >> 24) & 0xf];
> +		*buf++ = hexchars[(tmp_l >> 20) & 0xf];
> +		*buf++ = hexchars[(tmp_l >> 16) & 0xf];
> +		*buf++ = hexchars[(tmp_l >> 12) & 0xf];
> +		*buf++ = hexchars[(tmp_l >> 8) & 0xf];
> +		*buf++ = hexchars[(tmp_l >> 4) & 0xf];
> +		*buf++ = hexchars[tmp_l & 0xf];
> +#else
> +		*buf++ = hexchars[(tmp_l >> 4) & 0xf];
> +		*buf++ = hexchars[tmp_l & 0xf];
> +		*buf++ = hexchars[(tmp_l >> 12) & 0xf];
> +		*buf++ = hexchars[(tmp_l >> 8) & 0xf];
> +		*buf++ = hexchars[(tmp_l >> 20) & 0xf];
> +		*buf++ = hexchars[(tmp_l >> 16) & 0xf];
> +		*buf++ = hexchars[(tmp_l >> 28) & 0xf];
> +		*buf++ = hexchars[(tmp_l >> 24) & 0xf];
> +#endif
that could be used here too.
And again below.

> + * Convert the hex array pointed to by buf into binary to be placed in mem.
> + * Return a pointer to the character AFTER the last byte written.
> + * May return an error.
> + */
> +char *kgdb_hex2mem(char *buf, char *mem, int count)
> +{
> +	if ((count == 2) && (((long)mem & 1) == 0)) {
> +		u16 tmp_s = 0;
> +
> +#ifdef __BIG_ENDIAN
> +		tmp_s |= hex(*buf++) << 12;
> +		tmp_s |= hex(*buf++) << 8;
> +		tmp_s |= hex(*buf++) << 4;
> +		tmp_s |= hex(*buf++);
> +#else
> +		tmp_s |= hex(*buf++) << 4;
> +		tmp_s |= hex(*buf++);
> +		tmp_s |= hex(*buf++) << 12;
> +		tmp_s |= hex(*buf++) << 8;
> +#endif
small helper function again.

> +int kgdb_isremovedbreak(unsigned long addr)
> +{
> +	int i;
> +
> +	for (i = 0; i < KGDB_MAX_BREAKPOINTS; i++) {
> +		if ((kgdb_break[i].state == BP_REMOVED) &&
> +					(kgdb_break[i].bpt_addr == addr))
> +			return 1;
> +	}
> +	return 0;
> +}
static?

> +
> +int remove_all_break(void)
> +{
> +	unsigned long addr;
> +	int error;
> +	int i;
> +
> +	/* Clear memory breakpoints. */
> +	for (i = 0; i < KGDB_MAX_BREAKPOINTS; i++) {
> +		if (kgdb_break[i].state != BP_SET)
> +			continue;
> +		addr = kgdb_break[i].bpt_addr;
> +		error = kgdb_arch_remove_breakpoint(addr,
> +				kgdb_break[i].saved_instr);
> +		if (error)
> +			return error;
> +		kgdb_break[i].state = BP_REMOVED;
> +	}
> +
> +	/* Clear hardware breakpoints. */
> +	if (arch_kgdb_ops.remove_all_hw_break)
> +		arch_kgdb_ops.remove_all_hw_break();
> +
> +	return 0;
> +}
static?

> +/*
> + * Return true if there is a valid kgdb I/O module.  Also if no
> + * debugger is attached a message can be printed to the console about
> + * waiting for the debugger to attach.
> + *
> + * The print_wait argument is only to be true when called from inside
> + * the core kgdb_handle_exception, because it will wait for the
> + * debugger to attach.
> + */
> +int kgdb_io_ready(int print_wait)
> +{
> +	if (!kgdb_io_ops)
> +		return 0;
> +	if (kgdb_connected)
> +		return 1;
> +	if (atomic_read(&kgdb_setting_breakpoint))
> +		return 1;
> +	if (print_wait)
> +		printk(KERN_CRIT "KGDB: Waiting for remote debugger\n");
> +	return 1;
> +}
static?
Please review the rest of the code-base for static candidates.

> new file mode 100644
> index 0000000..afa61bb
> --- /dev/null
> +++ b/lib/Kconfig.kgdb
> @@ -0,0 +1,32 @@
> +
> +menuconfig KGDB
> +	bool "KGDB: kernel debugging with remote gdb"
> +	select KGDB_ARCH_HAS_SHADOW_INFO if X86_64
> +	select DEBUG_INFO
> +	select FRAME_POINTER
> +	depends on DEBUG_KERNEL && ADD_A_KGDB_ARCH

Replace ADD_A_...
with
HAVE_KGDB

and then let arch do:
	select HAVE_KGDB if they support KGDB

See Documentation/kbuild/kconfig-language.txt

	Sam

  parent reply	other threads:[~2008-02-09 15:29 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 [this message]
2008-02-09 17:27   ` Christoph Hellwig
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=20080209152920.GA7492@uranus.ravnborg.org \
    --to=sam@ravnborg.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).