| Message ID | d65ca57567e245aa99a944cefc195f2bc6974384.1712051763.git.esben@geanix.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] printk: export pr_flush() | expand |
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
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
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
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); }