Messages in this thread | | | Subject | Re: [PATCH printk v1 10/10] serial: 8250: implement write_atomic | From | Jiri Slaby <> | Date | Thu, 5 Aug 2021 09:47:12 +0200 |
| |
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@linutronix.de/
thanks, -- js suse labs
| |