On Thu 14-07-16 23:34:50, Sergey Senozhatsky wrote:
> Hello Jan,
>
> On (07/14/16 16:12), Jan Kara wrote:
> [..]
> > > *** a printk() call from here will kill the system. either it will
> > > recurse printk(), or spin forever in 'nested' printk() on one of
> > > the already taken spin locks.
> [..]
> > And with sync printk the above deadlock doesn't trigger only by chance - if
> > there happened to be a waiter on console_sem while we suspend, the same
> > deadlock would trigger because up(&console_sem) will try to wake him up and
> > the warning in timekeeping code will cause recursive printk.
> >
> > So I think your patch doesn't really address the real issue - it only
> > works around the particular WARN_ON(timekeeping_enabled) warning but if
> > there was a different warning in timekeeping code which would trigger, it
> > has a potential for causing recursive printk deadlock (and indeed we had
> > such issues previously - see e.g. 504d58745c9c "timer: Fix lock inversion
> > between hrtimer_bases.lock and scheduler locks").
>
> we switch to sync printk in suspend_console(), that is happening
> long before we start bringing cpu downs
>
> suspend_devices_and_enter()
> suspend_console()
> ...
> suspend_enter()
> ...
> dpm_suspend_late
> ...
> disable_nonboot_cpus
>
>
>
> and cpu_down() in printk does
>
> static int console_cpu_notify(struct notifier_block *self,
> unsigned long action, void *hcpu)
> {
> switch (action) {
> case CPU_ONLINE:
> case CPU_DEAD:
> case CPU_DOWN_FAILED:
> case CPU_UP_CANCELED:
> console_lock();
> console_unlock();
> }
> return NOTIFY_OK;
> }
>
> so I think this console_lock() sort of guarantees that there should be
> no sleeping tasks in console semaphore wait list. or am I missing something?
No, probably you're right - unless there would be a CPU notifier executed
after console_cpu_notify() which would try to acquire console_sem for some
reason. But that is a wild speculation and I tend to agree that in
synchronous printk case and current code the wakeup cannot happen.
But my point really is that I don't see why changing process state (which
is what wakeup actually is) should be problematic even this late during
suspend...
Honza
--
Jan Kara <j...@suse.com>
SUSE Labs, CR
