Re: [PATCH next v1 1/2] serial: 8250: Switch to nbcon console

[Petr Mladek <pmladek@suse.com>]

On Thu 2024-09-05 15:53:18, 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.
> 
> The legacy code is kept in order to easily switch back to legacy mode
> by defining USE_SERIAL_8250_LEGACY_CONSOLE.
> 
> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -388,6 +388,7 @@ void __init serial8250_register_ports(struct uart_driver *drv, struct device *de
>  
>  #ifdef CONFIG_SERIAL_8250_CONSOLE
>  
> +#ifdef USE_SERIAL_8250_LEGACY_CONSOLE

Just for record. I agree that it is better to simply remove the
obsolete legacy code.

Or maybe, we would need to keep it for the rs485 consoles, see below.

>  static void univ8250_console_write(struct console *co, const char *s,
>  				   unsigned int count)
>  {
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -546,6 +546,13 @@ static int serial8250_em485_init(struct uart_8250_port *p)
>  	if (!p->em485)
>  		return -ENOMEM;
>  
> +#ifndef USE_SERIAL_8250_LEGACY_CONSOLE
> +	if (uart_console(&p->port)) {
> +		dev_warn(p->port.dev, "no atomic printing for rs485 consoles\n");
> +		p->port.cons->write_atomic = NULL;
> +	}

Wait! This makes the rs485 consoles much less usable for debugging.
They might have troubles to see the emergency and panic messages.
Or do I miss anything, please?

Is this acceptable? Why?
Why is this limitation exactly needed?

Is is because the following code is not safe enough for the write_atomic
variant when it is guarded only by the nbcon context ownership?

void serial8250_console_write_thread(struct uart_8250_port *up,
				     struct nbcon_write_context *wctxt)
{
[...]
	if (em485) {
		if (em485->tx_stopped)
			up->rs485_start_tx(up);
		mdelay(port->rs485.delay_rts_before_send);
	}
[...]
	if (em485) {
		mdelay(port->rs485.delay_rts_after_send);
		if (em485->tx_stopped)
			up->rs485_stop_tx(up);
	}
[...]

Would it break even the nbcon console context it taken over the safe
way? Or only by "unsafe" takeover?

IMHO, we should risk the "unsafe" takeover. We still would be
in a better situation than the legacy code which ignores
the port->lock during panic() all the time (after bust_

> +#endif
> +
>  	hrtimer_init(&p->em485->stop_tx_timer, CLOCK_MONOTONIC,
>  		     HRTIMER_MODE_REL);
>  	hrtimer_init(&p->em485->start_tx_timer, CLOCK_MONOTONIC,
> @@ -3269,6 +3285,11 @@ static void serial8250_console_putchar(struct uart_port *port, unsigned char ch)
>  
>  	wait_for_xmitr(up, UART_LSR_THRE);
>  	serial_port_out(port, UART_TX, ch);
> +
> +	if (ch == '\n')
> +		up->console_newline_needed = false;
> +	else
> +		up->console_newline_needed = true;

I might be just dumb but this code confused me. I missed that the
variable was actually set after printing the character. I inverted
the logic in my head and it did not make sense.

I vote for adding a comment. Or better make the code more
straightforward by renaming the variable and inverting the logic:

	if (ch == '\n')
		up->console_line_ended = true;
	else
		up->console_line_ended = false;

>  }
>  
>  /*
> @@ -3421,6 +3443,125 @@ void serial8250_console_write(struct uart_8250_port *up, const char *s,
>  	if (locked)
>  		uart_port_unlock_irqrestore(port, flags);
>  }
> +#else
> +void serial8250_console_write_thread(struct uart_8250_port *up,
> +				     struct nbcon_write_context *wctxt)
> +{
> +	struct uart_8250_em485 *em485 = up->em485;
> +	struct uart_port *port = &up->port;
> +	unsigned int ier;
> +
> +	touch_nmi_watchdog();

This should not be needed in the write_thread() variant because
it allows to schedule after emitting one record.

> +	if (!nbcon_enter_unsafe(wctxt))
> +		return;
> +

The rest looks good.

Best Regards,
Petr