[PATCH v4 0/5] Handle NBCON consoles on KDB

[PATCH v4 0/5] Handle NBCON consoles on KDB

[Marcos Paulo de Souza]

In v4, only the first patch wasn't changed, all other received updates. A
new patch was added to export nbcon_write_context_set_buf helper. All
changes were made by following the suggestions from John Ogness and Petr
Mladek, thanks a lot for your reviews and suggestions!

As usual, how I tested the changes:

Testing
-------

I did the tests using qemu and reapplying commit f79b163c4231
('Revert "serial: 8250: Switch to nbcon console"') created originally by
John, just to exercise the common 8250 serial from qemu. The commit can
be checked on [1]. I had to solve some conflicts since the code has been
reworked after the commit was reverted.

Then I would create three different serial entries on qemu:
-serial mon:stdio -serial pty -serial pty

And for the kernel command line I added:
earlyprintk=serial,ttyS2 console=ttyS2 console=ttyS1 console=ttyS1 kgdboc=ttyS1,115200

Without the last patch on this patchset, when KDB is triggered, the mirroring
only worked on the earlyprintk console, since it's using the legacy console.

With the last patch applied, KDB mirroring works on legacy and nbcon
console. For debugging I added some messages to be printed by KDB, showing
also the console->name and console->index, and I was able to see both
->write and ->write_atomic being called, and it all working together.

[1]: https://github.com/marcosps/linux/commit/618bd49f8533db85d9c322f9ad1cb0da22aca9ee

Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
---
Changes in v4:
- Added ifdefs to only check for KGDB if KGDB was enabled, suggested by John Ogness
- Updated comments about KDB on acquire_direct, suggested by Petr and John
- Added a new patch to export nbcon_write_context_set_buf, suggested by Petr and John
- Link to v3: https://lore.kernel.org/r/20250902-nbcon-kgdboc-v3-0-cd30a8106f1c@suse.com

Changes in v3:
- Only call nbcon_context_release if nbcon_context_exit_unsafe returns true (John Ogness)
- Dropped the prototype of console_is_usable from kernel/printk/internal. (Petr Mladek)
- Add comments to the new functions introduced (Petr Mladek)
- Flush KDB console on nbcon_kdb_release (Petr Mladek)
- Add an exception for KDB on nbcon_context_try_acquire_direct (John Ogness and Petr Mladek)
- Link to v2: https://lore.kernel.org/r/20250811-nbcon-kgdboc-v2-0-c7c72bcdeaf6@suse.com

Changes in v2:
- Set by mistake ..
- Link to v1: https://lore.kernel.org/r/20250713-nbcon-kgdboc-v1-0-51eccd9247a8@suse.com

---
Marcos Paulo de Souza (5):
      printk: nbcon: Export console_is_usable
      printk: nbcon: Introduce KDB helpers
      printk: nbcon: Allow KDB to acquire the NBCON context
      printk: nbcon: Export nbcon_write_context_set_buf
      kdb: Adapt kdb_msg_write to work with NBCON consoles

 include/linux/console.h   | 54 ++++++++++++++++++++++++++++++++++
 include/linux/kdb.h       |  6 ++++
 kernel/debug/kdb/kdb_io.c | 47 +++++++++++++++++++----------
 kernel/printk/internal.h  | 44 ---------------------------
 kernel/printk/nbcon.c     | 75 +++++++++++++++++++++++++++++++++++++++++++++--
 5 files changed, 165 insertions(+), 61 deletions(-)
---
base-commit: caa285912f4901a5acdb38624f1ae157e3cd9c4c
change-id: 20250713-nbcon-kgdboc-efcfc37fde46

Best regards,
-- 
Marcos Paulo de Souza <mpdesouza@suse.com>

[PATCH v4 1/5] printk: nbcon: Export console_is_usable

[Marcos Paulo de Souza]

The helper will be used on KDB code in the next commits.

Reviewed-by: Petr Mladek <pmladek@suse.com>
Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
---
 include/linux/console.h  | 44 ++++++++++++++++++++++++++++++++++++++++++++
 kernel/printk/internal.h | 44 --------------------------------------------
 2 files changed, 44 insertions(+), 44 deletions(-)

diff --git a/include/linux/console.h b/include/linux/console.h
index 8f10d0a85bb4536e4b0dda4e8ccbdf87978bbb4a..67af483574727c00eea1d5a1eacc994755c92607 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -605,6 +605,48 @@ extern bool nbcon_can_proceed(struct nbcon_write_context *wctxt);
 extern bool nbcon_enter_unsafe(struct nbcon_write_context *wctxt);
 extern bool nbcon_exit_unsafe(struct nbcon_write_context *wctxt);
 extern void nbcon_reacquire_nobuf(struct nbcon_write_context *wctxt);
+
+/*
+ * Check if the given console is currently capable and allowed to print
+ * records. Note that this function does not consider the current context,
+ * which can also play a role in deciding if @con can be used to print
+ * records.
+ */
+static inline bool console_is_usable(struct console *con, short flags, bool use_atomic)
+{
+	if (!(flags & CON_ENABLED))
+		return false;
+
+	if ((flags & CON_SUSPENDED))
+		return false;
+
+	if (flags & CON_NBCON) {
+		/* The write_atomic() callback is optional. */
+		if (use_atomic && !con->write_atomic)
+			return false;
+
+		/*
+		 * For the !use_atomic case, @printk_kthreads_running is not
+		 * checked because the write_thread() callback is also used
+		 * via the legacy loop when the printer threads are not
+		 * available.
+		 */
+	} else {
+		if (!con->write)
+			return false;
+	}
+
+	/*
+	 * Console drivers may assume that per-cpu resources have been
+	 * allocated. So unless they're explicitly marked as being able to
+	 * cope (CON_ANYTIME) don't call them until this CPU is officially up.
+	 */
+	if (!cpu_online(raw_smp_processor_id()) && !(flags & CON_ANYTIME))
+		return false;
+
+	return true;
+}
+
 #else
 static inline void nbcon_cpu_emergency_enter(void) { }
 static inline void nbcon_cpu_emergency_exit(void) { }
@@ -612,6 +654,8 @@ static inline bool nbcon_can_proceed(struct nbcon_write_context *wctxt) { return
 static inline bool nbcon_enter_unsafe(struct nbcon_write_context *wctxt) { return false; }
 static inline bool nbcon_exit_unsafe(struct nbcon_write_context *wctxt) { return false; }
 static inline void nbcon_reacquire_nobuf(struct nbcon_write_context *wctxt) { }
+static inline bool console_is_usable(struct console *con, short flags,
+				     bool use_atomic) { return false; }
 #endif
 
 extern int console_set_on_cmdline;
diff --git a/kernel/printk/internal.h b/kernel/printk/internal.h
index ef282001f200fdbbacae3171932bf9f049037a85..bef97f2d11793191280bfb46f7f8b13bf2560351 100644
--- a/kernel/printk/internal.h
+++ b/kernel/printk/internal.h
@@ -112,47 +112,6 @@ bool nbcon_kthread_create(struct console *con);
 void nbcon_kthread_stop(struct console *con);
 void nbcon_kthreads_wake(void);
 
-/*
- * Check if the given console is currently capable and allowed to print
- * records. Note that this function does not consider the current context,
- * which can also play a role in deciding if @con can be used to print
- * records.
- */
-static inline bool console_is_usable(struct console *con, short flags, bool use_atomic)
-{
-	if (!(flags & CON_ENABLED))
-		return false;
-
-	if ((flags & CON_SUSPENDED))
-		return false;
-
-	if (flags & CON_NBCON) {
-		/* The write_atomic() callback is optional. */
-		if (use_atomic && !con->write_atomic)
-			return false;
-
-		/*
-		 * For the !use_atomic case, @printk_kthreads_running is not
-		 * checked because the write_thread() callback is also used
-		 * via the legacy loop when the printer threads are not
-		 * available.
-		 */
-	} else {
-		if (!con->write)
-			return false;
-	}
-
-	/*
-	 * Console drivers may assume that per-cpu resources have been
-	 * allocated. So unless they're explicitly marked as being able to
-	 * cope (CON_ANYTIME) don't call them until this CPU is officially up.
-	 */
-	if (!cpu_online(raw_smp_processor_id()) && !(flags & CON_ANYTIME))
-		return false;
-
-	return true;
-}
-
 /**
  * nbcon_kthread_wake - Wake up a console printing thread
  * @con:	Console to operate on
@@ -204,9 +163,6 @@ static inline bool nbcon_legacy_emit_next_record(struct console *con, bool *hand
 static inline void nbcon_kthread_wake(struct console *con) { }
 static inline void nbcon_kthreads_wake(void) { }
 
-static inline bool console_is_usable(struct console *con, short flags,
-				     bool use_atomic) { return false; }
-
 #endif /* CONFIG_PRINTK */
 
 extern bool have_boot_console;

-- 
2.51.0

Re: [PATCH v4 1/5] printk: nbcon: Export console_is_usable

[Petr Mladek]

On Mon 2025-09-15 08:20:30, Marcos Paulo de Souza wrote:
> The helper will be used on KDB code in the next commits.
> 
> Reviewed-by: Petr Mladek <pmladek@suse.com>
> Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>

Nit :-)

The ordering of the tags is important. It defines the timeline of
the related actions.

The above ordering might be understood as that Petr reviewed
an older version of patch. But it was later modified by Marcos.

The expected ordering is:

Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
Reviewed-by: Petr Mladek <pmladek@suse.com>

It means that Macros developed a patch and Petr reviewed it.
The patch was not longer modified.

Note the Reviewed-by tag might be preserved even when Marcos
later did some cosmetic changes, e.g. fixed a typo, formatting,
or rebased.

Best Regards,
Petr

PS: There is no need to send v5 just because of this. I could fix
    it when committing the patch...

Re: [PATCH v4 1/5] printk: nbcon: Export console_is_usable

[Marcos Paulo de Souza]

On Wed, 2025-09-17 at 10:57 +0200, Petr Mladek wrote:
> On Mon 2025-09-15 08:20:30, Marcos Paulo de Souza wrote:
> > The helper will be used on KDB code in the next commits.
> > 
> > Reviewed-by: Petr Mladek <pmladek@suse.com>
> > Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
> 
> Nit :-)
> 
> The ordering of the tags is important. It defines the timeline of
> the related actions.
> 
> The above ordering might be understood as that Petr reviewed
> an older version of patch. But it was later modified by Marcos.
> 
> The expected ordering is:
> 
> Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
> Reviewed-by: Petr Mladek <pmladek@suse.com>
> 
> It means that Macros developed a patch and Petr reviewed it.
> The patch was not longer modified.
> 
> Note the Reviewed-by tag might be preserved even when Marcos
> later did some cosmetic changes, e.g. fixed a typo, formatting,
> or rebased.

Got it. I used b4 prep --update-trailers, and it added the Reviewed-by
before my Sign-off. But still, this patch didn't change since the last
submission, so I thought that it would be ok.

But I'll double check next time.

> 
> Best Regards,
> Petr
> 
> PS: There is no need to send v5 just because of this. I could fix
>     it when committing the patch...

Re: [PATCH v4 1/5] printk: nbcon: Export console_is_usable

[Konstantin Ryabitsev]

On Wed, Sep 17, 2025 at 09:21:48AM -0300, Marcos Paulo de Souza wrote:
> > Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
> > Reviewed-by: Petr Mladek <pmladek@suse.com>
> > 
> > It means that Macros developed a patch and Petr reviewed it.
> > The patch was not longer modified.

That's not entirely correct. The signed-off trailer is used as the boundary
indicating who was the person responsible for collecting the trailers. When
the trailers are collected by the original author as part of their iteration
cycles, their signed-off-by trailer goes below any trailers they have
received.

When the trailers are collected by the maintainer, any trailers they retrieve
will go below the original author's S-o-b, but above their own.

> > 
> > Note the Reviewed-by tag might be preserved even when Marcos
> > later did some cosmetic changes, e.g. fixed a typo, formatting,
> > or rebased.
> 
> Got it. I used b4 prep --update-trailers, and it added the Reviewed-by
> before my Sign-off. But still, this patch didn't change since the last
> submission, so I thought that it would be ok.

That is the correct behaviour.

-K

Re: [PATCH v4 1/5] printk: nbcon: Export console_is_usable

[Petr Mladek]

On Wed 2025-09-17 10:07:47, Konstantin Ryabitsev wrote:
> On Wed, Sep 17, 2025 at 09:21:48AM -0300, Marcos Paulo de Souza wrote:
> > > Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
> > > Reviewed-by: Petr Mladek <pmladek@suse.com>
> > > 
> > > It means that Macros developed a patch and Petr reviewed it.
> > > The patch was not longer modified.
> 
> That's not entirely correct. The signed-off trailer is used as the boundary
> indicating who was the person responsible for collecting the trailers. When
> the trailers are collected by the original author as part of their iteration
> cycles, their signed-off-by trailer goes below any trailers they have
> received.

This is an interesting interpretation. I am just curious. Has this
already been discussed anywhere, please?

It seems that the ordering of the trailers is not much described
in the process documentation, see
https://docs.kernel.org/process/submitting-patches.html

Best Regards,
Petr

[PATCH v4 2/5] printk: nbcon: Introduce KDB helpers

[Marcos Paulo de Souza]

These helpers will be used when calling console->write_atomic on
KDB code in the next patch. It's basically the same implementation
as nbcon_device_try_acquire, but using NBCON_PRIO_EMERGENCY when
acquiring the context.

If the acquire succeeds, the message and message length are assigned to
nbcon_write_context so ->write_atomic can print the message.

After release try to flush the console since there may be a backlog of
messages in the ringbuffer. The kthread console printers do not get a
chance to run while kdb is active.

Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
---
 include/linux/console.h |  6 +++++
 kernel/printk/nbcon.c   | 66 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 72 insertions(+)

diff --git a/include/linux/console.h b/include/linux/console.h
index 67af483574727c00eea1d5a1eacc994755c92607..b34c5a0b86303e2fb4583fa467d8be43761cf756 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -605,6 +605,9 @@ extern bool nbcon_can_proceed(struct nbcon_write_context *wctxt);
 extern bool nbcon_enter_unsafe(struct nbcon_write_context *wctxt);
 extern bool nbcon_exit_unsafe(struct nbcon_write_context *wctxt);
 extern void nbcon_reacquire_nobuf(struct nbcon_write_context *wctxt);
+extern bool nbcon_kdb_try_acquire(struct console *con,
+				  struct nbcon_write_context *wctxt);
+extern void nbcon_kdb_release(struct nbcon_write_context *wctxt);
 
 /*
  * Check if the given console is currently capable and allowed to print
@@ -654,6 +657,9 @@ static inline bool nbcon_can_proceed(struct nbcon_write_context *wctxt) { return
 static inline bool nbcon_enter_unsafe(struct nbcon_write_context *wctxt) { return false; }
 static inline bool nbcon_exit_unsafe(struct nbcon_write_context *wctxt) { return false; }
 static inline void nbcon_reacquire_nobuf(struct nbcon_write_context *wctxt) { }
+static inline bool nbcon_kdb_try_acquire(struct console *con,
+					 struct nbcon_write_context *wctxt) { return false; }
+static inline void nbcon_kdb_release(struct console *con) { }
 static inline bool console_is_usable(struct console *con, short flags,
 				     bool use_atomic) { return false; }
 #endif
diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
index 646801813415f0abe40cabf2f28ca9e30664f028..ff218e95a505fd10521c2c4dfb00ad5ec5773953 100644
--- a/kernel/printk/nbcon.c
+++ b/kernel/printk/nbcon.c
@@ -1855,3 +1855,69 @@ void nbcon_device_release(struct console *con)
 	console_srcu_read_unlock(cookie);
 }
 EXPORT_SYMBOL_GPL(nbcon_device_release);
+
+/**
+ * nbcon_kdb_try_acquire - Try to acquire nbcon console, enter unsafe
+ *				section, and initialized nbcon write context
+ * @con:	The nbcon console to acquire
+ * @wctxt:	The nbcon write context to be used on success
+ *
+ * Context:	Under console_srcu_read_lock() for emiting a single kdb message
+ *		using the given con->write_atomic() callback. Can be called
+ *		only when the console is usable at the moment.
+ *
+ * Return:	True if the console was acquired. False otherwise.
+ *
+ * kdb emits messages on consoles registered for printk() without
+ * storing them into the ring buffer. It has to acquire the console
+ * ownerhip so that it could call con->write_atomic() callback a safe way.
+ *
+ * This function acquires the nbcon console using priority NBCON_PRIO_EMERGENCY
+ * and marks it unsafe for handover/takeover.
+ */
+bool nbcon_kdb_try_acquire(struct console *con,
+			   struct nbcon_write_context *wctxt)
+{
+	struct nbcon_context *ctxt = &ACCESS_PRIVATE(wctxt, ctxt);
+
+	memset(ctxt, 0, sizeof(*ctxt));
+	ctxt->console = con;
+	ctxt->prio    = NBCON_PRIO_EMERGENCY;
+
+	if (!nbcon_context_try_acquire(ctxt, false))
+		return false;
+
+	if (!nbcon_context_enter_unsafe(ctxt))
+		return false;
+
+	return true;
+}
+
+/**
+ * nbcon_kdb_release - Exit unsafe section and release the nbcon console
+ *
+ * @wctxt:	The nbcon write context initialized by a successful
+ *		nbcon_kdb_try_acquire()
+ *
+ * Context:	Under console_srcu_read_lock() for emiting a single kdb message
+ *		using the given con->write_atomic() callback. Can be called
+ *		only when the console is usable at the moment.
+ */
+void nbcon_kdb_release(struct nbcon_write_context *wctxt)
+{
+	struct nbcon_context *ctxt = &ACCESS_PRIVATE(wctxt, ctxt);
+
+	if (!nbcon_context_exit_unsafe(ctxt))
+		return;
+
+	nbcon_context_release(ctxt);
+
+	/*
+	 * Flush any new printk() messages added when the console was blocked.
+	 * Only the console used by the given write context was	blocked.
+	 * The console was locked only when the write_atomic() callback
+	 * was usable.
+	 */
+	__nbcon_atomic_flush_pending_con(ctxt->console,
+					 prb_next_reserve_seq(prb), false);
+}

-- 
2.51.0

Re: [PATCH v4 2/5] printk: nbcon: Introduce KDB helpers

[Petr Mladek]

On Mon 2025-09-15 08:20:31, Marcos Paulo de Souza wrote:
> These helpers will be used when calling console->write_atomic on
> KDB code in the next patch. It's basically the same implementation
> as nbcon_device_try_acquire, but using NBCON_PRIO_EMERGENCY when
> acquiring the context.
> 
> If the acquire succeeds, the message and message length are assigned to
> nbcon_write_context so ->write_atomic can print the message.
> 
> After release try to flush the console since there may be a backlog of
> messages in the ringbuffer. The kthread console printers do not get a
> chance to run while kdb is active.
> 
> Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

[PATCH v4 3/5] printk: nbcon: Allow KDB to acquire the NBCON context

[Marcos Paulo de Souza]

KDB can interrupt any console to execute the "mirrored printing" at any
time, so add an exception to nbcon_context_try_acquire_direct to allow
to get the context if the current CPU is the same as kdb_printf_cpu.

This change will be necessary for the next patch, which fixes
kdb_msg_write to work with NBCON consoles by calling ->write_atomic on
such consoles. But to print it first needs to acquire the ownership of
the console, so nbcon_context_try_acquire_direct is fixed here.

Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
---
 include/linux/kdb.h   | 6 ++++++
 kernel/printk/nbcon.c | 7 ++++++-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/include/linux/kdb.h b/include/linux/kdb.h
index ecbf819deeca118f27e98bf71bb37dd27a257ebb..9417ad7f124e95987caced07bc8684a1a6c04df4 100644
--- a/include/linux/kdb.h
+++ b/include/linux/kdb.h
@@ -207,11 +207,17 @@ static inline const char *kdb_walk_kallsyms(loff_t *pos)
 /* Dynamic kdb shell command registration */
 extern int kdb_register(kdbtab_t *cmd);
 extern void kdb_unregister(kdbtab_t *cmd);
+
+#define KDB_IS_ACTIVE() (READ_ONCE(kdb_printf_cpu) != raw_smp_processor_id())
+
 #else /* ! CONFIG_KGDB_KDB */
 static inline __printf(1, 2) int kdb_printf(const char *fmt, ...) { return 0; }
 static inline void kdb_init(int level) {}
 static inline int kdb_register(kdbtab_t *cmd) { return 0; }
 static inline void kdb_unregister(kdbtab_t *cmd) {}
+
+#define KDB_IS_ACTIVE() false
+
 #endif	/* CONFIG_KGDB_KDB */
 enum {
 	KDB_NOT_INITIALIZED,
diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
index ff218e95a505fd10521c2c4dfb00ad5ec5773953..8644e019e2391797e623fcc124d37ed4d460ccd9 100644
--- a/kernel/printk/nbcon.c
+++ b/kernel/printk/nbcon.c
@@ -10,6 +10,7 @@
 #include <linux/export.h>
 #include <linux/init.h>
 #include <linux/irqflags.h>
+#include <linux/kdb.h>
 #include <linux/kthread.h>
 #include <linux/minmax.h>
 #include <linux/percpu.h>
@@ -248,13 +249,17 @@ static int nbcon_context_try_acquire_direct(struct nbcon_context *ctxt,
 		 * since all non-panic CPUs are stopped during panic(), it
 		 * is safer to have them avoid gaining console ownership.
 		 *
-		 * If this acquire is a reacquire (and an unsafe takeover
+		 * One exception is if kdb is active, which may print
+		 * from multiple CPUs during a panic.
+		 *
+		 * Second exception is a reacquire (and an unsafe takeover
 		 * has not previously occurred) then it is allowed to attempt
 		 * a direct acquire in panic. This gives console drivers an
 		 * opportunity to perform any necessary cleanup if they were
 		 * interrupted by the panic CPU while printing.
 		 */
 		if (other_cpu_in_panic() &&
+		    !KDB_IS_ACTIVE() &&
 		    (!is_reacquire || cur->unsafe_takeover)) {
 			return -EPERM;
 		}

-- 
2.51.0

Re: [PATCH v4 3/5] printk: nbcon: Allow KDB to acquire the NBCON context

[Petr Mladek]

On Mon 2025-09-15 08:20:32, Marcos Paulo de Souza wrote:
> KDB can interrupt any console to execute the "mirrored printing" at any
> time, so add an exception to nbcon_context_try_acquire_direct to allow
> to get the context if the current CPU is the same as kdb_printf_cpu.
> 
> This change will be necessary for the next patch, which fixes
> kdb_msg_write to work with NBCON consoles by calling ->write_atomic on
> such consoles. But to print it first needs to acquire the ownership of
> the console, so nbcon_context_try_acquire_direct is fixed here.
> 
> --- a/include/linux/kdb.h
> +++ b/include/linux/kdb.h
> @@ -207,11 +207,17 @@ static inline const char *kdb_walk_kallsyms(loff_t *pos)
>  /* Dynamic kdb shell command registration */
>  extern int kdb_register(kdbtab_t *cmd);
>  extern void kdb_unregister(kdbtab_t *cmd);
> +
> +#define KDB_IS_ACTIVE() (READ_ONCE(kdb_printf_cpu) != raw_smp_processor_id())

The condition looks inverted. It should be true when the CPU ID matches.

I actually think about using similar approach and naming scheme
as for the similar API checking @panic_cpu. There are patches
in -mm tree which consolidated that API, see
https://lore.kernel.org/r/20250825022947.1596226-2-wangjinchao600@gmail.com

In our case, the similar API would be:

/* Return true when KDB has locked for printing a message on this CPU. */
static inline
bool kdb_printf_on_this_cpu(void)
{
	/*
	 * We can use raw_smp_processor_id() here because the task could
	 * not get migrated when KDB has locked for printing on this CPU.
	 */
	return unlikely(READ_ONCE(kdb_printf_cpu) == raw_smp_processor_id());
}

> +
>  #else /* ! CONFIG_KGDB_KDB */
>  static inline __printf(1, 2) int kdb_printf(const char *fmt, ...) { return 0; }
>  static inline void kdb_init(int level) {}
>  static inline int kdb_register(kdbtab_t *cmd) { return 0; }
>  static inline void kdb_unregister(kdbtab_t *cmd) {}
> +
> +#define KDB_IS_ACTIVE() false

and here to match the style above:

static inline bool kdb_printf_on_this_cpu(void) { return false };

> +
>  #endif	/* CONFIG_KGDB_KDB */
>  enum {
>  	KDB_NOT_INITIALIZED,
> diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
> index ff218e95a505fd10521c2c4dfb00ad5ec5773953..8644e019e2391797e623fcc124d37ed4d460ccd9 100644
> --- a/kernel/printk/nbcon.c
> +++ b/kernel/printk/nbcon.c
> @@ -248,13 +249,17 @@ static int nbcon_context_try_acquire_direct(struct nbcon_context *ctxt,
>  		 * since all non-panic CPUs are stopped during panic(), it
>  		 * is safer to have them avoid gaining console ownership.
>  		 *
> -		 * If this acquire is a reacquire (and an unsafe takeover
> +		 * One exception is if kdb is active, which may print
> +		 * from multiple CPUs during a panic.

Also here the "active" is a bit ambiguous term. I would use:

		 * One exception is when kdb has locked for printing on this
		 * CPU.

> +		 *
> +		 * Second exception is a reacquire (and an unsafe takeover
>  		 * has not previously occurred) then it is allowed to attempt
>  		 * a direct acquire in panic. This gives console drivers an
>  		 * opportunity to perform any necessary cleanup if they were
>  		 * interrupted by the panic CPU while printing.
>  		 */
>  		if (other_cpu_in_panic() &&
> +		    !KDB_IS_ACTIVE() &&
>  		    (!is_reacquire || cur->unsafe_takeover)) {
>  			return -EPERM;
>  		}

I am sorry that I did not suggested the better names already when
this new API was discussed in v3.

Best Regards,
Petr

[PATCH v4 4/5] printk: nbcon: Export nbcon_write_context_set_buf

[Marcos Paulo de Souza]

This function will be used in the next patch to allow a driver to set
both the message and message length of a nbcon_write_context. This is
necessary because the function also initializes the ->unsafe_takeover
struct member. By using this helper we ensure that the struct is
initialized correctly.

Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
---
 include/linux/console.h | 4 ++++
 kernel/printk/nbcon.c   | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/include/linux/console.h b/include/linux/console.h
index b34c5a0b86303e2fb4583fa467d8be43761cf756..e0fc2608bd9d6a886f5ddc56d26f19b21ae8663d 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -602,6 +602,8 @@ static inline bool console_is_registered(const struct console *con)
 extern void nbcon_cpu_emergency_enter(void);
 extern void nbcon_cpu_emergency_exit(void);
 extern bool nbcon_can_proceed(struct nbcon_write_context *wctxt);
+extern void nbcon_write_context_set_buf(struct nbcon_write_context *wctxt,
+					char *buf, unsigned int len);
 extern bool nbcon_enter_unsafe(struct nbcon_write_context *wctxt);
 extern bool nbcon_exit_unsafe(struct nbcon_write_context *wctxt);
 extern void nbcon_reacquire_nobuf(struct nbcon_write_context *wctxt);
@@ -654,6 +656,8 @@ static inline bool console_is_usable(struct console *con, short flags, bool use_
 static inline void nbcon_cpu_emergency_enter(void) { }
 static inline void nbcon_cpu_emergency_exit(void) { }
 static inline bool nbcon_can_proceed(struct nbcon_write_context *wctxt) { return false; }
+static inline void nbcon_write_context_set_buf(struct nbcon_write_context *wctxt,
+					       char *buf, unsigned int len) { }
 static inline bool nbcon_enter_unsafe(struct nbcon_write_context *wctxt) { return false; }
 static inline bool nbcon_exit_unsafe(struct nbcon_write_context *wctxt) { return false; }
 static inline void nbcon_reacquire_nobuf(struct nbcon_write_context *wctxt) { }
diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
index 8644e019e2391797e623fcc124d37ed4d460ccd9..085930386d4c4accf69214d341c950aafb5d0b32 100644
--- a/kernel/printk/nbcon.c
+++ b/kernel/printk/nbcon.c
@@ -855,7 +855,7 @@ static bool __nbcon_context_update_unsafe(struct nbcon_context *ctxt, bool unsaf
 	return nbcon_context_can_proceed(ctxt, &cur);
 }
 
-static void nbcon_write_context_set_buf(struct nbcon_write_context *wctxt,
+void nbcon_write_context_set_buf(struct nbcon_write_context *wctxt,
 					char *buf, unsigned int len)
 {
 	struct nbcon_context *ctxt = &ACCESS_PRIVATE(wctxt, ctxt);

-- 
2.51.0

Re: [PATCH v4 4/5] printk: nbcon: Export nbcon_write_context_set_buf

[Petr Mladek]

On Mon 2025-09-15 08:20:33, Marcos Paulo de Souza wrote:
> This function will be used in the next patch to allow a driver to set
> both the message and message length of a nbcon_write_context. This is
> necessary because the function also initializes the ->unsafe_takeover
> struct member. By using this helper we ensure that the struct is
> initialized correctly.
> 
> Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

[PATCH v4 5/5] kdb: Adapt kdb_msg_write to work with NBCON consoles

[Marcos Paulo de Souza]

Function kdb_msg_write was calling con->write for any found console,
but it won't work on NBCON consoles. In this case we should acquire the
ownership of the console using NBCON_PRIO_EMERGENCY, since printing
kdb messages should only be interrupted by a panic.

At this point, the console is required to use the atomic callback. The
console is skipped if the write_atomic callback is not set or if the
context could not be acquired. The validation of NBCON is done by the
console_is_usable helper. The context is released right after
write_atomic finishes.

The oops_in_progress handling is only needed in the legacy consoles,
so it was moved around the con->write callback.

Suggested-by: Petr Mladek <pmladek@suse.com>
Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
---
 kernel/debug/kdb/kdb_io.c | 47 ++++++++++++++++++++++++++++++++---------------
 1 file changed, 32 insertions(+), 15 deletions(-)

diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
index 9b11b10b120cf07e451a7a4d92ce50f9a6c066b2..7f41ce5d1b4b2b9970f7c84d8df40d13c9e9a084 100644
--- a/kernel/debug/kdb/kdb_io.c
+++ b/kernel/debug/kdb/kdb_io.c
@@ -589,24 +589,41 @@ static void kdb_msg_write(const char *msg, int msg_len)
 	 */
 	cookie = console_srcu_read_lock();
 	for_each_console_srcu(c) {
-		if (!(console_srcu_read_flags(c) & CON_ENABLED))
+		short flags = console_srcu_read_flags(c);
+
+		if (!console_is_usable(c, flags, true))
 			continue;
 		if (c == dbg_io_ops->cons)
 			continue;
-		if (!c->write)
-			continue;
-		/*
-		 * Set oops_in_progress to encourage the console drivers to
-		 * disregard their internal spin locks: in the current calling
-		 * context the risk of deadlock is a bigger problem than risks
-		 * due to re-entering the console driver. We operate directly on
-		 * oops_in_progress rather than using bust_spinlocks() because
-		 * the calls bust_spinlocks() makes on exit are not appropriate
-		 * for this calling context.
-		 */
-		++oops_in_progress;
-		c->write(c, msg, msg_len);
-		--oops_in_progress;
+
+		if (flags & CON_NBCON) {
+			struct nbcon_write_context wctxt = { };
+
+			/*
+			 * Do not continue if the console is NBCON and the context
+			 * can't be acquired.
+			 */
+			if (!nbcon_kdb_try_acquire(c, &wctxt))
+				continue;
+
+			nbcon_write_context_set_buf(&wctxt, (char *)msg, msg_len);
+
+			c->write_atomic(c, &wctxt);
+			nbcon_kdb_release(&wctxt);
+		} else {
+			/*
+			 * Set oops_in_progress to encourage the console drivers to
+			 * disregard their internal spin locks: in the current calling
+			 * context the risk of deadlock is a bigger problem than risks
+			 * due to re-entering the console driver. We operate directly on
+			 * oops_in_progress rather than using bust_spinlocks() because
+			 * the calls bust_spinlocks() makes on exit are not appropriate
+			 * for this calling context.
+			 */
+			++oops_in_progress;
+			c->write(c, msg, msg_len);
+			--oops_in_progress;
+		}
 		touch_nmi_watchdog();
 	}
 	console_srcu_read_unlock(cookie);

-- 
2.51.0

Re: [PATCH v4 5/5] kdb: Adapt kdb_msg_write to work with NBCON consoles

[Petr Mladek]

On Mon 2025-09-15 08:20:34, Marcos Paulo de Souza wrote:
> Function kdb_msg_write was calling con->write for any found console,
> but it won't work on NBCON consoles. In this case we should acquire the
> ownership of the console using NBCON_PRIO_EMERGENCY, since printing
> kdb messages should only be interrupted by a panic.
> 
> At this point, the console is required to use the atomic callback. The
> console is skipped if the write_atomic callback is not set or if the
> context could not be acquired. The validation of NBCON is done by the
> console_is_usable helper. The context is released right after
> write_atomic finishes.
> 
> The oops_in_progress handling is only needed in the legacy consoles,
> so it was moved around the con->write callback.
> 
> Suggested-by: Petr Mladek <pmladek@suse.com>
> Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>

It looks good to me:

Reviewed-by: Petr Mladek <pmladek@suse.com>

See one note below.

> --- a/kernel/debug/kdb/kdb_io.c
> +++ b/kernel/debug/kdb/kdb_io.c
> @@ -589,24 +589,41 @@ static void kdb_msg_write(const char *msg, int msg_len)
>  	 */
>  	cookie = console_srcu_read_lock();
>  	for_each_console_srcu(c) {
> -		if (!(console_srcu_read_flags(c) & CON_ENABLED))
> +		short flags = console_srcu_read_flags(c);
> +
> +		if (!console_is_usable(c, flags, true))
>  			continue;
>  		if (c == dbg_io_ops->cons)
>  			continue;
> -		if (!c->write)
> -			continue;
> -		/*
> -		 * Set oops_in_progress to encourage the console drivers to
> -		 * disregard their internal spin locks: in the current calling
> -		 * context the risk of deadlock is a bigger problem than risks
> -		 * due to re-entering the console driver. We operate directly on
> -		 * oops_in_progress rather than using bust_spinlocks() because
> -		 * the calls bust_spinlocks() makes on exit are not appropriate
> -		 * for this calling context.
> -		 */
> -		++oops_in_progress;
> -		c->write(c, msg, msg_len);
> -		--oops_in_progress;
> +
> +		if (flags & CON_NBCON) {
> +			struct nbcon_write_context wctxt = { };
> +
> +			/*
> +			 * Do not continue if the console is NBCON and the context
> +			 * can't be acquired.
> +			 */
> +			if (!nbcon_kdb_try_acquire(c, &wctxt))
> +				continue;
> +
> +			nbcon_write_context_set_buf(&wctxt, (char *)msg, msg_len);

I have overlooked the (char *) cast in the earlier versions of the
patchset. It would be nice to fix the nbcon API so that the parameter
could be passed as (const char *). It looks that the API using
struct nbcon_write_context never modifies the given buffer so
it would be the right thing.

But it is beyond the scope of this patchset. It would be material
for a separate code clean up ;-)

Best Regards,
Petr