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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
