Skip to contentSkip to conversation

serial: 8250: serialize DMA cancellation with buffer reuse

PATCH Superseded James Hilliard 6 participants 58 messages Nov 2 – Nov 9

Subsystems (14)

v9 of 11 Superseded Latest is v11 →

lore
Share & tools
lore
#0 lore raw

v9 cover serial: 8250: serialize DMA cancellation with buffer reuse

Reserve canceled transmit and receive storage until DMA and its completion callbacks have retired. Serial-core write backpressure protects a flushed TX FIFO while normal transfers continue to DMA directly from that FIFO.

The preparatory patches separate OMAP's locked TX completion processing, serialize asynchronous write wakeups, and fix OMAP RX locking and shutdown ordering. Serial-core reservation helpers then protect 8250 TX storage. Flushing empties the logical queue immediately. While the old transfer owns its storage, write_room() reports zero and write-ready notifications wait for retirement. Failed termination retains the reservation until normal completion, without advancing the reset FIFO. DMA retirement releases the reservation independently of port readiness. Serial core keeps writes and write-ready notifications gated during shutdown until successful startup or resume. Flushes still notify threads waiting for the logical FIFO to empty, independently of whether its storage can be reused.

RX delivers paused data before the PIO fallback and retires callbacks in process context. Track FIFO access separately from buffer retirement so PIO and the DesignWare empty-FIFO timeout handling can run once DMA has released the FIFO. Retry ambiguous pause failures until a successful pause or reported completion makes the buffer stable. Keep receive interrupts masked meanwhile, preserving the requested enables in the IER shadow. Back off pause retries from 1 ms to one second, with completion and shutdown waking the worker immediately. Report prolonged stalls while retaining the transfer, and reset the retry delay when the pending flush finishes. Confirmed DMA errors use synchronous recovery and leave RX on PIO until reopen. Shutdown excludes new retirement work before releasing the channels.

Final shutdown retains both channels and their storage until synchronous termination succeeds. Retry failed channels outside the port lock, starting with a 1 ms sleep and doubling it up to one second to limit sustained retries. Report the initial error, rate-limit subsequent failures with elapsed time, and report recovery once both channels have stopped and drained. A permanently failed DMA engine blocks teardown while retaining its storage and callback state.

DMA providers must supply the callback-drain guarantee of dmaengine_synchronize() before retirement releases storage. The related provider synchronization fixes are tracked separately and must precede these consumer changes on affected controllers.

Based on tty-next at 6b4b543fa187 with the posted shutdown guard [0], channel configuration error handling [1], RX TTY accounting fix [2], RZ/N1 transfer-length fix [3], transmit-PM shutdown fix [4], serport write-wakeup deferral v2 [5], DesignWare BUSY-safe fractional-divisor programming [6], and Intel MID RX DMA timeout locking [7]. The serport fix is needed before serializing the remaining write-wakeup callers with the port lock.

[0] https://lore.kernel.org/r/20261021-submit-serial-8250-dma-shutdown-v1-v1-1-3d3ce383c614@gmail.com
[1] https://lore.kernel.org/r/20261021-submit-serial-8250-dma-config-errors-v1-v1-1-c067587d1ef2@gmail.com
[2] https://lore.kernel.org/r/20261021-submit-serial-8250-dma-rx-accounting-v1-v1-1-c11795914ab4@gmail.com
[3] https://lore.kernel.org/r/20261023-submit-serial-8250-dma-tx-length-v1-v2-1-c3106ef4d2df@gmail.com
[4] https://lore.kernel.org/r/20261022-submit-serial-8250-tx-pm-shutdown-v1-v1-1-0e894afa3bb8@gmail.com
[5] https://lore.kernel.org/r/20261028-submit-serport-write-wakeup-v1-v2-1-c05d96259985@gmail.com
[6] https://lore.kernel.org/r/20261027-submit-serial-8250-dw-divisor-v1-v4-1-41714a75e15a@gmail.com
[7] https://lore.kernel.org/r/20261024-submit-serial-8250-mid-rx-lock-v1-v1-1-2b9afdf869d7@gmail.com

Assisted-by Codex:gpt-6-astra

---
Changes in v9:
- Back off pending RX pause retries from 1 ms to one second, wake the worker
on completion and shutdown, and report prolonged stalls once per flush
through a rate-limited warning. Reset the delay for each new flush.
- Lock ST ASC startup's transmit servicing and RX interrupt enable after
requesting the IRQ, covering write wakeups from an empty FIFO as well.
- Wake logical-FIFO drain waiters after a flush even while TX storage is
reserved or the port is not ready, keeping line-discipline write-ready
callbacks deferred until writes are allowed.
- Use plain port locking where interrupt context or an enclosing IRQ-saving
lock already supplies the required exclusion, retaining IRQ-saving locks
in workers, tasklets and mixed-context callers.
- Separate OMAP RX completion/flush locking and shutdown IRQ draining into
two preparatory fixes, with their own rationale and Fixes tags.
- Place the new delay header before the existing include block.
- Back off failed final DMA termination retries from 1 ms to a one-second
cap. Report the initial failure, rate-limit ongoing failures with elapsed
time and current RX/TX errors, and report successful recovery.
- Link to v8: https://patch.msgid.link/20261029-submit-serial-8250-dma-cancel-v1-v8-0-3fd56ba352ed@gmail.com

Changes in v8:
- Retain RX transfers after ambiguous pause failures and retry in the worker
until pause succeeds or completion is reported, preserving delayed data
while masking RX interrupts and allowing shutdown to take over.
- Retain both DMA channels and their storage during final shutdown until
synchronous termination succeeds, retrying failures outside the port lock.
- Update the serport write-wakeup prerequisite to the posted v2.
- Cover DZ transmit servicing, DECstation ZS TX/modem handling and
JSM Classic modem notifications with the port lock.
- Defer ZS console-triggered TX restart to a worker, preserving the
port-before-SCC lock order and draining work during teardown.
- Release Lantiq's private hardware lock before write-wakeup callbacks.
- Assert the port-lock contract in uart_write_wakeup().
- Retain the tx_running guard in generic and OMAP TX callbacks.
- Publish OMAP's shutdown gate before the final RX flush, serialize
RX completion and runtime-suspend flushes with port and RX DMA locks,
and drain its UART interrupt before releasing DMA state.
- Link to v7: https://patch.msgid.link/20261027-submit-serial-8250-dma-cancel-v1-v7-0-985a023ba4b4@gmail.com

Changes in v7:
- Rebase onto tty-next at 6b4b543fa187.
- Use v4 of the DesignWare BUSY-safe fractional-divisor prerequisite.
- Use an unsigned long bitmap and for_each_set_bit() for SCCNXP
deferred write wakeups, with unsigned port indices.
- Narrow SB1250 modem-status locking to shared-state updates and
avoid taking the port lock for RX-only interrupts. Keep TX FIFO
consumption and interrupt masking serialized with writers.
- Link to v6: https://lore.kernel.org/r/20261024-submit-serial-8250-dma-cancel-v1-v6-0-6f366591f566@gmail.com

Changes in v6:
- Use v2 of the DesignWare BUSY-safe fractional-divisor prerequisite.
- Serialize CLPS711x TX servicing and interrupt-enable state with the
port lock. Use non-waiting IRQ disable in stop_tx(), skip queued TX
interrupts after stopping, and drain the handler outside the lock
during shutdown.
- Make the Atmel RX/TX tasklets and SB1250 termios locking IRQ-safe
alongside their interrupt-handler locking, preserving the caller's
interrupt state when releasing the port lock.
- Report RX DMA recovery after releasing the port lock, keeping the
state changes and interrupt-enable restoration serialized.
- Link to v5: https://patch.msgid.link/20261024-submit-serial-8250-dma-cancel-v1-v5-0-9e52968928d2@gmail.com

Changes in v5:
- Add the posted serport write-wakeup deferral as a prerequisite for the
caller-locking preparation.
- Add a preparatory patch serializing asynchronous write-wakeup callers
with the port lock, including indirect CTS and transmit-helper calls.
Keep sleeping register accesses outside the lock and defer SCCNXP
notifications until its controller lock is released.
- Use the separately posted DesignWare BUSY-safe fractional-divisor fix
and Intel MID RX DMA timeout locking fix as prerequisites, leaving five
patches in this series.
- Separate serial-core write readiness from DMA storage reservations. Record
retirement during startup and shutdown while keeping writes and wakeups
gated until the port is ready.
- Wake deferred writers after successful startup or resume, including when
reopening with a retained FIFO, and restart any retained queued output.
- Keep the write-ready notification pending while retained output is at or
above the normal wakeup threshold, until TX completion drains it.
- Publish resume readiness under the TX restart lock without releasing a
reservation that may belong to a newly flushed transfer.
- Release 8250's TX reservation after shutdown has stopped DMA and unmapped
the source; serial core retains control of write readiness.
- Link to v4: https://patch.msgid.link/20261023-submit-serial-8250-dma-cancel-v1-v4-0-e759de1839de@gmail.com

Changes in v4:
- Retain flushed TX reservations through shutdown and failed resume. Release
them after successful resume, or when allocating a new FIFO on reopen,
so retirement cannot wake writers into a closing port.
- Treat RX flush as complete once the channel is inactive, and clear the
shutdown guard before startup configures the UART and tests interrupts.
- Check completion after a failed pause so a transfer that finished before
its callback acquired the port lock is delivered and retired normally.
- Keep DesignWare's BUSY-safe divisor setter after fractional-divisor
discovery, writing DLF and DLL/DLH only after confirming DLAB.
- Link to v3: https://patch.msgid.link/20261023-submit-serial-8250-dma-cancel-v1-v3-0-d78f9e4c4932@gmail.com

Changes in v3:
- Defer write-ready wakeups while flushed TX storage is reserved, and
serialize the flush wakeup with the reservation under the port lock.
- Suppress ordinary TX starts during the reservation while preserving
high-priority flow-control characters, which use separate storage.
- Separate RX FIFO access from callback retirement, allowing PIO and the
DesignWare empty-FIFO timeout workaround after a successful pause and
termination while keeping the receive buffer reserved.
- Use the existing IER shadow for requested enables and a shared register
writer to keep receive interrupts masked throughout DMA error recovery,
including termios, RS485, modem-status and overrun-backoff updates.
- Preserve the timeout-enable bit across stop_rx(), exclude late shutdown
interrupts from PIO, and avoid FIFO resets during RX DMA recovery.
- Preserve the completion-path LSR read after stop_rx(), while gating DMA
resubmission on the receive interrupt enable.
- Pack the cancellation flags after moving all their accesses under the
port lock, including the recovery worker's snapshot and initialization.
- Apply the early-return, shared-helper and line-wrapping cleanups.
- Link to v2: https://patch.msgid.link/20261022-submit-serial-8250-dma-cancel-v1-v2-0-088ea51e3891@gmail.com

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

To: Greg Kroah-Hartman <[email protected]>
To: Jiri Slaby <[email protected]>
To: Richard Genoud <[email protected]>
To: Nicolas Ferre <[email protected]>
To: Alexandre Belloni <[email protected]>
To: Claudiu Beznea <[email protected]>
To: "Maciej W. Rozycki" <[email protected]>
To: Jacky Huang <[email protected]>
To: Shan-Chun Hung <[email protected]>
To: Pali Rohár <[email protected]>
To: Frank Li <[email protected]>
To: Sascha Hauer <[email protected]>
To: Pengutronix Kernel Team <[email protected]>
To: Fabio Estevam <[email protected]>
To: Timur Tabi <[email protected]>
To: Peter Ujfalusi <[email protected]>
To: Vignesh Raghavendra <[email protected]>
To: Tony Lindgren <[email protected]>
To: Raul E Rangel <[email protected]>
To: Ilpo Järvinen <[email protected]>
To: Andy Shevchenko <[email protected]>
To: Heikki Krogerus <[email protected]>
To: Patrice Chotard <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]

---
James Hilliard (8):
serial: 8250_omap: separate locked TX DMA completion processing
serial: serialize asynchronous write wakeups with the port lock
serial: 8250_omap: serialize RX DMA completion and flushing
serial: 8250_omap: quiesce DMA IRQ handling before shutdown
serial: core: allow drivers to reserve flushed transmit storage
serial: 8250: drain canceled TX DMA before reusing its buffer
serial: 8250: retire canceled RX DMA before reusing the receive buffer
serial: 8250: retain DMA resources when termination fails

 drivers/tty/serial/21285.c          |   2 +
 drivers/tty/serial/8250/8250.h      |  66 +++++-
 drivers/tty/serial/8250/8250_core.c |   2 +-
 drivers/tty/serial/8250/8250_dma.c  | 395 +++++++++++++++++++++++++++++-------
 drivers/tty/serial/8250/8250_dw.c   |  28 ++-
 drivers/tty/serial/8250/8250_omap.c |  68 +++++--
 drivers/tty/serial/8250/8250_port.c |  36 +++-
 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/serial_core.c    | 106 +++++++++-
 drivers/tty/serial/st-asc.c         |   2 +
 drivers/tty/serial/ucc_uart.c       |   6 +-
 drivers/tty/serial/zs.c             |  94 ++++++---
 drivers/tty/serial/zs.h             |   3 +
 include/linux/serial_core.h         |  18 +-
 29 files changed, 790 insertions(+), 219 deletions(-)

---
base-commit: 6b4b543fa18757297ac756a6fe6e64f506b7844a
change-id: 20261021-submit-serial-8250-dma-cancel-v1-b208f84e89c1
prerequisite-patch-id: 3604bfaa3a1c214997de1f11162b81a4541af5ab
prerequisite-patch-id: 79dd093ffb35e254e790dffb8887faf51b64f947
prerequisite-patch-id: 80a5061d84c055a64c760268febed179c687b99f
prerequisite-patch-id: 7dbb404baf1f861982cf6a4b755d39cfec548223
prerequisite-patch-id: 40881d81a27b3a1aa45203d171214faaf2197768
prerequisite-patch-id: 76506fd9b234c7268227ff41e4d0989e82a1a433
prerequisite-patch-id: d8219673210dac82d3adab41578ff2f76f5862c9
prerequisite-patch-id: 48659e5ef3f5fb377d0b751f0830c0de4699262c

Best regards,

Signature · 1 line
James Hilliard <[email protected]>
#1 lore raw

PATCH v9 1/8 serial: 8250_omap: separate locked TX DMA completion processing

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.

---
 drivers/tty/serial/8250/8250_omap.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)
drivers/tty/serial/8250/8250_omap.c +13 -6
index 4805b644ccb1..b6f108d34854 100644
@@ -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;
Signature · 1 line
2.53.0
#2 lore raw

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

Sashiko has reviewed this patch and found no issues.
Quoted 6 lines

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.

Sashiko has reviewed this patch and found no issues.

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-submit-serial-8250-dma-cancel-v1-v9-0-9159caade148@gmail.com?part=1
#4 lore raw

PATCH v9 2/8 serial: serialize asynchronous write wakeups with the port lock

In reply to 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 plain port locking in hard-IRQ handlers and in helpers whose callers already mask interrupts. Atmel status handling is also called during resume, where lock_suspended is acquired with interrupts disabled; JSM interrupt processing holds its IRQ-saving board lock. Keep IRQ-saving locking in workers, tasklets and other mixed-context paths.

Take the port lock after requesting the ST ASC interrupt and hold it through startup's transmit servicing and RX interrupt enable. The transmit helper can notify writers even when the FIFO is empty, and serial core calls startup without holding the port spinlock.

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.

---
 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/st-asc.c        |  2 +
 drivers/tty/serial/ucc_uart.c      |  6 ++-
 drivers/tty/serial/zs.c            | 94 ++++++++++++++++++++++++++++----------
 drivers/tty/serial/zs.h            |  3 ++
 21 files changed, 202 insertions(+), 88 deletions(-)
drivers/tty/serial/21285.c +2 -0
index 4de0c975ebdc..c12b888c1b7e 100644
@@ -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)(port);
+
 	uart_port_tx_limited(port, ch, 256,
 		!(*CSR_UARTFLG & 0x20),
 		*CSR_UARTDR = ch,
drivers/tty/serial/atmel_serial.c +6 -6
index c3880259e3bc..4c3a6e10d92f 100644
@@ -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)(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,
drivers/tty/serial/clps711x.c +17 -6
index 83186bf50002..37b2635dda47 100644
@@ -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)(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,
drivers/tty/serial/cpm_uart.c +2 -0
index 39f54bb7b485..3f1e51b04f9f 100644
@@ -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)(port);
+
 	cpm_uart_tx_pump(port);
 }
 
drivers/tty/serial/dz.c +3 -6
index 39d93e9c2d15..c86453937a0f 100644
@@ -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)(&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);
-	}
 }
 
 /*
drivers/tty/serial/fsl_lpuart.c +1 -1
index c8575c965203..cb04571c3d0e 100644
@@ -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);
drivers/tty/serial/jsm/jsm_cls.c +4 -2
index 6e40792f92cf..e8e90181504b 100644
@@ -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, &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, &ch->uart_port)
+			cls_parse_modem(ch, readb(&ch->ch_cls_uart->msr));
 	}
 }
 
drivers/tty/serial/jsm/jsm_neo.c +6 -3
index e8e13bf056e2..d27aa2166364 100644
@@ -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, &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, &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, &ch->uart_port)
+			neo_copy_data_from_queue_to_uart(ch);
 	}
 }
 
drivers/tty/serial/lantiq.c +7 -5
index 62cd9e0bb377..e0b2c7335692 100644
@@ -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(&ltq_port->lock, flags);
-	uart_port_tx(port, ch,
-		lqasc_tx_ready(port),
-		writeb(ch, port->membase + LTQ_ASC_TBUF));
-	spin_unlock_irqrestore(&ltq_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(&ltq_port->lock, flags));
 	return;
 }
 
@@ -239,7 +240,8 @@ lqasc_tx_int(int irq, void *_port)
 	spin_lock_irqsave(&ltq_port->lock, flags);
 	__raw_writel(ASC_IRNCR_TIR, port->membase + LTQ_ASC_IRNCR);
 	spin_unlock_irqrestore(&ltq_port->lock, flags);
-	lqasc_start_tx(port);
+	scoped_guard(uart_port_lock, port)
+		lqasc_start_tx(port);
 	return IRQ_HANDLED;
 }
 
drivers/tty/serial/ma35d1_serial.c +4 -2
index 3be30fb05513..a59a8496a83b 100644
@@ -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, port)
+			transmit_chars(up);
+	}
 	if (fsr & MA35_FSR_TX_OVER_IF)
 		serial_out(up, MA35_FSR_REG, MA35_FSR_TX_OVER_IF);
 
drivers/tty/serial/max3100.c +4 -2
index f858a6669011..5a8817eb82ad 100644
@@ -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) &&
drivers/tty/serial/max310x.c +6 -3
index a0ed70185555..d9ce6ba53c7f 100644
@@ -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);
drivers/tty/serial/mux.c +8 -5
index 3a77a7e5c7bc..8be5b3031749 100644
@@ -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);
 }
 
 /**
drivers/tty/serial/mvebu-uart.c +2 -0
index 33ea496c93b2..35b55138637b 100644
@@ -334,6 +334,8 @@ static void mvebu_uart_tx_chars(struct uart_port *port, unsigned int status)
 {
 	u8 ch;
 
+	guard(uart_port_lock)(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)),
drivers/tty/serial/mxs-auart.c +2 -0
index 697318dbb146..6a310309b55d 100644
@@ -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();
drivers/tty/serial/sb1250-duart.c +15 -10
index b4e1b90e5960..af6a0fe1df70 100644
@@ -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)(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, 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);
 }
 
 
drivers/tty/serial/sccnxp.c +34 -11

104 lines not on this page · raw message

drivers/tty/serial/st-asc.c +2 -0
index 6ed9a327702b..5fefa3325a1f 100644
@@ -422,6 +422,8 @@ static int asc_startup(struct uart_port *port)
 		return -ENODEV;
 	}
 
+	guard(uart_port_lock_irqsave)(port);
+
 	asc_transmit_chars(port);
 	asc_enable_rx_interrupts(port);
 
drivers/tty/serial/ucc_uart.c +4 -2
index 6b7dcbbacd13..4c4e8e4621ec 100644
@@ -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, &qe_port->port)
+			qe_uart_tx_pump(qe_port);
+	}
 
 	return events ? IRQ_HANDLED : IRQ_NONE;
 }
drivers/tty/serial/zs.c +70 -24

195 lines not on this page · raw message

drivers/tty/serial/zs.h +3 -0
index e0d3c189b33f..0404294e4533 100644
@@ -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.  */
 
Signature · 1 line
2.53.0
#5 lore raw

Re: [PATCH v9 2/8] serial: serialize asynchronous write wakeups with the port lock

Sashiko has reviewed this patch and found no issues.
Quoted 20 lines

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 plain port locking in hard-IRQ handlers and in helpers whose callers already mask interrupts. Atmel status handling is also called during resume, where lock_suspended is acquired with interrupts disabled; JSM interrupt processing holds its IRQ-saving board lock. Keep IRQ-saving [ ... ]
the restart worker during shutdown and removal. Release Lantiq's hardware lock through the transmit helper's completion hook before notifying the line discipline.

Quoted 1 line

Sashiko has reviewed this patch and found no issues.

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-submit-serial-8250-dma-cancel-v1-v9-0-9159caade148@gmail.com?part=2
#7 lore raw

Re: [PATCH v9 2/8] serial: serialize asynchronous write wakeups with the port lock

In reply to James Hilliard
Quoted 59 lines · 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 plain port locking in hard-IRQ handlers and in helpers whose callers already mask interrupts. Atmel status handling is also called during resume, where lock_suspended is acquired with interrupts disabled; JSM interrupt processing holds its IRQ-saving board lock. Keep IRQ-saving locking in workers, tasklets and other mixed-context paths.

Take the port lock after requesting the ST ASC interrupt and hold it through startup's transmit servicing and RX interrupt enable. The transmit helper can notify writers even when the FIFO is empty, and serial core calls startup without holding the port spinlock.

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.

...

drivers/tty/serial/sccnxp.c line 13

Quoted 6 lines

+++ b/drivers/tty/serial/sccnxp.c

drivers/tty/serial/sccnxp.c +1 -0
@@ -10,6 +10,7 @@
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/err.h>
+#include <linux/find.h>

Try to understand what AI is trying to achieve and why it compiles. The above is wrong.

Quoted 3 lines
drivers/tty/serial/sccnxp.c +0 -0
 #include <linux/module.h>
 #include <linux/device.h>
 #include <linux/console.h>

...

drivers/tty/serial/sccnxp.c static void sccnxp_handle_tx(struct uart_port *port)

Quoted 5 lines
drivers/tty/serial/sccnxp.c +2 -1
+static unsigned long sccnxp_handle_events(struct sccnxp_port *s)
 {
 
-	int i;
+	unsigned int i;

This change seems unrelated to the patch. Again, slow down and READ what AI produces before sending, please!

Quoted 1 line

u8 isr;

Signature · 2 lines
With Best Regards,
Andy Shevchenko
#8 lore raw

Re: [PATCH v9 2/8] serial: serialize asynchronous write wakeups with the port lock

Quoted 8 lines · Andy Shevchenko

drivers/tty/serial/sccnxp.c static void sccnxp_handle_tx(struct uart_port *port)

Quoted 5 lines
drivers/tty/serial/sccnxp.c +2 -1
+static unsigned long sccnxp_handle_events(struct sccnxp_port *s)
 {
 
-	int i;
+	unsigned int i;

This change seems unrelated to the patch. Again, slow down and READ what AI produces before sending, please!

Indeed. FWIW I've thrown v8 at hardware I look after, that is:

 drivers/tty/serial/dz.c            |  9 ++--
 drivers/tty/serial/sb1250-duart.c  | 25 ++++++----
 drivers/tty/serial/zs.c            | 94 ++++++++++++++++++++++++++++----------
 drivers/tty/serial/zs.h            |  3 ++

and that has caused no observable regressions in my simple verification as direct terminal lines, both in the line mode and with ncurses.

I have no leased-line modem hardware to put this under stress with all
the signalling; I could see, and probably should, whether I can set up a SLIP connection with a direct serial link and see how it performs, but it'd take me more time than I can afford right now, especially as I'm locally short of one PSU for the systems in question that is waiting for parts to arrive for repair, and swapping the one working unit between systems is quite involving (ones in the lab won't run SLIP owing to the firmware limitation of the console servers they're wired to).

But I didn't have a chance to reply before v9 has been posted with more
updates, so I'll wait for things to settle before reviewing the changes in detail.

Please consider this a NAK for the drivers listed above until the final
version of the patch has been posted and I've had a chance to verify it.

Maciej

#9 lore raw

PATCH v9 3/8 serial: 8250_omap: serialize RX DMA completion and flushing

In reply to James Hilliard

Moving RX DMA locking out of __dma_rx_do_complete() left the DMA completion callback calling it without rx_dma_lock. Shutdown and runtime suspend flush RX under rx_dma_lock but without the port lock, allowing these paths to overlap the callback's buffer accounting and flip-buffer insertion. RX submission and UART interrupt handling also require the port lock.

Take rx_dma_lock inside the port lock in the completion callback. Hold the port lock around the shutdown and runtime-suspend flushes, matching the existing interrupt-path lock order. Assert both locks in the common completion helper and the port lock at the flush entry point.

Fixes 789898416749 ("serial: 8250: 8250_omap: Move locking out from __dma_rx_do_complete()")
---
 drivers/tty/serial/8250/8250_omap.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)
drivers/tty/serial/8250/8250_omap.c +14 -4
index b6f108d34854..322403cec2de 100644
@@ -771,8 +771,10 @@ static void omap_8250_shutdown(struct uart_port *port)
 	guard(serial8250_rpm)(up);
 
 	flush_work(&priv->qos_work);
-	if (up->dma)
+	if (up->dma) {
+		guard(uart_port_lock_irq)(port);
 		omap_8250_rx_dma_flush(up);
+	}
 
 	serial_out(up, UART_OMAP_WER, 0);
 	if (priv->habit & UART_HAS_EFR2)
@@ -898,7 +900,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 +913,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 +982,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 +1001,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) {
@@ -1811,8 +1819,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);
Signature · 1 line
2.53.0
#10 lore raw

Re: [PATCH v9 3/8] serial: 8250_omap: serialize RX DMA completion and flushing

Sashiko has reviewed this patch and found no issues.
Quoted 7 lines

Moving RX DMA locking out of __dma_rx_do_complete() left the DMA completion callback calling it without rx_dma_lock. Shutdown and runtime suspend flush RX under rx_dma_lock but without the port lock, allowing these paths to overlap the callback's buffer accounting and flip-buffer insertion. RX submission and UART interrupt handling also require the port lock.

Quoted 7 lines

Take rx_dma_lock inside the port lock in the completion callback. Hold the port lock around the shutdown and runtime-suspend flushes, matching the existing interrupt-path lock order. Assert both locks in the common completion helper and the port lock at the flush entry point.

Fixes 789898416749 ("serial: 8250: 8250_omap: Move locking out from __dma_rx_do_complete()")

Sashiko has reviewed this patch and found no issues.

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-submit-serial-8250-dma-cancel-v1-v9-0-9159caade148@gmail.com?part=3
maintainer
#11 lore raw

Re: [PATCH v9 3/8] serial: 8250_omap: serialize RX DMA completion and flushing

In reply to James Hilliard
Quoted 35 lines · James Hilliard

Moving RX DMA locking out of __dma_rx_do_complete() left the DMA completion callback calling it without rx_dma_lock. Shutdown and runtime suspend flush RX under rx_dma_lock but without the port lock, allowing these paths to overlap the callback's buffer accounting and flip-buffer insertion. RX submission and UART interrupt handling also require the port lock.

Take rx_dma_lock inside the port lock in the completion callback. Hold the port lock around the shutdown and runtime-suspend flushes, matching the existing interrupt-path lock order. Assert both locks in the common completion helper and the port lock at the flush entry point.

Fixes 789898416749 ("serial: 8250: 8250_omap: Move locking out from __dma_rx_do_complete()")
---
 drivers/tty/serial/8250/8250_omap.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)
drivers/tty/serial/8250/8250_omap.c +3 -1
index b6f108d34854..322403cec2de 100644
@@ -771,8 +771,10 @@ static void omap_8250_shutdown(struct uart_port *port)
 	guard(serial8250_rpm)(up);
 
 	flush_work(&priv->qos_work);
-	if (up->dma)
+	if (up->dma) {
+		guard(uart_port_lock_irq)(port);
 		omap_8250_rx_dma_flush(up);
+	}
 
 	serial_out(up, UART_OMAP_WER, 0);
 	if (priv->habit & UART_HAS_EFR2)

drivers/tty/serial/8250/8250_omap.c static int omap8250_rs485_config(struct uart_port *port, · line 903

Quoted 5 lines
drivers/tty/serial/8250/8250_omap.c +1 -1
@@ -898,7 +900,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. */

Nit, following kerneldoc's style, this would start with Context: (you don't need to make it full kerneldoc but it is useful to follow the kernel doc style where possible).

Quoted 46 lines
drivers/tty/serial/8250/8250_omap.c +10 -2
 static void __dma_rx_do_complete(struct uart_8250_port *p)
 {
 	struct uart_8250_dma    *dma = p->dma;
@@ -911,6 +913,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 +982,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 +1001,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) {
@@ -1811,8 +1819,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);
Signature · 1 line
 i.
#13 lore raw

PATCH v9 4/8 serial: 8250_omap: quiesce DMA IRQ handling before shutdown

In reply to James Hilliard

disable_irq_nosync() masks the UART interrupt without waiting for a running handler. That handler can have observed up->dma and continue using it while shutdown releases the DMA channels and clears the pointer. Synchronizing the IRQ later in serial core does not protect those accesses.

Publish the DMA shutdown gate under the port lock before the final RX flush and interrupt masking. Have DMA IRQ handling check the same gate under the lock, so a queued interrupt cannot restart receive processing after the final flush.

Drain the UART IRQ outside the port lock before releasing DMA state. The lock excludes competing RX processing during the flush, while disable_irq() also waits for a handler that observed up->dma before acquiring the lock.

Fixes fef4f600319e ("serial: 8250: omap: Fix life cycle issues for interrupt handlers")
---
 drivers/tty/serial/8250/8250_omap.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)
drivers/tty/serial/8250/8250_omap.c +10 -6
index 322403cec2de..fc1a83d6fa7c 100644
@@ -771,10 +771,6 @@ static void omap_8250_shutdown(struct uart_port *port)
 	guard(serial8250_rpm)(up);
 
 	flush_work(&priv->qos_work);
-	if (up->dma) {
-		guard(uart_port_lock_irq)(port);
-		omap_8250_rx_dma_flush(up);
-	}
 
 	serial_out(up, UART_OMAP_WER, 0);
 	if (priv->habit & UART_HAS_EFR2)
@@ -782,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;
@@ -1330,6 +1329,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) {
Signature · 1 line
2.53.0
#14 lore raw

Re: [PATCH v9 4/8] serial: 8250_omap: quiesce DMA IRQ handling before shutdown

Sashiko has reviewed this patch and found no issues.
Quoted 10 lines

disable_irq_nosync() masks the UART interrupt without waiting for a running handler. That handler can have observed up->dma and continue using it while shutdown releases the DMA channels and clears the pointer. Synchronizing the IRQ later in serial core does not protect those accesses.

Publish the DMA shutdown gate under the port lock before the final RX flush and interrupt masking. Have DMA IRQ handling check the same gate under the lock, so a queued interrupt cannot restart receive processing after the final flush.

Quoted 7 lines

Drain the UART IRQ outside the port lock before releasing DMA state. The lock excludes competing RX processing during the flush, while disable_irq() also waits for a handler that observed up->dma before acquiring the lock.

Fixes fef4f600319e ("serial: 8250: omap: Fix life cycle issues for interrupt handlers")

Sashiko has reviewed this patch and found no issues.

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-submit-serial-8250-dma-cancel-v1-v9-0-9159caade148@gmail.com?part=4
#16 lore raw

PATCH v9 5/8 serial: core: allow drivers to reserve flushed transmit storage

In reply to 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's write-ready notification under the port lock so it cannot race another reservation.

Wake tty->write_wait after flushing even when writes remain blocked. tty_wait_until_sent() waits for the logical FIFO to empty, independently of whether its storage can be reused. Notify these drain waiters without invoking the line discipline's write_wakeup() callback, so a reservation does not prevent a completed logical flush from waking a drain waiter or consume a one-shot write-ready notification prematurely.

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.

---
 drivers/tty/serial/serial_core.c | 106 +++++++++++++++++++++++++++++++++++----
 include/linux/serial_core.h      |  18 ++++++-
 2 files changed, 113 insertions(+), 11 deletions(-)
drivers/tty/serial/serial_core.c +97 -9
index c0deec0e6af4..baf786d5c34f 100644
@@ -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.
+ * Line-discipline 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,12 @@ 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);
+
+	/* Drain waiters observe FIFO emptiness, not write readiness. */
+	wake_up_interruptible(&tty->write_wait);
 }
 
 /*
@@ -1748,8 +1832,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 +2477,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 +2585,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?
include/linux/serial_core.h +16 -2
index 4eeb8253752a..41598a38a408 100644
@@ -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()
Signature · 1 line
2.53.0
#17 lore raw

Re: [PATCH v9 5/8] serial: core: allow drivers to reserve flushed transmit storage

Sashiko has reviewed this patch and found no issues.
Quoted 14 lines

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's write-ready notification under the port lock so it cannot race another reservation.

Quoted 7 lines

Wake tty->write_wait after flushing even when writes remain blocked. [ ... ]
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.

Sashiko has reviewed this patch and found no issues.

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-submit-serial-8250-dma-cancel-v1-v9-0-9159caade148@gmail.com?part=5
#19 lore raw

Re: [PATCH v9 5/8] serial: core: allow drivers to reserve flushed transmit storage

In reply to James Hilliard
Quoted 42 lines · 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's write-ready notification under the port lock so it cannot race another reservation.

Wake tty->write_wait after flushing even when writes remain blocked. tty_wait_until_sent() waits for the logical FIFO to empty, independently of whether its storage can be reused. Notify these drain waiters without invoking the line discipline's write_wakeup() callback, so a reservation does not prevent a completed logical flush from waking a drain waiter or consume a one-shot write-ready notification prematurely.

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.

...

drivers/tty/serial/serial_core.c static inline struct uart_port *uart_port_check(struct uart_state *state) · line 147

Quoted 3 lines
drivers/tty/serial/serial_core.c +3 -0
+	/* Retained output must drain before consuming a write-ready notification. */
+	if (kfifo_len(&state->port.xmit_fifo) < WAKEUP_CHARS)
+		uart_write_wakeup(port);

I believe I have seen this already a few times. Perhaps a helper with documented idea behind it?

Signature · 2 lines
With Best Regards,
Andy Shevchenko
#20 lore raw

PATCH v9 6/8 serial: 8250: drain canceled TX DMA before reusing its buffer

In reply to 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")
---
 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(-)
drivers/tty/serial/8250/8250.h +17 -3
index 974a47f24084..9f01fa36fa3c 100644
@@ -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);
drivers/tty/serial/8250/8250_dma.c +94 -36

226 lines not on this page · raw message

drivers/tty/serial/8250/8250_omap.c +14 -7
index fc1a83d6fa7c..9211ccda316a 100644
@@ -1099,7 +1099,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;
@@ -1132,7 +1137,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);
 }
@@ -1147,7 +1152,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)) {
 
@@ -1206,24 +1211,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;
Signature · 1 line
2.53.0
#21 lore raw

Re: [PATCH v9 6/8] serial: 8250: drain canceled TX DMA before reusing its buffer

Sashiko has reviewed this patch and found no issues.
Quoted 14 lines

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.

Quoted 7 lines

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

Sashiko has reviewed this patch and found no issues.

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-submit-serial-8250-dma-cancel-v1-v9-0-9159caade148@gmail.com?part=6
#23 lore raw

Re: [PATCH v9 6/8] serial: 8250: drain canceled TX DMA before reusing its buffer

In reply to James Hilliard
Quoted 33 lines · 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.

...

Quoted 7 lines
drivers/tty/serial/8250/8250.h +2 -2
-	if (!dma->tx_running)
+	if (!dma->tx_running || serial8250_tx_dma_blocked(dma))
 		return;
 
-	if (!dma->tx_running)
+	if (!dma->tx_running || serial8250_tx_dma_blocked(dma))
 		return;

...

drivers/tty/serial/8250/8250_omap.c static void omap_8250_dma_tx_complete(void *param) · line 1141

Quoted 5 lines
drivers/tty/serial/8250/8250_omap.c +1 -1
 	guard(uart_port_lock_irqsave)(&p->port);
 
-	if (dma->shutting_down)
+	if (serial8250_tx_dma_blocked(dma) || !dma->tx_running)
 		return;

Try to make these checks consistent. It's always better to choose the order and be aware of the potential side-effects of the bit-wise operations.

...

Quoted 4 lines
drivers/tty/serial/8250/8250_omap.c +1 -1
-	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)) {
Signature · 2 lines
With Best Regards,
Andy Shevchenko
#24 lore raw

PATCH v9 7/8 serial: 8250: retire canceled RX DMA before reusing the receive buffer

In reply to 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.

Start pause retries at 1 ms and double the delay up to one second to limit CPU use during persistent failure. Sleep outside the port lock on a timed waitqueue, with completion and shutdown publishing a wakeup under the same lock as the worker's retry check. This also catches notifications arriving just before the worker sleeps, avoiding a full backoff delay after completion or during shutdown. Reset the delay for each new flush. Report a stall lasting at least one second once per flush, rate-limited across repeated stalls. 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.

Fixes 9ee4b83e51f7 ("serial: 8250: Add support for dmaengine")
---
 drivers/tty/serial/8250/8250.h      |  46 +++++++-
 drivers/tty/serial/8250/8250_core.c |   2 +-
 drivers/tty/serial/8250/8250_dma.c  | 222 ++++++++++++++++++++++++++++++------
 drivers/tty/serial/8250/8250_dw.c   |  28 +++--
 drivers/tty/serial/8250/8250_omap.c |   2 +
 drivers/tty/serial/8250/8250_port.c |  36 ++++--
 6 files changed, 276 insertions(+), 60 deletions(-)
drivers/tty/serial/8250/8250.h +42 -4
index 9f01fa36fa3c..d5f847f865e6 100644
@@ -11,6 +11,7 @@
 #include <linux/serial_8250.h>
 #include <linux/serial_core.h>
 #include <linux/dmaengine.h>
+#include <linux/wait.h>
 #include <linux/workqueue.h>
 
 #include "../serial_mctrl_gpio.h"
@@ -23,6 +24,10 @@ 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;
+	wait_queue_head_t rx_pause_wait;
+	/* Written under the port lock; sampled locklessly by the wait condition. */
+	bool rx_pause_wakeup;
 	struct uart_8250_port *port;
 
 	/* Filter function */
@@ -55,11 +60,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 +158,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 +254,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 +266,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 +450,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 +506,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;
drivers/tty/serial/8250/8250_core.c +1 -1
index 42319843cb5b..ca06288038ed 100644
@@ -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,
drivers/tty/serial/8250/8250_dma.c +185 -37

286 lines not on this page · raw message

drivers/tty/serial/8250/8250_dw.c +20 -8
index 9cbe076463e0..050a1c23d9f0 100644
@@ -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)) {
drivers/tty/serial/8250/8250_omap.c +2 -0
index 9211ccda316a..e305fb4264fe 100644
@@ -780,6 +780,8 @@ static void omap_8250_shutdown(struct uart_port *port)
 	scoped_guard(uart_port_lock_irq, port) {
 		if (up->dma) {
 			up->dma->shutting_down = true;
+			WRITE_ONCE(up->dma->rx_pause_wakeup, true);
+			wake_up(&up->dma->rx_pause_wait);
 			omap_8250_rx_dma_flush(up);
 		}
 		up->ier = 0;
drivers/tty/serial/8250/8250_port.c +26 -10
index cfbd154bf5f9..49d6a0b19fb4 100644
@@ -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);
 
 	/*
@@ -2447,8 +2460,11 @@ void serial8250_do_shutdown(struct uart_port *port)
 	 * Synchronize UART_IER access against the console.
 	 */
 	scoped_guard(uart_port_lock_irqsave, port) {
-		if (up->dma)
+		if (up->dma) {
 			up->dma->shutting_down = true;
+			WRITE_ONCE(up->dma->rx_pause_wakeup, true);
+			wake_up(&up->dma->rx_pause_wait);
+		}
 		up->ier = 0;
 		serial_port_out(port, UART_IER, 0);
 	}
@@ -2786,7 +2802,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)
Signature · 1 line
2.53.0
#25 lore raw

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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] serial: 8250: unbounded pause retry loop in serial8250_rx_dma_flush_work() --

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] serial: 8250: unbounded pause retry loop in serial8250_rx_dma_flush_work() --

commit 7216db0ad0fa3a64d9a52f2eff54fb53c9c43c75
Author: James Hilliard <[email protected]>

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

This commit reserves the RX DMA receive buffer until a worker has drained canceled callbacks and adds retry and recovery logic for RX DMA pause and termination failures.

Quoted 4 lines
drivers/tty/serial/8250/8250_dma.c +0 -0
index 344f0dd24b416..fd2a0ed211657 100644

[ ... ]

Quoted 26 lines
drivers/tty/serial/8250/8250_dma.c +26 -0
+/* 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;
+	}

[ ... ]

Quoted 29 lines
drivers/tty/serial/8250/8250_dma.c +29 -0
+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;
+	unsigned int delay_us = USEC_PER_MSEC;
+	ktime_t start = ktime_get();
+	bool recover, retry, restore_rx;
+	bool warned = false;
+	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;
+			/* Serialize reset with completion and shutdown notifications. */
+			WRITE_ONCE(dma->rx_pause_wakeup, false);
+			retry = dma->rx_pause_pending && !dma->rx_terminating;
+			if (retry) {
+				ret = serial8250_rx_dma_try_flush(p);
+				retry = dma->rx_pause_pending && !dma->rx_terminating;
+			}
+			recover = dma->rx_err;
+		}
+		if (!retry)
+			break;
+		if (!warned && ktime_ms_delta(ktime_get(), start) >= MSEC_PER_SEC) {

drivers/tty/serial/8250/8250_dma.c void serial8250_tx_dma_sync_buffer(struct uart_8250_port *p, · line 231

Quoted 10 lines
drivers/tty/serial/8250/8250_dma.c +10 -0
+			dev_warn_ratelimited(p->port.dev,
+					     "RX DMA pause stalled: %d; retaining transfer and retrying\n",
+					     ret);
+			warned = true;
+		}
+		/* Notifications also cover completion or shutdown before sleeping. */
+		wait_event_hrtimeout(dma->rx_pause_wait, READ_ONCE(dma->rx_pause_wakeup),
+				     ns_to_ktime(delay_us * NSEC_PER_USEC));
+		delay_us = min(delay_us * 2, USEC_PER_SEC);
+	}

[Severity: High]
As reported by Raul E Rangel on v8 [1], when dmaengine_pause(dma->rxchan) persistently returns an error during a partial RX DMA transfer (where fewer than dma->rx_size bytes have arrived and dmaengine_tx_status() remains DMA_IN_PROGRESS), serial8250_rx_dma_try_flush() sets dma->rx_pause_pending = true and masks RX interrupts via serial8250_out_IER().

In serial8250_rx_dma_flush_work(), this causes the for (;;) loop to retry serial8250_rx_dma_try_flush() indefinitely until port shutdown instead of escalating to serial8250_rx_dma_recover() after a bounded retry budget.

While a brief retry outside p->port.lock allows an already-completed transfer whose DMA interrupt was masked by p->port.lock to publish DMA_COMPLETE, once p->port.lock and local IRQs have been released across a few sleeps, any pending completion interrupt has already run. If dmaengine_pause() continues to fail while the remote peer is idle (for example, if dw-axi-dmac times out waiting for DWAXIDMAC_IRQ_SUSPENDED and leaves the channel hardware-suspended while returning -EAGAIN, or if tegra186-gpc-dma returns -ENOSYS when !hw_support_pause), dmaengine_tx_status() will never reach DMA_COMPLETE.

While dma->rx_pause_pending remains stuck:
- UART reception is permanently wedged until the port is closed because RX interrupts stay masked in UART_IER and serial8250_rx_dma_recovering() forces skip_rx = true in serial8250_handle_irq_locked(), preventing PIO fallback.
- serial8250_rx_dma_flush_work() holds a runtime PM reference via guard(serial8250_rpm)(p) for the lifetime of the open port.
- serial8250_rx_dma_flush() returns -EAGAIN, which causes dw8250_idle_enter() in drivers/tty/serial/8250/8250_dw.c to abort divisor and LCR updates:

drivers/tty/serial/8250/8250_dw.c:dw8250_idle_enter() {
	...
	if (up->dma) {
		ret = serial8250_rx_dma_flush(up);
		if (ret) {
			serial8250_out_IER(up, up->ier);
			d->in_idle = 0;
			return ret;
		}
	...
}

Since the recover path right after the loop in serial8250_rx_dma_flush_work() already calls dmaengine_terminate_sync(dma->rxchan) and retains the buffer with RX masked if synchronous termination fails, can this loop bound the number of pause retries and escalate to serial8250_rx_dma_recover(p) so the port can terminate the transfer and fall back to PIO?

[1] https://lore.kernel.org/r/[email protected]

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-submit-serial-8250-dma-cancel-v1-v9-0-9159caade148@gmail.com?part=7
#26 lore raw

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

Quoted 93 lines · On Mon, Nov 2, 2026 at 4:49 PM <[email protected]> wrote:

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] serial: 8250: unbounded pause retry loop in serial8250_rx_dma_flush_work() --

commit 7216db0ad0fa3a64d9a52f2eff54fb53c9c43c75
Author: James Hilliard <[email protected]>

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

This commit reserves the RX DMA receive buffer until a worker has drained canceled callbacks and adds retry and recovery logic for RX DMA pause and termination failures.

Quoted 4 lines
drivers/tty/serial/8250/8250_dma.c +0 -0
index 344f0dd24b416..fd2a0ed211657 100644

[ ... ]

Quoted 26 lines
drivers/tty/serial/8250/8250_dma.c +26 -0
+/* 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;
+     }

[ ... ]

Quoted 29 lines
drivers/tty/serial/8250/8250_dma.c +29 -0
+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;
+     unsigned int delay_us = USEC_PER_MSEC;
+     ktime_t start = ktime_get();
+     bool recover, retry, restore_rx;
+     bool warned = false;
+     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;
+                     /* Serialize reset with completion and shutdown notifications. */
+                     WRITE_ONCE(dma->rx_pause_wakeup, false);
+                     retry = dma->rx_pause_pending && !dma->rx_terminating;
+                     if (retry) {
+                             ret = serial8250_rx_dma_try_flush(p);
+                             retry = dma->rx_pause_pending && !dma->rx_terminating;
+                     }
+                     recover = dma->rx_err;
+             }
+             if (!retry)
+                     break;
+             if (!warned && ktime_ms_delta(ktime_get(), start) >= MSEC_PER_SEC) {

drivers/tty/serial/8250/8250_dma.c void serial8250_tx_dma_sync_buffer(struct uart_8250_port *p, · line 231

Quoted 10 lines
drivers/tty/serial/8250/8250_dma.c +10 -0
+                     dev_warn_ratelimited(p->port.dev,
+                                          "RX DMA pause stalled: %d; retaining transfer and retrying\n",
+                                          ret);
+                     warned = true;
+             }
+             /* Notifications also cover completion or shutdown before sleeping. */
+             wait_event_hrtimeout(dma->rx_pause_wait, READ_ONCE(dma->rx_pause_wakeup),
+                                  ns_to_ktime(delay_us * NSEC_PER_USEC));
+             delay_us = min(delay_us * 2, USEC_PER_SEC);
+     }

[Severity: High]
As reported by Raul E Rangel on v8 [1], when dmaengine_pause(dma->rxchan) persistently returns an error during a partial RX DMA transfer (where fewer than dma->rx_size bytes have arrived and dmaengine_tx_status() remains DMA_IN_PROGRESS), serial8250_rx_dma_try_flush() sets dma->rx_pause_pending = true and masks RX interrupts via serial8250_out_IER().

Quoted 3 lines

In serial8250_rx_dma_flush_work(), this causes the for (;;) loop to retry serial8250_rx_dma_try_flush() indefinitely until port shutdown instead of escalating to serial8250_rx_dma_recover() after a bounded retry budget.

The reason for retaining the descriptor is that a failed pause is not necessarily a failed transfer. Hardware can finish receiving before the provider publishes DMA_COMPLETE. At that point pause may fail because there is no active transfer left to pause, while the cookie still appears incomplete.

Treating that combination as fatal can discard an otherwise valid receive buffer and disable RX DMA until reopen. The current approach tries to preserve both the received data and the ability to continue using DMA when the failure is transient.

Without a successful pause or reported completion, the consumer does not yet have the stable receive snapshot needed for the normal flush. The worker therefore retains ownership and retries after releasing the port lock and enabling local interrupts, allowing the provider to publish completion. Once pause succeeds or completion is reported, it delivers the data and drains the old callbacks before buffer reuse.

The backoff limits repeated failed calls, while completion and shutdown wake the worker without waiting for the next retry timeout. It does not solve persistent pause failure: if the peer is idle and the provider never pauses or completes, reception remains stalled. Retaining the buffer is an ownership-safety measure, not a recovery guarantee.

Quoted 31 lines

While a brief retry outside p->port.lock allows an already-completed transfer whose DMA interrupt was masked by p->port.lock to publish DMA_COMPLETE, once p->port.lock and local IRQs have been released across a few sleeps, any pending completion interrupt has already run. If dmaengine_pause() continues to fail while the remote peer is idle (for example, if dw-axi-dmac times out waiting for DWAXIDMAC_IRQ_SUSPENDED and leaves the channel hardware-suspended while returning -EAGAIN, or if tegra186-gpc-dma returns -ENOSYS when !hw_support_pause), dmaengine_tx_status() will never reach DMA_COMPLETE.

While dma->rx_pause_pending remains stuck:
- UART reception is permanently wedged until the port is closed because RX interrupts stay masked in UART_IER and serial8250_rx_dma_recovering() forces skip_rx = true in serial8250_handle_irq_locked(), preventing PIO fallback.
- serial8250_rx_dma_flush_work() holds a runtime PM reference via guard(serial8250_rpm)(p) for the lifetime of the open port.
- serial8250_rx_dma_flush() returns -EAGAIN, which causes dw8250_idle_enter() in drivers/tty/serial/8250/8250_dw.c to abort divisor and LCR updates:

drivers/tty/serial/8250/8250_dw.c:dw8250_idle_enter() {
        ...
        if (up->dma) {
                ret = serial8250_rx_dma_flush(up);
                if (ret) {
                        serial8250_out_IER(up, up->ier);
                        d->in_idle = 0;
                        return ret;
                }
        ...
}
Quoted 5 lines

Since the recover path right after the loop in serial8250_rx_dma_flush_work() already calls dmaengine_terminate_sync(dma->rxchan) and retains the buffer with RX masked if synchronous termination fails, can this loop bound the number of pause retries and escalate to serial8250_rx_dma_recover(p) so the port can terminate the transfer and fall back to PIO?

Successful synchronous termination would make subsequent buffer reuse and PIO access safe under the DMAengine contract. The concern is not that termination cannot establish safety, but that this recovery path discards the pending data and sets rx_err until reopen.

A retry budget would make that an explicit, potentially lossy recovery policy. It would not distinguish a permanent failure from a transient one. However, continuing to retain ownership indefinitely also fails to provide useful reception when the provider cannot recover.

The Tegra186 case should also be distinguished from a transient timeout:
advertising cmd_pause when the hardware cannot implement it allows generic 8250 to select a DMA configuration whose requirements are not met.

Quoted 5 lines

[1] https://lore.kernel.org/r/[email protected]

--
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-submit-serial-8250-dma-cancel-v1-v9-0-9159caade148@gmail.com?part=7

#27 lore raw

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

In reply to James Hilliard

Hi James,

Thanks for v9 — fixing asc_startup() locking in Patch 2/8, waking tty->write_wait for drain waiters in Patch 5/8, splitting OMAP RX locking and IRQ quiescing into Patches 3/8 and 4/8, and adding rx_pause_wait with immediate wakeups from dma_rx_complete() and serial8250_do_shutdown() all look good. Waking rx_pause_wait under port->lock cleanly avoids delaying normal completion or cancel_work_sync(&dma->rx_flush_work) during shutdown.

Two remaining comments on Patch 7/8:

1. Escalating to serial8250_rx_dma_recover() after a bounded pause-stall window on partial RX transfers:

The commit message notes:

Quoted 3 lines

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.

I completely agree that a timeout alone cannot establish that *reusing* or *freeing* dma->rx_buf is safe while the channel is still running. However, calling serial8250_rx_dma_recover(p) under p->port.lock once the retry budget expires does *not* reuse or free dma->rx_buf without stopping the channel:

- Calling serial8250_rx_dma_recover(p) under p->port.lock inside the

     worker's retry block sets dma->rx_err = true and
     dma->rx_terminating = true (and keeps RX masked in UART_IER), which
     causes serial8250_rx_dma_flush_work() to exit the for (;;) loop
     (retry = false, recover = true) and call
     dmaengine_terminate_sync(dma->rxchan) outside p->port.lock in process
     context (8250_dma.c:233-239).
   - If dmaengine_terminate_sync(dma->rxchan) succeeds (ret == 0), the
     channel is stopped and its callbacks are drained, so clearing
     rx_pause_pending/rx_terminating and restoring p->ier safely falls back
     to PIO ("RX DMA failed, using PIO\n").
   - If dmaengine_terminate_sync(dma->rxchan) also fails (ret != 0),
     lines 235-239 already retain dma->rx_buf and return early with RX
     interrupts masked.

Why does this matter in practice?
Look at drivers/dma/tegra186-gpc-dma.c:
- Line 1521 unconditionally assigns .device_pause = tegra_dma_device_pause, so dma_get_slave_caps() (drivers/dma/dmaengine.c:614) sets caps.cmd_pause = true and serial8250_request_dma() enables RX DMA.
- On Tegra186, tegra186_dma_chip_data sets .hw_support_pause = false (line 1351), so tegra_dma_device_pause() (lines 420-423) unconditionally returns -ENOSYS. (On Tegra194/234/264, tegra_dma_pause() at lines 389-414 polls TEGRA_GPCDMA_STATUS_BUSY after setting TEGRA_GPCDMA_CHAN_CSRE_PAUSE for 20 us and returns -ETIMEDOUT if the channel does not pause.)

When an RX timeout interrupt (UART_IIR_RX_TIMEOUT) fires on a partial transfer (e.g. a few bytes typed at an interactive console or a short command waiting for a reply, count < 4096), hardware transfer completion will never occur while the remote peer is waiting for our response. With an unbounded for (;;) loop:
- Those partial bytes in dma->rx_buf are never delivered anyway (even if we wait forever until the user closes the port, serial8250_release_dma() terminates the channel without copying dma->rx_buf).
- UART RX remains permanently dead with RLSI/RDI masked and never falls back to PIO.
- serial8250_rx_dma_flush_work() holds a runtime PM reference (guard(serial8250_rpm)(p)) for the lifetime of the open port.
- On 8250_dw, dw8250_idle_enter() (8250_dw.c:151) sees

     serial8250_rx_dma_flush(up) == -EAGAIN and silently aborts
     dw8250_set_divisor() and dw8250_check_lcr(), breaking baud-rate and
     LCR updates while rx_pause_pending is stuck.

If serial8250_rx_dma_flush_work() escalates to serial8250_rx_dma_recover(p) under p->port.lock once the stall threshold is reached (or immediately when dmaengine_pause() returns -ENOSYS / -EOPNOTSUPP), dmaengine_terminate_sync() safely stops the channel before any buffer state is released and the UART recovers on PIO instead of wedging until close().

2. Minor nit in drivers/tty/serial/8250/8250_omap.c:783-784:
In omap_8250_shutdown():

     WRITE_ONCE(up->dma->rx_pause_wakeup, true);
     wake_up(&up->dma->rx_pause_wait);

8250_omap uses its own synchronous omap_8250_rx_dma_flush() and never schedules rx_flush_work or sleeps on rx_pause_wait, so those two lines in omap_8250_shutdown() can be dropped.

Thanks,
Raul

#28 lore raw

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

Quoted 20 lines · Raul E Rangel

Hi James,

Thanks for v9 — fixing asc_startup() locking in Patch 2/8, waking tty->write_wait for drain waiters in Patch 5/8, splitting OMAP RX locking and IRQ quiescing into Patches 3/8 and 4/8, and adding rx_pause_wait with immediate wakeups from dma_rx_complete() and serial8250_do_shutdown() all look good. Waking rx_pause_wait under port->lock cleanly avoids delaying normal completion or cancel_work_sync(&dma->rx_flush_work) during shutdown.

Two remaining comments on Patch 7/8:

1. Escalating to serial8250_rx_dma_recover() after a bounded pause-stall window on partial RX transfers:

The commit message notes:

Quoted 3 lines

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.

Quoted 5 lines

I completely agree that a timeout alone cannot establish that *reusing* or *freeing* dma->rx_buf is safe while the channel is still running. However, calling serial8250_rx_dma_recover(p) under p->port.lock once the retry budget expires does *not* reuse or free dma->rx_buf without stopping the channel:

Successful dmaengine_terminate_sync() establishes the required stop and callback-draining guarantees. The concern is not that the buffer remains unsafe after successful termination, but that this recovery path discards the pending receive data and disables RX DMA until reopen.

A failed pause followed by an incomplete status can be temporary, including when completion has not yet been published. Retrying preserves the chance to deliver that transfer without switching to PIO. Memory safety after termination and the decision to discard the transfer are separate issues.

Quoted 15 lines

- Calling serial8250_rx_dma_recover(p) under p->port.lock inside the

     worker's retry block sets dma->rx_err = true and
     dma->rx_terminating = true (and keeps RX masked in UART_IER), which
     causes serial8250_rx_dma_flush_work() to exit the for (;;) loop
     (retry = false, recover = true) and call
     dmaengine_terminate_sync(dma->rxchan) outside p->port.lock in process
     context (8250_dma.c:233-239).
   - If dmaengine_terminate_sync(dma->rxchan) succeeds (ret == 0), the
     channel is stopped and its callbacks are drained, so clearing
     rx_pause_pending/rx_terminating and restoring p->ier safely falls back
     to PIO ("RX DMA failed, using PIO\n").
   - If dmaengine_terminate_sync(dma->rxchan) also fails (ret != 0),
     lines 235-239 already retain dma->rx_buf and return early with RX
     interrupts masked.
Quoted 11 lines

Why does this matter in practice?
Look at drivers/dma/tegra186-gpc-dma.c:
- Line 1521 unconditionally assigns .device_pause = tegra_dma_device_pause, so dma_get_slave_caps() (drivers/dma/dmaengine.c:614) sets caps.cmd_pause = true and serial8250_request_dma() enables RX DMA.
- On Tegra186, tegra186_dma_chip_data sets .hw_support_pause = false (line 1351), so tegra_dma_device_pause() (lines 420-423) unconditionally returns -ENOSYS. (On Tegra194/234/264, tegra_dma_pause() at lines 389-414 polls TEGRA_GPCDMA_STATUS_BUSY after setting TEGRA_GPCDMA_CHAN_CSRE_PAUSE for 20 us and returns -ETIMEDOUT if the channel does not pause.)

The Tegra186 case is a permanent capability mismatch. dma_get_slave_caps() derives cmd_pause from the presence of device_pause, but this provider installs that callback even when hw_support_pause is false.

serial8250_request_dma() already rejects RX channels without cmd_pause. Correcting the provider's capability reporting would prevent this unsupported configuration from being enabled in the first place, rather than discovering it after receiving data. That is different from a pause timeout on hardware which supports the operation.

Quoted 8 lines

When an RX timeout interrupt (UART_IIR_RX_TIMEOUT) fires on a partial transfer (e.g. a few bytes typed at an interactive console or a short command waiting for a reply, count < 4096), hardware transfer completion will never occur while the remote peer is waiting for our response. With an unbounded for (;;) loop:
- Those partial bytes in dma->rx_buf are never delivered anyway (even if we wait forever until the user closes the port, serial8250_release_dma() terminates the channel without copying dma->rx_buf).

For a provider which never pauses or completes, retries cannot restore reception. A request/response peer can indeed leave the descriptor incomplete indefinitely, and closing the port then loses those bytes. Backoff and diagnostics limit the retry cost and expose the stall; they do not solve that liveness problem.

For pause-capable hardware, a recovery timeout does not have to prove that the failure is permanent. The concern is what happens after that
timeout: the current recovery path discards the pending transfer and leaves RX on PIO until reopen.

Is there a recovery approach that avoids an indefinite RX stall without turning a potentially temporary pause failure into loss of DMA for the rest of the open session?

Quoted 25 lines

- UART RX remains permanently dead with RLSI/RDI masked and never falls back to PIO.
- serial8250_rx_dma_flush_work() holds a runtime PM reference (guard(serial8250_rpm)(p)) for the lifetime of the open port.
- On 8250_dw, dw8250_idle_enter() (8250_dw.c:151) sees

     serial8250_rx_dma_flush(up) == -EAGAIN and silently aborts
     dw8250_set_divisor() and dw8250_check_lcr(), breaking baud-rate and
     LCR updates while rx_pause_pending is stuck.

If serial8250_rx_dma_flush_work() escalates to serial8250_rx_dma_recover(p) under p->port.lock once the stall threshold is reached (or immediately when dmaengine_pause() returns -ENOSYS / -EOPNOTSUPP), dmaengine_terminate_sync() safely stops the channel before any buffer state is released and the UART recovers on PIO instead of wedging until close().

2. Minor nit in drivers/tty/serial/8250/8250_omap.c:783-784:
In omap_8250_shutdown():

     WRITE_ONCE(up->dma->rx_pause_wakeup, true);
     wake_up(&up->dma->rx_pause_wait);

8250_omap uses its own synchronous omap_8250_rx_dma_flush() and never schedules rx_flush_work or sleeps on rx_pause_wait, so those two lines in omap_8250_shutdown() can be dropped.

Thanks,
Raul

#29 lore raw

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

In reply to James Hilliard
Quoted 64 lines · 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.

Start pause retries at 1 ms and double the delay up to one second to limit CPU use during persistent failure. Sleep outside the port lock on a timed waitqueue, with completion and shutdown publishing a wakeup under the same lock as the worker's retry check. This also catches notifications arriving just before the worker sleeps, avoiding a full backoff delay after completion or during shutdown. Reset the delay for each new flush. Report a stall lasting at least one second once per flush, rate-limited across repeated stalls. 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.

...

drivers/tty/serial/8250/8250.h static inline void serial_out(struct uart_8250_port *up, int offset, int value) · line 169

Quoted 9 lines
drivers/tty/serial/8250/8250.h +9 -0
+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));

Slightly better formatting is to put some line breaks.

	return up->dma &&
	       (up->dma->rx_pause_pending || (up->dma->rx_err && up->dma->rx_terminating));

Alternatively

	if (!up->dma)
		return false;

	return up->dma->rx_pause_pending || (up->dma->rx_err && up->dma->rx_terminating);
Quoted 13 lines
drivers/tty/serial/8250/8250.h +12 -0
+}
 
+/* 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);
+}

...

Quoted 6 lines
drivers/tty/serial/8250/8250_dma.c +0 -5
 	struct tty_port		*tty_port = &p->port.state->port;
-	struct dma_tx_state	state;
-	enum dma_status		dma_status;
-	int			count;
-
-	/*

drivers/tty/serial/8250/8250_dma.c void serial8250_tx_dma_sync_buffer(struct uart_8250_port *p, · line 97

Quoted 10 lines
drivers/tty/serial/8250/8250_dma.c +1 -9
-	 * 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;

No need to put too many spaces between type and variable name. (Yes, the first one will be left untouched.)

...

Quoted 22 lines
drivers/tty/serial/8250/8250_dma.c +22 -0
+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;
+	unsigned int delay_us = USEC_PER_MSEC;
+	ktime_t start = ktime_get();
+	bool recover, retry, restore_rx;
+	bool warned = false;
+	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;
+			/* Serialize reset with completion and shutdown notifications. */
+			WRITE_ONCE(dma->rx_pause_wakeup, false);
+			retry = dma->rx_pause_pending && !dma->rx_terminating;
+			if (retry) {
+				ret = serial8250_rx_dma_try_flush(p);

drivers/tty/serial/8250/8250_dma.c void serial8250_tx_dma_sync_buffer(struct uart_8250_port *p, · line 224

Quoted 10 lines
drivers/tty/serial/8250/8250_dma.c +10 -0
+				retry = dma->rx_pause_pending && !dma->rx_terminating;
+			}
+			recover = dma->rx_err;
+		}
+		if (!retry)
+			break;
+		if (!warned && ktime_ms_delta(ktime_get(), start) >= MSEC_PER_SEC) {
+			dev_warn_ratelimited(p->port.dev,
+					     "RX DMA pause stalled: %d; retaining transfer and retrying\n",
+					     ret);

_ratelimited() is a slop of AI without understanding the code. It makes nonsense in combination with warned.

drivers/tty/serial/8250/8250_dma.c void serial8250_tx_dma_sync_buffer(struct uart_8250_port *p, · line 225

Quoted 1 line
drivers/tty/serial/8250/8250_dma.c +1 -0
+			warned = true;

Altogether NIH _once().

Quoted 35 lines
drivers/tty/serial/8250/8250_dma.c +34 -0
+		}
+		/* Notifications also cover completion or shutdown before sleeping. */
+		wait_event_hrtimeout(dma->rx_pause_wait, READ_ONCE(dma->rx_pause_wakeup),
+				     ns_to_ktime(delay_us * NSEC_PER_USEC));
+		delay_us = min(delay_us * 2, USEC_PER_SEC);
+	}
+
+	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");
 }

...

Quoted 4 lines
drivers/tty/serial/8250/8250_dma.c +1 -1
-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;

drivers/tty/serial/8250/8250_dma.c int serial8250_rx_dma(struct uart_8250_port *p) · line 399

Quoted 10 lines
drivers/tty/serial/8250/8250_dma.c +3 -5
+	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)

You introduced a helper serial8250_rx_dma_running(). Use it.

Quoted 14 lines
drivers/tty/serial/8250/8250_dma.c +12 -0
+		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);

...

drivers/tty/serial/8250/8250_dw.c static int dw8250_handle_irq(struct uart_port *p) · line 469

Quoted 5 lines
drivers/tty/serial/8250/8250_dw.c +3 -1
 	/* 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) {

Re-wrap it logically (also be consistent with parentheses when & versus && is used).

	if ((quirks & DW_UART_QUIRK_IS_DMA_FC) && rx_timeout &&
	    serial8250_rx_dma_running(up) &&
	    !up->dma->rx_terminating && !up->dma->rx_pause_pending) {
Quoted 1 line

}

Signature · 2 lines
With Best Regards,
Andy Shevchenko
#30 lore raw

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

Quoted 156 lines · Andy Shevchenko
Quoted 64 lines · 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.

Start pause retries at 1 ms and double the delay up to one second to limit CPU use during persistent failure. Sleep outside the port lock on a timed waitqueue, with completion and shutdown publishing a wakeup under the same lock as the worker's retry check. This also catches notifications arriving just before the worker sleeps, avoiding a full backoff delay after completion or during shutdown. Reset the delay for each new flush. Report a stall lasting at least one second once per flush, rate-limited across repeated stalls. 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.

...

drivers/tty/serial/8250/8250.h static inline void serial_out(struct uart_8250_port *up, int offset, int value) · line 169

Quoted 9 lines
drivers/tty/serial/8250/8250.h +9 -0
+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));

Slightly better formatting is to put some line breaks.

        return up->dma &&
               (up->dma->rx_pause_pending || (up->dma->rx_err && up->dma->rx_terminating));

Alternatively

        if (!up->dma)
                return false;

        return up->dma->rx_pause_pending || (up->dma->rx_err && up->dma->rx_terminating);
Quoted 13 lines
drivers/tty/serial/8250/8250.h +12 -0
+}
 
+/* 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);
+}

...

Quoted 6 lines
      struct tty_port         *tty_port = &p->port.state->port;
drivers/tty/serial/8250/8250_dma.c +0 -5
-     struct dma_tx_state     state;
-     enum dma_status         dma_status;
-     int                     count;
-
-     /*

drivers/tty/serial/8250/8250_dma.c void serial8250_tx_dma_sync_buffer(struct uart_8250_port *p, · line 97

Quoted 10 lines
drivers/tty/serial/8250/8250_dma.c +1 -9
-      * 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;

No need to put too many spaces between type and variable name. (Yes, the first one will be left untouched.)

...

Quoted 27 lines
drivers/tty/serial/8250/8250_dma.c +27 -0
+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;
+     unsigned int delay_us = USEC_PER_MSEC;
+     ktime_t start = ktime_get();
+     bool recover, retry, restore_rx;
+     bool warned = false;
+     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;
+                     /* Serialize reset with completion and shutdown notifications. */
+                     WRITE_ONCE(dma->rx_pause_wakeup, false);
+                     retry = dma->rx_pause_pending && !dma->rx_terminating;
+                     if (retry) {
+                             ret = serial8250_rx_dma_try_flush(p);
+                             retry = dma->rx_pause_pending && !dma->rx_terminating;
+                     }
+                     recover = dma->rx_err;
+             }
+             if (!retry)
Quoted 8 lines

drivers/tty/serial/8250/8250_dma.c void serial8250_tx_dma_sync_buffer(struct uart_8250_port *p, · line 224

Quoted 5 lines
drivers/tty/serial/8250/8250_dma.c +5 -0
+                     break;
+             if (!warned && ktime_ms_delta(ktime_get(), start) >= MSEC_PER_SEC) {
+                     dev_warn_ratelimited(p->port.dev,
+                                          "RX DMA pause stalled: %d; retaining transfer and retrying\n",
+                                          ret);

_ratelimited() is a slop of AI without understanding the code. It makes nonsense in combination with warned.

The two controls have different scopes. warned is local to one invocation of the flush worker and prevents repeated warning attempts while that flush remains stalled. A later flush starts with warned cleared again.

The rate limit applies across worker invocations and UART ports. It limits messages from separate stalls, rather than repeated retries of the same stall.

Quoted 4 lines
Quoted 2 lines

+ warned = true;

Altogether NIH _once().

dev_warn_once() uses a static flag at the call site, shared by all ports. After the first warning, subsequent stalls would remain silent even after recovery or reopening the port, including stalls on another UART.

The intended behavior here is at most one warning attempt per prolonged stall, while keeping later incidents diagnosable and limiting their aggregate logging rate.

Quoted 93 lines
Quoted 36 lines
drivers/tty/serial/8250/8250_dma.c +34 -0
+             }
+             /* Notifications also cover completion or shutdown before sleeping. */
+             wait_event_hrtimeout(dma->rx_pause_wait, READ_ONCE(dma->rx_pause_wakeup),
+                                  ns_to_ktime(delay_us * NSEC_PER_USEC));
+             delay_us = min(delay_us * 2, USEC_PER_SEC);
+     }
+
+     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");
 }

...

Quoted 4 lines
drivers/tty/serial/8250/8250_dma.c +1 -1
-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;

drivers/tty/serial/8250/8250_dma.c int serial8250_rx_dma(struct uart_8250_port *p) · line 399

Quoted 10 lines
drivers/tty/serial/8250/8250_dma.c +3 -5
+     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)

You introduced a helper serial8250_rx_dma_running(). Use it.

Quoted 14 lines
drivers/tty/serial/8250/8250_dma.c +12 -0
+             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);

...

drivers/tty/serial/8250/8250_dw.c static int dw8250_handle_irq(struct uart_port *p) · line 469

Quoted 5 lines
      /* Manually stop the Rx DMA transfer when acting as flow controller */
drivers/tty/serial/8250/8250_dw.c +3 -1
-     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) {

Re-wrap it logically (also be consistent with parentheses when & versus && is used).

        if ((quirks & DW_UART_QUIRK_IS_DMA_FC) && rx_timeout &&
            serial8250_rx_dma_running(up) &&
            !up->dma->rx_terminating && !up->dma->rx_pause_pending) {
Quoted 1 line
      }

--
With Best Regards,
Andy Shevchenko

#31 lore raw

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

Quoted 3 lines · James Hilliard
Quoted 1 line · Andy Shevchenko

On Mon, Nov 02, 2026 at 04:14:34PM -0700, James Hilliard wrote:

...

Quoted 36 lines
Quoted 27 lines
Quoted 27 lines
drivers/tty/serial/8250/8250_dma.c +27 -0
+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;
+     unsigned int delay_us = USEC_PER_MSEC;
+     ktime_t start = ktime_get();
+     bool recover, retry, restore_rx;
+     bool warned = false;
+     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;
+                     /* Serialize reset with completion and shutdown notifications. */
+                     WRITE_ONCE(dma->rx_pause_wakeup, false);
+                     retry = dma->rx_pause_pending && !dma->rx_terminating;
+                     if (retry) {
+                             ret = serial8250_rx_dma_try_flush(p);
+                             retry = dma->rx_pause_pending && !dma->rx_terminating;
+                     }
+                     recover = dma->rx_err;
+             }
+             if (!retry)
Quoted 8 lines

drivers/tty/serial/8250/8250_dma.c void serial8250_tx_dma_sync_buffer(struct uart_8250_port *p, · line 224

Quoted 5 lines
drivers/tty/serial/8250/8250_dma.c +5 -0
+                     break;
+             if (!warned && ktime_ms_delta(ktime_get(), start) >= MSEC_PER_SEC) {
+                     dev_warn_ratelimited(p->port.dev,
+                                          "RX DMA pause stalled: %d; retaining transfer and retrying\n",
+                                          ret);

_ratelimited() is a slop of AI without understanding the code. It makes nonsense in combination with warned.

Quoted 7 lines

The two controls have different scopes. warned is local to one invocation of the flush worker and prevents repeated warning attempts while that flush remains stalled. A later flush starts with warned cleared again.

The rate limit applies across worker invocations and UART ports. It limits messages from separate stalls, rather than repeated retries of the same stall.

How? The whole _ratelimited() is nonsense here.

Quoted 11 lines
Quoted 3 lines
Quoted 1 line

+ warned = true;

Altogether NIH _once().

dev_warn_once() uses a static flag at the call site, shared by all ports. After the first warning, subsequent stalls would remain silent even after recovery or reopening the port, including stalls on another UART.

The intended behavior here is at most one warning attempt per prolonged stall, while keeping later incidents diagnosable and limiting their aggregate logging rate.

Yes, so what the point to use _ratelimited()?

Quoted 35 lines
Quoted 35 lines
Quoted 35 lines
drivers/tty/serial/8250/8250_dma.c +34 -0
+             }
+             /* Notifications also cover completion or shutdown before sleeping. */
+             wait_event_hrtimeout(dma->rx_pause_wait, READ_ONCE(dma->rx_pause_wakeup),
+                                  ns_to_ktime(delay_us * NSEC_PER_USEC));
+             delay_us = min(delay_us * 2, USEC_PER_SEC);
+     }
+
+     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");
 }
Signature · 2 lines
With Best Regards,
Andy Shevchenko
#32 lore raw

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

Quoted 48 lines · Andy Shevchenko
Quoted 3 lines · James Hilliard
Quoted 1 line · Andy Shevchenko

On Mon, Nov 02, 2026 at 04:14:34PM -0700, James Hilliard wrote:

...

Quoted 40 lines
Quoted 27 lines
Quoted 27 lines
drivers/tty/serial/8250/8250_dma.c +27 -0
+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;
+     unsigned int delay_us = USEC_PER_MSEC;
+     ktime_t start = ktime_get();
+     bool recover, retry, restore_rx;
+     bool warned = false;
+     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;
+                     /* Serialize reset with completion and shutdown notifications. */
+                     WRITE_ONCE(dma->rx_pause_wakeup, false);
+                     retry = dma->rx_pause_pending && !dma->rx_terminating;
+                     if (retry) {
+                             ret = serial8250_rx_dma_try_flush(p);
+                             retry = dma->rx_pause_pending && !dma->rx_terminating;
+                     }
+                     recover = dma->rx_err;
+             }
+             if (!retry)
Quoted 8 lines

drivers/tty/serial/8250/8250_dma.c void serial8250_tx_dma_sync_buffer(struct uart_8250_port *p, · line 224

Quoted 5 lines
drivers/tty/serial/8250/8250_dma.c +5 -0
+                     break;
+             if (!warned && ktime_ms_delta(ktime_get(), start) >= MSEC_PER_SEC) {
+                     dev_warn_ratelimited(p->port.dev,
+                                          "RX DMA pause stalled: %d; retaining transfer and retrying\n",
+                                          ret);

_ratelimited() is a slop of AI without understanding the code. It makes nonsense in combination with warned.

The two controls have different scopes. warned is local to one invocation of the flush worker and prevents repeated warning attempts while that flush remains stalled. A later flush starts with warned cleared again.

Quoted 5 lines
Quoted 3 lines

The rate limit applies across worker invocations and UART ports. It limits messages from separate stalls, rather than repeated retries of the same stall.

How? The whole _ratelimited() is nonsense here.

dev_warn_ratelimited() uses a static ratelimit_state at the call site, shared by all ports. The intent was to limit warning bursts across ports and separate flushes, while warned only covers one flush.

Quoted 9 lines
Quoted 9 lines
Quoted 4 lines
Quoted 2 lines

+ warned = true;

Altogether NIH _once().

dev_warn_once() uses a static flag at the call site, shared by all ports. After the first warning, subsequent stalls would remain silent even after recovery or reopening the port, including stalls on another UART.

Quoted 5 lines
Quoted 3 lines

The intended behavior here is at most one warning attempt per prolonged stall, while keeping later incidents diagnosable and limiting their aggregate logging rate.

Yes, so what the point to use _ratelimited()?

It was intended to limit warning bursts across multiple ports and separate stalls, while warned prevented repeated messages for one stalled flush.

I'll use dev_warn() with the per-flush warned flag and one-second threshold instead. That keeps one warning per prolonged stall and lets each port report its own failure independently.

Quoted 42 lines
Quoted 28 lines
Quoted 28 lines
Quoted 28 lines
drivers/tty/serial/8250/8250_dma.c +27 -0
+             }
+             /* Notifications also cover completion or shutdown before sleeping. */
+             wait_event_hrtimeout(dma->rx_pause_wait, READ_ONCE(dma->rx_pause_wakeup),
+                                  ns_to_ktime(delay_us * NSEC_PER_USEC));
+             delay_us = min(delay_us * 2, USEC_PER_SEC);
+     }
+
+     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)
Quoted 8 lines
Quoted 8 lines

drivers/tty/serial/8250/8250_dma.c void serial8250_tx_dma_sync_buffer(struct uart_8250_port *p, · line 260

Quoted 8 lines
drivers/tty/serial/8250/8250_dma.c +7 -0
+                     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");
 }

--
With Best Regards,
Andy Shevchenko

#33 lore raw

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

Quoted 6 lines · James Hilliard
Quoted 4 lines · Andy Shevchenko
Quoted 3 lines · James Hilliard
Quoted 1 line · Andy Shevchenko

On Mon, Nov 02, 2026 at 04:14:34PM -0700, James Hilliard wrote:

...

Quoted 44 lines
Quoted 44 lines
Quoted 43 lines
Quoted 27 lines
Quoted 27 lines
drivers/tty/serial/8250/8250_dma.c +27 -0
+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;
+     unsigned int delay_us = USEC_PER_MSEC;
+     ktime_t start = ktime_get();
+     bool recover, retry, restore_rx;
+     bool warned = false;
+     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;
+                     /* Serialize reset with completion and shutdown notifications. */
+                     WRITE_ONCE(dma->rx_pause_wakeup, false);
+                     retry = dma->rx_pause_pending && !dma->rx_terminating;
+                     if (retry) {
+                             ret = serial8250_rx_dma_try_flush(p);
+                             retry = dma->rx_pause_pending && !dma->rx_terminating;
+                     }
+                     recover = dma->rx_err;
+             }
+             if (!retry)
Quoted 8 lines

drivers/tty/serial/8250/8250_dma.c void serial8250_tx_dma_sync_buffer(struct uart_8250_port *p, · line 224

Quoted 5 lines
drivers/tty/serial/8250/8250_dma.c +5 -0
+                     break;
+             if (!warned && ktime_ms_delta(ktime_get(), start) >= MSEC_PER_SEC) {
+                     dev_warn_ratelimited(p->port.dev,
+                                          "RX DMA pause stalled: %d; retaining transfer and retrying\n",
+                                          ret);

_ratelimited() is a slop of AI without understanding the code. It makes nonsense in combination with warned.

The two controls have different scopes. warned is local to one invocation of the flush worker and prevents repeated warning attempts while that flush remains stalled. A later flush starts with warned cleared again.

The rate limit applies across worker invocations and UART ports. It limits messages from separate stalls, rather than repeated retries of the same stall.

Quoted 5 lines
Quoted 1 line

How? The whole _ratelimited() is nonsense here.

dev_warn_ratelimited() uses a static ratelimit_state at the call site, shared by all ports. The intent was to limit warning bursts across ports and separate flushes, while warned only covers one flush.

Right and imagine if the two or more devices are trying to printing the same message in parallel. What will this give to the user? Something like

	$DEVICE: ...error message...
	$FUNC: N callbacks suppressed

And one may not know how many devices are affected by this N callbacks that suppressed.

Quoted 14 lines
Quoted 8 lines
Quoted 8 lines
Quoted 3 lines
Quoted 1 line

+ warned = true;

Altogether NIH _once().

dev_warn_once() uses a static flag at the call site, shared by all ports. After the first warning, subsequent stalls would remain silent even after recovery or reopening the port, including stalls on another UART.

Quoted 5 lines
Quoted 3 lines

The intended behavior here is at most one warning attempt per prolonged stall, while keeping later incidents diagnosable and limiting their aggregate logging rate.

Yes, so what the point to use _ratelimited()?

Quoted 7 lines

It was intended to limit warning bursts across multiple ports and separate stalls, while warned prevented repeated messages for one stalled flush.

I'll use dev_warn() with the per-flush warned flag and one-second threshold instead. That keeps one warning per prolonged stall and lets each port report its own failure independently.

This sounds better.

Quoted 35 lines
Quoted 35 lines
Quoted 35 lines
Quoted 35 lines
Quoted 35 lines
drivers/tty/serial/8250/8250_dma.c +34 -0
+             }
+             /* Notifications also cover completion or shutdown before sleeping. */
+             wait_event_hrtimeout(dma->rx_pause_wait, READ_ONCE(dma->rx_pause_wakeup),
+                                  ns_to_ktime(delay_us * NSEC_PER_USEC));
+             delay_us = min(delay_us * 2, USEC_PER_SEC);
+     }
+
+     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");
 }
Signature · 2 lines
With Best Regards,
Andy Shevchenko
#34 lore raw

PATCH v9 8/8 serial: 8250: retain DMA resources when termination fails

In reply to 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.

Start retries with a 1 ms sleep and double it up to one second. This makes short-lived failures responsive while bounding sustained retry activity; neither delay is a hardware timeout. Report the initial failure, rate-limit ongoing failures with the elapsed time and current RX/TX error codes, and report recovery once both channels have stopped and drained.

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")
---
 drivers/tty/serial/8250/8250_dma.c | 43 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 41 insertions(+), 2 deletions(-)
drivers/tty/serial/8250/8250_dma.c +41 -2
index fd2a0ed21165..6307d0261ceb 100644
@@ -530,6 +530,46 @@ 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;
+	unsigned int delay_us = USEC_PER_MSEC;
+	ktime_t start;
+	int rx_ret, tx_ret;
+
+	rx_ret = dmaengine_terminate_sync(dma->rxchan);
+	tx_ret = dmaengine_terminate_sync(dma->txchan);
+	if (!rx_ret && !tx_ret)
+		return;
+
+	start = ktime_get();
+	dev_err(p->port.dev,
+		"DMA termination failed: RX %d, TX %d; teardown blocked\n",
+		rx_ret, tx_ret);
+
+	/*
+	 * 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) {
+		fsleep(delay_us);
+		/* Retry promptly at first, then limit sustained retry activity. */
+		delay_us = min(delay_us * 2, USEC_PER_SEC);
+		if (rx_ret)
+			rx_ret = dmaengine_terminate_sync(dma->rxchan);
+		if (tx_ret)
+			tx_ret = dmaengine_terminate_sync(dma->txchan);
+		if (rx_ret || tx_ret)
+			dev_err_ratelimited(p->port.dev,
+					    "DMA termination still failing after %lld ms: RX %d, TX %d\n",
+					    ktime_ms_delta(ktime_get(), start), rx_ret, tx_ret);
+	}
+	dev_info(p->port.dev, "DMA termination recovered after %lld ms\n",
+		 ktime_ms_delta(ktime_get(), start));
+}
+
 void serial8250_release_dma(struct uart_8250_port *p)
 {
 	struct uart_8250_dma *dma = p->dma;
@@ -539,9 +579,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;
@@ -553,7 +593,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);
Signature · 1 line
2.53.0
#35 lore raw

Re: [PATCH v9 8/8] serial: 8250: retain DMA resources when termination fails

Sashiko has reviewed this patch and found no issues.
Quoted 22 lines

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.

Start retries with a 1 ms sleep and double it up to one second. This makes short-lived failures responsive while bounding sustained retry activity; neither delay is a hardware timeout. Report the initial failure, rate-limit ongoing failures with the elapsed time and current RX/TX error codes, and report recovery once both channels have stopped and drained.

Quoted 6 lines

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

Sashiko has reviewed this patch and found no issues.

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-submit-serial-8250-dma-cancel-v1-v9-0-9159caade148@gmail.com?part=8
#36 lore raw

Re: [PATCH v9 8/8] serial: 8250: retain DMA resources when termination fails

In reply to James Hilliard

Hi James,

Thanks for replacing the fixed 100 ms sleep with exponential backoff (1 ms -> 1 s) and adding the initial/ongoing/recovery diagnostics in v9.

Following up on our v8 discussion regarding blocking in serial8250_terminate_dma() versus non-blocking resource quarantine:

1. Why non-blocking quarantine cannot be done purely inside 8250_dma:
I agree with your v8 reply and the updated commit message here that serial8250_release_dma() cannot simply return early while leaving serial_core to proceed with normal teardown:

a) Immediately after serial8250_release_dma(up) returns in

      serial8250_do_shutdown() (8250_port.c:2474-2511, and similarly in
      omap_8250_shutdown() at 8250_omap.c:794-803), the 8250 driver itself
      clears the hardware FIFOs (serial8250_clear_fifos()), reads UART_RX,
      frees the IRQ (up->ops->release_irq(up)), and drops runtime PM
      (serial8250_rpm_put_tx(up) / serial8250_rpm_put(up)).
   b) In uart_tty_port_shutdown() (serial_core.c:1841-1853), immediately
      after uart_port_shutdown(port) returns, serial_core unconditionally
      calls uart_free_xmit_buf(port) (line 1850) — freeing the page that
      dma->tx_addr is still DMA-mapped to — and powers down the UART via
      uart_change_pm(state, UART_PM_STATE_OFF) (line 1852). Likewise,
      uart_suspend_port() (serial_core.c:2502-2514) calls ops->shutdown(uport)
      (line 2502) and unconditionally sets UART_PM_STATE_OFF (line 2512).
   c) Even if startup() returns -EIO on reopen when DMA is still active,
      uart_port_startup() (serial_core.c:422-423) has:
        if (retval && capable(CAP_SYS_ADMIN))
                return 1;
      which causes uart_port_activate() (serial_core.c:2060-2062) to set
      tty_port_set_active(port, true) with tty_port_initialized(port) == false
      and allows root open() (serial_core.c:2040-2042) to succeed (0). From
      that state, ioctl(TIOCSSERIAL) -> uart_set_info() (serial_core.c:1068)
      -> uart_shutdown() (serial_core.c:466-511, which calls
      uart_free_xmit_buf() at line 510 outside if (tty_port_initialized))
      or uart_hangup() (serial_core.c:1942-1950) or uart_change_port() ->
      release_port() (serial_core.c:932-933, 1072) can free the TX buffer,
      power down the UART, and release MMIO mappings while DMA is active.

We modeled these cross-layer paths in Serial8250TxDmaV8.tla (QuarantineGuardsShutdown, QuarantineGuardsAdminOpen, and NoDmaOnPoweredOffUart in Serial8250TxDmaV8_UnguardedQuarantineHazard.cfg and Serial8250TxDmaV8_AdminReopenQuarantineHazard.cfg vs. Serial8250TxDmaV8_TerminateRetryHang_Fixed.cfg, alongside Serial8250TxDmaV8_QuarantinePioReopenHazard.cfg at
https://github.com/ismell/linux-8250-dma-tla), and TLC confirms that quarantine without those serial_core and PM guards violates NoDmaOnPoweredOffUart / NoConcurrentFifoAccess (in 6 states) and NoUsersAtFree (in 7 states). Given that enforcing non-blocking quarantine requires coordinated changes across serial_core.c, runtime PM, and 8250 shutdown, addressing non-blocking quarantine in a separate follow-up series and documenting the limitation here makes sense.

2. What in-tree DMAengine providers actually return from terminate_all:
For context on when the retry loop in serial8250_terminate_dma() can trigger, I audited all 78 .device_terminate_all implementations in drivers/dma/:
- 64 out of 78 unconditionally return 0.
- Among DMA controllers used with 8250 UARTs, synchronous FIFO-draining drivers (drivers/dma/dw/core.c dwc_chan_pause ~40 us, drivers/dma/idma64.c idma64_chan_deactivate ~100 us) already poll in microseconds inside terminate_all and return 0.
- The only 8250-paired DMA controllers that can return non-zero are:
* drivers/dma/tegra186-gpc-dma.c (tegra_dma_terminate_all, lines 714-743), which already polls readl_relaxed_poll_timeout_atomic() for up to 5000 us (5 ms) in tegra_dma_stop_client() / tegra_dma_pause() / tegra_dma_pause_noerr() (lines 389-414, 676-712) before returning -ETIMEDOUT; and
* drivers/dma/at_xdmac.c (at_xdmac_device_terminate_all, lines 1999-2039), which only returns < 0 if pm_runtime_resume_and_get() fails.
(Even across all 14 non-zero .device_terminate_all implementations in drivers/dma/, the only one designed around a caller retry loop is cppi41_stop_chan() -> cppi41_tear_down_chan() in drivers/dma/ti/cppi41.c:
652-770, which is a USB MUSB controller used only by musb_cppi41.c.)

Thanks,
Raul

#37 lore raw

Re: [PATCH v9 8/8] serial: 8250: retain DMA resources when termination fails

In reply to James Hilliard
Quoted 25 lines · 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.

Start retries with a 1 ms sleep and double it up to one second. This makes short-lived failures responsive while bounding sustained retry activity; neither delay is a hardware timeout. Report the initial failure, rate-limit ongoing failures with the elapsed time and current RX/TX error codes, and report recovery once both channels have stopped and drained.

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.

...

drivers/tty/serial/8250/8250_dma.c int serial8250_request_dma(struct uart_8250_port *p) · line 536

Quoted 4 lines
drivers/tty/serial/8250/8250_dma.c +4 -0
+static void serial8250_terminate_dma(struct uart_8250_port *p)
+{
+	struct uart_8250_dma *dma = p->dma;
+	unsigned int delay_us = USEC_PER_MSEC;

Don't use multipliers without a number, it's harder to read (when syntax highlighting is on) and follow in general.

1 * USEC_PER_MSEC

Quoted 13 lines
drivers/tty/serial/8250/8250_dma.c +13 -0
+	ktime_t start;
+	int rx_ret, tx_ret;
+
+	rx_ret = dmaengine_terminate_sync(dma->rxchan);
+	tx_ret = dmaengine_terminate_sync(dma->txchan);
+	if (!rx_ret && !tx_ret)
+		return;
+
+	start = ktime_get();
+	dev_err(p->port.dev,
+		"DMA termination failed: RX %d, TX %d; teardown blocked\n",
+		rx_ret, tx_ret);
+

drivers/tty/serial/8250/8250_dma.c int serial8250_request_dma(struct uart_8250_port *p) · line 559

Quoted 10 lines
drivers/tty/serial/8250/8250_dma.c +10 -0
+	/*
+	 * 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) {
+		fsleep(delay_us);
+		/* Retry promptly at first, then limit sustained retry activity. */
+		delay_us = min(delay_us * 2, USEC_PER_SEC);

Ditto.

Quoted 12 lines
drivers/tty/serial/8250/8250_dma.c +12 -0
+		if (rx_ret)
+			rx_ret = dmaengine_terminate_sync(dma->rxchan);
+		if (tx_ret)
+			tx_ret = dmaengine_terminate_sync(dma->txchan);
+		if (rx_ret || tx_ret)
+			dev_err_ratelimited(p->port.dev,
+					    "DMA termination still failing after %lld ms: RX %d, TX %d\n",
+					    ktime_ms_delta(ktime_get(), start), rx_ret, tx_ret);
+	}
+	dev_info(p->port.dev, "DMA termination recovered after %lld ms\n",
+		 ktime_ms_delta(ktime_get(), start));
+}
Signature · 2 lines
With Best Regards,
Andy Shevchenko
#38 lore raw

v10 cover serial: 8250: serialize DMA cancellation with buffer reuse

Reserve canceled transmit and receive storage until DMA and its completion callbacks have retired. Serial-core write backpressure protects a flushed TX FIFO while normal transfers continue to DMA directly from that FIFO.

The preparatory patches separate OMAP's locked TX completion processing, serialize asynchronous write wakeups, and fix OMAP RX locking and shutdown ordering. Serial-core reservation helpers then protect 8250 TX storage. Flushing empties the logical queue immediately. While the old transfer owns its storage, write_room() reports zero and write-ready notifications wait for retirement. Failed termination retains the reservation until normal completion, without advancing the reset FIFO. DMA retirement releases the reservation independently of port readiness. Serial core keeps writes and write-ready notifications gated during shutdown until successful startup or resume. Flushes still notify threads waiting for the logical FIFO to empty, independently of whether its storage can be reused.

RX delivers paused data before the PIO fallback and retires callbacks in process context. Track FIFO access separately from buffer retirement so PIO and the DesignWare empty-FIFO timeout handling can run once DMA has released the FIFO. Retry ambiguous pause failures until a successful pause or reported completion makes the buffer stable. Keep receive interrupts masked meanwhile, preserving the requested enables in the IER shadow. Back off pause retries from 1 ms to one second, with completion and shutdown waking the worker immediately. Report each prolonged stall once per flush, independently of other ports, while retaining the transfer. Reset the retry delay when the pending flush finishes.
Confirmed DMA errors use synchronous recovery and leave RX on PIO until reopen. Shutdown excludes new retirement work before releasing the channels.

Final shutdown retains both channels and their storage until synchronous termination succeeds. Retry failed channels outside the port lock, starting with a 1 ms sleep and doubling it up to one second to limit sustained retries. Report the initial error, ongoing failures at five-second intervals tracked separately for each teardown, and recovery once both channels have stopped and drained. A permanently failed DMA engine blocks teardown while retaining its storage and callback state.

DMA providers must supply the callback-drain guarantee of dmaengine_synchronize() before retirement releases storage. The related provider synchronization fixes are tracked separately and must precede these consumer changes on affected controllers.

Based on tty-next at 6628e7d4c6b2 with the posted shutdown guard [0], channel configuration error handling [1], RX TTY accounting fix [2], RZ/N1 transfer-length fix [3], transmit-PM shutdown fix [4], serport write-wakeup deferral v2 [5], DesignWare BUSY-safe fractional-divisor programming [6], and Intel MID RX DMA timeout locking [7]. The serport fix is needed before serializing the remaining write-wakeup callers with the port lock.

[0] https://lore.kernel.org/r/20261021-submit-serial-8250-dma-shutdown-v1-v1-1-3d3ce383c614@gmail.com
[1] https://lore.kernel.org/r/20261021-submit-serial-8250-dma-config-errors-v1-v1-1-c067587d1ef2@gmail.com
[2] https://lore.kernel.org/r/20261021-submit-serial-8250-dma-rx-accounting-v1-v1-1-c11795914ab4@gmail.com
[3] https://lore.kernel.org/r/20261023-submit-serial-8250-dma-tx-length-v1-v2-1-c3106ef4d2df@gmail.com
[4] https://lore.kernel.org/r/20261022-submit-serial-8250-tx-pm-shutdown-v1-v1-1-0e894afa3bb8@gmail.com
[5] https://lore.kernel.org/r/20261028-submit-serport-write-wakeup-v1-v2-1-c05d96259985@gmail.com
[6] https://lore.kernel.org/r/20261027-submit-serial-8250-dw-divisor-v1-v4-1-41714a75e15a@gmail.com
[7] https://lore.kernel.org/r/20261024-submit-serial-8250-mid-rx-lock-v1-v1-1-2b9afdf869d7@gmail.com

Assisted-by Codex:gpt-6-astra

---
Changes in v10:
- Rebase onto tty-next at 6628e7d4c6b2, retaining the latest posted versions
of the prerequisites.
- Warn once per stalled RX flush without a shared rate limit, so concurrent
stalls on different ports and later stalls on the same port are reported.
- Remove OMAP shutdown notifications for the unused generic RX pause worker.
- Use consistent TX cancellation predicates and the shared RX-running helper,
simplify condition formatting, and describe OMAP locking with Context:.
- Spell retry intervals as explicit multiples of the time-unit constants.
- Use the public bitmap header for SCCNXP wakeup iteration and leave the
unrelated event-handler iterator type unchanged.
- Share and document the low-water write-wakeup check used by serial-core
readiness and generic 8250/OMAP TX DMA completion.
- Explain why ambiguous pause failures retain the transfer instead of using timeout-triggered PIO recovery, distinguishing data preservation from
termination safety and documenting the possible indefinite RX stall.
- Track final-termination progress reporting separately for each teardown,
reporting ongoing failures at five-second intervals without a shared
rate limiter. Keep the initial failure and recovery reports
unconditional.
- Collect Raul's Reviewed-by tags for the first six patches.
- Link to v9: https://patch.msgid.link/20261102-submit-serial-8250-dma-cancel-v1-v9-0-9159caade148@gmail.com

Changes in v9:
- Back off pending RX pause retries from 1 ms to one second, wake the worker
on completion and shutdown, and report prolonged stalls once per flush
through a rate-limited warning. Reset the delay for each new flush.
- Lock ST ASC startup's transmit servicing and RX interrupt enable after
requesting the IRQ, covering write wakeups from an empty FIFO as well.
- Wake logical-FIFO drain waiters after a flush even while TX storage is
reserved or the port is not ready, keeping line-discipline write-ready
callbacks deferred until writes are allowed.
- Use plain port locking where interrupt context or an enclosing IRQ-saving
lock already supplies the required exclusion, retaining IRQ-saving locks
in workers, tasklets and mixed-context callers.
- Separate OMAP RX completion/flush locking and shutdown IRQ draining into
two preparatory fixes, with their own rationale and Fixes tags.
- Place the new delay header before the existing include block.
- Back off failed final DMA termination retries from 1 ms to a one-second
cap. Report the initial failure, rate-limit ongoing failures with elapsed
time and current RX/TX errors, and report successful recovery.
- Link to v8: https://patch.msgid.link/20261029-submit-serial-8250-dma-cancel-v1-v8-0-3fd56ba352ed@gmail.com

Changes in v8:
- Retain RX transfers after ambiguous pause failures and retry in the worker
until pause succeeds or completion is reported, preserving delayed data
while masking RX interrupts and allowing shutdown to take over.
- Retain both DMA channels and their storage during final shutdown until
synchronous termination succeeds, retrying failures outside the port lock.
- Update the serport write-wakeup prerequisite to the posted v2.
- Cover DZ transmit servicing, DECstation ZS TX/modem handling and
JSM Classic modem notifications with the port lock.
- Defer ZS console-triggered TX restart to a worker, preserving the
port-before-SCC lock order and draining work during teardown.
- Release Lantiq's private hardware lock before write-wakeup callbacks.
- Assert the port-lock contract in uart_write_wakeup().
- Retain the tx_running guard in generic and OMAP TX callbacks.
- Publish OMAP's shutdown gate before the final RX flush, serialize
RX completion and runtime-suspend flushes with port and RX DMA locks,
and drain its UART interrupt before releasing DMA state.
- Link to v7: https://patch.msgid.link/20261027-submit-serial-8250-dma-cancel-v1-v7-0-985a023ba4b4@gmail.com

Changes in v7:
- Rebase onto tty-next at 6b4b543fa187.
- Use v4 of the DesignWare BUSY-safe fractional-divisor prerequisite.
- Use an unsigned long bitmap and for_each_set_bit() for SCCNXP
deferred write wakeups, with unsigned port indices.
- Narrow SB1250 modem-status locking to shared-state updates and
avoid taking the port lock for RX-only interrupts. Keep TX FIFO
consumption and interrupt masking serialized with writers.
- Link to v6: https://lore.kernel.org/r/20261024-submit-serial-8250-dma-cancel-v1-v6-0-6f366591f566@gmail.com

Changes in v6:
- Use v2 of the DesignWare BUSY-safe fractional-divisor prerequisite.
- Serialize CLPS711x TX servicing and interrupt-enable state with the
port lock. Use non-waiting IRQ disable in stop_tx(), skip queued TX
interrupts after stopping, and drain the handler outside the lock
during shutdown.
- Make the Atmel RX/TX tasklets and SB1250 termios locking IRQ-safe
alongside their interrupt-handler locking, preserving the caller's
interrupt state when releasing the port lock.
- Report RX DMA recovery after releasing the port lock, keeping the
state changes and interrupt-enable restoration serialized.
- Link to v5: https://patch.msgid.link/20261024-submit-serial-8250-dma-cancel-v1-v5-0-9e52968928d2@gmail.com

Changes in v5:
- Add the posted serport write-wakeup deferral as a prerequisite for the
caller-locking preparation.
- Add a preparatory patch serializing asynchronous write-wakeup callers
with the port lock, including indirect CTS and transmit-helper calls.
Keep sleeping register accesses outside the lock and defer SCCNXP
notifications until its controller lock is released.
- Use the separately posted DesignWare BUSY-safe fractional-divisor fix
and Intel MID RX DMA timeout locking fix as prerequisites, leaving five
patches in this series.
- Separate serial-core write readiness from DMA storage reservations. Record
retirement during startup and shutdown while keeping writes and wakeups
gated until the port is ready.
- Wake deferred writers after successful startup or resume, including when
reopening with a retained FIFO, and restart any retained queued output.
- Keep the write-ready notification pending while retained output is at or
above the normal wakeup threshold, until TX completion drains it.
- Publish resume readiness under the TX restart lock without releasing a
reservation that may belong to a newly flushed transfer.
- Release 8250's TX reservation after shutdown has stopped DMA and unmapped
the source; serial core retains control of write readiness.
- Link to v4: https://patch.msgid.link/20261023-submit-serial-8250-dma-cancel-v1-v4-0-e759de1839de@gmail.com

Changes in v4:
- Retain flushed TX reservations through shutdown and failed resume. Release
them after successful resume, or when allocating a new FIFO on reopen,
so retirement cannot wake writers into a closing port.
- Treat RX flush as complete once the channel is inactive, and clear the
shutdown guard before startup configures the UART and tests interrupts.
- Check completion after a failed pause so a transfer that finished before
its callback acquired the port lock is delivered and retired normally.
- Keep DesignWare's BUSY-safe divisor setter after fractional-divisor
discovery, writing DLF and DLL/DLH only after confirming DLAB.
- Link to v3: https://patch.msgid.link/20261023-submit-serial-8250-dma-cancel-v1-v3-0-d78f9e4c4932@gmail.com

Changes in v3:
- Defer write-ready wakeups while flushed TX storage is reserved, and
serialize the flush wakeup with the reservation under the port lock.
- Suppress ordinary TX starts during the reservation while preserving
high-priority flow-control characters, which use separate storage.
- Separate RX FIFO access from callback retirement, allowing PIO and the
DesignWare empty-FIFO timeout workaround after a successful pause and
termination while keeping the receive buffer reserved.
- Use the existing IER shadow for requested enables and a shared register
writer to keep receive interrupts masked throughout DMA error recovery,
including termios, RS485, modem-status and overrun-backoff updates.
- Preserve the timeout-enable bit across stop_rx(), exclude late shutdown
interrupts from PIO, and avoid FIFO resets during RX DMA recovery.
- Preserve the completion-path LSR read after stop_rx(), while gating DMA
resubmission on the receive interrupt enable.
- Pack the cancellation flags after moving all their accesses under the
port lock, including the recovery worker's snapshot and initialization.
- Apply the early-return, shared-helper and line-wrapping cleanups.
- Link to v2: https://patch.msgid.link/20261022-submit-serial-8250-dma-cancel-v1-v2-0-088ea51e3891@gmail.com

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

To: Greg Kroah-Hartman <[email protected]>
To: Jiri Slaby <[email protected]>
To: Richard Genoud <[email protected]>
To: Nicolas Ferre <[email protected]>
To: Alexandre Belloni <[email protected]>
To: Claudiu Beznea <[email protected]>
To: "Maciej W. Rozycki" <[email protected]>
To: Jacky Huang <[email protected]>
To: Shan-Chun Hung <[email protected]>
To: Pali Rohár <[email protected]>
To: Frank Li <[email protected]>
To: Sascha Hauer <[email protected]>
To: Pengutronix Kernel Team <[email protected]>
To: Fabio Estevam <[email protected]>
To: Timur Tabi <[email protected]>
To: Peter Ujfalusi <[email protected]>
To: Vignesh Raghavendra <[email protected]>
To: Tony Lindgren <[email protected]>
To: Raul E Rangel <[email protected]>
To: Ilpo Järvinen <[email protected]>
To: Andy Shevchenko <[email protected]>
To: Heikki Krogerus <[email protected]>
To: Patrice Chotard <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]

---
James Hilliard (8):
serial: 8250_omap: separate locked TX DMA completion processing
serial: serialize asynchronous write wakeups with the port lock
serial: 8250_omap: serialize RX DMA completion and flushing
serial: 8250_omap: quiesce DMA IRQ handling before shutdown
serial: core: allow drivers to reserve flushed transmit storage
serial: 8250: drain canceled TX DMA before reusing its buffer
serial: 8250: retire canceled RX DMA before reusing the receive buffer
serial: 8250: retain DMA resources when termination fails

 drivers/tty/serial/21285.c          |   2 +
 drivers/tty/serial/8250/8250.h      |  66 +++++-
 drivers/tty/serial/8250/8250_core.c |   2 +-
 drivers/tty/serial/8250/8250_dma.c  | 406 +++++++++++++++++++++++++++++-------
 drivers/tty/serial/8250/8250_dw.c   |  28 ++-
 drivers/tty/serial/8250/8250_omap.c |  69 ++++--
 drivers/tty/serial/8250/8250_port.c |  36 +++-
 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         |  43 +++-
 drivers/tty/serial/serial_core.c    | 104 ++++++++-
 drivers/tty/serial/st-asc.c         |   2 +
 drivers/tty/serial/ucc_uart.c       |   6 +-
 drivers/tty/serial/zs.c             |  94 ++++++---
 drivers/tty/serial/zs.h             |   3 +
 include/linux/serial_core.h         |  34 ++-
 29 files changed, 810 insertions(+), 223 deletions(-)

---
base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
change-id: 20261021-submit-serial-8250-dma-cancel-v1-b208f84e89c1
prerequisite-patch-id: 3604bfaa3a1c214997de1f11162b81a4541af5ab
prerequisite-patch-id: 79dd093ffb35e254e790dffb8887faf51b64f947
prerequisite-patch-id: 80a5061d84c055a64c760268febed179c687b99f
prerequisite-patch-id: 7dbb404baf1f861982cf6a4b755d39cfec548223
prerequisite-patch-id: 40881d81a27b3a1aa45203d171214faaf2197768
prerequisite-patch-id: 76506fd9b234c7268227ff41e4d0989e82a1a433
prerequisite-patch-id: d8219673210dac82d3adab41578ff2f76f5862c9
prerequisite-patch-id: 48659e5ef3f5fb377d0b751f0830c0de4699262c

Best regards,

Signature · 1 line
James Hilliard <[email protected]>
#39 lore raw

PATCH v10 1/8 serial: 8250_omap: separate locked TX DMA completion processing

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.

---
 drivers/tty/serial/8250/8250_omap.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)
drivers/tty/serial/8250/8250_omap.c +13 -6
index 4805b644ccb1..b6f108d34854 100644
@@ -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;
Signature · 1 line
2.53.0
#40 lore raw

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

Sashiko has reviewed this patch and found no issues.
Quoted 7 lines

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.

Sashiko has reviewed this patch and found no issues.

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/20261108-submit-serial-8250-dma-cancel-v1-v10-0-e6227ddb95b1@gmail.com?part=1
#41 lore raw

PATCH v10 2/8 serial: serialize asynchronous write wakeups with the port lock

In reply to 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 plain port locking in hard-IRQ handlers and in helpers whose callers already mask interrupts. Atmel status handling is also called during resume, where lock_suspended is acquired with interrupts disabled; JSM interrupt processing holds its IRQ-saving board lock. Keep IRQ-saving locking in workers, tasklets and other mixed-context paths.

Take the port lock after requesting the ST ASC interrupt and hold it through startup's transmit servicing and RX interrupt enable. The transmit helper can notify writers even when the FIFO is empty, and serial core calls startup without holding the port spinlock.

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.

---
 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        | 43 +++++++++++++----
 drivers/tty/serial/st-asc.c        |  2 +
 drivers/tty/serial/ucc_uart.c      |  6 ++-
 drivers/tty/serial/zs.c            | 94 ++++++++++++++++++++++++++++----------
 drivers/tty/serial/zs.h            |  3 ++
 21 files changed, 201 insertions(+), 87 deletions(-)
drivers/tty/serial/21285.c +2 -0
index 4de0c975ebdc..c12b888c1b7e 100644
@@ -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)(port);
+
 	uart_port_tx_limited(port, ch, 256,
 		!(*CSR_UARTFLG & 0x20),
 		*CSR_UARTDR = ch,
drivers/tty/serial/atmel_serial.c +6 -6
index c3880259e3bc..4c3a6e10d92f 100644
@@ -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)(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,
drivers/tty/serial/clps711x.c +17 -6
index 83186bf50002..37b2635dda47 100644
@@ -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)(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,
drivers/tty/serial/cpm_uart.c +2 -0
index 39f54bb7b485..3f1e51b04f9f 100644
@@ -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)(port);
+
 	cpm_uart_tx_pump(port);
 }
 
drivers/tty/serial/dz.c +3 -6
index 39d93e9c2d15..c86453937a0f 100644
@@ -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)(&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);
-	}
 }
 
 /*
drivers/tty/serial/fsl_lpuart.c +1 -1
index c8575c965203..cb04571c3d0e 100644
@@ -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);
drivers/tty/serial/jsm/jsm_cls.c +4 -2
index 6e40792f92cf..e8e90181504b 100644
@@ -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, &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, &ch->uart_port)
+			cls_parse_modem(ch, readb(&ch->ch_cls_uart->msr));
 	}
 }
 
drivers/tty/serial/jsm/jsm_neo.c +6 -3
index e8e13bf056e2..d27aa2166364 100644
@@ -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, &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, &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, &ch->uart_port)
+			neo_copy_data_from_queue_to_uart(ch);
 	}
 }
 
drivers/tty/serial/lantiq.c +7 -5
index 62cd9e0bb377..e0b2c7335692 100644
@@ -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(&ltq_port->lock, flags);
-	uart_port_tx(port, ch,
-		lqasc_tx_ready(port),
-		writeb(ch, port->membase + LTQ_ASC_TBUF));
-	spin_unlock_irqrestore(&ltq_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(&ltq_port->lock, flags));
 	return;
 }
 
@@ -239,7 +240,8 @@ lqasc_tx_int(int irq, void *_port)
 	spin_lock_irqsave(&ltq_port->lock, flags);
 	__raw_writel(ASC_IRNCR_TIR, port->membase + LTQ_ASC_IRNCR);
 	spin_unlock_irqrestore(&ltq_port->lock, flags);
-	lqasc_start_tx(port);
+	scoped_guard(uart_port_lock, port)
+		lqasc_start_tx(port);
 	return IRQ_HANDLED;
 }
 
drivers/tty/serial/ma35d1_serial.c +4 -2
index 3be30fb05513..a59a8496a83b 100644
@@ -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, port)
+			transmit_chars(up);
+	}
 	if (fsr & MA35_FSR_TX_OVER_IF)
 		serial_out(up, MA35_FSR_REG, MA35_FSR_TX_OVER_IF);
 
drivers/tty/serial/max3100.c +4 -2
index f858a6669011..5a8817eb82ad 100644
@@ -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) &&
drivers/tty/serial/max310x.c +6 -3
index a0ed70185555..d9ce6ba53c7f 100644
@@ -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);
drivers/tty/serial/mux.c +8 -5
index 3a77a7e5c7bc..8be5b3031749 100644
@@ -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);
 }
 
 /**
drivers/tty/serial/mvebu-uart.c +2 -0
index 33ea496c93b2..35b55138637b 100644
@@ -334,6 +334,8 @@ static void mvebu_uart_tx_chars(struct uart_port *port, unsigned int status)
 {
 	u8 ch;
 
+	guard(uart_port_lock)(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)),
drivers/tty/serial/mxs-auart.c +2 -0
index 697318dbb146..6a310309b55d 100644
@@ -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();
drivers/tty/serial/sb1250-duart.c +15 -10
index b4e1b90e5960..af6a0fe1df70 100644
@@ -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)(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, 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);
 }
 
 
drivers/tty/serial/sccnxp.c +33 -10

102 lines not on this page · raw message

drivers/tty/serial/st-asc.c +2 -0
index 6ed9a327702b..5fefa3325a1f 100644
@@ -422,6 +422,8 @@ static int asc_startup(struct uart_port *port)
 		return -ENODEV;
 	}
 
+	guard(uart_port_lock_irqsave)(port);
+
 	asc_transmit_chars(port);
 	asc_enable_rx_interrupts(port);
 
drivers/tty/serial/ucc_uart.c +4 -2
index 6b7dcbbacd13..4c4e8e4621ec 100644
@@ -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, &qe_port->port)
+			qe_uart_tx_pump(qe_port);
+	}
 
 	return events ? IRQ_HANDLED : IRQ_NONE;
 }
drivers/tty/serial/zs.c +70 -24

195 lines not on this page · raw message

drivers/tty/serial/zs.h +3 -0
index e0d3c189b33f..0404294e4533 100644
@@ -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.  */
 
Signature · 1 line
2.53.0
#42 lore raw

Re: [PATCH v10 2/8] serial: serialize asynchronous write wakeups with the port lock

Sashiko has reviewed this patch and found no issues.
Quoted 20 lines

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 plain port locking in hard-IRQ handlers and in helpers whose callers already mask interrupts. Atmel status handling is also called during resume, where lock_suspended is acquired with interrupts disabled; JSM interrupt processing holds its IRQ-saving board lock. Keep IRQ-saving [ ... ]
the restart worker during shutdown and removal. Release Lantiq's hardware lock through the transmit helper's completion hook before notifying the line discipline.

Quoted 1 line

Sashiko has reviewed this patch and found no issues.

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/20261108-submit-serial-8250-dma-cancel-v1-v10-0-e6227ddb95b1@gmail.com?part=2
#43 lore raw

Re: [PATCH v10 2/8] serial: serialize asynchronous write wakeups with the port lock

In reply to James Hilliard
Quoted 59 lines · 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 plain port locking in hard-IRQ handlers and in helpers whose callers already mask interrupts. Atmel status handling is also called during resume, where lock_suspended is acquired with interrupts disabled; JSM interrupt processing holds its IRQ-saving board lock. Keep IRQ-saving locking in workers, tasklets and other mixed-context paths.

Take the port lock after requesting the ST ASC interrupt and hold it through startup's transmit servicing and RX interrupt enable. The transmit helper can notify writers even when the FIFO is empty, and serial core calls startup without holding the port spinlock.

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.

...

drivers/tty/serial/sb1250-duart.c static void sbd_status_handle(struct sbd_port *sport) · line 431

Quoted 7 lines
drivers/tty/serial/sb1250-duart.c +2 -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)))

Just keep it a single line.

Quoted 14 lines
drivers/tty/serial/sb1250-duart.c +7 -5
+		return;
 
-	if (delta & (M_DUART_IN_PIN2_VAL << S_DUART_IN_PIN_CHNG))
-		uport->icount.dsr++;
+	scoped_guard(uart_port_lock, 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++;
+	}

...

Quoted 11 lines
drivers/tty/serial/sccnxp.c +2 -1
 
-static void sccnxp_handle_events(struct sccnxp_port *s)
+static unsigned long sccnxp_handle_events(struct sccnxp_port *s)
 {
+	unsigned long wakeup = 0;
 	int i;
 	u8 isr;

drivers/tty/serial/sccnxp.c static void sccnxp_handle_events(struct sccnxp_port *s) · line 497

Quoted 8 lines
drivers/tty/serial/sccnxp.c +3 -2
@@ -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);

__set_bit()

Quoted 5 lines
drivers/tty/serial/sccnxp.c +3 -0
 		}
 	} while (1);
+
+	return wakeup;
+}
Signature · 2 lines
With Best Regards,
Andy Shevchenko
#44 lore raw

PATCH v10 3/8 serial: 8250_omap: serialize RX DMA completion and flushing

In reply to James Hilliard

Moving RX DMA locking out of __dma_rx_do_complete() left the DMA completion callback calling it without rx_dma_lock. Shutdown and runtime suspend flush RX under rx_dma_lock but without the port lock, allowing these paths to overlap the callback's buffer accounting and flip-buffer insertion. RX submission and UART interrupt handling also require the port lock.

Take rx_dma_lock inside the port lock in the completion callback. Hold the port lock around the shutdown and runtime-suspend flushes, matching the existing interrupt-path lock order. Assert both locks in the common completion helper and the port lock at the flush entry point.

Fixes 789898416749 ("serial: 8250: 8250_omap: Move locking out from __dma_rx_do_complete()")
---
 drivers/tty/serial/8250/8250_omap.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)
drivers/tty/serial/8250/8250_omap.c +14 -4
index b6f108d34854..5e114dd8f9bf 100644
@@ -771,8 +771,10 @@ static void omap_8250_shutdown(struct uart_port *port)
 	guard(serial8250_rpm)(up);
 
 	flush_work(&priv->qos_work);
-	if (up->dma)
+	if (up->dma) {
+		guard(uart_port_lock_irq)(port);
 		omap_8250_rx_dma_flush(up);
+	}
 
 	serial_out(up, UART_OMAP_WER, 0);
 	if (priv->habit & UART_HAS_EFR2)
@@ -898,7 +900,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 */
+/* Context: 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 +913,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 +982,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 +1001,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) {
@@ -1811,8 +1819,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);
Signature · 1 line
2.53.0
#45 lore raw

Re: [PATCH v10 3/8] serial: 8250_omap: serialize RX DMA completion and flushing

Sashiko has reviewed this patch and found no issues.
Quoted 7 lines

Moving RX DMA locking out of __dma_rx_do_complete() left the DMA completion callback calling it without rx_dma_lock. Shutdown and runtime suspend flush RX under rx_dma_lock but without the port lock, allowing these paths to overlap the callback's buffer accounting and flip-buffer insertion. RX submission and UART interrupt handling also require the port lock.

Quoted 8 lines

Take rx_dma_lock inside the port lock in the completion callback. Hold the port lock around the shutdown and runtime-suspend flushes, matching the existing interrupt-path lock order. Assert both locks in the common completion helper and the port lock at the flush entry point.

Fixes 789898416749 ("serial: 8250: 8250_omap: Move locking out from __dma_rx_do_complete()")

Sashiko has reviewed this patch and found no issues.

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/20261108-submit-serial-8250-dma-cancel-v1-v10-0-e6227ddb95b1@gmail.com?part=3
#46 lore raw

PATCH v10 4/8 serial: 8250_omap: quiesce DMA IRQ handling before shutdown

In reply to James Hilliard

disable_irq_nosync() masks the UART interrupt without waiting for a running handler. That handler can have observed up->dma and continue using it while shutdown releases the DMA channels and clears the pointer. Synchronizing the IRQ later in serial core does not protect those accesses.

Publish the DMA shutdown gate under the port lock before the final RX flush and interrupt masking. Have DMA IRQ handling check the same gate under the lock, so a queued interrupt cannot restart receive processing after the final flush.

Drain the UART IRQ outside the port lock before releasing DMA state. The lock excludes competing RX processing during the flush, while disable_irq() also waits for a handler that observed up->dma before acquiring the lock.

Fixes fef4f600319e ("serial: 8250: omap: Fix life cycle issues for interrupt handlers")
---
 drivers/tty/serial/8250/8250_omap.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)
drivers/tty/serial/8250/8250_omap.c +10 -6
index 5e114dd8f9bf..51013dd2d851 100644
@@ -771,10 +771,6 @@ static void omap_8250_shutdown(struct uart_port *port)
 	guard(serial8250_rpm)(up);
 
 	flush_work(&priv->qos_work);
-	if (up->dma) {
-		guard(uart_port_lock_irq)(port);
-		omap_8250_rx_dma_flush(up);
-	}
 
 	serial_out(up, UART_OMAP_WER, 0);
 	if (priv->habit & UART_HAS_EFR2)
@@ -782,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;
@@ -1330,6 +1329,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) {
Signature · 1 line
2.53.0
#47 lore raw

Re: [PATCH v10 4/8] serial: 8250_omap: quiesce DMA IRQ handling before shutdown

Sashiko has reviewed this patch and found no issues.
Quoted 10 lines

disable_irq_nosync() masks the UART interrupt without waiting for a running handler. That handler can have observed up->dma and continue using it while shutdown releases the DMA channels and clears the pointer. Synchronizing the IRQ later in serial core does not protect those accesses.

Publish the DMA shutdown gate under the port lock before the final RX flush and interrupt masking. Have DMA IRQ handling check the same gate under the lock, so a queued interrupt cannot restart receive processing after the final flush.

Quoted 8 lines

Drain the UART IRQ outside the port lock before releasing DMA state. The lock excludes competing RX processing during the flush, while disable_irq() also waits for a handler that observed up->dma before acquiring the lock.

Fixes fef4f600319e ("serial: 8250: omap: Fix life cycle issues for interrupt handlers")

Sashiko has reviewed this patch and found no issues.

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/20261108-submit-serial-8250-dma-cancel-v1-v10-0-e6227ddb95b1@gmail.com?part=4
#48 lore raw

PATCH v10 5/8 serial: core: allow drivers to reserve flushed transmit storage

In reply to 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's write-ready notification under the port lock so it cannot race another reservation.

Wake tty->write_wait after flushing even when writes remain blocked. tty_wait_until_sent() waits for the logical FIFO to empty, independently of whether its storage can be reused. Notify these drain waiters without invoking the line discipline's write_wakeup() callback, so a reservation does not prevent a completed logical flush from waking a drain waiter or consume a one-shot write-ready notification prematurely.

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. Share that threshold check with generic 8250 and OMAP TX DMA completion through uart_write_wakeup_if_needed().

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.

---
 drivers/tty/serial/8250/8250_dma.c  |   4 +-
 drivers/tty/serial/8250/8250_omap.c |   3 +-
 drivers/tty/serial/serial_core.c    | 104 ++++++++++++++++++++++++++++++++----
 include/linux/serial_core.h         |  34 +++++++++++-
 4 files changed, 129 insertions(+), 16 deletions(-)
drivers/tty/serial/8250/8250_dma.c +1 -3
index 57fc7915adac..43c55cb991a5 100644
@@ -15,7 +15,6 @@ static void __dma_tx_complete(void *param)
 {
 	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;
 
@@ -30,8 +29,7 @@ static void __dma_tx_complete(void *param)
 
 	uart_xmit_advance(&p->port, dma->tx_size);
 
-	if (kfifo_len(&tport->xmit_fifo) < WAKEUP_CHARS)
-		uart_write_wakeup(&p->port);
+	uart_write_wakeup_if_needed(&p->port);
 
 	ret = serial8250_tx_dma(p);
 	if (ret || !dma->tx_running)
drivers/tty/serial/8250/8250_omap.c +1 -2
index 51013dd2d851..477d645be9b0 100644
@@ -1106,8 +1106,7 @@ static void omap_8250_tx_dma_complete(struct uart_8250_port *p)
 		omap8250_restore_regs(p);
 	}
 
-	if (kfifo_len(&tport->xmit_fifo) < WAKEUP_CHARS)
-		uart_write_wakeup(&p->port);
+	uart_write_wakeup_if_needed(&p->port);
 
 	if (!kfifo_is_empty(&tport->xmit_fifo) && !uart_tx_stopped(&p->port)) {
 		int ret;
drivers/tty/serial/serial_core.c +95 -9

191 lines not on this page · raw message

include/linux/serial_core.h +32 -2
index 4eeb8253752a..2297b870cb86 100644
@@ -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,24 @@ 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);
+
+/**
+ * uart_write_wakeup_if_needed - notify writers when queued output is low
+ * @port: port to be processed
+ *
+ * Keep one-shot write-wakeup notifications pending until fewer than
+ * %WAKEUP_CHARS remain in the transmit FIFO. This also applies to output
+ * retained across suspend and resume.
+ *
+ * Context: @port->lock held.
+ */
+static inline void uart_write_wakeup_if_needed(struct uart_port *port)
+{
+	if (kfifo_len(&port->state->port.xmit_fifo) < WAKEUP_CHARS)
+		uart_write_wakeup(port);
+}
 
 /**
  * enum UART_TX_FLAGS -- flags for uart_port_tx_flags()
Signature · 1 line
2.53.0
#49 lore raw

Re: [PATCH v10 5/8] serial: core: allow drivers to reserve flushed transmit storage

Sashiko has reviewed this patch and found no issues.
Quoted 14 lines

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's write-ready notification under the port lock so it cannot race another reservation.

Quoted 7 lines

Wake tty->write_wait after flushing even when writes remain blocked. [ ... ]
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.

Sashiko has reviewed this patch and found no issues.

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/20261108-submit-serial-8250-dma-cancel-v1-v10-0-e6227ddb95b1@gmail.com?part=5
#50 lore raw

PATCH v10 6/8 serial: 8250: drain canceled TX DMA before reusing its buffer

In reply to 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")
---
 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(-)
drivers/tty/serial/8250/8250.h +17 -3
index 974a47f24084..1e739aceff39 100644
@@ -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 (serial8250_tx_dma_blocked(dma) || !dma->tx_running)
 		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 (serial8250_tx_dma_blocked(dma) || !dma->tx_running)
 		return;
 
 	dmaengine_resume(dma->txchan);
drivers/tty/serial/8250/8250_dma.c +94 -36

225 lines not on this page · raw message

drivers/tty/serial/8250/8250_omap.c +14 -7
index 477d645be9b0..a7cd5b918622 100644
@@ -1099,7 +1099,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;
@@ -1131,7 +1136,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);
 }
@@ -1146,7 +1151,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)) {
 
@@ -1205,24 +1210,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;
Signature · 1 line
2.53.0
#51 lore raw

Re: [PATCH v10 6/8] serial: 8250: drain canceled TX DMA before reusing its buffer

Sashiko has reviewed this patch and found no issues.
Quoted 14 lines

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.

Quoted 7 lines

Reuse the locked completion processing after retirement, including OMAP [ ... ]
that guarantee for controllers which previously lacked it.

Fixes a424a34b8fad ("serial: 8250: Fix TX deadlock when using DMA")

Sashiko has reviewed this patch and found no issues.

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/20261108-submit-serial-8250-dma-cancel-v1-v10-0-e6227ddb95b1@gmail.com?part=6
#52 lore raw

PATCH v10 7/8 serial: 8250: retire canceled RX DMA before reusing the receive buffer

In reply to 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. 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.

A bounded retry window could instead enter synchronous recovery. Successful dmaengine_terminate_sync() would stop the transfer and drain its callbacks, making buffer reuse and PIO access safe under the DMAengine contract. The concern is data preservation, not memory safety after successful termination: this recovery path discards undelivered receive data and disables RX DMA until reopen. Retrying ambiguous pause failures preserves the chance to deliver that data and continue using DMA when the provider recovers. Reserve the existing recovery path for DMA_ERROR or termination failure rather than selecting it solely on elapsed time.

RX DMA admission already requires cmd_pause. A provider whose hardware cannot pause must report that capability correctly so the unsupported channel is rejected before receiving data.

Start pause retries at 1 ms and double the delay up to one second to limit CPU use during persistent failure. Sleep outside the port lock on a timed waitqueue, with completion and shutdown publishing a wakeup under the same lock as the worker's retry check. This also catches notifications arriving just before the worker sleeps, avoiding a full backoff delay after completion or during shutdown. Reset the delay for each new flush. Report a stall lasting at least one second once per flush. Keep the warning local to that flush so simultaneous stalls on different ports and later stalls on the same port are reported independently.

Retries and diagnostics do not guarantee progress or lossless reception. If a provider never pauses or completes, a partial transfer can remain pending indefinitely while a request/response peer waits for a reply. Receive interrupts stay masked, the worker holds its runtime PM reference, and DesignWare line-setting updates cannot proceed through the idle sequence. Further incoming data can overrun the FIFO. This retains DMA ownership until an error or shutdown; the backoff bounds retry cost, not the duration of the stall.

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.

Fixes 9ee4b83e51f7 ("serial: 8250: Add support for dmaengine")
---
 drivers/tty/serial/8250/8250.h      |  46 +++++++-
 drivers/tty/serial/8250/8250_core.c |   2 +-
 drivers/tty/serial/8250/8250_dma.c  | 226 +++++++++++++++++++++++++++++-------
 drivers/tty/serial/8250/8250_dw.c   |  28 +++--
 drivers/tty/serial/8250/8250_port.c |  36 ++++--
 5 files changed, 276 insertions(+), 62 deletions(-)
drivers/tty/serial/8250/8250.h +42 -4
index 1e739aceff39..25ca59aa701d 100644
@@ -11,6 +11,7 @@
 #include <linux/serial_8250.h>
 #include <linux/serial_core.h>
 #include <linux/dmaengine.h>
+#include <linux/wait.h>
 #include <linux/workqueue.h>
 
 #include "../serial_mctrl_gpio.h"
@@ -23,6 +24,10 @@ 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;
+	wait_queue_head_t rx_pause_wait;
+	/* Written under the port lock; sampled locklessly by the wait condition. */
+	bool rx_pause_wakeup;
 	struct uart_8250_port *port;
 
 	/* Filter function */
@@ -55,11 +60,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 +158,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 +254,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 +266,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 +450,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 +506,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;
drivers/tty/serial/8250/8250_core.c +1 -1
index 42319843cb5b..ca06288038ed 100644
@@ -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,
drivers/tty/serial/8250/8250_dma.c +187 -39

288 lines not on this page · raw message

drivers/tty/serial/8250/8250_dw.c +20 -8
index 9cbe076463e0..2f6681910351 100644
@@ -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)) {
drivers/tty/serial/8250/8250_port.c +26 -10
index cfbd154bf5f9..49d6a0b19fb4 100644
@@ -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);
 
 	/*
@@ -2447,8 +2460,11 @@ void serial8250_do_shutdown(struct uart_port *port)
 	 * Synchronize UART_IER access against the console.
 	 */
 	scoped_guard(uart_port_lock_irqsave, port) {
-		if (up->dma)
+		if (up->dma) {
 			up->dma->shutting_down = true;
+			WRITE_ONCE(up->dma->rx_pause_wakeup, true);
+			wake_up(&up->dma->rx_pause_wait);
+		}
 		up->ier = 0;
 		serial_port_out(port, UART_IER, 0);
 	}
@@ -2786,7 +2802,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)
Signature · 1 line
2.53.0
#53 lore raw

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

Thank you for your contribution! Sashiko AI review found 1 potential issue to consider: - [High] serial: 8250: unbounded pause retry loop in serial8250_rx_dma_flush_work() --

Thank you for your contribution! Sashiko AI review found 1 potential issue to consider:
- [High] serial: 8250: unbounded pause retry loop in serial8250_rx_dma_flush_work()
--

commit 818afe206e7bf388666735a110a648d91146a4e7
Author: James Hilliard <[email protected]>

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

This commit defers reuse of a canceled RX DMA buffer until a worker has drained pending DMA callbacks, and retries failed pause attempts from process context before copying or recovering the channel.

Quoted 4 lines
drivers/tty/serial/8250/8250_dma.c +0 -0
index 739a8238e4dec..1ea22954150ca 100644

[ ... ]

Quoted 26 lines
drivers/tty/serial/8250/8250_dma.c +26 -0
+/* 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;
+	}

[ ... ]

Quoted 29 lines
drivers/tty/serial/8250/8250_dma.c +29 -0
+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;
+	unsigned int delay_us = 1 * USEC_PER_MSEC;
+	ktime_t start = ktime_get();
+	bool recover, retry, restore_rx;
+	bool warned = false;
+	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;
+			/* Serialize reset with completion and shutdown notifications. */
+			WRITE_ONCE(dma->rx_pause_wakeup, false);
+			retry = dma->rx_pause_pending && !dma->rx_terminating;
+			if (retry) {
+				ret = serial8250_rx_dma_try_flush(p);
+				retry = dma->rx_pause_pending && !dma->rx_terminating;
+			}
+			recover = dma->rx_err;
+		}
+		if (!retry)
+			break;
+		if (!warned && ktime_ms_delta(ktime_get(), start) >= 1 * MSEC_PER_SEC) {

drivers/tty/serial/8250/8250_dma.c void serial8250_tx_dma_sync_buffer(struct uart_8250_port *p, · line 229

Quoted 10 lines
drivers/tty/serial/8250/8250_dma.c +10 -0
+			dev_warn(p->port.dev,
+				 "RX DMA pause stalled: %d; retaining transfer and retrying\n",
+				 ret);
+			warned = true;
+		}
+		/* Notifications also cover completion or shutdown before sleeping. */
+		wait_event_hrtimeout(dma->rx_pause_wait, READ_ONCE(dma->rx_pause_wakeup),
+				     ns_to_ktime(delay_us * NSEC_PER_USEC));
+		delay_us = min(delay_us * 2, 1 * USEC_PER_SEC);
+	}

[Severity: High]
As reported by Raul E Rangel on v8 [1], can serial8250_rx_dma_flush_work() spin in its retry loop until the port is closed when dmaengine_pause() fails persistently on a partial transfer?

Once p->port.lock and local IRQs have been released across a few sleeps, any pending completion hardirq for a finished transfer has already run. On a partial transfer that has not reached dma->rx_size, an in-tree provider such as dw-axi-dmac can time out waiting for DWAXIDMAC_IRQ_SUSPENDED in dma_chan_pause(), clear that status bit while leaving the hardware channel suspended, and return -EAGAIN on every subsequent retry:

drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c:dma_chan_pause() {
	...
	do {
		if (axi_chan_irq_read(chan) & DWAXIDMAC_IRQ_SUSPENDED)
			break;

		udelay(2);
	} while (--timeout);

	axi_chan_irq_clear(chan, DWAXIDMAC_IRQ_SUSPENDED);

	chan->is_paused = true;

	return timeout ? 0 : -EAGAIN;
}

Because the channel is suspended in hardware, it never receives more bytes to reach DMA_COMPLETE, and every retry in serial8250_rx_dma_try_flush() keeps dma->rx_pause_pending set. That wedges RX on the port (with UART_IER_RDI, UART_IER_RLSI, and UART_IER_RTOIE masked and skip_rx forced in serial8250_handle_irq_locked()), holds the runtime PM reference in serial8250_rx_dma_flush_work(), and causes dw8250_idle_enter() to fail with -EAGAIN until shutdown(), which then discards dma->rx_buf without delivering the partial data anyway.

Escalating to serial8250_rx_dma_recover(p) after a bounded retry budget (or immediately on -ENOSYS / -EOPNOTSUPP) would let dmaengine_terminate_sync() stop the channel and fall back to PIO.

[1] https://lore.kernel.org/r/[email protected]

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/20261108-submit-serial-8250-dma-cancel-v1-v10-0-e6227ddb95b1@gmail.com?part=7
#54 lore raw

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

In reply to James Hilliard

Hi James,

Thanks for dropping the unused rx_pause_wakeup lines in omap_8250_shutdown() in v10. Following up here on your question from the v9 7/8 thread:

Quoted 8 lines · James Hilliard

For pause-capable hardware, a recovery timeout does not have to prove that the failure is permanent. The concern is what happens after that
timeout: the current recovery path discards the pending transfer and leaves RX on PIO until reopen.

Is there a recovery approach that avoids an indefinite RX stall without turning a potentially temporary pause failure into loss of DMA for the rest of the open session?

Yes — the only reason the current recovery path disables RX DMA for the rest of the open session is that serial8250_rx_dma_recover() sets dma->rx_err = true and serial8250_rx_dma_flush_work() leaves it set after dmaengine_terminate_sync(dma->rxchan) succeeds, which causes serial8250_rx_dma() to return -EIO until reopen.

Notice that struct uart_8250_dma already distinguishes a normal post-pause async termination from an unpaused transfer that needs synchronous termination:
- When serial8250_rx_dma_try_flush() succeeds, it calls dmaengine_terminate_async(dma->rxchan), sets dma->rx_terminating = true, and clears dma->rx_running = 0.
- When serial8250_rx_dma_try_flush() fails, dma->rx_running remains 1.

So we can decouple bounded pause-timeout recovery from sticky PIO fallback without adding any new struct fields:

1. In serial8250_rx_dma_flush_work(), if retry is still true when the 1-second retry budget expires (or immediately if ret == -ENOSYS || ret == -EOPNOTSUPP):
- If ret == -ENOSYS || ret == -EOPNOTSUPP (e.g. Tegra186 where hw_support_pause is false), call serial8250_rx_dma_recover(p) so dma->rx_err = true permanently falls back to PIO for the session.
- Otherwise (a pause timeout such as -EAGAIN, -EBUSY, or

     -ETIMEDOUT), set dma->rx_terminating = true while keeping
     dma->rx_running = 1, dma->rx_pause_pending = true, and
     dma->rx_err = false, and set retry = false.

2. Derive recover from persistent state under port->lock:

     recover = dma->rx_err || (dma->rx_running && dma->rx_terminating);
     fall_back_pio = dma->rx_err;

And in serial8250_rx_dma_recovering() (8250.h):

     return up->dma &&
            (up->dma->rx_pause_pending ||
             ((up->dma->rx_err || up->dma->rx_running) &&
              up->dma->rx_terminating));

3. In if (recover), run ret = dmaengine_terminate_sync(dma->rxchan):
- While dmaengine_terminate_sync() runs outside port->lock,

     serial8250_rx_dma_recovering() is true (keeping RX interrupts
     masked in UART_IER and skip_rx = true) and dma->rx_terminating is
     true (blocking dma_rx_complete() and serial8250_rx_dma()).

- If dmaengine_terminate_sync(dma->rxchan) fails (ret != 0), latch dma->rx_err = true under port->lock and return early so RX stays masked and the buffer stays owned by DMA.
- If dmaengine_terminate_sync(dma->rxchan) succeeds (ret == 0), the existing post-sync cleanup block clears rx_pause_pending, rx_terminating, and rx_running and restores UART_IER with dma->rx_err == false.

Because 8250 RX DMA is armed on-demand when rx_running == 0 and !rx_err, any trigger-tail bytes remaining in the UART RX FIFO below the RDI trigger level are drained via PIO on UART_IIR_RX_TIMEOUT, and the next UART_IIR_RDI interrupt submits a fresh RX DMA descriptor — so RX DMA remains active for the rest of the open session.

(Regarding the dw-axi-dmac driver mentioned on v10 7/8:
drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c has separate provider bugs where dw_axi_dma_interrupt() clears CH_INTSTATUS across all channels including DWAXIDMAC_IRQ_SUSPENDED, dma_chan_pause() leaves DMAC_CHSUSPREG and chan->is_paused = true set when returning -EAGAIN, and dma_chan_terminate_all() only clears DMAC_CHEN without clearing DMAC_CHSUSPREG or chan->is_paused. Those belong in a dw-axi-dmac driver fix, whereas the non-sticky pause-timeout recovery above ensures 8250_dma.c never stalls RX indefinitely on a pause timeout while keeping RX DMA enabled for subsequent transfers.)

Thanks,
Raul

#55 lore raw

PATCH v10 8/8 serial: 8250: retain DMA resources when termination fails

In reply to 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.

Start retries with a 1 ms sleep and double it up to one second. This makes short-lived failures responsive while bounding sustained retry activity; neither delay is a hardware timeout. Report the initial failure and ongoing failures with the elapsed time and current RX/TX error codes. Track the five-second progress-report interval separately for each teardown, so failures on different ports are reported independently. Report recovery once both channels have stopped and drained.

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")
---
 drivers/tty/serial/8250/8250_dma.c | 50 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 48 insertions(+), 2 deletions(-)
drivers/tty/serial/8250/8250_dma.c +48 -2
index 1ea22954150c..a279e3a1a845 100644
@@ -528,6 +528,53 @@ 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;
+	unsigned int delay_us = 1 * USEC_PER_MSEC;
+	ktime_t start, last_report;
+	int rx_ret, tx_ret;
+
+	rx_ret = dmaengine_terminate_sync(dma->rxchan);
+	tx_ret = dmaengine_terminate_sync(dma->txchan);
+	if (!rx_ret && !tx_ret)
+		return;
+
+	start = ktime_get();
+	last_report = start;
+	dev_err(p->port.dev,
+		"DMA termination failed: RX %d, TX %d; teardown blocked\n",
+		rx_ret, tx_ret);
+
+	/*
+	 * 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) {
+		fsleep(delay_us);
+		/* Retry promptly at first, then limit sustained retry activity. */
+		delay_us = min(delay_us * 2, 1 * USEC_PER_SEC);
+		if (rx_ret)
+			rx_ret = dmaengine_terminate_sync(dma->rxchan);
+		if (tx_ret)
+			tx_ret = dmaengine_terminate_sync(dma->txchan);
+		if (rx_ret || tx_ret) {
+			ktime_t now = ktime_get();
+
+			if (ktime_ms_delta(now, last_report) < 5 * MSEC_PER_SEC)
+				continue;
+			dev_err(p->port.dev,
+				"DMA termination still failing after %lld ms: RX %d, TX %d\n",
+				ktime_ms_delta(now, start), rx_ret, tx_ret);
+			last_report = now;
+		}
+	}
+	dev_info(p->port.dev, "DMA termination recovered after %lld ms\n",
+		 ktime_ms_delta(ktime_get(), start));
+}
+
 void serial8250_release_dma(struct uart_8250_port *p)
 {
 	struct uart_8250_dma *dma = p->dma;
@@ -537,9 +584,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;
@@ -551,7 +598,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);
Signature · 1 line
2.53.0
#56 lore raw

Re: [PATCH v10 8/8] serial: 8250: retain DMA resources when termination fails

Sashiko has reviewed this patch and found no issues.
Quoted 24 lines

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.

Start retries with a 1 ms sleep and double it up to one second. This makes short-lived failures responsive while bounding sustained retry activity; neither delay is a hardware timeout. Report the initial failure and ongoing failures with the elapsed time and current RX/TX error codes. Track the five-second progress-report interval separately for each teardown, so failures on different ports are reported independently. Report recovery once both channels have stopped and drained.

Quoted 6 lines

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

Sashiko has reviewed this patch and found no issues.

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/20261108-submit-serial-8250-dma-cancel-v1-v10-0-e6227ddb95b1@gmail.com?part=8