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

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

On 2026-07-27, Sashiko wrote:
> > diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> > index 8c241ec7f4f29..5bcbb6deb12f7 100644
> > --- a/drivers/tty/serial/8250/8250_port.c
> > +++ b/drivers/tty/serial/8250/8250_port.c
> [ ... ]
> > @@ -3332,6 +3405,18 @@ void serial8250_console_write(struct uart_8250_port *up, const char *s,
> >  		mdelay(port->rs485.delay_rts_before_send);
> >  	}
> >  
> > +	/* If ownership was lost, no writing is allowed */
> > +	if (!nbcon_can_proceed(wctxt))
> > +		goto skip_write;
> > +
> > +	/*
> > +	 * If 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.
> > +	 */
> > +	if (!up->console_line_ended)
> > +		uart_console_write(port, "\n", 1, serial8250_console_wait_putchar);
> > +
>
> Will this cause the newline characters to be silently dropped if the
> hardware FIFO is already full from a preempted context?
>
> When uart_console_write() calls serial8250_console_wait_putchar(), it
> relies on wait_for_lsr() to wait for transmitter room. The timeout in
> wait_for_lsr() is calculated based on frame_time, which is only enough
> for a single character. If the preempted context left a full FIFO (e.g.,
> 16-64 bytes), the wait_for_lsr() timeout will expire before the FIFO
> drains, causing the hardware to drop the injected newline characters.

Sashiko is correct. For v11 I am writing helper functions to handle
@console_line_ended and the console line printing for fifo and
non-fifo. This causes a bit more churn than I would like to see at this
point (v11) but it will handle all situations, has increased code
re-use, and should be easier to read/maintain.

> [ ... ]
> >	while (!nbcon_enter_unsafe(wctxt))
> >		nbcon_reacquire_nobuf(wctxt);
> >
> >  	/*
> >  	 *	Finally, wait for transmitter to become empty
> >  	 *	and restore the IER
> >  	 */
> >  	wait_for_xmitr(up, UART_LSR_BOTH_EMPTY);
> >  
> > +	if (em485) {
> > +		mdelay(port->rs485.delay_rts_after_send);
> > +		if (em485->tx_stopped)
> > +			up->rs485_stop_tx(up, false);
> > +	}
> > +
> >  	serial_port_out(port, UART_IER, ier);
>
> Could the hardware state be corrupted here if a hostile takeover occurs
> while waiting for the transmitter to empty?
>
> The wait_for_xmitr() call can take a significant amount of time to let the
> FIFO drain. If a system panic triggers a hostile console takeover during
> this window, the panic context might reconfigure the hardware. Once the
> original context finishes waiting, it will unconditionally execute
> serial_port_out(), potentially undoing the panic context's configuration
> or causing interrupt storms.
>
> Should we check nbcon_can_proceed(wctxt) again before modifying the
> hardware state?

nbcon_can_proceed() is not enough because these final actions are
necessary. It would need to be:

	nbcon_exit_unsafe(wctxt);
	while (!nbcon_enter_unsafe(wctxt))
		nbcon_reacquire_nobuf(wctxt);

Although this does not close the window, it does make it quite
small. And if we are really in the unlikely situation of:

- a panic occurs
- the panic is unable to stop all the CPUs
- a console printer is actively printing from a non-stopped CPU
- the panic CPU performs a hostile takeover

then we should probably do everything we can (within reason) to avoid
interfering with the panic printing.

As Sashiko points out, the wait_for_xmitr() and mdelay() calls could be
relatively large. I agree that it makes sense to peform unsafe toggles
in these two locations. I will add that for v11. It will look something
like this:

@@ -3481,10 +3487,21 @@ void serial8250_console_write(struct uart_8250_port *up,
 
 	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);
+
 		if (em485->tx_stopped)
 			up->rs485_stop_tx(up, false);
 	}
 
+	/* Toggle unsafe after possibly long delay */
+	nbcon_exit_unsafe(wctxt);
+	while (!nbcon_enter_unsafe(wctxt))
+		nbcon_reacquire_nobuf(wctxt);
+
 	serial_port_out(port, UART_IER, ier);
 
 	/*

> > @@ -3400,8 +3509,24 @@ static unsigned int probe_baud(struct uart_port *port)
> >  	return (port->uartclk / 16) / quot;
> >  }
> >  
> > +/*
> > + * irq_work handler to perform modem control during console output.
> > + * Only triggered via ->write_atomic() callback because it may be
> > + * in a scheduler or NMI context, unable to wake tasks.
> > + */
> > +static void console_msr_handler(struct irq_work *iwp)
> > +{
> > +	struct uart_8250_port *up = container_of(iwp, struct uart_8250_port, console_msr_work);
> > +	struct uart_port *port = &up->port;
> > +
> > +	guard(uart_port_lock)(port);
> > +
> > +	serial8250_modem_status(up);
> > +}
>
> Does this code introduce a race condition that could cause invalid hardware
> accesses on a suspended device?
>
> If CPU1 is executing serial8250_console_write(), it reads
> up->console_msr_work_allow locklessly before queueing the work...

Sashiko is not correct. Both code blocks (reader and writer) are within
unsafe sections and therefore synchronized.

John