Re: [PATCH] tty: serial: 8250_omap: fix TX with DMA for am33xx

[Jiri Slaby <jirislaby@kernel.org>]

On 14. 05. 25, 9:20, Jiri Slaby (SUSE) wrote:
> Commit 1788cf6a91d9 ("tty: serial: switch from circ_buf to kfifo")
> introduced an error in the TX DMA handling for 8250_omap.
> 
> When the OMAP_DMA_TX_KICK flag is set, the "skip_byte" is pulled from
> the kfifo and emitted directly in order to start the DMA. While the
> kfifo is updated, dma->tx_size is not decreased. This leads to
> uart_xmit_advance() called in omap_8250_dma_tx_complete() advancing the
> kfifo by one too much.
> 
> In practice, transmitting N bytes has been seen to result in the last
> N-1 bytes being sent repeatedly.
> 
> This change fixes the problem by moving all of the dma setup after the
> OMAP_DMA_TX_KICK handling and using kfifo_len() instead of the DMA size
> for the 4-byte cutoff check. This slightly changes the behaviour at
> buffer wraparound, but it still transmits the correct bytes somehow.
> 
> Now, the "skip_byte" would no longer be accounted to the stats. As
> previously, dma->tx_size included also this skip byte, up->icount.tx was
> updated by aforementioned uart_xmit_advance() in
> omap_8250_dma_tx_complete(). Fix this by using the uart_fifo_out()
> helper instead of bare kfifo_get().
> 
> Based on patch by Mans Rullgard <mans@mansr.com>
> 
> Fixes: 1788cf6a91d9 ("tty: serial: switch from circ_buf to kfifo")
> Reported-by: Mans Rullgard <mans@mansr.com>
> Cc: stable@vger.kernel.org

I should have added this too:
Link: https://lore.kernel.org/all/20250506150748.3162-1-mans@mansr.com/

> ---
> The same as for the original patch, I would appreaciate if someone
> actually tests this one on a real HW too.
> 
> A patch to optimize the driver to use 2 sgls is still welcome. I will
> not add it without actually having the HW.
> ---
>   drivers/tty/serial/8250/8250_omap.c | 25 ++++++++++---------------
>   1 file changed, 10 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
> index c9b1c689a045..bb23afdd63f2 100644
> --- a/drivers/tty/serial/8250/8250_omap.c
> +++ b/drivers/tty/serial/8250/8250_omap.c
> @@ -1151,16 +1151,6 @@ static int omap_8250_tx_dma(struct uart_8250_port *p)
>   		return 0;
>   	}
>   
> -	sg_init_table(&sg, 1);
> -	ret = kfifo_dma_out_prepare_mapped(&tport->xmit_fifo, &sg, 1,
> -					   UART_XMIT_SIZE, dma->tx_addr);
> -	if (ret != 1) {
> -		serial8250_clear_THRI(p);
> -		return 0;
> -	}
> -
> -	dma->tx_size = sg_dma_len(&sg);
> -
>   	if (priv->habit & OMAP_DMA_TX_KICK) {
>   		unsigned char c;
>   		u8 tx_lvl;
> @@ -1185,18 +1175,22 @@ static int omap_8250_tx_dma(struct uart_8250_port *p)
>   			ret = -EBUSY;
>   			goto err;
>   		}
> -		if (dma->tx_size < 4) {
> +		if (kfifo_len(&tport->xmit_fifo) < 4) {
>   			ret = -EINVAL;
>   			goto err;
>   		}
> -		if (!kfifo_get(&tport->xmit_fifo, &c)) {
> +		if (!uart_fifo_out(&p->port, &c, 1)) {
>   			ret = -EINVAL;
>   			goto err;
>   		}
>   		skip_byte = c;
> -		/* now we need to recompute due to kfifo_get */
> -		kfifo_dma_out_prepare_mapped(&tport->xmit_fifo, &sg, 1,
> -				UART_XMIT_SIZE, dma->tx_addr);
> +	}
> +
> +	sg_init_table(&sg, 1);
> +	ret = kfifo_dma_out_prepare_mapped(&tport->xmit_fifo, &sg, 1, UART_XMIT_SIZE, dma->tx_addr);
> +	if (ret != 1) {
> +		ret = -EINVAL;
> +		goto err;
>   	}
>   
>   	desc = dmaengine_prep_slave_sg(dma->txchan, &sg, 1, DMA_MEM_TO_DEV,
> @@ -1206,6 +1200,7 @@ static int omap_8250_tx_dma(struct uart_8250_port *p)
>   		goto err;
>   	}
>   
> +	dma->tx_size = sg_dma_len(&sg);
>   	dma->tx_running = 1;
>   
>   	desc->callback = omap_8250_dma_tx_complete;


-- 
js
suse labs