Re: [PATCH v2 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 2 potential issue(s) to consider:
- [High] printk: deadlock in braille_write() when preempting same-priority nbcon owner
- [Medium] printk: oops_in_progress fallback in braille_write() is silently dropped
--

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

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

This commit updates the Braille console driver to use the NBCON atomic
write callback when the underlying serial driver uses the NBCON API. It
adds functions to acquire and release the nbcon lock from the Braille
console and attempts to provide an oops-in-progress fallback.

> diff --git a/drivers/accessibility/braille/braille_console.c b/drivers/accessibility/braille/braille_console.c
> index 06b43b678d6ea..bae177cb8cf92 100644
> --- a/drivers/accessibility/braille/braille_console.c
> +++ b/drivers/accessibility/braille/braille_console.c
> @@ -62,14 +62,36 @@ 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;
> +
>  	u16 out;
>  	int i;
>  
>  	if (!braille_co)
>  		return;
>  
> +	if (braille_co->flags & CON_NBCON) {
> +		/*
> +		 * Braille console might be called from unknown context via
> +		 * vt_console_print() from console_unlock() from printk().
> +		 * Use the atomic callback and synchronize it just using
> +		 * the console context. Disable interrupts to prevent a nested
> +		 * call into the driver code which might cause a deadlock when
> +		 * trying to acquire the console ownership, see
> +		 * __nbcon_atomic_flush_pending_con().
> +		 */
> +		local_irq_save(flags);
> +		do {
> +			locked = nbcon_braille_try_acquire(braille_co, &wctxt);
> +			if (!locked)
> +				cpu_relax();
> +		} while (!locked && !oops_in_progress);

[Severity: High]
Can this loop cause a deadlock on the same CPU?

If uart_port_lock() is used, it acquires the nbcon lock at NBCON_PRIO_NORMAL
but does not disable interrupts. If an interrupt fires on this same CPU and
calls printk(), the VT legacy console will invoke braille_write().

Because braille_write() disables interrupts and enters this busy loop, and
nbcon_braille_try_acquire() hardcodes NBCON_PRIO_NORMAL, the acquisition fails
unconditionally. A context cannot preempt its own priority level.

Since interrupts are disabled, the interrupted lock owner can never resume to
release the lock, resulting in an infinite loop.

> +	}
> +
>  	if (!memcmp(lastwrite, buf, WIDTH * sizeof(*buf)))
> -		return;
> +		goto release_nbcon;
>  	memcpy(lastwrite, buf, WIDTH * sizeof(*buf));
>  
>  #define SOH 1
[ ... ]
> @@ -102,7 +124,24 @@ static void braille_write(u16 *buf)
>  	*c++ = csum;
>  	*c++ = ETX;
>  
> -	braille_co->write(braille_co, data, c - data);
> +	if (braille_co->flags & CON_NBCON) {
> +		if (braille_co->write_atomic &&
> +		    !(braille_co->flags & CON_NBCON_ATOMIC_UNSAFE)) {
> +			nbcon_write_context_set_buf(&wctxt, (char *)data, c - data);
> +			braille_co->write_atomic(braille_co, &wctxt);

[Severity: Medium]
Will this fallback silently drop oops messages if the lock is held by another
CPU?

If oops_in_progress is set and the lock is held by another CPU, the busy loop
above exits with locked = false. It then attempts to print directly by calling
braille_co->write_atomic() here.

Because wctxt does not own the console, the underlying driver's mandatory
nbcon_enter_unsafe() check will immediately fail, as nbcon_owner_matches()
returns false. This causes the underlying console driver to abort printing and
return immediately, discarding the oops message.

> +		} else {
> +			pr_warn_once("Braille requires a safe braille_co->write_atomic callback\n");
> +		}
> +	} else {
> +		braille_co->write(braille_co, data, c - data);
> +	}
> +
> +release_nbcon:
> +	if (braille_co->flags & CON_NBCON) {
> +		if (locked)
> +			nbcon_braille_release(&wctxt);
> +		local_irq_restore(flags);
> +	}
>  }
>  
>  /* Follow the VC cursor*/
[ ... ]
> diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
> index d17704fe93ae1..e5a0506330eba 100644
> --- a/kernel/printk/nbcon.c
> +++ b/kernel/printk/nbcon.c
[ ... ]
> +bool nbcon_braille_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;
> +	/* Keep in sync with nbcon_device_try_acquire(). */
> +	ctxt->prio = NBCON_PRIO_NORMAL;

[Severity: High]
Does hardcoding NBCON_PRIO_NORMAL here prevent the lock from being acquired
when the interrupted context on the same CPU already holds it?

> +
> +	return nbcon_context_try_acquire(ctxt, false);
> +}

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