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

[sashiko-bot@kernel.org]

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 <james.hilliard1@gmail.com>

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.

> diff --git a/drivers/tty/serial/8250/8250_dma.c b/drivers/tty/serial/8250/8250_dma.c
> index 344f0dd24b416..fd2a0ed211657 100644
> --- a/drivers/tty/serial/8250/8250_dma.c
> +++ b/drivers/tty/serial/8250/8250_dma.c
[ ... ]
> +/* 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;
> +	}
[ ... ]
> +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);
> +			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/20261029222734.3086943-1-rrangel@chromium.org

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