diff mbox series

[3/3] serial: 8250_dw: Fix clk-notifier/port suspend deadlock

Message ID 20200923161950.6237-4-Sergey.Semin@baikalelectronics.ru
State New, archived
Headers show
Series serial: 8250_dw: Fix clk-notifier/port suspend deadlock | expand

Commit Message

Serge Semin Sept. 23, 2020, 4:19 p.m. UTC
It has been discovered that there is a potential deadlock between
the clock-change-notifier thread and the UART port suspending one:

   CPU0 (suspend CPU/UART)   CPU1 (update clock)
            ----                    ----
   lock(&port->mutex);
                             lock((work_completion)(&data->clk_work));
                             lock(&port->mutex);
   lock((work_completion)(&data->clk_work));

   *** DEADLOCK ***

The best way to fix this is to eliminate the CPU0
port->mutex/work-completion scenario. So we suggest to register and
unregister the clock-notifier during the DW APB UART port probe/remove
procedures, instead of doing that at the points of the port
startup/shutdown.

Link: https://lore.kernel.org/linux-serial/f1cd5c75-9cda-6896-a4e2-42c5bfc3f5c3@redhat.com

Fixes: cc816969d7b5 ("serial: 8250_dw: Fix common clocks usage race condition")
Reported-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
---
 drivers/tty/serial/8250/8250_dw.c | 54 +++++++++++--------------------
 1 file changed, 19 insertions(+), 35 deletions(-)

Comments

Jonathan Liu Oct. 18, 2020, 12:32 a.m. UTC | #1
On Wed, 23 Sep 2020 at 16:19, Serge Semin
<Sergey.Semin@baikalelectronics.ru> wrote:
>
> It has been discovered that there is a potential deadlock between
> the clock-change-notifier thread and the UART port suspending one:
>
>    CPU0 (suspend CPU/UART)   CPU1 (update clock)
>             ----                    ----
>    lock(&port->mutex);
>                              lock((work_completion)(&data->clk_work));
>                              lock(&port->mutex);
>    lock((work_completion)(&data->clk_work));
>
>    *** DEADLOCK ***
>
> The best way to fix this is to eliminate the CPU0
> port->mutex/work-completion scenario. So we suggest to register and
> unregister the clock-notifier during the DW APB UART port probe/remove
> procedures, instead of doing that at the points of the port
> startup/shutdown.
>
> Link: https://lore.kernel.org/linux-serial/f1cd5c75-9cda-6896-a4e2-42c5bfc3f5c3@redhat.com
>
> Fixes: cc816969d7b5 ("serial: 8250_dw: Fix common clocks usage race condition")
> Reported-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>

Tested-by: Jonathan Liu <net147@gmail.com>

Fixes hang while closing the serial port on RK3399 that I was
experiencing often with Linux 5.9.
After applying this patch, it no longer hangs while closing the serial port.
No problems while rebooting either.

Thanks.

Regards,
Jonathan
diff mbox series

Patch

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 87f450b7c177..9e204f9b799a 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -373,39 +373,6 @@  static void dw8250_set_ldisc(struct uart_port *p, struct ktermios *termios)
 	serial8250_do_set_ldisc(p, termios);
 }
 
-static int dw8250_startup(struct uart_port *p)
-{
-	struct dw8250_data *d = to_dw8250_data(p->private_data);
-	int ret;
-
-	/*
-	 * Some platforms may provide a reference clock shared between several
-	 * devices. In this case before using the serial port first we have to
-	 * make sure that any clock state change is known to the UART port at
-	 * least post factum.
-	 */
-	if (d->clk) {
-		ret = clk_notifier_register(d->clk, &d->clk_notifier);
-		if (ret)
-			dev_warn(p->dev, "Failed to set the clock notifier\n");
-	}
-
-	return serial8250_do_startup(p);
-}
-
-static void dw8250_shutdown(struct uart_port *p)
-{
-	struct dw8250_data *d = to_dw8250_data(p->private_data);
-
-	serial8250_do_shutdown(p);
-
-	if (d->clk) {
-		clk_notifier_unregister(d->clk, &d->clk_notifier);
-
-		flush_work(&d->clk_work);
-	}
-}
-
 /*
  * dw8250_fallback_dma_filter will prevent the UART from getting just any free
  * channel on platforms that have DMA engines, but don't have any channels
@@ -501,8 +468,6 @@  static int dw8250_probe(struct platform_device *pdev)
 	p->serial_out	= dw8250_serial_out;
 	p->set_ldisc	= dw8250_set_ldisc;
 	p->set_termios	= dw8250_set_termios;
-	p->startup	= dw8250_startup;
-	p->shutdown	= dw8250_shutdown;
 
 	p->membase = devm_ioremap(dev, regs->start, resource_size(regs));
 	if (!p->membase)
@@ -622,6 +587,19 @@  static int dw8250_probe(struct platform_device *pdev)
 		goto err_reset;
 	}
 
+	/*
+	 * Some platforms may provide a reference clock shared between several
+	 * devices. In this case any clock state change must be known to the
+	 * UART port at least post factum.
+	 */
+	if (data->clk) {
+		err = clk_notifier_register(data->clk, &data->clk_notifier);
+		if (err)
+			dev_warn(p->dev, "Failed to set the clock notifier\n");
+		else
+			queue_work(system_unbound_wq, &data->clk_work);
+	}
+
 	platform_set_drvdata(pdev, data);
 
 	pm_runtime_set_active(dev);
@@ -648,6 +626,12 @@  static int dw8250_remove(struct platform_device *pdev)
 
 	pm_runtime_get_sync(dev);
 
+	if (data->clk) {
+		clk_notifier_unregister(data->clk, &data->clk_notifier);
+
+		flush_work(&data->clk_work);
+	}
+
 	serial8250_unregister_port(data->data.line);
 
 	reset_control_assert(data->rst);