Re: UART/TTY console deadlock

[Sergey Senozhatsky <sergey.senozhatsky@gmail.com>]

Cc-ing more people

https://lore.kernel.org/lkml/CAHp75Vd8nTzmZdnhpTDChdc11zyCaSfeigbxaCpOWZ1Lv9ZBMw@mail.gmail.com

On (20/06/22 20:37), Andy Shevchenko wrote:
> > [  279.759811] -> #2 (&irq_desc_lock_class){-.-.}:
> > [  279.759813]        _raw_spin_lock_irqsave+0x61/0x8d
> > [  279.759813]        __irq_get_desc_lock+0x65/0x89
> > [  279.759814]        __disable_irq_nosync+0x3b/0x93
> > [  279.759814]        serial8250_do_startup+0x451/0x75c
> > [  279.759815]        uart_startup+0x1b4/0x2ff
> > [  279.759815]        uart_port_activate+0x73/0xa0
> > [  279.759815]        tty_port_open+0xae/0x10a
> > [  279.759816]        uart_open+0x1b/0x26
> > [  279.759816]        tty_open+0x24d/0x3a0
> > [  279.759817]        chrdev_open+0xd5/0x1cc
> > [  279.759817]        do_dentry_open+0x299/0x3c8
> > [  279.759817]        path_openat+0x434/0x1100
> > [  279.759818]        do_filp_open+0x9b/0x10a
> > [  279.759818]        do_sys_open+0x15f/0x3d7
> > [  279.759819]        kernel_init_freeable+0x157/0x1dd
> > [  279.759819]        kernel_init+0xe/0x105
> > [  279.759819]        ret_from_fork+0x27/0x50
> > [  279.759820]
> > [  279.759820] -> #1 (&port_lock_key){-.-.}:
> > [  279.759822]        _raw_spin_lock_irqsave+0x61/0x8d
> > [  279.759822]        serial8250_console_write+0xa7/0x2a0
> > [  279.759823]        console_unlock+0x3b7/0x528
> > [  279.759823]        vprintk_emit+0x111/0x17f
> > [  279.759823]        printk+0x59/0x73
> > [  279.759824]        register_console+0x336/0x3a4
> > [  279.759824]        uart_add_one_port+0x51b/0x5be
> > [  279.759825]        serial8250_register_8250_port+0x454/0x55e
> > [  279.759825]        dw8250_probe+0x4dc/0x5b9
> > [  279.759825]        platform_drv_probe+0x67/0x8b
> > [  279.759826]        really_probe+0x14a/0x422
> > [  279.759826]        driver_probe_device+0x66/0x130
> > [  279.759827]        device_driver_attach+0x42/0x5b
> > [  279.759827]        __driver_attach+0xca/0x139
> > [  279.759827]        bus_for_each_dev+0x97/0xc9
> > [  279.759828]        bus_add_driver+0x12b/0x228
> > [  279.759828]        driver_register+0x64/0xed
> > [  279.759829]        do_one_initcall+0x20c/0x4a6
> > [  279.759829]        do_initcall_level+0xb5/0xc5
> > [  279.759829]        do_basic_setup+0x4c/0x58
> > [  279.759830]        kernel_init_freeable+0x13f/0x1dd
> > [  279.759830]        kernel_init+0xe/0x105
> > [  279.759831]        ret_from_fork+0x27/0x50
> > [  279.759831]
> > [  279.759831] -> #0 (console_owner){-...}:
> > [  279.759833]        __lock_acquire+0x118d/0x2714
> > [  279.759833]        lock_acquire+0x203/0x258
> > [  279.759834]        console_lock_spinning_enable+0x51/0x57
> > [  279.759834]        console_unlock+0x25d/0x528
> > [  279.759834]        vprintk_emit+0x111/0x17f
> > [  279.759835]        printk+0x59/0x73
> > [  279.759835]        __report_bad_irq+0xa3/0xba
> > [  279.759836]        note_interrupt+0x19a/0x1d6
> > [  279.759836]        handle_irq_event_percpu+0x57/0x79
> > [  279.759836]        handle_irq_event+0x36/0x55
> > [  279.759837]        handle_fasteoi_irq+0xc2/0x18a
> > [  279.759837]        do_IRQ+0xb3/0x157
> > [  279.759838]        ret_from_intr+0x0/0x1d
> > [  279.759838]        cpuidle_enter_state+0x12f/0x1fd
> > [  279.759838]        cpuidle_enter+0x2e/0x3d
> > [  279.759839]        do_idle+0x1ce/0x2ce
> > [  279.759839]        cpu_startup_entry+0x1d/0x1f
> > [  279.759840]        start_kernel+0x406/0x46a
> > [  279.759840]        secondary_startup_64+0xa4/0xb0

Hmm. So this is uart->port => desc->lock vs desc->lock => uart->port

chain #1:

 serial8250_do_startup()
  spin_lock_irqsave(&port->lock);
   disable_irq_nosync(port->irq);
    raw_spin_lock_irqsave(&desc->lock)

chain #2:

  __report_bad_irq()
   raw_spin_lock_irqsave(&desc->lock)
    for_each_action_of_desc()
     printk()
      spin_lock_irqsave(&port->lock);


Breaking up chain #2 is not an option, I suppose. Those are a rather
important KERN_ERR messages, printk_deferred() will upset people badly.

So... Do we need to hold uart->port when we disable port->irq? What do we
race with? Module removal? The function bumps device PM counter (albeit
for UART_CAP_RPM ports only). But, at the same time, we do a whole bunch
of unprotected port->FOO accesses in serial8250_do_startup(). We even set
the IRQF_SHARED up->port.irqflags without grabbing the port->lock:

	 up->port.irqflags |= IRQF_SHARED;
	 spin_lock_irqsave(&port->lock, flags);
	 if (up->port.irqflags & IRQF_SHARED)
	    disable_irq_nosync(port->irq);

IOW, can we do something like this?

---
 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
-- 
2.27.0