[PATCH 1/1] serial: 8250_dma: Rearm DMA Rx if more data is pending

[PATCH 1/1] serial: 8250_dma: Rearm DMA Rx if more data is pending

[Ilpo Järvinen]

When DMA Rx completes, the current behavior is to just exit the DMA
completion handler without future actions. If the transfer is still
on-going, UART will trigger an interrupt and that eventually rearms the
DMA Rx. The extra interrupt round-trip has an inherent latency cost
that increases the risk of FIFO overrun. In such situations, the
latency margin tends to already be less due to FIFO not being empty.

Add check into DMA Rx completion handler to detect if LSR has DR (Data
Ready) still set. DR indicates there will be more characters pending
and DMA Rx can be rearmed right away to handle them.

Cc: Gilles BULOZ <gilles.buloz@kontron.com>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/tty/serial/8250/8250_dma.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_dma.c b/drivers/tty/serial/8250/8250_dma.c
index b85c82616e8c..37d6af2ec427 100644
--- a/drivers/tty/serial/8250/8250_dma.c
+++ b/drivers/tty/serial/8250/8250_dma.c
@@ -38,9 +38,8 @@ static void __dma_tx_complete(void *param)
 	spin_unlock_irqrestore(&p->port.lock, flags);
 }
 
-static void __dma_rx_complete(void *param)
+static void __dma_rx_complete(struct uart_8250_port *p)
 {
-	struct uart_8250_port	*p = param;
 	struct uart_8250_dma	*dma = p->dma;
 	struct tty_port		*tty_port = &p->port.state->port;
 	struct dma_tx_state	state;
@@ -57,6 +56,20 @@ static void __dma_rx_complete(void *param)
 	tty_flip_buffer_push(tty_port);
 }
 
+static void dma_rx_complete(void *param)
+{
+	struct uart_8250_port *p = param;
+	struct uart_8250_dma *dma = p->dma;
+	unsigned long flags;
+
+	__dma_rx_complete(p);
+
+	spin_lock_irqsave(&p->port.lock, flags);
+	if (!dma->rx_running && (serial_lsr_in(p) & UART_LSR_DR))
+		p->dma->rx_dma(p);
+	spin_unlock_irqrestore(&p->port.lock, flags);
+}
+
 int serial8250_tx_dma(struct uart_8250_port *p)
 {
 	struct uart_8250_dma		*dma = p->dma;
@@ -130,7 +143,7 @@ int serial8250_rx_dma(struct uart_8250_port *p)
 		return -EBUSY;
 
 	dma->rx_running = 1;
-	desc->callback = __dma_rx_complete;
+	desc->callback = dma_rx_complete;
 	desc->callback_param = p;
 
 	dma->rx_cookie = dmaengine_submit(desc);
-- 
2.30.2

Re: [PATCH 1/1] serial: 8250_dma: Rearm DMA Rx if more data is pending

[Andy Shevchenko]

On Mon, Nov 07, 2022 at 12:21:26PM +0200, Ilpo Järvinen wrote:
> When DMA Rx completes, the current behavior is to just exit the DMA
> completion handler without future actions. If the transfer is still
> on-going, UART will trigger an interrupt and that eventually rearms the
> DMA Rx. The extra interrupt round-trip has an inherent latency cost
> that increases the risk of FIFO overrun. In such situations, the
> latency margin tends to already be less due to FIFO not being empty.
> 
> Add check into DMA Rx completion handler to detect if LSR has DR (Data
> Ready) still set. DR indicates there will be more characters pending
> and DMA Rx can be rearmed right away to handle them.

Yep, I used to have something like draft of the below change locally.
Thanks for putting it in shape and upstreamimg!

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Cc: Gilles BULOZ <gilles.buloz@kontron.com>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
>  drivers/tty/serial/8250/8250_dma.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_dma.c b/drivers/tty/serial/8250/8250_dma.c
> index b85c82616e8c..37d6af2ec427 100644
> --- a/drivers/tty/serial/8250/8250_dma.c
> +++ b/drivers/tty/serial/8250/8250_dma.c
> @@ -38,9 +38,8 @@ static void __dma_tx_complete(void *param)
>  	spin_unlock_irqrestore(&p->port.lock, flags);
>  }
>  
> -static void __dma_rx_complete(void *param)
> +static void __dma_rx_complete(struct uart_8250_port *p)
>  {
> -	struct uart_8250_port	*p = param;
>  	struct uart_8250_dma	*dma = p->dma;
>  	struct tty_port		*tty_port = &p->port.state->port;
>  	struct dma_tx_state	state;
> @@ -57,6 +56,20 @@ static void __dma_rx_complete(void *param)
>  	tty_flip_buffer_push(tty_port);
>  }
>  
> +static void dma_rx_complete(void *param)
> +{
> +	struct uart_8250_port *p = param;
> +	struct uart_8250_dma *dma = p->dma;
> +	unsigned long flags;
> +
> +	__dma_rx_complete(p);
> +
> +	spin_lock_irqsave(&p->port.lock, flags);
> +	if (!dma->rx_running && (serial_lsr_in(p) & UART_LSR_DR))
> +		p->dma->rx_dma(p);
> +	spin_unlock_irqrestore(&p->port.lock, flags);
> +}
> +
>  int serial8250_tx_dma(struct uart_8250_port *p)
>  {
>  	struct uart_8250_dma		*dma = p->dma;
> @@ -130,7 +143,7 @@ int serial8250_rx_dma(struct uart_8250_port *p)
>  		return -EBUSY;
>  
>  	dma->rx_running = 1;
> -	desc->callback = __dma_rx_complete;
> +	desc->callback = dma_rx_complete;
>  	desc->callback_param = p;
>  
>  	dma->rx_cookie = dmaengine_submit(desc);
> -- 
> 2.30.2
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/1] serial: 8250_dma: Rearm DMA Rx if more data is pending

[Andy Shevchenko]

On Mon, Nov 07, 2022 at 01:31:52PM +0200, Andy Shevchenko wrote:
> On Mon, Nov 07, 2022 at 12:21:26PM +0200, Ilpo Järvinen wrote:

...

> Yep, I used to have something like draft of the below change locally.
> Thanks for putting it in shape and upstreamimg!
> 
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

...

> > -static void __dma_rx_complete(void *param)
> > +static void __dma_rx_complete(struct uart_8250_port *p)
> >  {
> > -	struct uart_8250_port	*p = param;
> >  	struct uart_8250_dma	*dma = p->dma;

Btw, looking into my patch in archives I noticed that dma can also be passed as
a parameter...

...

> > +static void dma_rx_complete(void *param)
> > +{
> > +	struct uart_8250_port *p = param;

> > +	struct uart_8250_dma *dma = p->dma;

...since you have it already here.

> > +	unsigned long flags;
> > +
> > +	__dma_rx_complete(p);

-- 
With Best Regards,
Andy Shevchenko