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@xxxxxxxxxxxx>
> Cc: <stable@xxxxxxxxxxxxxxx> # 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
--
i.
