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

[John Ogness <john.ogness@linutronix.de>]

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