[BUG] serial: TX kfifo underflow when TCOFLUSH races with TX DMA (samsung, sprd, msm, pch_uart, tegra)

[BUG] serial: TX kfifo underflow when TCOFLUSH races with TX DMA (samsung, sprd, msm, pch_uart, tegra)

[Samarth Narang]

Hi,

The TX FIFO underflow previously fixed in 8250 by 9e512eaaf8f4 ("serial:
8250: Fix fifo underflow on flush") and in qcom-geni by 452d6fa37ae9,
7ea38c49e717, and e3c04834ae1a appears to affect five other serial
drivers. The line numbers below refer to mainline at 83f32c773255.

uart_flush_buffer() resets the transmit kfifo under the port lock before
calling the driver's flush_buffer() callback. If a TX DMA transfer is
still active and the driver does not account for it during the flush,
its completion callback may subsequently call uart_xmit_advance() using
the byte count from the old transfer. This can advance kfifo->out past
kfifo->in, causing kfifo_len() to wrap and the driver to transmit stale
buffer contents. As shown in the 8250 fix, a subsequent close can then
wait in tty_wait_until_sent() until it times out. The issue may arise
when a large write is followed by TCFLSH(TCOFLUSH), or when the port is
closed while TX DMA is active.

Four drivers have no flush_buffer() callback and therefore do not stop
or account for outstanding TX DMA when the kfifo is reset:

1. samsung_tty: Neither s3c64xx_serial_ops (line 1679) nor
   apple_s5l_serial_ops (line 1700) implements flush_buffer(), while
   s3c24xx_serial_tx_dma_complete() advances the kfifo by
   tx_bytes_requested - residue (line 342) without checking whether a
   flush occurred.

2. sprd_serial: serial_sprd_ops (line 944) has no flush_buffer()
   callback, and sprd_complete_tx_dma() advances the kfifo by
   tx_dma.trans_len (line 276).

3. msm_serial: msm_uart_pops (line 1547) does not implement
   flush_buffer(), while msm_complete_tx_dma() advances by
   sg_dma_len(&dma->tx_sg) - residue (line 482). Its check for an
   already stopped transfer relies on tx_sg, which is not cleared by a
   flush.

4. pch_uart: pch_uart_ops (line 1518) has no flush_buffer() callback,
   and pch_dma_tx_complete() advances the kfifo using each scatterlist
   entry's sg_dma_len() (line 756) without holding the port lock.

The fifth driver, serial-tegra, implements tegra_uart_flush_buffer()
(lines 1272-1279), but only clears tx_bytes and calls
dmaengine_terminate_all(). Its completion callback calculates the
transferred byte count from tx_bytes_requested - residue (line 537)
before acquiring the port lock and advances the kfifo afterward (line
540), while tx_bytes_requested is left unchanged by the flush. Although
terminated descriptors do not invoke their callbacks, a completion
already running when the flush occurs, particularly one waiting to
acquire the port lock held by uart_flush_buffer(), may still advance the
reset kfifo. This is the same race addressed by 452d6fa37ae9.

The existing 8250 and qcom-geni fixes provide possible approaches, with
the former clearing the outstanding DMA byte count under the port lock
and the latter marking completions as stale during a flush. As discussed
in 7ea38c49e717, comparing the completion count against kfifo_len()
alone is insufficient.

These findings came from a research static analyzer that checks whether
uart_ops implementations using TX DMA provide flush handling that
accounts for outstanding transfers, followed by manual inspection of the
reported paths. None has been reproduced on hardware. The same analysis
found flush handling in the 8250 core, atmel, fsl_lpuart, imx, sh-sci,
and qcom-geni, while amba-pl011 is not affected because it advances the
kfifo when preparing a transfer rather than on completion.

I would be happy to prepare separate patches for the affected drivers if
the maintainers agree with the analysis.

Thanks,
Samarth

Re: [BUG] serial: TX kfifo underflow when TCOFLUSH races with TX DMA (samsung, sprd, msm, pch_uart, tegra)

[Greg Kroah-Hartman]

On Sat, Oct 31, 2026 at 12:47:33AM +0000, Samarth Narang wrote:
> I would be happy to prepare separate patches for the affected drivers if
> the maintainers agree with the analysis.

Always just send us patches, after testing them, you never need to ask
if you should do so.

thanks,

greg k-h

[PATCH 0/5] serial: fix TX kfifo underflow on flush in five drivers

[Samarth Narang]

This series addresses the TX kfifo underflow reported in the parent
thread, where uart_flush_buffer() resets the transmit kfifo but a
completion callback from an earlier DMA transfer may still call
uart_xmit_advance() using the byte count from before the flush.

The fixes take two approaches depending on how each driver manages its
TX DMA mappings. In samsung and tegra, which map the TX buffer once,
samsung's new flush_buffer() terminates the outstanding transfer, as
tegra's already does, following the approach introduced in 8250 by
9e512eaaf8f4. A completion already in progress skips the kfifo update if
the transfer has since been retired by flush_buffer() or stop_tx().

In sprd, msm, and pch_uart, which map each transfer separately,
terminating DMA there would leave the flush hook to unmap a buffer the
controller may still be reading. These drivers therefore leave the
transfer running and mark it as flushed, allowing the completion
callback to unmap the buffer without advancing the kfifo. This means
that the data already queued for DMA before the flush may still be
transmitted.

All five patches have only been compile-tested with arm64 defconfig and
the relevant drivers enabled, including pch_uart through COMPILE_TEST,
using W=1 with no new warnings. I do not have access to the affected
hardware, so testing by someone with suitable devices would be
appreciated.

One race remains, which is also present in the 8250 fix: a completion
may already be running when the flush occurs but acquire the port lock
only after a new transfer has started, potentially mistaking the new
transfer's state for its own. Preventing this would require tracking
individual transfers, for example with a generation counter, which I
have left out of this series to keep the changes focused.

Samarth Narang (5):
  tty: serial: samsung: fix TX kfifo underflow on flush
  serial: tegra: don't advance the TX kfifo for a flushed transfer
  serial: sprd: fix TX kfifo underflow on flush
  tty: serial: msm: fix TX kfifo underflow on flush
  serial: pch_uart: fix TX kfifo underflow on flush

 drivers/tty/serial/msm_serial.c   | 19 ++++++++++++++++++-
 drivers/tty/serial/pch_uart.c     | 27 +++++++++++++++++++++++++--
 drivers/tty/serial/samsung_tty.c  | 22 ++++++++++++++++++++++
 drivers/tty/serial/serial-tegra.c |  8 ++++++++
 drivers/tty/serial/sprd_serial.c  | 24 ++++++++++++++++++++++--
 5 files changed, 95 insertions(+), 5 deletions(-)


base-commit: 36aad21d50d1af3f6954bb0c988257a908202477
-- 
2.34.1

[PATCH 1/5] tty: serial: samsung: fix TX kfifo underflow on flush

[Samarth Narang]

uart_flush_buffer() resets the transmit kfifo under the port lock before
calling the driver's flush_buffer() hook. Since samsung_tty does not
implement this hook, an outstanding TX DMA transfer may complete after
the flush and call uart_xmit_advance() using the old transfer's byte
count. This can advance the kfifo tail past its head, causing
kfifo_len() to wrap and the driver to transmit stale buffer contents.
The same issue was addressed in 8250 by commit 9e512eaaf8f4 ("serial:
8250: Fix fifo underflow on flush").

Add a flush_buffer() hook that terminates the outstanding TX DMA
transfer and clears tx_in_progress. Since the TX buffer remains mapped
while the port is open, DMA synchronization is not required before
returning from the hook.

A completion that is already running when the flush occurs may have
calculated its byte count before taking the port lock, so it must skip
the kfifo update if the transfer has since been retired by either
flush_buffer() or stop_tx(), which already advances the kfifo itself.

Fixes: 29bef7990845 ("serial: samsung: add DMA support for TX")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Samarth Narang <samarth.colleges@gmail.com>
---
 drivers/tty/serial/samsung_tty.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
index 640f85e2d636..2bea7ef078d2 100644
--- a/drivers/tty/serial/samsung_tty.c
+++ b/drivers/tty/serial/samsung_tty.c
@@ -317,6 +317,20 @@ static void s3c24xx_serial_stop_tx(struct uart_port *port)
 	ourport->tx_mode = 0;
 }
 
+static void s3c24xx_serial_flush_buffer(struct uart_port *port)
+{
+	struct s3c24xx_uart_port *ourport = to_ourport(port);
+	struct s3c24xx_uart_dma *dma = ourport->dma;
+
+	if (!dma || !dma->tx_chan || ourport->tx_in_progress != S3C24XX_TX_DMA)
+		return;
+
+	/* The kfifo has been reset: the completion must not advance it */
+	dmaengine_terminate_async(dma->tx_chan);
+	async_tx_ack(dma->tx_desc);
+	ourport->tx_in_progress = 0;
+}
+
 static void s3c24xx_serial_start_next_tx(struct s3c24xx_uart_port *ourport);
 
 static void s3c24xx_serial_tx_dma_complete(void *args)
@@ -339,6 +353,12 @@ static void s3c24xx_serial_tx_dma_complete(void *args)
 
 	uart_port_lock_irqsave(port, &flags);
 
+	/* Already retired by stop_tx or flush_buffer */
+	if (ourport->tx_in_progress != S3C24XX_TX_DMA) {
+		uart_port_unlock_irqrestore(port, flags);
+		return;
+	}
+
 	uart_xmit_advance(port, count);
 	ourport->tx_in_progress = 0;
 
@@ -1682,6 +1702,7 @@ static const struct uart_ops s3c64xx_serial_ops = {
 	.get_mctrl	= s3c24xx_serial_get_mctrl,
 	.set_mctrl	= s3c24xx_serial_set_mctrl,
 	.stop_tx	= s3c24xx_serial_stop_tx,
+	.flush_buffer	= s3c24xx_serial_flush_buffer,
 	.start_tx	= s3c24xx_serial_start_tx,
 	.stop_rx	= s3c24xx_serial_stop_rx,
 	.break_ctl	= s3c24xx_serial_break_ctl,
@@ -1703,6 +1724,7 @@ static const struct uart_ops apple_s5l_serial_ops = {
 	.get_mctrl	= s3c24xx_serial_get_mctrl,
 	.set_mctrl	= s3c24xx_serial_set_mctrl,
 	.stop_tx	= s3c24xx_serial_stop_tx,
+	.flush_buffer	= s3c24xx_serial_flush_buffer,
 	.start_tx	= s3c24xx_serial_start_tx,
 	.stop_rx	= s3c24xx_serial_stop_rx,
 	.break_ctl	= s3c24xx_serial_break_ctl,
-- 
2.34.1

[PATCH 2/5] serial: tegra: don't advance the TX kfifo for a flushed transfer

[Samarth Narang]

tegra_uart_flush_buffer() terminates the TX DMA transfer after the
serial core has reset the transmit kfifo, but leaves tx_in_progress set.
A completion callback that was already running may have calculated its
byte count from tx_bytes_requested before acquiring the port lock,
allowing it to call uart_xmit_advance() on the reset kfifo after the
flush. This can advance the kfifo tail past its head, causing
kfifo_len() to wrap and the driver to transmit stale buffer contents.
The same race was fixed in qcom-geni by commit 452d6fa37ae9 ("serial:
qcom_geni: fix kfifo underflow when flush precedes DMA completion IRQ").

Clear tx_in_progress in the flush hook and have the completion callback
skip the kfifo update if the transfer has already been retired by either
the flush or tegra_uart_stop_tx(), which also advances the kfifo itself.

Fixes: e9ea096dd225 ("serial: tegra: add serial driver")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Samarth Narang <samarth.colleges@gmail.com>
---
 drivers/tty/serial/serial-tegra.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/tty/serial/serial-tegra.c b/drivers/tty/serial/serial-tegra.c
index b0fc4ae3686f..8b712a9bc293 100644
--- a/drivers/tty/serial/serial-tegra.c
+++ b/drivers/tty/serial/serial-tegra.c
@@ -537,6 +537,11 @@ static void tegra_uart_tx_dma_complete(void *args)
 	count = tup->tx_bytes_requested - state.residue;
 	async_tx_ack(tup->tx_dma_desc);
 	uart_port_lock_irqsave(&tup->uport, &flags);
+	/* Already retired by stop_tx or flush_buffer */
+	if (tup->tx_in_progress != TEGRA_UART_TX_DMA) {
+		uart_port_unlock_irqrestore(&tup->uport, flags);
+		return;
+	}
 	uart_xmit_advance(&tup->uport, count);
 	tup->tx_in_progress = 0;
 	if (kfifo_len(&tport->xmit_fifo) < WAKEUP_CHARS)
@@ -1276,6 +1281,9 @@ static void tegra_uart_flush_buffer(struct uart_port *u)
 	tup->tx_bytes = 0;
 	if (tup->tx_dma_chan)
 		dmaengine_terminate_all(tup->tx_dma_chan);
+	/* The kfifo has been reset: the completion must not advance it */
+	if (tup->tx_in_progress == TEGRA_UART_TX_DMA)
+		tup->tx_in_progress = 0;
 }
 
 static void tegra_uart_shutdown(struct uart_port *u)
-- 
2.34.1

[PATCH 3/5] serial: sprd: fix TX kfifo underflow on flush

[Samarth Narang]

uart_flush_buffer() resets the transmit kfifo under the port lock before
calling the driver's flush_buffer() hook. Since sprd_serial does not
implement this hook, an outstanding TX DMA transfer may complete after
the flush and call uart_xmit_advance() using the flushed transfer's
tx_dma.trans_len. This can advance the kfifo tail past its head, causing
kfifo_len() to wrap and the driver to transmit stale buffer contents.
The same issue was fixed in 8250 by commit 9e512eaaf8f4 ("serial: 8250:
Fix fifo underflow on flush").

Since the TX buffer is mapped separately for each transfer and the flush
hook runs under the port lock, DMA cannot be terminated and its mapping
safely released synchronously from the hook. Instead, leave the
outstanding transfer running and mark it as flushed, allowing the
completion callback to unmap the buffer without advancing the kfifo.
Apply the same check in sprd_stop_tx_dma() if the transfer is stopped
before completion. With this approach, the data already queued for DMA
before the flush may still be transmitted.

Fixes: f4487db58eb7 ("serial: sprd: Add DMA mode support")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Samarth Narang <samarth.colleges@gmail.com>
---
 drivers/tty/serial/sprd_serial.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/sprd_serial.c b/drivers/tty/serial/sprd_serial.c
index 3fdf5939a62c..718732b683f1 100644
--- a/drivers/tty/serial/sprd_serial.c
+++ b/drivers/tty/serial/sprd_serial.c
@@ -133,6 +133,7 @@ struct sprd_uart_port {
 	struct clk *clk;
 	struct sprd_uart_dma tx_dma;
 	struct sprd_uart_dma rx_dma;
+	bool tx_flushed;
 	dma_addr_t pos;
 	unsigned char *rx_buf_tail;
 	const struct sprd_uart_data *pdata;
@@ -236,13 +237,15 @@ static void sprd_stop_tx_dma(struct uart_port *port)
 	dmaengine_tx_status(sp->tx_dma.chn, sp->tx_dma.cookie, &state);
 	if (state.residue) {
 		trans_len = state.residue - sp->tx_dma.phys_addr;
-		uart_xmit_advance(port, trans_len);
+		if (!sp->tx_flushed)
+			uart_xmit_advance(port, trans_len);
 		dma_unmap_single(port->dev, sp->tx_dma.phys_addr,
 				 sp->tx_dma.trans_len, DMA_TO_DEVICE);
 	}
 
 	dmaengine_terminate_all(sp->tx_dma.chn);
 	sp->tx_dma.trans_len = 0;
+	sp->tx_flushed = false;
 }
 
 static int sprd_tx_buf_remap(struct uart_port *port)
@@ -273,7 +276,9 @@ static void sprd_complete_tx_dma(void *data)
 	dma_unmap_single(port->dev, sp->tx_dma.phys_addr,
 			 sp->tx_dma.trans_len, DMA_TO_DEVICE);
 
-	uart_xmit_advance(port, sp->tx_dma.trans_len);
+	if (!sp->tx_flushed)
+		uart_xmit_advance(port, sp->tx_dma.trans_len);
+	sp->tx_flushed = false;
 
 	if (kfifo_len(&tport->xmit_fifo) < WAKEUP_CHARS)
 		uart_write_wakeup(port);
@@ -555,6 +560,20 @@ static void sprd_stop_tx(struct uart_port *port)
 	serial_out(port, SPRD_ICLR, iclr);
 }
 
+static void sprd_flush_buffer(struct uart_port *port)
+{
+	struct sprd_uart_port *sp = container_of(port, struct sprd_uart_port,
+						 port);
+
+	/*
+	 * The kfifo has been reset. Let the running transfer finish, so that
+	 * its buffer is unmapped only once it has stopped, but do not advance
+	 * the kfifo for it.
+	 */
+	if (sp->tx_dma.trans_len)
+		sp->tx_flushed = true;
+}
+
 static void sprd_start_tx(struct uart_port *port)
 {
 	struct sprd_uart_port *sp = container_of(port, struct sprd_uart_port,
@@ -946,6 +965,7 @@ static const struct uart_ops serial_sprd_ops = {
 	.get_mctrl = sprd_get_mctrl,
 	.set_mctrl = sprd_set_mctrl,
 	.stop_tx = sprd_stop_tx,
+	.flush_buffer = sprd_flush_buffer,
 	.start_tx = sprd_start_tx,
 	.stop_rx = sprd_stop_rx,
 	.break_ctl = sprd_break_ctl,
-- 
2.34.1

[PATCH 4/5] tty: serial: msm: fix TX kfifo underflow on flush

[Samarth Narang]

uart_flush_buffer() resets the transmit kfifo under the port lock before
calling the driver's flush_buffer() hook. Since msm_serial does not
implement this hook, an outstanding TX DMA transfer may complete after
the flush and call uart_xmit_advance() using the flushed transfer's byte
count. The existing check for a stopped transfer relies on tx_sg, which
the flush does not clear, so it does not catch this case. The kfifo tail
then advances past its head, causing kfifo_len() to wrap and the driver
to transmit stale buffer contents. The same issue was fixed in 8250 by
commit 9e512eaaf8f4 ("serial: 8250: Fix fifo underflow on flush").

Since the TX buffer is mapped separately for each transfer and the flush
hook runs under the port lock, the driver cannot synchronously terminate
DMA and safely unmap the buffer from that context. Instead, leave the
outstanding transfer running and mark it as flushed, allowing the
completion callback to unmap the buffer without advancing the kfifo.
Clear the flag when preparing the next transfer. This avoids the kfifo
underflow, although the data already queued for DMA before the flush may
still be transmitted.

Fixes: 3a878c430fd6 ("tty: serial: msm: Add TX DMA support")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Samarth Narang <samarth.colleges@gmail.com>
---
 drivers/tty/serial/msm_serial.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c
index bfa44b01c3e9..a9eca971e9ff 100644
--- a/drivers/tty/serial/msm_serial.c
+++ b/drivers/tty/serial/msm_serial.c
@@ -185,6 +185,7 @@ struct msm_port {
 	bool			break_detected;
 	struct msm_dma		tx_dma;
 	struct msm_dma		rx_dma;
+	bool			tx_dma_flushed;
 };
 
 static inline struct msm_port *to_msm_port(struct uart_port *up)
@@ -428,6 +429,19 @@ static void msm_stop_tx(struct uart_port *port)
 	msm_write(port, msm_port->imr, MSM_UART_IMR);
 }
 
+static void msm_flush_buffer(struct uart_port *port)
+{
+	struct msm_port *msm_port = to_msm_port(port);
+
+	/*
+	 * The kfifo has been reset. Let the running transfer finish, so that
+	 * its buffer is unmapped only once it has stopped, but do not advance
+	 * the kfifo for it.
+	 */
+	if (sg_dma_len(&msm_port->tx_dma.tx_sg))
+		msm_port->tx_dma_flushed = true;
+}
+
 static void msm_start_tx(struct uart_port *port)
 {
 	struct msm_port *msm_port = to_msm_port(port);
@@ -479,7 +493,8 @@ static void msm_complete_tx_dma(void *args)
 	}
 
 	count = sg_dma_len(&dma->tx_sg) - state.residue;
-	uart_xmit_advance(port, count);
+	if (!msm_port->tx_dma_flushed)
+		uart_xmit_advance(port, count);
 	sg_init_table(&dma->tx_sg, 1);
 
 	/* Restore "Tx FIFO below watermark" interrupt */
@@ -503,6 +518,7 @@ static int msm_handle_tx_dma(struct msm_port *msm_port, unsigned int count)
 	int ret;
 	u32 val;
 
+	msm_port->tx_dma_flushed = false;
 	sg_init_table(&dma->tx_sg, 1);
 	kfifo_dma_out_prepare(&tport->xmit_fifo, &dma->tx_sg, 1, count);
 
@@ -1549,6 +1565,7 @@ static const struct uart_ops msm_uart_pops = {
 	.set_mctrl = msm_set_mctrl,
 	.get_mctrl = msm_get_mctrl,
 	.stop_tx = msm_stop_tx,
+	.flush_buffer = msm_flush_buffer,
 	.start_tx = msm_start_tx,
 	.stop_rx = msm_stop_rx,
 	.enable_ms = msm_enable_ms,
-- 
2.34.1

[PATCH 5/5] serial: pch_uart: fix TX kfifo underflow on flush

[Samarth Narang]

uart_flush_buffer() resets the transmit kfifo under the port lock before
calling the driver's flush_buffer() hook. Since pch_uart does not
implement this hook, an outstanding TX DMA transfer may complete after
the flush and call uart_xmit_advance() for each entry of the flushed
transfer's scatterlist. This can advance the kfifo tail past its head,
causing kfifo_len() to wrap and the driver to transmit stale buffer
contents. The completion callback also updates the kfifo without holding
the port lock, allowing it to race with the flush itself. The same issue
was fixed in 8250 by commit 9e512eaaf8f4 ("serial: 8250: Fix fifo
underflow on flush").

Since the TX buffer is mapped separately for each transfer and the flush
hook runs under the port lock, the driver cannot synchronously terminate
DMA and safely unmap the buffer from that context. Instead, leave the
outstanding transfer running and mark it as flushed, allowing the
completion callback to unmap the buffer without advancing the kfifo.
Take the port lock in the completion callback to synchronize access to
the flush flag and kfifo state. This prevents the underflow, although
the data already queued for DMA before the flush may still be
transmitted.

Fixes: 3c6a483275f4 ("Serial: EG20T: add PCH_UART driver")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Samarth Narang <samarth.colleges@gmail.com>
---
 drivers/tty/serial/pch_uart.c | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/pch_uart.c b/drivers/tty/serial/pch_uart.c
index 9505d255f09c..122025c4a0a6 100644
--- a/drivers/tty/serial/pch_uart.c
+++ b/drivers/tty/serial/pch_uart.c
@@ -230,6 +230,7 @@ struct eg20t_port {
 	struct scatterlist		*sg_tx_p;
 	int				nent;
 	int				orig_nent;
+	bool				tx_dma_flushed;
 	struct scatterlist		sg_rx;
 	int				tx_dma_use;
 	void				*rx_buf_virt;
@@ -750,18 +751,23 @@ static void pch_dma_tx_complete(void *arg)
 	struct eg20t_port *priv = arg;
 	struct uart_port *port = &priv->port;
 	struct scatterlist *sg = priv->sg_tx_p;
+	unsigned long flags;
 	int i;
 
-	for (i = 0; i < priv->nent; i++, sg++)
-		uart_xmit_advance(port, sg_dma_len(sg));
+	uart_port_lock_irqsave(port, &flags);
+	if (!priv->tx_dma_flushed)
+		for (i = 0; i < priv->nent; i++, sg++)
+			uart_xmit_advance(port, sg_dma_len(sg));
 
 	async_tx_ack(priv->desc_tx);
 	dma_unmap_sg(port->dev, priv->sg_tx_p, priv->orig_nent, DMA_TO_DEVICE);
 	priv->tx_dma_use = 0;
+	priv->tx_dma_flushed = false;
 	priv->nent = 0;
 	priv->orig_nent = 0;
 	kfree(priv->sg_tx_p);
 	pch_uart_hal_enable_interrupt(priv, PCH_UART_HAL_TX_INT);
+	uart_port_unlock_irqrestore(port, flags);
 }
 
 static int handle_rx_to(struct eg20t_port *priv)
@@ -947,6 +953,7 @@ static unsigned int dma_handle_tx(struct eg20t_port *priv)
 	}
 	priv->orig_nent = num;
 	priv->nent = nent;
+	priv->tx_dma_flushed = false;
 
 	for (i = 0; i < nent; i++, sg++) {
 		sg->offset = tail + fifo_size * i;
@@ -1150,6 +1157,21 @@ static void pch_uart_stop_tx(struct uart_port *port)
 	priv->tx_dma_use = 0;
 }
 
+static void pch_uart_flush_buffer(struct uart_port *port)
+{
+	struct eg20t_port *priv;
+
+	priv = container_of(port, struct eg20t_port, port);
+
+	/*
+	 * The kfifo has been reset. Let the running transfer finish, so that
+	 * its buffer is unmapped only once it has stopped, but do not advance
+	 * the kfifo for it.
+	 */
+	if (priv->nent)
+		priv->tx_dma_flushed = true;
+}
+
 static void pch_uart_start_tx(struct uart_port *port)
 {
 	struct eg20t_port *priv;
@@ -1520,6 +1542,7 @@ static const struct uart_ops pch_uart_ops = {
 	.set_mctrl = pch_uart_set_mctrl,
 	.get_mctrl = pch_uart_get_mctrl,
 	.stop_tx = pch_uart_stop_tx,
+	.flush_buffer = pch_uart_flush_buffer,
 	.start_tx = pch_uart_start_tx,
 	.stop_rx = pch_uart_stop_rx,
 	.enable_ms = pch_uart_enable_ms,
-- 
2.34.1

Re: [PATCH 2/5] serial: tegra: don't advance the TX kfifo for a flushed transfer

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] serial: tegra: tx_in_progress ABA race in tegra_uart_tx_dma_complete()
- [Medium] serial: tegra: missing async_tx_ack() in tegra_uart_flush_buffer()
--

commit 1c08600f3e0cb4a1f9dcd1d5048434c3f968101c
Author: Samarth Narang <samarth.colleges@gmail.com>

serial: tegra: don't advance the TX kfifo for a flushed transfer

This commit clears tx_in_progress in tegra_uart_flush_buffer() and checks
whether tx_in_progress is still TEGRA_UART_TX_DMA in
tegra_uart_tx_dma_complete() before advancing the transmit kfifo. This aims
to prevent an in-flight DMA completion callback from advancing a reset
kfifo after a flush or stop_tx.

> diff --git a/drivers/tty/serial/serial-tegra.c b/drivers/tty/serial/serial-tegra.c
> index b0fc4ae3686f8..8b712a9bc293e 100644
> --- a/drivers/tty/serial/serial-tegra.c
> +++ b/drivers/tty/serial/serial-tegra.c
> @@ -537,6 +537,11 @@ static void tegra_uart_tx_dma_complete(void *args)
>  	count = tup->tx_bytes_requested - state.residue;
>  	async_tx_ack(tup->tx_dma_desc);
>  	uart_port_lock_irqsave(&tup->uport, &flags);
> +	/* Already retired by stop_tx or flush_buffer */
> +	if (tup->tx_in_progress != TEGRA_UART_TX_DMA) {
> +		uart_port_unlock_irqrestore(&tup->uport, flags);
> +		return;
> +	}

[Severity: High]
Can this still race if a new TX DMA transfer is started before
tegra_uart_tx_dma_complete() acquires tup->uport.lock?

In tegra_uart_tx_dma_complete(), dmaengine_tx_status(), count, and
async_tx_ack() run before uart_port_lock_irqsave():

drivers/tty/serial/serial-tegra.c:tegra_uart_tx_dma_complete() {
	dmaengine_tx_status(tup->tx_dma_chan, tup->tx_cookie, &state);
	count = tup->tx_bytes_requested - state.residue;
	async_tx_ack(tup->tx_dma_desc);
	uart_port_lock_irqsave(&tup->uport, &flags);
	/* Already retired by stop_tx or flush_buffer */
	if (tup->tx_in_progress != TEGRA_UART_TX_DMA) {
		uart_port_unlock_irqrestore(&tup->uport, flags);
		return;
	}
	uart_xmit_advance(&tup->uport, count);
	tup->tx_in_progress = 0;

If tegra_uart_flush_buffer() or tegra_uart_stop_tx() runs while
tegra_uart_tx_dma_complete() for a first transfer is already executing or
waiting on tup->uport.lock, it clears tup->tx_in_progress = 0 and releases
the port lock. Because tup->tx_in_progress is now 0 (and kfifo_reset() in
uart_flush_buffer() resets the kfifo tail to 0, which is aligned), a
subsequent uart_write() -> tegra_uart_start_tx() ->
tegra_uart_start_next_tx() -> tegra_uart_start_tx_dma() can acquire
tup->uport.lock first, start a second DMA transfer, overwrite
tup->tx_dma_desc, tup->tx_bytes_requested, and tup->tx_cookie, and set
tup->tx_in_progress back to TEGRA_UART_TX_DMA.

When the first transfer's tegra_uart_tx_dma_complete() then takes
tup->uport.lock, tup->tx_in_progress == TEGRA_UART_TX_DMA is true again:

1. If the callback computed count from the first transfer before the second
transfer started, it calls uart_xmit_advance() with the first transfer's
count on the second transfer's kfifo (which can still underflow xmit_fifo
if the first transfer was larger than the newly queued data), clears
tup->tx_in_progress = 0 while the second transfer is active in hardware,
and calls tegra_uart_start_next_tx().

2. If the unlocked preamble in tegra_uart_tx_dma_complete() runs
concurrently with or after tegra_uart_start_tx_dma(), it reads the second
transfer's tup->tx_cookie, tup->tx_bytes_requested, and tup->tx_dma_desc
(or a torn mix of both transfers), acknowledging and retiring the second
transfer while it is still DMA_IN_PROGRESS.

By comparison, tegra_uart_rx_dma_complete() acquires tup->uport.lock before
calling dmaengine_tx_status() and bails out when status == DMA_IN_PROGRESS:

drivers/tty/serial/serial-tegra.c:tegra_uart_rx_dma_complete() {
	uart_port_lock_irqsave(u, &flags);

	status = dmaengine_tx_status(tup->rx_dma_chan, tup->rx_cookie, &state);

	if (status == DMA_IN_PROGRESS) {
		dev_dbg(tup->uport.dev, "RX DMA is in progress\n");
		goto done;
	}

Should tegra_uart_tx_dma_complete() similarly take tup->uport.lock before
querying dmaengine_tx_status() and accessing tup->tx_cookie,
tup->tx_bytes_requested, and tup->tx_dma_desc, and check for
DMA_IN_PROGRESS?

> @@ -1276,6 +1281,9 @@ static void tegra_uart_flush_buffer(struct uart_port *u)
>  	tup->tx_bytes = 0;
>  	if (tup->tx_dma_chan)
>  		dmaengine_terminate_all(tup->tx_dma_chan);
> +	/* The kfifo has been reset: the completion must not advance it */
> +	if (tup->tx_in_progress == TEGRA_UART_TX_DMA)
> +		tup->tx_in_progress = 0;

[Severity: Medium]
Does clearing tup->tx_in_progress in tegra_uart_flush_buffer() without
calling async_tx_ack(tup->tx_dma_desc) leave the aborted TX DMA descriptor
unacknowledged on tegra20-apb-dma?

In tegra_uart_start_tx_dma(), tup->tx_dma_desc is prepared with
DMA_PREP_INTERRUPT and without DMA_CTRL_ACK:

	tup->tx_dma_desc = dmaengine_prep_slave_single(tup->tx_dma_chan,
				tx_phys_addr, tup->tx_bytes, DMA_MEM_TO_DEV,
				DMA_PREP_INTERRUPT);

When tegra_uart_flush_buffer() calls dmaengine_terminate_all(),
tegra_dma_terminate_all() in drivers/dma/tegra20-apb-dma.c moves the
descriptor to tdc->free_dma_desc without invoking the completion callback.
Previously, a subsequent tegra_uart_stop_tx() saw tup->tx_in_progress ==
TEGRA_UART_TX_DMA and called async_tx_ack(tup->tx_dma_desc), but with
tegra_uart_flush_buffer() setting tup->tx_in_progress = 0, neither
tegra_uart_stop_tx() nor tegra_uart_tx_dma_complete() will acknowledge it.

Because tegra_dma_desc_get() only reuses descriptors on tdc->free_dma_desc
when async_tx_test_ack(&dma_desc->txd) is true:

drivers/dma/tegra20-apb-dma.c:tegra_dma_desc_get() {
	/* Do not allocate if desc are waiting for ack */
	list_for_each_entry(dma_desc, &tdc->free_dma_desc, node) {
		if (async_tx_test_ack(&dma_desc->txd) && !dma_desc->cb_count) {
			list_del(&dma_desc->node);
			spin_unlock_irqrestore(&tdc->lock, flags);
			dma_desc->txd.flags = 0;
			return dma_desc;
		}
	}

won't each flushed TX DMA transfer leave an unacknowledged tegra_dma_desc
stranded on tdc->free_dma_desc until the port is shut down? Also, since
tegra186-gpc-dma frees the descriptor inside dmaengine_terminate_all(),
should async_tx_ack(tup->tx_dma_desc) be called before
dmaengine_terminate_all(tup->tx_dma_chan) when tup->tx_in_progress ==
TEGRA_UART_TX_DMA?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261031123911.3784690-1-samarth.colleges@gmail.com?part=2

Re: [PATCH 4/5] tty: serial: msm: fix TX kfifo underflow on flush

[sashiko-bot]

> uart_flush_buffer() resets the transmit kfifo under the port lock before
> calling the driver's flush_buffer() hook. Since msm_serial does not
> implement this hook, an outstanding TX DMA transfer may complete after
> the flush and call uart_xmit_advance() using the flushed transfer's byte
> count. The existing check for a stopped transfer relies on tx_sg, which
> the flush does not clear, so it does not catch this case. The kfifo tail
> then advances past its head, causing kfifo_len() to wrap and the driver
> to transmit stale buffer contents. The same issue was fixed in 8250 by
> commit 9e512eaaf8f4 ("serial: 8250: Fix fifo underflow on flush").
> 
> Since the TX buffer is mapped separately for each transfer and the flush
> hook runs under the port lock, the driver cannot synchronously terminate
> DMA and safely unmap the buffer from that context. Instead, leave the
> outstanding transfer running and mark it as flushed, allowing the
> completion callback to unmap the buffer without advancing the kfifo.
> Clear the flag when preparing the next transfer. This avoids the kfifo
> underflow, although the data already queued for DMA before the flush may
> still be transmitted.
> 
> Fixes: 3a878c430fd6 ("tty: serial: msm: Add TX DMA support")
> Cc: stable@vger.kernel.org
> Assisted-by: LLM
> Signed-off-by: Samarth Narang <samarth.colleges@gmail.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261031123911.3784690-1-samarth.colleges@gmail.com?part=4

Re: [PATCH 5/5] serial: pch_uart: fix TX kfifo underflow on flush

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] serial: pch_uart: unlocked sg_tx_p read and TX DMA state overwrite after stop_tx
- [High] serial: pch_uart: lock inversion and self-deadlock in pch_dma_tx_complete()
- [High] serial: pch_uart: active DMA buffer overwrite and duplicate TX after flush
--

commit 41c9c81259479647a63a193b50f8884cd2a2a063
Author: Samarth Narang <samarth.colleges@gmail.com>

serial: pch_uart: fix TX kfifo underflow on flush

This commit adds a flush_buffer() callback to mark in-flight TX DMA
transfers as flushed and takes the port lock in pch_dma_tx_complete() so
completed flushed transfers do not advance the transmit kfifo.

> diff --git a/drivers/tty/serial/pch_uart.c b/drivers/tty/serial/pch_uart.c
> index 9505d255f09c5..122025c4a0a66 100644
> --- a/drivers/tty/serial/pch_uart.c
> +++ b/drivers/tty/serial/pch_uart.c

[ ... ]

> @@ -750,18 +751,23 @@ static void pch_dma_tx_complete(void *arg)
>  	struct eg20t_port *priv = arg;
>  	struct uart_port *port = &priv->port;
>  	struct scatterlist *sg = priv->sg_tx_p;
> +	unsigned long flags;
>  	int i;
>  
> -	for (i = 0; i < priv->nent; i++, sg++)
> -		uart_xmit_advance(port, sg_dma_len(sg));
> +	uart_port_lock_irqsave(port, &flags);

[Severity: High]
Can taking port->lock in pch_dma_tx_complete() cause a lock inversion with
pd_chan->lock as well as a single-CPU recursive deadlock on port->lock?

In pch_uart_interrupt(), the interrupt handler holds priv->port.lock while
calling dma_handle_tx(), which calls into drivers/dma/pch_dma.c:

  pch_uart_interrupt()
    uart_port_lock(&priv->port)
    dma_handle_tx()
      dmaengine_prep_slave_sg() -> pd_prep_slave_sg() -> pdc_desc_get()
        spin_lock(&pd_chan->lock)
      dmaengine_submit() -> pd_tx_submit()
        spin_lock(&pd_chan->lock)
      dma_async_issue_pending() -> pd_issue_pending()
        spin_lock(&pd_chan->lock)

This establishes the lock order priv->port.lock -> pd_chan->lock.

In drivers/dma/pch_dma.c, every path that invokes the descriptor callback
pch_dma_tx_complete() holds pd_chan->lock:

  pdc_tasklet() / pd_issue_pending() / pd_device_terminate_all()
    spin_lock(&pd_chan->lock)
    pdc_advance_work() / pdc_complete_all() / pdc_handle_error()
      pdc_chain_complete()
        dmaengine_desc_callback_invoke()
          pch_dma_tx_complete()
            uart_port_lock_irqsave(port, &flags)

On SMP, if one CPU enters pch_uart_interrupt() -> dma_handle_tx() while
another CPU runs pdc_tasklet() -> pch_dma_tx_complete(), can this deadlock
between priv->port.lock and pd_chan->lock?

In addition, in pd_tx_submit(), when pd_chan->active_list is empty,
pdc_dostart() starts the hardware DMA transfer immediately. Two lines later
in dma_handle_tx() (still holding priv->port.lock),
dma_async_issue_pending() calls pd_issue_pending():

  if (pdc_is_idle(pd_chan)) {
          spin_lock(&pd_chan->lock);
          pdc_advance_work(pd_chan);
          spin_unlock(&pd_chan->lock);
  }

If a short transfer into the UART TX FIFO has already finished by the time
pdc_is_idle() is checked, pdc_advance_work() will invoke
pch_dma_tx_complete() synchronously on the same CPU while
pch_uart_interrupt() already holds priv->port.lock.

> +	if (!priv->tx_dma_flushed)
> +		for (i = 0; i < priv->nent; i++, sg++)
> +			uart_xmit_advance(port, sg_dma_len(sg));
>  
>  	async_tx_ack(priv->desc_tx);
>  	dma_unmap_sg(port->dev, priv->sg_tx_p, priv->orig_nent, DMA_TO_DEVICE);
>  	priv->tx_dma_use = 0;
> +	priv->tx_dma_flushed = false;
>  	priv->nent = 0;
>  	priv->orig_nent = 0;
>  	kfree(priv->sg_tx_p);
>  	pch_uart_hal_enable_interrupt(priv, PCH_UART_HAL_TX_INT);
> +	uart_port_unlock_irqrestore(port, flags);
>  }
>  
>  static int handle_rx_to(struct eg20t_port *priv)
> @@ -947,6 +953,7 @@ static unsigned int dma_handle_tx(struct eg20t_port *priv)
>  	}
>  	priv->orig_nent = num;
>  	priv->nent = nent;
> +	priv->tx_dma_flushed = false;

[Severity: Critical]
What happens if pch_uart_stop_tx() is called while a TX DMA transfer is in
flight, followed by a flush and a subsequent TX restart before that first
transfer completes?

In pch_dma_tx_complete(), sg is initialized from priv->sg_tx_p before
uart_port_lock_irqsave() is called:

  struct scatterlist *sg = priv->sg_tx_p;
  ...
  uart_port_lock_irqsave(port, &flags);
  if (!priv->tx_dma_flushed)
          for (i = 0; i < priv->nent; i++, sg++)
                  uart_xmit_advance(port, sg_dma_len(sg));

Meanwhile, pch_uart_stop_tx() clears priv->tx_dma_use = 0 without stopping
the in-flight DMA transfer. If TX is restarted via pch_uart_start_tx(),
dma_handle_tx() sees priv->tx_dma_use == 0 and starts a second TX DMA
transfer while the first transfer is still running:

  priv->sg_tx_p = kmalloc_objs(struct scatterlist, num, GFP_ATOMIC);
  ...
  priv->orig_nent = num;
  priv->nent = nent;
  priv->tx_dma_flushed = false;

When the first transfer's pch_dma_tx_complete() runs:

1. Because dma_handle_tx() reset priv->tx_dma_flushed to false, the flushed
first transfer still calls uart_xmit_advance() and underflows the kfifo.

2. If pch_dma_tx_complete() read sg = priv->sg_tx_p before dma_handle_tx()
replaced priv->sg_tx_p under port->lock, and the second transfer has a
larger priv->nent than the first, the loop over priv->nent reads past the
end of the first scatterlist allocation.

3. dma_unmap_sg() and kfree(priv->sg_tx_p) in the first completion will
unmap and free the second transfer's active scatterlist while the first
scatterlist is leaked, and the second completion will then unmap and kfree()
priv->sg_tx_p a second time.

>  
>  	for (i = 0; i < nent; i++, sg++) {
>  		sg->offset = tail + fifo_size * i;
> @@ -1150,6 +1157,21 @@ static void pch_uart_stop_tx(struct uart_port *port)
>  	priv->tx_dma_use = 0;
>  }
>  
> +static void pch_uart_flush_buffer(struct uart_port *port)
> +{
> +	struct eg20t_port *priv;
> +
> +	priv = container_of(port, struct eg20t_port, port);
> +
> +	/*
> +	 * The kfifo has been reset. Let the running transfer finish, so that
> +	 * its buffer is unmapped only once it has stopped, but do not advance
> +	 * the kfifo for it.
> +	 */
> +	if (priv->nent)
> +		priv->tx_dma_flushed = true;
> +}

[Severity: High]
Could leaving the TX DMA transfer running after uart_flush_buffer() resets
the kfifo cause post-flush writes to overwrite the active DMA buffer and be
transmitted twice?

When uart_flush_buffer() runs during an active TX DMA transfer:

  uart_flush_buffer()
    uart_port_lock_irqsave()
    kfifo_reset(&state->port.xmit_fifo)
    pch_uart_flush_buffer()
      priv->tx_dma_flushed = true
    uart_port_unlock_irqrestore()
    tty_port_tty_wakeup(&state->port)

Because kfifo_reset() resets xmit_fifo.in and xmit_fifo.out to offset 0 of
state->port.xmit_buf while the in-flight DMA transfer still has
state->port.xmit_buf mapped DMA_TO_DEVICE via priv->sg_tx_p, any new data
written through uart_write() or uart_put_char() before the DMA transfer
completes will write into state->port.xmit_buf starting at offset 0 while
DMA is actively reading from that buffer.

Then, if the in-flight DMA transfer reads those newly written bytes from
offset 0 and finishes, pch_dma_tx_complete() skips uart_xmit_advance()
because priv->tx_dma_flushed is set and enables PCH_UART_HAL_TX_INT, causing
dma_handle_tx() to transmit the same post-flush bytes from xmit_fifo a
second time.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261031123911.3784690-1-samarth.colleges@gmail.com?part=5

Re: [PATCH 3/5] serial: sprd: fix TX kfifo underflow on flush

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] serial: sprd: TX DMA left running in sprd_flush_buffer() corrupts xmit_buf
- [High] serial: sprd: DMA mapping leak and TX wedge after sprd_flush_buffer()
- [Medium] serial: sprd: race in sprd_stop_tx_dma() when TX DMA just completed
--

commit 8173f9afc9b36f262ce001244563fa23087de599
Author: Samarth Narang <samarth.colleges@gmail.com>

serial: sprd: fix TX kfifo underflow on flush

This commit adds a flush_buffer() hook to sprd_serial that marks an in-flight
TX DMA transfer with tx_flushed instead of terminating it under the port
lock, so that sprd_complete_tx_dma() and sprd_stop_tx_dma() skip calling
uart_xmit_advance() after the transmit kfifo has been reset.

> diff --git a/drivers/tty/serial/sprd_serial.c b/drivers/tty/serial/sprd_serial.c
> index 3fdf5939a62c1..718732b683f1b 100644
> --- a/drivers/tty/serial/sprd_serial.c
> +++ b/drivers/tty/serial/sprd_serial.c
> @@ -133,6 +133,7 @@ struct sprd_uart_port {
>  	struct clk *clk;
>  	struct sprd_uart_dma tx_dma;
>  	struct sprd_uart_dma rx_dma;
> +	bool tx_flushed;
>  	dma_addr_t pos;
>  	unsigned char *rx_buf_tail;
>  	const struct sprd_uart_data *pdata;
> @@ -236,13 +237,15 @@ static void sprd_stop_tx_dma(struct uart_port *port)
>  	dmaengine_tx_status(sp->tx_dma.chn, sp->tx_dma.cookie, &state);
>  	if (state.residue) {
>  		trans_len = state.residue - sp->tx_dma.phys_addr;
> -		uart_xmit_advance(port, trans_len);
> +		if (!sp->tx_flushed)
> +			uart_xmit_advance(port, trans_len);
>  		dma_unmap_single(port->dev, sp->tx_dma.phys_addr,
>  				 sp->tx_dma.trans_len, DMA_TO_DEVICE);
>  	}
>  
>  	dmaengine_terminate_all(sp->tx_dma.chn);
>  	sp->tx_dma.trans_len = 0;
> +	sp->tx_flushed = false;
>  }

[Severity: Medium]
What happens if sprd_stop_tx_dma() runs just after the hardware finishes the
flushed DMA transfer, before the sprd_complete_tx_dma() tasklet runs?

After uart_flush_buffer() resets tport->xmit_fifo while leaving the TX DMA
transfer running, kfifo_is_empty(&tport->xmit_fifo) is true. Any subsequent
call to sprd_stop_tx() or to sprd_start_tx() -> sprd_start_tx_dma() while the
kfifo is still empty (such as from uart_flush_chars() or uart_start()) will
enter sprd_stop_tx_dma():

drivers/tty/serial/sprd_serial.c:sprd_start_tx_dma() {
	if (kfifo_is_empty(&tport->xmit_fifo) || uart_tx_stopped(port)) {
		sprd_stop_tx_dma(port);
		return;
	}

In drivers/dma/sprd-dma.c, when the transfer completes in hardware,
dma_irq_handle() calls vchan_cookie_complete(), which marks sp->tx_dma.cookie
as DMA_COMPLETE and schedules the vchan_complete tasklet to invoke
sprd_complete_tx_dma(). Once the cookie is marked complete,
sprd_dma_tx_status() returns with state.residue == 0.

When sprd_stop_tx_dma() sees state.residue == 0, it skips the dma_unmap_single()
block, calls dmaengine_terminate_all(sp->tx_dma.chn), and clears both
sp->tx_dma.trans_len = 0 and sp->tx_flushed = false.

If vchan_complete has not yet spliced vc->desc_completed,
sprd_dma_terminate_all() -> vchan_get_all_descriptors() frees the completed
descriptor without calling sprd_complete_tx_dma(), leaking the DMA mapping on
sp->tx_dma.phys_addr. If vchan_complete on another CPU has already spliced
vc->desc_completed and sprd_complete_tx_dma() is waiting on port->lock,
sprd_complete_tx_dma() will run after sp->tx_dma.trans_len and sp->tx_flushed
have been cleared (or after a new TX DMA transfer has started), unmapping with
trans_len == 0 or prematurely unmapping and advancing the kfifo for the new
transfer.

>  
>  static int sprd_tx_buf_remap(struct uart_port *port)
> @@ -273,7 +276,9 @@ static void sprd_complete_tx_dma(void *data)
>  	dma_unmap_single(port->dev, sp->tx_dma.phys_addr,
>  			 sp->tx_dma.trans_len, DMA_TO_DEVICE);
>  
> -	uart_xmit_advance(port, sp->tx_dma.trans_len);
> +	if (!sp->tx_flushed)
> +		uart_xmit_advance(port, sp->tx_dma.trans_len);
> +	sp->tx_flushed = false;
>  
>  	if (kfifo_len(&tport->xmit_fifo) < WAKEUP_CHARS)
>  		uart_write_wakeup(port);
> @@ -555,6 +560,20 @@ static void sprd_stop_tx(struct uart_port *port)
>  	serial_out(port, SPRD_ICLR, iclr);
>  }
>  
> +static void sprd_flush_buffer(struct uart_port *port)
> +{
> +	struct sprd_uart_port *sp = container_of(port, struct sprd_uart_port,
> +						 port);
> +
> +	/*
> +	 * The kfifo has been reset. Let the running transfer finish, so that
> +	 * its buffer is unmapped only once it has stopped, but do not advance
> +	 * the kfifo for it.
> +	 */
> +	if (sp->tx_dma.trans_len)
> +		sp->tx_flushed = true;
> +}

[Severity: High]
Can leaving the TX DMA transfer running in sprd_flush_buffer() cause subsequent
writes to overwrite the active DMA buffer in place and transmit post-flush data
twice?

In sprd_tx_buf_remap(), TX DMA does not use a separate bounce buffer; it maps
the kfifo backing buffer (state->port.xmit_buf) directly in place:

drivers/tty/serial/sprd_serial.c:sprd_tx_buf_remap() {
	sp->tx_dma.trans_len = kfifo_out_linear_ptr(&tport->xmit_fifo, &tail,
			UART_XMIT_SIZE);

	sp->tx_dma.phys_addr = dma_map_single(port->dev, tail,
					      sp->tx_dma.trans_len,
					      DMA_TO_DEVICE);

In uart_flush_buffer(), kfifo_reset(&state->port.xmit_fifo) resets both
xmit_fifo.in and xmit_fifo.out to 0 before calling port->ops->flush_buffer(),
and then wakes blocked writers via tty_port_tty_wakeup().

If a subsequent uart_write() or uart_put_char() runs while the flushed TX DMA
transfer is still in flight, kfifo_in() writes the new bytes into xmit_buf
starting at offset 0, overwriting the DMA_TO_DEVICE region while the DMA
controller is actively reading from it. In sprd_start_tx_dma(), the new write
then returns early because sp->tx_dma.trans_len is still non-zero:

drivers/tty/serial/sprd_serial.c:sprd_start_tx_dma() {
	if (sp->tx_dma.trans_len)
		return;

When the flushed DMA transfer later finishes, sprd_complete_tx_dma() sees
sp->tx_flushed == true and skips uart_xmit_advance(), leaving the newly written
bytes at xmit_fifo.out == 0, and then starts a second DMA transfer for those
same bytes:

drivers/tty/serial/sprd_serial.c:sprd_complete_tx_dma() {
	if (!sp->tx_flushed)
		uart_xmit_advance(port, sp->tx_dma.trans_len);
	sp->tx_flushed = false;
	...
	if (kfifo_is_empty(&tport->xmit_fifo) || sprd_tx_buf_remap(port) ||
	    sprd_tx_dma_config(port))
		sp->tx_dma.trans_len = 0;

[Severity: High]
Also in sprd_flush_buffer(), what happens if the port is shut down after
uart_flush_buffer() before the in-flight TX DMA transfer completes (for example
during uart_hangup() or close)?

When sprd_flush_buffer() sets sp->tx_flushed = true and leaves the transfer
running with sp->tx_dma.trans_len > 0, it relies on sprd_complete_tx_dma() (or
sprd_stop_tx_dma()) to unmap sp->tx_dma.phys_addr and reset
sp->tx_dma.trans_len and sp->tx_flushed.

However, uart_shutdown() -> uart_port_shutdown() invokes sprd_shutdown() ->
sprd_release_dma() and then frees state->port.xmit_buf in uart_free_xmit_buf():

drivers/tty/serial/sprd_serial.c:sprd_release_dma() {
	sprd_uart_dma_enable(port, false);

	if (sp->rx_dma.enable)
		dma_release_channel(sp->rx_dma.chn);

	if (sp->tx_dma.enable)
		dma_release_channel(sp->tx_dma.chn);

	sp->tx_dma.enable = false;
	sp->rx_dma.enable = false;
}

In sprd_dma_free_chan_resources(), releasing sp->tx_dma.chn stops the channel
and frees all descriptors without invoking the completion callback. Because
neither sprd_release_dma() nor sprd_startup() unmaps sp->tx_dma.phys_addr or
clears sp->tx_dma.trans_len and sp->tx_flushed, won't xmit_buf be freed while
still DMA-mapped, and won't stale non-zero sp->tx_dma.trans_len cause
sprd_start_tx_dma() to return early on every write when the port is reopened?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261031123911.3784690-1-samarth.colleges@gmail.com?part=3

Re: [PATCH 0/5] serial: fix TX kfifo underflow on flush in five drivers

[Greg Kroah-Hartman]

On Sat, Oct 31, 2026 at 12:39:06PM +0000, Samarth Narang wrote:
> This series addresses the TX kfifo underflow reported in the parent
> thread, where uart_flush_buffer() resets the transmit kfifo but a
> completion callback from an earlier DMA transfer may still call
> uart_xmit_advance() using the byte count from before the flush.
> 
> The fixes take two approaches depending on how each driver manages its
> TX DMA mappings. In samsung and tegra, which map the TX buffer once,
> samsung's new flush_buffer() terminates the outstanding transfer, as
> tegra's already does, following the approach introduced in 8250 by
> 9e512eaaf8f4. A completion already in progress skips the kfifo update if
> the transfer has since been retired by flush_buffer() or stop_tx().
> 
> In sprd, msm, and pch_uart, which map each transfer separately,
> terminating DMA there would leave the flush hook to unmap a buffer the
> controller may still be reading. These drivers therefore leave the
> transfer running and mark it as flushed, allowing the completion
> callback to unmap the buffer without advancing the kfifo. This means
> that the data already queued for DMA before the flush may still be
> transmitted.
> 
> All five patches have only been compile-tested with arm64 defconfig and
> the relevant drivers enabled, including pch_uart through COMPILE_TEST,
> using W=1 with no new warnings. I do not have access to the affected
> hardware, so testing by someone with suitable devices would be
> appreciated.

If you don't have the hardware, and can't test that the race is even
there, are you sure this is needed?  :)

What about using qemu?

thanks,

greg k-h

Re: [PATCH 1/5] tty: serial: samsung: fix TX kfifo underflow on flush

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] serial: samsung: use-after-free on tx_desc in s3c24xx_serial_flush_buffer()
- [High] tty: serial: samsung: ABA race on tx_in_progress in s3c24xx_serial_tx_dma_complete()
--

commit 73ae60b92dcc88f73c03b4939f2ea2f4d964ab3a
Author: Samarth Narang <samarth.colleges@gmail.com>

tty: serial: samsung: fix TX kfifo underflow on flush

This commit adds a flush_buffer() hook to samsung_tty to terminate any active
TX DMA transfer when the transmit buffer is flushed, and updates
s3c24xx_serial_tx_dma_complete() to skip advancing the kfifo if the transfer
was already retired.

> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> index 640f85e2d636b..2bea7ef078d2d 100644
> --- a/drivers/tty/serial/samsung_tty.c
> +++ b/drivers/tty/serial/samsung_tty.c
> @@ -317,6 +317,20 @@ static void s3c24xx_serial_stop_tx(struct uart_port *port)
>  	ourport->tx_mode = 0;
>  }
>  
> +static void s3c24xx_serial_flush_buffer(struct uart_port *port)
> +{
> +	struct s3c24xx_uart_port *ourport = to_ourport(port);
> +	struct s3c24xx_uart_dma *dma = ourport->dma;
> +
> +	if (!dma || !dma->tx_chan || ourport->tx_in_progress != S3C24XX_TX_DMA)
> +		return;
> +
> +	/* The kfifo has been reset: the completion must not advance it */
> +	dmaengine_terminate_async(dma->tx_chan);
> +	async_tx_ack(dma->tx_desc);

[Severity: High]
Can calling async_tx_ack(dma->tx_desc) after dmaengine_terminate_async() in
drivers/tty/serial/samsung_tty.c:s3c24xx_serial_flush_buffer() cause a
use-after-free write on dma->tx_desc when flushing the TTY transmit buffer
while a TX DMA transfer is active (ourport->tx_in_progress == S3C24XX_TX_DMA)?

dmaengine_terminate_async(dma->tx_chan) invokes the DMA controller's
device_terminate_all() callback synchronously before returning. On Samsung
platforms using PL330, drivers/dma/pl330.c:pl330_terminate_all() returns all
channel descriptors back to the controller-wide descriptor pool:

	list_splice_tail_init(&pch->submitted_list, &pl330->desc_pool);
	list_splice_tail_init(&pch->work_list, &pl330->desc_pool);
	list_splice_tail_init(&pch->completed_list, &pl330->desc_pool);

where another channel can immediately reallocate the descriptor via
pl330_get_desc(). Similarly, on platforms using AMBA PL08x,
drivers/dma/amba-pl08x.c:pl08x_terminate_all() frees queued descriptors via
pl08x_free_txd_list() -> vchan_dma_desc_free_list() -> kfree(txd) before
dmaengine_terminate_async() returns.

When s3c24xx_serial_flush_buffer() then calls async_tx_ack(dma->tx_desc), it
writes DMA_CTRL_ACK to dma->tx_desc->flags on a descriptor that has already
been recycled or freed. In addition, if s3c24xx_serial_tx_dma_complete() is
running concurrently on another CPU, it also calls async_tx_ack(dma->tx_desc)
before acquiring port->lock, racing with s3c24xx_serial_flush_buffer()
terminating and freeing dma->tx_desc under port->lock.

> +	ourport->tx_in_progress = 0;
> +}
> +
>  static void s3c24xx_serial_start_next_tx(struct s3c24xx_uart_port *ourport);
>  
>  static void s3c24xx_serial_tx_dma_complete(void *args)
> @@ -339,6 +353,12 @@ static void s3c24xx_serial_tx_dma_complete(void *args)
>  
>  	uart_port_lock_irqsave(port, &flags);
>  
> +	/* Already retired by stop_tx or flush_buffer */
> +	if (ourport->tx_in_progress != S3C24XX_TX_DMA) {
> +		uart_port_unlock_irqrestore(port, flags);
> +		return;
> +	}

[Severity: High]
What happens if a new TX DMA transfer starts after
drivers/tty/serial/samsung_tty.c:s3c24xx_serial_flush_buffer() clears
ourport->tx_in_progress, but before an already-running
s3c24xx_serial_tx_dma_complete() acquires port->lock?

In drivers/tty/serial/samsung_tty.c:s3c24xx_serial_tx_dma_complete(), the
completion callback computes count from dma->tx_bytes_requested and
state.residue before taking port->lock:

	dmaengine_tx_status(dma->tx_chan, dma->tx_cookie, &state);
	count = dma->tx_bytes_requested - state.residue;
	async_tx_ack(dma->tx_desc);

	dma_sync_single_for_cpu(dmaengine_get_dma_device(dma->tx_chan),
				dma->tx_transfer_addr, dma->tx_size,
				DMA_TO_DEVICE);

	uart_port_lock_irqsave(port, &flags);

Because s3c24xx_serial_flush_buffer() uses dmaengine_terminate_async(), it
does not wait for an in-flight completion callback to finish before clearing
ourport->tx_in_progress = 0 and releasing port->lock in uart_flush_buffer().

If a subsequent or woken uart_write() of at least ourport->min_dma_size bytes
acquires port->lock before the stale s3c24xx_serial_tx_dma_complete() does,
__uart_start() -> s3c24xx_serial_start_tx() -> s3c24xx_serial_start_next_tx()
-> s3c24xx_serial_start_tx_dma() will see ourport->tx_in_progress == 0 and the
reset kfifo tail aligned to a cache line, overwrite the dma->tx_* fields, and
set ourport->tx_in_progress = S3C24XX_TX_DMA for the new transfer:

	dma->tx_bytes_requested = dma->tx_size;

	ourport->tx_in_progress = S3C24XX_TX_DMA;
	dma->tx_cookie = dmaengine_submit(dma->tx_desc);
	dma_async_issue_pending(dma->tx_chan);

When the first transfer's s3c24xx_serial_tx_dma_complete() then acquires
port->lock, ourport->tx_in_progress is S3C24XX_TX_DMA again. Wouldn't it
bypass the ourport->tx_in_progress != S3C24XX_TX_DMA check, call
uart_xmit_advance(port, count) with the first transfer's byte count on the
newly written kfifo (underflowing xmit_fifo if the new write is smaller than
count), and clear ourport->tx_in_progress = 0 while the second DMA transfer is
still active?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261031123911.3784690-1-samarth.colleges@gmail.com?part=1