Re: [PATCH] ftrace: Disable function tracing during suspend/resume and hibernation, again

[Steven Rostedt <rostedt@goodmis.org>]

On Mon, 2012-05-21 at 23:55 +0200, Rafael J. Wysocki wrote:
> On Monday, May 21, 2012, Srivatsa S. Bhat wrote:
> > If function tracing is enabled for some of the low-level suspend/resume
> > functions, it leads to triple fault during resume from suspend, ultimately
> > ending up in a reboot instead of a resume (or a total refusal to come out
> > of suspended state, on some machines).
> > 
> > This issue was explained in more detail in commit f42ac38c59e0a03d (ftrace:
> > disable tracing for suspend to ram). However, the changes made by that commit
> > got reverted by commit cbe2f5a6e84eebb (tracing: allow tracing of
> > suspend/resume & hibernation code again). So, unfortunately since things are
> > not yet robust enough to allow tracing of low-level suspend/resume functions,
> > suspend/resume is still broken when ftrace is enabled.
> > 
> > So fix this by disabling function tracing during suspend/resume & hibernation.
> 
> Steve, any comments?

I was actually the one to recommend this solution ;-)

But that said, I actually do have some.

> 
> > Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> > Cc: stable@vger.kernel.org
> > ---
> > 
> >  kernel/power/hibernate.c |    6 ++++++
> >  kernel/power/suspend.c   |    3 +++
> >  2 files changed, 9 insertions(+), 0 deletions(-)
> > 
> > diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> > index e09dfbf..52a1817 100644
> > --- a/kernel/power/hibernate.c
> > +++ b/kernel/power/hibernate.c
> > @@ -352,6 +352,7 @@ int hibernation_snapshot(int platform_mode)
> >  	}
> >  
> >  	suspend_console();
> > +	ftrace_stop();


Is this a point of no return?

This probably isn't a problem, but I'm being a little paranoid. As
ftrace_stop/start() is just an on off switch, anyplace that calls
ftrace_start() will enable function tracing again. Ftrace_start() is
called by tracing_start() which is called when
the /sys/kernel/debug/tracing/trace file is opened and then closed. If
the close happens while the suspend is starting, it possibly can start
function tracing again.

But this race would have to be done by the user, which should have full
control as they are in the process of suspending the machine. But they
may do it by accident.

Hmm, actually, I don't even think the ftrace_stop() is needed by
tracing_stop() anymore. I should just nuke it.

-- Steve


> >  	pm_restrict_gfp_mask();
> >  
> >  	error = dpm_suspend(PMSG_FREEZE);
> > @@ -377,6 +378,7 @@ int hibernation_snapshot(int platform_mode)
> >  	if (error || !in_suspend)
> >  		pm_restore_gfp_mask();
> >  
> > +	ftrace_start();
> >  	resume_console();
> >  	dpm_complete(msg);
> >  
> > @@ -479,6 +481,7 @@ int hibernation_restore(int platform_mode)
> >  
> >  	pm_prepare_console();
> >  	suspend_console();
> > +	ftrace_stop();
> >  	pm_restrict_gfp_mask();
> >  	error = dpm_suspend_start(PMSG_QUIESCE);
> >  	if (!error) {
> > @@ -486,6 +489,7 @@ int hibernation_restore(int platform_mode)
> >  		dpm_resume_end(PMSG_RECOVER);
> >  	}
> >  	pm_restore_gfp_mask();
> > +	ftrace_start();
> >  	resume_console();
> >  	pm_restore_console();
> >  	return error;
> > @@ -512,6 +516,7 @@ int hibernation_platform_enter(void)
> >  
> >  	entering_platform_hibernation = true;
> >  	suspend_console();
> > +	ftrace_stop();
> >  	error = dpm_suspend_start(PMSG_HIBERNATE);
> >  	if (error) {
> >  		if (hibernation_ops->recover)
> > @@ -555,6 +560,7 @@ int hibernation_platform_enter(void)
> >   Resume_devices:
> >  	entering_platform_hibernation = false;
> >  	dpm_resume_end(PMSG_RESTORE);
> > +	ftrace_start();
> >  	resume_console();
> >  
> >   Close:
> > diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> > index 396d262..c8b7446 100644
> > --- a/kernel/power/suspend.c
> > +++ b/kernel/power/suspend.c
> > @@ -24,6 +24,7 @@
> >  #include <linux/export.h>
> >  #include <linux/suspend.h>
> >  #include <linux/syscore_ops.h>
> > +#include <linux/ftrace.h>
> >  #include <trace/events/power.h>
> >  
> >  #include "power.h"
> > @@ -212,6 +213,7 @@ int suspend_devices_and_enter(suspend_state_t state)
> >  			goto Close;
> >  	}
> >  	suspend_console();
> > +	ftrace_stop();
> >  	suspend_test_start();
> >  	error = dpm_suspend_start(PMSG_SUSPEND);
> >  	if (error) {
> > @@ -231,6 +233,7 @@ int suspend_devices_and_enter(suspend_state_t state)
> >  	suspend_test_start();
> >  	dpm_resume_end(PMSG_RESUME);
> >  	suspend_test_finish("resume devices");
> > +	ftrace_start();
> >  	resume_console();
> >   Close:
> >  	if (suspend_ops->end)
> > 
> > 
> >