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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:   Mon, 14 Mar 2022 12:23:22 +0200
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc:     linux-serial@...r.kernel.org, Greg KH <gregkh@...uxfoundation.org>,
        Jiri Slaby <jirislaby@...nel.org>,
        linux-kernel@...r.kernel.org, Johan Hovold <johan@...nel.org>,
        Miquel Raynal <miquel.raynal@...tlin.com>,
        Lukas Wunner <lukas@...ner.de>,
        Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        Gilles Buloz <gilles.buloz@...tron.com>
Subject: Re: [PATCH 1/1] serial: 8250: fix XOFF/XON sending when DMA is used

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@...ux.intel.com>
One nit=pick below.

> Reported-by: Gilles Buloz <gilles.buloz@...tron.com>
> Tested-by: Gilles Buloz <gilles.buloz@...tron.com>
> Fixes: 9ee4b83e51f74 ("serial: 8250: Add support for dmaengine")
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@...ux.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