serial: qcom-geni: fix TX DMA buffer flush
cover serial: qcom-geni: fix TX DMA buffer flush
From: Jan Sebastian Götte <[email protected]>
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(-)
Signature · 1 line
2.53.0
PATCH 1/1 serial: qcom-geni: fix TX DMA buffer flush
From: Jan Sebastian Götte <[email protected]>
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.
--- drivers/tty/serial/qcom_geni_serial.c | 43 ++++++++++++++------------- 1 file changed, 22 insertions(+), 21 deletions(-)
drivers/tty/serial/qcom_geni_serial.c +22 -21
@@ -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,
Signature · 1 line
2.53.0
Hi
Quoted 115 lines · jaseg
From: Jan Sebastian Götte <[email protected]>
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_opsdoes not provide aflush_buffercallback, 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 clearstx_remainingandtx_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-5Signed-off-by Jan Sebastian Götte <[email protected]>Fixes 2aaa43c70778 ("tty: serial: qcom-geni-serial: add support for serial engine DMA")--- drivers/tty/serial/qcom_geni_serial.c | 43 ++++++++++++++------------- 1 file changed, 22 insertions(+), 21 deletions(-)drivers/tty/serial/qcom_geni_serial.c +21
-21@@ -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,
drivers/tty/serial/qcom_geni_serial.c static const struct uart_ops qcom_geni_uart_pops = { · line 1798
Quoted 7 lines
drivers/tty/serial/qcom_geni_serial.c +1
-0@@ -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,
Thanks,
Praveen Talari