* [PATCH -tip] x86: ftrace.c declare prepare_ftrace_return before they get used @ 2009-04-09 16:37 Jaswinder Singh Rajput 2009-04-09 17:20 ` Frederic Weisbecker 0 siblings, 1 reply; 6+ messages in thread From: Jaswinder Singh Rajput @ 2009-04-09 16:37 UTC (permalink / raw) To: Ingo Molnar, Steven Rostedt, x86 maintainers, LKML 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH -tip] x86: ftrace.c declare prepare_ftrace_return before they get used 2009-04-09 16:37 [PATCH -tip] x86: ftrace.c declare prepare_ftrace_return before they get used Jaswinder Singh Rajput @ 2009-04-09 17:20 ` Frederic Weisbecker 2009-04-09 17:41 ` Jaswinder Singh Rajput 0 siblings, 1 reply; 6+ messages in thread From: Frederic Weisbecker @ 2009-04-09 17:20 UTC (permalink / raw) To: Jaswinder Singh Rajput; +Cc: Ingo Molnar, Steven Rostedt, x86 maintainers, LKML 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/ ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH -tip] x86: ftrace.c declare prepare_ftrace_return before they get used 2009-04-09 17:20 ` Frederic Weisbecker @ 2009-04-09 17:41 ` Jaswinder Singh Rajput 2009-04-09 18:55 ` Steven Rostedt 0 siblings, 1 reply; 6+ messages in thread From: Jaswinder Singh Rajput @ 2009-04-09 17:41 UTC (permalink / raw) To: Frederic Weisbecker; +Cc: Ingo Molnar, Steven Rostedt, x86 maintainers, LKML 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH -tip] x86: ftrace.c declare prepare_ftrace_return before they get used 2009-04-09 17:41 ` Jaswinder Singh Rajput @ 2009-04-09 18:55 ` Steven Rostedt 2009-04-10 4:29 ` Jaswinder Singh Rajput 0 siblings, 1 reply; 6+ messages in thread From: Steven Rostedt @ 2009-04-09 18:55 UTC (permalink / raw) To: Jaswinder Singh Rajput Cc: Frederic Weisbecker, Ingo Molnar, x86 maintainers, LKML 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 > > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH -tip] x86: ftrace.c declare prepare_ftrace_return before they get used 2009-04-09 18:55 ` Steven Rostedt @ 2009-04-10 4:29 ` Jaswinder Singh Rajput 2009-04-10 12:58 ` Frederic Weisbecker 0 siblings, 1 reply; 6+ messages in thread From: Jaswinder Singh Rajput @ 2009-04-10 4:29 UTC (permalink / raw) To: Steven Rostedt Cc: Frederic Weisbecker, Ingo Molnar, x86 maintainers, LKML, josh 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH -tip] x86: ftrace.c declare prepare_ftrace_return before they get used 2009-04-10 4:29 ` Jaswinder Singh Rajput @ 2009-04-10 12:58 ` Frederic Weisbecker 0 siblings, 0 replies; 6+ messages in thread From: Frederic Weisbecker @ 2009-04-10 12:58 UTC (permalink / raw) To: Jaswinder Singh Rajput Cc: Steven Rostedt, Ingo Molnar, x86 maintainers, LKML, josh 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 > > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2009-04-10 12:58 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-04-09 16:37 [PATCH -tip] x86: ftrace.c declare prepare_ftrace_return before they get used Jaswinder Singh Rajput 2009-04-09 17:20 ` Frederic Weisbecker 2009-04-09 17:41 ` Jaswinder Singh Rajput 2009-04-09 18:55 ` Steven Rostedt 2009-04-10 4:29 ` Jaswinder Singh Rajput 2009-04-10 12:58 ` Frederic Weisbecker
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox