* [PATCH 0/1] Fix 8250 XON/XOFF for DMA-capable devices @ 2022-03-14 9:14 Ilpo Järvinen 2022-03-14 9:14 ` [PATCH 1/1] serial: 8250: fix XOFF/XON sending when DMA is used Ilpo Järvinen 0 siblings, 1 reply; 3+ messages in thread From: Ilpo Järvinen @ 2022-03-14 9:14 UTC (permalink / raw) To: linux-serial, Greg KH, Jiri Slaby Cc: linux-kernel, Johan Hovold, Andy Shevchenko, Miquel Raynal, Lukas Wunner, Heikki Krogerus, Gilles Buloz, 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* [PATCH 1/1] serial: 8250: fix XOFF/XON sending when DMA is used 2022-03-14 9:14 [PATCH 0/1] Fix 8250 XON/XOFF for DMA-capable devices Ilpo Järvinen @ 2022-03-14 9:14 ` Ilpo Järvinen 2022-03-14 10:23 ` Andy Shevchenko 0 siblings, 1 reply; 3+ messages in thread From: Ilpo Järvinen @ 2022-03-14 9:14 UTC (permalink / raw) To: linux-serial, Greg KH, Jiri Slaby Cc: linux-kernel, Johan Hovold, Andy Shevchenko, Miquel Raynal, Lukas Wunner, Heikki Krogerus, Gilles Buloz, 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 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH 1/1] serial: 8250: fix XOFF/XON sending when DMA is used 2022-03-14 9:14 ` [PATCH 1/1] serial: 8250: fix XOFF/XON sending when DMA is used Ilpo Järvinen @ 2022-03-14 10:23 ` Andy Shevchenko 0 siblings, 0 replies; 3+ messages in thread From: Andy Shevchenko @ 2022-03-14 10:23 UTC (permalink / raw) To: Ilpo Järvinen Cc: linux-serial, Greg KH, Jiri Slaby, linux-kernel, Johan Hovold, Miquel Raynal, Lukas Wunner, Heikki Krogerus, Gilles Buloz 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-03-14 10:24 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-03-14 9:14 [PATCH 0/1] Fix 8250 XON/XOFF for DMA-capable devices Ilpo Järvinen 2022-03-14 9:14 ` [PATCH 1/1] serial: 8250: fix XOFF/XON sending when DMA is used Ilpo Järvinen 2022-03-14 10:23 ` Andy Shevchenko
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox