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

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().

...

+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;

+	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.

+}

+static inline unsigned char serial8250_clear_IER(struct uart_8250_port *up)
+{
+	struct uart_port *port = &up->port;
+	unsigned int clearval = 0;
+	unsigned long flags;
+	unsigned int prior;
+	bool is_console;
+
+	is_console = uart_console(port);
+
+	if (up->capabilities & UART_CAP_UUE)
+		clearval = UART_IER_UUE;
+
+	if (is_console)
+		console_atomic_cpu_lock(flags);
+
+	prior = serial_port_in(port, UART_IER);
+	serial_port_out(port, UART_IER, clearval);
+
+	if (is_console)
+		console_atomic_cpu_unlock(flags);

Ditto.

+	return prior;
+}

...

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

I'm wondering why you can't call above function here?

...

+		is_console = uart_console(p);
+		if (is_console)
+			console_atomic_cpu_lock(flags);
 		ier = p->serial_in(p, UART_IER);
+		if (is_console)
+			console_atomic_cpu_unlock(flags);

Ditto.

...

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

Ditto.

...

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

Ditto.

Looking into above note, that uart_console(port) can give different results
here, AFAIR.

-- 
With Best Regards,
Andy Shevchenko