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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Mon, Jul 20, 2026 at 04:00:03PM +0206, John Ogness wrote:
> Implement the necessary callbacks to switch the 8250 console driver
> to perform as an nbcon console.
> 
> Add implementations for the nbcon console callbacks:
> 
>   ->write_atomic()
>   ->write_thread()
>   ->device_lock()
>   ->device_unlock()
> 
> and add CON_NBCON to the initial @flags.
> 
> All hardware access in the callbacks is within unsafe sections.
> The ->write_atomic() and ->write_thread() callbacks allow safe
> handover/takeover per byte and add a preceding newline if they
> take over from another context mid-line.
> 
> For the ->write_atomic() callback, a new irq_work is used to defer
> modem control since it may be called from a context that does not
> allow waking up tasks. During suspend/resume the irq_work is not
> used as this has been shown to cause suspend problems for some
> hardware. Upon resume, any pending modem control is performed.
> 
> Note: A new __serial8250_clear_IER() is introduced for direct
> clearing of UART_IER during console writing (which may not be
> holding the port lock for atomic printing). This allows restoring
> a lockdep check to serial8250_clear_IER() in a follow-up commit.

...

> +	/* irq_work allowed again. Handle MSR now if pending. */
> +	up->avoid_modem_status_work = false;
> +	guard(uart_port_lock_irqsave)(port);

To make guard()() visible, we usually add a blank line before and after
guard()() line. I dunno if scoped_guard() makes more sense here as Petr
proposed, I'm fine with either.

> +	if (uart_console(port) && up->msr_saved_flags)
> +		serial8250_modem_status(up);

...

> +static void serial8250_console_byte_write(struct uart_8250_port *up,
> +					  struct nbcon_write_context *wctxt)
> +{
> +	struct uart_port *port = &up->port;
> +	const char *s = wctxt->outbuf;
> +	const char *end = s + wctxt->len;
> +
> +	/*
> +	 * Write out the message. If a handover or takeover occurs, writing
> +	 * must be aborted since wctxt->outbuf and wctxt->len are no longer
> +	 * valid.
> +	 */
> +	while (s != end) {

Can len == 0? If not, I would write this as do {} while ().

> +		if (!nbcon_enter_unsafe(wctxt))
> +			return;
> +
> +		uart_console_write(port, s++, 1, serial8250_console_wait_putchar);
> +
> +		nbcon_exit_unsafe(wctxt);
> +	}
>  }

...

> +/*
> + * irq_work handler to perform modem control. Only triggered via
> + * ->write_atomic() callback because it may be in a scheduler or
> + * NMI context, unable to wake tasks.
> + */
> +static void modem_status_handler(struct irq_work *iwp)
> +{
> +	struct uart_8250_port *up = container_of(iwp, struct uart_8250_port, modem_status_work);
> +	struct uart_port *port = &up->port;
> +
> +	guard(uart_port_lock)(port);

+ blank line.

> +	serial8250_modem_status(up);
> +}

-- 
With Best Regards,
Andy Shevchenko