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

[Petr Mladek <pmladek@suse.com>]

On Fri 2026-07-24 15:39:49, John Ogness wrote:
> On 2026-07-24, Sashiko wrote:
> >> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> >> index 8c241ec7f4f29..855a7aa48c2d2 100644
> >> --- a/drivers/tty/serial/8250/8250_port.c
> >> +++ b/drivers/tty/serial/8250/8250_port.c
> >> @@ -3198,13 +3218,28 @@ 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 console writing, enter/exit an unsafe section for each byte
> >> +	 * in order to pass the ownership as quickly as possible if a higher
> >> +	 * priority context wants ownership. Otherwise, an attempt to take
> >> +	 * over the ownership might timeout. The new owner will wait for
> >> +	 * UART_LSR_THRE before reusing the fifo.
> >> +	 */
> >>  	for (i = 0; i < count; i++) {
> >> +		if (wctxt && !nbcon_enter_unsafe(wctxt))
> >> +			return;
> >> +
> >>  		if (wait_for_lsr(up, UART_LSR_THRE))
> >>  			return;
> >
> > Does this early return on success bypass the cleanup of the unsafe
> > section?
> >
> > If wait_for_lsr() returns true, wouldn't
> > serial8250_fifo_wait_for_lsr_thre() return immediately without calling
> > nbcon_exit_unsafe(wctxt)?
> >
> > Could this resource leak of the nbcon unsafe state prevent other
> > contexts from taking over ownership?
> 
> It simply has the effect of joining this unsafe section with the next
> one. In this case it really wouldn't matter. But still, it was not my
> intention. I will fix this for v10.
> 
> How I plan for it to look for v10:
> 
> 	for (i = 0; i < count; i++) {
> 		bool tx_ready;
> 
> 		if (wctxt && !nbcon_enter_unsafe(wctxt))
> 			return;
> 
> 		tx_ready = wait_for_lsr(up, UART_LSR_THRE);
> 
> 		if (wctxt)
> 			nbcon_exit_unsafe(wctxt);
> 
> 		if (tx_ready)
> 			break;
> 	}
> 
> I am wondering if we should add a WARN_ON_ONCE() to nbcon_enter_unsafe()
> if the context is already unsafe. It is a simple one-liner. That would
> at least catch these kinds of mistakes during runtime. (I would do that
> in a separate printk patch, not in this series.)

Yup, makes sense.

Best Regards,
Petr