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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Wed, Nov 11, 2026 at 03:16:32PM -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. Initially retain
> the transfer and retry pause and status in the worker. Mask receive
> interrupts to prevent a timeout storm and keep callbacks from copying or
> resubmitting this worker-owned transfer. Back off retries from 1 ms,
> allowing completion and shutdown to wake the worker immediately.
> 
> After one second of unsuccessful pause retries, stop the transfer with
> dmaengine_terminate_sync() outside the port lock. An indefinite wait could
> otherwise leave a request/response peer waiting for data that is stuck in
> a partial DMA buffer. Report that recovery discards the pending payload:
> without a stable residue it cannot safely be copied. Keep rx_running,
> rx_pause_pending and rx_terminating set until termination has stopped DMA
> and drained its callbacks. Then restore receive handling and allow another
> DMA descriptor, without latching a permanent PIO fallback for a pause
> timeout. This trades the stalled transfer's data for continued reception.
> 
> If synchronous termination fails, retain ownership and retry with a delay
> that doubles from 1 ms to one second. Report the first failure and ongoing
> failures every five seconds for this recovery. Completion and shutdown
> wake the retry wait; shutdown takes over final channel retirement. A
> permanent termination failure still leaves RX masked and its storage
> reserved: the pause retry budget does not guarantee recovery from a
> provider that cannot stop. Each flush starts with a fresh retry budget.
> 
> RX DMA admission already requires cmd_pause. A provider whose hardware
> cannot pause must advertise that correctly. If pause nevertheless returns
> -ENOSYS or -EOPNOTSUPP for an incomplete transfer, enter synchronous
> recovery immediately. Unsupported pause, DMA_ERROR and asynchronous
> termination failure retain the error fallback to PIO until reopen.
> 
> 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.
> 
> 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.

...

> +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, fallback;
> +	bool timed_out = false;
> +	int ret = 0;

Once set, it gets never cleared.

> +	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;
> +				if (retry &&
> +				    ktime_ms_delta(ktime_get(), start) >= 1 * MSEC_PER_SEC) {
> +					dma->rx_terminating = true;
> +					timed_out = true;
> +					retry = false;
> +				}
> +			}
> +			recover = dma->rx_err || (dma->rx_running && dma->rx_terminating);
> +			fallback = dma->rx_err;
> +		}
> +		if (!retry)
> +			break;
> +		/* 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);
> +	}
> +
> +	if (timed_out)
> +		dev_warn(p->port.dev, "RX DMA pause timed out: %d; discarding pending data\n", ret);

So here is the potential to get timed_out == true and stale ret. Yes, I see
that code couples them, but the logic of the above is quite complicated.

The scoped_guard() piece better to be in a separate helper (and replaced with
guard()() to make it on indentation level down).

> +	if (recover) {
> +		if (serial8250_rx_dma_terminate(p))
> +			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 (fallback)
> +		dev_warn(p->port.dev, "RX DMA failed, using PIO\n");
>  }

...

Overall it makes me think if this is a common DMA issue when we try to stop
ongoing transfers, shouldn't it be solved once for all somewhere in DMAengine?

-- 
With Best Regards,
Andy Shevchenko