[2/2] powerpc, ftrace: use create_branch lib function

From: Steven Rostedt <srostedt@redhat.com>

Impact: clean up, remove duplicate code

When ftrace was first ported to PowerPC, there existed a
create_function_call that would create the instruction to make a call
to a given address. Unfortunately, this call expected to write to
the address it was given, and since it used the address to calculate
the offset, it could not be faked.

ftrace needed a way to create the instruction without actually writing
that instruction to the text section. So ftrace had to implement its
own code.

Now we have create_branch in the code patching library, which does
exactly what ftrace needs. This patch replaces ftrace's implementation
with the library function.

Signed-off-by: Steven Rostedt <srostedt@redhat.com>
---
 arch/powerpc/kernel/ftrace.c |   14 +-------------
 1 files changed, 1 insertions(+), 13 deletions(-)

On Fri, 2009-02-13 at 10:00 -0500, Steven Rostedt wrote:
> plain text document attachment
> (0002-powerpc-ftrace-use-create_branch-lib-function.patch)
> From: Steven Rostedt <srostedt@redhat.com>
> 
> Impact: clean up, remove duplicate code
> 
> When ftrace was first ported to PowerPC, there existed a
> create_function_call that would create the instruction to make a call
> to a given address. Unfortunately, this call expected to write to
> the address it was given, and since it used the address to calculate
> the offset, it could not be faked.
> 
> ftrace needed a way to create the instruction without actually writing
> that instruction to the text section. So ftrace had to implement its
> own code.
> 
> Now we have create_branch in the code patching library, which does
> exactly what ftrace needs. This patch replaces ftrace's implementation
> with the library function.

Thanks for doing this, I was going to once the ftrace code had settled a
little but you beat me to it.

> @@ -46,17 +41,10 @@ ftrace_call_replace(unsigned long ip, unsigned long addr, int link)
>  {
>  	unsigned int op;
>  
> -	/*
> -	 * It would be nice to just use create_function_call, but that will
> -	 * update the code itself. Here we need to just return the
> -	 * instruction that is going to be modified, without modifying the
> -	 * code.
> -	 */
>  	addr = GET_ADDR(addr);
>  
>  	/* if (link) set op to 'bl' else 'b' */
> -	op = 0x48000000 | (link ? 1 : 0);
> -	op |= (ftrace_calc_offset(ip, addr) & 0x03fffffc);
> +	op = create_branch((unsigned int *)ip, addr, link ? 1 : 0);

If I was feeling nit-picky I'd say you should use:

op = create_branch((unsigned int *)ip, addr, link ? BRANCH_SET_LINK : 0);


But admittedly we're unlikely to ever change the flag handling, so it's
probably not worth the effort of a respin (or this email :).

cheers

On Sun, 15 Feb 2009, Michael Ellerman wrote:
> >  
> >  	/* if (link) set op to 'bl' else 'b' */
> > -	op = 0x48000000 | (link ? 1 : 0);
> > -	op |= (ftrace_calc_offset(ip, addr) & 0x03fffffc);
> > +	op = create_branch((unsigned int *)ip, addr, link ? 1 : 0);
> 
> If I was feeling nit-picky I'd say you should use:
> 
> op = create_branch((unsigned int *)ip, addr, link ? BRANCH_SET_LINK : 0);
> 
> 
> But admittedly we're unlikely to ever change the flag handling, so it's
> probably not worth the effort of a respin (or this email :).

Not worth the effort of a respin, I agree.

But it would be worth the effort of a clean up patch ;-)

Not for "oh this flag might change", but for reviewers looking at the
code and knowing exactly what that '1' means.

-- Steve