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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] vt/braille: sleep in atomic context via nbcon task callbacks
- [High] vt/braille: infinite busy-loop deadlock during concurrent panic
- [Low] tty/sifive: build failure due to unlinely() typo
- [Low] printk/8250: linker error due to missing EXPORT_SYMBOL_GPL
- [Low] vt/braille: uninitialized variable locked compiler warning
--

commit cad32ed5b6a2a2082f9354528d2e20acb9b2661a
Author: Petr Mladek <pmladek@suse.com>

braille: nbcon: Allow to use a serial console with NBCON API as Braille console

This commit allows serial consoles using the NBCON API to act as Braille
consoles. It simulates the original locking by taking the device lock and
acquiring the nbcon context, and it updates existing serial drivers to avoid
newline prepending when used as a Braille console.

> diff --git a/drivers/accessibility/braille/braille_console.c b/drivers/accessibility/braille/braille_console.c
> index 06b43b678d6ea..78611019b671e 100644
> --- a/drivers/accessibility/braille/braille_console.c
> +++ b/drivers/accessibility/braille/braille_console.c
> @@ -62,14 +62,45 @@ static void braille_write(u16 *buf)
>  {
>  	static u16 lastwrite[WIDTH];
>  	unsigned char data[1 + 1 + 2*WIDTH + 2 + 1], csum = 0, *c;
> +	struct nbcon_write_context wctxt = { };
> +	unsigned long flags;
> +	bool locked;

[Severity: Low]
Could this trigger a -Wmaybe-uninitialized compiler warning?

The compiler evaluates two independent calls to panic_on_this_cpu() in this
function. Because panic_on_this_cpu() reads an atomic variable, the compiler
may consider execution paths where the first check is false (skipping
initialization of the locked variable) and the second is true (reading the
locked variable).

Should the result of panic_on_this_cpu() be cached in a local variable?

[ ... ]
> +	if (braille_co->flags & CON_NBCON) {
> +		if (panic_on_this_cpu()) {
> +			local_irq_save(flags);
> +			locked = nbcon_braille_try_acquire(braille_co, &wctxt);
> +			/* NBCON API strictly requires the ownership. */
> +			if (!locked) {
> +				local_irq_restore(flags);
> +				return;
> +			}
> +		} else {
> +			braille_co->device_lock(braille_co, &flags);

[Severity: High]
Does this sleep in an atomic context?

braille_write() is invoked from keyboard_notifier_call() via the
keyboard_notifier_list atomic notifier chain with local interrupts disabled.
The device_lock() callback is documented as a task-context API that may
block (e.g., using a mutex). Is it safe to call it here?

> +			while (!nbcon_braille_try_acquire(braille_co, &wctxt))
> +				cpu_relax();

[Severity: High]
Can this loop cause a deadlock during a concurrent panic?

If CPU A runs braille_write() in the non-panic path and enters this loop,
and concurrently CPU B triggers a panic and acquires the console context
with NBCON_PRIO_PANIC, CPU A will attempt to acquire with NBCON_PRIO_NORMAL
which unconditionally fails against panic priority.

Because braille_write() runs with local IRQs disabled, CPU A would spin
infinitely and could never be interrupted by the panic IPI from CPU B.

[ ... ]
> @@ -102,7 +133,27 @@ static void braille_write(u16 *buf)
>  	*c++ = csum;
>  	*c++ = ETX;
>  
> -	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 also sleep in an atomic context?

Similar to device_lock(), the write_thread() callback is strictly a
task-context API. Calling it from the atomic notifier context could lead
to deadlocks or might_sleep() splats.

[ ... ]
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index 45dc9ab5bfd87..9019b2fe221f0 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -3432,8 +3432,11 @@ static void __serial8250_console_write(struct uart_8250_port *up,
>  	 * If the console printer did not fully output the previous line, it
>  	 * must have been handed or taken over. Insert a newline in order to
>  	 * maintain clean output.
> +	 *
> +	 * Braille consoles are an exception. The serial port is not used
> +	 * for printk(). The driver is supposed to write exactly what it gets.
>  	 */
> -	if (!up->console_line_ended) {
> +	if (unlikely(!up->console_line_ended && !nbcon_write_context_is_braille(wctxt))) {

[Severity: Low]
Could this result in a linker error for modular builds?

__serial8250_console_write() calls nbcon_write_context_is_braille(), but
the latter is not exported via EXPORT_SYMBOL_GPL() in kernel/printk/nbcon.c.
Because the 8250 driver can be built as a loadable module 
(CONFIG_SERIAL_8250=m), will this break the build due to unresolved
external symbols?

[ ... ]
> diff --git a/drivers/tty/serial/sifive.c b/drivers/tty/serial/sifive.c
> index 5d76f04736260..9d67ee3561d60 100644
> --- a/drivers/tty/serial/sifive.c
> +++ b/drivers/tty/serial/sifive.c
> @@ -821,7 +821,7 @@ static void sifive_serial_console_write_atomic(struct console *co,
>  	ier = __ssp_readl(ssp, SIFIVE_SERIAL_IE_OFFS);
>  	__ssp_writel(0, SIFIVE_SERIAL_IE_OFFS, ssp);
>  
> -	if (!ssp->console_line_ended)
> +	if (unlinely(!ssp->console_line_ended && !nbcon_is_braille(co)))

[Severity: Low]
Does this code cause a build failure?

This looks like a typo ("unlinely" instead of "unlikely"). Because the macro
is undeclared, does this halt compilation when CONFIG_SERIAL_SIFIVE is
enabled?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001093946.112999-1-pmladek@suse.com?part=1