Messages in this thread | | | Date | Wed, 9 May 2012 16:05:39 +0200 | From | Andi Kleen <> | Subject | Re: [PATCH]KGTP (Linux Kernel debugger and tracer) lite patch for review |
| |
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)®s->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
| |