Re: [RFC][PATCHv3 5/5] printk: register PM notifier

From: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
To: Petr Mladek <pmladek@suse.com>
Cc: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>,
	Steven Rostedt <rostedt@goodmis.org>, Jan Kara <jack@suse.cz>,
	Andrew Morton <akpm@linux-foundation.org>,
	Peter Zijlstra <peterz@infradead.org>,
	"Rafael J . Wysocki" <rjw@rjwysocki.net>,
	Eric Biederman <ebiederm@xmission.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Jiri Slaby <jslaby@suse.com>, Pavel Machek <pavel@ucw.cz>,
	Andreas Mohr <andi@lisas.de>,
	Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>,
	linux-kernel@vger.kernel.org,
	Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
Subject: Re: [RFC][PATCHv3 5/5] printk: register PM notifier
Date: Wed, 31 May 2017 15:42:24 +0900	[thread overview]
Message-ID: <20170531064224.GB7672@jagdpanzerIV.localdomain> (raw)
In-Reply-To: <20170530095521.GC30903@pathway.suse.cz>

Hello Petr,

thanks for taking a look.

On (05/30/17 11:55), Petr Mladek wrote:
[..]
> My understanding is that notifiers should return NOTIFY_OK
> when they handled it and NOTIFY_DONE when they did nothing.
> For example, see cpu_hotplug_pm_callback().
> 
> In reality, there does not seem to be a big difference,
> see notifier_to_errno() in __pm_notifier_call_chain().
> 
> Anyway, I would try to follow the cpu_hotplug_pm_callback()
> example.

good points, will fix.

> > +	WARN_ON(register_pm_notifier(&printk_pm_nb) != 0);
> 
> I think that a simple error message might be enough.
> 
> Also we might want to force the emergency mode in case of error.
> Otherwise, the messages will not appear during suspend and such
> operations.

good point, will fix.

> BTW: Do you know about some locations that would need to be
> patched explicitly, for example, kexec?

a tricky question and I need to think more. I'm a bit overloaded
with some other stuff now, sorry for long replies.

in case of kexec_image->preserve_context my assumption was that
PM would send out PM notifications. but it seems that I need to have
a second look. quite possible we will have to register yet another
PM notifier - add syscore_ops to syscore_ops_list via
register_syscore_ops() - the one that would be called from
syscore_suspend().

the other branch invokes kernel_restart_prepare(), which updates
`system_statem' variable and we test it in console_offload_printing().

some other cases also update `system_statem'. the exception is maybe
sysrq emergency restart. but I'm a bit uncertain here. on one hand the
expected emergency restart behavior is "just reboot the system, OK?",
on the other hand there are people who expect kernel log messages to
be flushed before that "just reboot the system" thing.

> Some old versions of this patch touched console_suspend().
> This brought me to snapshot_ioctl(). It looks like an
> API that allows to create snapshots from user space.
> I wonder if we should switch to the emergency mode
> there are well, probably in the SNAPSHOT_FREEZE
> stage.

my assumption was that PM ioctl sends out PM notifications.
see snapshot_open()/snapshot_release().

	-ss