Re: [PATCH v2 1/2] tty: serial: 8250_omap: fix tx with dma

[Jiri Slaby <jirislaby@kernel.org>]

On 06. 05. 25, 17:07, Mans Rullgard 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, 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 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. At the point kfifo_dma_out_prepare_mapped is called,
> at least one byte is guaranteed to be in the fifo, so checking the
> return value is not necessary.
> 
> Fixes: 1788cf6a91d9 ("tty: serial: switch from circ_buf to kfifo")
> Cc: stable@vger.kernel.org
> Signed-off-by: Mans Rullgard <mans@mansr.com>
> ---
> v2: split patch in two
> ---
>   drivers/tty/serial/8250/8250_omap.c | 24 +++++++++---------------
>   1 file changed, 9 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
> index f1aee915bc02..180466e09605 100644
> --- a/drivers/tty/serial/8250/8250_omap.c
> +++ b/drivers/tty/serial/8250/8250_omap.c
> @@ -1173,16 +1173,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;
...
> @@ -1216,11 +1206,12 @@ 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);
>   	}
>   
> +	sg_init_table(&sg, 1);
> +	kfifo_dma_out_prepare_mapped(&tport->xmit_fifo, &sg, 1,
> +				     UART_XMIT_SIZE, dma->tx_addr);

This can fail (note the first call to this was checked). The latter 
(deliberately) not.

> +
>   	desc = dmaengine_prep_slave_sg(dma->txchan, &sg, 1, DMA_MEM_TO_DEV,
>   			DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
>   	if (!desc) {
...
> @@ -1248,8 +1240,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++;

This is still unrelated.

thanks,
-- 
js
suse labs