Function kdb_msg_write was calling con->write for any found console,
but it won't work on NBCON ones. 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 in the case it was
triggered by sysrq debug option. This is done by the
nbcon_kdb_{acquire,release} functions.
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 | 46 +++++++++++++++++++++++++++++++---------------
1 file changed, 31 insertions(+), 15 deletions(-)
diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
index 9b11b10b120cf07e451a7a4d92ce50f9a6c066b2..47bc31cc71bc84750db5d9304ed75a113cd382bf 100644
--- a/kernel/debug/kdb/kdb_io.c
+++ b/kernel/debug/kdb/kdb_io.c
@@ -589,24 +589,40 @@ 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))
+ struct nbcon_write_context wctxt = { };
+ 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) {
+ /*
+ * Do not continue if the console is NBCON and the context
+ * can't be acquired.
+ */
+ if (!nbcon_kdb_try_acquire(c, &wctxt))
+ continue;
+
+ wctxt.outbuf = (char *)msg;
+ wctxt.len = 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.50.0On Tue 2025-09-02 15:33:55, Marcos Paulo de Souza wrote:
> Function kdb_msg_write was calling con->write for any found console,
> but it won't work on NBCON ones. 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
I would end the paragraph here.
> in the case it was
> triggered by sysrq debug option.
This is not important. Also there are more ways how to trigger
panic(). For example, it might happen by an error in the kdb code.
Or I heard rumors that some HW even had a physical "trigger NMI" button.
> This is done by the
> nbcon_kdb_{acquire,release} functions.
I think that this is more or less obvious.
> 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.
> --- a/kernel/debug/kdb/kdb_io.c
> +++ b/kernel/debug/kdb/kdb_io.c
> @@ -589,24 +589,40 @@ 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))
> + struct nbcon_write_context wctxt = { };
> + 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) {
> + /*
> + * Do not continue if the console is NBCON and the context
> + * can't be acquired.
> + */
> + if (!nbcon_kdb_try_acquire(c, &wctxt))
> + continue;
> +
> + wctxt.outbuf = (char *)msg;
> + wctxt.len = msg_len;
I double checked whether we initialized all members of the structure
correctly. And I found that we didn't. We should call here:
nbcon_write_context_set_buf(&wctxt,
&pmsg.pbufs->outbuf[0],
pmsg.outbuf_len);
Sigh, this is easy to miss. I remember that I was not super happy
about this design. But the original code initialized the structure
on a single place...
> + 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);
Best Regards,
Petr
On Mon, 2025-09-08 at 17:51 +0200, Petr Mladek wrote:
> On Tue 2025-09-02 15:33:55, Marcos Paulo de Souza wrote:
> > Function kdb_msg_write was calling con->write for any found
> > console,
> > but it won't work on NBCON ones. 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
>
> I would end the paragraph here.
>
> > in the case it was
> > triggered by sysrq debug option.
>
> This is not important. Also there are more ways how to trigger
> panic(). For example, it might happen by an error in the kdb code.
> Or I heard rumors that some HW even had a physical "trigger NMI"
> button.
>
> > This is done by the
> > nbcon_kdb_{acquire,release} functions.
>
> I think that this is more or less obvious.
I'll adjust the commit message per you suggestion.
>
> > 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.
>
> > --- a/kernel/debug/kdb/kdb_io.c
> > +++ b/kernel/debug/kdb/kdb_io.c
> > @@ -589,24 +589,40 @@ 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))
> > + struct nbcon_write_context wctxt = { };
> > + 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) {
> > + /*
> > + * Do not continue if the console is NBCON
> > and the context
> > + * can't be acquired.
> > + */
> > + if (!nbcon_kdb_try_acquire(c, &wctxt))
> > + continue;
> > +
> > + wctxt.outbuf = (char *)msg;
> > + wctxt.len = msg_len;
>
> I double checked whether we initialized all members of the structure
> correctly. And I found that we didn't. We should call here:
>
> nbcon_write_context_set_buf(&wctxt,
> &pmsg.pbufs-
> >outbuf[0],
>
> pmsg.outbuf_len);
>
> Sigh, this is easy to miss. I remember that I was not super happy
> about this design. But the original code initialized the structure
> on a single place...
Ok, so I'll need to also export nbcon_write_context_set_buf, since it's
currently static inside kernel/printk/nbcon.c. I'll do it for the next
version.
>
> > + 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);
>
> Best Regards,
> Petr
On 2025-09-08, Marcos Paulo de Souza <mpdesouza@suse.com> wrote:
>> > --- a/kernel/debug/kdb/kdb_io.c
>> > +++ b/kernel/debug/kdb/kdb_io.c
>> > @@ -589,24 +589,40 @@ 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))
>> > + struct nbcon_write_context wctxt = { };
>> > + 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) {
>> > + /*
>> > + * Do not continue if the console is NBCON
>> > and the context
>> > + * can't be acquired.
>> > + */
>> > + if (!nbcon_kdb_try_acquire(c, &wctxt))
>> > + continue;
>> > +
>> > + wctxt.outbuf = (char *)msg;
>> > + wctxt.len = msg_len;
>>
>> I double checked whether we initialized all members of the structure
>> correctly. And I found that we didn't. We should call here:
>>
>> nbcon_write_context_set_buf(&wctxt,
>> &pmsg.pbufs-
>> >outbuf[0],
>>
>> pmsg.outbuf_len);
Nice catch.
>> Sigh, this is easy to miss. I remember that I was not super happy
>> about this design. But the original code initialized the structure
>> on a single place...
>
> Ok, so I'll need to also export nbcon_write_context_set_buf, since it's
> currently static inside kernel/printk/nbcon.c. I'll do it for the next
> version.
How about modifying nbcon_kdb_try_acquire() to also take @msg and
@msg_len. Then, on success, @wctxt is already prepared. I do not like
the idea of external code fiddling with the fields.
John
On Tue 2025-09-09 10:03:05, John Ogness wrote:
> On 2025-09-08, Marcos Paulo de Souza <mpdesouza@suse.com> wrote:
> >> > --- a/kernel/debug/kdb/kdb_io.c
> >> > +++ b/kernel/debug/kdb/kdb_io.c
> >> > @@ -589,24 +589,40 @@ 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))
> >> > + struct nbcon_write_context wctxt = { };
> >> > + 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) {
> >> > + /*
> >> > + * Do not continue if the console is NBCON
> >> > and the context
> >> > + * can't be acquired.
> >> > + */
> >> > + if (!nbcon_kdb_try_acquire(c, &wctxt))
> >> > + continue;
> >> > +
> >> > + wctxt.outbuf = (char *)msg;
> >> > + wctxt.len = msg_len;
> >>
> >> I double checked whether we initialized all members of the structure
> >> correctly. And I found that we didn't. We should call here:
> >>
> >> nbcon_write_context_set_buf(&wctxt,
> >> &pmsg.pbufs-
> >> >outbuf[0],
> >>
> >> pmsg.outbuf_len);
>
> Nice catch.
>
> >> Sigh, this is easy to miss. I remember that I was not super happy
> >> about this design.
I looked for details. I described my concerns at
https://lore.kernel.org/lkml/ZNY5gPNyyw9RTo4X@alley/#t
> >> But the original code initialized the structure
> >> on a single place...
> >
> > Ok, so I'll need to also export nbcon_write_context_set_buf, since it's
> > currently static inside kernel/printk/nbcon.c. I'll do it for the next
> > version.
>
> How about modifying nbcon_kdb_try_acquire() to also take @msg and
> @msg_len. Then, on success, @wctxt is already prepared. I do not like
> the idea of external code fiddling with the fields.
I was thinking about another solution, e.g. an nbcon_wctxt_init()
function. The problem is that wctxt->unsafe_takeover would need
to get updated after acquiring the context. And might be reused
for different consoles, ...
But wait. I do not see any code using wctxt->unsafe_takeover.
It seems that the motivation was that console drivers might
do something else when there was an unsafe_takeover in the past,
see https://lore.kernel.org/lkml/87cyz6ro62.fsf@jogness.linutronix.de/
But it seems that no console driver is using it.
So, I would prefer to remove the "unsafe_takeover" field from
struct nbcon_write_context and keep this kdb code as it is now.
We could always add it back when really needed.
Alternatively, we could create an API which could read this information
from struct wctxt.ctxt.con. But I would create this API only when
there is an user.
Best Regards,
Petr
On 2025-09-09, Petr Mladek <pmladek@suse.com> wrote: > The problem is that wctxt->unsafe_takeover would need to get updated > after acquiring the context. And might be reused for different > consoles, ... You are right. I think it is best to make nbcon_write_context_set_buf() available. > But wait. I do not see any code using wctxt->unsafe_takeover. > > It seems that the motivation was that console drivers might > do something else when there was an unsafe_takeover in the past, > see https://lore.kernel.org/lkml/87cyz6ro62.fsf@jogness.linutronix.de/ > But it seems that no console driver is using it. > > So, I would prefer to remove the "unsafe_takeover" field from > struct nbcon_write_context and keep this kdb code as it is now. No one is using it because the nbcon drivers are still implementing the "hope and pray" model on unsafe takeovers. The flag is an important part of the API to allow drivers to maximize their safety. I think it is too early to remove the flag when there are still so few nbcon drivers in existance. John
On Tue 2025-09-09 16:29:50, John Ogness wrote: > On 2025-09-09, Petr Mladek <pmladek@suse.com> wrote: > > The problem is that wctxt->unsafe_takeover would need to get updated > > after acquiring the context. And might be reused for different > > consoles, ... > > You are right. I think it is best to make nbcon_write_context_set_buf() > available. I am fine with it. > > But wait. I do not see any code using wctxt->unsafe_takeover. > > > > It seems that the motivation was that console drivers might > > do something else when there was an unsafe_takeover in the past, > > see https://lore.kernel.org/lkml/87cyz6ro62.fsf@jogness.linutronix.de/ > > But it seems that no console driver is using it. > > > > So, I would prefer to remove the "unsafe_takeover" field from > > struct nbcon_write_context and keep this kdb code as it is now. > > No one is using it because the nbcon drivers are still implementing the > "hope and pray" model on unsafe takeovers. The flag is an important part > of the API to allow drivers to maximize their safety. > > I think it is too early to remove the flag when there are still so few > nbcon drivers in existance. I feel that that I should be more strict and push for removing the flag because it is not used and complicates the design. I am sure that there are cleaner ways how to provide the information when anyone would need it. But I do not want to fight for it. It is not worth a blood (changing code back and forth). I am fine with exporting nbcon_write_context_set_buf() for now. We might know more about real users next time it causes problems ;-) Best Regards, Petr
On Tue, 2025-09-09 at 10:03 +0206, John Ogness wrote:
> On 2025-09-08, Marcos Paulo de Souza <mpdesouza@suse.com> wrote:
> > > > --- a/kernel/debug/kdb/kdb_io.c
> > > > +++ b/kernel/debug/kdb/kdb_io.c
> > > > @@ -589,24 +589,40 @@ 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))
> > > > + struct nbcon_write_context wctxt = { };
> > > > + 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) {
> > > > + /*
> > > > + * Do not continue if the console is
> > > > NBCON
> > > > and the context
> > > > + * can't be acquired.
> > > > + */
> > > > + if (!nbcon_kdb_try_acquire(c, &wctxt))
> > > > + continue;
> > > > +
> > > > + wctxt.outbuf = (char *)msg;
> > > > + wctxt.len = msg_len;
> > >
> > > I double checked whether we initialized all members of the
> > > structure
> > > correctly. And I found that we didn't. We should call here:
> > >
> > > nbcon_write_context_set_buf(&wctxt,
> > > &pmsg.pbufs-
> > > > outbuf[0],
> > >
> > > pmsg.outbuf_len);
>
> Nice catch.
>
> > > Sigh, this is easy to miss. I remember that I was not super happy
> > > about this design. But the original code initialized the
> > > structure
> > > on a single place...
> >
> > Ok, so I'll need to also export nbcon_write_context_set_buf, since
> > it's
> > currently static inside kernel/printk/nbcon.c. I'll do it for the
> > next
> > version.
>
> How about modifying nbcon_kdb_try_acquire() to also take @msg and
> @msg_len. Then, on success, @wctxt is already prepared. I do not like
> the idea of external code fiddling with the fields.
Hum.. indeed, it's ugly to pass msg and len, but it's better than
exposing more code for a case like this.
>
> John
© 2016 - 2025 Red Hat, Inc.