[PATCH -tip] x86: ftrace.c declare prepare_ftrace_return before they get used

[PATCH -tip] x86: ftrace.c declare prepare_ftrace_return before they get used

[Jaswinder Singh Rajput]

Impact: fix sparse warning:
  arch/x86/kernel/ftrace.c:411:6: warning: symbol 'prepare_ftrace_return' was not declared. Should it be static?

Signed-off-by: Jaswinder Singh Rajput <jaswinderrajput@gmail.com>
---
 arch/x86/include/asm/ftrace.h |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h
index bd2c651..49616d4 100644
--- a/arch/x86/include/asm/ftrace.h
+++ b/arch/x86/include/asm/ftrace.h
@@ -62,4 +62,10 @@ struct dyn_arch_ftrace {
 #endif /* __ASSEMBLY__ */
 #endif /* CONFIG_FUNCTION_TRACER */
 
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+#ifndef __ASSEMBLY__
+extern void prepare_ftrace_return(unsigned long *, unsigned long);
+#endif
+#endif
+
 #endif /* _ASM_X86_FTRACE_H */
-- 
1.6.0.6

Re: [PATCH -tip] x86: ftrace.c declare prepare_ftrace_return before they get used

[Frederic Weisbecker]

On Thu, Apr 09, 2009 at 10:07:48PM +0530, Jaswinder Singh Rajput wrote:
> Impact: fix sparse warning:
>   arch/x86/kernel/ftrace.c:411:6: warning: symbol 'prepare_ftrace_return' was not declared. Should it be static?
> 
> Signed-off-by: Jaswinder Singh Rajput <jaswinderrajput@gmail.com>
> ---
>  arch/x86/include/asm/ftrace.h |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h
> index bd2c651..49616d4 100644
> --- a/arch/x86/include/asm/ftrace.h
> +++ b/arch/x86/include/asm/ftrace.h
> @@ -62,4 +62,10 @@ struct dyn_arch_ftrace {
>  #endif /* __ASSEMBLY__ */
>  #endif /* CONFIG_FUNCTION_TRACER */
>  
> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> +#ifndef __ASSEMBLY__
> +extern void prepare_ftrace_return(unsigned long *, unsigned long);


But it is only used from asm code so there is no need to make
its prototype public.

I don't think this is necessary but if you really think it is, then I would prefer
you use the already existing #ifndef __ASSEMBLY__ block.

Thanks,
Frederic.



> +#endif
> +#endif
> +
>  #endif /* _ASM_X86_FTRACE_H */
> -- 
> 1.6.0.6
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH -tip] x86: ftrace.c declare prepare_ftrace_return before they get used

[Jaswinder Singh Rajput]

On Thu, 2009-04-09 at 19:20 +0200, Frederic Weisbecker wrote:
> On Thu, Apr 09, 2009 at 10:07:48PM +0530, Jaswinder Singh Rajput wrote:
> > Impact: fix sparse warning:
> >   arch/x86/kernel/ftrace.c:411:6: warning: symbol 'prepare_ftrace_return' was not declared. Should it be static?
> > 
> > Signed-off-by: Jaswinder Singh Rajput <jaswinderrajput@gmail.com>
> > ---
> >  arch/x86/include/asm/ftrace.h |    6 ++++++
> >  1 files changed, 6 insertions(+), 0 deletions(-)
> > 
> > diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h
> > index bd2c651..49616d4 100644
> > --- a/arch/x86/include/asm/ftrace.h
> > +++ b/arch/x86/include/asm/ftrace.h
> > @@ -62,4 +62,10 @@ struct dyn_arch_ftrace {
> >  #endif /* __ASSEMBLY__ */
> >  #endif /* CONFIG_FUNCTION_TRACER */
> >  
> > +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> > +#ifndef __ASSEMBLY__
> > +extern void prepare_ftrace_return(unsigned long *, unsigned long);
> 
> 
> But it is only used from asm code so there is no need to make
> its prototype public.
> 
> I don't think this is necessary but if you really think it is, then I would prefer
> you use the already existing #ifndef __ASSEMBLY__ block.
> 

Like this:

Subject: [PATCH] x86: ftrace.c declare prepare_ftrace_return before they get used

Impact: fix sparse warning:
  arch/x86/kernel/ftrace.c:411:6: warning: symbol 'prepare_ftrace_return' was not declared. Should it be static?

Signed-off-by: Jaswinder Singh Rajput <jaswinderrajput@gmail.com>
---
 arch/x86/include/asm/ftrace.h |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h
index bd2c651..ddc9236 100644
--- a/arch/x86/include/asm/ftrace.h
+++ b/arch/x86/include/asm/ftrace.h
@@ -59,6 +59,11 @@ struct dyn_arch_ftrace {
 };
 
 #endif /*  CONFIG_DYNAMIC_FTRACE */
+
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+extern void prepare_ftrace_return(unsigned long *, unsigned long);
+#endif
+
 #endif /* __ASSEMBLY__ */
 #endif /* CONFIG_FUNCTION_TRACER */
 
-- 
1.6.0.6

Re: [PATCH -tip] x86: ftrace.c declare prepare_ftrace_return before they get used

[Steven Rostedt]

On Thu, 9 Apr 2009, Jaswinder Singh Rajput wrote:

> On Thu, 2009-04-09 at 19:20 +0200, Frederic Weisbecker wrote:
> > On Thu, Apr 09, 2009 at 10:07:48PM +0530, Jaswinder Singh Rajput wrote:
> > > Impact: fix sparse warning:
> > >   arch/x86/kernel/ftrace.c:411:6: warning: symbol 'prepare_ftrace_return' was not declared. Should it be static?
> > > 
> > > Signed-off-by: Jaswinder Singh Rajput <jaswinderrajput@gmail.com>
> > > ---
> > >  arch/x86/include/asm/ftrace.h |    6 ++++++
> > >  1 files changed, 6 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h
> > > index bd2c651..49616d4 100644
> > > --- a/arch/x86/include/asm/ftrace.h
> > > +++ b/arch/x86/include/asm/ftrace.h
> > > @@ -62,4 +62,10 @@ struct dyn_arch_ftrace {
> > >  #endif /* __ASSEMBLY__ */
> > >  #endif /* CONFIG_FUNCTION_TRACER */
> > >  
> > > +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> > > +#ifndef __ASSEMBLY__
> > > +extern void prepare_ftrace_return(unsigned long *, unsigned long);
> > 
> > 
> > But it is only used from asm code so there is no need to make
> > its prototype public.
> > 
> > I don't think this is necessary but if you really think it is, then I would prefer
> > you use the already existing #ifndef __ASSEMBLY__ block.
> > 

It is not necessary.

> 
> Like this:
> 
> Subject: [PATCH] x86: ftrace.c declare prepare_ftrace_return before they get used
> 
> Impact: fix sparse warning:
>   arch/x86/kernel/ftrace.c:411:6: warning: symbol 'prepare_ftrace_return' was not declared. Should it be static?
> 
> Signed-off-by: Jaswinder Singh Rajput <jaswinderrajput@gmail.com>
> ---
>  arch/x86/include/asm/ftrace.h |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h
> index bd2c651..ddc9236 100644
> --- a/arch/x86/include/asm/ftrace.h
> +++ b/arch/x86/include/asm/ftrace.h
> @@ -59,6 +59,11 @@ struct dyn_arch_ftrace {
>  };
>  
>  #endif /*  CONFIG_DYNAMIC_FTRACE */
> +
> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> +extern void prepare_ftrace_return(unsigned long *, unsigned long);
> +#endif
> +

The only caller is from assembly, so this patch is unnecessary. Unless we 
have some rule that all functions must have a prototype, even when it is 
only called from assembly.

-- Steve


>  #endif /* __ASSEMBLY__ */
>  #endif /* CONFIG_FUNCTION_TRACER */
>  
> -- 
> 1.6.0.6
> 
> 
> 
> 

Re: [PATCH -tip] x86: ftrace.c declare prepare_ftrace_return before they get used

[Jaswinder Singh Rajput]

On Thu, 2009-04-09 at 14:55 -0400, Steven Rostedt wrote:
> 
> On Thu, 9 Apr 2009, Jaswinder Singh Rajput wrote:
> 
> > 
> > Subject: [PATCH] x86: ftrace.c declare prepare_ftrace_return before they get used
> > 
> > Impact: fix sparse warning:
> >   arch/x86/kernel/ftrace.c:411:6: warning: symbol 'prepare_ftrace_return' was not declared. Should it be static?
> > 
> > Signed-off-by: Jaswinder Singh Rajput <jaswinderrajput@gmail.com>
> > ---
> >  arch/x86/include/asm/ftrace.h |    5 +++++
> >  1 files changed, 5 insertions(+), 0 deletions(-)
> > 
> > diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h
> > index bd2c651..ddc9236 100644
> > --- a/arch/x86/include/asm/ftrace.h
> > +++ b/arch/x86/include/asm/ftrace.h
> > @@ -59,6 +59,11 @@ struct dyn_arch_ftrace {
> >  };
> >  
> >  #endif /*  CONFIG_DYNAMIC_FTRACE */
> > +
> > +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> > +extern void prepare_ftrace_return(unsigned long *, unsigned long);
> > +#endif
> > +
> 
> The only caller is from assembly, so this patch is unnecessary. Unless we 
> have some rule that all functions must have a prototype, even when it is 
> only called from assembly.
> 

Then:

diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index 18dfa30..75c0682 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -408,6 +408,7 @@ int ftrace_disable_ftrace_graph_caller(void)
  * Hook the return address and push it in the stack of return addrs
  * in current thread info.
  */
+asmlinkage
 void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr)
 {
        unsigned long old;


Josh: Do you think sparse should not warn like for this case.
What will be the best solution for this case.

--
JSR

Re: [PATCH -tip] x86: ftrace.c declare prepare_ftrace_return before they get used

[Frederic Weisbecker]

On Fri, Apr 10, 2009 at 09:59:55AM +0530, Jaswinder Singh Rajput wrote:
> On Thu, 2009-04-09 at 14:55 -0400, Steven Rostedt wrote:
> > 
> > On Thu, 9 Apr 2009, Jaswinder Singh Rajput wrote:
> > 
> > > 
> > > Subject: [PATCH] x86: ftrace.c declare prepare_ftrace_return before they get used
> > > 
> > > Impact: fix sparse warning:
> > >   arch/x86/kernel/ftrace.c:411:6: warning: symbol 'prepare_ftrace_return' was not declared. Should it be static?
> > > 
> > > Signed-off-by: Jaswinder Singh Rajput <jaswinderrajput@gmail.com>
> > > ---
> > >  arch/x86/include/asm/ftrace.h |    5 +++++
> > >  1 files changed, 5 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h
> > > index bd2c651..ddc9236 100644
> > > --- a/arch/x86/include/asm/ftrace.h
> > > +++ b/arch/x86/include/asm/ftrace.h
> > > @@ -59,6 +59,11 @@ struct dyn_arch_ftrace {
> > >  };
> > >  
> > >  #endif /*  CONFIG_DYNAMIC_FTRACE */
> > > +
> > > +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> > > +extern void prepare_ftrace_return(unsigned long *, unsigned long);
> > > +#endif
> > > +
> > 
> > The only caller is from assembly, so this patch is unnecessary. Unless we 
> > have some rule that all functions must have a prototype, even when it is 
> > only called from assembly.
> > 
> 
> Then:
> 
> diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
> index 18dfa30..75c0682 100644
> --- a/arch/x86/kernel/ftrace.c
> +++ b/arch/x86/kernel/ftrace.c
> @@ -408,6 +408,7 @@ int ftrace_disable_ftrace_graph_caller(void)
>   * Hook the return address and push it in the stack of return addrs
>   * in current thread info.
>   */
> +asmlinkage



No. This is how prepare_ftrace_return was called at first.
But note that prepare_ftrace_return may be called for every
kernel function while tracing, hence every pico optimization
is important. asmlinkage will push the arguments to the stack
while we want to keep the fastcall here by passing them to the
registers.

Sparse is a useful tool, but sometime there are exceptions when its
warnings should be ignored IMO.

Frederic.

>  void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr)
>  {
>         unsigned long old;
> 
> 
> Josh: Do you think sparse should not warn like for this case.
> What will be the best solution for this case.
> 
> --
> JSR
> 
>