Message ID | 20090213150147.180922180@goodmis.org (mailing list archive) |
---|---|
State | Accepted, archived |
Commit | bb9b903527eb16c8fdad59a562c29e89f5dcf233 |
Delegated to: | Benjamin Herrenschmidt |
Headers | show |
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
diff --git a/arch/powerpc/kernel/ftrace.c b/arch/powerpc/kernel/ftrace.c index 610c852..4c75a1c 100644 --- a/arch/powerpc/kernel/ftrace.c +++ b/arch/powerpc/kernel/ftrace.c @@ -31,11 +31,6 @@ #endif #ifdef CONFIG_DYNAMIC_FTRACE -static unsigned int ftrace_calc_offset(long ip, long addr) -{ - return (int)(addr - ip); -} - static unsigned int ftrace_nop_replace(void) { return PPC_NOP_INSTR; @@ -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); return op; }