Re: [PATCH v9 3/8] serial: 8250_omap: serialize RX DMA completion and flushing

[Ilpo Järvinen]

On Mon, 2 Nov 2026, James Hilliard wrote:

> Moving RX DMA locking out of __dma_rx_do_complete() left the DMA
> completion callback calling it without rx_dma_lock. Shutdown and runtime
> suspend flush RX under rx_dma_lock but without the port lock, allowing
> these paths to overlap the callback's buffer accounting and flip-buffer
> insertion. RX submission and UART interrupt handling also require the
> port lock.
> 
> Take rx_dma_lock inside the port lock in the completion callback. Hold
> the port lock around the shutdown and runtime-suspend flushes, matching
> the existing interrupt-path lock order. Assert both locks in the common
> completion helper and the port lock at the flush entry point.
> 
> Fixes: 789898416749 ("serial: 8250: 8250_omap: Move locking out from __dma_rx_do_complete()")
> Signed-off-by: James Hilliard <james.hilliard1@xxxxxxxxx>
> ---
>  drivers/tty/serial/8250/8250_omap.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
> index b6f108d34854..322403cec2de 100644
> --- a/drivers/tty/serial/8250/8250_omap.c
> +++ b/drivers/tty/serial/8250/8250_omap.c
> @@ -771,8 +771,10 @@ static void omap_8250_shutdown(struct uart_port *port)
>  	guard(serial8250_rpm)(up);
>  
>  	flush_work(&priv->qos_work);
> -	if (up->dma)
> +	if (up->dma) {
> +		guard(uart_port_lock_irq)(port);
>  		omap_8250_rx_dma_flush(up);
> +	}
>  
>  	serial_out(up, UART_OMAP_WER, 0);
>  	if (priv->habit & UART_HAS_EFR2)
> @@ -898,7 +900,7 @@ static int omap8250_rs485_config(struct uart_port *port,
>  #ifdef CONFIG_SERIAL_8250_DMA
>  static int omap_8250_rx_dma(struct uart_8250_port *p);
>  
> -/* Must be called while priv->rx_dma_lock is held */
> +/* Must be called with the port lock and priv->rx_dma_lock held. */

Nit, following kerneldoc's style, this would start with Context: (you 
don't need to make it full kerneldoc but it is useful to follow the kernel 
doc style where possible).

>  static void __dma_rx_do_complete(struct uart_8250_port *p)
>  {
>  	struct uart_8250_dma    *dma = p->dma;
> @@ -911,6 +913,9 @@ static void __dma_rx_do_complete(struct uart_8250_port *p)
>  	int			ret;
>  	u32			reg;
>  
> +	lockdep_assert_held_once(&p->port.lock);
> +	lockdep_assert_held_once(&priv->rx_dma_lock);
> +
>  	if (!dma->rx_running)
>  		goto out;
>  
> @@ -977,7 +982,8 @@ static void __dma_rx_complete(void *param)
>  	if (dmaengine_tx_status(dma->rxchan, dma->rx_cookie, &state) != DMA_COMPLETE)
>  		return;
>  
> -	__dma_rx_do_complete(p);
> +	scoped_guard(spinlock, &priv->rx_dma_lock)
> +		__dma_rx_do_complete(p);
>  	if (priv->throttled)
>  		return;
>  
> @@ -995,6 +1001,8 @@ static void omap_8250_rx_dma_flush(struct uart_8250_port *p)
>  	unsigned long		flags;
>  	int ret;
>  
> +	lockdep_assert_held_once(&p->port.lock);
> +
>  	spin_lock_irqsave(&priv->rx_dma_lock, flags);
>  
>  	if (!dma->rx_running) {
> @@ -1811,8 +1819,10 @@ static int omap8250_runtime_suspend(struct device *dev)
>  		}
>  	}
>  
> -	if (up && up->dma && up->dma->rxchan)
> +	if (up && up->dma && up->dma->rxchan) {
> +		guard(uart_port_lock_irqsave)(&up->port);
>  		omap_8250_rx_dma_flush(up);
> +	}
>  
>  	priv->latency = PM_QOS_CPU_LATENCY_DEFAULT_VALUE;
>  	schedule_work(&priv->qos_work);
> 
> 

-- 
 i.