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

[John Ogness]

Hi,

Sashiko did not find anything legitimate with this version. The two
issues are incorrect (I explain why below).

If anyone has been waiting for Sashiko to "finish", this is the version
that is ready for human review.

John

On 2026-07-29, Sashiko wrote:
> > diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> > index 8c241ec7f4f29..b9ea4f898474e 100644
> > --- a/drivers/tty/serial/8250/8250_port.c
> > +++ b/drivers/tty/serial/8250/8250_port.c
> > @@ -3286,39 +3339,92 @@ static void serial8250_console_fifo_write(struct uart_8250_port *up,
> [ ... ]
> >  /*
> > - *	Print a string to the serial port trying not to disturb
> > - *	any possible real use of the port...
> > - *
> > - *	The console_lock must be held when we get here.
> > - *
> > - *	Doing runtime PM is really a bad idea for the kernel console.
> > - *	Thus, we assume the function is called when device is powered up.
> > + * Print a string to the serial port trying not to disturb
> > + * any possible real use of the port...
> >   */
> > -void serial8250_console_write(struct uart_8250_port *up, const char *s,
> > -			      unsigned int count)
> > +void serial8250_console_write(struct uart_8250_port *up,
> > +			      struct nbcon_write_context *wctxt,
> > +			      bool is_atomic)
> >  {
> >  	struct uart_8250_em485 *em485 = up->em485;
> >  	struct uart_port *port = &up->port;
> > -	unsigned long flags;
> > -	unsigned int ier, use_fifo;
> > -	int locked = 1;
> > -
> > -	touch_nmi_watchdog();
> > +	unsigned int ier;
> > +	bool use_fifo;
> >  
> > -	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 emergency and panic
> > +	 * printing is synchronized only by nbcon ownership without
> > +	 * holding the port lock.
> >  	 */
> >  	ier = serial_port_in(port, UART_IER);
> > -	serial8250_clear_IER(up);
> > +	__serial8250_clear_IER(up);
>
> Will this race with normal driver paths since the port lock is no longer
> acquired for atomic printing?

No. This is synchronized by nbcon ownership, which normal driver paths
also acquire via the port->lock locking wrappers. It is even within an
unsafe section.

> > @@ -3378,11 +3502,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);
>
> Is there a race condition here between checking the flag and queuing the
> work?

Ditto.

John