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

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

[James Hilliard]

Reserve canceled transmit and receive storage until DMA and its completion
callbacks have retired. This revision uses serial-core write backpressure
to protect a flushed TX FIFO, following the discussion of the private TX
buffer in v1.

The series first separates OMAP's locked completion processing, then adds
the serial-core reservation helpers and applies them to 8250 TX. Flushing
still empties the logical queue immediately. While the old transfer owns
its storage, write_room() reports zero and writers wait for retirement.
Normal TX continues to DMA directly from the FIFO. Failed termination
retains the reservation until normal completion, without advancing the
reset FIFO.

RX delivers paused data before the PIO fallback and retires callbacks in
process context. Normal retirement leaves further receive handling to the
UART interrupt. Failed pause, status or termination instead masks receive
interrupts until DMA has stopped, then keeps RX on PIO until reopen.
Shutdown excludes new retirement work before releasing the channels.

Based on tty-next with the posted shutdown guard [0], channel
configuration error handling [1], RX TTY accounting fix [2], RZ/N1
transfer-length fix [3], and transmit-PM shutdown fix [4].

[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
[3] https://lore.kernel.org/r/20261022-submit-serial-8250-dma-tx-length-v1-v1-1-18cecc89a099@gmail.com
[4] https://lore.kernel.org/r/20261022-submit-serial-8250-tx-pm-shutdown-v1-v1-1-0e894afa3bb8@gmail.com

Assisted-by: Codex:gpt-6-astra
Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
Changes in v2:
- Add serial-core helpers to reserve flushed transmit storage until DMA
  retirement. Apply write backpressure during that interval and keep normal
  TX using the FIFO directly, replacing the private transmit buffer in v1.
- Document the flush callback's reservation, writer backpressure and
  retirement requirements, including release before shutdown returns.
- Split the OMAP completion refactor into a preparatory patch and move
  the RZ/N1 preparation-order and shutdown PM fixes to independent
  submissions, keeping four patches in this series.
- Take runtime PM references around worker UART accesses, and leave normal
  RX retirement to release ownership without reading LSR or restarting DMA.
- Synchronize the queued TX ranges before preparing the descriptor and
  return the mapping to CPU ownership before releasing the reservation.
- Keep the TX transfer length separate from FIFO cancellation, so a failed
  termination can account completion without advancing the reset FIFO.
- Exclude late RX flush work during shutdown, including Intel MID DMA
  timeout interrupts which are independent of UART_IER.
- Accept DMA_IN_PROGRESS after successful pause, and recover from failed
  pause, termination or DMA_ERROR before handing the receiver to PIO.
- Preserve stop_rx() and shutdown across recovery, and retain the
  DesignWare empty-FIFO timeout workaround when receiving through PIO.
- Apply the requested guard spacing and container_of() formatting.
- Link to v1: https://patch.msgid.link/20261021-submit-serial-8250-dma-cancel-v1-v1-0-efe2b89f2303@gmail.com

---
James Hilliard (4):
      serial: 8250_omap: separate locked TX DMA completion processing
      serial: core: allow drivers to reserve flushed transmit storage
      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      |  27 +++-
 drivers/tty/serial/8250/8250_dma.c  | 272 ++++++++++++++++++++++++++++--------
 drivers/tty/serial/8250/8250_dw.c   |  15 +-
 drivers/tty/serial/8250/8250_mid.c  |   6 +-
 drivers/tty/serial/8250/8250_omap.c |  37 +++--
 drivers/tty/serial/8250/8250_port.c |   5 +-
 drivers/tty/serial/serial_core.c    |  47 ++++++-
 include/linux/serial_core.h         |  16 ++-
 8 files changed, 335 insertions(+), 90 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
prerequisite-patch-id: 7dbb404baf1f861982cf6a4b755d39cfec548223
prerequisite-patch-id: 40881d81a27b3a1aa45203d171214faaf2197768

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

[PATCH v2 1/4] serial: 8250_omap: separate locked TX DMA completion processing

[James Hilliard]

Separate the port-locked completion processing from the DMA callback
wrapper. Keep the shutdown check and locking in the callback, and retain
the existing FIFO accounting, delayed register restore and THRI handling
in the helper.

Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
 drivers/tty/serial/8250/8250_omap.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
index 4805b644ccb1..46f1fff361dd 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -1080,18 +1080,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);
 
@@ -1123,6 +1118,17 @@ 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)
+		omap_8250_tx_dma_complete(p);
+}
+
 static int omap_8250_tx_dma(struct uart_8250_port *p)
 {
 	struct uart_8250_dma		*dma = p->dma;

-- 
2.53.0

[PATCH v2 2/4] serial: core: allow drivers to reserve flushed transmit storage

[James Hilliard]

uart_flush_buffer() resets the transmit FIFO before calling the driver's
flush callback. An asynchronous DMA transfer can still access that storage
after the callback returns, while a new write can immediately reuse it.

Add uart_xmit_block() and uart_xmit_unblock() so a driver can reserve the
flushed storage until its transfer and completion callbacks have retired.
Serialize the reservation with FIFO producers using the port lock.

While the storage is reserved, write_room() reports no space and write()
and put_char() accept no data. Releasing the reservation wakes writers
through the existing TTY wakeup path. This preserves immediate logical
flushing while applying write backpressure until the storage is reusable.

Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
 drivers/tty/serial/serial_core.c | 47 +++++++++++++++++++++++++++++++++++++---
 include/linux/serial_core.h      | 16 ++++++++++++--
 2 files changed, 58 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index c0deec0e6af4..f82c812f842f 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -123,6 +123,46 @@ void uart_write_wakeup(struct uart_port *port)
 }
 EXPORT_SYMBOL(uart_write_wakeup);
 
+/**
+ * uart_xmit_block - reserve flushed transmit storage
+ * @port: port whose transmit storage is still in use
+ *
+ * A driver may call this from ->flush_buffer() if DMA still owns the old
+ * transmit data. The logical FIFO has already been emptied, but writers
+ * must not overwrite its storage until uart_xmit_unblock() is called.
+ * write_room() reports zero and write()/put_char() accept no data meanwhile.
+ *
+ * The driver must unblock after retiring the transfer, including on shutdown.
+ * Repeated calls are idempotent, not nested reservations.
+ *
+ * Context: @port->lock held.
+ */
+void uart_xmit_block(struct uart_port *port)
+{
+	lockdep_assert_held_once(&port->lock);
+	port->state->xmit_blocked = true;
+}
+EXPORT_SYMBOL_GPL(uart_xmit_block);
+
+/**
+ * uart_xmit_unblock - release reserved transmit storage and wake writers
+ * @port: port whose storage is available again
+ *
+ * The old transfer must no longer access the FIFO storage, and any required
+ * DMA synchronization must have completed before this call.
+ *
+ * Context: @port->lock held.
+ */
+void uart_xmit_unblock(struct uart_port *port)
+{
+	lockdep_assert_held_once(&port->lock);
+	if (!port->state->xmit_blocked)
+		return;
+	port->state->xmit_blocked = false;
+	uart_write_wakeup(port);
+}
+EXPORT_SYMBOL_GPL(uart_xmit_unblock);
+
 static void uart_stop(struct tty_struct *tty)
 {
 	struct uart_state *state = tty->driver_data;
@@ -260,6 +300,7 @@ static int uart_alloc_xmit_buf(struct tty_port *port)
 	uport = uart_port_ref_lock(state, &flags);
 	if (!state->port.xmit_buf) {
 		state->port.xmit_buf = buf;
+		state->xmit_blocked = false;
 		kfifo_init(&state->port.xmit_fifo, state->port.xmit_buf,
 				PAGE_SIZE);
 		uart_port_unlock_deref(uport, flags);
@@ -593,7 +634,7 @@ static int uart_put_char(struct tty_struct *tty, u8 c)
 		return 0;
 	}
 
-	if (port)
+	if (port && !state->xmit_blocked)
 		ret = kfifo_put(&state->port.xmit_fifo, c);
 	uart_port_unlock_deref(port, flags);
 	return ret;
@@ -624,7 +665,7 @@ static ssize_t uart_write(struct tty_struct *tty, const u8 *buf, size_t count)
 		return 0;
 	}
 
-	if (port)
+	if (port && !state->xmit_blocked)
 		ret = kfifo_in(&state->port.xmit_fifo, buf, count);
 
 	__uart_start(state);
@@ -640,7 +681,7 @@ static unsigned int uart_write_room(struct tty_struct *tty)
 	unsigned int ret;
 
 	port = uart_port_ref_lock(state, &flags);
-	if (!state->port.xmit_buf)
+	if (!state->port.xmit_buf || state->xmit_blocked)
 		ret = 0;
 	else
 		ret = kfifo_avail(&state->port.xmit_fifo);
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 4eeb8253752a..92164bd598ba 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -208,8 +208,16 @@ struct gpio_desc;
  *	Flush any write buffers, reset any DMA state and stop any ongoing DMA
  *	transfers.
  *
- *	This will be called whenever the @port->state->xmit circular buffer is
- *	cleared.
+ *	This is called after the transmit FIFO has been cleared, with @port->lock
+ *	still held. A driver whose asynchronous transfer can still access the
+ *	flushed storage must call uart_xmit_block() before returning. This keeps
+ *	write_room() at zero and prevents write() and put_char() from reusing the
+ *	storage until the driver calls uart_xmit_unblock().
+ *
+ *	Release the reservation only after the transfer and its callbacks have
+ *	retired and any required DMA synchronization has completed. Shutdown must
+ *	also retire the transfer and release the reservation before returning,
+ *	since serial core then frees the transmit buffer.
  *
  *	Locking: @port->lock taken.
  *	Interrupts: locally disabled.
@@ -834,6 +842,8 @@ enum uart_pm_state {
  */
 struct uart_state {
 	struct tty_port		port;
+	/* Flushed TX storage is reserved; protected by the UART port lock. */
+	bool			xmit_blocked;
 
 	enum uart_pm_state	pm_state;
 
@@ -909,6 +919,8 @@ struct uart_driver {
 };
 
 void uart_write_wakeup(struct uart_port *port);
+void uart_xmit_block(struct uart_port *port);
+void uart_xmit_unblock(struct uart_port *port);
 
 /**
  * enum UART_TX_FLAGS -- flags for uart_port_tx_flags()

-- 
2.53.0

[PATCH v2 3/4] serial: 8250: drain canceled TX DMA before reusing its buffer

[James Hilliard]

uart_flush_buffer() resets the transmit kfifo before calling the driver.
serial8250_tx_dma_flush() terminates DMA asynchronously and clears
tx_running, allowing a new write to replace tx_size while the old callback
is still waiting for the port lock. That callback can then consume bytes
from the new transmission. Reusing the reset kfifo can also overwrite the
source of a transfer that has not stopped yet.

Reserve the flushed FIFO storage through serial core before returning
from the flush callback. Keep tx_running set until a worker synchronizes
the terminated channel, suppressing old callbacks and new submissions
during that interval. Return the mapping to CPU ownership before releasing
the reservation and waking writers. Normal transfers continue to use the
FIFO directly.

Reuse the locked completion processing after retirement, including OMAP
delayed register restoration and transmitter-empty handling. Take a runtime
PM reference around worker UART accesses and drain the worker before
releasing the channels. Shutdown releases the reservation after stopping
DMA and unmapping its source.

Track a flushed FIFO separately from the transfer length. If termination
fails, retain the reservation until normal completion and account the
completed bytes without advancing the reset FIFO. After successful
cancellation, process a zero-length completion once callbacks have drained.

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  | 123 ++++++++++++++++++++++++++----------
 drivers/tty/serial/8250/8250_omap.c |  21 ++++--
 3 files changed, 115 insertions(+), 42 deletions(-)

diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index 974a47f24084..ec837c380695 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;
@@ -53,6 +58,8 @@ struct uart_8250_dma {
 	unsigned char		rx_running;
 	/* Protected by the UART port lock. */
 	bool			shutting_down;
+	bool			tx_terminating;
+	bool			tx_flushed;
 };
 
 struct old_serial_port {
@@ -402,6 +409,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_sync_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 *);
@@ -434,7 +443,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);
@@ -444,7 +453,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 57fc7915adac..bb30d3588f16 100644
--- a/drivers/tty/serial/8250/8250_dma.c
+++ b/drivers/tty/serial/8250/8250_dma.c
@@ -11,24 +11,23 @@
 
 #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);
 
 	dma->tx_running = 0;
 
-	uart_xmit_advance(&p->port, dma->tx_size);
+	if (dma->tx_flushed) {
+		p->port.icount.tx += dma->tx_size;
+		uart_xmit_unblock(&p->port);
+	} else {
+		uart_xmit_advance(&p->port, dma->tx_size);
+	}
 
 	if (kfifo_len(&tport->xmit_fifo) < WAKEUP_CHARS)
 		uart_write_wakeup(&p->port);
@@ -36,11 +35,55 @@ 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(serial8250_rpm)(p);
+
+	guard(uart_port_lock_irqsave)(&p->port);
+
+	dma->tx_terminating = false;
+	dma->tx_size = 0;
+	if (!dma->shutting_down)
+		dma->tx_complete(p);
 }
 
+void serial8250_tx_dma_sync_buffer(struct uart_8250_port *p,
+				   struct scatterlist *sgl, unsigned int nents)
+{
+	struct uart_8250_dma *dma = p->dma;
+	struct device *dev = dmaengine_get_dma_device(dma->txchan);
+	struct scatterlist *sg;
+	unsigned int i;
+
+	/* DMA reads the FIFO directly; its occupied ranges remain reserved. */
+	for_each_sg(sgl, sg, nents, i) {
+		size_t offset = sg_dma_address(sg) - dma->tx_addr;
+
+		dma_sync_single_range_for_device(dev, dma->tx_addr, offset,
+						 sg_dma_len(sg), DMA_TO_DEVICE);
+	}
+}
+EXPORT_SYMBOL_GPL(serial8250_tx_dma_sync_buffer);
+
 static void __dma_rx_complete(struct uart_8250_port *p)
 {
 	struct uart_8250_dma	*dma = p->dma;
@@ -103,6 +146,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) {
@@ -131,24 +176,25 @@ int serial8250_tx_dma(struct uart_8250_port *p)
 		dma->tx_size += sg_dma_len(sg);
 
 	serial8250_do_prepare_tx_dma(p);
+	serial8250_tx_dma_sync_buffer(p, sgl, ret);
 
 	desc = dmaengine_prep_slave_sg(dma->txchan, sgl, ret,
 				       DMA_MEM_TO_DEV,
 				       DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
 	if (!desc) {
+		dma_sync_single_for_cpu(dmaengine_get_dma_device(dma->txchan), dma->tx_addr,
+					UART_XMIT_SIZE, DMA_TO_DEVICE);
 		ret = -EBUSY;
 		goto err;
 	}
 
 	dma->tx_running = 1;
+	dma->tx_flushed = false;
+	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);
-
 	dma_async_issue_pending(dma->txchan);
 	serial8250_clear_THRI(p);
 	dma->tx_err = 0;
@@ -162,32 +208,31 @@ 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;
 
 	/*
 	 * kfifo_reset() has been called by the serial core, avoid
-	 * advancing and underflowing in __dma_tx_complete().
+	 * advancing the reset FIFO when the old transfer completes.
 	 */
-	dma->tx_size = 0;
+	dma->tx_flushed = true;
+	/* The core reset the FIFO indices, but DMA may still read its storage. */
+	uart_xmit_block(&p->port);
 
 	/*
 	 * 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)
@@ -312,21 +357,29 @@ int serial8250_request_dma(struct uart_8250_port *p)
 		goto err;
 	}
 
-	/* TX buffer */
+	/* Normal TX uses serial core's FIFO; flush reserves it until retirement. */
 	dma->tx_addr = dma_map_single(tx_dev, p->port.state->port.xmit_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_rx;
 	}
+	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;
+	dma->tx_flushed = 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_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 +395,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 +409,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);
+	scoped_guard(uart_port_lock_irqsave, &p->port)
+		uart_xmit_unblock(&p->port);
 	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 46f1fff361dd..b9b3c18fc9f4 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -1092,7 +1092,12 @@ static void omap_8250_tx_dma_complete(struct uart_8250_port *p)
 
 	dma->tx_running = 0;
 
-	uart_xmit_advance(&p->port, dma->tx_size);
+	if (dma->tx_flushed) {
+		p->port.icount.tx += dma->tx_size;
+		uart_xmit_unblock(&p->port);
+	} else {
+		uart_xmit_advance(&p->port, dma->tx_size);
+	}
 
 	if (priv->delayed_restore) {
 		priv->delayed_restore = 0;
@@ -1125,7 +1130,7 @@ static void omap_8250_dma_tx_complete(void *param)
 
 	guard(uart_port_lock_irqsave)(&p->port);
 
-	if (!dma->shutting_down)
+	if (!dma->shutting_down && !dma->tx_terminating)
 		omap_8250_tx_dma_complete(p);
 }
 
@@ -1139,7 +1144,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)) {
 
@@ -1198,24 +1203,26 @@ static int omap_8250_tx_dma(struct uart_8250_port *p)
 		goto err;
 	}
 
+	serial8250_tx_dma_sync_buffer(p, &sg, 1);
+
 	desc = dmaengine_prep_slave_sg(dma->txchan, &sg, 1, DMA_MEM_TO_DEV,
 			DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
 	if (!desc) {
+		dma_sync_single_for_cpu(dmaengine_get_dma_device(dma->txchan), dma->tx_addr,
+					UART_XMIT_SIZE, DMA_TO_DEVICE);
 		ret = -EBUSY;
 		goto err;
 	}
 
 	dma->tx_size = sg_dma_len(&sg);
 	dma->tx_running = 1;
+	dma->tx_flushed = false;
+	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);
-
 	dma_async_issue_pending(dma->txchan);
 	if (dma->tx_err)
 		dma->tx_err = 0;

-- 
2.53.0

[PATCH v2 4/4] 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. Clearing rx_running at that point lets another receive
reuse the buffer and cookie while the old callback is still waiting for
the port lock. Inspecting the new cookie cannot distinguish that callback
from the completion of a new transfer.

Reserve the receive buffer until a worker has drained the canceled
callbacks. Deliver the paused data before allowing the interrupt handler
to drain the FIFO through PIO. Normal retirement only releases ownership;
leave subsequent receive handling to the UART interrupt instead of reading
LSR and restarting DMA from the worker.

Reject new flush work during shutdown, including late Intel MID DMA
timeouts. Serialize those timeout callers with the port lock. Stop and
drain retirement work before freeing the channels and buffer.

Check pause failures before accessing the buffer. A successful pause is
the quiescence guarantee, even when a provider still reports IN_PROGRESS.
On pause, status or termination failure, mask receive interrupts and let
the worker stop DMA before handing the FIFO to PIO. Preserve stop_rx() and
shutdown while restoring deferred interrupts, and hold a runtime PM
reference around worker register accesses. Keep RX on PIO until reopen.

Abort the DesignWare idle sequence when RX cannot be stopped. Preserve its
empty-FIFO timeout workaround during PIO recovery. If the provider still
cannot terminate the channel in process context, retain its buffer and
leave receive interrupts masked.

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

diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index ec837c380695..e3b5a605bb55 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 */
@@ -60,6 +61,10 @@ struct uart_8250_dma {
 	bool			shutting_down;
 	bool			tx_terminating;
 	bool			tx_flushed;
+	bool			rx_terminating;
+	bool			rx_err;
+	/* Receive interrupt enables deferred during RX DMA recovery. */
+	u8			rx_ier;
 };
 
 struct old_serial_port {
@@ -412,7 +417,7 @@ extern void serial8250_tx_dma_flush(struct uart_8250_port *);
 void serial8250_tx_dma_sync_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 *);
 
@@ -468,7 +473,12 @@ 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 bb30d3588f16..4cadb3eef941 100644
--- a/drivers/tty/serial/8250/8250_dma.c
+++ b/drivers/tty/serial/8250/8250_dma.c
@@ -84,55 +84,108 @@ void serial8250_tx_dma_sync_buffer(struct uart_8250_port *p,
 }
 EXPORT_SYMBOL_GPL(serial8250_tx_dma_sync_buffer);
 
-static void __dma_rx_complete(struct uart_8250_port *p)
+static void serial8250_rx_dma_copy(struct uart_8250_port *p,
+				   const struct dma_tx_state *state)
 {
 	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;
-
-	/*
-	 * 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.
-	 */
-	dma_status = dmaengine_tx_status(dma->rxchan, dma->rx_cookie, &state);
-	if (dma_status == DMA_IN_PROGRESS)
-		return;
-
-	count = dma->rx_size - state.residue;
+	size_t			count = dma->rx_size - state->residue;
 
 	if (tty_insert_flip_string(tty_port, dma->rx_buf, count) != count)
 		p->port.icount.buf_overrun++;
 	p->port.icount.rx += count;
-	dma->rx_running = 0;
 
 	tty_flip_buffer_push(tty_port);
 }
 
+static void serial8250_rx_dma_mask(struct uart_8250_port *p)
+{
+	u8 rx_ier = UART_IER_RDI | UART_IER_RLSI | UART_IER_RTOIE;
+
+	p->dma->rx_ier |= p->ier & rx_ier;
+	p->ier &= ~rx_ier;
+	serial_port_out(&p->port, UART_IER, p->ier);
+}
+
+static void serial8250_rx_dma_recover(struct uart_8250_port *p)
+{
+	struct uart_8250_dma *dma = p->dma;
+
+	/* Keep PIO and new descriptors out until the worker stops the channel. */
+	dma->rx_err = true;
+	dma->rx_terminating = true;
+	serial8250_rx_dma_mask(p);
+	schedule_work(&dma->rx_flush_work);
+}
+
 static void dma_rx_complete(void *param)
 {
 	struct uart_8250_port *p = param;
 	struct uart_8250_dma *dma = p->dma;
+	struct dma_tx_state state;
+	enum dma_status status;
 	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) {
+		status = dmaengine_tx_status(dma->rxchan, dma->rx_cookie, &state);
+		if (status == DMA_ERROR) {
+			serial8250_rx_dma_recover(p);
+			goto out;
+		}
+		if (status != DMA_COMPLETE)
+			goto out;
+		serial8250_rx_dma_copy(p, &state);
+		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;
+	int ret;
+
+	if (dma->rx_err) {
+		ret = dmaengine_terminate_sync(dma->rxchan);
+		if (ret) {
+			/* Retain ownership and keep RX masked if DMA cannot be stopped. */
+			dev_err(p->port.dev, "failed to stop RX DMA: %d\n", ret);
+			return;
+		}
+	} else {
+		dmaengine_synchronize(dma->rxchan);
+	}
+
+	guard(serial8250_rpm)(p);
+
+	guard(uart_port_lock_irqsave)(&p->port);
+
+	dma->rx_terminating = false;
+	dma->rx_running = 0;
+	if (dma->shutting_down)
+		return;
+
+	if (dma->rx_err) {
+		/* PIO may now drain the FIFO; stop_rx() can cancel these enables. */
+		p->ier |= dma->rx_ier;
+		dma->rx_ier = 0;
+		serial_port_out(&p->port, UART_IER, p->ier);
+		dev_warn(p->port.dev, "RX DMA failed, using PIO\n");
+	}
+}
+
 int serial8250_tx_dma(struct uart_8250_port *p)
 {
 	struct uart_8250_dma		*dma = p->dma;
@@ -242,6 +295,8 @@ int serial8250_rx_dma(struct uart_8250_port *p)
 
 	if (dma->shutting_down || dma->rx_running)
 		return 0;
+	if (dma->rx_err)
+		return -EIO;
 
 	serial8250_do_prepare_rx_dma(p);
 
@@ -262,16 +317,49 @@ 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;
+	struct dma_tx_state state;
+	enum dma_status status;
+	int ret;
 
-	if (!dma || !dma->rxchan || !dma->rx_running)
-		return;
+	lockdep_assert_held_once(&p->port.lock);
 
-	dmaengine_pause(dma->rxchan);
-	__dma_rx_complete(p);
-	dmaengine_terminate_async(dma->rxchan);
+	if (!dma || dma->shutting_down || !dma->rxchan || !dma->rx_running)
+		return 0;
+	if (dma->rx_terminating) {
+		if (dma->rx_err) {
+			/* A termios or RS485 change may have enabled RX interrupts. */
+			serial8250_rx_dma_mask(p);
+			return -EIO;
+		}
+		return 0;
+	}
+
+	ret = dmaengine_pause(dma->rxchan);
+	if (ret)
+		goto recover;
+
+	status = dmaengine_tx_status(dma->rxchan, dma->rx_cookie, &state);
+	if (status == DMA_ERROR) {
+		ret = -EIO;
+		goto recover;
+	}
+	/* Some providers still report IN_PROGRESS after a successful pause. */
+	serial8250_rx_dma_copy(p, &state);
+	dma->rx_terminating = true;
+	ret = dmaengine_terminate_async(dma->rxchan);
+	if (ret)
+		goto recover;
+
+	/* Keep the buffer reserved until any old completion has returned. */
+	schedule_work(&dma->rx_flush_work);
+	return 0;
+
+recover:
+	serial8250_rx_dma_recover(p);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(serial8250_rx_dma_flush);
 
@@ -369,8 +457,12 @@ 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->tx_flushed = false;
+	dma->rx_terminating = false;
+	dma->rx_err = false;
+	dma->rx_ier = 0;
 
 	scoped_guard(uart_port_lock_irqsave, &p->port)
 		dma->shutting_down = false;
@@ -396,6 +488,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..8a637a85f101 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);
 	}
@@ -435,10 +441,11 @@ static int dw8250_handle_irq(struct uart_port *p)
 	 * read.  If we don't do this then the "RX TIMEOUT" interrupt will
 	 * fire forever.
 	 *
-	 * This problem has only been observed so far when not in DMA mode
-	 * so we limit the workaround only to non-DMA mode.
+	 * Only read the FIFO when DMA no longer owns it, including PIO fallback
+	 * after a DMA error.
 	 */
-	if (!(quirks & DW_UART_QUIRK_SKIP_EMPTY_FIFO_READ) && !up->dma && rx_timeout) {
+	if (!(quirks & DW_UART_QUIRK_SKIP_EMPTY_FIFO_READ) &&
+	    (!up->dma || !up->dma->rx_running) && rx_timeout) {
 		status = serial_lsr_in(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 f36f7e18ab95..2f901e97aade 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1305,6 +1305,8 @@ static void serial8250_stop_rx(struct uart_port *port)
 
 	guard(serial8250_rpm)(up);
 
+	if (up->dma)
+		up->dma->rx_ier = 0;
 	up->ier &= ~(UART_IER_RLSI | UART_IER_RDI);
 	serial_port_out(port, UART_IER, up->ier);
 }
@@ -1816,8 +1818,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 v2 1/4] serial: 8250_omap: separate locked TX DMA completion processing

[sashiko-bot]

> Separate the port-locked completion processing from the DMA callback
> wrapper. Keep the shutdown check and locking in the callback, and retain
> the existing FIFO accounting, delayed register restore and THRI handling
> in the helper.
> 
> 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/20261022-submit-serial-8250-dma-cancel-v1-v2-0-088ea51e3891@gmail.com?part=1

Re: [PATCH v2 3/4] serial: 8250: drain canceled TX DMA before reusing its buffer

[sashiko-bot]

> uart_flush_buffer() resets the transmit kfifo before calling the driver.
> serial8250_tx_dma_flush() terminates DMA asynchronously and clears
> tx_running, allowing a new write to replace tx_size while the old callback
> is still waiting for the port lock. That callback can then consume bytes
> from the new transmission. Reusing the reset kfifo can also overwrite the
> source of a transfer that has not stopped yet.
> 
> Reserve the flushed FIFO storage through serial core before returning
> from the flush callback. Keep tx_running set until a worker synchronizes
> the terminated channel, suppressing old callbacks and new submissions
> during that interval. Return the mapping to CPU ownership before releasing
> the reservation and waking writers. Normal transfers continue to use the
> FIFO directly.
> 
> Reuse the locked completion processing after retirement, including OMAP
> delayed register restoration and transmitter-empty handling. Take a runtime
> PM reference around worker UART accesses and drain the worker before
> releasing the channels. Shutdown releases the reservation after stopping
> DMA and unmapping its source.
> 
> Track a flushed FIFO separately from the transfer length. If termination
> fails, retain the reservation until normal completion and account the
> completed bytes without advancing the reset FIFO. After successful
> cancellation, process a zero-length completion once callbacks have drained.
> 
> 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/20261022-submit-serial-8250-dma-cancel-v1-v2-0-088ea51e3891@gmail.com?part=3

Re: [PATCH v2 2/4] serial: core: allow drivers to reserve flushed transmit storage

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] serial: core: uart_flush_buffer() and uart_write_wakeup() ignore xmit_blocked
- [Medium] serial: core: uart_write() and __uart_start() start TX while xmit_blocked
--

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

serial: core: allow drivers to reserve flushed transmit storage

This commit adds uart_xmit_block() and uart_xmit_unblock() so UART drivers
with asynchronous TX DMA can reserve flushed transmit buffer storage until
in-flight transfers and completion callbacks retire. While blocked,
uart_write_room() returns zero and uart_write() and uart_put_char() reject
new data until uart_xmit_unblock() wakes writers.

> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index c0deec0e6af41..f82c812f842f1 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -123,6 +123,46 @@ void uart_write_wakeup(struct uart_port *port)
>  }
>  EXPORT_SYMBOL(uart_write_wakeup);
>  
> +/**
> + * uart_xmit_block - reserve flushed transmit storage
> + * @port: port whose transmit storage is still in use
> + *
> + * A driver may call this from ->flush_buffer() if DMA still owns the old
> + * transmit data. The logical FIFO has already been emptied, but writers
> + * must not overwrite its storage until uart_xmit_unblock() is called.
> + * write_room() reports zero and write()/put_char() accept no data meanwhile.
> + *
> + * The driver must unblock after retiring the transfer, including on shutdown.
> + * Repeated calls are idempotent, not nested reservations.
> + *
> + * Context: @port->lock held.
> + */
> +void uart_xmit_block(struct uart_port *port)
> +{
> +	lockdep_assert_held_once(&port->lock);
> +	port->state->xmit_blocked = true;
> +}
> +EXPORT_SYMBOL_GPL(uart_xmit_block);
> +
> +/**
> + * uart_xmit_unblock - release reserved transmit storage and wake writers
> + * @port: port whose storage is available again
> + *
> + * The old transfer must no longer access the FIFO storage, and any required
> + * DMA synchronization must have completed before this call.
> + *
> + * Context: @port->lock held.
> + */
> +void uart_xmit_unblock(struct uart_port *port)
> +{
> +	lockdep_assert_held_once(&port->lock);
> +	if (!port->state->xmit_blocked)
> +		return;
> +	port->state->xmit_blocked = false;
> +	uart_write_wakeup(port);
> +}
> +EXPORT_SYMBOL_GPL(uart_xmit_unblock);

[Severity: High]
Should uart_flush_buffer() and uart_write_wakeup() suppress waking the TTY
layer while state->xmit_blocked is true?

In drivers/tty/serial/serial_core.c, uart_flush_buffer() resets the FIFO,
calls port->ops->flush_buffer(port) where the driver calls
uart_xmit_block(port), drops port->lock, and then unconditionally calls
tty_port_tty_wakeup():

drivers/tty/serial/serial_core.c:uart_flush_buffer() {
	...
	kfifo_reset(&state->port.xmit_fifo);
	if (port->ops->flush_buffer)
		port->ops->flush_buffer(port);
	uart_port_unlock_deref(port, flags);
	tty_port_tty_wakeup(&state->port);
}

Similarly, uart_write_wakeup() does not check state->xmit_blocked:

drivers/tty/serial/serial_core.c:uart_write_wakeup() {
	struct uart_state *state = port->state;
	...
	BUG_ON(!state);
	tty_port_tty_wakeup(&state->port);
}

If a caller invokes uart_write_wakeup() while state->xmit_blocked is true
(for example via uart_handle_cts_change(), __uart_port_tx(), or a driver TX
interrupt seeing kfifo_len(&tport->xmit_fifo) < WAKEUP_CHARS), or when
uart_flush_buffer() finishes, tty_port_tty_wakeup() -> tty_wakeup() runs
while uart_write_room() is still 0 and uart_write() / uart_put_char() still
reject all data.

Because uart_flush_buffer() also drops port->lock before calling
tty_port_tty_wakeup(), a concurrent writer that sees state->xmit_blocked,
gets 0 from uart_write(), and sets TTY_DO_WRITE_WAKEUP in that window will
have its wakeup bit consumed immediately by uart_flush_buffer() while
state->xmit_blocked is still true.

When tty_wakeup() fires while state->xmit_blocked is true, several line
disciplines and TTY clients break:

1. In drivers/tty/n_tty.c:n_tty_write_wakeup():

	clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
	kill_fasync(&tty->fasync, SIGIO, POLL_OUT);

TTY_DO_WRITE_WAKEUP is cleared and SIGIO is sent while uart_write_room() is
0 (so n_tty_poll() reports no EPOLLOUT). If the process checks poll() or
select() rather than calling write() during the premature signal,
TTY_DO_WRITE_WAKEUP remains cleared and the later uart_xmit_unblock() ->
uart_write_wakeup() will not send SIGIO when the buffer is actually
unblocked.

2. In drivers/tty/serdev/serdev-ttyport.c:ttyport_write_wakeup() and
drivers/tty/n_gsm.c:gsm_data_kick(), TTY_DO_WRITE_WAKEUP is cleared and the
write wakeup callback is invoked while uart_write() still returns 0, losing
the one-shot wakeup needed when uart_xmit_unblock() is later called.

3. In drivers/net/mctp/mctp-serial.c:mctp_serial_tty_write_wakeup() ->
mctp_serial_tx_work():

	txlen = write_chunk(dev, buf + dev->txpos, 3 - dev->txpos);
	if (txlen <= 0) {
		dev->txstate = STATE_ERR;

Waking TX while state->xmit_blocked is true causes tty->ops->write() to
return 0, which puts the device into STATE_ERR and leaves its netdev TX
queue permanently stopped.

4. In drivers/input/joystick/iforce/iforce-serio.c:iforce_serio_write_wakeup()
-> iforce_serio_xmit(), queued packets are drained into tty->ops->write()
while state->xmit_blocked is true and silently dropped. In serdev clients
such as btnxpuart_tx_work(), waking TX while serdev_device_write_buf()
returns 0 causes the worker to dequeue an skb, write 0 bytes, requeue it at
the head of nxpdev->txq, and loop. In slcan_transmit(), slip_transmit(), and
qcauart_transmit(), when xleft / tx_left is 0, the premature wakeup clears
TTY_DO_WRITE_WAKEUP and calls netif_wake_queue() while the UART still
rejects writes.

[ ... ]

> @@ -624,7 +665,7 @@ static ssize_t uart_write(struct tty_struct *tty, const u8 *buf, size_t count)
>  		return 0;
>  	}
>  
> -	if (port)
> +	if (port && !state->xmit_blocked)
>  		ret = kfifo_in(&state->port.xmit_fifo, buf, count);
>  
>  	__uart_start(state);

[Severity: Medium]
When state->xmit_blocked is true, uart_write() skips kfifo_in() and queues 0
bytes, but still calls __uart_start(state) (which can also be reached via
uart_flush_chars() -> uart_start() when n_tty_write() sees 0 write_room in
O_OPOST mode):

drivers/tty/serial/serial_core.c:__uart_start() {
	struct uart_port *port = state->uart_port;
	struct serial_port_device *port_dev;
	int err;

	if (!port || port->flags & UPF_DEAD || uart_tx_stopped(port))
		return;
	...
	if (!pm_runtime_enabled(port->dev) || pm_runtime_active(&port_dev->dev))
		port->ops->start_tx(port);
}

Should __uart_start() (or uart_write()) avoid calling port->ops->start_tx()
while state->xmit_blocked is true?

In drivers whose start_tx() touches hardware before checking whether
xmit_fifo is empty, calling start_tx() with an empty FIFO while TX storage
is blocked has unintended side effects:

1. In drivers/tty/serial/fsl_lpuart.c:lpuart_start_tx(),
drivers/tty/serial/sh-sci.c:sci_start_tx(), and
drivers/tty/serial/atmel_serial.c:atmel_start_tx(), start_tx() enables TX
interrupts before checking kfifo_is_empty(), triggering a spurious TX
interrupt or tasklet that sees kfifo_len(&tport->xmit_fifo) < WAKEUP_CHARS
and calls uart_write_wakeup(port) while state->xmit_blocked is still true.

2. In drivers/tty/serial/atmel_serial.c:atmel_start_tx(), half-duplex RX is
stopped via atmel_stop_rx(port) and ATMEL_US_TXEN is re-enabled even though
TX storage is blocked and no data was queued.

3. In drivers/tty/serial/amba-pl011.c:pl011_start_tx(), when RS485 is
enabled (SER_RS485_ENABLED), pl011_rs485_tx_start() is called before
checking whether xmit_fifo has data, asserting RTS and disabling RX
(~UART011_CR_RXE) on the RS485 bus despite 0 bytes being accepted.

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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] serial: 8250: dw8250_handle_irq() skips RX timeout quirk while rx_terminating
- [High] serial: 8250: RX interrupts unmasked in UART_IER during RX DMA error recovery
- [Medium] serial: 8250: serial8250_stop_rx() drops UART_IER_RTOIE from dma->rx_ier
- [Medium] serial: 8250: serial8250_rx_dma_flush() allows PIO RX while DMA runs on shutdown
--

commit d3768e2140777caa150bf17338f045f59925e47a
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 until a workqueue item has drained
canceled completion callbacks after an RX timeout flush, and adds error
recovery that masks receive interrupts and falls back to PIO if pausing or
terminating RX DMA fails.

> diff --git a/drivers/tty/serial/8250/8250_dma.c b/drivers/tty/serial/8250/8250_dma.c
> index bb30d3588f16f..4cadb3eef9413 100644
> --- a/drivers/tty/serial/8250/8250_dma.c
> +++ b/drivers/tty/serial/8250/8250_dma.c

[ ... ]

> @@ -262,16 +317,49 @@ 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;
> +	struct dma_tx_state state;
> +	enum dma_status status;
> +	int ret;
>  
> -	if (!dma || !dma->rxchan || !dma->rx_running)
> -		return;
> +	lockdep_assert_held_once(&p->port.lock);
>  
> -	dmaengine_pause(dma->rxchan);
> -	__dma_rx_complete(p);
> -	dmaengine_terminate_async(dma->rxchan);
> +	if (!dma || dma->shutting_down || !dma->rxchan || !dma->rx_running)
> +		return 0;

[Severity: Medium]
When dma->shutting_down is true while dma->rx_running is still non-zero (or
while dma->rx_err is set during recovery), serial8250_rx_dma_flush() returns
0 without pausing or stopping the RX DMA channel.

In drivers/tty/serial/8250/8250_port.c, serial8250_do_shutdown() sets
up->dma->shutting_down = true under port->lock before calling
synchronize_irq(port->irq), and only terminates the RX DMA channel afterward
in serial8250_release_dma():

drivers/tty/serial/8250/8250_port.c:serial8250_do_shutdown() {
	scoped_guard(uart_port_lock_irqsave, port) {
		if (up->dma)
			up->dma->shutting_down = true;
		up->ier = 0;
		serial_port_out(port, UART_IER, 0);
	}

	synchronize_irq(port->irq);

	if (up->dma)
		serial8250_release_dma(up);
}

If an in-flight IRQ handler (such as serial8250_default_handle_irq() or
dw8250_handle_irq()) is waiting on port->lock when serial8250_do_shutdown()
sets shutting_down = true, serial8250_handle_irq_locked() -> handle_rx_dma()
evaluates !serial8250_rx_dma_flush(up) as true and proceeds to drain UART_RX
via serial8250_rx_chars().

Could that allow PIO FIFO reads to run concurrently with an active, unpaused
RX DMA transfer during shutdown?

> +	if (dma->rx_terminating) {
> +		if (dma->rx_err) {
> +			/* A termios or RS485 change may have enabled RX interrupts. */
> +			serial8250_rx_dma_mask(p);
> +			return -EIO;
> +		}
> +		return 0;
> +	}

[Severity: High]
Is re-masking RX interrupts inside serial8250_rx_dma_flush() sufficient when
termios, RS485, or overrun backoff paths modify UART_IER while dma->rx_err is
set?

During RX DMA error recovery (or permanently if dmaengine_terminate_sync()
fails in serial8250_rx_dma_flush_work()), serial8250_set_ier(),
serial8250_em485_stop_tx(), and serial_8250_overrun_backoff_work() in
drivers/tty/serial/8250/8250_port.c can still set UART_IER_RTOIE or
UART_IER_RLSI | UART_IER_RDI in up->ier and write up->ier directly to
UART_IER (with serial8250_em485_stop_tx() also resetting the FIFOs via
serial8250_clear_and_reinit_fifos()).

However, serial8250_handle_irq_locked() only calls handle_rx_dma() ->
serial8250_rx_dma_flush() when (status & (UART_LSR_DR | UART_LSR_BI)) &&
!skip_rx is true:

drivers/tty/serial/8250/8250_port.c:serial8250_handle_irq_locked() {
	if (!(status & (UART_LSR_FIFOE | UART_LSR_BRK_ERROR_BITS)) &&
	    (port->status & (UPSTAT_AUTOCTS | UPSTAT_AUTORTS)) &&
	    !(up->ier & (UART_IER_RLSI | UART_IER_RDI)))
		skip_rx = true;

	if (status & (UART_LSR_DR | UART_LSR_BI) && !skip_rx) {
		...
		if (!up->dma || handle_rx_dma(up, iir))
			status = serial8250_rx_chars(up, status);
	}
}

If serial8250_set_ier() enables UART_IER_RTOIE on a port with UART_CAP_RTOIE
and hardware flow control (UPSTAT_AUTOCTS | UPSTAT_AUTORTS) while
UART_IER_RLSI | UART_IER_RDI remain masked, skip_rx is true, so an RX timeout
interrupt will never reach handle_rx_dma() or serial8250_rx_chars() to be
cleared or re-masked.

Similarly, if an unmasked RX interrupt fires when UART_LSR_DR | UART_LSR_BI
is clear (for instance, on a DesignWare empty-FIFO UART_IIR_RX_TIMEOUT while
up->dma->rx_running is still 1, or if the unstopped DMA channel drains the
byte before serial_lsr_in()), handle_rx_dma() is skipped and the interrupt
remains unmasked.

Could those UART_IER writers avoid enabling RX interrupts while RX DMA
recovery owns the mask, rather than relying on serial8250_rx_dma_flush() to
catch the resulting interrupt?

> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> index eca02e8bd6d13..8a637a85f1013 100644
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c

[ ... ]

> @@ -435,10 +441,11 @@ static int dw8250_handle_irq(struct uart_port *p)
>  	 * read.  If we don't do this then the "RX TIMEOUT" interrupt will
>  	 * fire forever.
>  	 *
> -	 * This problem has only been observed so far when not in DMA mode
> -	 * so we limit the workaround only to non-DMA mode.
> +	 * Only read the FIFO when DMA no longer owns it, including PIO fallback
> +	 * after a DMA error.
>  	 */
> -	if (!(quirks & DW_UART_QUIRK_SKIP_EMPTY_FIFO_READ) && !up->dma && rx_timeout) {
> +	if (!(quirks & DW_UART_QUIRK_SKIP_EMPTY_FIFO_READ) &&
> +	    (!up->dma || !up->dma->rx_running) && rx_timeout) {
>  		status = serial_lsr_in(up);
>  
>  		/*

[Severity: High]
When an RX timeout flushes DMA in serial8250_rx_dma_flush(),
dma->rx_terminating is set to true and dmaengine_terminate_async() stops the
transfer so PIO can drain the FIFO, but dma->rx_running remains 1 until
serial8250_rx_dma_flush_work() runs in process context:

drivers/tty/serial/8250/8250_dma.c:serial8250_rx_dma_flush() {
	serial8250_rx_dma_copy(p, &state);
	dma->rx_terminating = true;
	ret = dmaengine_terminate_async(dma->rxchan);
	if (ret)
		goto recover;

	/* Keep the buffer reserved until any old completion has returned. */
	schedule_work(&dma->rx_flush_work);
	return 0;
}

If the DesignWare UART triggers an empty-FIFO UART_IIR_RX_TIMEOUT interrupt
during this window before serial8250_rx_dma_flush_work() executes,
(!up->dma || !up->dma->rx_running) in dw8250_handle_irq() evaluates to false
and skips the dummy serial_port_in(p, d->data.srbr) read. Because UART_LSR_DR
and UART_LSR_BI are clear, serial8250_handle_irq_locked() does not read RBR
either.

Wouldn't that leave the level-triggered RX timeout interrupt asserted and
cause an interrupt storm that prevents serial8250_rx_dma_flush_work() from
running on the local CPU? Also, right below in dw8250_handle_irq(), the
DW_UART_QUIRK_IS_DMA_FC check tests up->dma->rx_running and would still write
RZN1_UART_RDMACR and DW_UART_DMASA on subsequent RX timeouts even after
serial8250_rx_dma_flush() has already terminated the transfer.

> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index f36f7e18ab951..2f901e97aaded 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -1305,6 +1305,8 @@ static void serial8250_stop_rx(struct uart_port *port)
>  
>  	guard(serial8250_rpm)(up);
>  
> +	if (up->dma)
> +		up->dma->rx_ier = 0;
>  	up->ier &= ~(UART_IER_RLSI | UART_IER_RDI);
>  	serial_port_out(port, UART_IER, up->ier);
>  }

[Severity: Medium]
In drivers/tty/serial/8250/8250_dma.c, serial8250_rx_dma_mask() moves
UART_IER_RDI | UART_IER_RLSI | UART_IER_RTOIE out of p->ier and into
p->dma->rx_ier.

Here in serial8250_stop_rx(), up->ier only clears ~(UART_IER_RLSI |
UART_IER_RDI), whereas up->dma->rx_ier = 0 clears UART_IER_RTOIE as well. If
serial8250_stop_rx() is called during RX DMA error recovery (for example,
via serial8250_em485_start_tx() during half-duplex RS485 transmission, where
serial8250_em485_stop_tx() later only re-enables UART_IER_RLSI |
UART_IER_RDI), wouldn't clearing all of up->dma->rx_ier lose UART_IER_RTOIE
once PIO fallback completes?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022-submit-serial-8250-dma-cancel-v1-v2-0-088ea51e3891@gmail.com?part=4

Re: [PATCH v2 1/4] serial: 8250_omap: separate locked TX DMA completion processing

[Andy Shevchenko]

On Thu, Oct 22, 2026 at 06:34:02PM -0600, James Hilliard wrote:
> Separate the port-locked completion processing from the DMA callback
> wrapper. Keep the shutdown check and locking in the callback, and retain
> the existing FIFO accounting, delayed register restore and THRI handling
> in the helper.

...

> +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)
> +		omap_8250_tx_dma_complete(p);

Taking into account the name and the purpose of the function I would refactor
this to

	if (dma->shutting_down)
		return;

	omap_8250_tx_dma_complete(p);

> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/4] serial: 8250: drain canceled TX DMA before reusing its buffer

[Andy Shevchenko]

On Thu, Oct 22, 2026 at 06:34:04PM -0600, James Hilliard wrote:
> uart_flush_buffer() resets the transmit kfifo before calling the driver.
> serial8250_tx_dma_flush() terminates DMA asynchronously and clears
> tx_running, allowing a new write to replace tx_size while the old callback
> is still waiting for the port lock. That callback can then consume bytes
> from the new transmission. Reusing the reset kfifo can also overwrite the
> source of a transfer that has not stopped yet.
> 
> Reserve the flushed FIFO storage through serial core before returning
> from the flush callback. Keep tx_running set until a worker synchronizes
> the terminated channel, suppressing old callbacks and new submissions
> during that interval. Return the mapping to CPU ownership before releasing
> the reservation and waking writers. Normal transfers continue to use the
> FIFO directly.
> 
> Reuse the locked completion processing after retirement, including OMAP
> delayed register restoration and transmitter-empty handling. Take a runtime
> PM reference around worker UART accesses and drain the worker before
> releasing the channels. Shutdown releases the reservation after stopping
> DMA and unmapping its source.
> 
> Track a flushed FIFO separately from the transfer length. If termination
> fails, retain the reservation until normal completion and account the
> completed bytes without advancing the reset FIFO. After successful
> cancellation, process a zero-length completion once callbacks have drained.
> 
> The race was found by inspecting cancellation and restart while preparing
> H616 UART DMA support.

...

> -	if (!dma->tx_running)
> +	if (!dma->tx_running || dma->tx_terminating || dma->shutting_down)
>  		return;

...

> +	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(serial8250_rpm)(p);
> +
> +	guard(uart_port_lock_irqsave)(&p->port);
> +
> +	dma->tx_terminating = false;
> +	dma->tx_size = 0;
> +	if (!dma->shutting_down)

If you write this as

	if (!dma->tx_terminating && !dma->shutting_down)

> +		dma->tx_complete(p);
>  }

...

>  	if (dma->shutting_down)
>  		return 0;
> +	if (dma->tx_terminating)
> +		return 0;

And this

	if (dma->tx_terminating || dma->shutting_down)
		return 0;

...

> -	if (!dma->tx_running)
> +	if (!dma->tx_running || dma->tx_terminating || dma->shutting_down)
>  		return;
>  
>  	/*
>  	 * kfifo_reset() has been called by the serial core, avoid
> -	 * advancing and underflowing in __dma_tx_complete().
> +	 * advancing the reset FIFO when the old transfer completes.
>  	 */

...

> +	dma_sync_single_for_cpu(tx_dev, dma->tx_addr,
> +				UART_XMIT_SIZE, DMA_TO_DEVICE);

Make it a single line.

...

> +free_rx:
> +	dma_free_coherent(rx_dev, dma->rx_size,
> +			  dma->rx_buf, dma->rx_addr);

Ditto.

...

>  	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)) {

So you will have a couple of patterns for the combinations of the flags.
With some static helpers you may optimise that. Also in some cases you even
might consider checking tx_running while it's known it's true (or false)
to reduce amount of variations of the combinations of the flags.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Thu, Oct 22, 2026 at 06:34:05PM -0600, James Hilliard wrote:
> The RX timeout path pauses a transfer, copies its data and terminates it
> asynchronously. Clearing rx_running at that point lets another receive
> reuse the buffer and cookie while the old callback is still waiting for
> the port lock. Inspecting the new cookie cannot distinguish that callback
> from the completion of a new transfer.
> 
> Reserve the receive buffer until a worker has drained the canceled
> callbacks. Deliver the paused data before allowing the interrupt handler
> to drain the FIFO through PIO. Normal retirement only releases ownership;
> leave subsequent receive handling to the UART interrupt instead of reading
> LSR and restarting DMA from the worker.
> 
> Reject new flush work during shutdown, including late Intel MID DMA
> timeouts. Serialize those timeout callers with the port lock. Stop and
> drain retirement work before freeing the channels and buffer.
> 
> Check pause failures before accessing the buffer. A successful pause is
> the quiescence guarantee, even when a provider still reports IN_PROGRESS.
> On pause, status or termination failure, mask receive interrupts and let
> the worker stop DMA before handing the FIFO to PIO. Preserve stop_rx() and
> shutdown while restoring deferred interrupts, and hold a runtime PM
> reference around worker register accesses. Keep RX on PIO until reopen.
> 
> Abort the DesignWare idle sequence when RX cannot be stopped. Preserve its
> empty-FIFO timeout workaround during PIO recovery. If the provider still
> cannot terminate the channel in process context, retain its buffer and
> leave receive interrupts masked.

...

> struct uart_8250_dma {

>  	bool			shutting_down;
>  	bool			tx_terminating;
>  	bool			tx_flushed;
> +	bool			rx_terminating;
> +	bool			rx_err;

If they are protected by the same lock, we may actually move to bitfields.
Example:

	unsigned int		rx_err:1;

> +	/* Receive interrupt enables deferred during RX DMA recovery. */
> +	u8			rx_ier;
>  };

...

>  static void dma_rx_complete(void *param)
>  {
>  	struct uart_8250_port *p = param;
>  	struct uart_8250_dma *dma = p->dma;
> +	struct dma_tx_state state;
> +	enum dma_status status;
>  	unsigned long flags;
>  
>  	uart_port_lock_irqsave(&p->port, &flags);
> -	if (dma->shutting_down)
> +	if (dma->shutting_down || dma->rx_terminating)
>  		goto out;

Again repeating the same idiom you have in previous patch.

> -	if (dma->rx_running)
> -		__dma_rx_complete(p);
> +	if (dma->rx_running) {
> +		status = dmaengine_tx_status(dma->rxchan, dma->rx_cookie, &state);
> +		if (status == DMA_ERROR) {
> +			serial8250_rx_dma_recover(p);
> +			goto out;
> +		}
> +		if (status != DMA_COMPLETE)
> +			goto out;
> +		serial8250_rx_dma_copy(p, &state);
> +		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))

This has a side-effect. Earlier we do read LSR_DR, but now only if _RDI is set.
Would it be problematic?

>  		p->dma->rx_dma(p);
>  out:
>  	uart_port_unlock_irqrestore(&p->port, flags);
>  }

...

> +	dma->rx_terminating = false;
> +	dma->rx_running = 0;
> +	if (dma->shutting_down)
> +		return;

> +	if (dma->rx_err) {

Perhaps

	if (!dma->rx_err)
		return;

?

> +		/* PIO may now drain the FIFO; stop_rx() can cancel these enables. */
> +		p->ier |= dma->rx_ier;
> +		dma->rx_ier = 0;
> +		serial_port_out(&p->port, UART_IER, p->ier);
> +		dev_warn(p->port.dev, "RX DMA failed, using PIO\n");
> +	}
> +}

...

> +	/* Some providers still report IN_PROGRESS after a successful pause. */

Yeah, we need to address that. (I used to have some patches locally,
but they were rebased uncountable number of times without looking at
the result of those rebases, so it's probably rotten enough and may
not be used anymore.)

...

> -	 * This problem has only been observed so far when not in DMA mode
> -	 * so we limit the workaround only to non-DMA mode.
> +	 * Only read the FIFO when DMA no longer owns it, including PIO fallback
> +	 * after a DMA error.

> -	if (!(quirks & DW_UART_QUIRK_SKIP_EMPTY_FIFO_READ) && !up->dma && rx_timeout) {
> +	if (!(quirks & DW_UART_QUIRK_SKIP_EMPTY_FIFO_READ) &&
> +	    (!up->dma || !up->dma->rx_running) && rx_timeout) {
>  		status = serial_lsr_in(up);

This is a good question. We need to have this be tested on the affected
platforms.

...

>  	guard(serial8250_rpm)(up);
>  
> +	if (up->dma)
> +		up->dma->rx_ier = 0;

Now we have a copy of IER in UART port and in DMA structure. Why do we need
both?

>  	up->ier &= ~(UART_IER_RLSI | UART_IER_RDI);
>  	serial_port_out(port, UART_IER, up->ier);

-- 
With Best Regards,
Andy Shevchenko

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

[James Hilliard]

On Fri, Oct 23, 2026 at 2:30 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Oct 22, 2026 at 06:34:05PM -0600, James Hilliard wrote:
> > The RX timeout path pauses a transfer, copies its data and terminates it
> > asynchronously. Clearing rx_running at that point lets another receive
> > reuse the buffer and cookie while the old callback is still waiting for
> > the port lock. Inspecting the new cookie cannot distinguish that callback
> > from the completion of a new transfer.
> >
> > Reserve the receive buffer until a worker has drained the canceled
> > callbacks. Deliver the paused data before allowing the interrupt handler
> > to drain the FIFO through PIO. Normal retirement only releases ownership;
> > leave subsequent receive handling to the UART interrupt instead of reading
> > LSR and restarting DMA from the worker.
> >
> > Reject new flush work during shutdown, including late Intel MID DMA
> > timeouts. Serialize those timeout callers with the port lock. Stop and
> > drain retirement work before freeing the channels and buffer.
> >
> > Check pause failures before accessing the buffer. A successful pause is
> > the quiescence guarantee, even when a provider still reports IN_PROGRESS.
> > On pause, status or termination failure, mask receive interrupts and let
> > the worker stop DMA before handing the FIFO to PIO. Preserve stop_rx() and
> > shutdown while restoring deferred interrupts, and hold a runtime PM
> > reference around worker register accesses. Keep RX on PIO until reopen.
> >
> > Abort the DesignWare idle sequence when RX cannot be stopped. Preserve its
> > empty-FIFO timeout workaround during PIO recovery. If the provider still
> > cannot terminate the channel in process context, retain its buffer and
> > leave receive interrupts masked.
>
> ...
>
> > struct uart_8250_dma {
>
> >       bool                    shutting_down;
> >       bool                    tx_terminating;
> >       bool                    tx_flushed;
> > +     bool                    rx_terminating;
> > +     bool                    rx_err;
>
> If they are protected by the same lock, we may actually move to bitfields.
> Example:
>
>         unsigned int            rx_err:1;
>
> > +     /* Receive interrupt enables deferred during RX DMA recovery. */
> > +     u8                      rx_ier;
> >  };
>
> ...
>
> >  static void dma_rx_complete(void *param)
> >  {
> >       struct uart_8250_port *p = param;
> >       struct uart_8250_dma *dma = p->dma;
> > +     struct dma_tx_state state;
> > +     enum dma_status status;
> >       unsigned long flags;
> >
> >       uart_port_lock_irqsave(&p->port, &flags);
> > -     if (dma->shutting_down)
> > +     if (dma->shutting_down || dma->rx_terminating)
> >               goto out;
>
> Again repeating the same idiom you have in previous patch.
>
> > -     if (dma->rx_running)
> > -             __dma_rx_complete(p);
> > +     if (dma->rx_running) {
> > +             status = dmaengine_tx_status(dma->rxchan, dma->rx_cookie, &state);
> > +             if (status == DMA_ERROR) {
> > +                     serial8250_rx_dma_recover(p);
> > +                     goto out;
> > +             }
> > +             if (status != DMA_COMPLETE)
> > +                     goto out;
> > +             serial8250_rx_dma_copy(p, &state);
> > +             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))
>
> This has a side-effect. Earlier we do read LSR_DR, but now only if _RDI is set.
> Would it be problematic?

The RDI check is intended to prevent a completion from restarting RX DMA
after stop_rx(). That only requires gating resubmission. I'll preserve
the previous LSR read when rx_running is clear, then check RDI before
submitting another transfer.

>
> >               p->dma->rx_dma(p);
> >  out:
> >       uart_port_unlock_irqrestore(&p->port, flags);
> >  }
>
> ...
>
> > +     dma->rx_terminating = false;
> > +     dma->rx_running = 0;
> > +     if (dma->shutting_down)
> > +             return;
>
> > +     if (dma->rx_err) {
>
> Perhaps
>
>         if (!dma->rx_err)
>                 return;
>
> ?
>
> > +             /* PIO may now drain the FIFO; stop_rx() can cancel these enables. */
> > +             p->ier |= dma->rx_ier;
> > +             dma->rx_ier = 0;
> > +             serial_port_out(&p->port, UART_IER, p->ier);
> > +             dev_warn(p->port.dev, "RX DMA failed, using PIO\n");
> > +     }
> > +}
>
> ...
>
> > +     /* Some providers still report IN_PROGRESS after a successful pause. */
>
> Yeah, we need to address that. (I used to have some patches locally,
> but they were rebased uncountable number of times without looking at
> the result of those rebases, so it's probably rotten enough and may
> not be used anymore.)
>
> ...
>
> > -      * This problem has only been observed so far when not in DMA mode
> > -      * so we limit the workaround only to non-DMA mode.
> > +      * Only read the FIFO when DMA no longer owns it, including PIO fallback
> > +      * after a DMA error.
>
> > -     if (!(quirks & DW_UART_QUIRK_SKIP_EMPTY_FIFO_READ) && !up->dma && rx_timeout) {
> > +     if (!(quirks & DW_UART_QUIRK_SKIP_EMPTY_FIFO_READ) &&
> > +         (!up->dma || !up->dma->rx_running) && rx_timeout) {
> >               status = serial_lsr_in(up);
>
> This is a good question. We need to have this be tested on the affected
> platforms.

My hardware tests used H616 with additional local UART and sun6i DMAengine
changes applied.

There is also a distinction the current condition misses: rx_running
remains set while callbacks are being retired, even after DMA has stopped
accessing the FIFO. I'll make the FIFO-ownership check explicit rather
than using buffer retirement as a proxy. The dummy read must remain
excluded while DMA can access the FIFO. That revised handling needs
targeted testing of the empty-FIFO timeout case.

I'll also apply the early-return and shared-helper cleanups. Before
packing the flags into bitfields, I'll check the worker accesses as well
as the port-locked paths.

>
> ...
>
> >       guard(serial8250_rpm)(up);
> >
> > +     if (up->dma)
> > +             up->dma->rx_ier = 0;
>
> Now we have a copy of IER in UART port and in DMA structure. Why do we need
> both?

rx_ier records the receive-enable bits temporarily removed from up->ier
during recovery. Those interrupts must stay masked while DMA may still
access the FIFO, but recovery needs to distinguish that temporary mask
from an intentional stop_rx().

The separate saved enables make that distinction cumbersome, especially
when termios or RS485 changes IER during recovery. I'll rework this around
one desired-enable state and consistent application of the temporary
recovery mask, so the worker cannot restore an obsolete receive-enable
decision.

>
> >       up->ier &= ~(UART_IER_RLSI | UART_IER_RDI);
> >       serial_port_out(port, UART_IER, up->ier);
>
> --
> With Best Regards,
> Andy Shevchenko
>
>