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/[email protected]/
[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-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 <[email protected]>
Cc: <[email protected]> # 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
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 <[email protected]>
Cc: <[email protected]> # 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
Hi,
Is there anything more I should do regarding these changes ?
Best regards,
Lukasz
śr., 5 kwi 2023 o 13:19 Lukasz Majczak <[email protected]> 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/[email protected]/
> [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
>
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
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 <[email protected]>
> Cc: <[email protected]> # 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.