Re: [PATCH v5 5/5] 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_dma: dev_warn() called under port lock in serial8250_rx_dma_flush_work()
--

commit 0ccef758673ba1627cbc368a7ea2a4a045241f8f
Author: James Hilliard <james.hilliard1@gmail.com>

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

This commit reserves the RX DMA buffer until a worker has drained canceled
callbacks and falls back to PIO via a recovery worker if pausing, querying
status, or terminating the RX DMA channel fails.

> diff --git a/drivers/tty/serial/8250/8250_dma.c b/drivers/tty/serial/8250/8250_dma.c
> index 1a070967bf571..801221baded0a 100644
> --- a/drivers/tty/serial/8250/8250_dma.c
> +++ b/drivers/tty/serial/8250/8250_dma.c
> @@ -86,53 +86,96 @@ void serial8250_tx_dma_sync_buffer(struct uart_8250_port *p,
[ ... ]
> +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;
> +	bool recover;
> +	int ret;
> +
> +	/* All packed flags share the port lock, including this worker snapshot. */
> +	scoped_guard(uart_port_lock_irqsave, &p->port)
> +		recover = dma->rx_err;
> +
> +	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);
> +	}
> +
> +	guard(serial8250_rpm)(p);
>  
> -	uart_port_lock_irqsave(&p->port, &flags);
> +	guard(uart_port_lock_irqsave)(&p->port);
> +
> +	dma->rx_terminating = false;
> +	dma->rx_running = 0;
>  	if (dma->shutting_down)
> -		goto out;
> -	if (dma->rx_running)
> -		__dma_rx_complete(p);
> +		return;
>  
> -	/*
> -	 * Cannot be combined with the previous check because __dma_rx_complete()
> -	 * changes dma->rx_running.
> -	 */
> -	if (!dma->rx_running && (serial_lsr_in(p) & UART_LSR_DR))
> -		p->dma->rx_dma(p);
> -out:
> -	uart_port_unlock_irqrestore(&p->port, flags);
> +	if (!recover)
> +		return;
> +
> +	/* Apply the current enables, including any intervening stop_rx(). */
> +	serial8250_out_IER(p, p->ier);
> +	dev_warn(p->port.dev, "RX DMA failed, using PIO\n");

[Severity: High]
Does calling dev_warn() while holding p->port.lock with interrupts disabled in
serial8250_rx_dma_flush_work() violate the printk and serial console locking
hierarchy?

When RX DMA recovery is triggered via dma_rx_complete() or
serial8250_rx_dma_flush() -> serial8250_rx_dma_recover(),
serial8250_rx_dma_flush_work() takes guard(uart_port_lock_irqsave)(&p->port)
for the remainder of the function and calls dev_warn() under p->port.lock.

In drivers/tty/serial/serial_core.c, uart_port_spin_lock_init() assigns every
uart_port->lock the shared lockdep class port_lock_key:

  static void uart_port_spin_lock_init(struct uart_port *port)
  {
          spin_lock_init(&port->lock);
          lockdep_set_class(&port->lock, &port_lock_key);
  }

Normal console paths acquire console_lock_dep_map and console_owner_dep_map
outside port_lock_key. Calling dev_warn() while holding p->port.lock enters:

  vprintk_emit()
    -> console_trylock_spinning()
       -> console_flush_all()
          -> console_emit_next_record()
             -> console_lock_spinning_enable()

which acquires console_owner_dep_map inside port_lock_key and invokes legacy
serial console write callbacks that also acquire port_lock_key, risking lockdep
splats or a console-owner spin deadlock.

Should the port lock use scoped_guard(uart_port_lock_irqsave, &p->port) so
that p->port.lock is released after serial8250_out_IER(p, p->ier) and before
dev_warn() is called?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261024-submit-serial-8250-dma-cancel-v1-v5-0-9e52968928d2@gmail.com?part=5