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

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

Posted by Jiri Slaby (SUSE) 7 months ago

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

---
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;
-- 
2.49.0

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

Posted by Greg KH 6 months, 4 weeks ago

On Wed, May 14, 2025 at 09:20:35AM +0200, 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
> 
> ---

You forgot to sign off on this patch :(

Can you resend it with that?

thanks,

greg k-h

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

Posted by Jiri Slaby 6 months, 3 weeks ago

On 21. 05. 25, 13:38, Greg KH wrote:
>> Fixes: 1788cf6a91d9 ("tty: serial: switch from circ_buf to kfifo")
>> Reported-by: Mans Rullgard <mans@mansr.com>
>> Cc: stable@vger.kernel.org
>>
>> ---
> 
> You forgot to sign off on this patch :(

Huh.

> Can you resend it with that?

Sure:
20250522053835.3495975-1-jirislaby@kernel.org

-- 
js
suse labs

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

Posted by Måns Rullgård 7 months ago

"Jiri Slaby (SUSE)" <jirislaby@kernel.org> writes:

> 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
>
> ---
> 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.

Are you seriously expecting me to waste even more time on this?
Do your damn job like you should have to begin with.

-- 
Måns Rullgård

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

Posted by Jiri Slaby 7 months ago

On 14. 05. 25, 10:41, Måns Rullgård wrote:
>> A patch to optimize the driver to use 2 sgls is still welcome. I will
>> not add it without actually having the HW.
> 
> Are you seriously expecting me to waste even more time on this?

It needs not be namely you.

> Do your damn job like you should have to begin with.

Well, in the first place, I'm not your slave. It's definitely not my job 
to optimize code for you esp. in case I don't have the HW.

-- 
js
suse labs

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

Posted by Greg KH 7 months ago

On Wed, May 14, 2025 at 09:41:16AM +0100, Måns Rullgård wrote:
> "Jiri Slaby (SUSE)" <jirislaby@kernel.org> writes:
> 
> > 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
> >
> > ---
> > 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.
> 
> Are you seriously expecting me to waste even more time on this?
> Do your damn job like you should have to begin with.

That type of response is not allowed here.  Please refrain from this in
the future.

greg k-h

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

Posted by Jiri Slaby 7 months ago

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