linux-serial.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] tty: serial: imx: disable TXDC IRQ in imx_uart_shutdown() to avoid IRQ storm
@ 2020-09-25  8:24 Matthias Schiffer
  0 siblings, 0 replies; only message in thread
From: Matthias Schiffer @ 2020-09-25  8:24 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Jiri Slaby, Shawn Guo, Sascha Hauer
  Cc: Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	linux-serial, linux-arm-kernel, linux-kernel, Matthias Schiffer

The IPG clock is disabled at the end of imx_uart_shutdown(); we really
don't want to run any IRQ handlers after this point.

At least on i.MX8MN, the UART will happily continue to generate interrupts
even with its clocks disabled, but in this state, all register writes are
ignored (which will cause the shadow registers to differ from the actual
register values, resulting in all kinds of weirdness).

In a transfer without DMA, this could lead to the following sequence of
events:

- The UART finishes its transmission while imx_uart_shutdown() is run,
  triggering the TXDC interrupt (we can trigger this fairly reliably by
  writing a single byte to the TTY and closing it right away)
- imx_uart_shutdown() finishes, disabling the UART clocks
- imx_uart_int() -> imx_uart_transmit_buffer() -> imx_uart_stop_tx()

imx_uart_stop_tx() should now clear UCR4_TCEN to disable the TXDC
interrupt, but this register write is ineffective. This results in an
interrupt storm.

To disable all interrupts in the same place, and to avoid setting UCR4
twice, clearing UCR4_OREN is moved below del_timer_sync() as well; this
should be harmless.

Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
---

While debugging this, I found one more instance of register writes with
disabled clock: The IPG clock is disabled before calling
uart_add_one_port() at the end of imx_uart_probe(). This results in the
following call stack:

    imx_uart_writel+0x168/0x188
    imx_uart_set_mctrl+0x3c/0xb8
    uart_add_one_port+0x394/0x4c8
    imx_uart_probe+0x530/0x810

Fortunately, in this case the register already matches the value that is
written, so no inconsistent state results. I assume we'll have to do
something about the way we handle the clocks in this driver to fix
this...


 drivers/tty/serial/imx.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 4e6ead1f650e..1731d9728865 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1552,10 +1552,6 @@ static void imx_uart_shutdown(struct uart_port *port)
 	ucr2 = imx_uart_readl(sport, UCR2);
 	ucr2 &= ~(UCR2_TXEN | UCR2_ATEN);
 	imx_uart_writel(sport, ucr2, UCR2);
-
-	ucr4 = imx_uart_readl(sport, UCR4);
-	ucr4 &= ~UCR4_OREN;
-	imx_uart_writel(sport, ucr4, UCR4);
 	spin_unlock_irqrestore(&sport->port.lock, flags);
 
 	/*
@@ -1568,10 +1564,15 @@ static void imx_uart_shutdown(struct uart_port *port)
 	 */
 
 	spin_lock_irqsave(&sport->port.lock, flags);
+
 	ucr1 = imx_uart_readl(sport, UCR1);
 	ucr1 &= ~(UCR1_TRDYEN | UCR1_RRDYEN | UCR1_RTSDEN | UCR1_UARTEN | UCR1_RXDMAEN | UCR1_ATDMAEN);
-
 	imx_uart_writel(sport, ucr1, UCR1);
+
+	ucr4 = imx_uart_readl(sport, UCR4);
+	ucr4 &= ~(UCR4_OREN | UCR4_TCEN);
+	imx_uart_writel(sport, ucr4, UCR4);
+
 	spin_unlock_irqrestore(&sport->port.lock, flags);
 
 	clk_disable_unprepare(sport->clk_per);
-- 
2.17.1


^ permalink raw reply related	[flat|nested] only message in thread

only message in thread, other threads:[~2020-09-25  8:33 UTC | newest]

Thread overview: (only message) (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-25  8:24 [PATCH] tty: serial: imx: disable TXDC IRQ in imx_uart_shutdown() to avoid IRQ storm Matthias Schiffer

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).