[PATCH 0/1] Fix 8250 XON/XOFF for DMA-capable devices

[PATCH 0/1] Fix 8250 XON/XOFF for DMA-capable devices

[Ilpo Järvinen]

DMA-capable 8250 was not properly taking XON/XOFF into account
right since it was introduced in 2013. There was no attempt to
send x_char out at all which is fixed by this patch.

After making this fix, it was further reported by Gilles Buloz
that also rx side fails to work as expected if the receiving
side is not reading from tty. I took look into that issue and
and determined that XOFF char gets stuck in an intermediate
buffer until enough data can be processed in receive_buf().
So it's not very useful from flow control perspective :-). While
the problem is more likely to occur with DMA, the code does not
look robust and the problem can likely occur also w/o DMA.

I was thinking of fixing the rx side issue by adding lookahead.
The most natural place to call lookahead handler would seem
to be from flush_to_ldisc() in the case when receive_buf() does
not eat all characters that were passed to it. Does anybody have
other opinions on how it should be fixed?

Ilpo Järvinen (1):
  serial: 8250: fix XOFF/XON sending when DMA is used

 drivers/tty/serial/8250/8250_dma.c  | 11 ++++++++++-
 drivers/tty/serial/8250/8250_port.c |  4 +---
 drivers/tty/serial/serial_core.c    | 14 ++++++++++++++
 include/linux/serial_core.h         |  2 ++
 4 files changed, 27 insertions(+), 4 deletions(-)

-- 
2.30.2

[PATCH 1/1] serial: 8250: fix XOFF/XON sending when DMA is used

[Ilpo Järvinen]

When 8250 UART is using DMA, x_char (XON/XOFF) is never sent
to the wire. After this change, x_char is injected correctly.

Create uart_xchar_out() helper for sending the x_char out and
accounting related to it. It seems that almost every driver
does these same steps with x_char. Except for 8250, however,
almost all currently lack .serial_out so they cannot immediately
take advantage of this new helper.

The downside of this patch is that it might reintroduce
the problems some devices faced with mixed DMA/non-DMA transfer
which caused revert f967fc8f165f (Revert "serial: 8250_dma:
don't bother DMA with small transfers"). However, the impact
should be limited to cases with XON/XOFF (that didn't work
with DMA capable devices to begin with so this problem is not
very likely to cause a major issue, if any at all).

Reported-by: Gilles Buloz <gilles.buloz@kontron.com>
Tested-by: Gilles Buloz <gilles.buloz@kontron.com>
Fixes: 9ee4b83e51f74 ("serial: 8250: Add support for dmaengine")
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/tty/serial/8250/8250_dma.c  | 11 ++++++++++-
 drivers/tty/serial/8250/8250_port.c |  4 +---
 drivers/tty/serial/serial_core.c    | 14 ++++++++++++++
 include/linux/serial_core.h         |  2 ++
 4 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_dma.c b/drivers/tty/serial/8250/8250_dma.c
index 890fa7ddaa7f..b3c3f7e5851a 100644
--- a/drivers/tty/serial/8250/8250_dma.c
+++ b/drivers/tty/serial/8250/8250_dma.c
@@ -64,10 +64,19 @@ int serial8250_tx_dma(struct uart_8250_port *p)
 	struct uart_8250_dma		*dma = p->dma;
 	struct circ_buf			*xmit = &p->port.state->xmit;
 	struct dma_async_tx_descriptor	*desc;
+	struct uart_port		*up = &p->port;
 	int ret;
 
-	if (dma->tx_running)
+	if (dma->tx_running) {
+		if (up->x_char) {
+			dmaengine_pause(dma->txchan);
+			uart_xchar_out(up, UART_TX);
+			dmaengine_resume(dma->txchan);
+		}
 		return 0;
+	} else if (up->x_char) {
+		uart_xchar_out(up, UART_TX);
+	}
 
 	if (uart_tx_stopped(&p->port) || uart_circ_empty(xmit)) {
 		/* We have been called from __dma_tx_complete() */
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 3b12bfc1ed67..63e9bc6fce06 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1799,9 +1799,7 @@ void serial8250_tx_chars(struct uart_8250_port *up)
 	int count;
 
 	if (port->x_char) {
-		serial_out(up, UART_TX, port->x_char);
-		port->icount.tx++;
-		port->x_char = 0;
+		uart_xchar_out(port, UART_TX);
 		return;
 	}
 	if (uart_tx_stopped(port)) {
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 0db90be4c3bc..f67540ae2a88 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -644,6 +644,20 @@ static void uart_flush_buffer(struct tty_struct *tty)
 	tty_port_tty_wakeup(&state->port);
 }
 
+/*
+ * This function performs low-level write of high-priority XON/XOFF
+ * character and accounting for it.
+ *
+ * Requires uart_port to implement .serial_out().
+ */
+void uart_xchar_out(struct uart_port *uport, int offset)
+{
+	serial_port_out(uport, offset, uport->x_char);
+	uport->icount.tx++;
+	uport->x_char = 0;
+}
+EXPORT_SYMBOL_GPL(uart_xchar_out);
+
 /*
  * This function is used to send a high-priority XON/XOFF character to
  * the device
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index c58cc142d23f..8c32935e1059 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -458,6 +458,8 @@ extern void uart_handle_cts_change(struct uart_port *uport,
 extern void uart_insert_char(struct uart_port *port, unsigned int status,
 		 unsigned int overrun, unsigned int ch, unsigned int flag);
 
+void uart_xchar_out(struct uart_port *uport, int offset);
+
 #ifdef CONFIG_MAGIC_SYSRQ_SERIAL
 #define SYSRQ_TIMEOUT	(HZ * 5)
 
-- 
2.30.2

Re: [PATCH 1/1] serial: 8250: fix XOFF/XON sending when DMA is used

[Andy Shevchenko]

On Mon, Mar 14, 2022 at 11:14:32AM +0200, Ilpo Järvinen wrote:
> When 8250 UART is using DMA, x_char (XON/XOFF) is never sent
> to the wire. After this change, x_char is injected correctly.
> 
> Create uart_xchar_out() helper for sending the x_char out and
> accounting related to it. It seems that almost every driver
> does these same steps with x_char. Except for 8250, however,
> almost all currently lack .serial_out so they cannot immediately
> take advantage of this new helper.
> 
> The downside of this patch is that it might reintroduce
> the problems some devices faced with mixed DMA/non-DMA transfer
> which caused revert f967fc8f165f (Revert "serial: 8250_dma:
> don't bother DMA with small transfers"). However, the impact
> should be limited to cases with XON/XOFF (that didn't work
> with DMA capable devices to begin with so this problem is not
> very likely to cause a major issue, if any at all).

LGTM,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
One nit=pick below.

> Reported-by: Gilles Buloz <gilles.buloz@kontron.com>
> Tested-by: Gilles Buloz <gilles.buloz@kontron.com>
> Fixes: 9ee4b83e51f74 ("serial: 8250: Add support for dmaengine")
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
>  drivers/tty/serial/8250/8250_dma.c  | 11 ++++++++++-
>  drivers/tty/serial/8250/8250_port.c |  4 +---
>  drivers/tty/serial/serial_core.c    | 14 ++++++++++++++
>  include/linux/serial_core.h         |  2 ++
>  4 files changed, 27 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_dma.c b/drivers/tty/serial/8250/8250_dma.c
> index 890fa7ddaa7f..b3c3f7e5851a 100644
> --- a/drivers/tty/serial/8250/8250_dma.c
> +++ b/drivers/tty/serial/8250/8250_dma.c
> @@ -64,10 +64,19 @@ int serial8250_tx_dma(struct uart_8250_port *p)
>  	struct uart_8250_dma		*dma = p->dma;
>  	struct circ_buf			*xmit = &p->port.state->xmit;
>  	struct dma_async_tx_descriptor	*desc;
> +	struct uart_port		*up = &p->port;
>  	int ret;
>  
> -	if (dma->tx_running)
> +	if (dma->tx_running) {
> +		if (up->x_char) {
> +			dmaengine_pause(dma->txchan);
> +			uart_xchar_out(up, UART_TX);
> +			dmaengine_resume(dma->txchan);
> +		}
>  		return 0;
> +	} else if (up->x_char) {
> +		uart_xchar_out(up, UART_TX);
> +	}

This can be written as

	if (dma->tx_running) {
		...
		return 0;
	}

	if (up->x_char)
		uart_xchar_out(up, UART_TX);

But I'm fine with the original code (it might make sense to have redundant
'else' just to keep xON/xOFF handling grouped).

>  	if (uart_tx_stopped(&p->port) || uart_circ_empty(xmit)) {
>  		/* We have been called from __dma_tx_complete() */
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index 3b12bfc1ed67..63e9bc6fce06 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -1799,9 +1799,7 @@ void serial8250_tx_chars(struct uart_8250_port *up)
>  	int count;
>  
>  	if (port->x_char) {
> -		serial_out(up, UART_TX, port->x_char);
> -		port->icount.tx++;
> -		port->x_char = 0;
> +		uart_xchar_out(port, UART_TX);
>  		return;
>  	}
>  	if (uart_tx_stopped(port)) {
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 0db90be4c3bc..f67540ae2a88 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -644,6 +644,20 @@ static void uart_flush_buffer(struct tty_struct *tty)
>  	tty_port_tty_wakeup(&state->port);
>  }
>  
> +/*
> + * This function performs low-level write of high-priority XON/XOFF
> + * character and accounting for it.
> + *
> + * Requires uart_port to implement .serial_out().
> + */
> +void uart_xchar_out(struct uart_port *uport, int offset)
> +{
> +	serial_port_out(uport, offset, uport->x_char);
> +	uport->icount.tx++;
> +	uport->x_char = 0;
> +}
> +EXPORT_SYMBOL_GPL(uart_xchar_out);
> +
>  /*
>   * This function is used to send a high-priority XON/XOFF character to
>   * the device
> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> index c58cc142d23f..8c32935e1059 100644
> --- a/include/linux/serial_core.h
> +++ b/include/linux/serial_core.h
> @@ -458,6 +458,8 @@ extern void uart_handle_cts_change(struct uart_port *uport,
>  extern void uart_insert_char(struct uart_port *port, unsigned int status,
>  		 unsigned int overrun, unsigned int ch, unsigned int flag);
>  
> +void uart_xchar_out(struct uart_port *uport, int offset);
> +
>  #ifdef CONFIG_MAGIC_SYSRQ_SERIAL
>  #define SYSRQ_TIMEOUT	(HZ * 5)
>  
> -- 
> 2.30.2
> 

-- 
With Best Regards,
Andy Shevchenko