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

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,