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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1933345.ycXYzBI3bM@aspire.rjw.lan>
Date:   Wed, 16 Aug 2017 14:58:15 +0200
From:   "Rafael J. Wysocki" <rjw@...ysocki.net>
To:     Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
Cc:     Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        Petr Mladek <pmladek@...e.com>,
        Steven Rostedt <rostedt@...dmis.org>, Jan Kara <jack@...e.cz>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Eric Biederman <ebiederm@...ssion.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jiri Slaby <jslaby@...e.com>, Pavel Machek <pavel@....cz>,
        Andreas Mohr <andi@...as.de>,
        Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>,
        linux-kernel@...r.kernel.org
Subject: Re: [RFC][PATCHv5 06/13] printk: register PM notifier

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