[PATCH v2 0/2] serial: core: fix broken console after suspend

[PATCH v2 0/2] serial: core: fix broken console after suspend

[Lukasz Majczak]

This is v2 of a patch[1].
v1->v2:
* Fixed typos in commit message
* Tested[2] patch "serial: core: preserve cflags, ispeed and ospeed" on all
  current LTS kernels (4.14, 4.19, 5.4, 5.10, 5.15, 6.1) and on top of
  6.3-rc5 with positive results - console was working after resume
* During tests another issue was observed  on 6.1+ - FIFO not enabled after
  resume - and an additional change was needed ("serial: core: enable
  FIFO after resume")
* Test procedure:
  1) ensure the console output is ok
  2) suspend device with "echo freeze > /sys/power/state"
     (/sys/module/printk/parameters/console_suspend == N)
  3) resume device and check the console output
  4) suspend device with "echo freeze > /sys/power/state"
     (/sys/module/printk/parameters/console_suspend == Y)
  5) resume device and check the console output

[1] https://lore.kernel.org/lkml/20230301075751.43839-1-lma@semihalf.com/
[2] Test platforms: PC with i5-8400 + GB H370M D3H
		    HP Elite c1030 Chromebook Enterprise i5-10310U

Lukasz Majczak (2):
  serial: core: preserve cflags, ispeed and ospeed
  serial: core: enable FIFO after resume

 drivers/tty/serial/serial_core.c | 57 +++++++++++++++-----------------
 1 file changed, 27 insertions(+), 30 deletions(-)

-- 
2.40.0.577.gac1e443424-goog

[PATCH v2 1/2] serial: core: preserve cflags, ispeed and ospeed

[Lukasz Majczak]

Re-enable the console device after suspending, causes its cflags,
ispeed and ospeed to be set anew, basing on the values stored in
uport->cons. These values are set only once,
when parsing console parameters after boot (see uart_set_options()),
next after configuring a port in uart_port_startup() these parameters
(cflags, ispeed and ospeed) are copied to termios structure and
the original one (stored in uport->cons) are cleared, but there is no place
in code where those fields are checked against 0.
When kernel calls uart_resume_port() and setups console, it copies cflags,
ispeed and ospeed values from uart->cons, but those are already cleared.
The effect is that the console is broken.
This patch address the issue by preserving the cflags, ispeed and
ospeed fields in uart->cons during uart_port_startup().

Signed-off-by: Lukasz Majczak <lma@semihalf.com>
Cc: <stable@vger.kernel.org> # 4.14+
---
 drivers/tty/serial/serial_core.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 2bd32c8ece39..394a05c09d87 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -225,9 +225,6 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
 			tty->termios.c_cflag = uport->cons->cflag;
 			tty->termios.c_ispeed = uport->cons->ispeed;
 			tty->termios.c_ospeed = uport->cons->ospeed;
-			uport->cons->cflag = 0;
-			uport->cons->ispeed = 0;
-			uport->cons->ospeed = 0;
 		}
 		/*
 		 * Initialise the hardware port settings.
-- 
2.40.0.577.gac1e443424-goog

[PATCH v2 2/2] serial: core: enable FIFO after resume

[Lukasz Majczak]

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;
-- 
2.40.0.577.gac1e443424-goog

Re: [PATCH v2 0/2] serial: core: fix broken console after suspend

[Lukasz Majczak]

Hi,

Is there anything more I should do regarding these changes ?

Best regards,
Lukasz
śr., 5 kwi 2023 o 13:19 Lukasz Majczak <lma@semihalf.com> napisał(a):
>
> This is v2 of a patch[1].
> v1->v2:
> * Fixed typos in commit message
> * Tested[2] patch "serial: core: preserve cflags, ispeed and ospeed" on all
>   current LTS kernels (4.14, 4.19, 5.4, 5.10, 5.15, 6.1) and on top of
>   6.3-rc5 with positive results - console was working after resume
> * During tests another issue was observed  on 6.1+ - FIFO not enabled after
>   resume - and an additional change was needed ("serial: core: enable
>   FIFO after resume")
> * Test procedure:
>   1) ensure the console output is ok
>   2) suspend device with "echo freeze > /sys/power/state"
>      (/sys/module/printk/parameters/console_suspend == N)
>   3) resume device and check the console output
>   4) suspend device with "echo freeze > /sys/power/state"
>      (/sys/module/printk/parameters/console_suspend == Y)
>   5) resume device and check the console output
>
> [1] https://lore.kernel.org/lkml/20230301075751.43839-1-lma@semihalf.com/
> [2] Test platforms: PC with i5-8400 + GB H370M D3H
>                     HP Elite c1030 Chromebook Enterprise i5-10310U
>
> Lukasz Majczak (2):
>   serial: core: preserve cflags, ispeed and ospeed
>   serial: core: enable FIFO after resume
>
>  drivers/tty/serial/serial_core.c | 57 +++++++++++++++-----------------
>  1 file changed, 27 insertions(+), 30 deletions(-)
>
> --
> 2.40.0.577.gac1e443424-goog
>

Re: [PATCH v2 0/2] serial: core: fix broken console after suspend

[Greg Kroah-Hartman]

On Wed, May 10, 2023 at 08:10:28AM +0200, Lukasz Majczak wrote:
> Hi,
> 
> Is there anything more I should do regarding these changes ?

What changes?

The merge window just was finished, so I now have a bunch of patches to
review.  Hopefully someone else can help review this one in the
meantime, which would help out a lot in verifying that this change
really is correct or not.

thanks,

greg k-h

Re: [PATCH v2 2/2] serial: core: enable FIFO after resume

[Ilpo Järvinen]

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


-- 
 i.