Re: [PATCH]KGTP (Linux Kernel debugger and tracer) lite patch for review

[Andi Kleen <andi@firstfloor.org>]

Please provide better explanation of the use case for this module.
One paragraph why someone would want it in their kernel.

> +++ b/arch/arm/include/asm/gtp.h
> @@ -0,0 +1,34 @@
> +#ifndef _ASM_ARM_GTP_H_
> +#define _ASM_ARM_GTP_H_
> +
> +#define ULONGEST		uint64_t

So u64 in kernel. Just use that.

> +#define CORE_ADDR		unsigned long

In linux kernel CORE_ADDR is always unsigned long. Use that.

> +
> +#define GTP_REG_ASCII_SIZE	336
> +
> +static inline void
> +gtp_regs2ascii(struct pt_regs *regs, char *buf)
> +{
> +#ifdef __LITTLE_ENDIAN
> +#define SWAB(a)		swab32(a)
> +#else
> +#define SWAB(a)		(a)
> +#endif

That's just ntohl()? Just use that

Linux already has macros for this: 
> +	int	i;
> +
> +	for (i = 0; i < 16; i++) {
> +		sprintf(buf, "%08lx", (unsigned long) SWAB(regs->uregs[i]));

Is the gdb protocol really big endian in ASCII?

Also could you share code on this with the in kernel gdbstub?

> +#include <linux/slab.h>
> +#include <asm/gtp.h>
> +
> +#define GTP_DEBUG		KERN_WARNING

You should use the pr_* macros now

> +static int			gtp_disconnected_tracing;
> +static int			gtp_circular;
> +
> +static DEFINE_SPINLOCK(gtp_frame_lock);

Every spinlock needs a comment that describes what it protects.
I believe checkpatch warns about that. Did you run it?

> +
> +	tmp = gtp_frame_alloc(GTP_FRAME_REG_SIZE);
> +	if (!tmp)
> +		return NULL;
> +
> +	*next = tmp;
> +	tmp[0] = 'r';
> +	freg = (struct gtp_frame_reg *) (tmp + 1);
> +	memcpy(&freg->regs, regs, sizeof(struct pt_regs));
> +#if !defined CONFIG_X86_32 && !defined CONFIG_X86_64
> +	freg->regs.sp = (unsigned long)&regs->sp;
> +#endif	/* CONFIG_X86_32 CONFIG_X86_64 */

That looks weird. What does that do?

> +gtp_action_alloc(char type)
> +{
> +	struct action	*ret;
> +
> +	ret = kmalloc(sizeof(struct action), GFP_KERNEL);

kzalloc

Same problem in others.

> +static int
> +hex2int(char hex, int *i)

I'm sure we have code for this. strtoul et.al.?

Didn't read further so far.


-Andi