The "serial/8250: Use fifo in 8250 console driver" commit 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 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.
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;
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
linux-stable-mirror@lists.linaro.org
-
Ilpo Järvinen -
Lukasz Majczak