[PATCH printk v2 0/2] nbcon irq_work fixes

[PATCH printk v2 0/2] nbcon irq_work fixes

[John Ogness]

Hi,

This is v2 of a series to address some irq_work issues with nbcon.
Initially motivated as a workaround for hardware-related regressions
reported [0], it turns out that the workaround is actually a nice
improvement for nbcon deferred printing in general.

v1 is here [1].

John Ogness

The changes since v1:

- Add a preceding patch to flush the irq_work on nbcon_free(). This
  pre-existing issue was reported [2] by Sashiko while it was
  reviewing v1.

[0] https://lore.kernel.org/lkml/f3757a75-0ba1-4558-bf57-f19ab7e59a4c@nvidia.com
[1] https://lore.kernel.org/lkml/20260827184750.203521-1-john.ogness@linutronix.de
[2] https://sashiko.dev/#/patchset/20260827184750.203521-1-john.ogness%40linutronix.de

John Ogness (2):
  printk/nbcon: Flush nbcon_irq_work in nbcon_free()
  printk/nbcon: Change nbcon_irq_work to IRQ_WORK_LAZY

 kernel/printk/nbcon.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)


base-commit: ffe0486b139e45cd9c9ca2584f04a1910fe4f8a6
-- 
2.47.3

[PATCH printk v2 1/2] printk/nbcon: Flush nbcon_irq_work in nbcon_free()

[John Ogness]

Ensure any pending nbcon_irq_work is flushed before allowing the
console to be recycled.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 kernel/printk/nbcon.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
index a5921a84a80ed..218b1922a58de 100644
--- a/kernel/printk/nbcon.c
+++ b/kernel/printk/nbcon.c
@@ -1849,6 +1849,8 @@ void nbcon_free(struct console *con)
 			printk_kthreads_running = false;
 	}
 
+	irq_work_sync(&con->irq_work);
+
 	nbcon_state_set(con, &state);
 
 	/* Boot consoles share global printk buffers. */
-- 
2.47.3

[PATCH printk v2 2/2] printk/nbcon: Change nbcon_irq_work to IRQ_WORK_LAZY

[John Ogness]

For some platforms it is a problem to queue irq_work when entering
cpuidle states. Since nbcon uses irq_work for waking the printing
kthreads, any printk() calls when entering cpuidle states can lead
to the affected hardware hanging. Tegra20 and Tegra30 are examples
of such platforms. Avoiding raising the irq_work IRQ has shown to
circumvent the problem.

Change the nbcon_irq_work to be IRQ_WORK_LAZY, thus not raising
an IRQ upon irq_work queuing. The irq_work is then handled on the
next interrupt (worst case, kernel tick). This additional delay is
acceptable because nbcon_irq_work is only responsible for
non-emergency deferred printing, which is delayed anyway. This also
has the benefit of not needing to raise an IRQ for each printk()
call.

Link: https://lore.kernel.org/lkml/f3757a75-0ba1-4558-bf57-f19ab7e59a4c@nvidia.com
Fixes: 76f258bf3f2a ("printk: nbcon: Introduce printer kthreads")
Signed-off-by: John Ogness <john.ogness@linutronix.de>
Reviewed-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Reviewed-by: Petr Mladek <pmladek@suse.com>
---
 kernel/printk/nbcon.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
index 218b1922a58de..e5aede99304ed 100644
--- a/kernel/printk/nbcon.c
+++ b/kernel/printk/nbcon.c
@@ -1782,7 +1782,7 @@ bool nbcon_alloc(struct console *con)
 	}
 
 	rcuwait_init(&con->rcuwait);
-	init_irq_work(&con->irq_work, nbcon_irq_work);
+	con->irq_work = IRQ_WORK_INIT_LAZY(nbcon_irq_work);
 	atomic_long_set(&ACCESS_PRIVATE(con, nbcon_prev_seq), -1UL);
 	nbcon_state_set(con, &state);
 
-- 
2.47.3

Re: [PATCH printk v2 0/2] nbcon irq_work fixes

[Jon Hunter]

Hi John,

On 28/08/2026 15:02, John Ogness wrote:
> Hi,
> 
> This is v2 of a series to address some irq_work issues with nbcon.
> Initially motivated as a workaround for hardware-related regressions
> reported [0], it turns out that the workaround is actually a nice
> improvement for nbcon deferred printing in general.
> 
> v1 is here [1].
> 
> John Ogness
> 
> The changes since v1:
> 
> - Add a preceding patch to flush the irq_work on nbcon_free(). This
>    pre-existing issue was reported [2] by Sashiko while it was
>    reviewing v1.
> 
> [0] https://lore.kernel.org/lkml/f3757a75-0ba1-4558-bf57-f19ab7e59a4c@nvidia.com
> [1] https://lore.kernel.org/lkml/20260827184750.203521-1-john.ogness@linutronix.de
> [2] https://sashiko.dev/#/patchset/20260827184750.203521-1-john.ogness%40linutronix.de
> 
> John Ogness (2):
>    printk/nbcon: Flush nbcon_irq_work in nbcon_free()
>    printk/nbcon: Change nbcon_irq_work to IRQ_WORK_LAZY
> 
>   kernel/printk/nbcon.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)

I have tested these on Tegra and all is working well, so for
the series ...

Tested-by: Jon Hunter <jonathanh@nvidia.com>

Thanks for fixing this!

Cheers
Jon

-- 
nvpublic

Re: [PATCH printk v2 0/2] nbcon irq_work fixes

[Bradley Morgan]

On 28 August 2026 15:02:07 BST, John Ogness <john.ogness@linutronix.de>
wrote:
>Hi,
>
>This is v2 of a series to address some irq_work issues with nbcon.
>Initially motivated as a workaround for hardware-related regressions
>reported [0], it turns out that the workaround is actually a nice
>improvement for nbcon deferred printing in general.
>
>v1 is here [1].
>
>John Ogness
>
>The changes since v1:
>
>- Add a preceding patch to flush the irq_work on nbcon_free(). This
>  pre-existing issue was reported [2] by Sashiko while it was
>  reviewing v1.
>
>[0]
>https://lore.kernel.org/lkml/f3757a75-0ba1-4558-bf57-f19ab7e59a4c@nvidia.com
>[1]
>https://lore.kernel.org/lkml/20260827184750.203521-1-john.ogness@linutronix.de
>[2]
>https://sashiko.dev/#/patchset/20260827184750.203521-1-john.ogness%40linutronix.de
>
>John Ogness (2):
>  printk/nbcon: Flush nbcon_irq_work in nbcon_free()
>  printk/nbcon: Change nbcon_irq_work to IRQ_WORK_LAZY
>
> kernel/printk/nbcon.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
>
>base-commit: ffe0486b139e45cd9c9ca2584f04a1910fe4f8a6
>
Tested-by: Bradley Morgan <brads@mainlining.org> # PKVM, based on Pixel 7 (ARM64)



--- Thanks!
https://lore.kernel.org/all/EE579805-42F2-4C58-B752-F28779EEB717@grrlz.net/

Re: [PATCH printk v2 0/2] nbcon irq_work fixes

[Sebastian Andrzej Siewior]

On 2026-08-29 00:39:15 [+0100], Bradley Morgan wrote:
> Tested-by: Bradley Morgan <brads@mainlining.org> # PKVM, based on Pixel 7 (ARM64)

Does this PKVM even use a nbcon and if so which one?

Sebastian

Re: [PATCH printk v2 0/2] nbcon irq_work fixes

[Bradley Morgan]

On 31 August 2026 07:36:48 BST, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
>On 2026-08-29 00:39:15 [+0100], Bradley Morgan wrote:
>> Tested-by: Bradley Morgan <brads@mainlining.org> # PKVM, based on Pixel
>7 (ARM64)
>
>Does this PKVM even use a nbcon and if so which one?
>

heh, not PKVM itself, I booted a kernel inside PKVM, which is the
hypervisor, the kernel has nbcon inside. My T-B is for the kernel 

>Sebastian

--- Thanks!
https://lore.kernel.org/all/EE579805-42F2-4C58-B752-F28779EEB717@grrlz.net/

Re: [PATCH printk v2 0/2] nbcon irq_work fixes

[Sebastian Andrzej Siewior]

On 2026-08-31 08:31:22 [+0100], Bradley Morgan wrote:
> On 31 August 2026 07:36:48 BST, Sebastian Andrzej Siewior
> <bigeasy@linutronix.de> wrote:
> >On 2026-08-29 00:39:15 [+0100], Bradley Morgan wrote:
> >> Tested-by: Bradley Morgan <brads@mainlining.org> # PKVM, based on Pixel
> >7 (ARM64)
> >
> >Does this PKVM even use a nbcon and if so which one?
> >
> 
> heh, not PKVM itself, I booted a kernel inside PKVM, which is the
> hypervisor, the kernel has nbcon inside. My T-B is for the kernel 

That tested-by is futile if none of the suggested changes were actually
tested.

Sebastian

Re: [PATCH printk v2 0/2] nbcon irq_work fixes

[Bradley Morgan]

On 31 August 2026 09:22:37 BST, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
>On 2026-08-31 08:31:22 [+0100], Bradley Morgan wrote:
>> On 31 August 2026 07:36:48 BST, Sebastian Andrzej Siewior
>> <bigeasy@linutronix.de> wrote:
>> >On 2026-08-29 00:39:15 [+0100], Bradley Morgan wrote:
>> >> Tested-by: Bradley Morgan <brads@mainlining.org> # PKVM, based on
>Pixel
>> >7 (ARM64)
>> >
>> >Does this PKVM even use a nbcon and if so which one?
>> >
>> 
>> heh, not PKVM itself, I booted a kernel inside PKVM, which is the
>> hypervisor, the kernel has nbcon inside. My T-B is for the kernel 
>
>That tested-by is futile if none of the suggested changes were actually
>tested.
>
>Sebastian
urgh im on holiday, I'll elaborate later.
--- Thanks!
https://lore.kernel.org/all/EE579805-42F2-4C58-B752-F28779EEB717@grrlz.net/

Re: [PATCH printk v2 0/2] nbcon irq_work fixes

[Petr Mladek]

On Mon 2026-08-31 10:22:37, Sebastian Andrzej Siewior wrote:
> On 2026-08-31 08:31:22 [+0100], Bradley Morgan wrote:
> > On 31 August 2026 07:36:48 BST, Sebastian Andrzej Siewior
> > <bigeasy@linutronix.de> wrote:
> > >On 2026-08-29 00:39:15 [+0100], Bradley Morgan wrote:
> > >> Tested-by: Bradley Morgan <brads@mainlining.org> # PKVM, based on Pixel
> > >7 (ARM64)
> > >
> > >Does this PKVM even use a nbcon and if so which one?
> > >
> > 
> > heh, not PKVM itself, I booted a kernel inside PKVM, which is the
> > hypervisor, the kernel has nbcon inside. My T-B is for the kernel 
> 
> That tested-by is futile if none of the suggested changes were actually
> tested.

Yeah, I am going to omit this tested-by tag if I remember to remove
it from the patch generated by b4.

Best Regards,
Petr

PS: Bradley, this is not a good way to become part of the kernel
    community. The tested-by flag would be valuable only when
    the modified code was used during the test.

Re: [PATCH printk v2 1/2] printk/nbcon: Flush nbcon_irq_work in nbcon_free()

[Petr Mladek]

On Fri 2026-08-28 16:08:08, John Ogness wrote:
> Ensure any pending nbcon_irq_work is flushed before allowing the
> console to be recycled.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> ---
>  kernel/printk/nbcon.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
> index a5921a84a80ed..218b1922a58de 100644
> --- a/kernel/printk/nbcon.c
> +++ b/kernel/printk/nbcon.c
> @@ -1849,6 +1849,8 @@ void nbcon_free(struct console *con)
>  			printk_kthreads_running = false;
>  	}
>  
> +	irq_work_sync(&con->irq_work);

The irq_work work tries to wake up the printk kthread which
has been destroyed above. 

IMHO, we should move this up above

	if (printk_kthreads_running) {
		nbcon_kthread_stop(con);

Note that nbcon_free() is called when @con is not longer in
the console_list and after synchronize_srcu(). So that nobody
should queue this irq_work anymore.

Best Regards,
Petr

> +
>  	nbcon_state_set(con, &state);
>  
>  	/* Boot consoles share global printk buffers. */
> -- 
> 2.47.3

Re: [PATCH printk v2 1/2] printk/nbcon: Flush nbcon_irq_work in nbcon_free()

[John Ogness]

On 2026-08-31, Petr Mladek <pmladek@suse.com> wrote:
>> diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
>> index a5921a84a80ed..218b1922a58de 100644
>> --- a/kernel/printk/nbcon.c
>> +++ b/kernel/printk/nbcon.c
>> @@ -1849,6 +1849,8 @@ void nbcon_free(struct console *con)
>>  			printk_kthreads_running = false;
>>  	}
>>  
>> +	irq_work_sync(&con->irq_work);
>
> The irq_work work tries to wake up the printk kthread which
> has been destroyed above. 

Note that the irq_work only wakes up con->rcuwait waiters. Since the
kthread has already ended, it certainly is not waiting and there is no
wakeup.

The irq_work_sync() call is literally just to clear out the irq_work
queue, not to actually wake anything.

John

Re: [PATCH printk v2 1/2] printk/nbcon: Flush nbcon_irq_work in nbcon_free()

[Petr Mladek]

On Mon 2026-08-31 17:15:35, John Ogness wrote:
> On 2026-08-31, Petr Mladek <pmladek@suse.com> wrote:
> >> diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
> >> index a5921a84a80ed..218b1922a58de 100644
> >> --- a/kernel/printk/nbcon.c
> >> +++ b/kernel/printk/nbcon.c
> >> @@ -1849,6 +1849,8 @@ void nbcon_free(struct console *con)
> >>  			printk_kthreads_running = false;
> >>  	}
> >>  
> >> +	irq_work_sync(&con->irq_work);
> >
> > The irq_work work tries to wake up the printk kthread which
> > has been destroyed above. 
> 
> Note that the irq_work only wakes up con->rcuwait waiters. Since the
> kthread has already ended, it certainly is not waiting and there is no
> wakeup.

Yes, but this is an implementation detail.

> The irq_work_sync() call is literally just to clear out the irq_work
> queue, not to actually wake anything.

I still believe that this should rather be done before the kthread
is destroyed. IMHO, it would be more logical there. Or is there any
reason why you would prefer to flush it later?

I am sorry for being so stubborn. It is not a big deal.
The current ordering just looks suspicious and weird.
I could live with it. This is my last attempt to convince you ;-)

Best Regards,
Petr