[PATCH] serial: 8250: prevent DMA callbacks from restarting a closing port

[PATCH] serial: 8250: prevent DMA callbacks from restarting a closing port

[James Hilliard]

Disabling UART interrupts does not prevent a DMA completion callback from
running. A TX callback can submit more data or enable THRI after shutdown
has cleared IER. An RX callback can also submit a new receive if data is
still present. A callback already running when terminate_sync() starts
can therefore restart the channel whose buffers are about to be freed.

Mark DMA as shutting down under the port lock when disabling interrupts.
Check that state under the same lock in the completion callbacks and
submission paths, so callbacks either finish before the shutdown boundary
or leave the channel stopped. The DMA provider's synchronization still
has to drain callbacks before release_dma() frees their resources.

Apply the same boundary to OMAP, which has its own shutdown and DMA
callbacks but shares the channel-request and release helpers. Check it
in MediaTek's receive callback after taking the port lock as well: its
earlier private shutdown check cannot serialize a callback that has
already passed that check. Reset the state after requesting channels
on the next open. Leave the Broadcom integrated DMA path, which manages
its own shutdown state, unchanged.

Fixes: 9ee4b83e51f7 ("serial: 8250: Add support for dmaengine")
Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
 drivers/tty/serial/8250/8250.h      |  2 ++
 drivers/tty/serial/8250/8250_dma.c  | 17 ++++++++++++++---
 drivers/tty/serial/8250/8250_mtk.c  |  3 +++
 drivers/tty/serial/8250/8250_omap.c | 15 ++++++++++++---
 drivers/tty/serial/8250/8250_port.c |  2 ++
 5 files changed, 33 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index 21ed9c41b751..457732e941af 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -51,6 +51,8 @@ struct uart_8250_dma {
 	unsigned char		tx_running;
 	unsigned char		tx_err;
 	unsigned char		rx_running;
+	/* Protected by the UART port lock. */
+	bool			shutting_down;
 };
 
 struct old_serial_port {
diff --git a/drivers/tty/serial/8250/8250_dma.c b/drivers/tty/serial/8250/8250_dma.c
index 1da605cc8601..5f52ed5c14c3 100644
--- a/drivers/tty/serial/8250/8250_dma.c
+++ b/drivers/tty/serial/8250/8250_dma.c
@@ -19,11 +19,13 @@ static void __dma_tx_complete(void *param)
 	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);
 
-	uart_port_lock_irqsave(&p->port, &flags);
-
 	dma->tx_running = 0;
 
 	uart_xmit_advance(&p->port, dma->tx_size);
@@ -35,6 +37,7 @@ static void __dma_tx_complete(void *param)
 	if (ret || !dma->tx_running)
 		serial8250_set_THRI(p);
 
+out:
 	uart_port_unlock_irqrestore(&p->port, flags);
 }
 
@@ -71,6 +74,8 @@ static void dma_rx_complete(void *param)
 	unsigned long flags;
 
 	uart_port_lock_irqsave(&p->port, &flags);
+	if (dma->shutting_down)
+		goto out;
 	if (dma->rx_running)
 		__dma_rx_complete(p);
 
@@ -80,6 +85,7 @@ static void dma_rx_complete(void *param)
 	 */
 	if (!dma->rx_running && (serial_lsr_in(p) & UART_LSR_DR))
 		p->dma->rx_dma(p);
+out:
 	uart_port_unlock_irqrestore(&p->port, flags);
 }
 
@@ -94,6 +100,9 @@ int serial8250_tx_dma(struct uart_8250_port *p)
 	int i;
 	int ret;
 
+	if (dma->shutting_down)
+		return 0;
+
 	if (dma->tx_running) {
 		if (up->x_char) {
 			dmaengine_pause(dma->txchan);
@@ -185,7 +194,7 @@ int serial8250_rx_dma(struct uart_8250_port *p)
 	struct uart_8250_dma		*dma = p->dma;
 	struct dma_async_tx_descriptor	*desc;
 
-	if (dma->rx_running)
+	if (dma->shutting_down || dma->rx_running)
 		return 0;
 
 	serial8250_do_prepare_rx_dma(p);
@@ -308,6 +317,8 @@ int serial8250_request_dma(struct uart_8250_port *p)
 		goto err;
 	}
 
+	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;
diff --git a/drivers/tty/serial/8250/8250_mtk.c b/drivers/tty/serial/8250/8250_mtk.c
index d1fb917b762a..8a0ca8df46c5 100644
--- a/drivers/tty/serial/8250/8250_mtk.c
+++ b/drivers/tty/serial/8250/8250_mtk.c
@@ -106,6 +106,8 @@ static void mtk8250_dma_rx_complete(void *param)
 		return;
 
 	uart_port_lock_irqsave(&up->port, &flags);
+	if (dma->shutting_down)
+		goto out;
 
 	dmaengine_tx_status(dma->rxchan, dma->rx_cookie, &state);
 	total = dma->rx_size - state.residue;
@@ -131,6 +133,7 @@ static void mtk8250_dma_rx_complete(void *param)
 
 	mtk8250_rx_dma(up);
 
+out:
 	uart_port_unlock_irqrestore(&up->port, flags);
 }
 
diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
index 29a203ed93fd..e3f3322da379 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -780,6 +780,8 @@ static void omap_8250_shutdown(struct uart_port *port)
 
 	/* Synchronize UART_IER access against the console. */
 	scoped_guard(uart_port_lock_irq, port) {
+		if (up->dma)
+			up->dma->shutting_down = true;
 		up->ier = 0;
 		serial_out(up, UART_IER, 0);
 	}
@@ -963,6 +965,8 @@ static void __dma_rx_complete(void *param)
 
 	/* Synchronize UART_IER access against the console. */
 	guard(uart_port_lock_irqsave)(&p->port);
+	if (dma->shutting_down)
+		return;
 
 	/*
 	 * If the tx status is not DMA_COMPLETE, then this is a delayed
@@ -1019,6 +1023,9 @@ static int omap_8250_rx_dma(struct uart_8250_port *p)
 	/* Port locked to synchronize UART_IER access against the console. */
 	lockdep_assert_held_once(&p->port.lock);
 
+	if (dma->shutting_down)
+		return 0;
+
 	if (priv->rx_dma_broken)
 		return -EINVAL;
 
@@ -1080,11 +1087,13 @@ static void omap_8250_dma_tx_complete(void *param)
 	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);
 
-	guard(uart_port_lock_irqsave)(&p->port);
-
 	dma->tx_running = 0;
 
 	uart_xmit_advance(&p->port, dma->tx_size);
@@ -1123,7 +1132,7 @@ static int omap_8250_tx_dma(struct uart_8250_port *p)
 	int skip_byte = -1;
 	int ret;
 
-	if (dma->tx_running)
+	if (dma->shutting_down || dma->tx_running)
 		return 0;
 	if (uart_tx_stopped(&p->port) || kfifo_is_empty(&tport->xmit_fifo)) {
 
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index db066a42f61d..d476ebc37647 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -2446,6 +2446,8 @@ void serial8250_do_shutdown(struct uart_port *port)
 	 * Synchronize UART_IER access against the console.
 	 */
 	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);
 	}

---
base-commit: 1184f87e2790e8af9be3116f608d2e86b4792d7d
change-id: 20261021-submit-serial-8250-dma-shutdown-v1-533b4d64f45b

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

Re: [PATCH] serial: 8250: prevent DMA callbacks from restarting a closing port

[sashiko-bot]

> Disabling UART interrupts does not prevent a DMA completion callback from
> running. A TX callback can submit more data or enable THRI after shutdown
> has cleared IER. An RX callback can also submit a new receive if data is
> still present. A callback already running when terminate_sync() starts
> can therefore restart the channel whose buffers are about to be freed.
> 
> Mark DMA as shutting down under the port lock when disabling interrupts.
> Check that state under the same lock in the completion callbacks and
> submission paths, so callbacks either finish before the shutdown boundary
> or leave the channel stopped. The DMA provider's synchronization still
> has to drain callbacks before release_dma() frees their resources.
> 
> Apply the same boundary to OMAP, which has its own shutdown and DMA
> callbacks but shares the channel-request and release helpers. Check it
> in MediaTek's receive callback after taking the port lock as well: its
> earlier private shutdown check cannot serialize a callback that has
> already passed that check. Reset the state after requesting channels
> on the next open. Leave the Broadcom integrated DMA path, which manages
> its own shutdown state, unchanged.
> 
> Fixes: 9ee4b83e51f7 ("serial: 8250: Add support for dmaengine")
> 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-shutdown-v1-v1-1-3d3ce383c614@gmail.com?part=1