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@xxxxxxxxxxxxxxx>
One nit=pick below.
> Reported-by: Gilles Buloz <gilles.buloz@xxxxxxxxxxx>
> Tested-by: Gilles Buloz <gilles.buloz@xxxxxxxxxxx>
> Fixes: 9ee4b83e51f74 ("serial: 8250: Add support for dmaengine")
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
> ---
> 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
