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

On 2026-07-20, Petr Mladek <[email protected]> wrote:
> On Mon 2026-07-20 12:38:35, John Ogness wrote:
>> --- 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;
>
> Do we need to synchronize this against serial8250_console_write()
> where this flag is checked, please?

Yes. @avoid_modem_status_work needs to be modified under the port->lock
to ensure that serial8250_console_write() does not queue any new
irq_work. It is evaluated in atomic context, but taking the port->lock
also acquires nbcon ownership.

I will add this for v8 (in both places where @avoid_modem_status_work is
modified).

> This might even answer the question from Sashiko AI whether
> we should flush the related irq_work() here, see
> https://sashiko.dev/#/patchset/20260720103242.7265-1-john.ogness%40linutronix.de
>
> That said, I am not sure about RT_PREEMPT. AFAIK, it handles IRQs
> in a kthread. In this case, synchronize_srcu() would not make
> sure that the irq_work was procceed.
>
> Note that Sashiko AI suggests that we might need to flush the irq_work
> even in serial8250_console_exit(). I guess that the situation is
> the same there. It is called after synchronize_srcu()...

Yes, I will add irq_work_sync() in both locations.

>> @@ -620,6 +648,12 @@ void serial8250_resume_port(int line)
>>  		port->uartclk = 921600*16;
>>  	}
>>  	uart_resume_port(&serial8250_reg, port);
>> +
>> +	/* irq_work allowed again. Handle MSR now if pending. */
>> +	up->avoid_modem_status_work = false;
>> +	guard(uart_port_lock_irqsave)(port);
>> +	if (uart_console(port) && up->msr_saved_flags)
>> +		serial8250_modem_status(up);
>
> I would use scoped_guard() to make the scope clear. Something like:
>
> 	scoped_guard(uart_port_lock_irqsave, port) {
> 		if (uart_console(port) && up->msr_saved_flags)
> 			serial8250_modem_status(up);
> 	}
>
> Motivation: The guard() is pretty hidden. It can easily get overlooked
> 	when people add more code at the end of this function.

OK.

> Wait, this should not be needed if we make sure that the work
> was flushed in serial8250_suspend_port().

When !console_suspend_enabled, the driver will continue to print in
atomic mode, but will avoid the irq_work. It will however notice if
there are any MSR changes. Those changes are then handled here.

I suppose I should add an "if (!console_suspend_enabled)" to the
conditions.

>> -	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.
>
> I would make the comment more clear when it might happen and if it is
> safe. Something like:
>
> 	 * First, save the IER, then disable the interrupts. The special
> 	 * variant to clear the IER is used because an emergency and panic
> 	 * console printing is synchronized only by nbcon context without
> 	 * holding the port lock.

OK.

>> +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)) {
>
> This should be:
>
> 	if (!nbcon_enter_unsafe(wctxt))
>
> or even better:
>
> 	while (!nbcon_enter_unsafe(wctxt))
> 		nbcon_reacquire_nobuf(wctxt);

Right. I will take the while loop variant.

Thanks for your review! (You can ignore my v7 as you caught everything
that I missed in my v7.)

John