Re: [PATCH 1/2] ftrace, powerpc: replace debug macro with proper pr_deug

Re: [PATCH 1/2] ftrace, powerpc: replace debug macro with proper pr_deug

[Steven Rostedt]

[ forgot to add linuxppc ]

On Fri, 6 Feb 2009, Steven Rostedt wrote:

> From: Steven Rostedt <srostedt@redhat.com>
> 
> Impact: cleanup
> 
> The PowerPC ftrace code uses a hacked up DEBUGP macro for prints.
> This patch converts it to the standard pr_debug.
> 
> Signed-off-by: Steven Rostedt <srostedt@redhat.com>
> ---
>  arch/powerpc/kernel/ftrace.c |   26 ++++++++++----------------
>  1 files changed, 10 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/ftrace.c b/arch/powerpc/kernel/ftrace.c
> index 5355244..a913f91 100644
> --- a/arch/powerpc/kernel/ftrace.c
> +++ b/arch/powerpc/kernel/ftrace.c
> @@ -20,12 +20,6 @@
>  #include <asm/code-patching.h>
>  #include <asm/ftrace.h>
>  
> -#if 0
> -#define DEBUGP printk
> -#else
> -#define DEBUGP(fmt , ...)	do { } while (0)
> -#endif
> -
>  static unsigned int ftrace_nop = PPC_NOP_INSTR;
>  
>  #ifdef CONFIG_PPC32
> @@ -175,7 +169,7 @@ __ftrace_make_nop(struct module *mod,
>  	 * 0xe8, 0x4c, 0x00, 0x28,    ld      r2,40(r12)
>  	 */
>  
> -	DEBUGP("ip:%lx jumps to %lx r2: %lx", ip, tramp, mod->arch.toc);
> +	pr_debug("ip:%lx jumps to %lx r2: %lx", ip, tramp, mod->arch.toc);
>  
>  	/* Find where the trampoline jumps to */
>  	if (probe_kernel_read(jmp, (void *)tramp, sizeof(jmp))) {
> @@ -183,7 +177,7 @@ __ftrace_make_nop(struct module *mod,
>  		return -EFAULT;
>  	}
>  
> -	DEBUGP(" %08x %08x", jmp[0], jmp[1]);
> +	pr_debug(" %08x %08x", jmp[0], jmp[1]);
>  
>  	/* verify that this is what we expect it to be */
>  	if (((jmp[0] & 0xffff0000) != 0x3d820000) ||
> @@ -198,18 +192,18 @@ __ftrace_make_nop(struct module *mod,
>  	offset = (unsigned)((unsigned short)jmp[0]) << 16 |
>  		(unsigned)((unsigned short)jmp[1]);
>  
> -	DEBUGP(" %x ", offset);
> +	pr_debug(" %x ", offset);
>  
>  	/* get the address this jumps too */
>  	tramp = mod->arch.toc + offset + 32;
> -	DEBUGP("toc: %lx", tramp);
> +	pr_debug("toc: %lx", tramp);
>  
>  	if (probe_kernel_read(jmp, (void *)tramp, 8)) {
>  		printk(KERN_ERR "Failed to read %lx\n", tramp);
>  		return -EFAULT;
>  	}
>  
> -	DEBUGP(" %08x %08x\n", jmp[0], jmp[1]);
> +	pr_debug(" %08x %08x\n", jmp[0], jmp[1]);
>  
>  	ptr = ((unsigned long)jmp[0] << 32) + jmp[1];
>  
> @@ -286,7 +280,7 @@ __ftrace_make_nop(struct module *mod,
>  	 *  0x4e, 0x80, 0x04, 0x20  bctr
>  	 */
>  
> -	DEBUGP("ip:%lx jumps to %lx", ip, tramp);
> +	pr_debug("ip:%lx jumps to %lx", ip, tramp);
>  
>  	/* Find where the trampoline jumps to */
>  	if (probe_kernel_read(jmp, (void *)tramp, sizeof(jmp))) {
> @@ -294,7 +288,7 @@ __ftrace_make_nop(struct module *mod,
>  		return -EFAULT;
>  	}
>  
> -	DEBUGP(" %08x %08x ", jmp[0], jmp[1]);
> +	pr_debug(" %08x %08x ", jmp[0], jmp[1]);
>  
>  	/* verify that this is what we expect it to be */
>  	if (((jmp[0] & 0xffff0000) != 0x3d600000) ||
> @@ -310,7 +304,7 @@ __ftrace_make_nop(struct module *mod,
>  	if (tramp & 0x8000)
>  		tramp -= 0x10000;
>  
> -	DEBUGP(" %x ", tramp);
> +	pr_debug(" %x ", tramp);
>  
>  	if (tramp != addr) {
>  		printk(KERN_ERR
> @@ -413,7 +407,7 @@ __ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
>  	/* ld r2,40(r1) */
>  	op[1] = 0xe8410028;
>  
> -	DEBUGP("write to %lx\n", rec->ip);
> +	pr_debug("write to %lx\n", rec->ip);
>  
>  	if (probe_kernel_write((void *)ip, op, MCOUNT_INSN_SIZE * 2))
>  		return -EPERM;
> @@ -453,7 +447,7 @@ __ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
>  		return -EINVAL;
>  	}
>  
> -	DEBUGP("write to %lx\n", rec->ip);
> +	pr_debug("write to %lx\n", rec->ip);
>  
>  	if (probe_kernel_write((void *)ip, &op, MCOUNT_INSN_SIZE))
>  		return -EPERM;
> -- 
> 1.5.6.5
> 
> -- 
> 
> 

Re: [PATCH 2/2] powerpc, ftrace: fix compile error when modules not configured

[Steven Rostedt]

[ forgot to add linuxppc ]


On Fri, 6 Feb 2009, Steven Rostedt wrote:

> From: Steven Rostedt <srostedt@redhat.com>
> 
> Michael Neuling reported a compile bug when dynamic ftrace was
> configured in and modules were not. This was due to the ftrace
> code referencing module specific structures.
> 
> Reported-by: Michael Neuling <mikey@neuling.org>
> Signed-off-by: Steven Rostedt <srostedt@redhat.com>
> ---
>  arch/powerpc/kernel/ftrace.c |   16 +++++++++++++++-
>  1 files changed, 15 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/ftrace.c b/arch/powerpc/kernel/ftrace.c
> index a913f91..88c641d 100644
> --- a/arch/powerpc/kernel/ftrace.c
> +++ b/arch/powerpc/kernel/ftrace.c
> @@ -113,6 +113,8 @@ static int test_24bit_addr(unsigned long ip, unsigned long addr)
>  	return create_branch((unsigned int *)ip, addr, 0);
>  }
>  
> +#ifdef CONFIG_MODULES
> +
>  static int is_bl_op(unsigned int op)
>  {
>  	return (op & 0xfc000003) == 0x48000001;
> @@ -323,6 +325,7 @@ __ftrace_make_nop(struct module *mod,
>  	return 0;
>  }
>  #endif /* PPC64 */
> +#endif /* CONFIG_MODULES */
>  
>  int ftrace_make_nop(struct module *mod,
>  		    struct dyn_ftrace *rec, unsigned long addr)
> @@ -342,6 +345,7 @@ int ftrace_make_nop(struct module *mod,
>  		return ftrace_modify_code(ip, old, new);
>  	}
>  
> +#ifdef CONFIG_MODULES
>  	/*
>  	 * Out of range jumps are called from modules.
>  	 * We should either already have a pointer to the module
> @@ -366,9 +370,13 @@ int ftrace_make_nop(struct module *mod,
>  		mod = rec->arch.mod;
>  
>  	return __ftrace_make_nop(mod, rec, addr);
> -
> +#else
> +	/* We should not get here without modules */
> +	return -EINVAL;
> +#endif /* CONFIG_MODULES */
>  }
>  
> +#ifdef CONFIG_MODULES
>  #ifdef CONFIG_PPC64
>  static int
>  __ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
> @@ -457,6 +465,7 @@ __ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
>  	return 0;
>  }
>  #endif /* CONFIG_PPC64 */
> +#endif /* CONFIG_MODULES */
>  
>  int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
>  {
> @@ -475,6 +484,7 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
>  		return ftrace_modify_code(ip, old, new);
>  	}
>  
> +#ifdef CONFIG_MODULES
>  	/*
>  	 * Out of range jumps are called from modules.
>  	 * Being that we are converting from nop, it had better
> @@ -486,6 +496,10 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
>  	}
>  
>  	return __ftrace_make_call(rec, addr);
> +#else
> +	/* We should not get here without modules */
> +	return -EINVAL;
> +#endif /* CONFIG_MODULES */
>  }
>  
>  int ftrace_update_ftrace_func(ftrace_func_t func)
> -- 
> 1.5.6.5
> 
> -- 
> 
> 

Re: [PATCH 2/2] powerpc, ftrace: fix compile error when modules not configured

[Michael Neuling]

In message <20090206060527.369616736@goodmis.org> you wrote:
> From: Steven Rostedt <srostedt@redhat.com>
> 
> Michael Neuling reported a compile bug when dynamic ftrace was
> configured in and modules were not. This was due to the ftrace
> code referencing module specific structures.
> 
> Reported-by: Michael Neuling <mikey@neuling.org>
> Signed-off-by: Steven Rostedt <srostedt@redhat.com>

Steve,

Thanks, fixes the error I was seeing.  

As an aside, is there anyway we can merge some of the code in
arch/powerpc/kernel/ftrace.c between 32 and 64bit?  There seems to be a
lot of repeated code in there with only minor changes.

Mikey

> ---
>  arch/powerpc/kernel/ftrace.c |   16 +++++++++++++++-
>  1 files changed, 15 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/ftrace.c b/arch/powerpc/kernel/ftrace.c
> index a913f91..88c641d 100644
> --- a/arch/powerpc/kernel/ftrace.c
> +++ b/arch/powerpc/kernel/ftrace.c
> @@ -113,6 +113,8 @@ static int test_24bit_addr(unsigned long ip, unsigned lon
g addr)
>  	return create_branch((unsigned int *)ip, addr, 0);
>  }
>  
> +#ifdef CONFIG_MODULES
> +
>  static int is_bl_op(unsigned int op)
>  {
>  	return (op & 0xfc000003) == 0x48000001;
> @@ -323,6 +325,7 @@ __ftrace_make_nop(struct module *mod,
>  	return 0;
>  }
>  #endif /* PPC64 */
> +#endif /* CONFIG_MODULES */
>  
>  int ftrace_make_nop(struct module *mod,
>  		    struct dyn_ftrace *rec, unsigned long addr)
> @@ -342,6 +345,7 @@ int ftrace_make_nop(struct module *mod,
>  		return ftrace_modify_code(ip, old, new);
>  	}
>  
> +#ifdef CONFIG_MODULES
>  	/*
>  	 * Out of range jumps are called from modules.
>  	 * We should either already have a pointer to the module
> @@ -366,9 +370,13 @@ int ftrace_make_nop(struct module *mod,
>  		mod = rec->arch.mod;
>  
>  	return __ftrace_make_nop(mod, rec, addr);
> -
> +#else
> +	/* We should not get here without modules */
> +	return -EINVAL;
> +#endif /* CONFIG_MODULES */
>  }
>  
> +#ifdef CONFIG_MODULES
>  #ifdef CONFIG_PPC64
>  static int
>  __ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
> @@ -457,6 +465,7 @@ __ftrace_make_call(struct dyn_ftrace *rec, unsigned long 
addr)
>  	return 0;
>  }
>  #endif /* CONFIG_PPC64 */
> +#endif /* CONFIG_MODULES */
>  
>  int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
>  {
> @@ -475,6 +484,7 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned lon
g addr)
>  		return ftrace_modify_code(ip, old, new);
>  	}
>  
> +#ifdef CONFIG_MODULES
>  	/*
>  	 * Out of range jumps are called from modules.
>  	 * Being that we are converting from nop, it had better
> @@ -486,6 +496,10 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned lo
ng addr)
>  	}
>  
>  	return __ftrace_make_call(rec, addr);
> +#else
> +	/* We should not get here without modules */
> +	return -EINVAL;
> +#endif /* CONFIG_MODULES */
>  }
>  
>  int ftrace_update_ftrace_func(ftrace_func_t func)
> -- 
> 1.5.6.5
> 
> -- 
> 

Re: [PATCH 2/2] powerpc, ftrace: fix compile error when modules not configured

[Steven Rostedt]

On Fri, 2009-02-06 at 17:23 +1100, Michael Neuling wrote:
> In message <20090206060527.369616736@goodmis.org> you wrote:
> > From: Steven Rostedt <srostedt@redhat.com>
> > 
> > Michael Neuling reported a compile bug when dynamic ftrace was
> > configured in and modules were not. This was due to the ftrace
> > code referencing module specific structures.
> > 
> > Reported-by: Michael Neuling <mikey@neuling.org>
> > Signed-off-by: Steven Rostedt <srostedt@redhat.com>
> 
> Steve,
> 
> Thanks, fixes the error I was seeing.  

Cool.

> 
> As an aside, is there anyway we can merge some of the code in
> arch/powerpc/kernel/ftrace.c between 32 and 64bit?  There seems to be a
> lot of repeated code in there with only minor changes.

It looks similar, but there's enough differences that I would like to
keep them separate. The code is complex enough and has a lot of subtle
differences between the two archs.

-- Steve