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

On Fri 2026-07-31 09:54:44, John Ogness wrote:
> On 2026-07-30, Petr Mladek <[email protected]> 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.

Yup.

> > /**
> >  * 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()

Yes, it looks better.

> 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.

Yes, a good place to document it would be a comment above a helper
function.

> > 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/[email protected]/
> 
> 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.
> 
> > 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.

I have added the locking because of:

 + uart_ioctl()
   + uart_set_rs485_config()
     + uart_sanitize_serial_rs485()
       + uart_sanitize_serial_rs485_delays()

which does

	} else if (rs485->delay_rts_after_send > RS485_MAX_RTS_DELAY) {
		rs485->delay_rts_after_send = RS485_MAX_RTS_DELAY;

But I guess that this is is a rather theoretical race.

> > 		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.

Fair enough.

> > 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().

I agree that v11 looks good for the mainline.
We could always improve/clean up the code later.

Best Regards,
Petr