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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Ztm85Y_mo5-OJveq@smile.fi.intel.com>
Date: Thu, 5 Sep 2024 17:15:01 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: John Ogness <john.ogness@...utronix.de>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Jiri Slaby <jirislaby@...nel.org>, Petr Mladek <pmladek@...e.com>,
	Sergey Senozhatsky <senozhatsky@...omium.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	Thomas Gleixner <tglx@...utronix.de>, linux-serial@...r.kernel.org,
	linux-kernel@...r.kernel.org, Tony Lindgren <tony@...mide.com>,
	"Paul E. McKenney" <paulmck@...nel.org>,
	Uwe Kleine-König <u.kleine-koenig@...gutronix.de>,
	Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
	Serge Semin <fancer.lancer@...il.com>,
	Rengarajan S <rengarajan.s@...rochip.com>,
	Wolfram Sang <wsa+renesas@...g-engineering.com>
Subject: Re: [PATCH next v1 1/2] serial: 8250: Switch to nbcon console

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