[PATCH/RFC v1 0/1]

[PATCH/RFC v1 0/1]

[Ferry Toth]

Since 1788cf6a91d9 "tty: serial: switch from circ_buf to kfifo" serial 
transmit when using DMA uses kfifo with a SG list with a single entry. 
This obviously lead to 2 separate DMA transfers when the buffer wraps 
around.  

On the receiving end depending on the UART the receive DMA might 
terminate when there is a moment of silence (an interchar gap) of a few 
(typcially 5) characters length. The receivers FIFO will help to extend 
that time (depending on FIFO length and threshold).  Currently high 
speed UARTs (HSU) can have baud rates of 3.5MBd which means terminating 
a DMA transfer and handling a receive interrupt to set up a new DMA 
transfer has to complete in 180us which is hard to guarantee. Especially 
under heavy interrupt load.

As most serial protocols will transfer maximally a buffer length at a 
time it makes sense to eliminate the interchar gap on buffer wraps by 
using a SG list with 2 entries.

The following patch has been tested on Merrifield (Intel Edison) which 
is slow (500MHz) but has a HSU that can transmit up to 3.5MBd.

This patch should be viewed as a preparation for a follow-up patch that 
remove the interrupt on the receiving end entirely.

But first I would like to make sure that: 1) either all users can 
actually handle 2 SG entries 2) if not, get your advice on how to best 
make the number of entries configurable per platform

Possibly on RZN1 due to aa63d786cea2 ("serial: 8250: dw: Add support for 
DMA flow controlling devices") we might anticipate problems.

Please test on your platform and let me know  your suggestions.

Thanks!

Ferry Toth (1):
  tty: serial: 8250_dma: use sgl with 2 nents to take care of buffer
    wrap

 drivers/tty/serial/8250/8250_dma.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

-- 
2.43.0

[PATCH/RFC v1 1/1] tty: serial: 8250_dma: use sgl with 2 nents to take care of buffer wrap

[Ferry Toth]

Previously 8250_dma used a circular xmit->buf as DMA output buffer. This
causes messages that wrap around in the circular buffer to be
transmitted using 2 DMA transfers. Depending on baud rate and processor
load this can cause an interchar gap in the middle of the message. On
the receiving end the gap may cause a short receive timeout, possibly
long enough to terminate a DMA transfer, but too short to restart a
receive DMA transfer in time thus causing a receive buffer overrun.

This is especially a problem for devices with high speed UARTs (HSU)
where even deep 64 byte FIFO's are not sufficient to handle interrupt
latency.

The circular buffer has now been replaced by kfifo which requires a SG
list with a single entry, which still causes 2 dma transfers when a wrap
around occurs. Fix this by allowing up to 2 entries in the sgl.

Signed-off-by: Ferry Toth <ftoth@exalondelft.nl>
---
 drivers/tty/serial/8250/8250_dma.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_dma.c b/drivers/tty/serial/8250/8250_dma.c
index 8a353e3cc3dd..d215c494ee24 100644
--- a/drivers/tty/serial/8250/8250_dma.c
+++ b/drivers/tty/serial/8250/8250_dma.c
@@ -89,7 +89,9 @@ int serial8250_tx_dma(struct uart_8250_port *p)
 	struct tty_port			*tport = &p->port.state->port;
 	struct dma_async_tx_descriptor	*desc;
 	struct uart_port		*up = &p->port;
-	struct scatterlist sg;
+	struct scatterlist		*sg;
+	struct scatterlist		sgl[2];
+	int i;
 	int ret;
 
 	if (dma->tx_running) {
@@ -110,18 +112,17 @@ int serial8250_tx_dma(struct uart_8250_port *p)
 
 	serial8250_do_prepare_tx_dma(p);
 
-	sg_init_table(&sg, 1);
-	/* kfifo can do more than one sg, we don't (quite yet) */
-	ret = kfifo_dma_out_prepare_mapped(&tport->xmit_fifo, &sg, 1,
+	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);
 
-	/* we already checked empty fifo above, so there should be something */
-	if (WARN_ON_ONCE(ret != 1))
-		return 0;
+	dma->tx_size = 0;
 
-	dma->tx_size = sg_dma_len(&sg);
+	for_each_sg(sgl, sg, ret, i)
+		dma->tx_size += sg_dma_len(sg);
 
-	desc = dmaengine_prep_slave_sg(dma->txchan, &sg, 1,
+	desc = dmaengine_prep_slave_sg(dma->txchan, sgl, ret,
 				       DMA_MEM_TO_DEV,
 				       DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
 	if (!desc) {
-- 
2.43.0

Re: [PATCH/RFC v1 1/1] tty: serial: 8250_dma: use sgl with 2 nents to take care of buffer wrap

[Jiri Slaby]

On 03. 07. 24, 22:56, Ferry Toth wrote:
> Previously 8250_dma used a circular xmit->buf as DMA output buffer. This
> causes messages that wrap around in the circular buffer to be
> transmitted using 2 DMA transfers. Depending on baud rate and processor
> load this can cause an interchar gap in the middle of the message. On
> the receiving end the gap may cause a short receive timeout, possibly
> long enough to terminate a DMA transfer, but too short to restart a
> receive DMA transfer in time thus causing a receive buffer overrun.
> 
> This is especially a problem for devices with high speed UARTs (HSU)
> where even deep 64 byte FIFO's are not sufficient to handle interrupt
> latency.
> 
> The circular buffer has now been replaced by kfifo which requires a SG
> list with a single entry, which still causes 2 dma transfers when a wrap
> around occurs. Fix this by allowing up to 2 entries in the sgl.

As I stated earlier, from the DMA and TTY perspective, this looks all 
good™. So I welcome this.

 From the devices perspective, obviously testers needed ;). I believe we 
can merge this in 6.12-rc1 (or even 6.11-rc1?) and see. So please post a 
non-RFC patch.

Reviewed-by: Jiri Slaby <jirislaby@kernel.org>

> Signed-off-by: Ferry Toth <ftoth@exalondelft.nl>
> ---
>   drivers/tty/serial/8250/8250_dma.c | 19 ++++++++++---------
>   1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_dma.c b/drivers/tty/serial/8250/8250_dma.c
> index 8a353e3cc3dd..d215c494ee24 100644
> --- a/drivers/tty/serial/8250/8250_dma.c
> +++ b/drivers/tty/serial/8250/8250_dma.c
> @@ -89,7 +89,9 @@ int serial8250_tx_dma(struct uart_8250_port *p)
>   	struct tty_port			*tport = &p->port.state->port;
>   	struct dma_async_tx_descriptor	*desc;
>   	struct uart_port		*up = &p->port;
> -	struct scatterlist sg;
> +	struct scatterlist		*sg;
> +	struct scatterlist		sgl[2];
> +	int i;
>   	int ret;
>   
>   	if (dma->tx_running) {
> @@ -110,18 +112,17 @@ int serial8250_tx_dma(struct uart_8250_port *p)
>   
>   	serial8250_do_prepare_tx_dma(p);
>   
> -	sg_init_table(&sg, 1);
> -	/* kfifo can do more than one sg, we don't (quite yet) */
> -	ret = kfifo_dma_out_prepare_mapped(&tport->xmit_fifo, &sg, 1,
> +	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);
>   
> -	/* we already checked empty fifo above, so there should be something */
> -	if (WARN_ON_ONCE(ret != 1))
> -		return 0;
> +	dma->tx_size = 0;
>   
> -	dma->tx_size = sg_dma_len(&sg);
> +	for_each_sg(sgl, sg, ret, i)
> +		dma->tx_size += sg_dma_len(sg);
>   
> -	desc = dmaengine_prep_slave_sg(dma->txchan, &sg, 1,
> +	desc = dmaengine_prep_slave_sg(dma->txchan, sgl, ret,
>   				       DMA_MEM_TO_DEV,
>   				       DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
>   	if (!desc) {

-- 
js