Re: [PATCH v2 2/3] printk: nbcon: Introduce KDB helpers

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

On Tue, 2025-08-26 at 16:36 +0206, John Ogness wrote:
> On 2025-08-11, Marcos Paulo de Souza <mpdesouza@suse.com> wrote:
> > These helpers will be used when calling console->write_atomic on
> > KDB code in the next patch. It's basically the same implementaion
> > as nbcon_device_try_acquire, but using NBCON_PORIO_EMERGENCY when
> > acquiring the context.
> > 
> > For release, differently from nbcon_device_release, we don't need
> > to
> > flush the console, since all CPUs are stopped when KDB is active.
> > 
> > Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
> > ---
> >  include/linux/console.h |  6 ++++++
> >  kernel/printk/nbcon.c   | 26 ++++++++++++++++++++++++++
> >  2 files changed, 32 insertions(+)
> > 
> > diff --git a/include/linux/console.h b/include/linux/console.h
> > index
> > 67af483574727c00eea1d5a1eacc994755c92607..b34c5a0b86303e2fb4583fa46
> > 7d8be43761cf756 100644
> > --- a/include/linux/console.h
> > +++ b/include/linux/console.h
> > @@ -605,6 +605,9 @@ extern bool nbcon_can_proceed(struct
> > nbcon_write_context *wctxt);
> >  extern bool nbcon_enter_unsafe(struct nbcon_write_context *wctxt);
> >  extern bool nbcon_exit_unsafe(struct nbcon_write_context *wctxt);
> >  extern void nbcon_reacquire_nobuf(struct nbcon_write_context
> > *wctxt);
> > +extern bool nbcon_kdb_try_acquire(struct console *con,
> > +				  struct nbcon_write_context
> > *wctxt);
> > +extern void nbcon_kdb_release(struct nbcon_write_context *wctxt);
> >  
> >  /*
> >   * Check if the given console is currently capable and allowed to
> > print
> > @@ -654,6 +657,9 @@ static inline bool nbcon_can_proceed(struct
> > nbcon_write_context *wctxt) { return
> >  static inline bool nbcon_enter_unsafe(struct nbcon_write_context
> > *wctxt) { return false; }
> >  static inline bool nbcon_exit_unsafe(struct nbcon_write_context
> > *wctxt) { return false; }
> >  static inline void nbcon_reacquire_nobuf(struct
> > nbcon_write_context *wctxt) { }
> > +static inline bool nbcon_kdb_try_acquire(struct console *con,
> > +					 struct
> > nbcon_write_context *wctxt) { return false; }
> > +static inline void nbcon_kdb_release(struct console *con) { }
> >  static inline bool console_is_usable(struct console *con, short
> > flags,
> >  				     bool use_atomic) { return
> > false; }
> >  #endif
> > diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
> > index
> > 646801813415f0abe40cabf2f28ca9e30664f028..79d8c7437806119ad9787ddc4
> > 8382dc2c86c23c3 100644
> > --- a/kernel/printk/nbcon.c
> > +++ b/kernel/printk/nbcon.c
> > @@ -1855,3 +1855,29 @@ void nbcon_device_release(struct console
> > *con)
> >  	console_srcu_read_unlock(cookie);
> >  }
> >  EXPORT_SYMBOL_GPL(nbcon_device_release);
> > +
> > +bool nbcon_kdb_try_acquire(struct console *con,
> > +			   struct nbcon_write_context *wctxt)
> > +{
> > +	struct nbcon_context *ctxt = &ACCESS_PRIVATE(wctxt, ctxt);
> > +
> > +	memset(ctxt, 0, sizeof(*ctxt));
> > +	ctxt->console = con;
> > +	ctxt->prio    = NBCON_PRIO_EMERGENCY;
> > +
> > +	if (!nbcon_context_try_acquire(ctxt, false))
> > +		return false;
> > +
> > +	if (!nbcon_context_enter_unsafe(ctxt))
> > +		return false;
> > +
> > +	return true;
> > +}
> > +
> > +void nbcon_kdb_release(struct nbcon_write_context *wctxt)
> > +{
> > +	struct nbcon_context *ctxt = &ACCESS_PRIVATE(wctxt, ctxt);
> > +
> > +	nbcon_context_exit_unsafe(ctxt);
> > +	nbcon_context_release(ctxt);
> 
> If nbcon_context_exit_unsafe() fails, the current task is no longer
> the
> owner and thus a release is not needed. I would prefer:
> 
> 	if (nbcon_context_exit_unsafe(ctxt))
> 		nbcon_context_release(ctxt);
> 
> or
> 
> 	if (!nbcon_context_exit_unsafe(ctxt))
> 		return;
> 
> 	nbcon_context_release(ctxt);
> 
> You can find this same pattern in nbcon_device_release().

Thanks for spotting this issue. Fixed localy.

> 
> John