Re: UART/TTY console deadlock

[Petr Mladek <pmladek@suse.com>]

On Tue 2020-06-30 19:55:12, Sergey Senozhatsky wrote:
> On (20/06/30 12:21), Petr Mladek wrote:
> > Anyway, I believe that this is a false positive. If I get it correctly
> > serial8250_do_startup() must be called before the serial port could
> > be registered as a console. It means that it could not be called
> > from inside printk().
> 
> >From my understanding, I'm afraid we are talking about actual deadlock
> here, not about false positive report. Quoting the original email:

IMHO, it is not clear. The log contains lockdep report. It does not mean
the deadlock really happened. The lockdep reports cyclic dependency but it
is not able to say if the related code chains might be called at the
same time in reality.

And this combination has signs of false positives. As I said, I think
that the serial console could not be registered by printk() before
serial8250_do_startup() has finished.

Also there are some unrelated messages after the last backtrace.

Finally. we would be able to see the messages only when panic() was
called and oops_in_progress set. And there is nothing about panic().


>  : We are trying an S3 suspend stress test and occasionally while
>  : entering S3 we get a console deadlock.

Raul, have the system really died or have you just seen the lockdep
report, please?


> [..]
> 
> > >  drivers/tty/serial/8250/8250_port.c | 11 +++++++----
> > >  1 file changed, 7 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> > > index d64ca77d9cfa..ad30991e1b3b 100644
> > > --- a/drivers/tty/serial/8250/8250_port.c
> > > +++ b/drivers/tty/serial/8250/8250_port.c
> > > @@ -2275,6 +2275,11 @@ int serial8250_do_startup(struct uart_port *port)
> > >  
> > >  	if (port->irq && !(up->port.flags & UPF_NO_THRE_TEST)) {
> > >  		unsigned char iir1;
> > > +		bool irq_shared = up->port.irqflags & IRQF_SHARED;
> > > +
> > > +		if (irq_shared)
> > > +			disable_irq_nosync(port->irq);
> > > +
> > >  		/*
> > >  		 * Test for UARTs that do not reassert THRE when the
> > >  		 * transmitter is idle and the interrupt has already
> > > @@ -2284,8 +2289,6 @@ int serial8250_do_startup(struct uart_port *port)
> > >  		 * allow register changes to become visible.
> > >  		 */
> > >  		spin_lock_irqsave(&port->lock, flags);
> > > -		if (up->port.irqflags & IRQF_SHARED)
> > > -			disable_irq_nosync(port->irq);
> > >  
> > >  		wait_for_xmitr(up, UART_LSR_THRE);
> > >  		serial_port_out_sync(port, UART_IER, UART_IER_THRI);
> > > @@ -2297,9 +2300,9 @@ int serial8250_do_startup(struct uart_port *port)
> > >  		iir = serial_port_in(port, UART_IIR);
> > >  		serial_port_out(port, UART_IER, 0);
> > >  
> > > -		if (port->irqflags & IRQF_SHARED)
> > > -			enable_irq(port->irq);
> > >  		spin_unlock_irqrestore(&port->lock, flags);
> > > +		if (irq_shared)
> > > +			enable_irq(port->irq);
> > >  
> > >  		/*
> > >  		 * If the interrupt is not reasserted, or we otherwise
> > 
> > I think that it might be safe but I am not 100% sure, sigh.
> 
> Yeah, I'm not 100%, but I'd give it a try.

I do not feel brave enough to ack it today. But I am all for trying it
if anyone more familiar with the code is fine with it.

Best Regards,
Petr