Re: [PATCH printk v1 10/10] serial: 8250: implement write_atomic

On 03. 08. 21, 16:07, Andy Shevchenko wrote:

On Tue, Aug 03, 2021 at 03:19:01PM +0206, John Ogness wrote:

quoted

Implement an NMI-safe write_atomic() console function in order to
support synchronous console printing.

Since interrupts need to be disabled during transmit, all usage of
the IER register is wrapped with access functions that use the
printk cpulock to synchronize register access while tracking the
state of the interrupts. This is necessary because write_atomic()
can be called from an NMI context that has preempted write_atomic().

...

quoted

+static inline void serial8250_set_IER(struct uart_8250_port *up,
+				      unsigned char ier)
+{
+	struct uart_port *port = &up->port;
+	unsigned long flags;
+	bool is_console;

quoted

+	is_console = uart_console(port);
+
+	if (is_console)
+		console_atomic_cpu_lock(flags);
+
+	serial_out(up, UART_IER, ier);
+
+	if (is_console)
+		console_atomic_cpu_unlock(flags);

I would rewrite it as

	if (uart_console()) {
		console_atomic_cpu_lock(flags);
		serial_out(up, UART_IER, ier);
		console_atomic_cpu_unlock(flags);
	} else {
		serial_out(up, UART_IER, ier);
	}

No additional variable, easier to get the algorithm on the first glance, less
error prone.

Yes, the original is terrible.

Another option:

bool locked = console_atomic_cpu_lock(flags, uart_console());
serial_out(up, UART_IER, ier);
console_atomic_cpu_unlock(flags, locked);


Which makes console_atomic_cpu_lock to lock only if second parameter is 
true and return its value too.

BTW I actually don't know what console_atomic_cpu_lock does to think 
about it more as I was not CCed, and neither lore sees the other patches:
https://lore.kernel.org/linux-mips/20210803131301.5588-1-john.ogness@linutronix.de/ (local)

thanks,
-- 
js
suse labs

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel