1. Patchew
  2. linux
  3. View series

[PATCH] printk/nbcon: Restore IRQ in atomic flush after each emitted record

Petr Mladek posted 1 patch 2 months, 1 week ago
There is a newer version of this series
kernel/printk/nbcon.c | 29 ++++++++++++++++-------------
1 file changed, 16 insertions(+), 13 deletions(-)
[PATCH] printk/nbcon: Restore IRQ in atomic flush after each emitted record
Posted by Petr Mladek 2 months, 1 week ago 
The commit d5d399efff6577 ("printk/nbcon: Release nbcon consoles ownership
in atomic flush after each emitted record") prevented stall of a CPU
which lost nbcon console ownership because another CPU entered
an emergency flush.

But there is still the problem that the CPU doing the emergency flush
might cause a stall on its own.

Let's go even further and restore IRQ in the atomic flush after
each emitted record.

It is not a complete solution. The interrupts and/or scheduling might
still be blocked when the emergency atomic flush was called with
IRQs and/or scheduling disabled. But it should remove the following
lockup:

  mlx5_core 0000:03:00.0: Shutdown was called
  kvm: exiting hardware virtualization
  arm-smmu-v3 arm-smmu-v3.10.auto: CMD_SYNC timeout at 0x00000103 [hwprod 0x00000104, hwcons 0x00000102]
  smp: csd: Detected non-responsive CSD lock (#1) on CPU#4, waiting 5000000032 ns for CPU#00 do_nothing (kernel/smp.c:1057)
  smp:     csd: CSD lock (#1) unresponsive.
  [...]
  Call trace:
  pl011_console_write_atomic (./arch/arm64/include/asm/vdso/processor.h:12 drivers/tty/serial/amba-pl011.c:2540) (P)
  nbcon_emit_next_record (kernel/printk/nbcon.c:1049)
  __nbcon_atomic_flush_pending_con (kernel/printk/nbcon.c:1517)
  __nbcon_atomic_flush_pending.llvm.15488114865160659019 (./arch/arm64/include/asm/alternative-macros.h:254 ./arch/arm64/include/asm/cpufeature.h:808 ./arch/arm64/include/asm/irqflags.h:192 kernel/printk/nbcon.c:1562 kernel/printk/nbcon.c:1612)
  nbcon_atomic_flush_pending (kernel/printk/nbcon.c:1629)
  printk_kthreads_shutdown (kernel/printk/printk.c:?)
  syscore_shutdown (drivers/base/syscore.c:120)
  kernel_kexec (kernel/kexec_core.c:1045)
  __arm64_sys_reboot (kernel/reboot.c:794 kernel/reboot.c:722 kernel/reboot.c:722)
  invoke_syscall (arch/arm64/kernel/syscall.c:50)
  el0_svc_common.llvm.14158405452757855239 (arch/arm64/kernel/syscall.c:?)
  do_el0_svc (arch/arm64/kernel/syscall.c:152)
  el0_svc (./arch/arm64/include/asm/alternative-macros.h:254 ./arch/arm64/include/asm/cpufeature.h:808 ./arch/arm64/include/asm/irqflags.h:73 arch/arm64/kernel/entry-common.c:169 arch/arm64/kernel/entry-common.c:182 arch/arm64/kernel/entry-common.c:749)
  el0t_64_sync_handler (arch/arm64/kernel/entry-common.c:820)
  el0t_64_sync (arch/arm64/kernel/entry.S:600)

In this case, nbcon_atomic_flush_pending() is called from
printk_kthreads_shutdown() with IRQs and scheduling enabled.

Note that __nbcon_atomic_flush_pending_con() is directly called also from
nbcon_device_release() where the disabled IRQs might break PREEMPT_RT
guarantees. But the atomic flush is called only in emergency or panic
situations where the latencies are irrelevant anyway.

An ultimate solution would be a touching of watchdogs. But it would hide
all problems. Let's do it later when anyone reports a stall which does
not have a better solution.

Closes: https://lore.kernel.org/r/sqwajvt7utnt463tzxgwu2yctyn5m6bjwrslsnupfexeml6hkd@v6sqmpbu3vvu
Tested-by: Breno Leitao <leitao@debian.org>
Signed-off-by: Petr Mladek <pmladek@suse.com>
---
Hi,

I sent this patch for a proper review. It has been sent as RFC
at https://lore.kernel.org/all/aSnI8UQRNICSKxAb@pathway.suse.cz/

Changes against RFC:

  +  Added note about __nbcon_atomic_flush_pending_con() called from
     nbcon_device_release() and PREEMPT_RT into the commit message [John].

  + Added Tested-by [Breno]

kernel/printk/nbcon.c | 29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
index 3fa403f9831f..6b8becb6ecd9 100644
--- a/kernel/printk/nbcon.c
+++ b/kernel/printk/nbcon.c
@@ -1549,6 +1549,7 @@ static int __nbcon_atomic_flush_pending_con(struct console *con, u64 stop_seq)
 {
 	struct nbcon_write_context wctxt = { };
 	struct nbcon_context *ctxt = &ACCESS_PRIVATE(&wctxt, ctxt);
+	unsigned long flags;
 	int err = 0;
 
 	ctxt->console			= con;
@@ -1557,18 +1558,31 @@ static int __nbcon_atomic_flush_pending_con(struct console *con, u64 stop_seq)
 	ctxt->allow_unsafe_takeover	= nbcon_allow_unsafe_takeover();
 
 	while (nbcon_seq_read(con) < stop_seq) {
-		if (!nbcon_context_try_acquire(ctxt, false))
+		/*
+		 * Atomic flushing does not use console driver synchronization
+		 * (i.e. it does not hold the port lock for uart consoles).
+		 * Therefore IRQs must be disabled to avoid being interrupted
+		 * and then calling into a driver that will deadlock trying
+		 * to acquire console ownership.
+		 */
+		local_irq_save(flags);
+		if (!nbcon_context_try_acquire(ctxt, false)) {
+			local_irq_restore(flags);
 			return -EPERM;
+		}
 
 		/*
 		 * nbcon_emit_next_record() returns false when the console was
 		 * handed over or taken over. In both cases the context is no
 		 * longer valid.
 		 */
-		if (!nbcon_emit_next_record(&wctxt, true))
+		if (!nbcon_emit_next_record(&wctxt, true)) {
+			local_irq_restore(flags);
 			return -EAGAIN;
+		}
 
 		nbcon_context_release(ctxt);
+		local_irq_restore(flags);
 
 		if (!ctxt->backlog) {
 			/* Are there reserved but not yet finalized records? */
@@ -1595,22 +1609,11 @@ static int __nbcon_atomic_flush_pending_con(struct console *con, u64 stop_seq)
 static void nbcon_atomic_flush_pending_con(struct console *con, u64 stop_seq)
 {
 	struct console_flush_type ft;
-	unsigned long flags;
 	int err;
 
 again:
-	/*
-	 * Atomic flushing does not use console driver synchronization (i.e.
-	 * it does not hold the port lock for uart consoles). Therefore IRQs
-	 * must be disabled to avoid being interrupted and then calling into
-	 * a driver that will deadlock trying to acquire console ownership.
-	 */
-	local_irq_save(flags);
-
 	err = __nbcon_atomic_flush_pending_con(con, stop_seq);
 
-	local_irq_restore(flags);
-
 	/*
 	 * If there was a new owner (-EPERM, -EAGAIN), that context is
 	 * responsible for completing.
-- 
2.52.0
Re: [PATCH] printk/nbcon: Restore IRQ in atomic flush after each emitted record
Posted by John Ogness 2 months, 1 week ago 
On 2025-12-02, Petr Mladek <pmladek@suse.com> wrote:
> diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
> index 3fa403f9831f..6b8becb6ecd9 100644
> --- a/kernel/printk/nbcon.c
> +++ b/kernel/printk/nbcon.c
> @@ -1549,6 +1549,7 @@ static int __nbcon_atomic_flush_pending_con(struct console *con, u64 stop_seq)
>  {
>  	struct nbcon_write_context wctxt = { };
>  	struct nbcon_context *ctxt = &ACCESS_PRIVATE(&wctxt, ctxt);
> +	unsigned long flags;
>  	int err = 0;
>  
>  	ctxt->console			= con;
> @@ -1557,18 +1558,31 @@ static int __nbcon_atomic_flush_pending_con(struct console *con, u64 stop_seq)
>  	ctxt->allow_unsafe_takeover	= nbcon_allow_unsafe_takeover();
>  
>  	while (nbcon_seq_read(con) < stop_seq) {
> -		if (!nbcon_context_try_acquire(ctxt, false))
> +		/*
> +		 * Atomic flushing does not use console driver synchronization
> +		 * (i.e. it does not hold the port lock for uart consoles).
> +		 * Therefore IRQs must be disabled to avoid being interrupted
> +		 * and then calling into a driver that will deadlock trying
> +		 * to acquire console ownership.
> +		 */
> +		local_irq_save(flags);
> +		if (!nbcon_context_try_acquire(ctxt, false)) {
> +			local_irq_restore(flags);
>  			return -EPERM;
> +		}
>  
>  		/*
>  		 * nbcon_emit_next_record() returns false when the console was
>  		 * handed over or taken over. In both cases the context is no
>  		 * longer valid.
>  		 */
> -		if (!nbcon_emit_next_record(&wctxt, true))
> +		if (!nbcon_emit_next_record(&wctxt, true)) {
> +			local_irq_restore(flags);
>  			return -EAGAIN;
> +		}
>  
>  		nbcon_context_release(ctxt);
> +		local_irq_restore(flags);


I am not really happy about all the local_irq_restore() usage. Using
guard syntax would be nice here, but AFAICT there is no guard for
local_irq_save()/_restore(). :-/ And I could not come up with any other
alternative that looked more elegant. So let's just keep it this way.

Reviewed-by: John Ogness <john.ogness@linutronix.de>
Re: [PATCH] printk/nbcon: Restore IRQ in atomic flush after each emitted record
Posted by Leo Yan 2 months, 1 week ago 
On Tue, Dec 02, 2025 at 03:39:54PM +0106, John Ogness wrote:

[...]

> I am not really happy about all the local_irq_restore() usage. Using
> guard syntax would be nice here, but AFAICT there is no guard for
> local_irq_save()/_restore().

Sorry for suddenly jumping in.  Wouldn't guard(irqsave)() be helpful
here?
Re: [PATCH] printk/nbcon: Restore IRQ in atomic flush after each emitted record
Posted by John Ogness 2 months ago 
On 2025-12-02, Leo Yan <leo.yan@arm.com> wrote:
>> I am not really happy about all the local_irq_restore() usage. Using
>> guard syntax would be nice here, but AFAICT there is no guard for
>> local_irq_save()/_restore().
>
> Sorry for suddenly jumping in.  Wouldn't guard(irqsave)() be helpful
> here?

Thanks, I was not aware of the irqsave variant. We would want the scoped
version. So something like this?

$ git diff -w
diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
index 3fa403f9831f..55beb64c4f4a 100644
--- a/kernel/printk/nbcon.c
+++ b/kernel/printk/nbcon.c
@@ -1557,6 +1557,14 @@ static int __nbcon_atomic_flush_pending_con(struct console *con, u64 stop_seq)
 	ctxt->allow_unsafe_takeover	= nbcon_allow_unsafe_takeover();
 
 	while (nbcon_seq_read(con) < stop_seq) {
+		/*
+		 * Atomic flushing does not use console driver synchronization
+		 * (i.e. it does not hold the port lock for uart consoles).
+		 * Therefore IRQs must be disabled to avoid being interrupted
+		 * and then calling into a driver that will deadlock trying
+		 * to acquire console ownership.
+		 */
+		scoped_guard(irqsave) {
 			if (!nbcon_context_try_acquire(ctxt, false))
 				return -EPERM;
 
@@ -1569,6 +1577,7 @@ static int __nbcon_atomic_flush_pending_con(struct console *con, u64 stop_seq)
 				return -EAGAIN;
 
 			nbcon_context_release(ctxt);
+		}
 
 		if (!ctxt->backlog) {
 			/* Are there reserved but not yet finalized records? */

John
Re: [PATCH] printk/nbcon: Restore IRQ in atomic flush after each emitted record
Posted by Petr Mladek 2 months ago 
On Wed 2025-12-03 13:33:38, John Ogness wrote:
> On 2025-12-02, Leo Yan <leo.yan@arm.com> wrote:
> >> I am not really happy about all the local_irq_restore() usage. Using
> >> guard syntax would be nice here, but AFAICT there is no guard for
> >> local_irq_save()/_restore().
> >
> > Sorry for suddenly jumping in.  Wouldn't guard(irqsave)() be helpful
> > here?
> 
> Thanks, I was not aware of the irqsave variant. We would want the scoped
> version. So something like this?
> 
> $ git diff -w
> diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
> index 3fa403f9831f..55beb64c4f4a 100644
> --- a/kernel/printk/nbcon.c
> +++ b/kernel/printk/nbcon.c
> @@ -1557,6 +1557,14 @@ static int __nbcon_atomic_flush_pending_con(struct console *con, u64 stop_seq)
>  	ctxt->allow_unsafe_takeover	= nbcon_allow_unsafe_takeover();
>  
>  	while (nbcon_seq_read(con) < stop_seq) {
> +		/*
> +		 * Atomic flushing does not use console driver synchronization
> +		 * (i.e. it does not hold the port lock for uart consoles).
> +		 * Therefore IRQs must be disabled to avoid being interrupted
> +		 * and then calling into a driver that will deadlock trying
> +		 * to acquire console ownership.
> +		 */
> +		scoped_guard(irqsave) {
>  			if (!nbcon_context_try_acquire(ctxt, false))
>  				return -EPERM;
>  
> @@ -1569,6 +1577,7 @@ static int __nbcon_atomic_flush_pending_con(struct console *con, u64 stop_seq)
>  				return -EAGAIN;
>  
>  			nbcon_context_release(ctxt);
> +		}
>  
>  		if (!ctxt->backlog) {
>  			/* Are there reserved but not yet finalized records? */
> 

Great. Thanks for hint. I am going to send v2 with this change.

Best Regards,
Petr

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.