[PATCH] ftrace, powerpc64: fix math to calculate offset in TOC

[PATCH] ftrace, powerpc64: fix math to calculate offset in TOC

[Steven Rostedt]

Paul,

I found the bug that was causing large modules to fail in setting
up dynamic ftrace. It wound up being a simple math error. To calculate
the offset in the TOC, I had used an OR, but the bottom half was
a signed extended short, and it should have been an addition.
The fix is in my tree below, as well as posted here.

-- Steve


The following patch is in:

  git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git

    branch: ppc/ftrace


Steven Rostedt (1):
      ftrace, powerpc64: fix math to calculate offset in TOC

----
 arch/powerpc/kernel/ftrace.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)
---------------------------
commit 60e611577d908119bb515ead908a46b025a4c9f9
Author: Steven Rostedt <srostedt@redhat.com>
Date:   Sat Feb 7 22:00:26 2009 -0800

    ftrace, powerpc64: fix math to calculate offset in TOC
    
    Impact: fix dynamic ftrace with large modules in PPC64
    
    The math to calculate the offset into the TOC that is taken from reading
    the trampoline is incorrect. The bottom half of the offset is a signed
    extended short. The current code was using an OR to create the offset
    when it should have been using an addition.
    
    Signed-off-by: Steven Rostedt <srostedt@redhat.com>

diff --git a/arch/powerpc/kernel/ftrace.c b/arch/powerpc/kernel/ftrace.c
index 88c641d..4112175 100644
--- a/arch/powerpc/kernel/ftrace.c
+++ b/arch/powerpc/kernel/ftrace.c
@@ -191,8 +191,9 @@ __ftrace_make_nop(struct module *mod,
 		return -EINVAL;
 	}
 
-	offset = (unsigned)((unsigned short)jmp[0]) << 16 |
-		(unsigned)((unsigned short)jmp[1]);
+	/* The bottom half is signed extended */
+	offset = ((unsigned)((unsigned short)jmp[0]) << 16) +
+		(int)((short)jmp[1]);
 
 	pr_debug(" %x ", offset);
 

Re: [PATCH] ftrace, powerpc64: fix math to calculate offset in TOC

[Ingo Molnar]

* Steven Rostedt <rostedt@goodmis.org> wrote:

> 
> Paul,
> 
> I found the bug that was causing large modules to fail in setting
> up dynamic ftrace. It wound up being a simple math error. To calculate
> the offset in the TOC, I had used an OR, but the bottom half was
> a signed extended short, and it should have been an addition.
> The fix is in my tree below, as well as posted here.

Looks like a .29 candidate?

	Ingo

Re: [PATCH] ftrace, powerpc64: fix math to calculate offset in TOC

[Remis Lima Baima]

I have just tried the patch here and everything worked great!
Very well done.

On Mon, Feb 9, 2009 at 10:40 AM, Ingo Molnar <mingo@elte.hu> wrote:
>
> * Steven Rostedt <rostedt@goodmis.org> wrote:
>
>>
>> Paul,
>>
>> I found the bug that was causing large modules to fail in setting
>> up dynamic ftrace. It wound up being a simple math error. To calculate
>> the offset in the TOC, I had used an OR, but the bottom half was
>> a signed extended short, and it should have been an addition.
>> The fix is in my tree below, as well as posted here.
>
> Looks like a .29 candidate?
>
>        Ingo

I agree. A functional ("out-of-the-box") dynamic-ftrace helps quite a lot ;-)


Regards,
Remis

Re: [PATCH] ftrace, powerpc64: fix math to calculate offset in TOC

[Steven Rostedt]

On Mon, 9 Feb 2009, Ingo Molnar wrote:

> 
> * Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > 
> > Paul,
> > 
> > I found the bug that was causing large modules to fail in setting
> > up dynamic ftrace. It wound up being a simple math error. To calculate
> > the offset in the TOC, I had used an OR, but the bottom half was
> > a signed extended short, and it should have been an addition.
> > The fix is in my tree below, as well as posted here.
> 
> Looks like a .29 candidate?

Ingo,

I think Ben pulled them into their tree. The changes are specific to ppc, 
and will probably get better user testing there. Unless you feel that we 
have a bit of ppc testers using tip.

-- Steve

Re: [PATCH] ftrace, powerpc64: fix math to calculate offset in TOC

[Ingo Molnar]

* Steven Rostedt <rostedt@goodmis.org> wrote:

> 
> On Mon, 9 Feb 2009, Ingo Molnar wrote:
> 
> > 
> > * Steven Rostedt <rostedt@goodmis.org> wrote:
> > 
> > > 
> > > Paul,
> > > 
> > > I found the bug that was causing large modules to fail in setting
> > > up dynamic ftrace. It wound up being a simple math error. To calculate
> > > the offset in the TOC, I had used an OR, but the bottom half was
> > > a signed extended short, and it should have been an addition.
> > > The fix is in my tree below, as well as posted here.
> > 
> > Looks like a .29 candidate?
> 
> Ingo,
> 
> I think Ben pulled them into their tree. The changes are specific to ppc, 
> and will probably get better user testing there. Unless you feel that we 
> have a bit of ppc testers using tip.

no, that's fine - just wanted to make sure it goes upstream via some method.

	Ingo

Re: [PATCH] ftrace, powerpc64: fix math to calculate offset in TOC

[Geoff Levand]

Steven Rostedt wrote:
> I found the bug that was causing large modules to fail in setting
> up dynamic ftrace. It wound up being a simple math error. To calculate
> the offset in the TOC, I had used an OR, but the bottom half was
> a signed extended short, and it should have been an addition.
> The fix is in my tree below, as well as posted here.
> ----
>  arch/powerpc/kernel/ftrace.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)

I tested this on PS3 with CONFIG_FTRACE_SELFTEST=y and
CONFIG_FTRACE_SELFTEST=y, and all modules now load and
the system seems to work properly.  I did not run any
tracer specific tests though.

Acked-by: Geoff Levand <geoffrey.levand@am.sony.com>

Re: [PATCH] ftrace, powerpc64: fix math to calculate offset in TOC

[Benjamin Herrenschmidt]

On Mon, 2009-02-09 at 10:40 +0100, Ingo Molnar wrote:
> * Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > 
> > Paul,
> > 
> > I found the bug that was causing large modules to fail in setting
> > up dynamic ftrace. It wound up being a simple math error. To calculate
> > the offset in the TOC, I had used an OR, but the bottom half was
> > a signed extended short, and it should have been an addition.
> > The fix is in my tree below, as well as posted here.
> 
> Looks like a .29 candidate?

Yup, I was planning to send it to Linus along with a handful of other
powerpc fixes today, unless you prefer taking it yourself.

Cheers,
Ben.