* [PATCH v8 1/6] serial: 8250_omap: separate locked TX DMA completion processing
2026-10-29 7:36 [PATCH v8 0/6] serial: 8250: serialize DMA cancellation with buffer reuse James Hilliard
@ 2026-10-29 7:36 ` James Hilliard
2026-10-29 7:41 ` sashiko-bot
2026-10-29 7:36 ` [PATCH v8 2/6] serial: serialize asynchronous write wakeups with the port lock James Hilliard
` (4 subsequent siblings)
5 siblings, 1 reply; 23+ messages in thread
From: James Hilliard @ 2026-10-29 7:36 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby, Raul E Rangel, Ilpo Järvinen,
Andy Shevchenko, Heikki Krogerus, Tony Lindgren,
Sebastian Andrzej Siewior, Richard Genoud, Nicolas Ferre,
Alexandre Belloni, Claudiu Beznea, Jacky Huang, Shan-Chun Hung,
Pali Rohár, Frank Li, Sascha Hauer, Pengutronix Kernel Team,
Fabio Estevam, Maciej W. Rozycki, Timur Tabi
Cc: linux-kernel, linux-serial, linux-arm-kernel, imx, linuxppc-dev,
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 | 19 +++++++++++++------
1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
index 4805b644ccb1..b6f108d34854 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,18 @@ 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)
+ return;
+ 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
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH v8 2/6] serial: serialize asynchronous write wakeups with the port lock
2026-10-29 7:36 [PATCH v8 0/6] serial: 8250: serialize DMA cancellation with buffer reuse James Hilliard
2026-10-29 7:36 ` [PATCH v8 1/6] serial: 8250_omap: separate locked TX DMA completion processing James Hilliard
@ 2026-10-29 7:36 ` James Hilliard
2026-10-29 7:49 ` sashiko-bot
2026-10-29 14:17 ` Andy Shevchenko
2026-10-29 7:36 ` [PATCH v8 3/6] serial: core: allow drivers to reserve flushed transmit storage James Hilliard
` (3 subsequent siblings)
5 siblings, 2 replies; 23+ messages in thread
From: James Hilliard @ 2026-10-29 7:36 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby, Raul E Rangel, Ilpo Järvinen,
Andy Shevchenko, Heikki Krogerus, Tony Lindgren,
Sebastian Andrzej Siewior, Richard Genoud, Nicolas Ferre,
Alexandre Belloni, Claudiu Beznea, Jacky Huang, Shan-Chun Hung,
Pali Rohár, Frank Li, Sascha Hauer, Pengutronix Kernel Team,
Fabio Estevam, Maciej W. Rozycki, Timur Tabi
Cc: linux-kernel, linux-serial, linux-arm-kernel, imx, linuxppc-dev,
James Hilliard
uart_write_wakeup() expects the UART port lock to be held. Several worker,
DMA completion and interrupt paths call it without that lock, including
indirect calls from uart_handle_cts_change() and the transmit helpers.
Take the port lock around these notifications or their transmit helper.
Keep sleeping SPI and regmap accesses outside the locked region, and keep
the FSL LPUART completion lock held through its wakeup. Acquire the port
lock after releasing private channel locks to preserve the ordering used
by UART callbacks. In the MUX timer, publish available FIFO space under
the port lock and keep the hardware FIFO-drain wait outside it.
Use IRQ-safe locking in the Atmel RX and TX tasklets and SB1250 termios
path, since their interrupt handlers now acquire the same port lock.
Preserve the caller's interrupt state when releasing those locks.
On SB1250, lock modem-state updates after reading and filtering the change
status, and wake modem-status waiters after unlocking. Take the TX lock
inside its service routine so RX-only interrupts need no acquisition.
Keep that lock across x_char handling, FIFO consumption and the decision
to mask TX interrupts: a writer must not enqueue data and enable TX between
the empty-FIFO check and the interrupt-mask write. Each TX invocation sends
at most one character.
Serialize CLPS711x TX servicing and its interrupt-enable state with the
port lock. Disable TX interrupts without waiting in stop_tx(), since a
threaded handler may need that same lock. Have an already queued handler
leave stopped output untouched, and drain it outside the lock during
shutdown before the transmit FIFO can be released.
SCCNXP handles interrupts under a controller lock which its UART callbacks
acquire with the port lock already held. Collect the ports needing wakeups
in an unsigned long bitmap while servicing the controller, then release
that lock before visiting the set bits, taking each port lock and
rechecking the wakeup threshold. Removal drains the IRQ or timer before
unregistering the ports.
This serializes write notifications with port-locked serial-core state
changes, allowing readiness and transmit-storage reservations to gate the
notification without a check-versus-shutdown race.
Cover DZ TX servicing and JSM Classic modem notifications as well. On
DECstation ZS, acquire the port lock before the SCC lock and notify writers
after releasing the SCC lock. Defer normal TX restart after console output
to a worker, since console output can already run under the port lock.
Check transmitter readiness before consuming another character and drain
the restart worker during shutdown and removal. Release Lantiq's hardware
lock through the transmit helper's completion hook before notifying the
line discipline.
Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
drivers/tty/serial/21285.c | 2 +
drivers/tty/serial/atmel_serial.c | 12 ++---
drivers/tty/serial/clps711x.c | 23 +++++++---
drivers/tty/serial/cpm_uart.c | 2 +
drivers/tty/serial/dz.c | 9 ++--
drivers/tty/serial/fsl_lpuart.c | 2 +-
drivers/tty/serial/jsm/jsm_cls.c | 6 ++-
drivers/tty/serial/jsm/jsm_neo.c | 9 ++--
drivers/tty/serial/lantiq.c | 12 +++--
drivers/tty/serial/ma35d1_serial.c | 6 ++-
drivers/tty/serial/max3100.c | 6 ++-
drivers/tty/serial/max310x.c | 9 ++--
drivers/tty/serial/mux.c | 13 ++++--
drivers/tty/serial/mvebu-uart.c | 2 +
drivers/tty/serial/mxs-auart.c | 2 +
drivers/tty/serial/sb1250-duart.c | 25 ++++++----
drivers/tty/serial/sccnxp.c | 45 +++++++++++++-----
drivers/tty/serial/ucc_uart.c | 6 ++-
drivers/tty/serial/zs.c | 94 ++++++++++++++++++++++++++++----------
drivers/tty/serial/zs.h | 3 ++
20 files changed, 200 insertions(+), 88 deletions(-)
diff --git a/drivers/tty/serial/21285.c b/drivers/tty/serial/21285.c
index 4de0c975ebdc..e4149aea4f66 100644
--- a/drivers/tty/serial/21285.c
+++ b/drivers/tty/serial/21285.c
@@ -157,6 +157,8 @@ static irqreturn_t serial21285_tx_chars(int irq, void *dev_id)
struct uart_port *port = dev_id;
u8 ch;
+ guard(uart_port_lock_irqsave)(port);
+
uart_port_tx_limited(port, ch, 256,
!(*CSR_UARTFLG & 0x20),
*CSR_UARTDR = ch,
diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index c3880259e3bc..0433bc70f279 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -1371,6 +1371,8 @@ atmel_handle_status(struct uart_port *port, unsigned int pending,
if (status_change & (ATMEL_US_RI | ATMEL_US_DSR
| ATMEL_US_DCD | ATMEL_US_CTS)) {
+ guard(uart_port_lock_irqsave)(port);
+
/* TODO: All reads to CSR will clear these interrupts! */
if (status_change & ATMEL_US_RI)
port->icount.rng++;
@@ -1702,10 +1704,9 @@ static void atmel_tasklet_rx_func(struct tasklet_struct *t)
tasklet_rx);
struct uart_port *port = &atmel_port->uart;
- /* The interrupt handler does not take the lock */
- uart_port_lock(port);
+ guard(uart_port_lock_irqsave)(port);
+
atmel_port->schedule_rx(port);
- uart_port_unlock(port);
}
static void atmel_tasklet_tx_func(struct tasklet_struct *t)
@@ -1714,10 +1715,9 @@ static void atmel_tasklet_tx_func(struct tasklet_struct *t)
tasklet_tx);
struct uart_port *port = &atmel_port->uart;
- /* The interrupt handler does not take the lock */
- uart_port_lock(port);
+ guard(uart_port_lock_irqsave)(port);
+
atmel_port->schedule_tx(port);
- uart_port_unlock(port);
}
static void atmel_init_property(struct atmel_uart_port *atmel_port,
diff --git a/drivers/tty/serial/clps711x.c b/drivers/tty/serial/clps711x.c
index 83186bf50002..06ed8ba7c9fb 100644
--- a/drivers/tty/serial/clps711x.c
+++ b/drivers/tty/serial/clps711x.c
@@ -73,7 +73,8 @@ static void uart_clps711x_stop_tx(struct uart_port *port)
struct clps711x_port *s = dev_get_drvdata(port->dev);
if (s->tx_enabled) {
- disable_irq(port->irq);
+ /* The handler also takes the port lock. */
+ disable_irq_nosync(port->irq);
s->tx_enabled = 0;
}
}
@@ -146,9 +147,16 @@ static irqreturn_t uart_clps711x_int_tx(int irq, void *dev_id)
{
struct uart_port *port = dev_id;
struct clps711x_port *s = dev_get_drvdata(port->dev);
- struct tty_port *tport = &port->state->port;
+ struct tty_port *tport;
unsigned char c;
+ guard(uart_port_lock_irqsave)(port);
+
+ if (!s->tx_enabled)
+ return IRQ_HANDLED;
+
+ tport = &port->state->port;
+
if (port->x_char) {
writew(port->x_char, port->membase + UARTDR_OFFSET);
port->icount.tx++;
@@ -157,10 +165,7 @@ static irqreturn_t uart_clps711x_int_tx(int irq, void *dev_id)
}
if (kfifo_is_empty(&tport->xmit_fifo) || uart_tx_stopped(port)) {
- if (s->tx_enabled) {
- disable_irq_nosync(port->irq);
- s->tx_enabled = 0;
- }
+ uart_clps711x_stop_tx(port);
return IRQ_HANDLED;
}
@@ -245,8 +250,14 @@ static void uart_clps711x_shutdown(struct uart_port *port)
{
struct clps711x_port *s = dev_get_drvdata(port->dev);
+ scoped_guard(uart_port_lock_irqsave, port)
+ uart_clps711x_stop_tx(port);
+
/* Disable the port */
regmap_update_bits(s->syscon, SYSCON_OFFSET, SYSCON_UARTEN, 0);
+
+ /* Drain the non-waiting TX disable before releasing the FIFO. */
+ synchronize_irq(port->irq);
}
static void uart_clps711x_set_termios(struct uart_port *port,
diff --git a/drivers/tty/serial/cpm_uart.c b/drivers/tty/serial/cpm_uart.c
index 39f54bb7b485..028a01fa02a9 100644
--- a/drivers/tty/serial/cpm_uart.c
+++ b/drivers/tty/serial/cpm_uart.c
@@ -214,6 +214,8 @@ static void cpm_uart_int_tx(struct uart_port *port)
{
pr_debug("CPM uart[%d]:TX INT\n", port->line);
+ guard(uart_port_lock_irqsave)(port);
+
cpm_uart_tx_pump(port);
}
diff --git a/drivers/tty/serial/dz.c b/drivers/tty/serial/dz.c
index 39d93e9c2d15..684691fed0f1 100644
--- a/drivers/tty/serial/dz.c
+++ b/drivers/tty/serial/dz.c
@@ -254,6 +254,8 @@ static inline void dz_transmit_chars(struct dz_mux *mux)
status = dz_in(dport, DZ_CSR);
dport = &mux->dport[LINE(status)];
+ guard(uart_port_lock_irqsave)(&dport->port);
+
tport = &dport->port.state->port;
if (dport->port.x_char) { /* XON/XOFF chars */
@@ -265,9 +267,7 @@ static inline void dz_transmit_chars(struct dz_mux *mux)
/* If nothing to do or stopped or hardware stopped. */
if (uart_tx_stopped(&dport->port) ||
!uart_fifo_get(&dport->port, &tmp)) {
- uart_port_lock(&dport->port);
dz_stop_tx(&dport->port);
- uart_port_unlock(&dport->port);
return;
}
@@ -281,11 +281,8 @@ static inline void dz_transmit_chars(struct dz_mux *mux)
uart_write_wakeup(&dport->port);
/* Are we are done. */
- if (kfifo_is_empty(&tport->xmit_fifo)) {
- uart_port_lock(&dport->port);
+ if (kfifo_is_empty(&tport->xmit_fifo))
dz_stop_tx(&dport->port);
- uart_port_unlock(&dport->port);
- }
}
/*
diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
index c8575c965203..cb04571c3d0e 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -540,10 +540,10 @@ static void lpuart_dma_tx_complete(void *arg)
uart_xmit_advance(&sport->port, sport->dma_tx_bytes);
sport->dma_tx_in_progress = false;
- uart_port_unlock_irqrestore(&sport->port, flags);
if (kfifo_len(&tport->xmit_fifo) < WAKEUP_CHARS)
uart_write_wakeup(&sport->port);
+ uart_port_unlock_irqrestore(&sport->port, flags);
if (waitqueue_active(&sport->dma_wait)) {
wake_up(&sport->dma_wait);
diff --git a/drivers/tty/serial/jsm/jsm_cls.c b/drivers/tty/serial/jsm/jsm_cls.c
index 6e40792f92cf..9224bb57eaf3 100644
--- a/drivers/tty/serial/jsm/jsm_cls.c
+++ b/drivers/tty/serial/jsm/jsm_cls.c
@@ -571,7 +571,8 @@ static inline void cls_parse_isr(struct jsm_board *brd, uint port)
spin_lock_irqsave(&ch->ch_lock, flags);
ch->ch_flags |= (CH_TX_FIFO_EMPTY | CH_TX_FIFO_LWM);
spin_unlock_irqrestore(&ch->ch_lock, flags);
- cls_copy_data_from_queue_to_uart(ch);
+ scoped_guard(uart_port_lock_irqsave, &ch->uart_port)
+ cls_copy_data_from_queue_to_uart(ch);
}
/*
@@ -581,7 +582,8 @@ static inline void cls_parse_isr(struct jsm_board *brd, uint port)
*/
/* Parse any modem signal changes */
- cls_parse_modem(ch, readb(&ch->ch_cls_uart->msr));
+ scoped_guard(uart_port_lock_irqsave, &ch->uart_port)
+ cls_parse_modem(ch, readb(&ch->ch_cls_uart->msr));
}
}
diff --git a/drivers/tty/serial/jsm/jsm_neo.c b/drivers/tty/serial/jsm/jsm_neo.c
index e8e13bf056e2..cbd9af43c68b 100644
--- a/drivers/tty/serial/jsm/jsm_neo.c
+++ b/drivers/tty/serial/jsm/jsm_neo.c
@@ -740,7 +740,8 @@ static void neo_parse_isr(struct jsm_board *brd, u32 port)
spin_lock_irqsave(&ch->ch_lock, lock_flags);
ch->ch_flags |= (CH_TX_FIFO_EMPTY | CH_TX_FIFO_LWM);
spin_unlock_irqrestore(&ch->ch_lock, lock_flags);
- neo_copy_data_from_queue_to_uart(ch);
+ scoped_guard(uart_port_lock_irqsave, &ch->uart_port)
+ neo_copy_data_from_queue_to_uart(ch);
}
if (isr & UART_17158_IIR_XONXOFF) {
@@ -895,7 +896,8 @@ static inline void neo_parse_lsr(struct jsm_board *brd, u32 port)
spin_unlock_irqrestore(&ch->ch_lock, lock_flags);
/* Transfer data (if any) from Write Queue -> UART. */
- neo_copy_data_from_queue_to_uart(ch);
+ scoped_guard(uart_port_lock_irqsave, &ch->uart_port)
+ neo_copy_data_from_queue_to_uart(ch);
}
else if (linestatus & UART_17158_TX_AND_FIFO_CLR) {
spin_lock_irqsave(&ch->ch_lock, lock_flags);
@@ -903,7 +905,8 @@ static inline void neo_parse_lsr(struct jsm_board *brd, u32 port)
spin_unlock_irqrestore(&ch->ch_lock, lock_flags);
/* Transfer data (if any) from Write Queue -> UART. */
- neo_copy_data_from_queue_to_uart(ch);
+ scoped_guard(uart_port_lock_irqsave, &ch->uart_port)
+ neo_copy_data_from_queue_to_uart(ch);
}
}
diff --git a/drivers/tty/serial/lantiq.c b/drivers/tty/serial/lantiq.c
index 62cd9e0bb377..ed1d31791402 100644
--- a/drivers/tty/serial/lantiq.c
+++ b/drivers/tty/serial/lantiq.c
@@ -153,11 +153,12 @@ lqasc_start_tx(struct uart_port *port)
struct ltq_uart_port *ltq_port = to_ltq_uart_port(port);
u8 ch;
+ /* Drop the hardware lock before the helper notifies the line discipline. */
spin_lock_irqsave(<q_port->lock, flags);
- uart_port_tx(port, ch,
- lqasc_tx_ready(port),
- writeb(ch, port->membase + LTQ_ASC_TBUF));
- spin_unlock_irqrestore(<q_port->lock, flags);
+ uart_port_tx_limited(port, ch, port->fifosize,
+ lqasc_tx_ready(port),
+ writeb(ch, port->membase + LTQ_ASC_TBUF),
+ spin_unlock_irqrestore(<q_port->lock, flags));
return;
}
@@ -239,7 +240,8 @@ lqasc_tx_int(int irq, void *_port)
spin_lock_irqsave(<q_port->lock, flags);
__raw_writel(ASC_IRNCR_TIR, port->membase + LTQ_ASC_IRNCR);
spin_unlock_irqrestore(<q_port->lock, flags);
- lqasc_start_tx(port);
+ scoped_guard(uart_port_lock_irqsave, port)
+ lqasc_start_tx(port);
return IRQ_HANDLED;
}
diff --git a/drivers/tty/serial/ma35d1_serial.c b/drivers/tty/serial/ma35d1_serial.c
index 3be30fb05513..ac44681bcb73 100644
--- a/drivers/tty/serial/ma35d1_serial.c
+++ b/drivers/tty/serial/ma35d1_serial.c
@@ -295,8 +295,10 @@ static irqreturn_t ma35d1serial_interrupt(int irq, void *dev_id)
if (isr & (MA35_ISR_RDA_IF | MA35_ISR_RXTO_IF))
receive_chars(up);
- if (isr & MA35_ISR_THRE_INT)
- transmit_chars(up);
+ if (isr & MA35_ISR_THRE_INT) {
+ scoped_guard(uart_port_lock_irqsave, port)
+ transmit_chars(up);
+ }
if (fsr & MA35_FSR_TX_OVER_IF)
serial_out(up, MA35_FSR_REG, MA35_FSR_TX_OVER_IF);
diff --git a/drivers/tty/serial/max3100.c b/drivers/tty/serial/max3100.c
index f858a6669011..5a8817eb82ad 100644
--- a/drivers/tty/serial/max3100.c
+++ b/drivers/tty/serial/max3100.c
@@ -287,8 +287,10 @@ static void max3100_work(struct work_struct *w)
tty_flip_buffer_push(&s->port.state->port);
rxchars = 0;
}
- if (kfifo_len(&tport->xmit_fifo) < WAKEUP_CHARS)
- uart_write_wakeup(&s->port);
+ scoped_guard(uart_port_lock_irqsave, &s->port) {
+ if (kfifo_len(&tport->xmit_fifo) < WAKEUP_CHARS)
+ uart_write_wakeup(&s->port);
+ }
} while (!s->force_end_work &&
!freezing(current) &&
diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index a0ed70185555..d9ce6ba53c7f 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -950,8 +950,10 @@ static void max310x_handle_tx(struct uart_port *port)
uart_xmit_advance(port, to_send);
}
- if (kfifo_len(&tport->xmit_fifo) < WAKEUP_CHARS)
- uart_write_wakeup(port);
+ scoped_guard(uart_port_lock_irqsave, port) {
+ if (kfifo_len(&tport->xmit_fifo) < WAKEUP_CHARS)
+ uart_write_wakeup(port);
+ }
}
/*
@@ -1043,7 +1045,8 @@ static irqreturn_t max310x_port_irq(struct max310x_port *s, int portno)
if (ists & MAX310X_IRQ_CTS_BIT) {
lsr = max310x_port_read(port, MAX310X_LSR_IRQSTS_REG);
- uart_handle_cts_change(port, lsr & MAX310X_LSR_CTS_BIT);
+ scoped_guard(uart_port_lock_irqsave, port)
+ uart_handle_cts_change(port, lsr & MAX310X_LSR_CTS_BIT);
}
if (rxlen)
max310x_handle_rx(port, rxlen);
diff --git a/drivers/tty/serial/mux.c b/drivers/tty/serial/mux.c
index 3a77a7e5c7bc..8be5b3031749 100644
--- a/drivers/tty/serial/mux.c
+++ b/drivers/tty/serial/mux.c
@@ -189,11 +189,14 @@ static void mux_write(struct uart_port *port)
{
u8 ch;
- uart_port_tx_limited(port, ch,
- port->fifosize - UART_GET_FIFO_CNT(port),
- true,
- UART_PUT_CHAR(port, ch),
- mux_tx_done(port));
+ scoped_guard(uart_port_lock_irqsave, port) {
+ uart_port_tx_limited(port, ch,
+ port->fifosize - UART_GET_FIFO_CNT(port),
+ true,
+ UART_PUT_CHAR(port, ch),
+ ({}));
+ }
+ mux_tx_done(port);
}
/**
diff --git a/drivers/tty/serial/mvebu-uart.c b/drivers/tty/serial/mvebu-uart.c
index 33ea496c93b2..7bc0d0d3760e 100644
--- a/drivers/tty/serial/mvebu-uart.c
+++ b/drivers/tty/serial/mvebu-uart.c
@@ -334,6 +334,8 @@ static void mvebu_uart_tx_chars(struct uart_port *port, unsigned int status)
{
u8 ch;
+ guard(uart_port_lock_irqsave)(port);
+
uart_port_tx_limited(port, ch, port->fifosize,
!(readl(port->membase + UART_STAT) & STAT_TX_FIFO_FUL),
writel(ch, port->membase + UART_TSH(port)),
diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
index 697318dbb146..6a310309b55d 100644
--- a/drivers/tty/serial/mxs-auart.c
+++ b/drivers/tty/serial/mxs-auart.c
@@ -521,6 +521,8 @@ static void dma_tx_callback(void *param)
dma_unmap_sg(s->dev, &s->tx_sgl, 1, DMA_TO_DEVICE);
+ guard(uart_port_lock_irqsave)(&s->port);
+
/* clear the bit used to serialize the DMA tx. */
clear_bit(MXS_AUART_DMA_TX_SYNC, &s->flags);
smp_mb__after_atomic();
diff --git a/drivers/tty/serial/sb1250-duart.c b/drivers/tty/serial/sb1250-duart.c
index b4e1b90e5960..ff3019e1631d 100644
--- a/drivers/tty/serial/sb1250-duart.c
+++ b/drivers/tty/serial/sb1250-duart.c
@@ -387,6 +387,9 @@ static void sbd_transmit_chars(struct sbd_port *sport)
unsigned int mask;
int stop_tx;
+ /* Serialize FIFO consumption and interrupt masking with start_tx(). */
+ guard(uart_port_lock_irqsave)(uport);
+
/* XON/XOFF chars. */
if (sport->port.x_char) {
write_sbdchn(sport, R_DUART_TX_HOLD, sport->port.x_char);
@@ -424,15 +427,19 @@ static void sbd_status_handle(struct sbd_port *sport)
delta = read_sbdshr(sport, R_DUART_INCHREG((uport->line) % 2));
delta >>= (uport->line) % 2;
- if (delta & (M_DUART_IN_PIN0_VAL << S_DUART_IN_PIN_CHNG))
- uart_handle_cts_change(uport, !(delta & M_DUART_IN_PIN0_VAL));
+ if (!(delta & ((M_DUART_IN_PIN2_VAL | M_DUART_IN_PIN0_VAL) <<
+ S_DUART_IN_PIN_CHNG)))
+ return;
- if (delta & (M_DUART_IN_PIN2_VAL << S_DUART_IN_PIN_CHNG))
- uport->icount.dsr++;
+ scoped_guard(uart_port_lock_irqsave, uport) {
+ if (delta & (M_DUART_IN_PIN0_VAL << S_DUART_IN_PIN_CHNG))
+ uart_handle_cts_change(uport, !(delta & M_DUART_IN_PIN0_VAL));
- if (delta & ((M_DUART_IN_PIN2_VAL | M_DUART_IN_PIN0_VAL) <<
- S_DUART_IN_PIN_CHNG))
- wake_up_interruptible(&uport->state->port.delta_msr_wait);
+ if (delta & (M_DUART_IN_PIN2_VAL << S_DUART_IN_PIN_CHNG))
+ uport->icount.dsr++;
+ }
+
+ wake_up_interruptible(&uport->state->port.delta_msr_wait);
}
static irqreturn_t sbd_interrupt(int irq, void *dev_id)
@@ -611,7 +618,7 @@ static void sbd_set_termios(struct uart_port *uport, struct ktermios *termios,
else
aux &= ~M_DUART_CTS_CHNG_ENA;
- uart_port_lock(uport);
+ guard(uart_port_lock_irqsave)(uport);
if (sport->tx_stopped)
command |= M_DUART_TX_DIS;
@@ -632,8 +639,6 @@ static void sbd_set_termios(struct uart_port *uport, struct ktermios *termios,
write_sbdchn(sport, R_DUART_AUXCTL_X, aux | oldaux);
write_sbdchn(sport, R_DUART_CMD, command);
-
- uart_port_unlock(uport);
}
diff --git a/drivers/tty/serial/sccnxp.c b/drivers/tty/serial/sccnxp.c
index cb31e3aceb62..50845e64d633 100644
--- a/drivers/tty/serial/sccnxp.c
+++ b/drivers/tty/serial/sccnxp.c
@@ -10,6 +10,7 @@
#include <linux/clk.h>
#include <linux/delay.h>
#include <linux/err.h>
+#include <linux/find.h>
#include <linux/module.h>
#include <linux/device.h>
#include <linux/console.h>
@@ -435,7 +436,7 @@ static void sccnxp_handle_rx(struct uart_port *port)
tty_flip_buffer_push(&port->state->port);
}
-static void sccnxp_handle_tx(struct uart_port *port)
+static bool sccnxp_handle_tx(struct uart_port *port)
{
u8 sr;
struct tty_port *tport = &port->state->port;
@@ -445,7 +446,7 @@ static void sccnxp_handle_tx(struct uart_port *port)
sccnxp_port_write(port, SCCNXP_THR_REG, port->x_char);
port->icount.tx++;
port->x_char = 0;
- return;
+ return false;
}
if (kfifo_is_empty(&tport->xmit_fifo) || uart_tx_stopped(port)) {
@@ -457,7 +458,7 @@ static void sccnxp_handle_tx(struct uart_port *port)
if (s->chip->flags & SCCNXP_HAVE_IO)
sccnxp_set_bit(port, DIR_OP, 0);
}
- return;
+ return false;
}
while (1) {
@@ -473,13 +474,13 @@ static void sccnxp_handle_tx(struct uart_port *port)
sccnxp_port_write(port, SCCNXP_THR_REG, ch);
}
- if (kfifo_len(&tport->xmit_fifo) < WAKEUP_CHARS)
- uart_write_wakeup(port);
+ return kfifo_len(&tport->xmit_fifo) < WAKEUP_CHARS;
}
-static void sccnxp_handle_events(struct sccnxp_port *s)
+static unsigned long sccnxp_handle_events(struct sccnxp_port *s)
{
- int i;
+ unsigned long wakeup = 0;
+ unsigned int i;
u8 isr;
do {
@@ -491,20 +492,40 @@ static void sccnxp_handle_events(struct sccnxp_port *s)
for (i = 0; i < s->uart.nr; i++) {
if (s->opened[i] && (isr & ISR_RXRDY(i)))
sccnxp_handle_rx(&s->port[i]);
- if (s->opened[i] && (isr & ISR_TXRDY(i)))
- sccnxp_handle_tx(&s->port[i]);
+ if (s->opened[i] && (isr & ISR_TXRDY(i)) &&
+ sccnxp_handle_tx(&s->port[i]))
+ wakeup |= BIT(i);
}
} while (1);
+
+ return wakeup;
+}
+
+static void sccnxp_write_wakeup(struct sccnxp_port *s, unsigned long wakeup)
+{
+ unsigned int i;
+
+ /* UART callbacks take s->lock with the port lock already held. */
+ for_each_set_bit(i, &wakeup, s->uart.nr) {
+ struct uart_port *port = &s->port[i];
+
+ scoped_guard(uart_port_lock_irqsave, port) {
+ if (kfifo_len(&port->state->port.xmit_fifo) < WAKEUP_CHARS)
+ uart_write_wakeup(port);
+ }
+ }
}
static void sccnxp_timer(struct timer_list *t)
{
struct sccnxp_port *s = timer_container_of(s, t, timer);
unsigned long flags;
+ unsigned long wakeup;
spin_lock_irqsave(&s->lock, flags);
- sccnxp_handle_events(s);
+ wakeup = sccnxp_handle_events(s);
spin_unlock_irqrestore(&s->lock, flags);
+ sccnxp_write_wakeup(s, wakeup);
mod_timer(&s->timer, jiffies + usecs_to_jiffies(s->pdata.poll_time_us));
}
@@ -513,10 +534,12 @@ static irqreturn_t sccnxp_ist(int irq, void *dev_id)
{
struct sccnxp_port *s = (struct sccnxp_port *)dev_id;
unsigned long flags;
+ unsigned long wakeup;
spin_lock_irqsave(&s->lock, flags);
- sccnxp_handle_events(s);
+ wakeup = sccnxp_handle_events(s);
spin_unlock_irqrestore(&s->lock, flags);
+ sccnxp_write_wakeup(s, wakeup);
return IRQ_HANDLED;
}
diff --git a/drivers/tty/serial/ucc_uart.c b/drivers/tty/serial/ucc_uart.c
index 6b7dcbbacd13..60f72c19d67c 100644
--- a/drivers/tty/serial/ucc_uart.c
+++ b/drivers/tty/serial/ucc_uart.c
@@ -570,8 +570,10 @@ static irqreturn_t qe_uart_int(int irq, void *data)
if (events & UCC_UART_UCCE_RX)
qe_uart_int_rx(qe_port);
- if (events & UCC_UART_UCCE_TX)
- qe_uart_tx_pump(qe_port);
+ if (events & UCC_UART_UCCE_TX) {
+ scoped_guard(uart_port_lock_irqsave, &qe_port->port)
+ qe_uart_tx_pump(qe_port);
+ }
return events ? IRQ_HANDLED : IRQ_NONE;
}
diff --git a/drivers/tty/serial/zs.c b/drivers/tty/serial/zs.c
index 8f92b4129a38..f1c9fec6d513 100644
--- a/drivers/tty/serial/zs.c
+++ b/drivers/tty/serial/zs.c
@@ -431,20 +431,23 @@ static void zs_stop_tx(struct uart_port *uport)
spin_unlock(&scc->zlock);
}
-static void zs_raw_transmit_chars(struct zs_port *);
+static bool zs_raw_transmit_chars(struct zs_port *);
static void zs_start_tx(struct uart_port *uport)
{
struct zs_port *zport = to_zport(uport);
struct zs_scc *scc = zport->scc;
+ bool wakeup = false;
spin_lock(&scc->zlock);
if (zport->tx_stopped) {
zs_transmit_drain(zport, 0);
zport->tx_stopped = 0;
- zs_raw_transmit_chars(zport);
+ wakeup = zs_raw_transmit_chars(zport);
}
spin_unlock(&scc->zlock);
+ if (wakeup)
+ uart_write_wakeup(uport);
}
static void zs_stop_rx(struct uart_port *uport)
@@ -603,51 +606,81 @@ static void zs_receive_chars(struct zs_port *zport)
tty_flip_buffer_push(&uport->state->port);
}
-static void zs_raw_transmit_chars(struct zs_port *zport)
+static bool zs_raw_transmit_chars(struct zs_port *zport)
{
struct tty_port *tport = &zport->port.state->port;
unsigned char ch;
+ bool wakeup;
+
+ /* A deferred console restart can race a pending TX interrupt. */
+ if (!(read_zsreg(zport, R0) & Tx_BUF_EMP))
+ return false;
/* XON/XOFF chars. */
if (zport->port.x_char) {
write_zsdata(zport, zport->port.x_char);
zport->port.icount.tx++;
zport->port.x_char = 0;
- return;
+ return false;
}
/* If nothing to do or stopped or hardware stopped. */
if (uart_tx_stopped(&zport->port) ||
!uart_fifo_get(&zport->port, &ch)) {
zs_raw_stop_tx(zport);
- return;
+ return false;
}
/* Send char. */
write_zsdata(zport, ch);
- if (kfifo_len(&tport->xmit_fifo) < WAKEUP_CHARS)
- uart_write_wakeup(&zport->port);
+ wakeup = kfifo_len(&tport->xmit_fifo) < WAKEUP_CHARS;
/* Are we are done? */
if (kfifo_is_empty(&tport->xmit_fifo))
zs_raw_stop_tx(zport);
+
+ return wakeup;
}
static void zs_transmit_chars(struct zs_port *zport)
{
struct zs_scc *scc = zport->scc;
+ bool wakeup;
+
+ guard(uart_port_lock_irqsave)(&zport->port);
spin_lock(&scc->zlock);
- zs_raw_transmit_chars(zport);
+ wakeup = zs_raw_transmit_chars(zport);
spin_unlock(&scc->zlock);
+ if (wakeup)
+ uart_write_wakeup(&zport->port);
+}
+
+static void zs_tx_work(struct work_struct *work)
+{
+ struct zs_port *zport = container_of(work, struct zs_port, tx_work);
+ struct uart_port *uport = &zport->port;
+ bool wakeup = false;
+
+ guard(uart_port_lock_irqsave)(uport);
+
+ if (!uport->state || !uport->state->port.xmit_buf)
+ return;
+
+ scoped_guard(spinlock, &zport->scc->zlock) {
+ if (!zport->tx_stopped)
+ wakeup = zs_raw_transmit_chars(zport);
+ }
+ if (wakeup)
+ uart_write_wakeup(uport);
}
static void zs_status_handle(struct zs_port *zport, struct zs_port *zport_a)
{
struct uart_port *uport = &zport->port;
struct zs_scc *scc = zport->scc;
- unsigned int delta;
+ unsigned int delta, mctrl;
u8 status, brk;
spin_lock(&scc->zlock);
@@ -670,18 +703,19 @@ static void zs_status_handle(struct zs_port *zport, struct zs_port *zport_a)
if (zport != zport_a) {
delta = zs_raw_xor_mctrl(zport);
+ mctrl = zport->mctrl;
spin_unlock(&scc->zlock);
- if (delta & TIOCM_CTS)
- uart_handle_cts_change(uport,
- zport->mctrl & TIOCM_CTS);
- if (delta & TIOCM_CAR)
- uart_handle_dcd_change(uport,
- zport->mctrl & TIOCM_CAR);
- if (delta & TIOCM_RNG)
- uport->icount.rng++;
- if (delta & TIOCM_DSR)
- uport->icount.dsr++;
+ scoped_guard(uart_port_lock_irqsave, uport) {
+ if (delta & TIOCM_CTS)
+ uart_handle_cts_change(uport, mctrl & TIOCM_CTS);
+ if (delta & TIOCM_CAR)
+ uart_handle_dcd_change(uport, mctrl & TIOCM_CAR);
+ if (delta & TIOCM_RNG)
+ uport->icount.rng++;
+ if (delta & TIOCM_DSR)
+ uport->icount.dsr++;
+ }
if (delta)
wake_up_interruptible(&uport->state->port.delta_msr_wait);
@@ -809,13 +843,19 @@ static void zs_shutdown(struct uart_port *uport)
unsigned long flags;
int irq_guard;
- spin_lock_irqsave(&scc->zlock, flags);
+ uart_port_lock_irqsave(uport, &flags);
+ spin_lock(&scc->zlock);
+ zs_raw_stop_tx(zport);
+ zport->regs[1] &= ~TxINT_ENAB;
+ write_zsreg(zport, R1, zport->regs[1]);
zport->regs[3] &= ~RxENABLE;
write_zsreg(zport, R5, zport->regs[5]);
write_zsreg(zport, R3, zport->regs[3]);
- spin_unlock_irqrestore(&scc->zlock, flags);
+ spin_unlock(&scc->zlock);
+ uart_port_unlock_irqrestore(uport, flags);
+ cancel_work_sync(&zport->tx_work);
irq_guard = atomic_add_return(-1, &scc->irq_guard);
if (!irq_guard)
@@ -1066,6 +1106,7 @@ static int __init zs_probe(struct platform_device *pdev)
zport->scc = &zs_sccs[chip];
zport->clk_mode = 16;
+ INIT_WORK(&zport->tx_work, zs_tx_work);
uport->dev = &pdev->dev;
uport->has_sysrq = IS_ENABLED(CONFIG_SERIAL_ZS_CONSOLE);
@@ -1098,8 +1139,10 @@ static void __exit zs_remove(struct platform_device *pdev)
struct zs_port *zport = &zs_sccs[chip].zport[side];
struct uart_port *uport = &zport->port;
- if (uport->dev)
+ if (uport->dev) {
uart_remove_one_port(&zs_reg, uport);
+ cancel_work_sync(&zport->tx_work);
+ }
}
}
@@ -1161,9 +1204,12 @@ static void zs_console_write(struct console *co, const char *s,
zport->regs[1] |= TxINT_ENAB;
write_zsreg(zport, R1, zport->regs[1]);
- /* Resume any transmission as the TxIP bit won't be set. */
+ /*
+ * TxIP won't be set. Defer the restart: console output may be
+ * called with the port lock held, and TX must acquire it first.
+ */
if (!zport->tx_stopped)
- zs_raw_transmit_chars(zport);
+ schedule_work(&zport->tx_work);
}
spin_unlock_irqrestore(&scc->zlock, flags);
}
diff --git a/drivers/tty/serial/zs.h b/drivers/tty/serial/zs.h
index e0d3c189b33f..0404294e4533 100644
--- a/drivers/tty/serial/zs.h
+++ b/drivers/tty/serial/zs.h
@@ -14,6 +14,8 @@
#ifdef __KERNEL__
+#include <linux/workqueue.h>
+
#define ZS_NUM_REGS 16
/*
@@ -22,6 +24,7 @@
struct zs_port {
struct zs_scc *scc; /* Containing SCC. */
struct uart_port port; /* Underlying UART. */
+ struct work_struct tx_work; /* Restart after console output. */
int clk_mode; /* May be 1, 16, 32, or 64. */
--
2.53.0
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v8 2/6] serial: serialize asynchronous write wakeups with the port lock
2026-10-29 7:36 ` [PATCH v8 2/6] serial: serialize asynchronous write wakeups with the port lock James Hilliard
@ 2026-10-29 7:49 ` sashiko-bot
2026-10-29 14:17 ` Andy Shevchenko
1 sibling, 0 replies; 23+ messages in thread
From: sashiko-bot @ 2026-10-29 7:49 UTC (permalink / raw)
To: James Hilliard; +Cc: linux-serial
> uart_write_wakeup() expects the UART port lock to be held. Several worker,
> DMA completion and interrupt paths call it without that lock, including
> indirect calls from uart_handle_cts_change() and the transmit helpers.
>
> Take the port lock around these notifications or their transmit helper.
> Keep sleeping SPI and regmap accesses outside the locked region, and keep
> the FSL LPUART completion lock held through its wakeup. Acquire the port
> lock after releasing private channel locks to preserve the ordering used
> by UART callbacks. In the MUX timer, publish available FIFO space under
> the port lock and keep the hardware FIFO-drain wait outside it.
>
> Use IRQ-safe locking in the Atmel RX and TX tasklets and SB1250 termios
> path, since their interrupt handlers now acquire the same port lock.
> Preserve the caller's interrupt state when releasing those locks.
>
> [ ... ]
> the restart worker during shutdown and removal. Release Lantiq's hardware
> lock through the transmit helper's completion hook before notifying the
> line discipline.
>
> 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/20261029-submit-serial-8250-dma-cancel-v1-v8-0-3fd56ba352ed@gmail.com?part=2
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v8 2/6] serial: serialize asynchronous write wakeups with the port lock
2026-10-29 7:36 ` [PATCH v8 2/6] serial: serialize asynchronous write wakeups with the port lock James Hilliard
2026-10-29 7:49 ` sashiko-bot
@ 2026-10-29 14:17 ` Andy Shevchenko
2026-10-29 17:13 ` James Hilliard
1 sibling, 1 reply; 23+ messages in thread
From: Andy Shevchenko @ 2026-10-29 14:17 UTC (permalink / raw)
To: James Hilliard
Cc: Greg Kroah-Hartman, Jiri Slaby, Raul E Rangel, Ilpo Järvinen,
Heikki Krogerus, Tony Lindgren, Sebastian Andrzej Siewior,
Richard Genoud, Nicolas Ferre, Alexandre Belloni, Claudiu Beznea,
Jacky Huang, Shan-Chun Hung, Pali Rohár, Frank Li,
Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
Maciej W. Rozycki, Timur Tabi, linux-kernel, linux-serial,
linux-arm-kernel, imx, linuxppc-dev
On Thu, Oct 29, 2026 at 01:36:58AM -0600, James Hilliard wrote:
> uart_write_wakeup() expects the UART port lock to be held. Several worker,
> DMA completion and interrupt paths call it without that lock, including
> indirect calls from uart_handle_cts_change() and the transmit helpers.
>
> Take the port lock around these notifications or their transmit helper.
> Keep sleeping SPI and regmap accesses outside the locked region, and keep
> the FSL LPUART completion lock held through its wakeup. Acquire the port
> lock after releasing private channel locks to preserve the ordering used
> by UART callbacks. In the MUX timer, publish available FIFO space under
> the port lock and keep the hardware FIFO-drain wait outside it.
>
> Use IRQ-safe locking in the Atmel RX and TX tasklets and SB1250 termios
> path, since their interrupt handlers now acquire the same port lock.
> Preserve the caller's interrupt state when releasing those locks.
>
> On SB1250, lock modem-state updates after reading and filtering the change
> status, and wake modem-status waiters after unlocking. Take the TX lock
> inside its service routine so RX-only interrupts need no acquisition.
> Keep that lock across x_char handling, FIFO consumption and the decision
> to mask TX interrupts: a writer must not enqueue data and enable TX between
> the empty-FIFO check and the interrupt-mask write. Each TX invocation sends
> at most one character.
>
> Serialize CLPS711x TX servicing and its interrupt-enable state with the
> port lock. Disable TX interrupts without waiting in stop_tx(), since a
> threaded handler may need that same lock. Have an already queued handler
> leave stopped output untouched, and drain it outside the lock during
> shutdown before the transmit FIFO can be released.
>
> SCCNXP handles interrupts under a controller lock which its UART callbacks
> acquire with the port lock already held. Collect the ports needing wakeups
> in an unsigned long bitmap while servicing the controller, then release
> that lock before visiting the set bits, taking each port lock and
> rechecking the wakeup threshold. Removal drains the IRQ or timer before
> unregistering the ports.
>
> This serializes write notifications with port-locked serial-core state
> changes, allowing readiness and transmit-storage reservations to gate the
> notification without a check-versus-shutdown race.
>
> Cover DZ TX servicing and JSM Classic modem notifications as well. On
> DECstation ZS, acquire the port lock before the SCC lock and notify writers
> after releasing the SCC lock. Defer normal TX restart after console output
> to a worker, since console output can already run under the port lock.
> Check transmitter readiness before consuming another character and drain
> the restart worker during shutdown and removal. Release Lantiq's hardware
> lock through the transmit helper's completion hook before notifying the
> line discipline.
...
> static irqreturn_t serial21285_tx_chars(int irq, void *dev_id)
> struct uart_port *port = dev_id;
> u8 ch;
>
> + guard(uart_port_lock_irqsave)(port);
Why _irqsave?
> uart_port_tx_limited(port, ch, 256,
> !(*CSR_UARTFLG & 0x20),
> *CSR_UARTDR = ch,
...
Same Q everywhere (it might be legit in some cases,
it might be too strong in others).
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v8 2/6] serial: serialize asynchronous write wakeups with the port lock
2026-10-29 14:17 ` Andy Shevchenko
@ 2026-10-29 17:13 ` James Hilliard
0 siblings, 0 replies; 23+ messages in thread
From: James Hilliard @ 2026-10-29 17:13 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Greg Kroah-Hartman, Jiri Slaby, Raul E Rangel, Ilpo Järvinen,
Heikki Krogerus, Tony Lindgren, Sebastian Andrzej Siewior,
Richard Genoud, Nicolas Ferre, Alexandre Belloni, Claudiu Beznea,
Jacky Huang, Shan-Chun Hung, Pali Rohár, Frank Li,
Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
Maciej W. Rozycki, Timur Tabi, linux-kernel, linux-serial,
linux-arm-kernel, imx, linuxppc-dev
On Thu, Oct 29, 2026 at 8:18 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Oct 29, 2026 at 01:36:58AM -0600, James Hilliard wrote:
> > uart_write_wakeup() expects the UART port lock to be held. Several worker,
> > DMA completion and interrupt paths call it without that lock, including
> > indirect calls from uart_handle_cts_change() and the transmit helpers.
> >
> > Take the port lock around these notifications or their transmit helper.
> > Keep sleeping SPI and regmap accesses outside the locked region, and keep
> > the FSL LPUART completion lock held through its wakeup. Acquire the port
> > lock after releasing private channel locks to preserve the ordering used
> > by UART callbacks. In the MUX timer, publish available FIFO space under
> > the port lock and keep the hardware FIFO-drain wait outside it.
> >
> > Use IRQ-safe locking in the Atmel RX and TX tasklets and SB1250 termios
> > path, since their interrupt handlers now acquire the same port lock.
> > Preserve the caller's interrupt state when releasing those locks.
> >
> > On SB1250, lock modem-state updates after reading and filtering the change
> > status, and wake modem-status waiters after unlocking. Take the TX lock
> > inside its service routine so RX-only interrupts need no acquisition.
> > Keep that lock across x_char handling, FIFO consumption and the decision
> > to mask TX interrupts: a writer must not enqueue data and enable TX between
> > the empty-FIFO check and the interrupt-mask write. Each TX invocation sends
> > at most one character.
> >
> > Serialize CLPS711x TX servicing and its interrupt-enable state with the
> > port lock. Disable TX interrupts without waiting in stop_tx(), since a
> > threaded handler may need that same lock. Have an already queued handler
> > leave stopped output untouched, and drain it outside the lock during
> > shutdown before the transmit FIFO can be released.
> >
> > SCCNXP handles interrupts under a controller lock which its UART callbacks
> > acquire with the port lock already held. Collect the ports needing wakeups
> > in an unsigned long bitmap while servicing the controller, then release
> > that lock before visiting the set bits, taking each port lock and
> > rechecking the wakeup threshold. Removal drains the IRQ or timer before
> > unregistering the ports.
> >
> > This serializes write notifications with port-locked serial-core state
> > changes, allowing readiness and transmit-storage reservations to gate the
> > notification without a check-versus-shutdown race.
> >
> > Cover DZ TX servicing and JSM Classic modem notifications as well. On
> > DECstation ZS, acquire the port lock before the SCC lock and notify writers
> > after releasing the SCC lock. Defer normal TX restart after console output
> > to a worker, since console output can already run under the port lock.
> > Check transmitter readiness before consuming another character and drain
> > the restart worker during shutdown and removal. Release Lantiq's hardware
> > lock through the transmit helper's completion hook before notifying the
> > line discipline.
>
> ...
>
> > static irqreturn_t serial21285_tx_chars(int irq, void *dev_id)
>
> > struct uart_port *port = dev_id;
> > u8 ch;
> >
> > + guard(uart_port_lock_irqsave)(port);
>
> Why _irqsave?
For serial21285_tx_chars(), a plain uart_port_lock guard is sufficient.
It is only registered as an IRQ handler; saving and restoring the IRQ
state adds no protection on the hard-IRQ path. The necessary change
here is acquiring the port lock.
>
> > uart_port_tx_limited(port, ch, 256,
> > !(*CSR_UARTFLG & 0x20),
> > *CSR_UARTDR = ch,
>
> ...
>
> Same Q everywhere (it might be legit in some cases,
> it might be too strong in others).
The other sites have different calling contexts. Workers and tasklets
can share the lock with a hard IRQ, and some helpers have callers with
different incoming IRQ states. Those need different treatment from
hard-IRQ-only handlers.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v8 3/6] serial: core: allow drivers to reserve flushed transmit storage
2026-10-29 7:36 [PATCH v8 0/6] serial: 8250: serialize DMA cancellation with buffer reuse James Hilliard
2026-10-29 7:36 ` [PATCH v8 1/6] serial: 8250_omap: separate locked TX DMA completion processing James Hilliard
2026-10-29 7:36 ` [PATCH v8 2/6] serial: serialize asynchronous write wakeups with the port lock James Hilliard
@ 2026-10-29 7:36 ` James Hilliard
2026-10-29 8:06 ` sashiko-bot
2026-10-29 7:37 ` [PATCH v8 4/6] serial: 8250: drain canceled TX DMA before reusing its buffer James Hilliard
` (2 subsequent siblings)
5 siblings, 1 reply; 23+ messages in thread
From: James Hilliard @ 2026-10-29 7:36 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby, Raul E Rangel, Ilpo Järvinen,
Andy Shevchenko, Heikki Krogerus, Tony Lindgren,
Sebastian Andrzej Siewior, Richard Genoud, Nicolas Ferre,
Alexandre Belloni, Claudiu Beznea, Jacky Huang, Shan-Chun Hung,
Pali Rohár, Frank Li, Sascha Hauer, Pengutronix Kernel Team,
Fabio Estevam, Maciej W. Rozycki, Timur Tabi
Cc: linux-kernel, linux-serial, linux-arm-kernel, imx, linuxppc-dev,
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. Defer write-ready notifications until the
reservation is released, preserving one-shot TTY wakeups. Keep the flush
wakeup under the port lock so it cannot race another reservation.
Suppress ordinary TX starts while the FIFO storage is blocked. Preserve
high-priority flow-control characters, which use separate storage.
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.
Track port readiness separately from DMA ownership. Retirement releases the
reservation even during startup or shutdown, while writes and wakeups stay
gated until successful startup or resume. Reopening or restarting a port
wakes deferred writers with either a new or retained FIFO. Restart queued
output when reusing a FIFO after suspension. Keep its write-ready
notification pending until the retained output drains below the normal
wakeup threshold.
Publish resume readiness under the same port lock as the TX restart,
without clearing any reservation. A concurrent flush can then reserve a
new transfer without resume releasing its storage prematurely.
Assert the port-lock requirement at uart_write_wakeup(), alongside the
reservation helpers, so callers cannot inspect readiness or release a
one-shot notification concurrently with a reservation change.
Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
drivers/tty/serial/serial_core.c | 103 +++++++++++++++++++++++++++++++++++----
include/linux/serial_core.h | 18 ++++++-
2 files changed, 110 insertions(+), 11 deletions(-)
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index c0deec0e6af4..11bd1260cdc6 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -100,6 +100,11 @@ static inline struct uart_port *uart_port_check(struct uart_state *state)
return state->uart_port;
}
+static bool uart_xmit_writable(const struct uart_state *state)
+{
+ return state->xmit_ready && !state->xmit_blocked;
+}
+
/**
* uart_write_wakeup - schedule write processing
* @port: port to be processed
@@ -114,15 +119,79 @@ static inline struct uart_port *uart_port_check(struct uart_state *state)
void uart_write_wakeup(struct uart_port *port)
{
struct uart_state *state = port->state;
+
+ lockdep_assert_held_once(&port->lock);
+
/*
* This means you called this function _after_ the port was
* closed. No cookie for you.
*/
BUG_ON(!state);
+ if (!uart_xmit_writable(state))
+ return;
tty_port_tty_wakeup(&state->port);
}
EXPORT_SYMBOL(uart_write_wakeup);
+/* Publish readiness without releasing any transfer's storage reservation. */
+static void uart_xmit_enable(struct uart_port *port)
+{
+ struct uart_state *state = port->state;
+
+ lockdep_assert_held_once(&port->lock);
+ if (state->xmit_ready)
+ return;
+ state->xmit_ready = true;
+ /* Retained output must drain before consuming a write-ready notification. */
+ if (kfifo_len(&state->port.xmit_fifo) < WAKEUP_CHARS)
+ uart_write_wakeup(port);
+}
+
+/**
+ * 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.
+ * Write wakeups are deferred until the reservation is released.
+ *
+ * The driver must unblock after retiring the transfer, including on shutdown.
+ * Serial core separately gates writes and wakeups until startup or resume
+ * has made the port ready.
+ * 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.
+ * Retirement is recorded even while the port is inactive. Writes and wakeups
+ * remain deferred until startup or resume has made the port ready.
+ *
+ * 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;
@@ -141,7 +210,10 @@ static void __uart_start(struct uart_state *state)
struct serial_port_device *port_dev;
int err;
- if (!port || port->flags & UPF_DEAD || uart_tx_stopped(port))
+ if (!port || port->flags & UPF_DEAD || uart_tx_stopped(port) || !state->xmit_ready)
+ return;
+ /* High-priority flow-control characters have their own storage. */
+ if (state->xmit_blocked && !port->x_char)
return;
port_dev = port->port_dev;
@@ -375,6 +447,11 @@ static int uart_startup(struct tty_struct *tty, struct uart_state *state,
return -EIO;
serial_base_port_startup(uport);
+ scoped_guard(uart_port_lock_irq, uport) {
+ uart_xmit_enable(uport);
+ if (!kfifo_is_empty(&port->xmit_fifo) || uport->x_char)
+ __uart_start(state);
+ }
return 0;
}
@@ -397,8 +474,11 @@ static void uart_shutdown(struct tty_struct *tty, struct uart_state *state)
if (tty)
set_bit(TTY_IO_ERROR, &tty->flags);
- if (uport)
+ if (uport) {
+ scoped_guard(uart_port_lock_irq, uport)
+ state->xmit_ready = false;
serial_base_port_shutdown(uport);
+ }
if (tty_port_initialized(port)) {
tty_port_set_initialized(port, false);
@@ -593,7 +673,7 @@ static int uart_put_char(struct tty_struct *tty, u8 c)
return 0;
}
- if (port)
+ if (port && uart_xmit_writable(state))
ret = kfifo_put(&state->port.xmit_fifo, c);
uart_port_unlock_deref(port, flags);
return ret;
@@ -624,7 +704,7 @@ static ssize_t uart_write(struct tty_struct *tty, const u8 *buf, size_t count)
return 0;
}
- if (port)
+ if (port && uart_xmit_writable(state))
ret = kfifo_in(&state->port.xmit_fifo, buf, count);
__uart_start(state);
@@ -640,7 +720,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 || !uart_xmit_writable(state))
ret = 0;
else
ret = kfifo_avail(&state->port.xmit_fifo);
@@ -682,8 +762,9 @@ static void uart_flush_buffer(struct tty_struct *tty)
kfifo_reset(&state->port.xmit_fifo);
if (port->ops->flush_buffer)
port->ops->flush_buffer(port);
+ /* Serialize the wakeup with reservation changes, including another flush. */
+ uart_write_wakeup(port);
uart_port_unlock_deref(port, flags);
- tty_port_tty_wakeup(&state->port);
}
/*
@@ -1748,8 +1829,10 @@ static void uart_tty_port_shutdown(struct tty_port *port)
if (WARN(!uport, "detached port still initialized!\n"))
return;
- scoped_guard(uart_port_lock_irq, uport)
+ scoped_guard(uart_port_lock_irq, uport) {
+ state->xmit_ready = false;
uport->ops->stop_rx(uport);
+ }
serial_base_port_shutdown(uport);
uart_port_shutdown(port);
@@ -2391,9 +2474,10 @@ int uart_suspend_port(struct uart_driver *drv, struct uart_port *uport)
unsigned int mctrl;
tty_port_set_suspended(port, true);
- tty_port_set_initialized(port, false);
scoped_guard(uart_port_lock_irq, uport) {
+ state->xmit_ready = false;
+ tty_port_set_initialized(port, false);
ops->stop_tx(uport);
if (!(uport->rs485.flags & SER_RS485_ENABLED))
ops->set_mctrl(uport, 0);
@@ -2498,8 +2582,9 @@ int uart_resume_port(struct uart_driver *drv, struct uart_port *uport)
if (!(uport->rs485.flags & SER_RS485_ENABLED))
ops->set_mctrl(uport, uport->mctrl);
ops->start_tx(uport);
+ tty_port_set_initialized(port, true);
+ uart_xmit_enable(uport);
}
- tty_port_set_initialized(port, true);
} else {
/*
* Failed to resume - maybe hardware went away?
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 4eeb8253752a..41598a38a408 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -208,8 +208,17 @@ 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. Serial core separately
+ * gates writes and wakeups until startup or resume has made the port ready.
*
* Locking: @port->lock taken.
* Interrupts: locally disabled.
@@ -834,6 +843,9 @@ enum uart_pm_state {
*/
struct uart_state {
struct tty_port port;
+ /* TX readiness and storage ownership; protected by the UART port lock. */
+ bool xmit_ready;
+ bool xmit_blocked;
enum uart_pm_state pm_state;
@@ -909,6 +921,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
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v8 3/6] serial: core: allow drivers to reserve flushed transmit storage
2026-10-29 7:36 ` [PATCH v8 3/6] serial: core: allow drivers to reserve flushed transmit storage James Hilliard
@ 2026-10-29 8:06 ` sashiko-bot
0 siblings, 0 replies; 23+ messages in thread
From: sashiko-bot @ 2026-10-29 8:06 UTC (permalink / raw)
To: James Hilliard; +Cc: linux-serial, imx, Frank.Li
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] serial: core: lockdep assertion failure in uart_write_wakeup() via asc_startup()
- [High] serial: core: lost tty->write_wait wakeup on flush when xmit is not writable
--
commit 887d77aad417709eb7acabd98bee111266577f06
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 serial drivers
can reserve flushed transmit FIFO storage until asynchronous DMA transfers
have retired. It also gates writes and write-ready wakeups on port readiness
and storage reservation state under the UART port lock.
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index c0deec0e6af41..11bd1260cdc63 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -100,6 +100,11 @@ static inline struct uart_port *uart_port_check(struct uart_state *state)
> return state->uart_port;
> }
>
> +static bool uart_xmit_writable(const struct uart_state *state)
> +{
> + return state->xmit_ready && !state->xmit_blocked;
> +}
> +
> /**
> * uart_write_wakeup - schedule write processing
> * @port: port to be processed
> @@ -114,15 +119,79 @@ static inline struct uart_port *uart_port_check(struct uart_state *state)
> void uart_write_wakeup(struct uart_port *port)
> {
> struct uart_state *state = port->state;
> +
> + lockdep_assert_held_once(&port->lock);
[Severity: High]
Can this trigger a lockdep warning when opening or resuming an ST ASC serial
port?
During port startup or resume, uart_port_startup() and uart_resume_port()
invoke uport->ops->startup(uport) with port->mutex held, but without
port->lock held.
In drivers/tty/serial/st-asc.c, asc_startup() calls asc_transmit_chars()
without holding port->lock:
drivers/tty/serial/st-asc.c:asc_startup() {
...
asc_transmit_chars(port);
asc_enable_rx_interrupts(port);
return 0;
}
asc_transmit_chars() calls uart_port_tx_limited(), which expands
__uart_port_tx() in include/linux/serial_core.h:
pending = kfifo_len(&__tport->xmit_fifo);
if (pending < WAKEUP_CHARS) {
uart_write_wakeup(__port);
Because pending is below WAKEUP_CHARS on startup, __uart_port_tx() calls
uart_write_wakeup() without port->lock held, hitting the new
lockdep_assert_held_once(&port->lock) assertion and reading
state->xmit_ready and state->xmit_blocked unlocked.
[ ... ]
> @@ -682,8 +762,9 @@ static void uart_flush_buffer(struct tty_struct *tty)
> kfifo_reset(&state->port.xmit_fifo);
> if (port->ops->flush_buffer)
> port->ops->flush_buffer(port);
> + /* Serialize the wakeup with reservation changes, including another flush. */
> + uart_write_wakeup(port);
> uart_port_unlock_deref(port, flags);
> - tty_port_tty_wakeup(&state->port);
> }
[Severity: High]
Could replacing tty_port_tty_wakeup(&state->port) with uart_write_wakeup(port)
here cause threads waiting for the transmit buffer to drain in
tty_wait_until_sent() or set_termios() to miss their wakeup and hang?
Waiters in tty_wait_until_sent() and set_termios() sleep on tty->write_wait
until !tty_chars_in_buffer(tty), which checks kfifo_len(&state->port.xmit_fifo).
Previously, uart_flush_buffer() emptied xmit_fifo via kfifo_reset() and
unconditionally called tty_port_tty_wakeup(&state->port), which woke
tty->write_wait.
With uart_write_wakeup(port), the wakeup is skipped whenever
!uart_xmit_writable(state) (when state->xmit_ready is false or
state->xmit_blocked is true):
1. If uart_flush_buffer() runs while the port is suspended
(state->xmit_ready is false), kfifo_reset() empties xmit_fifo so
tty_chars_in_buffer(tty) becomes 0, but uart_write_wakeup() returns early
without waking tty->write_wait.
2. If port->ops->flush_buffer(port) calls uart_xmit_block(port),
state->xmit_blocked is set to true and uart_write_wakeup() suppresses the
wakeup in uart_flush_buffer(). If the port is subsequently shut down or
suspended before the async transfer retires (via uart_shutdown(),
uart_tty_port_shutdown(), or uart_suspend_port()), state->xmit_ready is set
to false before uport->ops->shutdown(uport) runs:
drivers/tty/serial/serial_core.c:uart_shutdown() {
...
if (uport) {
scoped_guard(uart_port_lock_irq, uport)
state->xmit_ready = false;
serial_base_port_shutdown(uport);
}
...
if (tty_port_initialized(port)) {
...
uart_port_shutdown(port);
}
}
When uport->ops->shutdown(uport) then retires the transfer and calls
uart_xmit_unblock(uport), uart_write_wakeup(uport) sees state->xmit_ready
is false and returns early again, permanently losing the deferred wakeup
for any drain waiters on tty->write_wait.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261029-submit-serial-8250-dma-cancel-v1-v8-0-3fd56ba352ed@gmail.com?part=3
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v8 4/6] serial: 8250: drain canceled TX DMA before reusing its buffer
2026-10-29 7:36 [PATCH v8 0/6] serial: 8250: serialize DMA cancellation with buffer reuse James Hilliard
` (2 preceding siblings ...)
2026-10-29 7:36 ` [PATCH v8 3/6] serial: core: allow drivers to reserve flushed transmit storage James Hilliard
@ 2026-10-29 7:37 ` James Hilliard
2026-10-29 7:51 ` sashiko-bot
2026-10-29 14:19 ` Andy Shevchenko
2026-10-29 7:37 ` [PATCH v8 5/6] serial: 8250: retire canceled RX DMA before reusing the receive buffer James Hilliard
2026-10-29 7:37 ` [PATCH v8 6/6] serial: 8250: retain DMA resources when termination fails James Hilliard
5 siblings, 2 replies; 23+ messages in thread
From: James Hilliard @ 2026-10-29 7:37 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby, Raul E Rangel, Ilpo Järvinen,
Andy Shevchenko, Heikki Krogerus, Tony Lindgren,
Sebastian Andrzej Siewior, Richard Genoud, Nicolas Ferre,
Alexandre Belloni, Claudiu Beznea, Jacky Huang, Shan-Chun Hung,
Pali Rohár, Frank Li, Sascha Hauer, Pengutronix Kernel Team,
Fabio Estevam, Maciej W. Rozycki, Timur Tabi
Cc: linux-kernel, linux-serial, linux-arm-kernel, imx, linuxppc-dev,
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 a detached
callback has not yet run. 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 FIFO reservation after
stopping DMA and unmapping its source. Serial core keeps writes and wakeups
gated until startup or resume makes the port ready again.
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.
Retain the tx_running check in both completion callbacks. Releasing the
reservation relies on the DMA provider draining detached callbacks when
dmaengine_synchronize() returns; provider callback-lifetime fixes supply
that guarantee for controllers which previously lacked it.
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 | 20 +++++-
drivers/tty/serial/8250/8250_dma.c | 130 ++++++++++++++++++++++++++----------
drivers/tty/serial/8250/8250_omap.c | 21 ++++--
3 files changed, 125 insertions(+), 46 deletions(-)
diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index 974a47f24084..9f01fa36fa3c 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;
@@ -52,7 +57,9 @@ struct uart_8250_dma {
unsigned char tx_err;
unsigned char rx_running;
/* Protected by the UART port lock. */
- bool shutting_down;
+ bool shutting_down:1;
+ bool tx_terminating:1;
+ bool tx_flushed:1;
};
struct old_serial_port {
@@ -400,8 +407,15 @@ static inline int is_omap1510_8250(struct uart_8250_port *pt)
#endif
#ifdef CONFIG_SERIAL_8250_DMA
+static inline bool serial8250_tx_dma_blocked(struct uart_8250_dma *dma)
+{
+ return dma->shutting_down || dma->tx_terminating;
+}
+
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 +448,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 || serial8250_tx_dma_blocked(dma))
return;
dmaengine_pause(dma->txchan);
@@ -444,7 +458,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 || serial8250_tx_dma_blocked(dma))
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..344f0dd24b41 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,57 @@ 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 (serial8250_tx_dma_blocked(dma) || !dma->tx_running)
+ return;
+ 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)
+ return;
+ 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;
@@ -101,7 +146,7 @@ int serial8250_tx_dma(struct uart_8250_port *p)
int i;
int ret;
- if (dma->shutting_down)
+ if (serial8250_tx_dma_blocked(dma))
return 0;
if (dma->tx_running) {
@@ -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 || serial8250_tx_dma_blocked(dma))
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,28 @@ 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);
- scoped_guard(uart_port_lock_irqsave, &p->port)
+ scoped_guard(uart_port_lock_irqsave, &p->port) {
+ dma->tx_terminating = false;
+ dma->tx_flushed = false;
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 +394,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;
@@ -356,7 +410,11 @@ void serial8250_release_dma(struct uart_8250_port *p)
UART_XMIT_SIZE, DMA_TO_DEVICE);
dma_release_channel(dma->txchan);
dma->txchan = NULL;
- dma->tx_running = 0;
+ scoped_guard(uart_port_lock_irqsave, &p->port) {
+ dma->tx_running = 0;
+ dma->tx_terminating = false;
+ uart_xmit_unblock(&p->port);
+ }
dev_dbg_ratelimited(p->port.dev, "dma channels released\n");
}
diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
index b6f108d34854..13507409a5b6 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 (serial8250_tx_dma_blocked(dma) || !dma->tx_running)
return;
omap_8250_tx_dma_complete(p);
}
@@ -1140,7 +1145,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 (serial8250_tx_dma_blocked(dma) || dma->tx_running)
return 0;
if (uart_tx_stopped(&p->port) || kfifo_is_empty(&tport->xmit_fifo)) {
@@ -1199,24 +1204,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
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v8 4/6] serial: 8250: drain canceled TX DMA before reusing its buffer
2026-10-29 7:37 ` [PATCH v8 4/6] serial: 8250: drain canceled TX DMA before reusing its buffer James Hilliard
@ 2026-10-29 7:51 ` sashiko-bot
2026-10-29 14:19 ` Andy Shevchenko
1 sibling, 0 replies; 23+ messages in thread
From: sashiko-bot @ 2026-10-29 7:51 UTC (permalink / raw)
To: James Hilliard; +Cc: linux-serial
> 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 a detached
> callback has not yet run. 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
> [ ... ]
> dmaengine_synchronize() returns; provider callback-lifetime fixes supply
> that guarantee for controllers which previously lacked it.
>
> 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/20261029-submit-serial-8250-dma-cancel-v1-v8-0-3fd56ba352ed@gmail.com?part=4
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v8 4/6] serial: 8250: drain canceled TX DMA before reusing its buffer
2026-10-29 7:37 ` [PATCH v8 4/6] serial: 8250: drain canceled TX DMA before reusing its buffer James Hilliard
2026-10-29 7:51 ` sashiko-bot
@ 2026-10-29 14:19 ` Andy Shevchenko
2026-10-29 17:15 ` James Hilliard
1 sibling, 1 reply; 23+ messages in thread
From: Andy Shevchenko @ 2026-10-29 14:19 UTC (permalink / raw)
To: James Hilliard
Cc: Greg Kroah-Hartman, Jiri Slaby, Raul E Rangel, Ilpo Järvinen,
Heikki Krogerus, Tony Lindgren, Sebastian Andrzej Siewior,
Richard Genoud, Nicolas Ferre, Alexandre Belloni, Claudiu Beznea,
Jacky Huang, Shan-Chun Hung, Pali Rohár, Frank Li,
Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
Maciej W. Rozycki, Timur Tabi, linux-kernel, linux-serial,
linux-arm-kernel, imx, linuxppc-dev
On Thu, Oct 29, 2026 at 01:37:00AM -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 a detached
> callback has not yet run. 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 FIFO reservation after
> stopping DMA and unmapping its source. Serial core keeps writes and wakeups
> gated until startup or resume makes the port ready again.
>
> 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.
>
> Retain the tx_running check in both completion callbacks. Releasing the
> reservation relies on the DMA provider draining detached callbacks when
> dmaengine_synchronize() returns; provider callback-lifetime fixes supply
> that guarantee for controllers which previously lacked it.
...
> - bool shutting_down;
> + bool shutting_down:1;
> + bool tx_terminating:1;
> + bool tx_flushed:1;
What is the meaning of bool XXX:1? Have you checked with `pahole` the result?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v8 4/6] serial: 8250: drain canceled TX DMA before reusing its buffer
2026-10-29 14:19 ` Andy Shevchenko
@ 2026-10-29 17:15 ` James Hilliard
0 siblings, 0 replies; 23+ messages in thread
From: James Hilliard @ 2026-10-29 17:15 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Greg Kroah-Hartman, Jiri Slaby, Raul E Rangel, Ilpo Järvinen,
Heikki Krogerus, Tony Lindgren, Sebastian Andrzej Siewior,
Richard Genoud, Nicolas Ferre, Alexandre Belloni, Claudiu Beznea,
Jacky Huang, Shan-Chun Hung, Pali Rohár, Frank Li,
Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
Maciej W. Rozycki, Timur Tabi, linux-kernel, linux-serial,
linux-arm-kernel, imx, linuxppc-dev
On Thu, Oct 29, 2026 at 8:19 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Oct 29, 2026 at 01:37:00AM -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 a detached
> > callback has not yet run. 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 FIFO reservation after
> > stopping DMA and unmapping its source. Serial core keeps writes and wakeups
> > gated until startup or resume makes the port ready again.
> >
> > 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.
> >
> > Retain the tx_running check in both completion callbacks. Releasing the
> > reservation relies on the DMA provider draining detached callbacks when
> > dmaengine_synchronize() returns; provider callback-lifetime fixes supply
> > that guarantee for controllers which previously lacked it.
>
> ...
>
> > - bool shutting_down;
> > + bool shutting_down:1;
> > + bool tx_terminating:1;
> > + bool tx_flushed:1;
>
> What is the meaning of bool XXX:1? Have you checked with `pahole` the result?
These are one-bit boolean fields, intended to pack the state flags
into one byte.
I've now compared layout probes using the kernel type definitions and
saved build flags, changing only those bitfields to ordinary bools.
For the complete v8 structure, including the RX flags, pahole reports:
Bitfields Plain bool
ARM32 / Clang 212 220
ARM64 / GCC 368 376
ARM64 / GCC / PREEMPT_RT 368 376
The sizes are in bytes. All six flags share one byte in the packed
version. This saves eight bytes in each checked configuration.
The packing is a layout choice, not an atomicity mechanism; updates
still require the UART port lock. Given the measured saving, I'd keep
it unless there is another concern with using bool bitfields here.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v8 5/6] serial: 8250: retire canceled RX DMA before reusing the receive buffer
2026-10-29 7:36 [PATCH v8 0/6] serial: 8250: serialize DMA cancellation with buffer reuse James Hilliard
` (3 preceding siblings ...)
2026-10-29 7:37 ` [PATCH v8 4/6] serial: 8250: drain canceled TX DMA before reusing its buffer James Hilliard
@ 2026-10-29 7:37 ` James Hilliard
2026-10-29 7:51 ` sashiko-bot
` (2 more replies)
2026-10-29 7:37 ` [PATCH v8 6/6] serial: 8250: retain DMA resources when termination fails James Hilliard
5 siblings, 3 replies; 23+ messages in thread
From: James Hilliard @ 2026-10-29 7:37 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby, Raul E Rangel, Ilpo Järvinen,
Andy Shevchenko, Heikki Krogerus, Tony Lindgren,
Sebastian Andrzej Siewior, Richard Genoud, Nicolas Ferre,
Alexandre Belloni, Claudiu Beznea, Jacky Huang, Shan-Chun Hung,
Pali Rohár, Frank Li, Sascha Hauer, Pengutronix Kernel Team,
Fabio Estevam, Maciej W. Rozycki, Timur Tabi
Cc: linux-kernel, linux-serial, linux-arm-kernel, imx, linuxppc-dev,
James Hilliard
The generic RX timeout path pauses a transfer, copies its data and
terminates it asynchronously. Clearing rx_running at that point lets a
new receive reuse the buffer and cookie while the old callback has not
yet run. 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. Keep FIFO access separate from callback retirement: after a
successful pause and termination, clear rx_running while rx_terminating
prevents another descriptor from reusing the buffer. Deliver the paused
data before allowing the interrupt handler to drain the FIFO through PIO.
Normal retirement only releases ownership and leaves subsequent receive
handling to the UART interrupt.
Preserve the normal completion-path LSR read and gate DMA resubmission on
the receive interrupt enable, so stop_rx() prevents another transfer while
the status read retains its existing side effects.
Reject new flush work and PIO receive handling during shutdown, including
late Intel MID DMA timeouts. Stop and drain retirement work before freeing
the channels and buffer. Once RX has stopped, allow the idle flush used by
DesignWare LCR recovery during shutdown. Clear the shutdown guard before
startup configures the UART and tests its interrupts.
Check pause failures before accessing the buffer. A successful pause is
the quiescence guarantee, even when a provider still reports IN_PROGRESS.
Also accept DMA_COMPLETE after a failed pause: completion can stop the
channel before its callback acquires the port lock, leaving nothing for
the provider to pause. Retire that completed transfer normally.
A failed pause with an incomplete cookie is ambiguous: hardware may have
finished while its completion interrupt is still pending. Canceling that
descriptor can discard the entire completed receive buffer and switch
permanently to PIO. Retain the transfer and retry pause and status in the
worker, with receive interrupts masked to prevent a timeout interrupt
storm. The worker copies only after a successful pause or reported
completion, then terminates and drains the old callback before reuse.
The pending-pause guard keeps callbacks from copying or resubmitting the
transfer concurrently. Retries sleep outside the port lock and stop when
shutdown takes over. A provider which never pauses or completes retains
RX ownership until an error or shutdown; a timeout alone cannot establish
that discarding the buffer is safe.
On DMA_ERROR or termination failure, let the worker stop DMA before
handing the FIFO to PIO. Keep the requested enables in the existing IER
shadow and filter hardware writes during pause retries and recovery,
including termios, RS485 and overrun-backoff changes. Apply the current
enables after retirement so stop_rx() and shutdown remain effective.
Hold a runtime PM reference around worker register accesses and keep RX
on PIO until reopen after a confirmed error.
Abort the DesignWare idle sequence when RX cannot be stopped. Allow
the empty-FIFO timeout workaround once DMA has released the FIFO, including
while callbacks retire. Exclude FIFO resets during RX DMA recovery. If the
provider still cannot terminate the channel in process context, retain
its buffer and leave receive interrupts masked.
For OMAP, publish the shutdown gate under the port lock before the final
RX flush and interrupt masking. Take the RX DMA lock inside the port lock
in the completion callback, and serialize the runtime-suspend flush with
the same order. Drain the UART IRQ before releasing DMA state so an
in-flight handler cannot retain up->dma across its release.
Fixes: 9ee4b83e51f7 ("serial: 8250: Add support for dmaengine")
Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
drivers/tty/serial/8250/8250.h | 42 +++++++-
drivers/tty/serial/8250/8250_core.c | 2 +-
drivers/tty/serial/8250/8250_dma.c | 197 +++++++++++++++++++++++++++++-------
drivers/tty/serial/8250/8250_dw.c | 28 +++--
drivers/tty/serial/8250/8250_omap.c | 28 +++--
drivers/tty/serial/8250/8250_port.c | 31 ++++--
6 files changed, 262 insertions(+), 66 deletions(-)
diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index 9f01fa36fa3c..d51a0aa0aae8 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 */
@@ -55,11 +56,16 @@ struct uart_8250_dma {
unsigned char tx_running;
unsigned char tx_err;
+ /* RX DMA may access the FIFO; independent of callback retirement. */
unsigned char rx_running;
/* Protected by the UART port lock. */
bool shutting_down:1;
bool tx_terminating:1;
bool tx_flushed:1;
+ bool rx_terminating:1;
+ /* Retry pause without discarding a completion awaiting its DMA IRQ. */
+ bool rx_pause_pending:1;
+ bool rx_err:1;
};
struct old_serial_port {
@@ -148,6 +154,29 @@ static inline void serial_out(struct uart_8250_port *up, int offset, int value)
up->port.serial_out(&up->port, offset, value);
}
+static inline bool serial8250_rx_dma_running(struct uart_8250_port *up)
+{
+ return up->dma && up->dma->rx_running;
+}
+
+static inline bool serial8250_rx_dma_recovering(struct uart_8250_port *up)
+{
+ return up->dma && (up->dma->rx_pause_pending ||
+ (up->dma->rx_err && up->dma->rx_terminating));
+}
+
+/* Keep the desired enables in up->ier while RX DMA recovery masks hardware. */
+static inline void serial8250_out_IER(struct uart_8250_port *up, u8 ier)
+{
+ lockdep_assert_held_once(&up->port.lock);
+
+ if (up->dma && up->dma->shutting_down)
+ ier = 0;
+ else if (serial8250_rx_dma_recovering(up))
+ ier &= ~(UART_IER_RDI | UART_IER_RLSI | UART_IER_RTOIE);
+ serial_out(up, UART_IER, ier);
+}
+
/**
* serial_lsr_in - Read LSR register and preserve flags across reads
* @up: uart 8250 port
@@ -221,7 +250,7 @@ static inline bool serial8250_set_THRI(struct uart_8250_port *up)
if (up->ier & UART_IER_THRI)
return false;
up->ier |= UART_IER_THRI;
- serial_out(up, UART_IER, up->ier);
+ serial8250_out_IER(up, up->ier);
return true;
}
@@ -233,7 +262,7 @@ static inline bool serial8250_clear_THRI(struct uart_8250_port *up)
if (!(up->ier & UART_IER_THRI))
return false;
up->ier &= ~UART_IER_THRI;
- serial_out(up, UART_IER, up->ier);
+ serial8250_out_IER(up, up->ier);
return true;
}
@@ -417,7 +446,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 *);
@@ -473,7 +502,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_core.c b/drivers/tty/serial/8250/8250_core.c
index 42319843cb5b..ca06288038ed 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -738,7 +738,7 @@ static void serial_8250_overrun_backoff_work(struct work_struct *work)
guard(uart_port_lock_irqsave)(&up->port);
up->ier |= UART_IER_RLSI | UART_IER_RDI;
- serial_out(up, UART_IER, up->ier);
+ serial8250_out_IER(up, up->ier);
}
static int __serial8250_register_8250_port(struct uart_8250_port *uart,
diff --git a/drivers/tty/serial/8250/8250_dma.c b/drivers/tty/serial/8250/8250_dma.c
index 344f0dd24b41..f980b4faf6a3 100644
--- a/drivers/tty/serial/8250/8250_dma.c
+++ b/drivers/tty/serial/8250/8250_dma.c
@@ -8,6 +8,7 @@
#include <linux/tty_flip.h>
#include <linux/serial_reg.h>
#include <linux/dma-mapping.h>
+#include <linux/delay.h>
#include "8250.h"
@@ -86,53 +87,153 @@ 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_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_out_IER(p, p->ier);
+}
+
+/* Try to obtain a stable residue before retiring the current descriptor. */
+static int serial8250_rx_dma_try_flush(struct uart_8250_port *p)
+{
+ struct uart_8250_dma *dma = p->dma;
+ struct dma_tx_state state;
+ enum dma_status status;
+ int ret;
+
+ lockdep_assert_held_once(&p->port.lock);
+
+ ret = dmaengine_pause(dma->rxchan);
+ status = dmaengine_tx_status(dma->rxchan, dma->rx_cookie, &state);
+ if (status == DMA_ERROR) {
+ ret = -EIO;
+ goto recover;
+ }
+ if (ret && status != DMA_COMPLETE) {
+ /*
+ * Hardware may be idle before the provider publishes completion.
+ * Leave the descriptor alive and retry outside interrupt context;
+ * neither copying its buffer nor canceling it is safe yet.
+ */
+ dma->rx_pause_pending = true;
+ serial8250_out_IER(p, p->ier);
+ return ret;
+ }
+ /* 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;
+
+ /* DMA released the FIFO; retain the buffer until callbacks retire. */
+ dma->rx_running = 0;
+ return 0;
+
+recover:
+ serial8250_rx_dma_recover(p);
+ return ret;
+}
+
static void dma_rx_complete(void *param)
{
struct uart_8250_port *p = param;
struct uart_8250_dma *dma = p->dma;
- unsigned long flags;
+ struct dma_tx_state state;
+ enum dma_status status;
- uart_port_lock_irqsave(&p->port, &flags);
- if (dma->shutting_down)
- goto out;
- if (dma->rx_running)
- __dma_rx_complete(p);
+ guard(uart_port_lock_irqsave)(&p->port);
- /*
- * Cannot be combined with the previous check because __dma_rx_complete()
- * changes dma->rx_running.
- */
- if (!dma->rx_running && (serial_lsr_in(p) & UART_LSR_DR))
- p->dma->rx_dma(p);
-out:
- uart_port_unlock_irqrestore(&p->port, flags);
+ if (dma->shutting_down || dma->rx_terminating || dma->rx_pause_pending)
+ return;
+ if (dma->rx_running) {
+ status = dmaengine_tx_status(dma->rxchan, dma->rx_cookie, &state);
+ if (status == DMA_ERROR) {
+ serial8250_rx_dma_recover(p);
+ schedule_work(&dma->rx_flush_work);
+ return;
+ }
+ if (status != DMA_COMPLETE)
+ return;
+ serial8250_rx_dma_copy(p, &state);
+ dma->rx_running = 0;
+ }
+
+ /* Preserve LSR read side effects when stop_rx() has disabled resubmission. */
+ if ((serial_lsr_in(p) & UART_LSR_DR) && (p->ier & UART_IER_RDI))
+ dma->rx_dma(p);
+}
+
+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;
+ bool recover, retry, restore_rx;
+ int ret;
+
+ guard(serial8250_rpm)(p);
+
+ for (;;) {
+ scoped_guard(uart_port_lock_irqsave, &p->port) {
+ /* Shutdown cancels this work and drains both DMA channels. */
+ if (dma->shutting_down)
+ return;
+ if (dma->rx_pause_pending && !dma->rx_terminating)
+ serial8250_rx_dma_try_flush(p);
+ retry = dma->rx_pause_pending && !dma->rx_terminating;
+ recover = dma->rx_err;
+ }
+ if (!retry)
+ break;
+ /* Let the DMA IRQ publish completion or a pending pause settle. */
+ usleep_range(1000, 2000);
+ }
+
+ if (recover) {
+ 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);
+ }
+
+ scoped_guard(uart_port_lock_irqsave, &p->port) {
+ restore_rx = recover || dma->rx_pause_pending;
+ dma->rx_pause_pending = false;
+ dma->rx_terminating = false;
+ dma->rx_running = 0;
+ if (dma->shutting_down)
+ return;
+
+ if (!restore_rx)
+ return;
+
+ /* Apply the current enables, including any intervening stop_rx(). */
+ serial8250_out_IER(p, p->ier);
+ }
+ if (recover)
+ dev_warn(p->port.dev, "RX DMA failed, using PIO\n");
}
int serial8250_tx_dma(struct uart_8250_port *p)
@@ -242,6 +343,9 @@ int serial8250_rx_dma(struct uart_8250_port *p)
if (dma->shutting_down || dma->rx_running)
return 0;
+ /* A retiring transfer has released the FIFO, but not its buffer. */
+ if (dma->rx_err || dma->rx_terminating)
+ return -EIO;
serial8250_do_prepare_rx_dma(p);
@@ -262,16 +366,26 @@ 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;
+ 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->rx_running)
+ return 0;
+ /* A late interrupt must not hand a still-active receiver to PIO. */
+ if (dma->shutting_down)
+ return -ESHUTDOWN;
+ if (dma->rx_terminating)
+ return -EIO;
+ if (dma->rx_pause_pending)
+ return -EAGAIN;
+
+ ret = serial8250_rx_dma_try_flush(p);
+ schedule_work(&dma->rx_flush_work);
+ return ret;
}
EXPORT_SYMBOL_GPL(serial8250_rx_dma_flush);
@@ -368,10 +482,14 @@ 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);
scoped_guard(uart_port_lock_irqsave, &p->port) {
dma->tx_terminating = false;
dma->tx_flushed = false;
+ dma->rx_terminating = false;
+ dma->rx_pause_pending = false;
+ dma->rx_err = false;
dma->shutting_down = false;
}
dev_dbg_ratelimited(p->port.dev, "got both dma channels\n");
@@ -395,10 +513,15 @@ 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);
- dma->rx_running = 0;
+ scoped_guard(uart_port_lock_irqsave, &p->port) {
+ dma->rx_running = 0;
+ dma->rx_terminating = false;
+ dma->rx_pause_pending = false;
+ }
dma_free_coherent(dmaengine_get_dma_device(dma->rxchan), dma->rx_size,
dma->rx_buf, dma->rx_addr);
dma_release_channel(dma->rxchan);
diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 9cbe076463e0..050a1c23d9f0 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -112,7 +112,7 @@ static void dw8250_idle_exit(struct uart_port *p)
if (up->capabilities & UART_CAP_FIFO)
serial_port_out(p, UART_FCR, up->fcr);
serial_port_out(p, UART_MCR, up->mcr);
- serial_port_out(p, UART_IER, up->ier);
+ serial8250_out_IER(up, up->ier);
/* DMA Rx is restarted by IRQ handler as needed. */
if (up->dma)
@@ -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) {
+ serial8250_out_IER(up, up->ier);
+ d->in_idle = 0;
+ return ret;
+ }
if (serial8250_tx_dma_running(up))
serial8250_tx_dma_pause(up);
}
@@ -390,9 +396,9 @@ static void dw8250_quirk_ier_kick(struct uart_port *p)
if (!(lsr & UART_LSR_THRE))
return;
- serial_port_out(p, UART_IER, up->ier | UART_IER_THRI);
+ serial8250_out_IER(up, up->ier | UART_IER_THRI);
serial_port_in(p, UART_LCR); /* safe, no side-effects */
- serial_port_out(p, UART_IER, up->ier);
+ serial8250_out_IER(up, up->ier);
}
static int dw8250_handle_irq(struct uart_port *p)
@@ -429,6 +435,9 @@ static int dw8250_handle_irq(struct uart_port *p)
d->no_int_count = 0;
+ if (up->dma && up->dma->shutting_down)
+ return 1;
+
/*
* There are ways to get Designware-based UARTs into a state where
* they are asserting UART_IIR_RX_TIMEOUT but there is no actual
@@ -436,10 +445,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) &&
+ !serial8250_rx_dma_running(up) && rx_timeout) {
status = serial_lsr_in(up);
/*
@@ -454,7 +464,9 @@ static int dw8250_handle_irq(struct uart_port *p)
}
/* Manually stop the Rx DMA transfer when acting as flow controller */
- if (quirks & DW_UART_QUIRK_IS_DMA_FC && up->dma && up->dma->rx_running && rx_timeout) {
+ if (quirks & DW_UART_QUIRK_IS_DMA_FC && rx_timeout &&
+ serial8250_rx_dma_running(up) && !up->dma->rx_terminating &&
+ !up->dma->rx_pause_pending) {
status = serial_lsr_in(up);
if (status & (UART_LSR_DR | UART_LSR_BI)) {
diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
index 13507409a5b6..9211ccda316a 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -771,8 +771,6 @@ static void omap_8250_shutdown(struct uart_port *port)
guard(serial8250_rpm)(up);
flush_work(&priv->qos_work);
- if (up->dma)
- omap_8250_rx_dma_flush(up);
serial_out(up, UART_OMAP_WER, 0);
if (priv->habit & UART_HAS_EFR2)
@@ -780,13 +778,16 @@ 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)
+ if (up->dma) {
up->dma->shutting_down = true;
+ omap_8250_rx_dma_flush(up);
+ }
up->ier = 0;
serial_out(up, UART_IER, 0);
}
- disable_irq_nosync(port->irq);
+ /* An in-flight handler may still be using up->dma. */
+ disable_irq(port->irq);
serial8250_release_dma(up);
up->dma = NULL;
@@ -898,7 +899,7 @@ static int omap8250_rs485_config(struct uart_port *port,
#ifdef CONFIG_SERIAL_8250_DMA
static int omap_8250_rx_dma(struct uart_8250_port *p);
-/* Must be called while priv->rx_dma_lock is held */
+/* Must be called with the port lock and priv->rx_dma_lock held. */
static void __dma_rx_do_complete(struct uart_8250_port *p)
{
struct uart_8250_dma *dma = p->dma;
@@ -911,6 +912,9 @@ static void __dma_rx_do_complete(struct uart_8250_port *p)
int ret;
u32 reg;
+ lockdep_assert_held_once(&p->port.lock);
+ lockdep_assert_held_once(&priv->rx_dma_lock);
+
if (!dma->rx_running)
goto out;
@@ -977,7 +981,8 @@ static void __dma_rx_complete(void *param)
if (dmaengine_tx_status(dma->rxchan, dma->rx_cookie, &state) != DMA_COMPLETE)
return;
- __dma_rx_do_complete(p);
+ scoped_guard(spinlock, &priv->rx_dma_lock)
+ __dma_rx_do_complete(p);
if (priv->throttled)
return;
@@ -995,6 +1000,8 @@ static void omap_8250_rx_dma_flush(struct uart_8250_port *p)
unsigned long flags;
int ret;
+ lockdep_assert_held_once(&p->port.lock);
+
spin_lock_irqsave(&priv->rx_dma_lock, flags);
if (!dma->rx_running) {
@@ -1329,6 +1336,11 @@ static int omap_8250_dma_handle_irq(struct uart_port *port)
uart_port_lock(port);
+ if (up->dma->shutting_down) {
+ uart_port_unlock(port);
+ return IRQ_HANDLED;
+ }
+
status = serial_port_in(port, UART_LSR);
if ((iir & 0x3f) != UART_IIR_THRI) {
@@ -1818,8 +1830,10 @@ static int omap8250_runtime_suspend(struct device *dev)
}
}
- if (up && up->dma && up->dma->rxchan)
+ if (up && up->dma && up->dma->rxchan) {
+ guard(uart_port_lock_irqsave)(&up->port);
omap_8250_rx_dma_flush(up);
+ }
priv->latency = PM_QOS_CPU_LATENCY_DEFAULT_VALUE;
schedule_work(&priv->qos_work);
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index cfbd154bf5f9..51ecaacf9097 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1306,7 +1306,7 @@ static void serial8250_stop_rx(struct uart_port *port)
guard(serial8250_rpm)(up);
up->ier &= ~(UART_IER_RLSI | UART_IER_RDI);
- serial_port_out(port, UART_IER, up->ier);
+ serial8250_out_IER(up, up->ier);
}
/**
@@ -1332,7 +1332,8 @@ void serial8250_em485_stop_tx(struct uart_8250_port *p, bool toggle_ier)
* Enable previously disabled RX interrupts.
*/
if (!(p->port.rs485.flags & SER_RS485_RX_DURING_TX)) {
- serial8250_clear_and_reinit_fifos(p);
+ if (!serial8250_rx_dma_recovering(p))
+ serial8250_clear_and_reinit_fifos(p);
if (toggle_ier) {
/*
@@ -1346,7 +1347,7 @@ void serial8250_em485_stop_tx(struct uart_8250_port *p, bool toggle_ier)
lockdep_assert_held_once(&p->port.lock);
p->ier |= UART_IER_RLSI | UART_IER_RDI;
- serial_port_out(&p->port, UART_IER, p->ier);
+ serial8250_out_IER(p, p->ier);
}
}
}
@@ -1600,7 +1601,7 @@ static void serial8250_disable_ms(struct uart_port *port)
mctrl_gpio_disable_ms_no_sync(up->gpios);
up->ier &= ~UART_IER_MSI;
- serial_port_out(port, UART_IER, up->ier);
+ serial8250_out_IER(up, up->ier);
}
static void serial8250_enable_ms(struct uart_port *port)
@@ -1619,7 +1620,7 @@ static void serial8250_enable_ms(struct uart_port *port)
up->ier |= UART_IER_MSI;
guard(serial8250_rpm)(up);
- serial_port_out(port, UART_IER, up->ier);
+ serial8250_out_IER(up, up->ier);
}
void serial8250_read_char(struct uart_8250_port *up, u16 lsr)
@@ -1816,8 +1817,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);
}
@@ -1837,15 +1837,22 @@ void serial8250_handle_irq_locked(struct uart_port *port, unsigned int iir)
lockdep_assert_held_once(&port->lock);
+ if (up->dma && up->dma->shutting_down)
+ return;
+
status = serial_lsr_in(up);
/*
* Recover from no-data-ready and FIFO error condition to avoid getting
* stuck in the ISR.
*/
- if (!(status & UART_LSR_DR) && (status & UART_LSR_FIFOE))
+ if (!(status & UART_LSR_DR) && (status & UART_LSR_FIFOE) &&
+ !serial8250_rx_dma_running(up))
serial8250_clear_and_reinit_fifos(up);
+ if (serial8250_rx_dma_recovering(up))
+ skip_rx = true;
+
/*
* If port is stopped and there are no error conditions in the
* FIFO, then don't drain the FIFO, as this may lead to TTY buffer
@@ -2345,6 +2352,12 @@ int serial8250_do_startup(struct uart_port *port)
guard(serial8250_rpm)(up);
+ /* The previous shutdown has drained DMA and its callbacks. */
+ scoped_guard(uart_port_lock_irqsave, port) {
+ if (up->dma)
+ up->dma->shutting_down = false;
+ }
+
serial8250_startup_special(port);
/*
@@ -2786,7 +2799,7 @@ static void serial8250_set_ier(struct uart_port *port, struct ktermios *termios)
if (up->capabilities & UART_CAP_RTOIE)
up->ier |= UART_IER_RTOIE;
- serial_port_out(port, UART_IER, up->ier);
+ serial8250_out_IER(up, up->ier);
}
static void serial8250_set_efr(struct uart_port *port, struct ktermios *termios)
--
2.53.0
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v8 5/6] serial: 8250: retire canceled RX DMA before reusing the receive buffer
2026-10-29 7:37 ` [PATCH v8 5/6] serial: 8250: retire canceled RX DMA before reusing the receive buffer James Hilliard
@ 2026-10-29 7:51 ` sashiko-bot
2026-10-29 14:22 ` Andy Shevchenko
2026-10-29 22:27 ` Raul E Rangel
2 siblings, 0 replies; 23+ messages in thread
From: sashiko-bot @ 2026-10-29 7:51 UTC (permalink / raw)
To: James Hilliard; +Cc: linux-serial
> The generic RX timeout path pauses a transfer, copies its data and
> terminates it asynchronously. Clearing rx_running at that point lets a
> new receive reuse the buffer and cookie while the old callback has not
> yet run. 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. Keep FIFO access separate from callback retirement: after a
> successful pause and termination, clear rx_running while rx_terminating
> prevents another descriptor from reusing the buffer. Deliver the paused
> data before allowing the interrupt handler to drain the FIFO through PIO.
> Normal retirement only releases ownership and leaves subsequent receive
> handling to the UART interrupt.
>
> Preserve the normal completion-path LSR read and gate DMA resubmission on
> [ ... ]
> the same order. Drain the UART IRQ before releasing DMA state so an
> in-flight handler cannot retain up->dma across its release.
>
> 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/20261029-submit-serial-8250-dma-cancel-v1-v8-0-3fd56ba352ed@gmail.com?part=5
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v8 5/6] serial: 8250: retire canceled RX DMA before reusing the receive buffer
2026-10-29 7:37 ` [PATCH v8 5/6] serial: 8250: retire canceled RX DMA before reusing the receive buffer James Hilliard
2026-10-29 7:51 ` sashiko-bot
@ 2026-10-29 14:22 ` Andy Shevchenko
2026-10-29 17:17 ` James Hilliard
2026-10-29 22:27 ` Raul E Rangel
2 siblings, 1 reply; 23+ messages in thread
From: Andy Shevchenko @ 2026-10-29 14:22 UTC (permalink / raw)
To: James Hilliard
Cc: Greg Kroah-Hartman, Jiri Slaby, Raul E Rangel, Ilpo Järvinen,
Heikki Krogerus, Tony Lindgren, Sebastian Andrzej Siewior,
Richard Genoud, Nicolas Ferre, Alexandre Belloni, Claudiu Beznea,
Jacky Huang, Shan-Chun Hung, Pali Rohár, Frank Li,
Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
Maciej W. Rozycki, Timur Tabi, linux-kernel, linux-serial,
linux-arm-kernel, imx, linuxppc-dev
On Thu, Oct 29, 2026 at 01:37:01AM -0600, James Hilliard wrote:
> The generic RX timeout path pauses a transfer, copies its data and
> terminates it asynchronously. Clearing rx_running at that point lets a
> new receive reuse the buffer and cookie while the old callback has not
> yet run. 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. Keep FIFO access separate from callback retirement: after a
> successful pause and termination, clear rx_running while rx_terminating
> prevents another descriptor from reusing the buffer. Deliver the paused
> data before allowing the interrupt handler to drain the FIFO through PIO.
> Normal retirement only releases ownership and leaves subsequent receive
> handling to the UART interrupt.
>
> Preserve the normal completion-path LSR read and gate DMA resubmission on
> the receive interrupt enable, so stop_rx() prevents another transfer while
> the status read retains its existing side effects.
>
> Reject new flush work and PIO receive handling during shutdown, including
> late Intel MID DMA timeouts. Stop and drain retirement work before freeing
> the channels and buffer. Once RX has stopped, allow the idle flush used by
> DesignWare LCR recovery during shutdown. Clear the shutdown guard before
> startup configures the UART and tests its interrupts.
>
> Check pause failures before accessing the buffer. A successful pause is
> the quiescence guarantee, even when a provider still reports IN_PROGRESS.
> Also accept DMA_COMPLETE after a failed pause: completion can stop the
> channel before its callback acquires the port lock, leaving nothing for
> the provider to pause. Retire that completed transfer normally.
>
> A failed pause with an incomplete cookie is ambiguous: hardware may have
> finished while its completion interrupt is still pending. Canceling that
> descriptor can discard the entire completed receive buffer and switch
> permanently to PIO. Retain the transfer and retry pause and status in the
> worker, with receive interrupts masked to prevent a timeout interrupt
> storm. The worker copies only after a successful pause or reported
> completion, then terminates and drains the old callback before reuse.
> The pending-pause guard keeps callbacks from copying or resubmitting the
> transfer concurrently. Retries sleep outside the port lock and stop when
> shutdown takes over. A provider which never pauses or completes retains
> RX ownership until an error or shutdown; a timeout alone cannot establish
> that discarding the buffer is safe.
>
> On DMA_ERROR or termination failure, let the worker stop DMA before
> handing the FIFO to PIO. Keep the requested enables in the existing IER
> shadow and filter hardware writes during pause retries and recovery,
> including termios, RS485 and overrun-backoff changes. Apply the current
> enables after retirement so stop_rx() and shutdown remain effective.
> Hold a runtime PM reference around worker register accesses and keep RX
> on PIO until reopen after a confirmed error.
>
> Abort the DesignWare idle sequence when RX cannot be stopped. Allow
> the empty-FIFO timeout workaround once DMA has released the FIFO, including
> while callbacks retire. Exclude FIFO resets during RX DMA recovery. If the
> provider still cannot terminate the channel in process context, retain
> its buffer and leave receive interrupts masked.
>
> For OMAP, publish the shutdown gate under the port lock before the final
> RX flush and interrupt masking. Take the RX DMA lock inside the port lock
> in the completion callback, and serialize the runtime-suspend flush with
> the same order. Drain the UART IRQ before releasing DMA state so an
> in-flight handler cannot retain up->dma across its release.
Can this patch be split to smaller logically isolated (to some extent) pieces?
...
> +++ b/drivers/tty/serial/8250/8250_dma.c
> #include <linux/tty_flip.h>
> #include <linux/serial_reg.h>
> #include <linux/dma-mapping.h>
> +#include <linux/delay.h>
When adding a new header to an unsorted block, try to squeeze it the way that
you get the longest ordered chain (yes, interleaved with some unsorted
elements, but still).
> #include "8250.h"
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v8 5/6] serial: 8250: retire canceled RX DMA before reusing the receive buffer
2026-10-29 14:22 ` Andy Shevchenko
@ 2026-10-29 17:17 ` James Hilliard
0 siblings, 0 replies; 23+ messages in thread
From: James Hilliard @ 2026-10-29 17:17 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Greg Kroah-Hartman, Jiri Slaby, Raul E Rangel, Ilpo Järvinen,
Heikki Krogerus, Tony Lindgren, Sebastian Andrzej Siewior,
Richard Genoud, Nicolas Ferre, Alexandre Belloni, Claudiu Beznea,
Jacky Huang, Shan-Chun Hung, Pali Rohár, Frank Li,
Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
Maciej W. Rozycki, Timur Tabi, linux-kernel, linux-serial,
linux-arm-kernel, imx, linuxppc-dev
On Thu, Oct 29, 2026 at 8:22 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Oct 29, 2026 at 01:37:01AM -0600, James Hilliard wrote:
> > The generic RX timeout path pauses a transfer, copies its data and
> > terminates it asynchronously. Clearing rx_running at that point lets a
> > new receive reuse the buffer and cookie while the old callback has not
> > yet run. 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. Keep FIFO access separate from callback retirement: after a
> > successful pause and termination, clear rx_running while rx_terminating
> > prevents another descriptor from reusing the buffer. Deliver the paused
> > data before allowing the interrupt handler to drain the FIFO through PIO.
> > Normal retirement only releases ownership and leaves subsequent receive
> > handling to the UART interrupt.
> >
> > Preserve the normal completion-path LSR read and gate DMA resubmission on
> > the receive interrupt enable, so stop_rx() prevents another transfer while
> > the status read retains its existing side effects.
> >
> > Reject new flush work and PIO receive handling during shutdown, including
> > late Intel MID DMA timeouts. Stop and drain retirement work before freeing
> > the channels and buffer. Once RX has stopped, allow the idle flush used by
> > DesignWare LCR recovery during shutdown. Clear the shutdown guard before
> > startup configures the UART and tests its interrupts.
> >
> > Check pause failures before accessing the buffer. A successful pause is
> > the quiescence guarantee, even when a provider still reports IN_PROGRESS.
> > Also accept DMA_COMPLETE after a failed pause: completion can stop the
> > channel before its callback acquires the port lock, leaving nothing for
> > the provider to pause. Retire that completed transfer normally.
> >
> > A failed pause with an incomplete cookie is ambiguous: hardware may have
> > finished while its completion interrupt is still pending. Canceling that
> > descriptor can discard the entire completed receive buffer and switch
> > permanently to PIO. Retain the transfer and retry pause and status in the
> > worker, with receive interrupts masked to prevent a timeout interrupt
> > storm. The worker copies only after a successful pause or reported
> > completion, then terminates and drains the old callback before reuse.
> > The pending-pause guard keeps callbacks from copying or resubmitting the
> > transfer concurrently. Retries sleep outside the port lock and stop when
> > shutdown takes over. A provider which never pauses or completes retains
> > RX ownership until an error or shutdown; a timeout alone cannot establish
> > that discarding the buffer is safe.
> >
> > On DMA_ERROR or termination failure, let the worker stop DMA before
> > handing the FIFO to PIO. Keep the requested enables in the existing IER
> > shadow and filter hardware writes during pause retries and recovery,
> > including termios, RS485 and overrun-backoff changes. Apply the current
> > enables after retirement so stop_rx() and shutdown remain effective.
> > Hold a runtime PM reference around worker register accesses and keep RX
> > on PIO until reopen after a confirmed error.
> >
> > Abort the DesignWare idle sequence when RX cannot be stopped. Allow
> > the empty-FIFO timeout workaround once DMA has released the FIFO, including
> > while callbacks retire. Exclude FIFO resets during RX DMA recovery. If the
> > provider still cannot terminate the channel in process context, retain
> > its buffer and leave receive interrupts masked.
> >
> > For OMAP, publish the shutdown gate under the port lock before the final
> > RX flush and interrupt masking. Take the RX DMA lock inside the port lock
> > in the completion callback, and serialize the runtime-suspend flush with
> > the same order. Drain the UART IRQ before releasing DMA state so an
> > in-flight handler cannot retain up->dma across its release.
>
> Can this patch be split to smaller logically isolated (to some extent) pieces?
The OMAP shutdown and locking changes look like they can be split
out.
The generic RX changes are more tightly coupled: the retirement
state, worker, IER filtering and FIFO-access guards must agree about
whether DMA still owns the receive path. Splitting those mechanically
could leave intermediate commits allowing PIO or another descriptor
to access a buffer that has not been retired.
>
> ...
>
> > +++ b/drivers/tty/serial/8250/8250_dma.c
>
> > #include <linux/tty_flip.h>
> > #include <linux/serial_reg.h>
> > #include <linux/dma-mapping.h>
> > +#include <linux/delay.h>
>
> When adding a new header to an unsorted block, try to squeeze it the way that
> you get the longest ordered chain (yes, interleaved with some unsorted
> elements, but still).
>
> > #include "8250.h"
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v8 5/6] serial: 8250: retire canceled RX DMA before reusing the receive buffer
2026-10-29 7:37 ` [PATCH v8 5/6] serial: 8250: retire canceled RX DMA before reusing the receive buffer James Hilliard
2026-10-29 7:51 ` sashiko-bot
2026-10-29 14:22 ` Andy Shevchenko
@ 2026-10-29 22:27 ` Raul E Rangel
2 siblings, 0 replies; 23+ messages in thread
From: Raul E Rangel @ 2026-10-29 22:27 UTC (permalink / raw)
To: James Hilliard
Cc: Greg Kroah-Hartman, Jiri Slaby, Ilpo Järvinen,
Andy Shevchenko, Heikki Krogerus, Tony Lindgren,
Sebastian Andrzej Siewior, Richard Genoud, Nicolas Ferre,
Alexandre Belloni, Claudiu Beznea, Jacky Huang, Shan-Chun Hung,
Pali Rohár, Frank Li, Sascha Hauer, Pengutronix Kernel Team,
Fabio Estevam, Maciej W. Rozycki, Timur Tabi, linux-kernel,
linux-serial, linux-arm-kernel, imx, linuxppc-dev
Hi James,
Thanks for the v8 updates and for opening PR #1 on the TLA+ repo! Confirming
that the v7 issues in Patch 2/6 (lantiq, dz, zs, jsm_cls), Patch 4/6
(__dma_tx_complete tx_running check), and 8250_omap.c shutdown locking all
look resolved in v8.
On Patch 5/6, we updated our TLA+ model for the new rx_pause_pending worker
retry loop:
> + ret = dmaengine_pause(dma->rxchan);
> + status = dmaengine_tx_status(dma->rxchan, dma->rx_cookie, &state);
> + if (status == DMA_ERROR) {
> + ret = -EIO;
> + goto recover;
> + }
> + if (ret && status != DMA_COMPLETE) {
> + dma->rx_pause_pending = true;
> + serial8250_out_IER(p, p->ier);
> + return ret;
> + }
...
> + for (;;) {
> + scoped_guard(uart_port_lock_irqsave, &p->port) {
> + if (dma->shutting_down)
> + return;
> + if (dma->rx_pause_pending && !dma->rx_terminating)
> + serial8250_rx_dma_try_flush(p);
> + retry = dma->rx_pause_pending && !dma->rx_terminating;
> + recover = dma->rx_err;
> + }
> + if (!retry)
> + break;
> + usleep_range(1000, 2000);
> + }
This cleanly resolves the race where a full 4 KB transfer completed in
hardware just before dmaengine_pause() ran while its DMA completion hardirq
was still pending (once serial8250_rx_dma_flush_work() drops port->lock and
sleeps, the pending DMA hardirq runs and marks rx_cookie as DMA_COMPLETE).
However, what happens if dmaengine_pause() fails on a *partial* RX transfer
(0 < count < 4096 bytes) during a normal UART_IIR_RX_TIMEOUT where the remote
peer stopped sending and is waiting for a reply?
(In PR #1's Serial8250RxLifetime.tla, Spec puts WF_vars(ProviderProgress) on
Receive at lines 67-70, which forces the remote peer to always keep
transmitting until count == Capacity and CompleteInHw fires.)
When the peer is idle after sending a partial burst (< 4096 bytes):
1. Hardware never reaches 4096 bytes, so status remains DMA_IN_PROGRESS
(neither DMA_COMPLETE nor DMA_ERROR).
2. If dmaengine_pause(dma->rxchan) persistently returns an error (-ENXIO,
-EBUSY, -EAGAIN, -EINVAL, etc.) on that partial transfer,
serial8250_rx_dma_flush_work() loops forever in `for (;;)` calling
usleep_range(1000, 2000):
- It holds a runtime PM reference (`guard(serial8250_rpm)(p)`) and wakes a
system_percpu_wq worker 500-1000 times per second until the port is closed.
- RX interrupts remain masked in hardware (`rx_pause_pending == true`), the
partial bytes are never flushed, and the port never falls back to PIO.
- On 8250_dw, dw8250_idle_enter() gets -EAGAIN from serial8250_rx_dma_flush()
while rx_pause_pending is true, causing dw8250_set_divisor() and
dw8250_check_lcr() to silently abort baud-rate and LCR updates.
Since the only reason to retry pause/status is to let an already-pending DMA
completion hardirq run after port->lock (and local IRQs) are released, could
we bound the retry loop in serial8250_rx_dma_flush_work() to a small number of
iterations (e.g. 5 retries)? If rx_pause_pending is still unresolved after the
retry budget expires, calling serial8250_rx_dma_recover(p) under port->lock
will set `dma->rx_err = true` and `dma->rx_terminating = true`, letting the
worker exit the retry loop, run dmaengine_terminate_sync(dma->rxchan), and
switch cleanly to PIO.
We pushed the updated v8 TLA+ models (`Serial8250RxDmaV8.tla`,
`Serial8250TxDmaV8.tla`, and `./run_v8.sh`) to:
https://github.com/ismell/linux-8250-dma-tla
Thanks,
Raul
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v8 6/6] serial: 8250: retain DMA resources when termination fails
2026-10-29 7:36 [PATCH v8 0/6] serial: 8250: serialize DMA cancellation with buffer reuse James Hilliard
` (4 preceding siblings ...)
2026-10-29 7:37 ` [PATCH v8 5/6] serial: 8250: retire canceled RX DMA before reusing the receive buffer James Hilliard
@ 2026-10-29 7:37 ` James Hilliard
2026-10-29 7:42 ` sashiko-bot
2026-10-29 14:23 ` Andy Shevchenko
5 siblings, 2 replies; 23+ messages in thread
From: James Hilliard @ 2026-10-29 7:37 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby, Raul E Rangel, Ilpo Järvinen,
Andy Shevchenko, Heikki Krogerus, Tony Lindgren,
Sebastian Andrzej Siewior, Richard Genoud, Nicolas Ferre,
Alexandre Belloni, Claudiu Beznea, Jacky Huang, Shan-Chun Hung,
Pali Rohár, Frank Li, Sascha Hauer, Pengutronix Kernel Team,
Fabio Estevam, Maciej W. Rozycki, Timur Tabi
Cc: linux-kernel, linux-serial, linux-arm-kernel, imx, linuxppc-dev,
James Hilliard
serial8250_release_dma() ignores the return from dmaengine_terminate_sync()
before freeing the coherent RX buffer, unmapping the TX FIFO and releasing
the channels. If the provider fails to stop a channel, the DMAengine
wrapper returns the error without synchronizing its callbacks. Hardware
or a detached callback can then access the released storage and state.
Checking the error and returning early would not preserve their lifetime:
serial core frees the TX FIFO after the driver's shutdown callback returns,
and port removal can then release the callback context as well.
Require successful synchronous termination of both channels before
releasing any DMA resources. Retry failed channels after a sleep outside
the port lock, leaving channels that have already stopped and drained
alone. Keep the shutdown gate and storage reservation in place throughout
the retries.
A permanently failed DMA engine blocks close or removal. This deliberately
retains the buffers, mappings, channels and callback context until the
provider can establish that they are no longer in use.
Fixes: 9ee4b83e51f7 ("serial: 8250: Add support for dmaengine")
Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
drivers/tty/serial/8250/8250_dma.c | 29 +++++++++++++++++++++++++++--
1 file changed, 27 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_dma.c b/drivers/tty/serial/8250/8250_dma.c
index f980b4faf6a3..ff7574b1be40 100644
--- a/drivers/tty/serial/8250/8250_dma.c
+++ b/drivers/tty/serial/8250/8250_dma.c
@@ -505,6 +505,32 @@ int serial8250_request_dma(struct uart_8250_port *p)
}
EXPORT_SYMBOL_GPL(serial8250_request_dma);
+static void serial8250_terminate_dma(struct uart_8250_port *p)
+{
+ struct uart_8250_dma *dma = p->dma;
+ int rx_ret, tx_ret;
+
+ rx_ret = dmaengine_terminate_sync(dma->rxchan);
+ tx_ret = dmaengine_terminate_sync(dma->txchan);
+
+ /*
+ * Shutdown cannot return while DMA owns storage or callback state:
+ * serial core frees the TX FIFO afterwards, and removal frees the port.
+ * Retain both channels and retry outside the port lock. A permanently
+ * failed DMA engine therefore prevents teardown from completing.
+ */
+ while (rx_ret || tx_ret) {
+ dev_err_ratelimited(p->port.dev,
+ "DMA termination failed: RX %d, TX %d; retrying\n",
+ rx_ret, tx_ret);
+ msleep(100);
+ if (rx_ret)
+ rx_ret = dmaengine_terminate_sync(dma->rxchan);
+ if (tx_ret)
+ tx_ret = dmaengine_terminate_sync(dma->txchan);
+ }
+}
+
void serial8250_release_dma(struct uart_8250_port *p)
{
struct uart_8250_dma *dma = p->dma;
@@ -514,9 +540,9 @@ void serial8250_release_dma(struct uart_8250_port *p)
cancel_work_sync(&dma->tx_flush_work);
cancel_work_sync(&dma->rx_flush_work);
+ serial8250_terminate_dma(p);
/* Release RX resources */
- dmaengine_terminate_sync(dma->rxchan);
scoped_guard(uart_port_lock_irqsave, &p->port) {
dma->rx_running = 0;
dma->rx_terminating = false;
@@ -528,7 +554,6 @@ void serial8250_release_dma(struct uart_8250_port *p)
dma->rxchan = NULL;
/* Release TX resources */
- dmaengine_terminate_sync(dma->txchan);
dma_unmap_single(dmaengine_get_dma_device(dma->txchan), dma->tx_addr,
UART_XMIT_SIZE, DMA_TO_DEVICE);
dma_release_channel(dma->txchan);
--
2.53.0
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v8 6/6] serial: 8250: retain DMA resources when termination fails
2026-10-29 7:37 ` [PATCH v8 6/6] serial: 8250: retain DMA resources when termination fails James Hilliard
@ 2026-10-29 7:42 ` sashiko-bot
2026-10-29 14:23 ` Andy Shevchenko
1 sibling, 0 replies; 23+ messages in thread
From: sashiko-bot @ 2026-10-29 7:42 UTC (permalink / raw)
To: James Hilliard; +Cc: linux-serial
> serial8250_release_dma() ignores the return from dmaengine_terminate_sync()
> before freeing the coherent RX buffer, unmapping the TX FIFO and releasing
> the channels. If the provider fails to stop a channel, the DMAengine
> wrapper returns the error without synchronizing its callbacks. Hardware
> or a detached callback can then access the released storage and state.
>
> Checking the error and returning early would not preserve their lifetime:
> serial core frees the TX FIFO after the driver's shutdown callback returns,
> and port removal can then release the callback context as well.
>
> Require successful synchronous termination of both channels before
> releasing any DMA resources. Retry failed channels after a sleep outside
> the port lock, leaving channels that have already stopped and drained
> alone. Keep the shutdown gate and storage reservation in place throughout
> the retries.
>
> A permanently failed DMA engine blocks close or removal. This deliberately
> retains the buffers, mappings, channels and callback context until the
> provider can establish that they are no longer in use.
>
> 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/20261029-submit-serial-8250-dma-cancel-v1-v8-0-3fd56ba352ed@gmail.com?part=6
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v8 6/6] serial: 8250: retain DMA resources when termination fails
2026-10-29 7:37 ` [PATCH v8 6/6] serial: 8250: retain DMA resources when termination fails James Hilliard
2026-10-29 7:42 ` sashiko-bot
@ 2026-10-29 14:23 ` Andy Shevchenko
2026-10-29 17:19 ` James Hilliard
1 sibling, 1 reply; 23+ messages in thread
From: Andy Shevchenko @ 2026-10-29 14:23 UTC (permalink / raw)
To: James Hilliard
Cc: Greg Kroah-Hartman, Jiri Slaby, Raul E Rangel, Ilpo Järvinen,
Heikki Krogerus, Tony Lindgren, Sebastian Andrzej Siewior,
Richard Genoud, Nicolas Ferre, Alexandre Belloni, Claudiu Beznea,
Jacky Huang, Shan-Chun Hung, Pali Rohár, Frank Li,
Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
Maciej W. Rozycki, Timur Tabi, linux-kernel, linux-serial,
linux-arm-kernel, imx, linuxppc-dev
On Thu, Oct 29, 2026 at 01:37:02AM -0600, James Hilliard wrote:
> serial8250_release_dma() ignores the return from dmaengine_terminate_sync()
> before freeing the coherent RX buffer, unmapping the TX FIFO and releasing
> the channels. If the provider fails to stop a channel, the DMAengine
> wrapper returns the error without synchronizing its callbacks. Hardware
> or a detached callback can then access the released storage and state.
>
> Checking the error and returning early would not preserve their lifetime:
> serial core frees the TX FIFO after the driver's shutdown callback returns,
> and port removal can then release the callback context as well.
>
> Require successful synchronous termination of both channels before
> releasing any DMA resources. Retry failed channels after a sleep outside
> the port lock, leaving channels that have already stopped and drained
> alone. Keep the shutdown gate and storage reservation in place throughout
> the retries.
>
> A permanently failed DMA engine blocks close or removal. This deliberately
> retains the buffers, mappings, channels and callback context until the
> provider can establish that they are no longer in use.
...
> + while (rx_ret || tx_ret) {
> + dev_err_ratelimited(p->port.dev,
> + "DMA termination failed: RX %d, TX %d; retrying\n",
> + rx_ret, tx_ret);
> + msleep(100);
Huh?!
> + if (rx_ret)
> + rx_ret = dmaengine_terminate_sync(dma->rxchan);
> + if (tx_ret)
> + tx_ret = dmaengine_terminate_sync(dma->txchan);
> + }
This is unacceptable. Sleeping is okay, but value should really match the
baudspeed and other parameters of the transfer.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v8 6/6] serial: 8250: retain DMA resources when termination fails
2026-10-29 14:23 ` Andy Shevchenko
@ 2026-10-29 17:19 ` James Hilliard
2026-10-29 22:27 ` Raul E Rangel
0 siblings, 1 reply; 23+ messages in thread
From: James Hilliard @ 2026-10-29 17:19 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Greg Kroah-Hartman, Jiri Slaby, Raul E Rangel, Ilpo Järvinen,
Heikki Krogerus, Tony Lindgren, Sebastian Andrzej Siewior,
Richard Genoud, Nicolas Ferre, Alexandre Belloni, Claudiu Beznea,
Jacky Huang, Shan-Chun Hung, Pali Rohár, Frank Li,
Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
Maciej W. Rozycki, Timur Tabi, linux-kernel, linux-serial,
linux-arm-kernel, imx, linuxppc-dev
On Thu, Oct 29, 2026 at 8:24 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Oct 29, 2026 at 01:37:02AM -0600, James Hilliard wrote:
> > serial8250_release_dma() ignores the return from dmaengine_terminate_sync()
> > before freeing the coherent RX buffer, unmapping the TX FIFO and releasing
> > the channels. If the provider fails to stop a channel, the DMAengine
> > wrapper returns the error without synchronizing its callbacks. Hardware
> > or a detached callback can then access the released storage and state.
> >
> > Checking the error and returning early would not preserve their lifetime:
> > serial core frees the TX FIFO after the driver's shutdown callback returns,
> > and port removal can then release the callback context as well.
> >
> > Require successful synchronous termination of both channels before
> > releasing any DMA resources. Retry failed channels after a sleep outside
> > the port lock, leaving channels that have already stopped and drained
> > alone. Keep the shutdown gate and storage reservation in place throughout
> > the retries.
> >
> > A permanently failed DMA engine blocks close or removal. This deliberately
> > retains the buffers, mappings, channels and callback context until the
> > provider can establish that they are no longer in use.
>
> ...
>
> > + while (rx_ret || tx_ret) {
> > + dev_err_ratelimited(p->port.dev,
> > + "DMA termination failed: RX %d, TX %d; retrying\n",
> > + rx_ret, tx_ret);
>
> > + msleep(100);
>
> Huh?!
>
> > + if (rx_ret)
> > + rx_ret = dmaengine_terminate_sync(dma->rxchan);
> > + if (tx_ret)
> > + tx_ret = dmaengine_terminate_sync(dma->txchan);
> > + }
>
> This is unacceptable. Sleeping is okay, but value should really match the
> baudspeed and other parameters of the transfer.
This sleep is backoff after dmaengine_terminate_sync() has returned
an error, rather than an estimate of UART transmission time. Each
retry asks the provider to stop the channel again; elapsed time is
never treated as evidence that the resources are safe to free.
The particular 100 ms interval has no hardware-derived justification.
However, I don't yet see a sound way to derive the retry cadence from
baud rate either. RX can be waiting for input, TX can be held by flow
control, and callback retirement is a separate requirement from
moving bytes through the UART.
Do you have a particular provider failure mode in mind where retrying
termination should follow the serial transfer timing? I'd like to
base the interval on the condition that can make termination succeed,
rather than substitute a wire-time estimate for an arbitrary delay.
Regardless of the retry interval, shutdown cannot release the buffers
until the provider establishes that DMA and its callbacks have stopped.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v8 6/6] serial: 8250: retain DMA resources when termination fails
2026-10-29 17:19 ` James Hilliard
@ 2026-10-29 22:27 ` Raul E Rangel
0 siblings, 0 replies; 23+ messages in thread
From: Raul E Rangel @ 2026-10-29 22:27 UTC (permalink / raw)
To: James Hilliard, Andy Shevchenko
Cc: Greg Kroah-Hartman, Jiri Slaby, Ilpo Järvinen,
Heikki Krogerus, Tony Lindgren, Sebastian Andrzej Siewior,
Richard Genoud, Nicolas Ferre, Alexandre Belloni, Claudiu Beznea,
Jacky Huang, Shan-Chun Hung, Pali Rohár, Frank Li,
Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
Maciej W. Rozycki, Timur Tabi, linux-kernel, linux-serial,
linux-arm-kernel, imx, linuxppc-dev
On Thu, Oct 29, 2026 at 11:19 AM James Hilliard <james.hilliard1@gmail.com> wrote:
> On Thu, Oct 29, 2026 at 8:24 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > > + while (rx_ret || tx_ret) {
> > > + dev_err_ratelimited(p->port.dev,
> > > > + "DMA termination failed: RX %d, TX %d; retrying\n",
> > > > + rx_ret, tx_ret);
> > > > + msleep(100);
> > > > + if (rx_ret)
> > > > + rx_ret = dmaengine_terminate_sync(dma->rxchan);
> > > > + if (tx_ret)
> > > > + tx_ret = dmaengine_terminate_sync(dma->txchan);
> > > > + }
> > >
> > > Huh?!
> > >
> > > This is unacceptable. Sleeping is okay, but value should really match the
> > > baudspeed and other parameters of the transfer.
> ...
> Regardless of the retry interval, shutdown cannot release the buffers
> until the provider establishes that DMA and its callbacks have stopped.
Agreed that if dmaengine_terminate_sync() fails, shutdown must not free
dma->rx_buf, unmap dma->tx_addr, or kfree(port->xmit_buf) while hardware
or a detached callback may still touch them.
However, because serial8250_release_dma() runs under port->mutex in
uart_shutdown(), uart_tty_port_shutdown(), and uart_suspend_port(), an
unbounded `while (rx_ret || tx_ret) { msleep(100); ... }` loop means that
if a DMA provider returns a persistent error from dmaengine_terminate_sync()
(-ENOSYS, -EINVAL, -ENODEV, -ENXIO, -ETIMEDOUT, or a wedged controller),
close(), hangup(), and system suspend will hang forever in
TASK_UNINTERRUPTIBLE while holding port->mutex (triggering hung_task_panic
after 120 seconds).
Would it make sense to satisfy both constraints (no UAF on buffers/callbacks,
and no infinite sleep in close()/suspend()) by quarantining/leaking only the
failed channel's resources instead of looping forever? Specifically:
1. Bound the retry loop in serial8250_terminate_dma() (or do not loop on
non-transient errors, checking `if (dma->rxchan)` / `if (dma->txchan)`).
2. If rx_ret or tx_ret is still non-zero, warn once (`dev_err` / `WARN_ON_ONCE`)
and intentionally quarantine/leak only the failed channel's resources:
- If rx_ret != 0, leave dma->rxchan non-NULL, keep dma->shutting_down = true,
and skip dma_free_coherent() / dma_release_channel(dma->rxchan).
- If tx_ret != 0, leave dma->txchan non-NULL, keep dma->shutting_down = true
and state->xmit_blocked = true, skip dma_unmap_single() /
uart_xmit_unblock() / dma_release_channel(dma->txchan), and in
serial_core.c:uart_free_xmit_buf(), skip `kfree(xmit_buf)` if
`state->xmit_blocked` is still set.
3. For a subsequent port reopen after a quarantined failure:
- Remove the redundant `up->dma->shutting_down = false;` in
serial8250_do_startup() before serial8250_request_dma(up) (which already
clears shutting_down under port->lock), and have serial8250_request_dma()
return -EBUSY if `dma->rxchan || dma->txchan` is still non-NULL so
reopen falls back cleanly to PIO (`up->dma = NULL`).
- Guard the DMA completion callbacks (`__dma_tx_complete`, `dma_rx_complete`,
`omap_8250_dma_tx_complete`, `__dma_rx_complete`) with `if (!dma || ...)`
so a late callback after `up->dma = NULL` is ignored safely.
That way, neither hardware nor a detached callback can ever UAF rx_buf or
xmit_buf, while close(), system suspend, and subsequent PIO reopen all
complete cleanly without an unbounded msleep() loop.
Thanks,
Raul
^ permalink raw reply [flat|nested] 23+ messages in thread