[PATCH 0/2] serial: 8250: serialize DMA cancellation with buffer reuse

[PATCH 0/2] serial: 8250: serialize DMA cancellation with buffer reuse

[James Hilliard]

Keep canceled transmit and receive buffers reserved until the provider's
completion callbacks have finished. The TX fix also separates the DMA
source from the serial core's kfifo, which is reset before the driver is
called to flush a transfer. The RX fix keeps partial-buffer delivery ahead
of the PIO fallback while the canceled descriptor is retired.

These changes repair the existing finite-transfer DMA path.
DMA providers must honor the existing terminate/synchronize contract;
fixes for providers that do not are separate DMAengine work.

Based on tty-next with the posted shutdown guard [0], channel
configuration error handling [1], and RX TTY accounting fix [2].

[0] https://lore.kernel.org/r/20261021-submit-serial-8250-dma-shutdown-v1-v1-1-3d3ce383c614@gmail.com
[1] https://lore.kernel.org/r/20261021-submit-serial-8250-dma-config-errors-v1-v1-1-c067587d1ef2@gmail.com
[2] https://lore.kernel.org/r/20261021-submit-serial-8250-dma-rx-accounting-v1-v1-1-c11795914ab4@gmail.com

Assisted-by: Codex:gpt-6-astra
Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
James Hilliard (2):
      serial: 8250: drain canceled TX DMA before reusing its buffer
      serial: 8250: retire canceled RX DMA before reusing the receive buffer

 drivers/tty/serial/8250/8250.h      |  20 +++-
 drivers/tty/serial/8250/8250_dma.c  | 205 ++++++++++++++++++++++++++----------
 drivers/tty/serial/8250/8250_dw.c   |   8 +-
 drivers/tty/serial/8250/8250_mid.c  |   6 +-
 drivers/tty/serial/8250/8250_omap.c |  26 +++--
 drivers/tty/serial/8250/8250_port.c |   3 +-
 6 files changed, 191 insertions(+), 77 deletions(-)
---
base-commit: 1184f87e2790e8af9be3116f608d2e86b4792d7d
change-id: 20261021-submit-serial-8250-dma-cancel-v1-b208f84e89c1
prerequisite-patch-id: 3604bfaa3a1c214997de1f11162b81a4541af5ab
prerequisite-patch-id: 79dd093ffb35e254e790dffb8887faf51b64f947
prerequisite-patch-id: 80a5061d84c055a64c760268febed179c687b99f

Best regards,
--  
James Hilliard <james.hilliard1@gmail.com>

[PATCH 1/2] serial: 8250: drain canceled TX DMA before reusing its buffer

[James Hilliard]

uart_flush_buffer() resets the transmit kfifo before calling the driver's
flush callback. serial8250_tx_dma_flush() terminates DMA asynchronously and
clears tx_running, allowing a new write to replace tx_size before the old
completion callback has finished. That callback can then consume bytes
from the new transmission. The serial core can also overwrite the old
DMA source as soon as it reuses the reset kfifo.

Give DMA a private transmit buffer, keeping the kfifo offsets used by
FIFO-window providers. Copy only the queued ranges before handing the
buffer to the device. Flushing the kfifo can then no longer change an
in-flight transfer's source.

Keep tx_running set while a worker synchronizes a successfully terminated
channel. Suppress callbacks and new submissions during that interval.
The DesignWare idle helpers must not pause or resume a retiring channel.
Once synchronization finishes, process the zero-length completion under
the port lock and start any data queued after the flush. Reuse the normal
completion processing for OMAP too, including its delayed register
restore and runtime-PM handling. Drain the worker before releasing DMA
resources on shutdown.

Calculate the queued length before calling the platform preparation hook,
so it sees the length of the transfer being prepared rather than the
previous transfer's length.

If termination fails, leave the buffer owned by the transfer and let its
normal completion release it. Do not submit another transfer before that
completion. A DMA completion still does not mean that the UART transmitter
is empty; the existing line-status checks remain in place.

The race was found by inspecting cancellation and restart while preparing
H616 UART DMA support.

Fixes: a424a34b8fad ("serial: 8250: Fix TX deadlock when using DMA")
Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
 drivers/tty/serial/8250/8250.h      |  13 +++-
 drivers/tty/serial/8250/8250_dma.c  | 118 ++++++++++++++++++++++++++----------
 drivers/tty/serial/8250/8250_omap.c |  26 ++++----
 3 files changed, 111 insertions(+), 46 deletions(-)

diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index 457732e941af..002e456fa384 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -11,6 +11,7 @@
 #include <linux/serial_8250.h>
 #include <linux/serial_core.h>
 #include <linux/dmaengine.h>
+#include <linux/workqueue.h>
 
 #include "../serial_mctrl_gpio.h"
 
@@ -19,6 +20,10 @@ struct uart_8250_dma {
 	int (*rx_dma)(struct uart_8250_port *p);
 	void (*prepare_tx_dma)(struct uart_8250_port *p);
 	void (*prepare_rx_dma)(struct uart_8250_port *p);
+	/* Completion processing with the port lock held. */
+	void (*tx_complete)(struct uart_8250_port *p);
+	struct work_struct tx_flush_work;
+	struct uart_8250_port *port;
 
 	/* Filter function */
 	dma_filter_fn		fn;
@@ -44,6 +49,7 @@ struct uart_8250_dma {
 	dma_cookie_t		tx_cookie;
 
 	void			*rx_buf;
+	void			*tx_buf;
 
 	size_t			rx_size;
 	size_t			tx_size;
@@ -53,6 +59,7 @@ struct uart_8250_dma {
 	unsigned char		rx_running;
 	/* Protected by the UART port lock. */
 	bool			shutting_down;
+	bool			tx_terminating;
 };
 
 struct old_serial_port {
@@ -401,6 +408,8 @@ static inline int is_omap1510_8250(struct uart_8250_port *pt)
 #ifdef CONFIG_SERIAL_8250_DMA
 extern int serial8250_tx_dma(struct uart_8250_port *);
 extern void serial8250_tx_dma_flush(struct uart_8250_port *);
+void serial8250_tx_dma_prepare_buffer(struct uart_8250_port *p,
+				      struct scatterlist *sgl, unsigned int nents);
 extern int serial8250_rx_dma(struct uart_8250_port *);
 extern void serial8250_rx_dma_flush(struct uart_8250_port *);
 extern int serial8250_request_dma(struct uart_8250_port *);
@@ -433,7 +442,7 @@ static inline void serial8250_tx_dma_pause(struct uart_8250_port *p)
 {
 	struct uart_8250_dma *dma = p->dma;
 
-	if (!dma->tx_running)
+	if (!dma->tx_running || dma->tx_terminating || dma->shutting_down)
 		return;
 
 	dmaengine_pause(dma->txchan);
@@ -443,7 +452,7 @@ static inline void serial8250_tx_dma_resume(struct uart_8250_port *p)
 {
 	struct uart_8250_dma *dma = p->dma;
 
-	if (!dma->tx_running)
+	if (!dma->tx_running || dma->tx_terminating || dma->shutting_down)
 		return;
 
 	dmaengine_resume(dma->txchan);
diff --git a/drivers/tty/serial/8250/8250_dma.c b/drivers/tty/serial/8250/8250_dma.c
index 60e99a0db9b8..d4b1e2a059eb 100644
--- a/drivers/tty/serial/8250/8250_dma.c
+++ b/drivers/tty/serial/8250/8250_dma.c
@@ -8,21 +8,16 @@
 #include <linux/tty_flip.h>
 #include <linux/serial_reg.h>
 #include <linux/dma-mapping.h>
+#include <linux/slab.h>
 
 #include "8250.h"
 
-static void __dma_tx_complete(void *param)
+static void serial8250_tx_dma_complete(struct uart_8250_port *p)
 {
-	struct uart_8250_port	*p = param;
 	struct uart_8250_dma	*dma = p->dma;
 	struct tty_port		*tport = &p->port.state->port;
-	unsigned long	flags;
 	int		ret;
 
-	uart_port_lock_irqsave(&p->port, &flags);
-	if (dma->shutting_down)
-		goto out;
-
 	dma_sync_single_for_cpu(dmaengine_get_dma_device(dma->txchan), dma->tx_addr,
 				UART_XMIT_SIZE, DMA_TO_DEVICE);
 
@@ -36,11 +31,52 @@ static void __dma_tx_complete(void *param)
 	ret = serial8250_tx_dma(p);
 	if (ret || !dma->tx_running)
 		serial8250_set_THRI(p);
+}
 
-out:
-	uart_port_unlock_irqrestore(&p->port, flags);
+static void __dma_tx_complete(void *param)
+{
+	struct uart_8250_port *p = param;
+	struct uart_8250_dma *dma = p->dma;
+
+	guard(uart_port_lock_irqsave)(&p->port);
+	if (!dma->shutting_down && !dma->tx_terminating)
+		serial8250_tx_dma_complete(p);
 }
 
+static void serial8250_tx_dma_flush_work(struct work_struct *work)
+{
+	struct uart_8250_dma *dma = container_of(work, struct uart_8250_dma,
+					       tx_flush_work);
+	struct uart_8250_port *p = dma->port;
+
+	/* Neither the channel nor its buffer may be reused before this returns. */
+	dmaengine_synchronize(dma->txchan);
+
+	guard(uart_port_lock_irqsave)(&p->port);
+	dma->tx_terminating = false;
+	if (!dma->shutting_down)
+		dma->tx_complete(p);
+}
+
+void serial8250_tx_dma_prepare_buffer(struct uart_8250_port *p,
+				    struct scatterlist *sgl, unsigned int nents)
+{
+	struct uart_8250_dma *dma = p->dma;
+	struct scatterlist *sg;
+	unsigned int i;
+
+	/* Preserve ring offsets for DMA engines which implement a FIFO window. */
+	for_each_sg(sgl, sg, nents, i) {
+		size_t offset = sg_dma_address(sg) - dma->tx_addr;
+
+		memcpy(dma->tx_buf + offset,
+		       p->port.state->port.xmit_buf + offset, sg_dma_len(sg));
+	}
+	dma_sync_single_for_device(dmaengine_get_dma_device(dma->txchan), dma->tx_addr,
+				   UART_XMIT_SIZE, DMA_TO_DEVICE);
+}
+EXPORT_SYMBOL_GPL(serial8250_tx_dma_prepare_buffer);
+
 static void __dma_rx_complete(struct uart_8250_port *p)
 {
 	struct uart_8250_dma	*dma = p->dma;
@@ -103,6 +139,8 @@ int serial8250_tx_dma(struct uart_8250_port *p)
 
 	if (dma->shutting_down)
 		return 0;
+	if (dma->tx_terminating)
+		return 0;
 
 	if (dma->tx_running) {
 		if (up->x_char) {
@@ -120,8 +158,6 @@ int serial8250_tx_dma(struct uart_8250_port *p)
 		return 0;
 	}
 
-	serial8250_do_prepare_tx_dma(p);
-
 	sg_init_table(sgl, ARRAY_SIZE(sgl));
 
 	ret = kfifo_dma_out_prepare_mapped(&tport->xmit_fifo, sgl, ARRAY_SIZE(sgl),
@@ -132,6 +168,8 @@ int serial8250_tx_dma(struct uart_8250_port *p)
 	for_each_sg(sgl, sg, ret, i)
 		dma->tx_size += sg_dma_len(sg);
 
+	serial8250_do_prepare_tx_dma(p);
+
 	desc = dmaengine_prep_slave_sg(dma->txchan, sgl, ret,
 				       DMA_MEM_TO_DEV,
 				       DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
@@ -141,14 +179,13 @@ int serial8250_tx_dma(struct uart_8250_port *p)
 	}
 
 	dma->tx_running = 1;
+	dma->tx_complete = serial8250_tx_dma_complete;
 	desc->callback = __dma_tx_complete;
 	desc->callback_param = p;
 
-	dma->tx_cookie = dmaengine_submit(desc);
-
-	dma_sync_single_for_device(dmaengine_get_dma_device(dma->txchan), dma->tx_addr,
-				   UART_XMIT_SIZE, DMA_TO_DEVICE);
+	serial8250_tx_dma_prepare_buffer(p, sgl, ret);
 
+	dma->tx_cookie = dmaengine_submit(desc);
 	dma_async_issue_pending(dma->txchan);
 	serial8250_clear_THRI(p);
 	dma->tx_err = 0;
@@ -162,8 +199,9 @@ int serial8250_tx_dma(struct uart_8250_port *p)
 void serial8250_tx_dma_flush(struct uart_8250_port *p)
 {
 	struct uart_8250_dma *dma = p->dma;
+	int ret;
 
-	if (!dma->tx_running)
+	if (!dma->tx_running || dma->tx_terminating || dma->shutting_down)
 		return;
 
 	/*
@@ -176,18 +214,14 @@ void serial8250_tx_dma_flush(struct uart_8250_port *p)
 	 * We can't use `dmaengine_terminate_sync` because `uart_flush_buffer` is
 	 * holding the uart port spinlock.
 	 */
-	dmaengine_terminate_async(dma->txchan);
-
-	/*
-	 * The callback might or might not run. If it doesn't run, we need to ensure
-	 * that `tx_running` is cleared so that we can schedule new transactions.
-	 * If it does run, then the zombie callback will clear `tx_running` again
-	 * and perform a no-op since `tx_size` was cleared above.
-	 *
-	 * In either case, we ASSUME the DMA transaction will terminate before we
-	 * issue a new `serial8250_tx_dma`.
-	 */
-	dma->tx_running = 0;
+	dma->tx_terminating = true;
+	ret = dmaengine_terminate_async(dma->txchan);
+	if (ret) {
+		/* Keep the buffer owned by the transfer until its callback runs. */
+		dma->tx_terminating = false;
+		return;
+	}
+	schedule_work(&dma->tx_flush_work);
 }
 
 int serial8250_rx_dma(struct uart_8250_port *p)
@@ -313,20 +347,34 @@ int serial8250_request_dma(struct uart_8250_port *p)
 	}
 
 	/* TX buffer */
-	dma->tx_addr = dma_map_single(tx_dev, p->port.state->port.xmit_buf,
-				      UART_XMIT_SIZE, DMA_TO_DEVICE);
+	dma->tx_buf = kmalloc(UART_XMIT_SIZE, GFP_KERNEL);
+	if (!dma->tx_buf) {
+		ret = -ENOMEM;
+		goto free_rx;
+	}
+	dma->tx_addr = dma_map_single(tx_dev, dma->tx_buf,
+				     UART_XMIT_SIZE, DMA_TO_DEVICE);
 	if (dma_mapping_error(tx_dev, dma->tx_addr)) {
-		dma_free_coherent(rx_dev, dma->rx_size,
-				  dma->rx_buf, dma->rx_addr);
 		ret = -ENOMEM;
-		goto err;
+		goto free_tx;
 	}
+	dma_sync_single_for_cpu(tx_dev, dma->tx_addr,
+				UART_XMIT_SIZE, DMA_TO_DEVICE);
+
+	dma->port = p;
+	INIT_WORK(&dma->tx_flush_work, serial8250_tx_dma_flush_work);
+	dma->tx_terminating = false;
 
 	scoped_guard(uart_port_lock_irqsave, &p->port)
 		dma->shutting_down = false;
 	dev_dbg_ratelimited(p->port.dev, "got both dma channels\n");
 
 	return 0;
+free_tx:
+	kfree(dma->tx_buf);
+free_rx:
+	dma_free_coherent(rx_dev, dma->rx_size,
+			  dma->rx_buf, dma->rx_addr);
 err:
 	dma_release_channel(dma->txchan);
 release_rx:
@@ -342,6 +390,8 @@ void serial8250_release_dma(struct uart_8250_port *p)
 	if (!dma)
 		return;
 
+	cancel_work_sync(&dma->tx_flush_work);
+
 	/* Release RX resources */
 	dmaengine_terminate_sync(dma->rxchan);
 	dma->rx_running = 0;
@@ -354,6 +404,8 @@ void serial8250_release_dma(struct uart_8250_port *p)
 	dmaengine_terminate_sync(dma->txchan);
 	dma_unmap_single(dmaengine_get_dma_device(dma->txchan), dma->tx_addr,
 			 UART_XMIT_SIZE, DMA_TO_DEVICE);
+	kfree(dma->tx_buf);
+	dma->tx_buf = NULL;
 	dma_release_channel(dma->txchan);
 	dma->txchan = NULL;
 	dma->tx_running = 0;
diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
index e3f3322da379..d8ea1a1a406a 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -1079,18 +1079,13 @@ static int omap_8250_rx_dma(struct uart_8250_port *p)
 
 static int omap_8250_tx_dma(struct uart_8250_port *p);
 
-static void omap_8250_dma_tx_complete(void *param)
+static void omap_8250_tx_dma_complete(struct uart_8250_port *p)
 {
-	struct uart_8250_port	*p = param;
 	struct uart_8250_dma	*dma = p->dma;
 	struct tty_port		*tport = &p->port.state->port;
 	bool			en_thri = false;
 	struct omap8250_priv	*priv = p->port.private_data;
 
-	guard(uart_port_lock_irqsave)(&p->port);
-	if (dma->shutting_down)
-		return;
-
 	dma_sync_single_for_cpu(dmaengine_get_dma_device(dma->txchan), dma->tx_addr,
 				UART_XMIT_SIZE, DMA_TO_DEVICE);
 
@@ -1122,6 +1117,16 @@ static void omap_8250_dma_tx_complete(void *param)
 	}
 }
 
+static void omap_8250_dma_tx_complete(void *param)
+{
+	struct uart_8250_port *p = param;
+	struct uart_8250_dma *dma = p->dma;
+
+	guard(uart_port_lock_irqsave)(&p->port);
+	if (!dma->shutting_down && !dma->tx_terminating)
+		omap_8250_tx_dma_complete(p);
+}
+
 static int omap_8250_tx_dma(struct uart_8250_port *p)
 {
 	struct uart_8250_dma		*dma = p->dma;
@@ -1132,7 +1137,7 @@ static int omap_8250_tx_dma(struct uart_8250_port *p)
 	int skip_byte = -1;
 	int ret;
 
-	if (dma->shutting_down || dma->tx_running)
+	if (dma->shutting_down || dma->tx_terminating || dma->tx_running)
 		return 0;
 	if (uart_tx_stopped(&p->port) || kfifo_is_empty(&tport->xmit_fifo)) {
 
@@ -1200,15 +1205,14 @@ static int omap_8250_tx_dma(struct uart_8250_port *p)
 
 	dma->tx_size = sg_dma_len(&sg);
 	dma->tx_running = 1;
+	dma->tx_complete = omap_8250_tx_dma_complete;
 
 	desc->callback = omap_8250_dma_tx_complete;
 	desc->callback_param = p;
 
-	dma->tx_cookie = dmaengine_submit(desc);
-
-	dma_sync_single_for_device(dmaengine_get_dma_device(dma->txchan), dma->tx_addr,
-				   UART_XMIT_SIZE, DMA_TO_DEVICE);
+	serial8250_tx_dma_prepare_buffer(p, &sg, 1);
 
+	dma->tx_cookie = dmaengine_submit(desc);
 	dma_async_issue_pending(dma->txchan);
 	if (dma->tx_err)
 		dma->tx_err = 0;

-- 
2.53.0

Re: [PATCH 1/2] serial: 8250: drain canceled TX DMA before reusing its buffer

[sashiko-bot]

> uart_flush_buffer() resets the transmit kfifo before calling the driver's
> flush callback. serial8250_tx_dma_flush() terminates DMA asynchronously and
> clears tx_running, allowing a new write to replace tx_size before the old
> completion callback has finished. That callback can then consume bytes
> from the new transmission. The serial core can also overwrite the old
> DMA source as soon as it reuses the reset kfifo.
> 
> Give DMA a private transmit buffer, keeping the kfifo offsets used by
> FIFO-window providers. Copy only the queued ranges before handing the
> buffer to the device. Flushing the kfifo can then no longer change an
> in-flight transfer's source.
> 
> Keep tx_running set while a worker synchronizes a successfully terminated
> channel. Suppress callbacks and new submissions during that interval.
> The DesignWare idle helpers must not pause or resume a retiring channel.
> [ ... ]
> The race was found by inspecting cancellation and restart while preparing
> H616 UART DMA support.
> 
> Fixes: a424a34b8fad ("serial: 8250: Fix TX deadlock when using DMA")
> Signed-off-by: James Hilliard <james.hilliard1@gmail.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021-submit-serial-8250-dma-cancel-v1-v1-0-efe2b89f2303@gmail.com?part=1

Re: [PATCH 1/2] serial: 8250: drain canceled TX DMA before reusing its buffer

[Andy Shevchenko]

On Wed, Oct 21, 2026 at 04:37:43PM -0600, James Hilliard wrote:
> uart_flush_buffer() resets the transmit kfifo before calling the driver's
> flush callback. serial8250_tx_dma_flush() terminates DMA asynchronously and
> clears tx_running, allowing a new write to replace tx_size before the old
> completion callback has finished. That callback can then consume bytes
> from the new transmission. The serial core can also overwrite the old
> DMA source as soon as it reuses the reset kfifo.
> 
> Give DMA a private transmit buffer, keeping the kfifo offsets used by
> FIFO-window providers. Copy only the queued ranges before handing the
> buffer to the device. Flushing the kfifo can then no longer change an
> in-flight transfer's source.
> 
> Keep tx_running set while a worker synchronizes a successfully terminated
> channel. Suppress callbacks and new submissions during that interval.
> The DesignWare idle helpers must not pause or resume a retiring channel.
> Once synchronization finishes, process the zero-length completion under
> the port lock and start any data queued after the flush. Reuse the normal
> completion processing for OMAP too, including its delayed register
> restore and runtime-PM handling. Drain the worker before releasing DMA
> resources on shutdown.
> 
> Calculate the queued length before calling the platform preparation hook,
> so it sees the length of the transfer being prepared rather than the
> previous transfer's length.
> 
> If termination fails, leave the buffer owned by the transfer and let its
> normal completion release it. Do not submit another transfer before that
> completion. A DMA completion still does not mean that the UART transmitter
> is empty; the existing line-status checks remain in place.
> 
> The race was found by inspecting cancellation and restart while preparing
> H616 UART DMA support.

...

> +static void __dma_tx_complete(void *param)
> +{
> +	struct uart_8250_port *p = param;
> +	struct uart_8250_dma *dma = p->dma;
> +
> +	guard(uart_port_lock_irqsave)(&p->port);

Add a blank line after guard()() everywhere, otherwise it's harder to see it
in the code.

> +	if (!dma->shutting_down && !dma->tx_terminating)
> +		serial8250_tx_dma_complete(p);
>  }

...

> +static void serial8250_tx_dma_flush_work(struct work_struct *work)
> +{
> +	struct uart_8250_dma *dma = container_of(work, struct uart_8250_dma,
> +					       tx_flush_work);

Make it a single line.

> +	struct uart_8250_port *p = dma->port;
> +
> +	/* Neither the channel nor its buffer may be reused before this returns. */
> +	dmaengine_synchronize(dma->txchan);
> +
> +	guard(uart_port_lock_irqsave)(&p->port);
> +	dma->tx_terminating = false;
> +	if (!dma->shutting_down)
> +		dma->tx_complete(p);
> +}
> +
> +void serial8250_tx_dma_prepare_buffer(struct uart_8250_port *p,
> +				    struct scatterlist *sgl, unsigned int nents)
> +{
> +	struct uart_8250_dma *dma = p->dma;
> +	struct scatterlist *sg;
> +	unsigned int i;
> +
> +	/* Preserve ring offsets for DMA engines which implement a FIFO window. */
> +	for_each_sg(sgl, sg, nents, i) {
> +		size_t offset = sg_dma_address(sg) - dma->tx_addr;
> +
> +		memcpy(dma->tx_buf + offset,
> +		       p->port.state->port.xmit_buf + offset, sg_dma_len(sg));
> +	}
> +	dma_sync_single_for_device(dmaengine_get_dma_device(dma->txchan), dma->tx_addr,
> +				   UART_XMIT_SIZE, DMA_TO_DEVICE);
> +}

This is a bounce buffer, why do we need it? Shouldn't this be rather the
respective DMAengine issue on how to treat the pieces?

...

Overall I have a feeling that we turned somewhere in a wrong direction.
This all activity on 8250 DMA support looks like over engineered approach.

Perhaps we need to reconsider the whole picture?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] serial: 8250: drain canceled TX DMA before reusing its buffer

[James Hilliard]

On Thu, Oct 22, 2026 at 8:00 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Wed, Oct 21, 2026 at 04:37:43PM -0600, James Hilliard wrote:
> > uart_flush_buffer() resets the transmit kfifo before calling the driver's
> > flush callback. serial8250_tx_dma_flush() terminates DMA asynchronously and
> > clears tx_running, allowing a new write to replace tx_size before the old
> > completion callback has finished. That callback can then consume bytes
> > from the new transmission. The serial core can also overwrite the old
> > DMA source as soon as it reuses the reset kfifo.
> >
> > Give DMA a private transmit buffer, keeping the kfifo offsets used by
> > FIFO-window providers. Copy only the queued ranges before handing the
> > buffer to the device. Flushing the kfifo can then no longer change an
> > in-flight transfer's source.
> >
> > Keep tx_running set while a worker synchronizes a successfully terminated
> > channel. Suppress callbacks and new submissions during that interval.
> > The DesignWare idle helpers must not pause or resume a retiring channel.
> > Once synchronization finishes, process the zero-length completion under
> > the port lock and start any data queued after the flush. Reuse the normal
> > completion processing for OMAP too, including its delayed register
> > restore and runtime-PM handling. Drain the worker before releasing DMA
> > resources on shutdown.
> >
> > Calculate the queued length before calling the platform preparation hook,
> > so it sees the length of the transfer being prepared rather than the
> > previous transfer's length.
> >
> > If termination fails, leave the buffer owned by the transfer and let its
> > normal completion release it. Do not submit another transfer before that
> > completion. A DMA completion still does not mean that the UART transmitter
> > is empty; the existing line-status checks remain in place.
> >
> > The race was found by inspecting cancellation and restart while preparing
> > H616 UART DMA support.
>
> ...
>
> > +static void __dma_tx_complete(void *param)
> > +{
> > +     struct uart_8250_port *p = param;
> > +     struct uart_8250_dma *dma = p->dma;
> > +
> > +     guard(uart_port_lock_irqsave)(&p->port);
>
> Add a blank line after guard()() everywhere, otherwise it's harder to see it
> in the code.
>
> > +     if (!dma->shutting_down && !dma->tx_terminating)
> > +             serial8250_tx_dma_complete(p);
> >  }
>
> ...
>
> > +static void serial8250_tx_dma_flush_work(struct work_struct *work)
> > +{
> > +     struct uart_8250_dma *dma = container_of(work, struct uart_8250_dma,
> > +                                            tx_flush_work);
>
> Make it a single line.
>
> > +     struct uart_8250_port *p = dma->port;
> > +
> > +     /* Neither the channel nor its buffer may be reused before this returns. */
> > +     dmaengine_synchronize(dma->txchan);
> > +
> > +     guard(uart_port_lock_irqsave)(&p->port);
> > +     dma->tx_terminating = false;
> > +     if (!dma->shutting_down)
> > +             dma->tx_complete(p);
> > +}
> > +
> > +void serial8250_tx_dma_prepare_buffer(struct uart_8250_port *p,
> > +                                 struct scatterlist *sgl, unsigned int nents)
> > +{
> > +     struct uart_8250_dma *dma = p->dma;
> > +     struct scatterlist *sg;
> > +     unsigned int i;
> > +
> > +     /* Preserve ring offsets for DMA engines which implement a FIFO window. */
> > +     for_each_sg(sgl, sg, nents, i) {
> > +             size_t offset = sg_dma_address(sg) - dma->tx_addr;
> > +
> > +             memcpy(dma->tx_buf + offset,
> > +                    p->port.state->port.xmit_buf + offset, sg_dma_len(sg));
> > +     }
> > +     dma_sync_single_for_device(dmaengine_get_dma_device(dma->txchan), dma->tx_addr,
> > +                                UART_XMIT_SIZE, DMA_TO_DEVICE);
> > +}
>
> This is a bounce buffer, why do we need it? Shouldn't this be rather the
> respective DMAengine issue on how to treat the pieces?

The problem is ownership of the serial core's transmit storage across
flush. uart_flush_buffer() resets the FIFO before calling the driver,
and writers can reuse that storage after the callback returns.

dmaengine_terminate_async() does not guarantee that the transfer or its
completion callback has finished when it returns. The driver's flush
callback holds the port spinlock, so it cannot wait there using
dmaengine_synchronize(). This lifetime gap therefore exists even with
a provider implementing the DMAengine contract correctly.

The bounce buffer in v1 protected the transfer's source from those new
writes, but it added an allocation and copying to normal transmission.

> ...
>
> Overall I have a feeling that we turned somewhere in a wrong direction.
> This all activity on 8250 DMA support looks like over engineered approach.
>
> Perhaps we need to reconsider the whole picture?

I've reworked TX in v2[0] around a serial-core storage reservation instead.
The logical FIFO is still flushed immediately, but the driver can reserve
its storage until DMA and the old callbacks have retired. During that
interval write_room() reports zero, and write()/put_char() accept no data.

Normal transmission continues to DMA directly from the FIFO. A worker
performs synchronization outside the port lock, returns the mapping to
CPU ownership and releases the reservation. This removes the private TX
buffer and copying while retaining the required lifetime protection.

[0] https://lore.kernel.org/r/20261022-submit-serial-8250-dma-cancel-v1-v2-0-088ea51e3891@gmail.com/

>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Re: [PATCH 1/2] serial: 8250: drain canceled TX DMA before reusing its buffer

[Raul E Rangel]

Hi James,

Thanks for looking into the asynchronous termination race with xmit_fifo
reuse. A few issues with this patch:

> +static void serial8250_tx_dma_flush_work(struct work_struct *work)
> +{
> +	struct uart_8250_dma *dma = container_of(work, struct uart_8250_dma,
> +						 tx_flush_work);
> +	struct uart_8250_port *p = dma->port;
> +
> +	dmaengine_synchronize(dma->txchan);
> +
> +	guard(uart_port_lock_irqsave)(&p->port);
> +	dma->tx_terminating = false;
> +	if (!dma->shutting_down)
> +		dma->tx_complete(p);
> +}

1. Missing runtime PM reference in process context:
   `serial8250_tx_dma_flush_work()` runs asynchronously on `system_wq` and
   calls `dma->tx_complete(p)` (`serial8250_tx_dma_complete()` or
   `omap_8250_tx_dma_complete()`), which accesses UART registers
   (`UART_IER`, `RZN1_UART_TDMACR`, `UART_TX`, `omap8250_restore_regs()`)
   without calling `serial8250_rpm_get(p)` / `serial8250_rpm_put(p)`. If the
   port autosuspended before the work item ran, this will access unclocked
   registers. Also, on `UART_CAP_RPM` ports (such as `8250_omap`), if
   `dma->shutting_down` is true here, `dma->tx_complete(p)` is skipped and
   `serial8250_rpm_put_tx(p)` is never called, leaking `rpm_tx_active`.

2. `dmaengine_synchronize()` is a no-op on several 8250 DMA providers:
   `dmaengine_synchronize()` only waits if the provider implements
   `.device_synchronize`. Several DMA controllers used with 8250 — including
   `drivers/dma/sun6i-dma.c` (Allwinner H616, mentioned in your commit
   message), `drivers/dma/dw/core.c` (`dw_dmac`), `drivers/dma/pl330.c`, and
   `drivers/dma/mediatek/mtk-uart-apdma.c` — schedule callbacks from a
   tasklet (`virt-dma` `vc.task` or driver tasklet) but do not wire up
   `.device_synchronize`. On those controllers, `dmaengine_synchronize()`
   returns immediately while a completion tasklet from the old transfer can
   still be scheduled, so clearing `dma->tx_terminating = false` and starting
   a new transfer re-exposes the stale-callback race.

> +void serial8250_tx_dma_flush(struct uart_8250_port *p)
> +{
...
>  	dma->tx_size = 0;
> +	dma->tx_terminating = true;
> +	ret = dmaengine_terminate_async(dma->txchan);
> +	if (ret) {
> +		/* Keep the buffer owned by the transfer until its callback runs. */
> +		dma->tx_terminating = false;
> +		return;
> +	}

3. Zeroing `dma->tx_size = 0` before `dmaengine_terminate_async()`:
   If `dmaengine_terminate_async()` fails (`ret != 0`), the transfer
   continues to completion, but `dma->tx_size` has already been zeroed so
   `uart_xmit_advance()` in `tx_complete()` will not account for the
   transmitted bytes in `icount.tx`.

> +void serial8250_tx_dma_prepare_buffer(struct uart_8250_port *p,
> +				     struct scatterlist *sgl, unsigned int nents)
> +{
> +	struct uart_8250_dma *dma = p->dma;
> +	struct scatterlist *sg;
> +	unsigned int i;
> +
> +	for_each_sg(sgl, sg, nents, i) {
> +		size_t offset = sg_dma_address(sg) - dma->tx_addr;
> +
> +		memcpy(dma->tx_buf + offset,
> +		       p->port.state->port.xmit_buf + offset, sg_dma_len(sg));
> +	}
> +	dma_sync_single_for_device(dmaengine_get_dma_device(dma->txchan), dma->tx_addr,
> +				   UART_XMIT_SIZE, DMA_TO_DEVICE);
> +}

4. Bounce buffer layout and DMA sync:
   - Both `serial8250_tx_dma()` and `omap_8250_tx_dma()` call
     `dmaengine_prep_slave_sg()` *before* `serial8250_tx_dma_prepare_buffer()`
     copies the bytes and calls `dma_sync_single_for_device()`. The copy and
     DMA sync should happen before preparing the descriptor.
   - Syncing the full `UART_XMIT_SIZE` (4 KB) on every transfer (even 1 byte)
     is unnecessary overhead, especially with swiotlb or non-coherent DMA.
   - If we are copying out of `xmit_fifo` into a dedicated `dma->tx_buf`
     anyway, why preserve the `kfifo` ring offsets (`sgl[2]`) instead of
     copying contiguously to `dma->tx_buf` at offset 0 and syncing only
     `dma->tx_size` bytes?

5. Please also consider splitting the Renesas RZ/N1 `serial8250_do_prepare_tx_dma()`
   `tx_size` ordering fix (`Fixes: 16b539ffb574`) and the `8250_omap`
   completion refactoring into separate preparatory patches rather than
   bundling them into a single `Fixes:` commit.

Thanks,
Raul

[PATCH 2/2] serial: 8250: retire canceled RX DMA before reusing the receive buffer

[James Hilliard]

The RX timeout path pauses a transfer, copies its data, and terminates it
asynchronously. It also clears rx_running before an old callback has
necessarily returned. The next receive can therefore reuse the same
buffer and cookie while that callback is still waiting for the port lock.
Looking up the new cookie is not sufficient: a new transfer may already
be complete or paused when the old callback obtains the lock.

Keep the receive buffer reserved after termination and drain callbacks
in a worker before allowing another descriptor to use it. The interrupt
handler can receive through PIO while the canceled transfer is retired;
data already received by DMA is delivered first. Do not perform another
termination while synchronization is pending. Shutdown drains the worker
before freeing the buffer. Both the callback and retirement worker
honor disabled receive interrupts before restarting DMA.

Check pause failures before reading the DMA buffer or handing its FIFO
back to PIO. Make the DesignWare idle path abort rather than proceed with
an unpaused receiver. Track the delivered offset so a failed termination
followed by resumed DMA cannot deliver the same prefix a second time.

Serialize the Intel MID timeout callers with the same port lock used by
the callbacks and retirement worker. Keep that lock scoped to the flush,
since serial8250_handle_irq() takes it separately for the UART interrupt.

This relies on the provider honoring pause and synchronization; it does
not turn a pause-register write into a hardware-quiescence guarantee.

Fixes: 9ee4b83e51f7 ("serial: 8250: Add support for dmaengine")
Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
 drivers/tty/serial/8250/8250.h      |  7 ++-
 drivers/tty/serial/8250/8250_dma.c  | 87 +++++++++++++++++++++++++++----------
 drivers/tty/serial/8250/8250_dw.c   |  8 +++-
 drivers/tty/serial/8250/8250_mid.c  |  6 ++-
 drivers/tty/serial/8250/8250_port.c |  3 +-
 5 files changed, 80 insertions(+), 31 deletions(-)

diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index 002e456fa384..c69c41c9df65 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -23,6 +23,7 @@ struct uart_8250_dma {
 	/* Completion processing with the port lock held. */
 	void (*tx_complete)(struct uart_8250_port *p);
 	struct work_struct tx_flush_work;
+	struct work_struct rx_flush_work;
 	struct uart_8250_port *port;
 
 	/* Filter function */
@@ -52,6 +53,7 @@ struct uart_8250_dma {
 	void			*tx_buf;
 
 	size_t			rx_size;
+	size_t			rx_pos;
 	size_t			tx_size;
 
 	unsigned char		tx_running;
@@ -60,6 +62,7 @@ struct uart_8250_dma {
 	/* Protected by the UART port lock. */
 	bool			shutting_down;
 	bool			tx_terminating;
+	bool			rx_terminating;
 };
 
 struct old_serial_port {
@@ -411,7 +414,7 @@ extern void serial8250_tx_dma_flush(struct uart_8250_port *);
 void serial8250_tx_dma_prepare_buffer(struct uart_8250_port *p,
 				      struct scatterlist *sgl, unsigned int nents);
 extern int serial8250_rx_dma(struct uart_8250_port *);
-extern void serial8250_rx_dma_flush(struct uart_8250_port *);
+int serial8250_rx_dma_flush(struct uart_8250_port *p);
 extern int serial8250_request_dma(struct uart_8250_port *);
 extern void serial8250_release_dma(struct uart_8250_port *);
 
@@ -467,7 +470,7 @@ static inline int serial8250_rx_dma(struct uart_8250_port *p)
 {
 	return -1;
 }
-static inline void serial8250_rx_dma_flush(struct uart_8250_port *p) { }
+static inline int serial8250_rx_dma_flush(struct uart_8250_port *p) { return 0; }
 static inline int serial8250_request_dma(struct uart_8250_port *p)
 {
 	return -1;
diff --git a/drivers/tty/serial/8250/8250_dma.c b/drivers/tty/serial/8250/8250_dma.c
index d4b1e2a059eb..821ccc7a3a54 100644
--- a/drivers/tty/serial/8250/8250_dma.c
+++ b/drivers/tty/serial/8250/8250_dma.c
@@ -77,31 +77,29 @@ void serial8250_tx_dma_prepare_buffer(struct uart_8250_port *p,
 }
 EXPORT_SYMBOL_GPL(serial8250_tx_dma_prepare_buffer);
 
-static void __dma_rx_complete(struct uart_8250_port *p)
+static enum dma_status serial8250_rx_dma_complete(struct uart_8250_port *p)
 {
 	struct uart_8250_dma	*dma = p->dma;
 	struct tty_port		*tty_port = &p->port.state->port;
 	struct dma_tx_state	state;
 	enum dma_status		dma_status;
-	int			count;
+	size_t			count;
 
-	/*
-	 * New DMA Rx can be started during the completion handler before it
-	 * could acquire port's lock and it might still be ongoing. Don't to
-	 * anything in such case.
-	 */
+	/* Only inspect the buffer after completion or a successful pause. */
 	dma_status = dmaengine_tx_status(dma->rxchan, dma->rx_cookie, &state);
-	if (dma_status == DMA_IN_PROGRESS)
-		return;
+	if (dma_status != DMA_COMPLETE && dma_status != DMA_PAUSED)
+		return dma_status;
 
-	count = dma->rx_size - state.residue;
+	count = dma->rx_size - state.residue - dma->rx_pos;
 
-	if (tty_insert_flip_string(tty_port, dma->rx_buf, count) != count)
+	if (tty_insert_flip_string(tty_port, dma->rx_buf + dma->rx_pos,
+				   count) != count)
 		p->port.icount.buf_overrun++;
 	p->port.icount.rx += count;
-	dma->rx_running = 0;
+	dma->rx_pos += count;
 
 	tty_flip_buffer_push(tty_port);
+	return dma_status;
 }
 
 static void dma_rx_complete(void *param)
@@ -111,21 +109,37 @@ static void dma_rx_complete(void *param)
 	unsigned long flags;
 
 	uart_port_lock_irqsave(&p->port, &flags);
-	if (dma->shutting_down)
+	if (dma->shutting_down || dma->rx_terminating)
 		goto out;
-	if (dma->rx_running)
-		__dma_rx_complete(p);
+	if (dma->rx_running && serial8250_rx_dma_complete(p) == DMA_COMPLETE)
+		dma->rx_running = 0;
 
 	/*
-	 * Cannot be combined with the previous check because __dma_rx_complete()
-	 * changes dma->rx_running.
+	 * The completed transfer no longer owns the receive buffer.
 	 */
-	if (!dma->rx_running && (serial_lsr_in(p) & UART_LSR_DR))
+	if (!dma->rx_running && (p->ier & UART_IER_RDI) &&
+	    (serial_lsr_in(p) & UART_LSR_DR))
 		p->dma->rx_dma(p);
 out:
 	uart_port_unlock_irqrestore(&p->port, flags);
 }
 
+static void serial8250_rx_dma_flush_work(struct work_struct *work)
+{
+	struct uart_8250_dma *dma = container_of(work, struct uart_8250_dma,
+					       rx_flush_work);
+	struct uart_8250_port *p = dma->port;
+
+	dmaengine_synchronize(dma->rxchan);
+
+	guard(uart_port_lock_irqsave)(&p->port);
+	dma->rx_terminating = false;
+	dma->rx_running = 0;
+	if (!dma->shutting_down && (p->ier & UART_IER_RDI) &&
+	    (serial_lsr_in(p) & UART_LSR_DR))
+		dma->rx_dma(p);
+}
+
 int serial8250_tx_dma(struct uart_8250_port *p)
 {
 	struct uart_8250_dma		*dma = p->dma;
@@ -241,6 +255,7 @@ int serial8250_rx_dma(struct uart_8250_port *p)
 		return -EBUSY;
 
 	dma->rx_running = 1;
+	dma->rx_pos = 0;
 	desc->callback = dma_rx_complete;
 	desc->callback_param = p;
 
@@ -251,16 +266,37 @@ int serial8250_rx_dma(struct uart_8250_port *p)
 	return 0;
 }
 
-void serial8250_rx_dma_flush(struct uart_8250_port *p)
+int serial8250_rx_dma_flush(struct uart_8250_port *p)
 {
 	struct uart_8250_dma *dma = p->dma;
+	enum dma_status status;
+	int ret;
 
-	if (!dma || !dma->rxchan || !dma->rx_running)
-		return;
+	lockdep_assert_held_once(&p->port.lock);
+
+	if (!dma || !dma->rxchan || !dma->rx_running || dma->rx_terminating)
+		return 0;
+
+	ret = dmaengine_pause(dma->rxchan);
+	if (ret)
+		return ret;
+
+	status = serial8250_rx_dma_complete(p);
+	if (status != DMA_PAUSED && status != DMA_COMPLETE) {
+		dmaengine_resume(dma->rxchan);
+		return -EIO;
+	}
+	dma->rx_terminating = true;
+	ret = dmaengine_terminate_async(dma->rxchan);
+	if (ret) {
+		dma->rx_terminating = false;
+		dmaengine_resume(dma->rxchan);
+		return ret;
+	}
 
-	dmaengine_pause(dma->rxchan);
-	__dma_rx_complete(p);
-	dmaengine_terminate_async(dma->rxchan);
+	/* Keep the buffer reserved until any old completion has returned. */
+	schedule_work(&dma->rx_flush_work);
+	return 0;
 }
 EXPORT_SYMBOL_GPL(serial8250_rx_dma_flush);
 
@@ -363,7 +399,9 @@ int serial8250_request_dma(struct uart_8250_port *p)
 
 	dma->port = p;
 	INIT_WORK(&dma->tx_flush_work, serial8250_tx_dma_flush_work);
+	INIT_WORK(&dma->rx_flush_work, serial8250_rx_dma_flush_work);
 	dma->tx_terminating = false;
+	dma->rx_terminating = false;
 
 	scoped_guard(uart_port_lock_irqsave, &p->port)
 		dma->shutting_down = false;
@@ -391,6 +429,7 @@ void serial8250_release_dma(struct uart_8250_port *p)
 		return;
 
 	cancel_work_sync(&dma->tx_flush_work);
+	cancel_work_sync(&dma->rx_flush_work);
 
 	/* Release RX resources */
 	dmaengine_terminate_sync(dma->rxchan);
diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index eca02e8bd6d1..d8998b6f1d81 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -134,6 +134,7 @@ static int dw8250_idle_enter(struct uart_port *p)
 	unsigned int usr_reg = d->pdata ? d->pdata->usr_reg : DW_UART_USR;
 	struct uart_8250_port *up = up_to_u8250p(p);
 	int retries;
+	int ret;
 	u32 lsr;
 
 	lockdep_assert_held_once(&p->lock);
@@ -147,7 +148,12 @@ static int dw8250_idle_enter(struct uart_port *p)
 	serial_port_out(p, UART_IER, 0);
 
 	if (up->dma) {
-		serial8250_rx_dma_flush(up);
+		ret = serial8250_rx_dma_flush(up);
+		if (ret) {
+			serial_port_out(p, UART_IER, up->ier);
+			d->in_idle = 0;
+			return ret;
+		}
 		if (serial8250_tx_dma_running(up))
 			serial8250_tx_dma_pause(up);
 	}
diff --git a/drivers/tty/serial/8250/8250_mid.c b/drivers/tty/serial/8250/8250_mid.c
index 44b43163df51..83f928b1f643 100644
--- a/drivers/tty/serial/8250/8250_mid.c
+++ b/drivers/tty/serial/8250/8250_mid.c
@@ -94,7 +94,8 @@ static int tng_handle_irq(struct uart_port *p)
 	/* Rx DMA */
 	err = hsu_dma_get_status(chip, mid->dma_index * 2 + 1, &status);
 	if (err > 0) {
-		serial8250_rx_dma_flush(up);
+		scoped_guard(uart_port_lock_irqsave, p)
+			serial8250_rx_dma_flush(up);
 		ret |= 1;
 	} else if (err == 0)
 		ret |= hsu_dma_do_irq(chip, mid->dma_index * 2 + 1, status);
@@ -148,7 +149,8 @@ static int dnv_handle_irq(struct uart_port *p)
 	if (fisr & BIT(2)) {
 		err = hsu_dma_get_status(&mid->dma_chip, 1, &status);
 		if (err > 0) {
-			serial8250_rx_dma_flush(up);
+			scoped_guard(uart_port_lock_irqsave, p)
+				serial8250_rx_dma_flush(up);
 			ret |= 1;
 		} else if (err == 0)
 			ret |= hsu_dma_do_irq(&mid->dma_chip, 1, status);
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index d476ebc37647..b9837a3c4681 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1815,8 +1815,7 @@ static bool handle_rx_dma(struct uart_8250_port *up, unsigned int iir)
 		fallthrough;
 	case UART_IIR_RLSI:
 	case UART_IIR_RX_TIMEOUT:
-		serial8250_rx_dma_flush(up);
-		return true;
+		return !serial8250_rx_dma_flush(up);
 	}
 	return up->dma->rx_dma(up);
 }

-- 
2.53.0

Re: [PATCH 2/2] serial: 8250: retire canceled RX DMA before reusing the receive buffer

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] serial: 8250: missing shutting_down check in serial8250_rx_dma_flush()
- [High] serial: 8250: rx dma flush fails on paused DMA_IN_PROGRESS and DMA_ERROR
--

commit d4e9133dca0ba3f456e79c0c4b6fad29f9996583
Author: James Hilliard <james.hilliard1@gmail.com>

serial: 8250: retire canceled RX DMA before reusing the receive buffer

This commit reserves the RX DMA buffer across asynchronous termination and
synchronizes in-flight completion callbacks in a workqueue worker before
allowing a new RX DMA transfer to reuse the buffer. It also checks pause and
status results during RX DMA flushes and serializes Intel MID timeout callers
with the UART port lock.

> diff --git a/drivers/tty/serial/8250/8250_dma.c b/drivers/tty/serial/8250/8250_dma.c
> index d4b1e2a059eb1..821ccc7a3a54d 100644
> --- a/drivers/tty/serial/8250/8250_dma.c
> +++ b/drivers/tty/serial/8250/8250_dma.c
> @@ -77,31 +77,29 @@ void serial8250_tx_dma_prepare_buffer(struct uart_8250_port *p,
>  }
>  EXPORT_SYMBOL_GPL(serial8250_tx_dma_prepare_buffer);
>  
> -static void __dma_rx_complete(struct uart_8250_port *p)
> +static enum dma_status serial8250_rx_dma_complete(struct uart_8250_port *p)
>  {
>  	struct uart_8250_dma	*dma = p->dma;
>  	struct tty_port		*tty_port = &p->port.state->port;
>  	struct dma_tx_state	state;
>  	enum dma_status		dma_status;
> -	int			count;
> +	size_t			count;
>  
> -	/*
> -	 * New DMA Rx can be started during the completion handler before it
> -	 * could acquire port's lock and it might still be ongoing. Don't to
> -	 * anything in such case.
> -	 */
> +	/* Only inspect the buffer after completion or a successful pause. */
>  	dma_status = dmaengine_tx_status(dma->rxchan, dma->rx_cookie, &state);
> -	if (dma_status == DMA_IN_PROGRESS)
> -		return;
> +	if (dma_status != DMA_COMPLETE && dma_status != DMA_PAUSED)
> +		return dma_status;

[ ... ]

> +static void serial8250_rx_dma_flush_work(struct work_struct *work)
> +{
> +	struct uart_8250_dma *dma = container_of(work, struct uart_8250_dma,
> +					       rx_flush_work);
> +	struct uart_8250_port *p = dma->port;
> +
> +	dmaengine_synchronize(dma->rxchan);
> +
> +	guard(uart_port_lock_irqsave)(&p->port);
> +	dma->rx_terminating = false;
> +	dma->rx_running = 0;
> +	if (!dma->shutting_down && (p->ier & UART_IER_RDI) &&
> +	    (serial_lsr_in(p) & UART_LSR_DR))
> +		dma->rx_dma(p);
> +}

[ ... ]

> -void serial8250_rx_dma_flush(struct uart_8250_port *p)
> +int serial8250_rx_dma_flush(struct uart_8250_port *p)
>  {
>  	struct uart_8250_dma *dma = p->dma;
> +	enum dma_status status;
> +	int ret;
>  
> -	if (!dma || !dma->rxchan || !dma->rx_running)
> -		return;
> +	lockdep_assert_held_once(&p->port.lock);
> +
> +	if (!dma || !dma->rxchan || !dma->rx_running || dma->rx_terminating)
> +		return 0;

[Severity: High]
Should serial8250_rx_dma_flush() also check dma->shutting_down before
proceeding, similar to serial8250_tx_dma_flush(), dma_rx_complete(), and
serial8250_rx_dma()?

During shutdown, serial8250_do_shutdown() in
drivers/tty/serial/8250/8250_port.c sets up->dma->shutting_down = true and
clears UART_IER under port->lock, calls synchronize_irq(port->irq), and then
calls serial8250_release_dma(up) while port->irq is still registered.

In drivers/tty/serial/8250/8250_dma.c, serial8250_release_dma() cancels
rx_flush_work before terminating dma->rxchan and clearing dma->rx_running:

drivers/tty/serial/8250/8250_dma.c:serial8250_release_dma() {
	...
	cancel_work_sync(&dma->tx_flush_work);
	cancel_work_sync(&dma->rx_flush_work);

	/* Release RX resources */
	dmaengine_terminate_sync(dma->rxchan);
	dma->rx_running = 0;
	dma_free_coherent(dmaengine_get_dma_device(dma->rxchan), dma->rx_size,
			  dma->rx_buf, dma->rx_addr);
	dma_release_channel(dma->rxchan);
	dma->rxchan = NULL;
	...
}

On Intel MID platforms, tng_handle_irq() and dnv_handle_irq() in
drivers/tty/serial/8250/8250_mid.c handle HSU DMA channel timeout interrupts
on shared port->irq, which are not masked by clearing UART_IER. While the RX
DMA transfer is still active prior to dmaengine_terminate_sync(dma->rxchan)
(or if a concurrent IRQ handler read UART_IIR just before
serial8250_do_shutdown() took port->lock), the handler can acquire port->lock
and call serial8250_rx_dma_flush(up) after cancel_work_sync(&dma->rx_flush_work)
has already returned.

Without a dma->shutting_down check, serial8250_rx_dma_flush() will push flip
buffer data after shutdown has started and call
schedule_work(&dma->rx_flush_work). When serial8250_rx_dma_flush_work()
subsequently runs, serial8250_release_dma() may have already released
dma->rxchan and set dma->rxchan = NULL, causing
dmaengine_synchronize(dma->rxchan) to dereference dma->rxchan == NULL or a
freed channel.

> +
> +	ret = dmaengine_pause(dma->rxchan);
> +	if (ret)
> +		return ret;
> +
> +	status = serial8250_rx_dma_complete(p);
> +	if (status != DMA_PAUSED && status != DMA_COMPLETE) {
> +		dmaengine_resume(dma->rxchan);
> +		return -EIO;
> +	}

[Severity: High]
Could rejecting statuses other than DMA_PAUSED and DMA_COMPLETE here and in
serial8250_rx_dma_complete() wedge RX DMA or cause an unhandled interrupt
storm on RX timeout?

First, several dmaengine providers that pass the capability checks in
serial8250_request_dma() (such as sun6i-dma in drivers/dma/sun6i-dma.c and
dw-axi-dmac in drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c, used with
8250_dw) pause the hardware channel in .device_pause and return 0, but their
.device_tx_status callbacks still return dma_cookie_status() (DMA_IN_PROGRESS)
rather than DMA_PAUSED. With this check, serial8250_rx_dma_flush() resumes
the channel and fails with -EIO on every RX timeout flush and in
dw8250_idle_enter().

Second, if an RX DMA transfer encounters a channel error (DMA_ERROR, for
example in drivers/dma/hsu/hsu.c or drivers/dma/idma64.c where .device_pause
only sets DMA_PAUSED when the descriptor is DMA_IN_PROGRESS), neither
dma_rx_complete() nor serial8250_rx_dma_flush() terminates the transfer or
clears dma->rx_running, leaving RX DMA permanently stuck.

Third, when serial8250_rx_dma_flush() returns an error on UART_IIR_RX_TIMEOUT,
UART_IIR_RDI, or UART_IIR_RLSI, handle_rx_dma() in
drivers/tty/serial/8250/8250_port.c now returns false, causing
serial8250_handle_irq_locked() to skip serial8250_rx_chars():

drivers/tty/serial/8250/8250_port.c:handle_rx_dma() {
	...
	case UART_IIR_RLSI:
	case UART_IIR_RX_TIMEOUT:
		return !serial8250_rx_dma_flush(up);
}

drivers/tty/serial/8250/8250_port.c:serial8250_handle_irq_locked() {
	...
	if (!up->dma || handle_rx_dma(up, iir))
		status = serial8250_rx_chars(up, status);
}

Because UART_IIR_RX_TIMEOUT is only cleared by reading UART_RX and the FIFO
level on a timeout is below the DMA burst watermark (and in
dw8250_handle_irq() in drivers/tty/serial/8250/8250_dw.c with
DW_UART_QUIRK_IS_DMA_FC, RZN1_UART_RDMACR has already been cleared to 0
before serial8250_handle_irq_locked() is called), returning from the ISR
without reading UART_RX via PIO leaves the level-triggered UART interrupt
asserted.

[ ... ]

>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021-submit-serial-8250-dma-cancel-v1-v1-0-efe2b89f2303@gmail.com?part=2

Re: [PATCH 2/2] serial: 8250: retire canceled RX DMA before reusing the receive buffer

[Raul E Rangel]

Hi James,

Beyond the two issues Sashiko already flagged (`dma->shutting_down` not
checked in `serial8250_rx_dma_flush()`, and `DMA_IN_PROGRESS` rejected after
`dmaengine_pause()` leading to an unhandled `UART_IIR_RX_TIMEOUT` interrupt
storm when `!serial8250_rx_dma_flush(up)` returns `false` in `handle_rx_dma()`),
there are a few additional issues here:

> -static void __dma_rx_complete(struct uart_8250_port *p)
> +static enum dma_status serial8250_rx_dma_complete(struct uart_8250_port *p)
>  {
...
> -	ret = tty_insert_flip_string(tty_port, dma->rx_buf, count);
> -	p->port.icount.rx += ret;
> -	p->port.icount.buf_overrun += count - ret;
> +	count = dma->rx_size - state.residue - dma->rx_pos;
> +
> +	if (tty_insert_flip_string(tty_port, dma->rx_buf + dma->rx_pos,
> +				   count) != count)
> +		p->port.icount.buf_overrun++;
> +	p->port.icount.rx += count;
> +	dma->rx_pos += count;

1. Accidental revert of prerequisite patch [2]:
   This chunk reverts your prerequisite patch ("serial: 8250: account for
   partial receive DMA delivery to TTY") in `8250_dma.c`, restoring the old
   `icount.rx += count` and `icount.buf_overrun++` accounting.

> +static void serial8250_rx_dma_flush_work(struct work_struct *work)
> +{
> +	struct uart_8250_dma *dma = container_of(work, struct uart_8250_dma,
> +						 rx_flush_work);
> +	struct uart_8250_port *p = dma->port;
> +
> +	dmaengine_synchronize(dma->rxchan);
> +
> +	guard(uart_port_lock_irqsave)(&p->port);
> +	dma->rx_terminating = false;
> +	dma->rx_running = 0;
> +	if (!dma->shutting_down && (p->ier & UART_IER_RDI) &&
> +	    (serial_lsr_in(p) & UART_LSR_DR))
> +		dma->rx_dma(p);
> +}

2. Spurious RX DMA restart during `dw8250_idle_enter()`:
   In `drivers/tty/serial/8250/8250_dw.c`, `dw8250_idle_enter()` sets
   `d->in_idle = 1` and writes `0` to hardware `UART_IER`, but intentionally
   leaves `UART_IER_RDI` set in `up->ier` so `dw8250_idle_exit()` can restore
   it. `dw8250_idle_enter()` then calls `serial8250_rx_dma_flush(up)`, which
   schedules `rx_flush_work`. If `rx_flush_work` runs while `d->in_idle == 1`
   (while the port is in loopback mode reconfiguring LCR/divisors),
   `p->ier & UART_IER_RDI` is still true and `rx_flush_work` will restart
   `dma->rx_dma(p)` in the middle of the idle sequence.

3. Missing runtime PM reference and `serial_lsr_in()` side effects in
   `serial8250_rx_dma_flush_work()`:
   - `serial8250_rx_dma_flush_work()` runs on `system_wq` and reads
     `serial_lsr_in(p)` / starts `dma->rx_dma(p)` without acquiring
     `serial8250_rpm_get(p)` / `serial8250_rpm_put(p)`. After the RX timeout
     ISR exits and drops its runtime PM reference, the port can autosuspend
     before `rx_flush_work` executes.
   - Reading `serial_lsr_in(p)` outside IRQ context clears any pending
     `UART_IIR_RLSI` in hardware and starts RX DMA before any saved LSR error
     bits (`OE`/`PE`/`FE`/`BI`) are processed with the corresponding byte in
     the FIFO.
   - If `UART_IIR_RDI` is enabled in hardware, why does `rx_flush_work` need
     to poll `UART_LSR_DR` and restart `rx_dma(p)` at all rather than letting
     the UART interrupt handler start RX DMA (or drain via PIO) when data is
     present?

4. `dmaengine_resume()` on error in `serial8250_rx_dma_flush()`:
   `serial8250_request_dma()` only checks `caps.cmd_pause` and
   `caps.cmd_terminate`, not `caps.cmd_resume` (e.g. `pl330` and
   `mtk-uart-apdma` implement pause to read residue before termination, but
   do not implement `.device_resume`). Also, on Renesas RZ/N1
   (`DW_UART_QUIRK_IS_DMA_FC`), `dw8250_handle_irq()` has already cleared
   `RZN1_UART_RDMACR = 0` before calling `serial8250_rx_dma_flush()`, so the
   error-recovery path leaves RX DMA disabled.

Thanks,
Raul