Re: [PATCH v3 4/4] kdb: Adapt kdb_msg_write to work with NBCON consoles

[Petr Mladek <pmladek@suse.com>]

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