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

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

On 2026-07-22, Sashiko wrote:
>> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
>> index 8c241ec7f4f29..7726cca00d364 100644
>> --- a/drivers/tty/serial/8250/8250_port.c
>> +++ b/drivers/tty/serial/8250/8250_port.c
>> @@ -3198,11 +3218,22 @@ void serial8250_set_defaults(struct uart_8250_port *up)
>>  }
>>  EXPORT_SYMBOL_GPL(serial8250_set_defaults);
>>  
>> -void serial8250_fifo_wait_for_lsr_thre(struct uart_8250_port *up, unsigned int count)
>> +void serial8250_fifo_wait_for_lsr_thre(struct uart_8250_port *up,
>> +				       struct nbcon_write_context *wctxt,
>> +				       unsigned int count)
>>  {
>>  	unsigned int i;
>>  
>>  	for (i = 0; i < count; i++) {
>> +		/*
>> +		 * Pass the ownership as quickly as possible to a higher
>> +		 * priority context. Otherwise, its attempt to take over
>> +		 * the ownership might timeout. The new owner will wait
>> +		 * for UART_LSR_THRE before reusing the fifo.
>> +		 */
>> +		if (wctxt && !nbcon_can_proceed(wctxt))
>> +			return;
>> +
>>  		if (wait_for_lsr(up, UART_LSR_THRE))
>>  			return;
>>  	}
>
> Does this leave the hardware state vulnerable to corruption during an
> emergency console takeover?
>
> The serial8250_fifo_wait_for_lsr_thre() function polls the UART_LSR
> register by calling wait_for_lsr() and modifies up->lsr_saved_flags
> non-atomically.  Since reading UART_LSR clears hardware error flags
> (parity, framing, overrun, break), concurrent reads can cause one CPU
> to consume and lose the flags before the other can observe them.
>
> Because serial8250_fifo_wait_for_lsr_thre() is called outside the
> nbcon_enter_unsafe() block, the nbcon system might grant a safe
> takeover to an emergency message (e.g. sysrq, panic, or NMI printk)
> preempting this loop, as it assumes no hardware access is taking
> place. The emergency writer will then access the same hardware
> registers concurrently.

Sashiko is correct. I did not know that the serial_lsr_in() macro (used
by wait_for_lsr()) updates @lsr_saved_flags! I looked at the other
serial_lsr_in() call sites and they are all in nbcon unsafe sections.

The for-loop in serial8250_fifo_wait_for_lsr_thre() should look like
this:

	for (i = 0; i < count; i++) {
		/* ... */
		if (wctxt && !nbcon_enter_unsafe(wctxt))
			return;

		if (wait_for_lsr(up, UART_LSR_THRE))
			return;

		if (wctxt)
			nbcon_exit_unsafe(wctxt);
	}

Note that Sashiko already reported [0] this in v7, but I did not look
deep enough at that comment, thinking Sashiko was misunderstanding nbcon
ownership. Instead it was me who was misunderstanding. :-/

John

[0] https://sashiko.dev/#/patchset/20260720135407.3925-1-john.ogness%40linutronix.de