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

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

On 2026-07-20, Sashiko wrote:
>> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
>> index c0e8a4efbdcc8..f504621c5e24a 100644
>> --- a/drivers/tty/serial/8250/8250_core.c
>> +++ b/drivers/tty/serial/8250/8250_core.c
>> @@ -584,6 +609,9 @@ void serial8250_suspend_port(int line)
>>  	struct uart_8250_port *up = &serial8250_ports[line];
>>  	struct uart_port *port = &up->port;
>>  
>> +	/* No irq_work may be queued when suspending. */
>> +	up->avoid_modem_status_work = true;
>> +
>>  	if (!console_suspend_enabled && uart_console(port) &&
>>  	    port->type != PORT_8250) {
>>  		unsigned char canary = 0xa5;
>
> Is a call to irq_work_sync(&up->modem_status_work) needed here to
> flush any already queued work before the port is suspended?
>
> If an atomic console write triggers irq_work_queue() just as the
> system initiates suspend, this flag blocks new work but doesn't
> synchronize existing work.
>
> The asynchronous handler could then execute after the device clock is
> gated or memory unmapped, which might crash on serial_in(up,
> UART_MSR). (A similar teardown might also be needed in
> serial8250_console_exit()).

Sashiko is correct. For v7 I will add irq_work_sync() here and in
serial8250_console_exit().

>> -void serial8250_console_write(struct uart_8250_port *up, const char *s,
>> -			      unsigned int count)
>> +void serial8250_console_write(struct uart_8250_port *up,
>> +			      struct nbcon_write_context *wctxt,
>> +			      bool is_atomic)
>>  {
>>  	struct uart_8250_em485 *em485 = up->em485;
>>  	struct uart_port *port = &up->port;
>> -	unsigned long flags;
>> -	unsigned int ier, use_fifo;
>> -	int locked = 1;
>> -
>> -	touch_nmi_watchdog();
>> +	unsigned int ier;
>> +	bool use_fifo;
>>  
>> -	if (oops_in_progress)
>> -		locked = uart_port_trylock_irqsave(port, &flags);
>> -	else
>> -		uart_port_lock_irqsave(port, &flags);
>> +	if (!nbcon_enter_unsafe(wctxt))
>> +		return;
>>  
>>  	/*
>> -	 *	First save the IER then disable the interrupts
>> +	 * First, save the IER, then disable the interrupts. The special
>> +	 * variant to clear the IER is used because console printing may
>> +	 * occur without holding the port lock.
>>  	 */
>>  	ier = serial_port_in(port, UART_IER);
>> -	serial8250_clear_IER(up);
>> +	__serial8250_clear_IER(up);
>
> Since UART_IER is now read and saved into a local variable without
> holding port->lock during an atomic write, can concurrent driver
> updates be clobbered?
>
> If a normal driver operation like serial8250_start_tx() runs
> concurrently on another CPU, takes port->lock, and modifies UART_IER
> (e.g., setting UART_IER_THRI), the atomic console writer will later
> restore UART_IER from the stale local ier variable at the end of this
> function.
>
> Does this cause the concurrent driver update to be lost?

No, this is safe because the the port->lock is taken via wrappers
uart_port_lock*(), which also acquire nbcon ownership. For atomic
writes, nbcon ownership is acquired.

>> @@ -3352,10 +3425,23 @@ void serial8250_console_write(struct uart_8250_port *up, const char *s,
>>  		 */
>>  		!uart_console_hwflow_active(&up->port);
>>  
>> +	nbcon_exit_unsafe(wctxt);
>> +
>>  	if (likely(use_fifo))
>> -		serial8250_console_fifo_write(up, s, count);
>> +		serial8250_console_fifo_write(up, wctxt);
>>  	else
>> -		uart_console_write(port, s, count, serial8250_console_wait_putchar);
>> +		serial8250_console_byte_write(up, wctxt);
>> +skip_write:
>> +	/*
>> +	 * If ownership was lost, this context must reacquire ownership and
>> +	 * re-enter the unsafe section in order to perform final actions
>> +	 * (such as re-enabling interrupts).
>> +	 */
>> +	if (!nbcon_can_proceed(wctxt)) {
>> +		do {
>> +			nbcon_reacquire_nobuf(wctxt);
>> +		} while (!nbcon_enter_unsafe(wctxt));
>> +	}
>
> Does this path leave the nbcon unsafe section unbalanced?
>
> If console ownership is not lost, nbcon_can_proceed(wctxt) evaluates
> to true, so the if block is skipped and nbcon_enter_unsafe() is never
> called.
>
> This leaves the subsequent hardware access wait_for_xmitr()
> unprotected, and the final nbcon_exit_unsafe(wctxt) call will
> underflow the unsafe section tracker because it was never re-entered.

Sashiko is correct. Because nbcon ownership can be lost at any time,
such underflows cannot be detected. For v7 I will change this block to:

	/*
	 * Re-enter the 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);

>> 	/*
>> 	 *	Finally, wait for transmitter to become empty
>> 	 *	and restore the IER
>> 	 */
>> 	wait_for_xmitr(up, UART_LSR_BOTH_EMPTY);
>> 	serial_port_out(port, UART_IER, ier);
>
> Since serial8250_console_write() no longer holds port->lock during an
> atomic write, do calls to wait_for_xmitr() and
> serial8250_fifo_wait_for_lsr_thre() introduce data races on
> up->lsr_saved_flags and up->msr_saved_flags?
>
> For example, wait_for_xmitr() calls serial_in(up, UART_MSR) which
> updates flags like so:
>
> wait_for_xmitr()
>     msr = serial_in(up, UART_MSR);
>     up->msr_saved_flags |= msr & MSR_SAVE_FLAGS;
>
> This read-modify-write races with the normal
> serial8250_handle_irq_locked() IRQ handler running concurrently under
> port->lock, potentially overwriting and permanently dropping hardware
> events like parity errors or modem control changes.

No, this is safe because the the port->lock is taken via wrappers
uart_port_lock*(), which also acquire nbcon ownership. wait_for_xmitr()
and serial8250_fifo_wait_for_lsr_thre() are called with nbcon ownership.

John