Message-ID: <sashiko-outbox-204402@kernel.org>
From: sashiko-bot@kernel.org
Subject: Re: [PATCH v10 7/8] serial: 8250: retire canceled RX DMA before
 reusing the receive buffer
Reply-To: sashiko-reviews@lists.linux.dev
To: "James Hilliard" <james.hilliard1@gmail.com>
Cc: linux-serial@vger.kernel.org, imx@lists.linux.dev, Frank.Li@kernel.org
In-Reply-To: <20261108-submit-serial-8250-dma-cancel-v1-v10-7-e6227ddb95b1@gmail.com>
References: <20261108-submit-serial-8250-dma-cancel-v1-v10-0-e6227ddb95b1@gmail.com>
 <20261108-submit-serial-8250-dma-cancel-v1-v10-7-e6227ddb95b1@gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Date: Mon, 09 Nov 2026 04:22:32 +0000
Precedence: list

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

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.

> diff --git a/drivers/tty/serial/8250/8250_dma.c b/drivers/tty/serial/8250=
/8250_dma.c
> index 739a8238e4dec..1ea22954150ca 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 =3D p->dma;
> +	struct dma_tx_state state;
> +	enum dma_status status;
> +	int ret;
> +
> +	lockdep_assert_held_once(&p->port.lock);
> +
> +	ret =3D dmaengine_pause(dma->rxchan);
> +	status =3D dmaengine_tx_status(dma->rxchan, dma->rx_cookie, &state);
> +	if (status =3D=3D DMA_ERROR) {
> +		ret =3D -EIO;
> +		goto recover;
> +	}
> +	if (ret && status !=3D 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 =3D 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 =3D container_of(work, struct uart_8250_dma, =
rx_flush_work);
> +	struct uart_8250_port *p =3D dma->port;
> +	unsigned int delay_us =3D 1 * USEC_PER_MSEC;
> +	ktime_t start =3D ktime_get();
> +	bool recover, retry, restore_rx;
> +	bool warned =3D 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 =3D dma->rx_pause_pending && !dma->rx_terminating;
> +			if (retry) {
> +				ret =3D serial8250_rx_dma_try_flush(p);
> +				retry =3D dma->rx_pause_pending && !dma->rx_terminating;
> +			}
> +			recover =3D dma->rx_err;
> +		}
> +		if (!retry)
> +			break;
> +		if (!warned && ktime_ms_delta(ktime_get(), start) >=3D 1 * MSEC_PER_SE=
C) {
> +			dev_warn(p->port.dev,
> +				 "RX DMA pause stalled: %d; retaining transfer and retrying\n",
> +				 ret);
> +			warned =3D true;
> +		}
> +		/* Notifications also cover completion or shutdown before sleeping. */
> +		wait_event_hrtimeout(dma->rx_pause_wait, READ_ONCE(dma->rx_pause_wakeu=
p),
> +				     ns_to_ktime(delay_us * NSEC_PER_USEC));
> +		delay_us =3D 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 =3D 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/20261029222734.3086943-1-rrangel@chromium.org

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20261108-submit-ser=
ial-8250-dma-cancel-v1-v10-0-e6227ddb95b1@gmail.com?part=3D7