lkml.org 
[lkml]   [2012]   [May]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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)&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


\
 
 \ /
  Last update: 2012-05-09 16:41    [W:0.093 / U:0.168 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site