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

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

On 2026-07-24, Sashiko wrote:
>> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
>> index c0e8a4efbdcc8..5042a8e8c6fb1 100644
>> --- a/drivers/tty/serial/8250/8250_core.c
>> +++ b/drivers/tty/serial/8250/8250_core.c
>> @@ -584,13 +609,19 @@ void serial8250_suspend_port(int line)
>>  	struct uart_8250_port *up = &serial8250_ports[line];
>>  	struct uart_port *port = &up->port;
>>  
>> -	if (!console_suspend_enabled && uart_console(port) &&
>> -	    port->type != PORT_8250) {
>> -		unsigned char canary = 0xa5;
>> +	if (uart_console(port)) {
>> +		/* No irq_work may be queued when suspending */
>> +		scoped_guard(uart_port_lock_irq, port)
>> +			up->console_msr_work_allow = false;
>> +		irq_work_sync(&up->console_msr_work);
>
> Does clearing up->console_msr_work_allow under uart_port_lock_irq
> properly serialize against serial8250_console_write()?

Yes.

> Since serial8250_console_write() performs atomic writes without taking
> the port lock, could a delayed execution check
> up->console_msr_work_allow locklessly, get preempted or delayed, and
> then serial8250_suspend_port() clear the flag and call irq_work_sync()
> before the delayed thread resumes?

No. uart_port_lock_irq() also acquires nbcon ownership. That is what
synchronizes against serial8250_console_write() when atomic printing.

>> 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.)

>> @@ -3378,11 +3469,25 @@ void serial8250_console_write(struct uart_8250_port *up, const char *s,
>>  	 *	call it if we have saved something in the saved flags
>>  	 *	while processing with interrupts off.
>>  	 */
>> -	if (up->msr_saved_flags)
>> -		serial8250_modem_status(up);
>> +	if (up->msr_saved_flags) {
>> +		if (is_atomic) {
>> +			/*
>> +			 * For atomic, MSR handling must be deferred to
>> +			 * irq_work because this may be a context that does
>> +			 * not permit waking up tasks.
>> +			 *
>> +			 * But no irq_work may be queued when suspending.
>> +			 * In that case, the MSR handling will occur during
>> +			 * resume in serial8250_resume_port().
>> +			 */
>> +			if (up->console_msr_work_allow)
>> +				irq_work_queue(&up->console_msr_work);
>
> This is where the lockless read of up->console_msr_work_allow occurs.
>
> If execution is delayed immediately after checking this flag, could
> the port be suspended and irq_work_sync() finish before
> irq_work_queue() is called here?

No. Both the reader and writer must first aquire exclusive nbcon
ownership. Also, WRT @console_msr_work_allow, both reader and writer are
even in unsafe sections and so cannot lose ownership except for on a
hostile takeover during the final unsafe flush of a panic.

John