[PATCH] tty: serial_core: Fix the incorrect configuration of baud rate and data length at the console serial port resume

[PATCH] tty: serial_core: Fix the incorrect configuration of baud rate and data length at the console serial port resume

[Lanqing Liu]

When userspace opens a serial port for console, uart_port_startup()
is called. This function assigns the uport->cons->cflag value to
TTY->termios.c_cflag, then it is cleared to 0. When the user space
closes this serial port, the TTY structure will be released, and at
this time uport->cons->cflag has also been cleared.

On the Spreadtrum platform, in some special scenarios, like charging mode,
userspace needs to close the console, which means the uport->cons->cflag
has also been cleared. But printing logs is still needed in the kernel. So
when system enters suspend and resume, the console needs to be configure
the baud rate and data length of the serial port according to its own cflag
when resuming the console port. At this time, the cflag is 0, which will
cause serial port to produce configuration errors that do not meet user
expectations.

To fix this, assigning the TTY->termios.c_cflag value to uport->cons->cflag
before the userspace closes this console serial port. It will ensure that
the correct cflag value can be gotten when the console serial port was
resumed.

Signed-off-by: Lanqing Liu <liuhhome@gmail.com>
---
 drivers/tty/serial/serial_core.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 351843f..f233cf8 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -1539,6 +1539,7 @@ static void uart_set_termios(struct tty_struct *tty,
 static void uart_close(struct tty_struct *tty, struct file *filp)
 {
 	struct uart_state *state = tty->driver_data;
+	struct uart_port *uport;
 
 	if (!state) {
 		struct uart_driver *drv = tty->driver->driver_state;
@@ -1553,6 +1554,9 @@ static void uart_close(struct tty_struct *tty, struct file *filp)
 	}
 
 	pr_debug("uart_close(%d) called\n", tty->index);
+	uport = uart_port_check(state);
+	if (uport && uart_console(uport))
+		uport->cons->cflag = tty->termios.c_cflag;
 
 	tty_port_close(tty->port, tty, filp);
 }
-- 
1.9.1

Re: [PATCH] tty: serial_core: Fix the incorrect configuration of baud rate and data length at the console serial port resume

[Johan Hovold]

On Thu, May 09, 2019 at 01:42:39PM +0800, Lanqing Liu wrote:
> When userspace opens a serial port for console, uart_port_startup()
> is called. This function assigns the uport->cons->cflag value to
> TTY->termios.c_cflag, then it is cleared to 0. When the user space
> closes this serial port, the TTY structure will be released, and at
> this time uport->cons->cflag has also been cleared.
> 
> On the Spreadtrum platform, in some special scenarios, like charging mode,
> userspace needs to close the console, which means the uport->cons->cflag
> has also been cleared. But printing logs is still needed in the kernel. So
> when system enters suspend and resume, the console needs to be configure
> the baud rate and data length of the serial port according to its own cflag
> when resuming the console port. At this time, the cflag is 0, which will
> cause serial port to produce configuration errors that do not meet user
> expectations.

This is actually yet another regression due to 761ed4a94582 ("tty:
serial_core: convert uart_close to use tty_port_close") which
incidentally removed the call to uart_shutdown() where the cflag was
being saved precisely to avoid the problem you're describing:

	ae84db9661ca ("serial: core: Preserve termios c_cflag for console resume")

Judging from a quick look it seems the xmit buf, which is released in
that function may now be leaking too.

> To fix this, assigning the TTY->termios.c_cflag value to uport->cons->cflag
> before the userspace closes this console serial port. It will ensure that
> the correct cflag value can be gotten when the console serial port was
> resumed.

Not sure this is the right fix, but I don't have time to look at this
right now.

Johan

Re: [PATCH] tty: serial_core: Fix the incorrect configuration of baud rate and data length at the console serial port resume

[hhome liu]

Johan Hovold <johan@kernel.org> 于2019年5月14日周二 下午3:35写道：

>
> On Thu, May 09, 2019 at 01:42:39PM +0800, Lanqing Liu wrote:
> > When userspace opens a serial port for console, uart_port_startup()
> > is called. This function assigns the uport->cons->cflag value to
> > TTY->termios.c_cflag, then it is cleared to 0. When the user space
> > closes this serial port, the TTY structure will be released, and at
> > this time uport->cons->cflag has also been cleared.
> >
> > On the Spreadtrum platform, in some special scenarios, like charging mode,
> > userspace needs to close the console, which means the uport->cons->cflag
> > has also been cleared. But printing logs is still needed in the kernel. So
> > when system enters suspend and resume, the console needs to be configure
> > the baud rate and data length of the serial port according to its own cflag
> > when resuming the console port. At this time, the cflag is 0, which will
> > cause serial port to produce configuration errors that do not meet user
> > expectations.
>
> This is actually yet another regression due to 761ed4a94582 ("tty:
> serial_core: convert uart_close to use tty_port_close") which
> incidentally removed the call to uart_shutdown() where the cflag was
> being saved precisely to avoid the problem you're describing:
>
>         ae84db9661ca ("serial: core: Preserve termios c_cflag for console resume")

Yes, agree with you.

>
> Judging from a quick look it seems the xmit buf, which is released in
> that function may now be leaking too.

We haven't found this issue before, but we can try to reproduce it on
our platform.

>
> > To fix this, assigning the TTY->termios.c_cflag value to uport->cons->cflag
> > before the userspace closes this console serial port. It will ensure that
> > the correct cflag value can be gotten when the console serial port was
> > resumed.
>
> Not sure this is the right fix, but I don't have time to look at this
> right now.

OK. Thanks for your comments.