Re: [PATCH] serial: 8250: don't attempt a trylock if in sysrq

[Sebastian Andrzej Siewior <bigeasy@linutronix.de>]

On 11/14/2014 07:00 PM, Rabin Vincent wrote:
> Attempting to use SysRq via the 8250 serial port with spin lock
> debugging on on a uniprocessor system results in the following splat:
>
>  SysRq :
>  BUG: spinlock trylock failure on UP on CPU#0, swapper/0
>   lock: serial8250_ports+0x0/0x8c0, .magic: dead4ead, .owner: swapper/0, .owner_cpu: 0
>  Call Trace:
>   <IRQ>  [<ffffffff812c8d27>] dump_stack+0x19/0x1b
>   [<ffffffff8106812e>] spin_dump+0x7e/0xd0
>   [<ffffffff810681a6>] spin_bug+0x26/0x30
>   [<ffffffff8106843c>] do_raw_spin_trylock+0x4c/0x60
>   [<ffffffff812cdb1d>] _raw_spin_trylock+0x1d/0x60
>   [<ffffffff812336d8>] serial8250_console_write+0x68/0x190
>   [<ffffffff811eb0b0>] ? sprintf+0x40/0x50
>   [<ffffffff8106ab5e>] call_console_drivers.constprop.11+0x9e/0xf0
>   [<ffffffff8106b276>] console_unlock+0x3e6/0x490
>   [<ffffffff8106b595>] vprintk_emit+0x275/0x530
>   [<ffffffff812c869a>] printk+0x4d/0x4f
>   [<ffffffff8121e612>] __handle_sysrq+0x62/0x1b0
>   [<ffffffff8121ebc6>] handle_sysrq+0x26/0x30
>   [<ffffffff81233157>] serial8250_rx_chars+0x1d7/0x250
>   [<ffffffff812338bb>] serial8250_handle_irq+0x7b/0x90
>   [<ffffffff812338f3>] serial8250_default_handle_irq+0x23/0x30
>   [<ffffffff812318b3>] serial8250_interrupt+0x63/0xe0
>   [<ffffffff8106d80e>] handle_irq_event_percpu+0x4e/0x200

>  HELP : loglevel(0-9) reboot(b) crash(c) show-all-locks(d) te...
> 
> Before ebade5e833eda30 ("serial: 8250: Clean up the locking for -rt")
> this was handled by not even attempting to try the lock if port->sysrq,
> since it is known to be taken by the interrupt handler; see
> https://bugzilla.kernel.org/show_bug.cgi?id=6716#c1.  Restore that
> behavior.

Given the callchain it is expected that the lock is already taken.
While this splat is easy to trigger in the sysrq way you might get the
same splat in the oops_in_progress case which would be still okay.

While this is harmless to apply, I wonder if it is worth the effort to
teach lockdep that in this case it would be okay if the try_lock fails.

> Signed-off-by: Rabin Vincent <rabin@rab.in>
> ---
>  drivers/tty/serial/8250/8250_core.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
> index ca5cfdc..aec51eb 100644
> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -3198,7 +3198,9 @@ serial8250_console_write(struct console *co, const char *s, unsigned int count)
>  
>  	serial8250_rpm_get(up);
>  
> -	if (port->sysrq || oops_in_progress)
> +	if (port->sysrq)
> +		locked = 0;
> +	else if (oops_in_progress)
>  		locked = spin_trylock_irqsave(&port->lock, flags);
>  	else
>  		spin_lock_irqsave(&port->lock, flags);
> 

Sebastian