On Wed, 5 Apr 2023, Lukasz Majczak wrote:
The "serial/8250: Use fifo in 8250 console driver" commit
Use canonical formatting when referring to commit:
commit 12char_SHA1 ("shortlog")
has revealed an issue of not re-enabling FIFO after resume. The problematic path is inside uart_resume_port() function. First, when the console device is re-enabled, a call to uport->ops->set_termios() internally initializes FIFO (in serial8250_do_set_termios()), although further code
I'd drop "First," and start with "When" and change "although" to "then"
disables it by issuing ops->startup() (pointer to serial8250_do_startup, internally calling serial8250_clear_fifos()). There is even a comment saying "Clear the FIFO buffers and disable them. (they will be reenabled in set_termios())", but in this scenario, set_termios() has been called already and FIFO remains disabled.
Also, you should reflow the text to 72 chars per line.
This patch address the issue by reversing the order - first checks if tty port is suspended and performs actions accordingly (e.g. call to ops->startup()), then tries to re-enable the console device after suspend (and call to uport->ops->set_termios()).
Signed-off-by: Lukasz Majczak lma@semihalf.com Cc: stable@vger.kernel.org # 6.1+
drivers/tty/serial/serial_core.c | 54 ++++++++++++++++---------------- 1 file changed, 27 insertions(+), 27 deletions(-)
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c index 394a05c09d87..57a153adba3a 100644 --- a/drivers/tty/serial/serial_core.c +++ b/drivers/tty/serial/serial_core.c @@ -2406,33 +2406,6 @@ int uart_resume_port(struct uart_driver *drv, struct uart_port *uport) put_device(tty_dev); uport->suspended = 0;
- /*
* Re-enable the console device after suspending.*/- if (uart_console(uport)) {
/** First try to use the console cflag setting.*/memset(&termios, 0, sizeof(struct ktermios));termios.c_cflag = uport->cons->cflag;termios.c_ispeed = uport->cons->ispeed;termios.c_ospeed = uport->cons->ospeed;/** If that's unset, use the tty termios setting.*/if (port->tty && termios.c_cflag == 0)termios = port->tty->termios;if (console_suspend_enabled)uart_change_pm(state, UART_PM_STATE_ON);uport->ops->set_termios(uport, &termios, NULL);if (!console_suspend_enabled && uport->ops->start_rx)uport->ops->start_rx(uport);if (console_suspend_enabled)console_start(uport->cons);- }
- if (tty_port_suspended(port)) { const struct uart_ops *ops = uport->ops; int ret;
@@ -2471,6 +2444,33 @@ int uart_resume_port(struct uart_driver *drv, struct uart_port *uport) tty_port_set_suspended(port, false); }
- /*
* Re-enable the console device after suspending.*/- if (uart_console(uport)) {
/** First try to use the console cflag setting.*/memset(&termios, 0, sizeof(struct ktermios));termios.c_cflag = uport->cons->cflag;termios.c_ispeed = uport->cons->ispeed;termios.c_ospeed = uport->cons->ospeed;/** If that's unset, use the tty termios setting.*/if (port->tty && termios.c_cflag == 0)termios = port->tty->termios;if (console_suspend_enabled)uart_change_pm(state, UART_PM_STATE_ON);uport->ops->set_termios(uport, &termios, NULL);if (!console_suspend_enabled && uport->ops->start_rx)uport->ops->start_rx(uport);if (console_suspend_enabled)console_start(uport->cons);- }
- mutex_unlock(&port->mutex);
return 0;
To me it looks the whole function is too messed up to fix anything this easily. I'd start with splitting the two large ifs block so that the ordering makes sense:
- set_termios / uart_change_line_settings is called only once - rx and tx is started only after set_termios - failure path (the one doing uart_shutdown) logic is reverse + gotoed