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

[Petr Mladek]

On Wed 2026-07-22 16:59:09, John Ogness wrote:
> 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.

Yeah, I missed this as well. I would not expect that a function
called serial_lsr_in() would do a write. But it updates the internal
structure....

> 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);
> 	}

LGTM.

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

I was confused as well.

Best Regards,
Petr