* Re: [PATCH] tty: serial: 8250_omap: fix tx with dma
[not found] <20250430163709.15850-1-mans@mansr.com>
@ 2025-05-05 11:43 ` Geert Uytterhoeven
0 siblings, 0 replies; only message in thread
From: Geert Uytterhoeven @ 2025-05-05 11:43 UTC (permalink / raw)
To: Mans Rullgard
Cc: Greg Kroah-Hartman, Jiri Slaby, linux-kernel, linux-serial,
Kevin Hilman, Aaro Koskinen, Andreas Kemnade, Roger Quadros,
Tony Lindgren, open list:TI ETHERNET SWITCH DRIVER (CPSW),
Matti Vaittinen
CC omap
On Wed, 30 Apr 2025 at 18:45, Mans Rullgard <mans@mansr.com> wrote:
> Commit 1788cf6a91d9 ("tty: serial: switch from circ_buf to kfifo")
> introduced two errors in the TX DMA handling for 8250_omap.
>
> Firstly, kfifo_dma_out_prepare_mapped() needs a scatterlist with two
> entries whereas only one is provided. The same error was fixed for
> 8250_dma in 59449c9dbdaa ("tty: serial: 8250_dma: use sgl with 2 nents
> to take care of buffer wrap").
>
> Secondly, when the OMAP_DMA_TX_KICK flag is set, one byte is pulled from
> the kfifo and emitted directly in order to start the DMA. This is done
> without updating DMA tx_size which leads to uart_xmit_advance() called
> in the DMA complete callback 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 both problems.
>
> Fixes: 1788cf6a91d9 ("tty: serial: switch from circ_buf to kfifo")
> Signed-off-by: Mans Rullgard <mans@mansr.com>
> ---
> drivers/tty/serial/8250/8250_omap.c | 35 +++++++++++++++--------------
> 1 file changed, 18 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
> index f1aee915bc02..84a2f013015e 100644
> --- a/drivers/tty/serial/8250/8250_omap.c
> +++ b/drivers/tty/serial/8250/8250_omap.c
> @@ -1152,9 +1152,11 @@ static int omap_8250_tx_dma(struct uart_8250_port *p)
> struct omap8250_priv *priv = p->port.private_data;
> struct tty_port *tport = &p->port.state->port;
> struct dma_async_tx_descriptor *desc;
> - struct scatterlist sg;
> + struct scatterlist *sg;
> + struct scatterlist sgl[2];
> int skip_byte = -1;
> int ret;
> + int i;
>
> if (dma->tx_running)
> return 0;
> @@ -1173,16 +1175,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;
> @@ -1207,7 +1199,7 @@ 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;
> }
> @@ -1216,12 +1208,19 @@ static int omap_8250_tx_dma(struct uart_8250_port *p)
> 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);
> }
>
> - desc = dmaengine_prep_slave_sg(dma->txchan, &sg, 1, DMA_MEM_TO_DEV,
> + sg_init_table(sgl, ARRAY_SIZE(sgl));
> +
> + ret = kfifo_dma_out_prepare_mapped(&tport->xmit_fifo, sgl, ARRAY_SIZE(sgl),
> + UART_XMIT_SIZE, dma->tx_addr);
> +
> + dma->tx_size = 0;
> +
> + for_each_sg(sgl, sg, ret, i)
> + dma->tx_size += sg_dma_len(sg);
> +
> + desc = dmaengine_prep_slave_sg(dma->txchan, sgl, ret, DMA_MEM_TO_DEV,
> DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
> if (!desc) {
> ret = -EBUSY;
> @@ -1248,8 +1247,10 @@ static int omap_8250_tx_dma(struct uart_8250_port *p)
> err:
> dma->tx_err = 1;
> out_skip:
> - if (skip_byte >= 0)
> + if (skip_byte >= 0) {
> serial_out(p, UART_TX, skip_byte);
> + p->port.icount.tx++;
> + }
> return ret;
> }
>
> --
> 2.49.0
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] only message in thread