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;
/* 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;