linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Jiri Slaby <jirislaby@kernel.org>,
	Shawn Guo <shawnguo@kernel.org>,
	Sascha Hauer <s.hauer@pengutronix.de>
Cc: Pengutronix Kernel Team <kernel@pengutronix.de>,
	Fabio Estevam <festevam@gmail.com>,
	NXP Linux Team <linux-imx@nxp.com>,
	linux-serial@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org,
	Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
Subject: [PATCH] tty: serial: imx: disable TXDC IRQ in imx_uart_shutdown() to avoid IRQ storm
Date: Fri, 25 Sep 2020 10:24:12 +0200	[thread overview]
Message-ID: <20200925082412.12960-1-matthias.schiffer@ew.tq-group.com> (raw)

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


                 reply	other threads:[~2020-09-25  8:26 UTC|newest]

Thread overview: [no followups] expand[flat|nested]  mbox.gz  Atom feed

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200925082412.12960-1-matthias.schiffer@ew.tq-group.com \
    --to=matthias.schiffer@ew.tq-group.com \
    --cc=festevam@gmail.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=jirislaby@kernel.org \
    --cc=kernel@pengutronix.de \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-imx@nxp.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-serial@vger.kernel.org \
    --cc=s.hauer@pengutronix.de \
    --cc=shawnguo@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).