diff mbox series

[2/3] serial: qcom-geni: fix soft lockup on sw flow control and suspend

Message ID 20240624133135.7445-3-johan+linaro@kernel.org
State New
Headers show
Series serial: qcom-geni: fix lockups | expand

Commit Message

Johan Hovold June 24, 2024, 1:31 p.m. UTC 
The stop_tx() callback is used to implement software flow control and
must not discard data as the Qualcomm GENI driver is currently doing
when there is an active TX command.

Cancelling an active command can also leave data in the hardware FIFO,
which prevents the watermark interrupt from being enabled when TX is
later restarted. This results in a soft lockup and is easily triggered
by stopping TX using software flow control in a serial console but this
can also happen after suspend.

Fix this by only stopping any active command, and effectively clearing
the hardware fifo, when shutting down the port. Make sure to temporarily
raise the watermark level so that the interrupt fires when TX is
restarted.

Fixes: c4f528795d1a ("tty: serial: msm_geni_serial: Add serial driver support for GENI based QUP")
Cc: stable@vger.kernel.org	# 4.17
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/tty/serial/qcom_geni_serial.c | 28 +++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

Comments

Johan Hovold July 4, 2024, 10:08 a.m. UTC | #1 
On Wed, Jun 26, 2024 at 09:54:40AM +0200, Johan Hovold wrote:
> On Mon, Jun 24, 2024 at 02:58:39PM -0700, Doug Anderson wrote:

> > 1. The function is named qcom_geni_serial_clear_tx_fifo() which
> > implies that when it finishes that the hardware FIFO will have nothing
> > in it. ...but how does your code ensure this?
> 
> Yeah, I realised after I sent out the series that this may not be the
> case. I was under the impression that cancelling a command would discard
> the data in the FIFO (e.g. when starting the next command) but that was
> probably an error in my mental model.

I went back and did some more reverse engineering and have now confirmed
that the hardware works as I assumed for v1, that is, that cancelling a
command leaves data in the fifo, which is later discarded when a new
command is issued.

> > 3. On my hardware you're setting the FIFO level to 16 here. The docs I
> > have say that if the FIFO level is "less than" the value you set here
> > then the interrupt will go off and further clarifies that if you set
> > the register to 1 here then you'll get interrupted when the FIFO is
> > empty. So what happens with your solution if the FIFO is completely
> > full? In that case you'd have to set this to 17, right? ...but then I
> > could believe that might confuse the interrupt handler which would get
> > told to start transmitting when there is no room for anything.
> 
> Indeed. I may implicitly be relying on the absence of hardware flow
> control as well so that waiting for one character to be sent is what
> makes this work.

I'm keeping the watermark level unchanged in v2 and instead restart tx
by issuing a short transfer command to clear any stale data from the
fifo which could prevent the watermark interrupt from firing.

Johan
diff mbox series

Patch

diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 1d5d6045879a..72addeb9f461 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -651,13 +651,8 @@  static void qcom_geni_serial_start_tx_fifo(struct uart_port *uport)
 {
 	u32 irq_en;
 
-	if (qcom_geni_serial_main_active(uport) ||
-	    !qcom_geni_serial_tx_empty(uport))
-		return;
-
 	irq_en = readl(uport->membase +	SE_GENI_M_IRQ_EN);
 	irq_en |= M_TX_FIFO_WATERMARK_EN | M_CMD_DONE_EN;
-
 	writel(DEF_TX_WM, uport->membase + SE_GENI_TX_WATERMARK_REG);
 	writel(irq_en, uport->membase +	SE_GENI_M_IRQ_EN);
 }
@@ -665,16 +660,28 @@  static void qcom_geni_serial_start_tx_fifo(struct uart_port *uport)
 static void qcom_geni_serial_stop_tx_fifo(struct uart_port *uport)
 {
 	u32 irq_en;
-	struct qcom_geni_serial_port *port = to_dev_port(uport);
 
 	irq_en = readl(uport->membase + SE_GENI_M_IRQ_EN);
 	irq_en &= ~(M_CMD_DONE_EN | M_TX_FIFO_WATERMARK_EN);
 	writel(0, uport->membase + SE_GENI_TX_WATERMARK_REG);
 	writel(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
-	/* Possible stop tx is called multiple times. */
+}
+
+static void qcom_geni_serial_clear_tx_fifo(struct uart_port *uport)
+{
+	struct qcom_geni_serial_port *port = to_dev_port(uport);
+
 	if (!qcom_geni_serial_main_active(uport))
 		return;
 
+	/*
+	 * Increase watermark level so that TX can be restarted and wait for
+	 * sequencer to start to prevent lockups.
+	 */
+	writel(port->tx_fifo_depth, uport->membase + SE_GENI_TX_WATERMARK_REG);
+	qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
+					M_TX_FIFO_WATERMARK_EN, true);
+
 	geni_se_cancel_m_cmd(&port->se);
 	if (!qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
 						M_CMD_CANCEL_EN, true)) {
@@ -684,6 +691,8 @@  static void qcom_geni_serial_stop_tx_fifo(struct uart_port *uport)
 		writel(M_CMD_ABORT_EN, uport->membase + SE_GENI_M_IRQ_CLEAR);
 	}
 	writel(M_CMD_CANCEL_EN, uport->membase + SE_GENI_M_IRQ_CLEAR);
+
+	port->tx_remaining = 0;
 }
 
 static void qcom_geni_serial_handle_rx_fifo(struct uart_port *uport, bool drop)
@@ -1069,11 +1078,10 @@  static void qcom_geni_serial_shutdown(struct uart_port *uport)
 {
 	disable_irq(uport->irq);
 
-	if (uart_console(uport))
-		return;
-
 	qcom_geni_serial_stop_tx(uport);
 	qcom_geni_serial_stop_rx(uport);
+
+	qcom_geni_serial_clear_tx_fifo(uport);
 }
 
 static int qcom_geni_serial_port_setup(struct uart_port *uport)