[PATCH 0/1] serial: qcom-geni: fix TX DMA buffer flush

[PATCH 0/1] serial: qcom-geni: fix TX DMA buffer flush

[jaseg]

From: Jan Sebastian Götte <linux@jaseg.de>

I ran into a weird issue using the UART on an Arduino Uno Q (Qualcomm
QRB2210) target. When doing a write immediately followed by a transmit
flush, the UART would get stuck infinitely repeating a stale DMA buffer
and become unresponsive to further write() calls.

With some assistance from an LLM, I tracked down the issue to the geni
serial driver's DMA mode code. There are at least two issues in the code
that contribute to this behavior:

(1) In DMA mode, uart_ops.flush_buffer is missing, leading to an
underflow in handle_tx_dma, which leads to an infinite loop of
page-sized buffers being sent out.

(2) In DMA mode, uart_ops.stop_tx was fishy, and unmapped the DMA buffer
without actually stopping the peripheral's DMA engine which led to a
stuck state where the UART wouldn't transmit any more bytes.

Qualcomm refused to give me access to their documentation, so I'm left
guessing and vibe checking LLM output on some of this, but I've
confirmed looking at hardware with an oscilloscope that both the fixes
in stop_tx, and the new flush_buffer are necessary to solve the issue.

What's left is that I'm pretty sure that even with my rework, stop_tx
will still lead to buffer contents being re-transmitted after restarting
when it's called during an ongoing DMA transfer. AFAICT there never was
anything here that would keep track of partial transfers, so I think I
didn't make things worse. I don't have a nice way to debug this, and I
don't know what the original intention of whoever wrote that code was,
so I'm leaving that for maybe someone at Qualcomm to have a look at.

Below is a reproducer that when the bug is present hangs and leads to an
infinite stream of alphabet soup on the UART TX pin.

(*snip*)
#include <fcntl.h>
#include <stdio.h>
#include <termios.h>
#include <unistd.h>

int main(int argc, char **argv)
{
	char buf[4096];
	int fd, i;
	for (i = 0; i < (int)sizeof(buf); i++)
		buf[i] = 'A' + i % 26;
	fd = open(argc > 1 ? argv[1] : "/dev/ttyHS1", O_RDWR | O_NOCTTY);
	write(fd, buf, sizeof(buf));
	usleep(20000);
	tcflush(fd, TCOFLUSH);
    write(fd, "foo\r\n", 5);
    usleep(100000);
	puts("tcdrain: hangs forever when wedged");
	tcdrain(fd);
	puts("tcdrain returned: NOT wedged");
	return 0;
}
(*snip*)

Jan Sebastian Götte (1):
  serial: qcom-geni: fix TX DMA buffer flush

 drivers/tty/serial/qcom_geni_serial.c | 43 ++++++++++++++-------------
 1 file changed, 22 insertions(+), 21 deletions(-)

-- 
2.53.0

[PATCH 1/1] serial: qcom-geni: fix TX DMA buffer flush

[jaseg]

From: Jan Sebastian Götte <linux@jaseg.de>

When transmit flushing a qcom-geni UART during an ongoing TX DMA, the
UART gets stuck infinitely repeating corrupted TX DMA frames.

The DMA-mode uart_ops does not provide a flush_buffer callback, so an
in-flight transfer can complete after serial core has reset the transmit
kfifo, underflowing its length and resubmitting page-sized transfers
indefinitely. Add one that stops the transfer and clears tx_remaining
and tx_queued.

The stop path was also broken: it unmapped the buffer while the serial
engine could still read it, and never reset the TX DMA state machine.
Cancel the main sequencer command first, then reset the state machine
and wait for it before unmapping. Drop the early return so a pending
mapping is also cleaned up when the main command is inactive.

The bug can be triggered from userspace with a large write immediately
followed by TCOFLUSH. A following tcdrain will hang forever. The bug was
reproduced and this fix was validated on Arduino Uno Q (QRB2210)
using /dev/ttyHS1.

Assisted-by: Claude:claude-5-opus Codex:gpt-5
Signed-off-by: Jan Sebastian Götte <linux@jaseg.de>
Fixes: 2aaa43c70778 ("tty: serial: qcom-geni-serial: add support for serial engine DMA")
Cc: stable@vger.kernel.org
---
 drivers/tty/serial/qcom_geni_serial.c | 43 ++++++++++++++-------------
 1 file changed, 22 insertions(+), 21 deletions(-)

diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 7ead87b4eb65..1e39122ec09f 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -158,6 +158,7 @@ static const struct uart_ops qcom_geni_uart_pops;
 static struct uart_driver qcom_geni_console_driver;
 static struct uart_driver qcom_geni_uart_driver;
 
+static void qcom_geni_serial_stop_tx_dma(struct uart_port *uport);
 static void __qcom_geni_serial_cancel_tx_cmd(struct uart_port *uport);
 static void qcom_geni_serial_cancel_tx_cmd(struct uart_port *uport);
 static int qcom_geni_serial_port_setup(struct uart_port *uport);
@@ -636,35 +637,34 @@ static unsigned int qcom_geni_serial_tx_empty(struct uart_port *uport)
 	return !readl(uport->membase + SE_GENI_TX_FIFO_STATUS);
 }
 
+static void qcom_geni_serial_flush_buffer_dma(struct uart_port *uport)
+{
+	struct qcom_geni_serial_port *port = to_dev_port(uport);
+
+	qcom_geni_serial_stop_tx_dma(uport);
+	port->tx_remaining = 0;
+	port->tx_queued = 0;
+}
+
 static void qcom_geni_serial_stop_tx_dma(struct uart_port *uport)
 {
 	struct qcom_geni_serial_port *port = to_dev_port(uport);
-	bool done;
 
-	if (!qcom_geni_serial_main_active(uport))
-		return;
+	if (qcom_geni_serial_main_active(uport))
+		__qcom_geni_serial_cancel_tx_cmd(uport);
 
 	if (port->tx_dma_addr) {
+		writel(1, uport->membase + SE_DMA_TX_FSM_RST);
+		if (!qcom_geni_serial_poll_bit(uport, SE_DMA_TX_IRQ_STAT,
+					       TX_RESET_DONE, true))
+			dev_err_ratelimited(uport->dev, "TX DMA reset failed");
+		writel(TX_RESET_DONE | TX_DMA_DONE,
+		       uport->membase + SE_DMA_TX_IRQ_CLR);
+
 		geni_se_tx_dma_unprep(&port->se, port->tx_dma_addr,
 				      port->tx_remaining);
 		port->tx_dma_addr = 0;
-		port->tx_remaining = 0;
 	}
-
-	geni_se_cancel_m_cmd(&port->se);
-
-	done = qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
-					 M_CMD_CANCEL_EN, true);
-	if (!done) {
-		geni_se_abort_m_cmd(&port->se);
-		done = qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
-						 M_CMD_ABORT_EN, true);
-		if (!done)
-			dev_err_ratelimited(uport->dev, "M_CMD_ABORT_EN not set");
-		writel(M_CMD_ABORT_EN, uport->membase + SE_GENI_M_IRQ_CLEAR);
-	}
-
-	writel(M_CMD_CANCEL_EN, uport->membase + SE_GENI_M_IRQ_CLEAR);
 }
 
 static void qcom_geni_serial_start_tx_dma(struct uart_port *uport)
@@ -1180,7 +1180,7 @@ static void qcom_geni_serial_shutdown(struct uart_port *uport)
 	uart_port_unlock_irq(uport);
 }
 
-static void qcom_geni_serial_flush_buffer(struct uart_port *uport)
+static void qcom_geni_serial_flush_buffer_fifo(struct uart_port *uport)
 {
 	qcom_geni_serial_cancel_tx_cmd(uport);
 }
@@ -1769,7 +1769,7 @@ static const struct uart_ops qcom_geni_console_pops = {
 	.request_port = qcom_geni_serial_request_port,
 	.config_port = qcom_geni_serial_config_port,
 	.shutdown = qcom_geni_serial_shutdown,
-	.flush_buffer = qcom_geni_serial_flush_buffer,
+	.flush_buffer = qcom_geni_serial_flush_buffer_fifo,
 	.type = qcom_geni_serial_get_type,
 	.set_mctrl = qcom_geni_serial_set_mctrl,
 	.get_mctrl = qcom_geni_serial_get_mctrl,
@@ -1792,6 +1792,7 @@ static const struct uart_ops qcom_geni_uart_pops = {
 	.request_port = qcom_geni_serial_request_port,
 	.config_port = qcom_geni_serial_config_port,
 	.shutdown = qcom_geni_serial_shutdown,
+	.flush_buffer = qcom_geni_serial_flush_buffer_dma,
 	.type = qcom_geni_serial_get_type,
 	.set_mctrl = qcom_geni_serial_set_mctrl,
 	.get_mctrl = qcom_geni_serial_get_mctrl,
-- 
2.53.0