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

[Andy Shevchenko]

On Mon, Nov 02, 2026 at 04:14:34PM -0700, James Hilliard wrote:
> The generic RX timeout path pauses a transfer, copies its data and
> terminates it asynchronously. Clearing rx_running at that point lets a
> new receive reuse the buffer and cookie while the old callback has not
> yet run. Inspecting the new cookie cannot distinguish that callback from
> the completion of a new transfer.
> 
> Reserve the receive buffer until a worker has drained the canceled
> callbacks. Keep FIFO access separate from callback retirement: after a
> successful pause and termination, clear rx_running while rx_terminating
> prevents another descriptor from reusing the buffer. Deliver the paused
> data before allowing the interrupt handler to drain the FIFO through PIO.
> Normal retirement only releases ownership and leaves subsequent receive
> handling to the UART interrupt.
> 
> Preserve the normal completion-path LSR read and gate DMA resubmission on
> the receive interrupt enable, so stop_rx() prevents another transfer while
> the status read retains its existing side effects.
> 
> Reject new flush work and PIO receive handling during shutdown, including
> late Intel MID DMA timeouts. Stop and drain retirement work before freeing
> the channels and buffer. Once RX has stopped, allow the idle flush used by
> DesignWare LCR recovery during shutdown. Clear the shutdown guard before
> startup configures the UART and tests its interrupts.
> 
> Check pause failures before accessing the buffer. A successful pause is
> the quiescence guarantee, even when a provider still reports IN_PROGRESS.
> Also accept DMA_COMPLETE after a failed pause: completion can stop the
> channel before its callback acquires the port lock, leaving nothing for
> the provider to pause. Retire that completed transfer normally.
> 
> A failed pause with an incomplete cookie is ambiguous: hardware may have
> finished while its completion interrupt is still pending. Canceling that
> descriptor can discard the entire completed receive buffer and switch
> permanently to PIO. Retain the transfer and retry pause and status in the
> worker, with receive interrupts masked to prevent a timeout interrupt
> storm. The worker copies only after a successful pause or reported
> completion, then terminates and drains the old callback before reuse.
> The pending-pause guard keeps callbacks from copying or resubmitting the
> transfer concurrently.
> 
> 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.

...

> +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);

> +}

> +/* 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);
> +}

...

>  	struct tty_port		*tty_port = &p->port.state->port;
> -	struct dma_tx_state	state;
> -	enum dma_status		dma_status;
> -	int			count;
> -
> -	/*
> -	 * New DMA Rx can be started during the completion handler before it
> -	 * could acquire port's lock and it might still be ongoing. Don't to
> -	 * anything in such case.
> -	 */
> -	dma_status = dmaengine_tx_status(dma->rxchan, dma->rx_cookie, &state);
> -	if (dma_status == DMA_IN_PROGRESS)
> -		return;
> -
> -	count = dma->rx_size - state.residue;
> +	size_t			count = dma->rx_size - state->residue;

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

...

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

> +			warned = true;

Altogether NIH _once().

> +		}
> +		/* 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");
>  }

...

> -void serial8250_rx_dma_flush(struct uart_8250_port *p)
> +int serial8250_rx_dma_flush(struct uart_8250_port *p)
>  {
>  	struct uart_8250_dma *dma = p->dma;
> +	int ret;
>  
> -	if (!dma || !dma->rxchan || !dma->rx_running)
> -		return;
> +	lockdep_assert_held_once(&p->port.lock);
>  
> -	dmaengine_pause(dma->rxchan);
> -	__dma_rx_complete(p);
> -	dmaengine_terminate_async(dma->rxchan);
> +	if (!dma || !dma->rx_running)

You introduced a helper serial8250_rx_dma_running(). Use it.

> +		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);

...

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

>  	}

-- 
With Best Regards,
Andy Shevchenko