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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 03. 08. 21, 16:07, Andy Shevchenko wrote:
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.

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@xxxxxxxxxxxxx/

thanks,
--
js
suse labs



[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux PPP]     [Linux FS]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Linmodem]     [Device Mapper]     [Linux Kernel for ARM]

  Powered by Linux