Re: [PATCH v3 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 12:54:37, John Ogness wrote:
> On 2026-10-01, Petr Mladek <pmladek@suse.com> wrote:
> > The Braille console is not registered in console_list. Instead, it is
> > integrated with the virtual terminal (VT) and shows what is displayed
> > on the terminal. It writes the data using con->write*() callback
> > of the associated serial console driver.
> >
> > --- 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;
> 
> There is no need for @locked because on failure, the function returns.

Great catch! I rewrote the the code many times and decided to send
it at some point...

> >  	u16 out;
> >  	int i;
> >  
> >  	if (!braille_co)
> >  		return;
> >  
> > +	/*
> > +	 * Braille console is not registered in console_list. Instead, it
> > +	 * is integrated with VT and shows what appears on the graphical
> > +	 * console under console_lock(). From this POV it is a legacy
> > +	 * console. But is calls serial console driver which might be
> 
>                      it ^^
> 
> > +	 * converted to the NBCON API. It is similar to
> > +	 * nbcon_legacy_emit_next_record() except that we should try
> > +	 * harder to get the lock. Othewise, the Braille device won't show
> 
>                          Otherwise ^^^^^^^^

My muscle memory is clearly wrong for this word.

> > +	 * everything what is displayed on the terminal.
> > +	 *
> > +	 * In short, simulate the original locking using NBCON API.
> > +	 */
> > +	if (braille_co->flags & CON_NBCON) {
> > +		if (panic_on_this_cpu()) {
> 
> How about adding here:
> 
> 			if (!braille_co->write_atomic)
> 				return;
>
> I see no reason to forbid Braille device usage just because it cannot
> show panics.

Fair enough. I am going to add the following in v4:

			/*
			 * 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);
> > +			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);
> > +			while (!nbcon_braille_try_acquire(braille_co, &wctxt))
> > +				cpu_relax();
> > +		}
> > +	}
> > +
> >  	if (!memcmp(lastwrite, buf, WIDTH * sizeof(*buf)))
> > -		return;
> > +		goto unlock_nbcon;
> >  	memcpy(lastwrite, buf, WIDTH * sizeof(*buf));
> >  
> >  #define SOH 1
> > @@ -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);
> > +	} else {
> > +		braille_co->write(braille_co, data, c - data);
> > +	}
> > +
> > +unlock_nbcon:
> > +	if (braille_co->flags & CON_NBCON) {
> > +		if (panic_on_this_cpu()) {
> > +			if (locked)
> > +				nbcon_braille_release(&wctxt);
> 
> There will never be a locked=false scenario here. We already returned.

Right!

> > +			local_irq_restore(flags);
> > +		} else {
> > +			nbcon_braille_release(&wctxt);
> > +			braille_co->device_unlock(braille_co, flags);
> > +		}
> > +	}
> >  }
> >  
> >  /* Follow the VC cursor*/
> > @@ -353,13 +404,22 @@ int braille_register_console(struct console *console, int index,
> >  	if (!console_options)
> >  		/* Only support VisioBraille for now */
> >  		console_options = "57600o8";
> > +
> >  	if (braille_co)
> >  		return -ENODEV;
> > +
> > +	if (console->flags & CON_NBCON &&
> > +	    (!console->write_atomic || console->flags & CON_NBCON_ATOMIC_UNSAFE)) {
> > +		pr_err("Braille console requires a safe braille_co->write_atomic callback\n");
> 
> IMO it is not necessary to restrict to !CON_NBCON_ATOMIC_UNSAFE consoles
> because if the acquire fails, an unsafe acquire is tried anyway. But as
> I suggested earlier, I think even NBCON consoles without
> ->write_atomic() should be allowed. Just no panic message for them.

I agree. I did not revisit this after I enabled the unsafe takeover
in panic(). I'll remove it in v4.

> > +		return -EINVAL;
> > +	}
> > +
> >  	if (console->setup) {
> >  		ret = console->setup(console, console_options);
> >  		if (ret != 0)
> >  			return ret;
> >  	}
> > +
> >  	console->flags |= CON_ENABLED;
> >  	console->index = index;
> >  	braille_co = console;
> > --- a/kernel/printk/nbcon.c
> > +++ b/kernel/printk/nbcon.c
> > @@ -2002,3 +2003,80 @@ void nbcon_kdb_release(struct nbcon_write_context *wctxt)
> >  	 */
> >  	__nbcon_atomic_flush_pending_con(ctxt->console, prb_next_reserve_seq(prb));
> >  }
> > +
> > +/**
> > + * nbcon_is_braille - Checks whether the nbcon write context is using Braille console
> > + *
> > + * @wctxt:	checked nbcon write context
> > + *
> > + * Return: True when the write context is associated with a Braille console.
> > + *	   Othrewise, return false.
> > + *
> > + * Context: Can be called in any context but only when Braille console is
> > + *	registered and the struct console could not disappear.
> > + */
> > +bool nbcon_write_context_is_braille(struct nbcon_write_context *wctxt)
> > +{
> > +	struct nbcon_context *ctxt = &ACCESS_PRIVATE(wctxt, ctxt);
> > +	struct console *con = ctxt->console;
> > +
> > +	return con && con->flags & CON_BRL;
> 
> I suggest parenthesis around "con->flags & CON_BRL".

Will do in v4.

> > +}
> > +

Thanks a lot for the quick review and catching so many details.

Best Regards,
Petr