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

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

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