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

[Marcos Paulo de Souza <mpdesouza@suse.com>]

On Sun, 2025-07-13 at 22:42 +0206, John Ogness wrote:
> On 2025-07-13, Marcos Paulo de Souza <mpdesouza@suse.com> wrote:
> > diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
> > index
> > 9b11b10b120cf07e451a7a4d92ce50f9a6c066b2..3b7365c11d06b01d487767fd8
> > 9f1081da10dd2ed 100644
> > --- a/kernel/debug/kdb/kdb_io.c
> > +++ b/kernel/debug/kdb/kdb_io.c
> > @@ -558,6 +558,25 @@ static int kdb_search_string(char *searched,
> > char *searchfor)
> >  	return 0;
> >  }
> >  
> > +static struct nbcon_context *nbcon_acquire_ctxt(struct console
> > *con,
> > +					struct nbcon_write_context
> > *wctxt,
> > +					char *msg, int msg_len)
> > +{
> > +	struct nbcon_context *ctxt = &ACCESS_PRIVATE(wctxt, ctxt);
> > +
> > +	ctxt->console		    = con;
> > +	ctxt->spinwait_max_us	    = 0;
> > +	ctxt->prio		    = NBCON_PRIO_EMERGENCY;
> > +	ctxt->allow_unsafe_takeover = false;
> > +	wctxt->outbuf		    = msg;
> > +	wctxt->len		    = msg_len;
> > +
> > +	if (!nbcon_context_try_acquire(ctxt))
> > +		return NULL;
> > +
> > +	return ctxt;
> 
> This function is grabbing a reference to a private member and
> returning
> it, thus exposing internals. Can we instead create a proper API in
> kernel/printk/nbcon.c for kdb?
> 
> For example, take a look at:
> 
> nbcon_device_try_acquire() and nbcon_device_release()
> 
> We could have something similar for kdb, such as:
> 
> bool *nbcon_kdb_try_acquire(struct nbcon_write_context *wctxt,
> 			    struct console *con, char *msg, int
> msg_len);
> 
> void nbcon_kdb_release(struct nbcon_write_context *wctxt);

Makes sense John! Thanks for the quick review and suggestion! I'll work
on it in the next few days, but also wait for more people to take a
look as well.

> 
> > +}
> > +
> >  static void kdb_msg_write(const char *msg, int msg_len)
> >  {
> >  	struct console *c;
> > @@ -589,12 +608,26 @@ 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 = { };
> > +		struct nbcon_context *ctxt;
> 
> With the above suggestion we do not need @ctxt.
> 
> > +		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;
> > +
> > +		/*
> > +		 * Do not continue if the console is NBCON and the
> > context
> > +		 * can't be acquired.
> > +		 */
> > +		if (flags & CON_NBCON) {
> > +			ctxt = nbcon_acquire_ctxt(c, &wctxt, (char
> > *)msg,
> > +						  msg_len);
> > +			if (!ctxt)
> > +				continue;
> 
> And this becomes:
> 
> 			if (!nbcon_kdb_try_acquire(&wctxt, c, (char
> *)msg, msg_len))
> 				continue;

Agreed.

> > +		}
> > +
> >  		/*
> >  		 * Set oops_in_progress to encourage the console
> > drivers to
> >  		 * disregard their internal spin locks: in the
> > current calling
> > @@ -605,7 +638,12 @@ static void kdb_msg_write(const char *msg, int
> > msg_len)
> >  		 * for this calling context.
> >  		 */
> >  		++oops_in_progress;
> > -		c->write(c, msg, msg_len);
> > +		if (flags & CON_NBCON) {
> > +			c->write_atomic(c, &wctxt);
> > +			nbcon_context_release(ctxt);
> 
> And this becomes:
> 
> 			nbcon_kdb_release(&wctxt);

Much better and cleaner!

> 
> > +		} else {
> > +			c->write(c, msg, msg_len);
> > +		}
> >  		--oops_in_progress;
> >  		touch_nmi_watchdog();
> >  	}
> 

Thanks for the quick review!

> John Ogness