Re: [PATCH v4 1/1] braille: nbcon: Allow to use a serial console with NBCON API as Braille console

[Petr Mladek <pmladek@suse.com>]

On Thu 2026-10-01 14:24:07, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] console: deadlock in braille_write() from atomic/IRQ context
> - [High] console: infinite loop in braille_write() when CONFIG_PRINTK is disabled
> --
> 
> commit 0bb1344e643dce4e38416fdf04095c16fe6023c8
> Author: Petr Mladek <pmladek@suse.com>
> 
> braille: nbcon: Allow to use a serial console with NBCON API as Braille console
> 
> This patch integrates the Braille console with the NBCON API by allowing
> braille_write() to attempt acquiring NBCON ownership before writing. It
> simulates the original locking using nbcon_braille_try_acquire() and routes
> output through write_atomic() or write_thread() callbacks.
> 
> > diff --git a/drivers/accessibility/braille/braille_console.c b/drivers/accessibility/braille/braille_console.c
> > --- a/drivers/accessibility/braille/braille_console.c
> > +++ b/drivers/accessibility/braille/braille_console.c
> [ ... ]
> > @@ -62,14 +62,50 @@ static void braille_write(u16 *buf)
> [ ... ]
> > +	if (braille_co->flags & CON_NBCON) {
> > +		if (panic_on_this_cpu()) {
> > +			/*
> > +			 * This should be good enough in practice. Most/all
> > +			 * serial console drivers have the atomic callback.
> > +			 */
> > +			if (!braille_co->write_atomic)
> > +				return;
> > +
> > +			local_irq_save(flags);
> > +			/* NBCON API strictly requires the ownership. */
> > +			if (!nbcon_braille_try_acquire(braille_co, &wctxt)) {
> > +				local_irq_restore(flags);
> > +				return;
> > +			}
> > +		} else {
> > +			braille_co->device_lock(braille_co, &flags);
> > +			while (!nbcon_braille_try_acquire(braille_co, &wctxt))
> > +				cpu_relax();
> > +		}
> > +	}
> 
> [Severity: High]
> Could this lead to a deadlock? braille_write() can be invoked by VT
> notifiers from atomic or IRQ contexts (e.g., from keyboard interrupts or
> console_flush_all()). By calling device_lock() and spinning on NBCON
> ownership with a cpu_relax() loop, if the interrupted context already holds
> the lock (like an active nbcon flush), nbcon_braille_try_acquire() will
> permanently return false, causing an infinite busy-loop.

The short answer is that we should be on the safe side because it
used to work with the legacy uart 8250 console driver which took
port->lock as well.

It should work when the code under port->lock:

   + is called with disabled interrupts.
   + do not call VT code (the is no reason to do so).
   + use only printk_deferred() (or better no printk() at all.

All the conditions should be fulfilled. Otherwise, it would cause
deadlocks even when the serial console driver is used for printk()
in the non-Braille mode.

> [ ... ]
> > @@ -102,7 +138,24 @@ static void braille_write(u16 *buf)
> [ ... ]
> > -	braille_co->write(braille_co, data, c - data);
> > +	if (braille_co->flags & CON_NBCON) {
> > +		nbcon_write_context_set_buf(&wctxt, (char *)data, c - data);
> > +		if (panic_on_this_cpu())
> > +			braille_co->write_atomic(braille_co, &wctxt);
> > +		else
> > +			braille_co->write_thread(braille_co, &wctxt);
> 
> [Severity: High]
> Does this violate the write_thread API constraints? The
> include/linux/console.h documentation specifies that write_thread must
> strictly execute in a task context. Since braille_write() can be called
> from an atomic or IRQ context, invoking the kthread-specific callback here
> could be unsafe.

It should be OK for all the currently converted serial console drivers
which are candidates for the Braille console. We might need to revisit
this in the future.

> > +	} else {
> > +		braille_co->write(braille_co, data, c - data);
> > +	}
> [ ... ]
> > diff --git a/include/linux/console.h b/include/linux/console.h
> > --- a/include/linux/console.h
> > +++ b/include/linux/console.h
> [ ... ]
> > @@ -678,8 +687,14 @@ 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 nbcon_write_context *wctxt) { }
> > +static inline
> > +bool nbcon_write_context_is_braille(struct nbcon_write_context *wctxt) { return false; }
> >  static inline bool console_is_usable(struct console *con, short flags,
> >  				     bool use_atomic) { return false; }
> > +static inline bool nbcon_braille_try_acquire(struct console *con,
> > +					     struct nbcon_write_context *wctxt) { return false; }
> 
> [Severity: High]
> Could this cause a hard lockup if CONFIG_A11Y_BRAILLE_CONSOLE is enabled
> but CONFIG_PRINTK is disabled?
> 
> Since this fallback stub unconditionally returns false, the
> while (!nbcon_braille_try_acquire(...)) cpu_relax(); loop in braille_write()
> will never terminate when triggered by terminal output.

Good point!

But there is a bigger problem. kernel/printk/nbcon.c is compiled only
when CONFIG_PRINTK is enabled. We expected that con->write*()
callbacks will be called only when there are printk() messages.

The Braille console is a new use-case. It uses con->write() callbacks
even for non-printk messages.

A proper solution would be to allow building nbcon.c even when
CONFIG_PRINTK is disbled. But we would need to review the entire
design.

A short term workaround is to make Braille console dependent on
CONFIG_PRINTK. It might be good enough in practice. I mean this:

diff --git a/drivers/accessibility/Kconfig b/drivers/accessibility/Kconfig
index 6b2f79d1f1b8..bd5db30e9aab 100644
--- a/drivers/accessibility/Kconfig
+++ b/drivers/accessibility/Kconfig
@@ -20,6 +20,7 @@ if ACCESSIBILITY
 config A11Y_BRAILLE_CONSOLE
 	bool "Console on braille device"
 	depends on VT
+	depends on PRINTK
 	depends on SERIAL_CORE_CONSOLE
 	help
 	  Enables console output on a braille device connected to a 8250


Best Regards,
Petr