Re: [RFC][PATCHv5 06/13] printk: register PM notifier

[Rafael J. Wysocki]

On Wednesday, August 16, 2017 9:31:17 AM CEST Sergey Senozhatsky wrote:
> On (08/15/17 13:51), Rafael J. Wysocki wrote:
> > On Tuesday, August 15, 2017 4:56:18 AM CEST Sergey Senozhatsky wrote:
> [..]
> > > This patch registers PM notifier, so PM can switch printk
> > > to emergency mode from PM_FOO_PREPARE notifiers and return
> > 
> > Isn't that too early?  That's before user space is frozen even.
> > 
> > > back to printk threaded mode from PM_POST_FOO notifiers.
> > 
> > And isn't that too late?
> 
> hm, those two are interesting questions. in short - well, it might
> be. I don't want to interfere with PM by doing 'accidental' offloading
> etc., PM is too complicated already. so I'd prefer to switch to old
> printk behavior early (besides, I tend to see lockups reports more
> often when the kernel is up and running, rather than during PM events.)
> but, once again, may be it is too early and we can move emergency_mode
> switch.

Well, that depends on what your goal is really.

I thought you wanted to do the offloading as far into the suspend as it
was safe to do (and analogously for resume), but now I see you want to
stop doing it as early as it makes sense. :-)

In that case I would call printk_emergency_begin_sync() from
dpm_prepare() and printk_emergency_end_sync() from dpm_complete().

> 
> [..]
> > > +static int printk_pm_notify(struct notifier_block *notify_block,
> > > +			    unsigned long mode, void *unused)
> > > +{
> > > +	switch (mode) {
> > > +	case PM_HIBERNATION_PREPARE:
> > > +	case PM_SUSPEND_PREPARE:
> > > +	case PM_RESTORE_PREPARE:
> > > +		printk_emergency_begin_sync();
> > 
> > I'm not sure what would be wrong with calling this directly
> > from dpm_suspend_noirq().
> > 
> > > +		break;
> > > +
> > > +	case PM_POST_SUSPEND:
> > > +	case PM_POST_HIBERNATION:
> > > +	case PM_POST_RESTORE:
> > > +		printk_emergency_end_sync();
> > 
> > And this could be called from dpm_resume_noirq().
> > 
> > In which case you wouldn't really need the stuff below.
> 
> we didn't want to spread printk_emergency_{begin, end}
> calls across the kernel.

But this adds one invocation of each of them anyway *plus* some
extra code around those.  Wouldn't it be cleaner to add those
invocations alone?

> 
> as of dpm_suspend_noirq/dpm_resume_noirq - I need to look more.
> isn't dpm_{suspend, resume}_noirq too late/early? :)
> 
> dpm_resume_noirq() happens much earlier than
> suspend_finish()->suspend_thaw_processes(), right?
> do we want to enable offloading this early?
> 
> currently what we have is the following sequence
> 
> suspend_finish()
>   suspend_thaw_processes()
>     pm_notifier_call_chain(PM_POST_SUSPEND)    // enable offloading
>       pm_restore_console()
> 
> which looks OK to me, frankly.
> do you see any problems here?

I just don't see much point in using the notifier thing if you can
achieve basically the same without using it. :-)

Thanks,
Rafael