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

[Andy Shevchenko]

On Thu, Sep 05, 2024 at 03:53:18PM +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.
> 
> The legacy code is kept in order to easily switch back to legacy mode
> by defining USE_SERIAL_8250_LEGACY_CONSOLE.

...

>  static struct console univ8250_console = {
>  	.name		= "ttyS",
> +#ifdef USE_SERIAL_8250_LEGACY_CONSOLE

Can it be done at run-time (theoretically or even practically)?
(Note that we have already knob to disable / enable consoles.)

>  	.write		= univ8250_console_write,
> +	.flags		= CON_PRINTBUFFER | CON_ANYTIME,
> +#else
> +	.write_atomic	= univ8250_console_write_atomic,
> +	.write_thread	= univ8250_console_write_thread,
> +	.device_lock	= univ8250_console_device_lock,
> +	.device_unlock	= univ8250_console_device_unlock,
> +	.flags		= CON_PRINTBUFFER | CON_ANYTIME | CON_NBCON,
> +#endif
>  	.device		= uart_console_device,
>  	.setup		= univ8250_console_setup,
>  	.exit		= univ8250_console_exit,
>  	.match		= univ8250_console_match,
> -	.flags		= CON_PRINTBUFFER | CON_ANYTIME,
>  	.index		= -1,
>  	.data		= &serial8250_reg,
>  };

I would arrange this slightly differently, but not a big deal.

static struct console univ8250_console = {
	.name		= "ttyS",
	.device		= uart_console_device,
#ifndef USE_SERIAL_8250_LEGACY_CONSOLE
	.flags		= CON_PRINTBUFFER | CON_ANYTIME | CON_NBCON,
	.write_atomic	= univ8250_console_write_atomic,
	.write_thread	= univ8250_console_write_thread,
	.device_lock	= univ8250_console_device_lock,
	.device_unlock	= univ8250_console_device_unlock,
#else
	.flags		= CON_PRINTBUFFER | CON_ANYTIME,
	.write		= univ8250_console_write,
#endif
	.setup		= univ8250_console_setup,
	.exit		= univ8250_console_exit,
	.match		= univ8250_console_match,
	.index		= -1,
	.data		= &serial8250_reg,
};

...

> +	if (nbcon_exit_unsafe(wctxt)) {
> +		int len = READ_ONCE(wctxt->len);

> +		int i;

unsigned ?

> +		/*
> +		 * Write out the message. Toggle unsafe for each byte in order
> +		 * to give another (higher priority) context the opportunity
> +		 * for a friendly takeover. If such a takeover occurs, this
> +		 * must abort writing since wctxt->outbuf and wctxt->len are
> +		 * no longer valid.
> +		 */
> +		for (i = 0; i < len; i++) {
> +			if (!nbcon_enter_unsafe(wctxt))
> +				break;
> +
> +			uart_console_write(port, wctxt->outbuf + i, 1, serial8250_console_putchar);
> +
> +			if (!nbcon_exit_unsafe(wctxt))
> +				break;
> +		}
> +	}

...

> +	/* Finally, wait for transmitter to become empty and restore IER. */
> +	wait_for_xmitr(up, UART_LSR_BOTH_EMPTY);
> +	if (em485) {
> +		mdelay(port->rs485.delay_rts_after_send);
> +		if (em485->tx_stopped)
> +			up->rs485_stop_tx(up);
> +	}
> +	serial_port_out(port, UART_IER, ier);
> +
> +	/*
> +	 * The receive handling will happen properly because the receive ready
> +	 * bit will still be set; it is not cleared on read.  However, modem
> +	 * control will not, we must 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);

(1)

...

> +	/* Atomic console not supported for rs485 mode. */

RS485

...

> +	/*
> +	 * First save IER then disable the interrupts. The special variant to
> +	 * clear IER is used because atomic printing may occur without holding
> +	 * the port lock.
> +	 */
> +	ier = serial_port_in(port, UART_IER);
> +	__serial8250_clear_IER(up);
> +
> +	/* Check scratch reg if port powered off during system sleep. */
> +	if (up->canary && (up->canary != serial_port_in(port, UART_SCR))) {
> +		serial8250_console_restore(up);
> +		up->canary = 0;
> +	}
> +
> +	if (up->console_newline_needed)
> +		uart_console_write(port, "\n", 1, serial8250_console_putchar);
> +	uart_console_write(port, wctxt->outbuf, wctxt->len, serial8250_console_putchar);
> +
> +	/* Finally, wait for transmitter to become empty and restore IER. */
> +	wait_for_xmitr(up, UART_LSR_BOTH_EMPTY);
> +	serial_port_out(port, UART_IER, ier);

(2)

Feels like parts (1) and (2) duplicates existing pieces of code. May it be
refactored to minimize the duplication?

-- 
With Best Regards,
Andy Shevchenko