[PATCH 1/2] printk: export pr_flush()

[PATCH 1/2] printk: export pr_flush()

[Esben Haabendal]

From: Martin Hundebøll <martin@geanix.com>

Outside prinkt users might wan't assure whatever printed has reached
its destination before continuing. E.g. during the shutdown-procedure,
where printk-buffers aren't emptied before the system goes down.

Signed-off-by: Martin Hundebøll <martin@geanix.com>
---
 include/linux/printk.h | 2 ++
 kernel/printk/printk.c | 5 ++---
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/include/linux/printk.h b/include/linux/printk.h
index a2d40a637226..c8ac0f17d828 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -142,6 +142,8 @@ void early_printk(const char *s, ...) { }
 
 struct dev_printk_info;
 
+bool pr_flush(int timeout_ms, bool reset_on_progress);
+
 #ifdef CONFIG_PRINTK
 asmlinkage __printf(4, 0)
 int vprintk_emit(int facility, int level,
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index e29f77f4f8b4..26a63457ba6c 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2484,7 +2484,6 @@ asmlinkage __visible int _printk(const char *fmt, ...)
 }
 EXPORT_SYMBOL(_printk);
 
-static bool pr_flush(int timeout_ms, bool reset_on_progress);
 static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progress);
 
 static struct task_struct *nbcon_legacy_kthread;
@@ -2505,7 +2504,7 @@ static inline void wake_up_legacy_kthread(void)
 
 static u64 syslog_seq;
 
-static bool pr_flush(int timeout_ms, bool reset_on_progress) { return true; }
+bool pr_flush(int timeout_ms, bool reset_on_progress) { return true; }
 static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progress) { return true; }
 
 static inline void nbcon_legacy_kthread_create(void) { }
@@ -4217,7 +4216,7 @@ static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progre
  * Context: Process context. May sleep while acquiring console lock.
  * Return: true if all usable printers are caught up.
  */
-static bool pr_flush(int timeout_ms, bool reset_on_progress)
+bool pr_flush(int timeout_ms, bool reset_on_progress)
 {
 	return __pr_flush(NULL, timeout_ms, reset_on_progress);
 }
-- 
2.44.0

[PATCH 2/2] reboot: flush printk buffers before final shutdown

[Esben Haabendal]

From: Martin Hundebøll <martin@geanix.com>

Make sure printed messages are in fact printed before putting the system
down.

Signed-off-by: Martin Hundebøll <martin@geanix.com>
---
 kernel/reboot.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/reboot.c b/kernel/reboot.c
index 22c16e2564cc..e3fa5613257f 100644
--- a/kernel/reboot.c
+++ b/kernel/reboot.c
@@ -283,6 +283,7 @@ void kernel_restart(char *cmd)
 	else
 		pr_emerg("Restarting system with command '%s'\n", cmd);
 	kmsg_dump(KMSG_DUMP_SHUTDOWN);
+	pr_flush(1000, false);
 	machine_restart(cmd);
 }
 EXPORT_SYMBOL_GPL(kernel_restart);
-- 
2.44.0

[PATCH 1/2] printk: export pr_flush()

[Esben Haabendal]

From: Martin Hundebøll <martin@geanix.com>

Outside prinkt users might wan't assure whatever printed has reached
its destination before continuing. E.g. during the shutdown-procedure,
where printk-buffers aren't emptied before the system goes down.

Signed-off-by: Martin Hundebøll <martin@geanix.com>
---
 include/linux/printk.h | 2 ++
 kernel/printk/printk.c | 5 ++---
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/include/linux/printk.h b/include/linux/printk.h
index a2d40a637226..c8ac0f17d828 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -142,6 +142,8 @@ void early_printk(const char *s, ...) { }
 
 struct dev_printk_info;
 
+bool pr_flush(int timeout_ms, bool reset_on_progress);
+
 #ifdef CONFIG_PRINTK
 asmlinkage __printf(4, 0)
 int vprintk_emit(int facility, int level,
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index e29f77f4f8b4..26a63457ba6c 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2484,7 +2484,6 @@ asmlinkage __visible int _printk(const char *fmt, ...)
 }
 EXPORT_SYMBOL(_printk);
 
-static bool pr_flush(int timeout_ms, bool reset_on_progress);
 static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progress);
 
 static struct task_struct *nbcon_legacy_kthread;
@@ -2505,7 +2504,7 @@ static inline void wake_up_legacy_kthread(void)
 
 static u64 syslog_seq;
 
-static bool pr_flush(int timeout_ms, bool reset_on_progress) { return true; }
+bool pr_flush(int timeout_ms, bool reset_on_progress) { return true; }
 static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progress) { return true; }
 
 static inline void nbcon_legacy_kthread_create(void) { }
@@ -4217,7 +4216,7 @@ static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progre
  * Context: Process context. May sleep while acquiring console lock.
  * Return: true if all usable printers are caught up.
  */
-static bool pr_flush(int timeout_ms, bool reset_on_progress)
+bool pr_flush(int timeout_ms, bool reset_on_progress)
 {
 	return __pr_flush(NULL, timeout_ms, reset_on_progress);
 }
-- 
2.44.0

[PATCH 2/2] reboot: flush printk buffers before final shutdown

[Esben Haabendal]

From: Martin Hundebøll <martin@geanix.com>

Make sure printed messages are in fact printed before putting the system
down.

Signed-off-by: Martin Hundebøll <martin@geanix.com>
---
 kernel/reboot.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/reboot.c b/kernel/reboot.c
index 22c16e2564cc..e3fa5613257f 100644
--- a/kernel/reboot.c
+++ b/kernel/reboot.c
@@ -283,6 +283,7 @@ void kernel_restart(char *cmd)
 	else
 		pr_emerg("Restarting system with command '%s'\n", cmd);
 	kmsg_dump(KMSG_DUMP_SHUTDOWN);
+	pr_flush(1000, false);
 	machine_restart(cmd);
 }
 EXPORT_SYMBOL_GPL(kernel_restart);
-- 
2.44.0

[PATCH 1/2] printk: export pr_flush()

[Esben Haabendal]

From: Martin Hundebøll <martin@geanix.com>

Outside prinkt users might wan't assure whatever printed has reached
its destination before continuing. E.g. during the shutdown-procedure,
where printk-buffers aren't emptied before the system goes down.

Signed-off-by: Martin Hundebøll <martin@geanix.com>
---
 include/linux/printk.h | 2 ++
 kernel/printk/printk.c | 5 ++---
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/include/linux/printk.h b/include/linux/printk.h
index a2d40a637226..c8ac0f17d828 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -142,6 +142,8 @@ void early_printk(const char *s, ...) { }
 
 struct dev_printk_info;
 
+bool pr_flush(int timeout_ms, bool reset_on_progress);
+
 #ifdef CONFIG_PRINTK
 asmlinkage __printf(4, 0)
 int vprintk_emit(int facility, int level,
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index e29f77f4f8b4..26a63457ba6c 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2484,7 +2484,6 @@ asmlinkage __visible int _printk(const char *fmt, ...)
 }
 EXPORT_SYMBOL(_printk);
 
-static bool pr_flush(int timeout_ms, bool reset_on_progress);
 static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progress);
 
 static struct task_struct *nbcon_legacy_kthread;
@@ -2505,7 +2504,7 @@ static inline void wake_up_legacy_kthread(void)
 
 static u64 syslog_seq;
 
-static bool pr_flush(int timeout_ms, bool reset_on_progress) { return true; }
+bool pr_flush(int timeout_ms, bool reset_on_progress) { return true; }
 static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progress) { return true; }
 
 static inline void nbcon_legacy_kthread_create(void) { }
@@ -4217,7 +4216,7 @@ static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progre
  * Context: Process context. May sleep while acquiring console lock.
  * Return: true if all usable printers are caught up.
  */
-static bool pr_flush(int timeout_ms, bool reset_on_progress)
+bool pr_flush(int timeout_ms, bool reset_on_progress)
 {
 	return __pr_flush(NULL, timeout_ms, reset_on_progress);
 }
-- 
2.44.0

[PATCH 2/2] reboot: flush printk buffers before final shutdown

[Esben Haabendal]

From: Martin Hundebøll <martin@geanix.com>

Make sure printed messages are in fact printed before putting the system
down.

Signed-off-by: Martin Hundebøll <martin@geanix.com>
---
 kernel/reboot.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/reboot.c b/kernel/reboot.c
index 22c16e2564cc..e3fa5613257f 100644
--- a/kernel/reboot.c
+++ b/kernel/reboot.c
@@ -283,6 +283,7 @@ void kernel_restart(char *cmd)
 	else
 		pr_emerg("Restarting system with command '%s'\n", cmd);
 	kmsg_dump(KMSG_DUMP_SHUTDOWN);
+	pr_flush(1000, false);
 	machine_restart(cmd);
 }
 EXPORT_SYMBOL_GPL(kernel_restart);
-- 
2.44.0

Re: [PATCH 1/2] printk: export pr_flush()

[John Ogness]

On 2024-04-02, Esben Haabendal <esben@geanix.com> wrote:
> Outside prinkt users might wan't assure whatever printed has reached
> its destination before continuing. E.g. during the shutdown-procedure,
> where printk-buffers aren't emptied before the system goes down.

This is a reposting of:

https://lore.kernel.org/lkml/20231030092432.3434623-1-martin@geanix.com

As I wrote in that thread:

printk() tries to print directly from the calling context. Are you
experiencing problems where you do not see the restarting message?

(If your answer is "yes", please explain the scenario and provide
information about the console driver you are using.)

I assume you have tried the latest RT development version.

John Ogness

Re: [PATCH 1/2] printk: export pr_flush()

[Esben Haabendal]

John Ogness <john.ogness@linutronix.de> writes:

> On 2024-04-02, Esben Haabendal <esben@geanix.com> wrote:
>> Outside prinkt users might wan't assure whatever printed has reached
>> its destination before continuing. E.g. during the shutdown-procedure,
>> where printk-buffers aren't emptied before the system goes down.
>
> This is a reposting of:
>
> https://lore.kernel.org/lkml/20231030092432.3434623-1-martin@geanix.com
>
> As I wrote in that thread:
>
> printk() tries to print directly from the calling context. Are you
> experiencing problems where you do not see the restarting message?

Yes, that is exactly what we are seing.

> (If your answer is "yes", please explain the scenario and provide
> information about the console driver you are using.)

It is an i.MX8MP system, and console is attached to ttymxc1
(drivers/tty/serial/imx.c).

Booting up, and simply executing "reboot" command. Without these two
patches, the "reboot: Restarting system" message is not written to
ttymxc1 console. With the patches, it is.

> I assume you have tried the latest RT development version.

I have reproduced the problem with v6.6.23-rt28. I haven't upgraded to
6.7 or 6.8 yet. Do you need me to retest with 6.8?

/Esben

Re: [PATCH 1/2] printk: export pr_flush()

[John Ogness]

On 2024-04-02, Esben Haabendal <esben@geanix.com> wrote:
>> printk() tries to print directly from the calling context. Are you
>> experiencing problems where you do not see the restarting message?
>
> Yes, that is exactly what we are seing.
>
> It is an i.MX8MP system, and console is attached to ttymxc1
> (drivers/tty/serial/imx.c).
>
> Booting up, and simply executing "reboot" command. Without these two
> patches, the "reboot: Restarting system" message is not written to
> ttymxc1 console. With the patches, it is.

This console driver is using the legacy console interface. For
PREEMPT_RT, legacy consoles run exclusively as a thread and thus may not
have a chance to flush messages before a shutdown/reboot. The correct
solution is to port the driver to the new nbcon console interface.

John Ogness

Re: [PATCH 1/2] printk: export pr_flush()

[Esben Haabendal]

John Ogness <john.ogness@linutronix.de> writes:

> On 2024-04-02, Esben Haabendal <esben@geanix.com> wrote:
>>> printk() tries to print directly from the calling context. Are you
>>> experiencing problems where you do not see the restarting message?
>>
>> Yes, that is exactly what we are seing.
>>
>> It is an i.MX8MP system, and console is attached to ttymxc1
>> (drivers/tty/serial/imx.c).
>>
>> Booting up, and simply executing "reboot" command. Without these two
>> patches, the "reboot: Restarting system" message is not written to
>> ttymxc1 console. With the patches, it is.
>
> This console driver is using the legacy console interface. For
> PREEMPT_RT, legacy consoles run exclusively as a thread and thus may not
> have a chance to flush messages before a shutdown/reboot. The correct
> solution is to port the driver to the new nbcon console interface.

I see. The `if (IS_ENABLED(CONFIG_PREEMPT_RT))` in console_unlock() is
preventing the console_lock/console_unlock dance to do what I need.

Are there any other examples other than 8250 for how to port a driver to
nbcon?

What is the plans for porting all of this to mainline?  Should all
drivers be ported first, or will a solution to prevent this type of
regression be implemented at some later time?

/Esben

Re: [PATCH 1/2] printk: export pr_flush()

[John Ogness]

On 2024-04-02, Esben Haabendal <esben@geanix.com> wrote:
> Are there any other examples other than 8250 for how to port a driver to
> nbcon?

No. 8250 is the first example. And even this example is changing a bit
with each new RT version as the underlying APIs and semantics have been
changing.

> What is the plans for porting all of this to mainline?  Should all
> drivers be ported first, or will a solution to prevent this type of
> regression be implemented at some later time?

First off, I want to mention that this behavior is only with the
PREEMPT_RT preemption model. And it has been this way for a long
time. The behavior of legacy consoles for other preemption models is the
same as mainline now. So from a mainline perspective, I would not
consider it a regression.

It is hard to say how this will actually play out though. We are
bringing the printk/console rework to mainline piece by piece. Until now
every piece has needed significant modifications in order to be accepted
mainline. (This is the main reason the 8250 driver keeps needing
changes.)

The updated 8250 driver will be the last piece to go mainline. Only then
would all the APIs, semantics, and documentation be official. Hopefully
it is quite similar to what we have in the PREEMPT_RT tree now.

I hope that once the printk/console rework is complete, there will be a
big push to port over the other serial drivers. Most of them are just
copy/paste of each other, which should simplify the porting. And since
the new nbcon consoles provide real advantages, there will also be
incentive to take on the porting effort.

Finally, if someone wanted to try porting another driver for PREEMPT_RT
(for example, the imx serial console), I would certainly be interested
in reviewing and integrating the patches for the PREEMPT_RT tree.

John Ogness

Re: [PATCH 1/2] printk: export pr_flush()

[Esben Haabendal]

John Ogness <john.ogness@linutronix.de> writes:

> On 2024-04-02, Esben Haabendal <esben@geanix.com> wrote:
>> Are there any other examples other than 8250 for how to port a driver to
>> nbcon?
>
> No. 8250 is the first example. And even this example is changing a bit
> with each new RT version as the underlying APIs and semantics have been
> changing.
>
>> What is the plans for porting all of this to mainline?  Should all
>> drivers be ported first, or will a solution to prevent this type of
>> regression be implemented at some later time?
>
> First off, I want to mention that this behavior is only with the
> PREEMPT_RT preemption model. And it has been this way for a long
> time. The behavior of legacy consoles for other preemption models is the
> same as mainline now. So from a mainline perspective, I would not
> consider it a regression.

Got it. I think the last version where I know that this problem was not
seen with PREEMPT_RT was 4.19. And yes, that is a long time ago :)

> It is hard to say how this will actually play out though. We are
> bringing the printk/console rework to mainline piece by piece. Until now
> every piece has needed significant modifications in order to be accepted
> mainline. (This is the main reason the 8250 driver keeps needing
> changes.)
>
> The updated 8250 driver will be the last piece to go mainline. Only then
> would all the APIs, semantics, and documentation be official. Hopefully
> it is quite similar to what we have in the PREEMPT_RT tree now.
>
> I hope that once the printk/console rework is complete, there will be a
> big push to port over the other serial drivers. Most of them are just
> copy/paste of each other, which should simplify the porting. And since
> the new nbcon consoles provide real advantages, there will also be
> incentive to take on the porting effort.
>
> Finally, if someone wanted to try porting another driver for PREEMPT_RT
> (for example, the imx serial console), I would certainly be interested
> in reviewing and integrating the patches for the PREEMPT_RT tree.

Sounds good. I have looked at porting imx uart driver to nbcon, and have
something working now. It does smell quite a lot of copy-paste
behaviour, but I will send it as an RFC for you to see when I have
tested it a bit more.

The write_thread and write_atomic functions share quite a lot of code,
carrying half of the copy-paste smell.
The other half is the inner loop of the write_thread function, which is
almost identical in 8250 and imx drivers.

I guess there is room to refactor this to avoid this amount of
copy-paste before starting mass-porting.

/Esben

Re: [PATCH 1/2] printk: export pr_flush()

[John Ogness]

On 2024-04-03, Esben Haabendal <esben@geanix.com> wrote:
>> Finally, if someone wanted to try porting another driver for PREEMPT_RT
>> (for example, the imx serial console), I would certainly be interested
>> in reviewing and integrating the patches for the PREEMPT_RT tree.
>
> Sounds good. I have looked at porting imx uart driver to nbcon, and have
> something working now. It does smell quite a lot of copy-paste
> behaviour, but I will send it as an RFC for you to see when I have
> tested it a bit more.

Great!

> The write_thread and write_atomic functions share quite a lot of code,
> carrying half of the copy-paste smell.
> The other half is the inner loop of the write_thread function, which is
> almost identical in 8250 and imx drivers.
>
> I guess there is room to refactor this to avoid this amount of
> copy-paste before starting mass-porting.

I certainly hope so. So far my 8250 work has been making sure it is
technically reliable. I expect there can be a lot of improvement to make
it more efficient and generic.

John