Re: [PATCH tty v11 1/2] serial: 8250: Switch to nbcon console, take 2

[John Ogness <john.ogness@linutronix.de>]

On 2026-07-30, Petr Mladek <pmladek@suse.com> wrote:
>> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
>> index 8c241ec7f4f29..b9ea4f898474e 100644
>> --- a/drivers/tty/serial/8250/8250_port.c
>> +++ b/drivers/tty/serial/8250/8250_port.c
>> @@ -3352,10 +3458,17 @@ void serial8250_console_write(struct uart_8250_port *up, const char *s,
>>  		 */
>>  		!uart_console_hwflow_active(&up->port);
>>  
>> -	if (likely(use_fifo))
>> -		serial8250_console_fifo_write(up, s, count);
>> -	else
>> -		uart_console_write(port, s, count, serial8250_console_wait_putchar);
>> +	nbcon_exit_unsafe(wctxt);
>> +
>> +	__serial8250_console_write(up, wctxt, use_fifo);
>> +
>> +	/*
>> +	 * Re-enter an unsafe section in order to perform final actions
>> +	 * (such as re-enabling interrupts). If ownership was lost, this
>> +	 * context must reacquire ownership.
>> +	 */
>> +	while (!nbcon_enter_unsafe(wctxt))
>> +		nbcon_reacquire_nobuf(wctxt);
>
> If we are going to use this on more locations than I would add
> a new API, e.g.

Agreed.

The 8250 has already played a significant role in influencing other
NBCON implementations. I expect there will be a lot of copy/paste in the
beginning. Once we start seeing which code is copied and how it is used
in other drivers, we should be be able to do some general nbcon and
serial_core consolidation.

> /**
>  * nbcon_enter_unsafe_reacquire - Enter an unsafe region in the driver,
>  *	reacquire when needed.
>  * @wctxt:	The write context that was handed to the write function
>  *
>  *
>  * The function is allowed to reacquire the console ownership to make
>  * sure that the caller can enter an unsafe region in the driver.
>  *
>  * Warning: The caller must not longer access the text buffer. It is
>  *	lost when the reacquire was needed. The function is intended
>  *	for cleanup operations after emitting the message, for example,
>  *	re-enabling interrupts on a used serial port.
>  */
> void nbcon_enter_unsafe_reacquire(struct nbcon_write_context *wctxt)
> {
> 	while (!nbcon_enter_unsafe(wctxt))
> 		nbcon_reacquire_nobuf(wctxt);
> }

I would name the function:

	nbcon_enter_unsafe_reacquire_nobuf()

to make it clear that the buffer will be lost on a reacquire.

>> @@ -3365,10 +3478,21 @@ void serial8250_console_write(struct uart_8250_port *up, const char *s,
>>  
>>  	if (em485) {
>>  		mdelay(port->rs485.delay_rts_after_send);
>> +
>> +		/* Toggle unsafe after possibly long delay */
>> +		nbcon_exit_unsafe(wctxt);
>> +		while (!nbcon_enter_unsafe(wctxt))
>> +			nbcon_reacquire_nobuf(wctxt);
>
> The comment describes what the code does but it does not explain why.

Well, the _why_ is the same for all the toggling that we do throughout
the console code.

> My first idea was that it would allow to takeover the ownership
> as soon as possible in an emergency context.

Yes. That is one of the purposes. Even takeovers in panic will patiently
wait up to 2ms before becoming hostile.

> But the motivation seems to be to reduce the race with a possible
> unsafe takeover in the final panic flush, see
> https://lore.kernel.org/all/87ecgntf01.fsf@jogness.linutronix.de/

Yes, this is also valid, but for all other toggle situations as
well. The driver should avoid touching the hardware if it is no longer
the owner.

> It opens some questions:
>
> First, is it really necessary to finish the clean up when
> this CPU is supposed to be stopped and the panic CPU
> is about to enter the infinite loop?

Why even ask that question? The driver should try to do things
correctly. We want to avoid driver code like:

	if (panic_on_other_cpu())
		return;

The driver should not care about panic and instead focus on following
nbcon ownership rules.

> Second, it is actually safe in the emergency context?
> For example, the mdelay() is there for a reason. Maybe,
> we should repeat it when the ownership has been lost here?

If ownership has been lost, the new owner will have performed the
mdelay() and rs485_stop_tx() if needed. One could argue that if
ownership was lost, it should _not_ call ->rs485_stop_tx() at all. But
the new owner will also make an unnecessary call to
->rs485_start_tx().

If making these calls unnecessarily is a problem, the driver will need
to detect the situation and add this condition.

I have added "RS485-handovers" to my list of things to do some stress
testing.

> Third, is it necessary to call the mdelay() in the unsafe context?

No. It is not necessary. Worst case it will cause the printing thread to
handle a WARN that could have been printed atomically. But still, we
should avoid that.

> I think about entering unsafe only when really needed.
> Instead of releasing it and taking again immediately.
>
> Something like:
>
> 	/*
> 	 * Re-enter an unsafe section in order to perform final actions
> 	 * (such as re-enabling interrupts). If ownership was lost, this
> 	 * context must reacquire ownership.
> 	 */
> 	nbcon_enter_unsafe_reacquire(wctxt);
>
> 	/*
> 	 *	Finally, wait for transmitter to become empty
> 	 *	and restore the IER
> 	 */
> 	wait_for_xmitr(up, UART_LSR_BOTH_EMPTY);
>
> 	/*
> 	 * Exit unsafe section after a potentially long wait. It allows
> 	 * a safe takeover before another potentially long way. Also
> 	 * it reduces the race window when a possible unsafe takeover
> 	 * happened unnoticed.
> 	 */
> 	nbcon_exit_unsafe()
>
> 	if (em485) {
> 		u32 delay;
>
> 		nbcon_enter_unsafe_reacquire(wctxt);
> 		delay = port->rs485.delay_rts_after_send;
> 		nbcon_exit_unsafe(wctxt);

This unsafe enter/exit is unnecessary. This value does not change.

> 		mdelay(port->rs485.delay_rts_after_send);
>
> 		nbcon_enter_unsafe_reacquire(wctxt);
> 		if (em485->tx_stopped)
> 			up->rs485_stop_tx(up, false);
> 		nbcon_exit_unsafe(wctxt);
> 	}
>
> 	nbcon_enter_unsafe_reacquire(wctxt);
>
> 	serial_port_out(port, UART_IER, ier);
>
> 	/*
> 	 *	The receive handling will happen properly because the
> 	 *	receive ready bit will still be set; it is not cleared
> 	 *	on read.  However, modem control will not, we must
> 	 *	call it if we have saved something in the saved flags
> 	 *	while processing with interrupts off.
> 	 */
> 	if (up->msr_saved_flags) {
> 		if (is_atomic) {
> 			/*
> 			 * For atomic, MSR handling must be deferred to
> 			 * irq_work because this may be a context that does
> 			 * not permit waking up tasks.
> 			 *
> 			 * But no irq_work may be queued when suspending.
> 			 * In that case, the MSR handling will occur during
> 			 * resume in serial8250_resume_port().
> 			 */
> 			if (up->console_msr_work_allow)
> 				irq_work_queue(&up->console_msr_work);
> 		} else {
> 			serial8250_modem_status(up);
> 		}
> 	}
>
> 	nbcon_exit_unsafe(wctxt);
> }
>
> But I have to say that both approaches are quite hairy. I am not sure
> if it is worth it.

I would just move the unsafe_exit before the mdelay(). I think that is
sufficient here.

> Otherwise, I do not see any real problems in the code.

Thanks Petr for taking a detailed look at this!

I think v11 is OK for mainline (Greg has it in tty-testing now.) If
anything else comes up and I need to touch the code again, I will
relocate the unsafe_exit to before the mdelay().

John